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

Summary

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

Test Plan

Ran in CircleCI, all workflows succeeded.

@codesandbox-ci
Copy link

codesandbox-ci 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 February 6, 2020 00:31
Comment on lines +367 to +368
# Fix line endings in Windows.
command: git config --global core.autocrlf input
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😑 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with this *run_yarn

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@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 February 12, 2020 22:11
@bvaughn
Copy link
Contributor

bvaughn commented Mar 4, 2020

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

@acdlite
Copy link
Collaborator

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 added a commit that referenced this pull request Mar 13, 2020
@wittgenst
Copy link
Contributor Author

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

The goal is to remove the dependency on AppVeyor.

@gaearon
Copy link
Collaborator

gaearon commented Mar 17, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants