-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Appveyor fixes: fresh yarn, only node6 build #2382
Conversation
@@ -26,6 +26,7 @@ test_script: | |||
- node --version | |||
- npm --version | |||
- yarn --version | |||
- which yarn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Windows has where
which is a native tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which
does work in this case. shrug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works because we have MSYS in PATH
:P
- nodejs_version: "6.9.1" | ||
platform: x86 | ||
- nodejs_version: "7" | ||
- nodejs_version: "6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, you don't want to test node.js 7 anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally i do, but i'm not sure the delay is worth it.
appveyor is significantly slower of a VM than our travis bots, and they don't have a reasonable timeout on inactivity. which means they're very regularly our last CI to finish.
so this evens the playing field a bit, and i don't think missing out on node7/windows is all that bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, OK, that's reasonable. Maybe have a comment for the future in this file?
I'm not affiliated with AppVeyor but I guess you could report any issues to them :)
On a side note, did you compare cold phase? Because caching isn't enabled if the build fails.
@paulirish: instead of us updating yarn, how about I ping AppVeyor to update to the latest yarn. :) About the rest of your comments, Travis does have it. matrix:
fast_finish: true So you better bring .travis.yml and .appveyor.yml on par especially for the node.js versions. |
awesome thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok to me. I haven't tested it but not that much changed.
I guess we're not waiting for an update from appveyor? If not we should create an extra bug to remove the npm install -g yarn
afterwards
We were seeing this error in the appveyor logs:
It seemed to point at this yarn bug: yarnpkg/yarn#2591
Appveyor has an older build of Yarn on the bots by default (0.21.3), so i've installed a fresh version of yarn (>= 0.24.5) in the build script. It sorts it out.
cc @XhmikosR
As a driveby: enabled
fast_finish
which means "immediately finish build once one of the jobs fails." We wanted this for travis too, but they don't have it. odd.Update
Two more drivebys: