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: move to npm ci where possible #21802

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 13, 2018

Recent events (involving a maliciously published version of a popular
module's dependency) have reinvigorated my interest in seeing us move to
npm ci instead of npm install. This moves us to npm ci where
possible in Makefile and vcbuild.bat.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Recent events (involving a maliciously published version of a popular
module's dependency) have reinvigorated my interest in seeing us move to
`npm ci` instead of `npm install`. This moves us to `npm ci` where
possible in Makefile and vcbuild.bat.
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Jul 13, 2018
@Trott
Copy link
Member Author

Trott commented Jul 13, 2018

@nodejs/build-files

@Trott Trott added build Issues and PRs related to build files or the CI. and removed build Issues and PRs related to build files or the CI. labels Jul 13, 2018
@MylesBorins
Copy link
Contributor

Some what of a dupe of #21538

I discovered that npm ci will always nuke and re-install a directory, whereas npm install will no-op if the directory exists. The way we currently handle npm install this would likely increase the time is takes to do certain operations.

Thoughs?

@Trott
Copy link
Member Author

Trott commented Jul 13, 2018

Some what of a dupe of #21538

I discovered that npm ci will always nuke and re-install a directory, whereas npm install will no-op if the directory exists. The way we currently handle npm install this would likely increase the time is takes to do certain operations.

Thoughs?

@MylesBorins Not sure the increase is all that significant, to be honest. If you really want to avoid it, we can add a check for the existence of the relevant node_modules directory and only npm ci if it does not exist. That will also require adding rm -rf node_modules to the relevant *-clean tasks.

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: does npm ci install devDependencies? We used to have --production which doesn't install those.

@Trott
Copy link
Member Author

Trott commented Jul 15, 2018

Question: does npm ci install devDependencies?

@TimothyGu Yes, it does, although you can specify --production to omit them like you do with npm install.

We used to have --production which doesn't install those.

The only place that has any effect is in tools/doc as the other package.json files do not have devDependencies. I have mixed feelings about this because they're all dev dependencies. We're stashing stuff that is required for the source-tarball only in devDependencies which feels...not quite right to me.

This PR currently does not apply to that package.json, but when we add it, we'll have to make a decision to either add --production or else fix up the package.json somehow so that there isn't this use of devDependencies that seems not quite right (to me, at least).

@Trott
Copy link
Member Author

Trott commented Jul 16, 2018

Checklist of stuff to do after this lands:

  1. Separate out devDependencies in tools/doc/package.json to its own directory/package.json. (The devDependencies are actually used for "stuff that builds the apiAssets".) That will require modifying Makefile too (and maybe vcbuild.bat, haven't looked yet to see if it includes the apiAssets building stuff.)
  2. Move the rest of Makefile/vcbuild.bat to npm ci.
  3. Profit!

@Trott
Copy link
Member Author

Trott commented Jul 16, 2018

@Trott
Copy link
Member Author

Trott commented Jul 17, 2018

@Trott
Copy link
Member Author

Trott commented Jul 18, 2018

This is landable, but I'd like a second review. @nodejs/npm @nodejs/build-files

Even though (as Myles pointed out) npm ci removes an existing node_modules every time, it doesn't seem to be an issue with the two tasks in this PR because there is some other mechanism letting make know that the task does not need to be run if it has been run before:

$ make lint-md-clean
rm -f -r tools/remark-cli/node_modules
rm -f -r tools/remark-preset-lint-node/node_modules
rm -f tools/.*mdlintstamp
$ make lint-md-build
Markdown linter: installing remark-cli into tools/
added 160 packages in 1.704s
Markdown linter: installing remark-preset-lint-node into tools/
added 53 packages in 1.089s
$ make lint-md-build
make: Nothing to be done for `lint-md-build'.
$ 

@richardlau
Copy link
Member

Even though (as Myles pointed out) npm ci removes an existing node_modules every time, it doesn't seem to be an issue with the two tasks in this PR because there is some other mechanism letting make know that the task does not need to be run if it has been run before

That's because for the two tasks (which in this case are directories) they are only run if the relevant package.json prerequisite is missing or newer than the target (i.e. directory):

node/Makefile

Line 1062 in b75bde3

tools/remark-cli/node_modules: tools/remark-cli/package.json

node/Makefile

Lines 1066 to 1067 in b75bde3

tools/remark-preset-lint-node/node_modules: \
tools/remark-preset-lint-node/package.json

Unfortunately there's no equivalent mechanism in vcbuild.bat at the moment.

@Trott
Copy link
Member Author

Trott commented Jul 18, 2018

Unfortunately there's no equivalent mechanism in vcbuild.bat at the moment.

@richardlau I wonder if that means we should back out the changes in this to vcbuild.bat and allow it to continue to use npm install there (so that multiple runs don't install over and over again).

@richardlau
Copy link
Member

Unfortunately there's no equivalent mechanism in vcbuild.bat at the moment.

@richardlau I wonder if that means we should back out the changes in this to vcbuild.bat and allow it to continue to use npm install there (so that multiple runs don't install over and over again).

@Trott AFAIK neither the makefile nor vcbuild.bat runs lint-md-build by default, which means the user has to opt in to running the tasks and I don't envisage a multiple install run scenario.

If I'm understanding correctly the motivation for using npm ci is that fixed levels of modules are installed, rather than the latest? In which case I'd prefer we are consistent (npm ci vs npm install) for the equivalent tasks in the makefile and vcbuild.bat and am happy with this PR as-is.

cc @nodejs/platform-windows

Trott added a commit to Trott/io.js that referenced this pull request Jul 19, 2018
Recent events (involving a maliciously published version of a popular
module's dependency) have reinvigorated my interest in seeing us move to
`npm ci` instead of `npm install`. This moves us to `npm ci` where
possible in Makefile and vcbuild.bat.

PR-URL: nodejs#21802
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Jul 19, 2018

Landed in fe67287

@Trott Trott closed this Jul 19, 2018
targos pushed a commit that referenced this pull request Jul 20, 2018
Recent events (involving a maliciously published version of a popular
module's dependency) have reinvigorated my interest in seeing us move to
`npm ci` instead of `npm install`. This moves us to `npm ci` where
possible in Makefile and vcbuild.bat.

PR-URL: #21802
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Jul 31, 2018
@refack refack mentioned this pull request Aug 19, 2018
2 tasks
refack added a commit to refack/node that referenced this pull request Aug 23, 2018
* remove obsolete `node_modules/js-yaml/package.json` target
* remove `@touch` since `npm ci` is always destructive

PR-URL: nodejs#22399
Refs: nodejs#21802
Refs: nodejs#21490
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Sam Ruby <rubys@intertwingly.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
targos pushed a commit that referenced this pull request Aug 24, 2018
* remove obsolete `node_modules/js-yaml/package.json` target
* remove `@touch` since `npm ci` is always destructive

PR-URL: #22399
Refs: #21802
Refs: #21490
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Sam Ruby <rubys@intertwingly.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
* remove obsolete `node_modules/js-yaml/package.json` target
* remove `@touch` since `npm ci` is always destructive

PR-URL: #22399
Refs: #21802
Refs: #21490
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Sam Ruby <rubys@intertwingly.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@Trott Trott deleted the npm-ci branch January 13, 2022 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants