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

Teach CI to use versions specified in package-lock.json #19176

Merged
merged 6 commits into from Apr 19, 2019

Conversation

Projects
None yet
2 participants
@jasonrudolph
Copy link
Member

commented Apr 17, 2019

Closes #19148

This pull request does the following:

  • Ensure that our CI builds use npm 6+ to install dependencies. There are two parts to this:
    • Make sure that npm 6+ is installed
    • When installing dependencies, make sure that npm 6+ is used (instead of using an older version of npm that might be installed)
  • Ensure that npm ci is used to install dependencies (instead of npm install). Our pre-existing build scripts already run npm ci when the CI environment variable is set to true. So, this pull request ensures that our CI builds set this environment variable.

TODO

Verify that npm ci is being used in:

jasonrudolph added some commits Apr 17, 2019

Require npm 6+ on CI
This change will allow us to use `npm ci` instead of `npm install` on 
CI, so that we can respect the package-lock.json file.
@jasonrudolph

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

Hmm. For some reason, Azure DevOps is apparently using npm 5, despite the fact that we have an explicit step that installs npm 6 globally. For example, here's a snippet of the macOS build config:

steps:
- task: NodeTool@0
inputs:
versionSpec: 8.9.3
displayName: Install Node.js 8.9.3
- script: npm install --global npm@6.2.0
displayName: Update npm
- script: script/bootstrap
displayName: Bootstrap build environment

Notice that we install npm 6.2.0 on line 19. However, when the script/bootstrap step runs on line 22, the build fails because it's using npm 5.5.1:

Screen Shot 2019-04-17 at 2 34 32 PM

I'll see if I can figure out why the build isn't using npm 6.2.0 in the script/bootstrap step.

@jasonrudolph

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

For some reason, Azure DevOps is apparently using npm 5, despite the fact that we have an explicit step that installs npm 6 globally. -- #19176 (comment)

👋 @smashwilson: I see that #17803 added the step that installs npm 6.2.0 globally. It's admittedly been a while since that pull request shipped, but do you happen to recall any potential reasons why some steps of the build wouldn't be using the global npm version that we're installing?

Set PATH so that Azure DevOps macOS builds use correct npm
Before we run script/bootstrap on the macOS build, the preceding build
step installs npm 6.2.0 as the global npm version. It installs npm at
`/usr/local/bin/npm`. However, a _different_ version of npm appears
earlier in the PATH. The PATH looks like this:

```
/Users/vsts/hostedtoolcache/node/8.9.3/x64/bin:
/usr/local/lib/ruby/gems/2.6.0/bin:
/usr/local/opt/ruby/bin:
/usr/local/opt/curl/bin:
/usr/local/bin:
/usr/local/sbin:
/Users/vsts/bin:
/Users/vsts/.yarn/bin:
/usr/local/go/bin:
/Users/vsts/Library/Android/sdk/tools:
/Users/vsts/Library/Android/sdk/platform-tools:
/Users/vsts/Library/Android/sdk/ndk-bundle:
/Library/Frameworks/Mono.framework/Versions/Current/Commands:
/usr/bin:
/bin:
/usr/sbin:
/sbin:
/Users/vsts/.azcopy
```

There's an npm executable at
/Users/vsts/hostedtoolcache/node/8.9.3/x64/bin/npm.

To get the build to use the global version of npm, this commit puts
/usr/local/bin at the beginning of the PATH.
@smashwilson

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Oh goodness, I remember running into this now. No, I never did figure out why it wasn't being picked up. It looks like you've made some progress tracking it down to PATH issues?

@jasonrudolph

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

It looks like you've made some progress tracking it down to PATH issues?

Yeah. I'm getting there. 😅On macOS, the first entry in the PATH is /Users/vsts/hostedtoolcache/node/8.9.3/x64/bin, and it contains npm 5.5.1. When we install npm 6.2.0 globally, that npm executable lives in /usr/local/bin, but /usr/local/bin appears later in the PATH. (See 680e484.) So, I've gotta make some adjustments in order to get the right version of npm used in the right places.

jasonrudolph added some commits Apr 18, 2019

@@ -27,6 +27,9 @@ jobs:
- script: |
ECHO Installing npm-windows-upgrade
npm install --global --production npm-windows-upgrade
displayName: Install npm-windows-upgrade
- script: |

This comment has been minimized.

Copy link
@jasonrudolph

jasonrudolph Apr 18, 2019

Author Member

I had to split this step into two separate steps. Prior to this change, the first command was getting executed, but the second command was not getting executed (for some reason).

Before (One step)

https://github.visualstudio.com/Atom/_build/results?buildId=38113&view=logs&jobId=97a617bf-bcbd-5dfa-bba2-cfba2747b693&taskId=ff3b67c9-4d3a-551a-4d02-46b3dfea0bbc&lineStart=12&lineEnd=16&colStart=1&colEnd=40

Screen Shot 2019-04-18 at 1 03 36 PM

After (Two steps)

Screen Shot 2019-04-18 at 1 11 19 PM

Screen Shot 2019-04-18 at 1 05 34 PM


/fyi @smashwilson

@jasonrudolph jasonrudolph marked this pull request as ready for review Apr 18, 2019

@jasonrudolph

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

I pushed up a temporary branch that includes these changes plus an intentional problem where package-lock.json is out of sync with package.json. I then ran the CI builds for that branch to verify that CI will fail. Indeed it does:

@jasonrudolph jasonrudolph requested a review from smashwilson Apr 18, 2019

@jasonrudolph

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

@smashwilson: Given that you've done some work with our npm version and using npm ci in the past, would you mind reviewing these changes when you have a moment?

@jasonrudolph

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

I'm gonna go ahead and merge this pull request, but I still welcome any feedback. If you have concerns about these changes, please leave a comment, and I'll be sure to follow up.

@jasonrudolph jasonrudolph merged commit f3f32c4 into master Apr 19, 2019

2 checks passed

Atom Pull Requests #20190418.7 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@jasonrudolph jasonrudolph deleted the respect-package-lock-on-ci branch Apr 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.