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

Build Tooling: Reevaluate "Build Artifacts" NPM install script #16157

Closed
aduth opened this issue Jun 13, 2019 · 2 comments · Fixed by #39865
Closed

Build Tooling: Reevaluate "Build Artifacts" NPM install script #16157

aduth opened this issue Jun 13, 2019 · 2 comments · Fixed by #39865
Labels
[Status] In Progress Tracking issues with work in progress [Type] Build Tooling Issues or PRs related to build tooling [Type] Task Issues or PRs that have been broken down into an individual action to take

Comments

@aduth
Copy link
Member

aduth commented Jun 13, 2019

Related: #16166

Previously: #14808, #15710 (comment)

The "Build Artifacts" Travis task is meant to capture whether any local changes would occur as a result of installing dependencies or running documentation tasks. For the former, these are largely to cover necessary updates to package-lock.json which should be committed.

By default, we use npm ci as an alternative to npm install in Travis, as it is intended to be optimized for use in a continuous integration (CI) environment like Travis.

It is also meant to exit with an error status when the package-lock.json file is inaccurate:

If dependencies in the package lock do not match those in package.json, npm ci will exit with an error, instead of updating the package lock.

However, as evidenced in the "Previously" links above, this does not occur.

For this reason, the "Build Artifacts" task was updated to use npm install as of #16166. Ideally, this can be reverted back to npm ci once either upstream bugginess is resolved, or an alternative approach can be determined.

For future consideration, the following set of commands should serve as testing for whether npm ci is working as we desire it should:

git checkout 5cf4e14~1 # The commit prior to 14808
npm ci
echo $?

If working as desired, this should output a non-zero value.

@aduth aduth added [Type] Task Issues or PRs that have been broken down into an individual action to take [Type] Build Tooling Issues or PRs related to build tooling labels Jun 13, 2019
@aduth
Copy link
Member Author

aduth commented Mar 16, 2020

Not exactly related to the original comment, but capturing for posterity's sake: The npm run check-local-changes ("Artifacts" task) script can yield different results between operating systems. Notably, I have seen quite a few changes in package-lock.json which stem from optional dependencies having a "resolved": false value.

Related:

Specifically, I often see local git changes in my environment (macOS) when running npm install. Ideally, these would never be allowed to be introduced in the first place, but because Travis skips these optional dependencies when running npm install, the package-lock.json is not updated, and thus npm run check-local-changes reports no errors.

Example build:

https://travis-ci.com/github/WordPress/gutenberg/jobs/298545635

$ npm install
...
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.2.9 (node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.2.9: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})
...
> ( git diff -U0 | xargs -0 node bin/process-git-diff ) || ( echo "There are local uncommitted changes after one or both of 'npm install' or 'npm run docs:build'!" && exit 1 );
The command "npm run check-local-changes" exited with 0.

@aduth
Copy link
Member Author

aduth commented Mar 19, 2020

Related to: #16157 (comment)

@ItsJonQ had mentioned seeing changes from a valid "resolved" to "resolved": false in his environment, which appeared to be cleared up when upgrading from npm@6.13.7 to npm@6.14.2. It may very well be an issue of NPM which has since been fixed. In the conversation at #20026 (comment), @gziolo mentioned having used npm@6.14.1 when seeing changes from a valid "resolved" to "resolved": false, which is (at the time of writing) one patch version old. It's not clear that the issues were cleared up if upgrading to the latest npm@6.14.2, and the associated CHANGELOG doesn't make an explicit mention of this issue, but it could have been fixed in this version.

Edit (2020-03-31): Another instance was encountered. @jorgefilipecosta was using 6.10.1 at the time.

Edit (2020-03-31): See #21306 for an attempt to help resolve instances of these issues.

Edit (2020-04-03): Another case at #20555 (comment) . @swissspidy may have been using NPM 6.14.4 at the time (the latest version!), but noted to have recalled updating to that latest version in the very recent history (the changes may have been made on an older version).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress [Type] Build Tooling Issues or PRs related to build tooling [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
1 participant