Use npm and apm ci in CI builds

[smashwilson]

I've added a --ci argument to script/build. When specified, invocations of npm and apm use the "ci" command rather than "install".

• npm ci documentation
• "Introducing npm ci" on the npm blog

I doubt this will substantially improve our build times; we're shaving time off of a step that isn't taking the bulk of our build time. The most notably benefit will be reliability: by installing the package versions pinned in the lockfile exactly, patch releases of transitive dependencies should no longer implicitly creep in and break our build.

[smashwilson]

So, an open discussion related to this. Lockfiles are annoying to maintain. Any time any package in our dependency tree releases a compatible update, every run of script/build will update the lockfile to match. As a result you tend to have your commit history littered with git commit -m ':lock:' commits.

We have a few options here:

1. Leave it the way it is. We can accept the commit noise and work with it the way it is now. Dependency tree changes will remain explicit rather than implicit and CI builds will use the --ci argument to gain reliability. This is consistent with the way that most npm packages expect to work, so it would be comfortable and expected to contributors with a Node background.

2. Default to --ci. Require an explicit --no-ci argument to script/build to disable the ci commands. package.json and package-lock.json will be untouched by the majority of builds. Compatible dependency updates in our dependency tree will only be received when we explicitly request them by running script/build --no-ci.

   As an enhancement, we could add a nightly build that runs script/build --no-ci for us, commits any changes to a new branch, and opens a pull request for us. That would let us see and explicitly accept transitive changes without us having to remember to run it ourselves periodically or polluting unrelated PRs. The downside here is that we'll get a lot of these bump PRs.

/cc @asheren @daviwil @queerviolet @jasonrudolph @maxbrunsfeld @as-cii @annthurium for opinions and thoughts 💭 while I work out the kinks in the build scripts 😄

[daviwil]

Aside from the possible use of process.env.CI, it looks great!

[daviwil]

Lockfiles are definitely a pain, I've been scratching my head every day recently and worrying about whether I'm going to break everything by committing changes. No harm in trying it for a while though until we get used to it and understand what to expect.

I had also thought about the auto-PR of new package-lock.json files but I think it'll be harder for us to actually keep up with those since they'll likely be coming every day. I think the only viable solution is to just start making lockfile change commits an obvious and expected part of our workflow so that we're more aware of when it's happening. But that also means that we can't just bump versions in package.json anymore, we'll have to run npm/apm install before committing them so that package-lock.json gets updated.

My biggest problem with the lockfiles changing is that I'm not sure I trust whether it's actually due to dependencies changing or npm doing funny business. More than once I've seen npm switch the package-lock.json dependencies back and forth between explicit versions and version ranges for seemingly no good reason.

Regardless of any solution, I think we'll have to be vigilant at first and make sure to really scrutinize the changes so that we don't end up with unexpected problems in release builds.

[smashwilson]

But that also means that we can't just bump versions in package.json anymore, we'll have to run npm/apm install before committing them so that package-lock.json gets updated.

Yeah. Would it be valuable to have an alternative script that runs npm install --package-lock-only to update the lockfiles without doing a full build? Or would we want to make sure we do a full script/build before committing dependency changes?

My biggest problem with the lockfiles changing is that I'm not sure I trust whether it's actually due to dependencies changing or npm doing funny business. More than once I've seen npm switch the package-lock.json dependencies back and forth between explicit versions and version ranges for seemingly no good reason.

True... I've seen that too. For a while there were problems with platform-specific optional dependencies flapping in and out of the lockfile and some ordering inconsistencies. In general I think npm is trying to make lockfile generation deterministic but there are definitely some cases where it isn't quite.

I think the ideal path forward here would be to increase our faith in our CI catching the kinds of breakage that we want to avoid, so that a green check gives us a strong assurance that an update is acceptable (and a red X gives us a signal that we need to tighten a dependency range or send a PR upstream).

Specifically, the greatest source of uncaught breakage that I've seen is when something breaks electron-link or snapshotting. Our transitive dependencies don't test or consider these because neither are common, so it's easy for a patch-version change upstream to break our build. React 16.4.2 did this: a change in the way their source was minimized breaks electron-link! A while ago we had a patch version of Relay that broke, too (but that would have been caught by our regular test suite, I believe - this was before we had any test coverage for our Relay code).

Aside from the possible use of process.env.CI, it looks great!

Ah, interesting. Can do 👍

[smashwilson]

Okay, this is weird. After running npm ci --global-style in apm, there are a bunch of unmet dependencies:

smashwilson @ hubtop ~/src/atom/atom/apm (aw/build-ci>) [1]
$ npm install --global-style
npm WARN atom-bundled-apm@ No license field.

added 339 packages from 160 contributors, removed 238 packages, updated 1 package and audited 6044 packages in 8.27s
found 21 vulnerabilities (20 moderate, 1 critical)
  run `npm audit fix` to fix them, or `npm audit` for details
smashwilson @ hubtop ~/src/atom/atom/apm (aw/build-ci>)
$ npm ls pacote
atom-bundled-apm@ /Users/smashwilson/src/atom/atom/apm
└─┬ atom-package-manager@2.1.0
  └─┬ npm@6.2.0
    ├─┬ libcipm@2.0.0
    │ └── pacote@8.1.6  deduped
    └── pacote@8.1.6
$ npm ci --global-style
# ...
$ npm ls pacote
smashwilson @ hubtop ~/src/atom/atom/apm (aw/build-ci>)
$ npm ls pacote
atom-bundled-apm@ /Users/smashwilson/src/atom/atom/apm
└─┬ atom-package-manager@2.1.0
  └─┬ npm@6.2.0
    ├─┬ UNMET DEPENDENCY libcipm@2.0.0
    │ └── UNMET DEPENDENCY pacote@8.1.6
    └── UNMET DEPENDENCY pacote@8.1.6

npm ERR! missing: libcipm@2.0.0, required by npm@6.2.0
npm ERR! missing: pacote@8.1.6, required by npm@6.2.0
npm ERR! missing: pacote@8.1.6, required by libcipm@2.0.0

My current guess is that apm's postinstall script is borking up npm:

echo ">> Downloading bundled Node"
node script/download-node.js


echo
echo ">> Rebuilding apm dependencies with bundled Node $(./bin/node -p "process.version + ' ' + process.arch")"
./bin/npm rebuild


echo
echo ">> Deduping apm dependencies"
./bin/npm dedupe

[daviwil]

Would it be valuable to have an alternative script that runs npm install --package-lock-only to update the lockfiles without doing a full build? Or would we want to make sure we do a full script/build before committing dependency changes?

One of those could be useful, and it might be even better if we can include a pre-commit hook check to see if package.json is newer than package-lock.json (or if package-lock.json is modified) and prompt the user to run npm install --package-lock-only before committing (or do it for them?)

I think the ideal path forward here would be to increase our faith in our CI catching the kinds of breakage that we want to avoid, so that a green check gives us a strong assurance that an update is acceptable (and a red X gives us a signal that we need to tighten a dependency range or send a PR upstream).

✔️

Regarding the apm dedupe issue, is it possible that the package-lock.json files are once again out of sync and causing the dependencies to not be installed?

[smashwilson]

I couldn't figure out why apm's install was breaking, so I'm just... always using npm install to install apm 😅

I was able to get a successful build locally with this, so let's see if CI is happy 🤞

[smashwilson]

One of those could be useful, and it might be even better if we can include a pre-commit hook check to see if package.json is newer than package-lock.json (or if package-lock.json is modified) and prompt the user to run npm install --package-lock-only before committing (or do it for them?)

Oh, neat, I like those ideas. git doesn't make it easy to distribute hooks, though. 🤔

Regarding the apm dedupe issue, is it possible that the package-lock.json files are once again out of sync and causing the dependencies to not be installed?

No, it shouldn't be... npm ci refuses to run if they aren't in sync.

Basically, the apm package includes a postinstall script that calls npm dedupe, presumably to deal with our path-too-long issues. atom/atom's build scripts install apm with the --global-style flag set so that apm's dependencies will be laid out beneath apm/node_modules/atom-package-manager/ and other scripts will work. Somehow, the combination of these two causes npm ci to omit certain apm dependencies, or install them to unexpected places, so that require calls would break at runtime. I couldn't figure out exactly what caused it, though.

[daviwil]

CI is 🍏, looks good to go!