Skip to content

/ react

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 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
Move MS Windows build to CircleCI
Loading status checks…
ec5d128
@codesandbox
Copy link

codesandbox bot commented Feb 5, 2020 •
edited

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
@zpao
zpao reviewed Feb 6, 2020
View changes
.circleci/config.yml
# Fix line endings in Windows.
command: git config --global core.autocrlf input
Comment on lines +367 to +368

This comment has been minimized.

Sign in to view
Copy link
@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.

Sign in to view
Copy link
@wittgenst

wittgenst Feb 6, 2020

Author Contributor

This was copied from the AppVeyor config. We can try if it works without.
.circleci/config.yml Outdated
- run:
command: yarn install --frozen-lockfile
Comment on lines 383 to 384

This comment has been minimized.

Sign in to view
Copy link
@zpao

zpao Feb 6, 2020

Member

Same with this *run_yarn
.circleci/config.yml
- 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.

Sign in to view
Copy link
@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.

Sign in to view
Copy link
@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.
.circleci/config.yml Outdated
- v2-win-node-{{ arch }}-{{ .Branch }}-
- v2-win-node-{{ arch }}-
- run:
command: nvm install 10.18.1

This comment has been minimized.

Sign in to view
Copy link
@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.

Sign in to view
Copy link
@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
Update to latest NodeJS
Loading status checks…
a09d7cd
@sizebot
Copy link

sizebot commented Feb 7, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against a09d7cd

@sizebot
Copy link

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
View changes
@bvaughn
Copy link
Contributor

bvaughn commented Mar 4, 2020

I don't really have much knowledge here. I guess this seems okay? 😄
@hramos hramos mentioned this pull request Mar 5, 2020
Closed
@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 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
@acdlite
Revert "Move MS Windows build to CircleCI (#17984)"
Loading status checks…
b7f8ffe 
This reverts commit 885ed46.
acdlite added a commit that referenced this pull request Mar 13, 2020
@acdlite
Revert "Move MS Windows build to CircleCI (#17984)" (#18302)
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
8c015e0 
This reverts commit 885ed46.
@wittgenst
Copy link
Contributor Author

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 commented Mar 13, 2020

What motivated this change in the first place, @wittgenst?
@wittgenst
Copy link
Contributor Author

wittgenst commented Mar 17, 2020

The goal is to remove the dependency on AppVeyor.
@gaearon
Copy link
Member

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
@O330oei
Merge pull request #1 from O330oei/revert-17984-ready
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
5bc91f6 
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
Reviewers

@zpao zpao

@bvaughn bvaughn

@gaearon gaearon

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@wittgenst @sizebot @bvaughn @acdlite @gaearon @zpao @facebook-github-bot
You can’t perform that action at this time.