tools: use bundled npm in update scripts #37613

ruyadorno · 2021-03-05T16:43:37Z

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

Trott

Perhaps it makes sense to do the same with node?

ruyadorno · 2021-03-05T16:51:41Z

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)

Trott · 2021-03-05T16:55:27Z

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.

aduh95

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.

tools/update-babel-eslint.sh

lpinca · 2021-03-05T17:42:42Z

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

aduh95 · 2021-03-05T17:44:32Z

@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).

lpinca · 2021-03-05T17:48:42Z

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.

lpinca · 2021-03-05T17:49:29Z

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

aduh95 · 2021-03-05T17:50:54Z

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.

lpinca · 2021-03-05T17:52:16Z

# Depends on node being in $PATH.

aduh95

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.

tools/update-babel-eslint.sh

ruyadorno · 2021-03-08T16:58:10Z

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.

nodejs-github-bot · 2021-03-10T14:16:40Z

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

aduh95 · 2021-03-10T14:44:09Z

@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

ruyadorno · 2021-03-10T14:56:27Z

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.

aduh95 · 2021-03-10T15:03:25Z

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.

nodejs-github-bot · 2021-03-12T12:42:02Z

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

ruyadorno · 2021-03-15T22:13:22Z

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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

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

tools/update-babel-eslint.sh Outdated Show resolved Hide resolved

addaleax approved these changes Mar 5, 2021

View reviewed changes

tools/update-babel-eslint.sh Outdated Show resolved Hide resolved

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 pending requests for changes, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: use bundled npm in update scripts #37613

tools: use bundled npm in update scripts #37613

ruyadorno commented Mar 5, 2021

Trott left a comment

ruyadorno commented Mar 5, 2021 •

edited

Loading

Trott commented Mar 5, 2021

aduh95 left a comment •

edited

Loading

lpinca commented Mar 5, 2021

aduh95 commented Mar 5, 2021 •

edited

Loading

lpinca commented Mar 5, 2021

lpinca commented Mar 5, 2021

aduh95 commented Mar 5, 2021

lpinca commented Mar 5, 2021 •

edited

Loading

aduh95 left a comment

ruyadorno commented Mar 8, 2021 •

edited

Loading

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

aduh95 commented Mar 10, 2021

ruyadorno commented Mar 10, 2021

aduh95 commented Mar 10, 2021

nodejs-github-bot commented Mar 12, 2021

ruyadorno commented Mar 15, 2021

tools: use bundled npm in update scripts #37613

tools: use bundled npm in update scripts #37613

Conversation

ruyadorno commented Mar 5, 2021

Trott left a comment

Choose a reason for hiding this comment

ruyadorno commented Mar 5, 2021 • edited Loading

Trott commented Mar 5, 2021

aduh95 left a comment • edited Loading

Choose a reason for hiding this comment

lpinca commented Mar 5, 2021

aduh95 commented Mar 5, 2021 • edited Loading

lpinca commented Mar 5, 2021

lpinca commented Mar 5, 2021

aduh95 commented Mar 5, 2021

lpinca commented Mar 5, 2021 • edited Loading

aduh95 left a comment

Choose a reason for hiding this comment

ruyadorno commented Mar 8, 2021 • edited Loading

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

aduh95 commented Mar 10, 2021

ruyadorno commented Mar 10, 2021

aduh95 commented Mar 10, 2021

nodejs-github-bot commented Mar 12, 2021

ruyadorno commented Mar 15, 2021

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