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

Travis CI currently broken #893

Closed
nfischer opened this issue Oct 10, 2018 · 9 comments
Closed

Travis CI currently broken #893

nfischer opened this issue Oct 10, 2018 · 9 comments

Comments

@nfischer
Copy link
Member

@nfischer nfischer commented Oct 10, 2018

Due to a npm change, npm install modifies package-lock.json, which turns bots red (we check for modified files at the end of the script).

Blocking #892, #889, and any other PRs.

nfischer added a commit that referenced this issue Oct 10, 2018
CI was broken because the npm which ships with node v8 (and only that
version of npm, for some reason), would modify package-lock.json when we
ran `npm install`. This triggered our post-test hook which enforces docs
changes: the modified file was detected as a false-positive for
uncommitted docs changes.

Fixes #893
nfischer added a commit that referenced this issue Oct 10, 2018
CI was broken because the npm which ships with node v8 (and only that
version of npm, for some reason), would modify package-lock.json when we
ran `npm install`. This triggered our post-test hook which enforces docs
changes: the modified file was detected as a false-positive for
uncommitted docs changes.

This fixes the issue by committing those docs changes. This seems to be
compatible with other npm versions.

Fixes #893
@nfischer
Copy link
Member Author

@nfischer nfischer commented Oct 10, 2018

Regression is likely in npm, within the range 5.6.0 (good build) and 6.4.1 (bad build).

@nfischer
Copy link
Member Author

@nfischer nfischer commented Oct 10, 2018

Turns out, the fix is not as simple as committing the new format.

node version v8.12.0 v9.4.0 v10.4.0
npm version 6.4.1 5.6.0 6.1.0
Accepts master branch? no yes no
Accepts this commit? yes no yes

Might have to remove package-lock.json, or exclude it from this the file-check at the end.

@nfischer
Copy link
Member Author

@nfischer nfischer commented Oct 10, 2018

Or, maybe we can use npm ci instead of npm install on npm v6+

@hutson
Copy link

@hutson hutson commented Oct 23, 2018

Modifications of package-lock.json by npm install is intentional - npm/npm#21043 (comment)

The recommendation that I hear is to always use npm ci in CI builds to respect the package-lock.json as-is.

@nfischer
Copy link
Member Author

@nfischer nfischer commented Oct 24, 2018

@hbetts thanks for linking to the npm ticket. I figured this is probably intentional. The issue is that we need to conditionally run npm install for npm versions which don't know about npm ci.

It would be nice to just install the newest npm and use that to install dependencies, but I think shelljs supports a wider range of node versions than npm.

One way to detect support (since I forget which version introduced it) would be:

$ npm help | egrep '\bci\b'

@nfischer nfischer self-assigned this Oct 24, 2018
@hutson
Copy link

@hutson hutson commented Oct 24, 2018

For what it's worth, npm ci was added in version 5.7.0. I also finally found their Node support policy.

Though, I'm curious, even if you check for the existence of ci, how could the lock file changing issue get resolved on those versions of npm without ci?

@nfischer
Copy link
Member Author

@nfischer nfischer commented Oct 25, 2018

how could the lock file changing issue get resolved on those versions of npm without ci?

Basic algorithm is:

Check npm version
 - if >= 5.7, use `npm ci`
 - else, use `npm install`

In the else-case, I don't expect npm install to change the package-lock. In the if-case, I expect (1) npm ci understands the package-lock, and (2) npm ci understands the package-lock in its format.

I still need to verify these assumptions, however.

nfischer added a commit that referenced this issue Oct 25, 2018
This adds a script which checks the npm version and runs either `npm ci`
or `npm install` based on support. This is primarily to work around an
issue where `npm install` modifies `package-lock.json` for newer npm
versions.

A side benefit is that `npm ci` is slightly faster than `npm install`.

Fixes #893
nfischer added a commit that referenced this issue Oct 27, 2018
* chore(npm): add ci-or-install script

This adds a script which checks the npm version and runs either `npm ci`
or `npm install` based on support. This is primarily to work around an
issue where `npm install` modifies `package-lock.json` for newer npm
versions.

A side benefit is that `npm ci` is slightly faster than `npm install`.

Fixes #893
nfischer added a commit that referenced this issue Nov 29, 2018
This removes our lockfile and our `ci-or-install` script. The lockfile
caused headaches, since `npm install` would modify the lockfile.

Unfortunately, `ci-or-install` (added in #896) didn't work reliably on
appveyor, so removing the lockfile seems easier. We remove the script
since it's now obsolete.

Fixes #893
nfischer added a commit that referenced this issue Nov 29, 2018
This removes our lockfile and our `ci-or-install` script. The lockfile
caused headaches, since `npm install` would modify the lockfile.

Unfortunately, `ci-or-install` (added in #896) didn't work reliably on
appveyor, so removing the lockfile seems easier. We remove the script
since it's now obsolete.

Fixes #893
@nfischer
Copy link
Member Author

@nfischer nfischer commented Nov 29, 2018

Reopening because #896 is flaking on appveyor. Trying a different approach instead.

@nfischer nfischer reopened this Nov 29, 2018
nfischer added a commit to shelljs/shx that referenced this issue Nov 29, 2018
This removes the package-lock, since `npm install` modifies the lockfile
(due to a disagreement between npm@5 and npm@6). We're removing it for
ShellJS, and removing it here for consistency.

See shelljs/shelljs#893
nfischer added a commit to shelljs/shx that referenced this issue Nov 29, 2018
This removes the package-lock, since `npm install` modifies the lockfile
(due to a disagreement between npm@5 and npm@6). We're removing it for
ShellJS, and removing it here for consistency.

See shelljs/shelljs#893
@nfischer
Copy link
Member Author

@nfischer nfischer commented Nov 29, 2018

#911 flakes as well, which makes me suspect ci-or-install works correctly (although is perhaps overly complicated), and it's appveyor which causes the flakiness.

@nfischer nfischer mentioned this issue Nov 29, 2018
4 tasks
nfischer added a commit to shelljs/shx that referenced this issue Nov 29, 2018
This removes the package-lock, since `npm install` modifies the lockfile
(due to a disagreement between npm@5 and npm@6). We're removing it for
ShellJS, and removing it here for consistency.

See shelljs/shelljs#893
nfischer added a commit that referenced this issue Nov 30, 2018
This removes our lockfile and our `ci-or-install` script. The lockfile
caused headaches, since `npm install` would modify the lockfile.

Unfortunately, `ci-or-install` (added in #896) didn't work reliably on
appveyor, so removing the lockfile seems easier. We remove the script
since it's now obsolete.

Fixes #893
nfischer added a commit to shelljs/shx that referenced this issue Nov 30, 2018
This removes the package-lock, since `npm install` modifies the lockfile
(due to a disagreement between npm@5 and npm@6). We're removing it for
ShellJS, and removing it here for consistency.

See shelljs/shelljs#893
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants