The Wayback Machine - https://web.archive.org/web/20210124183752/https://github.com/facebook/react/pull/17984
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move MS Windows build to CircleCI #17984

Merged
merged 2 commits into from Mar 13, 2020
Merged

Move MS Windows build to CircleCI #17984

merged 2 commits into from Mar 13, 2020

Conversation

@wittgenst
Copy link
Contributor

@wittgenst wittgenst commented Feb 5, 2020

Summary

Migrate the MS Windows continuous integration configuration from AppVeyor to CircleCI.

Test Plan

Ran in CircleCI, all workflows succeeded.

Chris Lüer
@codesandbox
Copy link

@codesandbox codesandbox bot commented Feb 5, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a09d7cd:

Sandbox Source
immutable-wind-tx285 Configuration
@wittgenst wittgenst marked this pull request as ready for review Feb 6, 2020
# Fix line endings in Windows.
command: git config --global core.autocrlf input
Comment on lines +367 to +368

This comment has been minimized.

@zpao

zpao Feb 6, 2020
Member

😑 Do we need to do this if we aren't making any commits? Also, doesn't input really only map to *nix use? (https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration#_code_core_autocrlf_code)

This comment has been minimized.

@wittgenst

wittgenst Feb 6, 2020
Author Contributor

This was copied from the AppVeyor config. We can try if it works without.

- run:
command: yarn install --frozen-lockfile
Comment on lines 383 to 384

This comment has been minimized.

@zpao

zpao Feb 6, 2020
Member

Same with this *run_yarn

- restore_cache:
keys:
- v2-win-node-{{ arch }}-{{ .Branch }}-{{ checksum "yarn.lock" }}
- v2-win-node-{{ arch }}-{{ .Branch }}-
- v2-win-node-{{ arch }}-
Comment on lines +370 to +374

This comment has been minimized.

@zpao

zpao Feb 6, 2020
Member

Should this just be - *restore_yarn_cache? Looks like that's what is used elsewhere for this step.

This comment has been minimized.

@wittgenst

wittgenst Feb 6, 2020
Author Contributor

We are using a different cache key for Win and Linux, since the files and folders might not be exactly the same - so we can't use the same restore step.

- v2-win-node-{{ arch }}-{{ .Branch }}-
- v2-win-node-{{ arch }}-
- run:
command: nvm install 10.18.1

This comment has been minimized.

@zpao

zpao Feb 6, 2020
Member

Looks like 12.x is used on Linux, any reason to use 10.x? (it'll be in maintenance soon)

This comment has been minimized.

@wittgenst

wittgenst Feb 6, 2020
Author Contributor

10.x is what the AppVeyor config uses, not sure if we should try upgrading?

Chris Lüer
@sizebot
Copy link

@sizebot sizebot commented Feb 7, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against a09d7cd

@sizebot
Copy link

@sizebot sizebot commented Feb 7, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against a09d7cd

@zpao zpao requested review from bvaughn and gaearon Feb 12, 2020
@zpao
zpao approved these changes Mar 4, 2020
@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Mar 4, 2020

I don't really have much knowledge here. I guess this seems okay? 😄

@wittgenst wittgenst merged commit 885ed46 into facebook:master Mar 13, 2020
23 checks passed
23 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_devtools_and_process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: build_experimental Your tests passed on CircleCI!
Details
ci/circleci: flow Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: lint_build Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts_experimental Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: sizebot Your tests passed on CircleCI!
Details
ci/circleci: sizebot_experimental Your tests passed on CircleCI!
Details
ci/circleci: test_build Your tests passed on CircleCI!
Details
ci/circleci: test_build_experimental Your tests passed on CircleCI!
Details
ci/circleci: test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: test_build_prod_experimental Your tests passed on CircleCI!
Details
ci/circleci: test_devtools Your tests passed on CircleCI!
Details
ci/circleci: test_dom_fixtures Your tests passed on CircleCI!
Details
ci/circleci: test_ms_windows Your tests passed on CircleCI!
Details
ci/circleci: test_source Your tests passed on CircleCI!
Details
ci/circleci: test_source_experimental Your tests passed on CircleCI!
Details
ci/circleci: test_source_persistent Your tests passed on CircleCI!
Details
ci/circleci: test_source_prod Your tests passed on CircleCI!
Details
ci/codesandbox Building packages succeeded.
Details
@wittgenst wittgenst deleted the wittgenst:ready branch Mar 13, 2020
@acdlite
Copy link
Member

@acdlite acdlite commented Mar 13, 2020

I'm reverting this because it slowed our CI times from ~6 minutes to ~23 minutes. That's because it runs the build command without concurrency.

Do we really need to run the tests against build? I don't remember why the Windows job exists.

Let's figure it out then re-land.

acdlite added a commit that referenced this pull request Mar 13, 2020
This reverts commit 885ed46.
acdlite added a commit that referenced this pull request Mar 13, 2020
This reverts commit 885ed46.
@wittgenst
Copy link
Contributor Author

@wittgenst wittgenst commented Mar 13, 2020

Do we really need to run the tests against build? I don't remember why the Windows job exists.

Do you know who would know? Can we add them to this thread?

Seems there are two questions:

  1. Is the Windows job even needed?
  2. Can it be sped up?
@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Mar 13, 2020

What motivated this change in the first place, @wittgenst?

@wittgenst
Copy link
Contributor Author

@wittgenst wittgenst commented Mar 17, 2020

The goal is to remove the dependency on AppVeyor.

@gaearon
Copy link
Member

@gaearon gaearon commented Mar 17, 2020

@wittgenst Let's chat on Workplace? Feel free to hit me up and I can give context.

O330oei added a commit to O330oei/fastct that referenced this pull request Mar 24, 2020
Revert "Move MS Windows build to CircleCI (facebook#17984)"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants