build: move to npm ci where possible

[Trott]

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

[nodejs-github-bot]

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/269/pipeline

[Trott]

@nodejs/build-files

[MylesBorins]

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]

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.

[TimothyGu]

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

[Trott]

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]

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]

CI: https://ci.nodejs.org/job/node-test-pull-request/15903/

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/15918/

[Trott]

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]

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]

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]

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]

Landed in fe67287