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

tools: use bundled npm in update scripts #37613

Closed

Conversation

ruyadorno
Copy link
Member

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:

  1. 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.
  2. 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.
  3. 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

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

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Perhaps it makes sense to do the same with node?

@ruyadorno
Copy link
Member Author

ruyadorno commented Mar 5, 2021

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
Copy link
Member

Trott commented Mar 5, 2021

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.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
tools/update-babel-eslint.sh Outdated Show resolved Hide resolved
tools/update-babel-eslint.sh Outdated Show resolved Hide resolved
tools/update-babel-eslint.sh Outdated Show resolved Hide resolved
tools/update-babel-eslint.sh Outdated Show resolved Hide resolved
@lpinca
Copy link
Member

lpinca commented Mar 5, 2021

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

@aduh95
Copy link
Contributor

aduh95 commented Mar 5, 2021

@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
Copy link
Member

lpinca commented Mar 5, 2021

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
Copy link
Member

lpinca commented Mar 5, 2021

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

@aduh95
Copy link
Contributor

aduh95 commented Mar 5, 2021

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
Copy link
Member

lpinca commented Mar 5, 2021

# Depends on node being in $PATH.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
tools/update-babel-eslint.sh Outdated Show resolved Hide resolved
@ruyadorno ruyadorno force-pushed the use-bundled-npm-in-tools-scripts branch from 58a4c7c to 568db99 Compare March 8, 2021 16:56
@ruyadorno
Copy link
Member Author

ruyadorno commented Mar 8, 2021

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

@nodejs-github-bot

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

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.
@ruyadorno ruyadorno force-pushed the use-bundled-npm-in-tools-scripts branch from 5c9d52d to d5da1a2 Compare March 10, 2021 14:16
@nodejs-github-bot
Copy link
Collaborator

@aduh95
Copy link
Contributor

aduh95 commented Mar 10, 2021

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

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
Copy link
Member Author

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
Copy link
Contributor

aduh95 commented Mar 10, 2021

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

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.

@ruyadorno 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
@nodejs-github-bot
Copy link
Collaborator

@ruyadorno
Copy link
Member Author

Landed in 6527e04

ruyadorno added a commit that referenced this pull request Mar 15, 2021
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>
@ruyadorno ruyadorno closed this Mar 15, 2021
danielleadams pushed a commit that referenced this pull request Mar 16, 2021
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>
danielleadams pushed a commit that referenced this pull request Mar 16, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants