tools: use bundled npm in update scripts by ruyadorno · Pull Request #37613 · nodejs/node · GitHub

The scripts ./tools/update-babel-eslint.sh and
./tools/update-eslint.sh are relying on the version of npm found in
the local-defined $PATH env.

This changeset proposes to modify these scripts to run the version of
npm bundled in the current branch (found at ./deps/npm) - in order to:

Standardize the version of npm that should be use to install these
deps, avoids the pitfall of having an inadverted user run these
scripts with an unsupported/incompatible npm version.
Given that npm7 has a different install algorithm than npm6 that
takes into account and install peer dependencies, it might be a safer
choice to ensure what version of npm should be use during this
transitional period in which users might still have npm6 by default in
their local system.
Avoids the possible extra churn of having different resulting files
being shuffled around between installs due to usage of a disparate
version of the npm cli.

cc @lpinca @Trott @richardlau

Perhaps it makes sense to do the same with node?

Perhaps it makes sense to do the same with node?

I think doing the same with node poses extra inconveniences such as a requirement on compiling node before being able to run these scripts and therefore the risk of being blocked on running them if there's a problem trying to compile node locally.

Keep using node from $PATH avoids this category of issues and might be the reason why the script was wrote like that at first 😊 (npm on the other hand doesn't require any compilation)

requirement on compiling node before being able to run these scripts

Ah! I thought this change would already require that. But now that I think about it, I suppose as long as you're running a relatively recent node in your PATH, it should still work.

We could use an env variable so users can choose to use their global npm install:

./tools/update-babel-eslint.sh # uses out/Release/node deps/npm
NPM=npm ./tools/update-babel-eslint.sh # uses npm from path

I think this is a better UX.

EDIT: see my updated suggestion below.

@aduh95 I think using a globally installed npm defeats the whole point of this PR.

@aduh95 I think using a globally installed npm defeats the whole point of this PR.

My suggestion is to use the bundled npm by default, and let user decides if they want the script to use something else (useful only if they want to run the script without building node first).

Yes, but from my understanding the point of this PR is to make sure the script is run with the same npm by everyone for the reasons listed in the description and in #37566 (comment), allowing the use of a custom npm seems a step back to me.

There is no need to build node to run the script.

There is no need to build node to run the script.

Well, npm needs node to run, so you do need to have it built somewhere.

# Depends on node being in $PATH.

I've updated my suggestion based on @lpinca's comments:

./tools/update-babel-eslint.sh # uses node from `out/Release/node` or PATH if not found.
NODE=/path/to/specific/node ./tools/update-babel-eslint.sh # uses specific node

Non-blocking, feel free to disregard if you prefer the current implementation.

thanks @aduh95 @addaleax, I incorporated the suggestions in the commit 😊

The scripts `./tools/update-babel-eslint.sh` and `./tools/update-eslint.sh` are relying on the version of `npm` found in the local-defined `$PATH` env. This changeset proposes to modify these scripts to run the version of npm bundled in the current branch (found at `./deps/npm`) - in order to: a) Standardize the version of npm that should be use to install these deps, avoids the pitfall of having an inadverted user run these scripts with an unsupported/incompatible npm version. b) Given that npm7 has a different install algorithm than npm6 that takes into account and install peer dependencies, it might be a safer choice to ensure what version of npm should be use during this transitional period in which users might still have npm6 by default in their local system. c) Avoids the possible extra churn of having different resulting files being shuffled around between installs due to usage of a disparate version of the npm cli.

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

@ruyadorno FYI this kind of PR does not require a full CI run, green GitHub Actions is sufficient.

node/doc/guides/collaborator-guide.md

Lines 214 to 216 in e6cbec5

    
           A passing (green) GitHub Actions CI result is required. A passing (green or 
        
           yellow) [Jenkins CI](https://ci.nodejs.org/) is also required if the pull 
        
           request contains changes that will affect the `node` binary. This is because

phew 😅 @aduh95 thanks for letting me know! CI has been extremely flaky recently 😞

With that in mind, I'm going to add author ready as soon as there's a green GH action run.

Well, actually, as long as the CI has started a PR can be labeled author ready: you don't even have to wait 😅

node/doc/guides/collaborator-guide.md

Lines 71 to 78 in e6cbec5

    
           A pull request is _author ready_ when: 
        
           * There is a CI run in progress or completed. 
        
           * There is at least one Collaborator approval. 
        
           * There are no outstanding review comments. 
        
           Please always add the `author ready` label to the pull request in that case. 
        
           Please always remove it again as soon as the conditions are not met anymore.

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

Landed in 6527e04

The scripts `./tools/update-babel-eslint.sh` and `./tools/update-eslint.sh` are relying on the version of `npm` found in the local-defined `$PATH` env. This changeset proposes to modify these scripts to run the version of npm bundled in the current branch (found at `./deps/npm`) - in order to: a) Standardize the version of npm that should be use to install these deps, avoids the pitfall of having an inadverted user run these scripts with an unsupported/incompatible npm version. b) Given that npm7 has a different install algorithm than npm6 that takes into account and install peer dependencies, it might be a safer choice to ensure what version of npm should be use during this transitional period in which users might still have npm6 by default in their local system. c) Avoids the possible extra churn of having different resulting files being shuffled around between installs due to usage of a disparate version of the npm cli. PR-URL: #37613 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Mar 5, 2021

Trott approved these changes Mar 5, 2021

View reviewed changes

ruyadorno added dont-land-on-v10.x labels Mar 5, 2021

lpinca approved these changes Mar 5, 2021

View reviewed changes

richardlau approved these changes Mar 5, 2021

View reviewed changes

aduh95 reviewed Mar 5, 2021

View reviewed changes

Comment thread tools/update-babel-eslint.sh Outdated

Comment thread tools/update-babel-eslint.sh Outdated

Comment thread tools/update-babel-eslint.sh Outdated

Comment thread tools/update-babel-eslint.sh Outdated

Comment thread tools/update-babel-eslint.sh Outdated

aduh95 reviewed Mar 5, 2021

View reviewed changes

Comment thread tools/update-babel-eslint.sh Outdated

addaleax approved these changes Mar 5, 2021

View reviewed changes

Comment thread tools/update-babel-eslint.sh Outdated

benjamingr approved these changes Mar 5, 2021

View reviewed changes

ruyadorno force-pushed the use-bundled-npm-in-tools-scripts branch from 58a4c7c to 568db99 Compare March 8, 2021 16:56

This comment has been minimized.

Sign in to view

ruyadorno force-pushed the use-bundled-npm-in-tools-scripts branch from 568db99 to bcea91d Compare March 9, 2021 20:17

This comment has been minimized.

Sign in to view

ruyadorno force-pushed the use-bundled-npm-in-tools-scripts branch from bcea91d to 5c9d52d Compare March 10, 2021 01:47

This comment has been minimized.

Sign in to view

ruyadorno force-pushed the use-bundled-npm-in-tools-scripts branch from 5c9d52d to d5da1a2 Compare March 10, 2021 14:16

aduh95 approved these changes Mar 10, 2021

View reviewed changes

ruyadorno added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 10, 2021

ruyadorno closed this Mar 15, 2021

danielleadams mentioned this pull request Mar 16, 2021

v15.12.0 proposal #37766

Merged

tools: use bundled npm in update scripts by ruyadorno · Pull Request #37613 · nodejs/node · GitHub

Uh oh!

Uh oh!

tools: use bundled npm in update scripts - #37613

tools: use bundled npm in update scripts#37613ruyadorno wants to merge 1 commit intonodejs:masternodejs/node:masterfrom ruyadorno:use-bundled-npm-in-tools-scriptsruyadorno/node:use-bundled-npm-in-tools-scriptsCopy head branch name to clipboard

Conversation

ruyadorno commented Mar 5, 2021

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

ruyadorno commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Mar 5, 2021

Uh oh!

aduh95 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lpinca commented Mar 5, 2021

Uh oh!

aduh95 commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Mar 5, 2021

Uh oh!

lpinca commented Mar 5, 2021

Uh oh!

aduh95 commented Mar 5, 2021

Uh oh!

lpinca commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ruyadorno commented Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Mar 10, 2021

Uh oh!

aduh95 commented Mar 10, 2021

Uh oh!

ruyadorno commented Mar 10, 2021

Uh oh!

aduh95 commented Mar 10, 2021

Uh oh!

nodejs-github-bot commented Mar 12, 2021

Uh oh!

ruyadorno commented Mar 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

tools: use bundled npm in update scripts#37613
ruyadorno wants to merge 1 commit into
nodejs:masterfrom
ruyadorno:use-bundled-npm-in-tools-scripts

ruyadorno commented Mar 5, 2021 •
edited
Loading

aduh95 left a comment •
edited
Loading

aduh95 commented Mar 5, 2021 •
edited
Loading

lpinca commented Mar 5, 2021 •
edited
Loading

ruyadorno commented Mar 8, 2021 •
edited
Loading