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

fix: cleanup bin contents #6554

Merged
merged 1 commit into from
Jun 14, 2023
Merged

fix: cleanup bin contents #6554

merged 1 commit into from
Jun 14, 2023

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Jun 14, 2023

This removes directories.bin from npm's package.json since it
instead uses the bin object syntax to create the bins for npm and
npx. The non-JS files in that directory are used by the Node
installer, and are not actually bin files that npm is responsible for
linking.

This also does a few items of cleanup around those bin/ files:

  • Removes the unused node-gyp-bin files. Those are remnants from
    before @npmcli/run-script was introduced in npm@7. Now that
    package is responsible for setting PATH with the appropriate
    node-gyp bin.
  • Fixes an issue in bin/npx where the exit code was not being read
    from the npx prefix call.
  • Test the contents of the bin/(npm|npx)(.cmd)? files to ensure the
    only differences between them are npm -> npx

@lukekarrys lukekarrys requested a review from a team as a code owner June 14, 2023 03:47
@lukekarrys lukekarrys force-pushed the lk/bin-cleanup branch 2 times, most recently from 67eb0a3 to 67aeffb Compare June 14, 2023 04:06
@lukekarrys lukekarrys changed the title fix: remove directories.bin and cleanup fix: cleanup bin contents Jun 14, 2023
@lukekarrys lukekarrys force-pushed the lk/bin-cleanup branch 2 times, most recently from 2145820 to fbfd2ca Compare June 14, 2023 07:25
This removes `directories.bin` from npm's `package.json` since it
instead uses the `bin` object syntax to create the bins for `npm` and
`npx`. The non-JS files in that directory are used by the Node
installer, and are not actually bin files that npm is responsible for
linking.

This also does a few items of cleanup around those `bin/` files:

- Removes the unused `node-gyp-bin` files. Those are remnants from
  before `@npmcli/run-script` was introduced in `npm@7`. Now that
  package is responsible for setting `PATH` with the appropriate
  `node-gyp` bin.
- Fixes an issue in `bin/npx` where the exit code was not being read
  from the `npx prefix` call.
- Test the contents of the `bin/(npm|npx)(.cmd)?` files to ensure the
  only differences between them are `npm -> npx`
@gauthierm
Copy link

Note: This breaks yarn 1.22.x as it relied on the node-gyp-bin folder. I think Yarn 1.x is frozen, but still sees a fair amount of use.

This is probably not desirable to fix, but I'm leaving a comment to let other folks know that node-gyp builds will fail with Yarn 1.22.x and newer versions of npm (such as npm 9.8.1 bundled with node 18.18.0).

See https://github.com/yarnpkg/yarn/blob/158d96dce95313d9a00218302631cd263877d164/src/util/execute-lifecycle-script.js#L177

lukekarrys added a commit that referenced this pull request Oct 25, 2023
This was an unintended breaking change as part of #6554. The `node-gyp`
bin files are not used by the CLI directly but they are relied on by
other tooling. This change is only intended to land on the v9 release
line.

This partially reverts commit 3a7378d.
lukekarrys added a commit that referenced this pull request Oct 25, 2023
This was an unintended breaking change as part of #6554. The `node-gyp`
bin files are not used by the CLI directly but they are relied on by
other tooling. This change is only intended to land on the v9 release
line.

This partially reverts commit 3a7378d.
lukekarrys added a commit that referenced this pull request Oct 26, 2023
This was an unintended breaking change as part of #6554. The `node-gyp`
bin files are not used by the CLI directly but they are relied on by
other tooling. This change is only intended to land on the v9 release
line.

This partially reverts commit 3a7378d.
@lukekarrys
Copy link
Contributor Author

lukekarrys commented Oct 26, 2023

Note: This breaks yarn 1.22.x as it relied on the node-gyp-bin folder. I think Yarn 1.x is frozen, but still sees a fair amount of use.

The portion of this PR that led to this breaking in Yarn 1.x has been reverted in #6932 and will land in the next release of npm@9, which will likely be npm@9.9.1 (#6926).

lukekarrys added a commit that referenced this pull request Oct 30, 2023
This was an unintended breaking change as part of #6554. The `node-gyp`
bin files are not used by the CLI directly but they are relied on by
other tooling.

This partially reverts commit 3a7378d.
wraithgar pushed a commit that referenced this pull request Oct 30, 2023
This was an unintended breaking change as part of #6554. The `node-gyp`
bin files are not used by the CLI directly but they are relied on by
other tooling.

This partially reverts commit 3a7378d.
@sag1v
Copy link

sag1v commented May 7, 2024

The portion of this PR that led to this breaking in Yarn 1.x has been reverted in #6932 and will land in the next release of npm@9, which will likely be npm@9.9.1 (#6926).

@lukekarrys Thanks. What version of node is this?

@gauthierm Thank you! 🙏 your comment should get much more attention.

@DeeDeeG
Copy link

DeeDeeG commented May 8, 2024

What version of node is this?

Perhaps you are asking what version of NodeJS bundled npm with the fix?

First, the affected versions of npm (click to expand, for details):
  • We can see this PR's commit landed in latest branch on the 14th of June 2023... and was included in npm 9.7.2 as well as npm 10.0.0.
    • npm 9.7.2-9.9.0 would appear to be affected ❌, from some quick research, whereas npm 9.9.1+ have the fix ✅.
    • npm 10.0.0-10.2.1 would appear to be affected ❌, from some quick research, whereas npm 10.2.2+ have the fix ✅.
Then for the NodeJS releases that were at one point affected (click to expand, for details):
  • NodeJS 16 and older were never affected ✅, since they shipped older npm that never removed these binstubs.
    • In terms of non-LTS versions of NodeJS, NodeJS 17 was also never affected ✅, for the same reason.
  • NodeJS 18: NodeJs 18.8.0 is affected ❌ due to bundling npm 9.8.1 which is affected. The immediate next version NodeJS 18.9.0 and newer are unaffected ✅ due to having npm 10.2.3 or newer, which has the fix.
    • NodeJS 19 is unaffected ✅, due to never bundling an affected version of npm.
  • NodeJS 20: NodeJS 20.4.0-20.9.0 are affected ❌ due to bundling variously npm 9.7.2, 9.8.0, 9.8.1, or 10.1.0, all of which are affected. 20.10.0 and newer are unaffected ✅, due to having npm 10.2.3 which has the fix.
  • NodeJS 21 had NodeJS 21.0.0 and 21.1.0 affected ❌, due to bunding npm 10.2.0, which is affected. NodeJS 21.2.0 and newer are unaffected ✅, due to having npm 10.2.3 or newer, which has the fix.
  • NodeJS 22 is unaffected ✅, due to bundling npm 10.5.1 or newer, which has the fix.

Review changelogs from here if you wish to confirm for yourselves: https://github.com/nodejs/node/blob/main/CHANGELOG.md

At this point, it appears that only historical NodeJS versions were ever affected, which are all now out of support, and furthermore have been superseded within their respective major versions with a release having the fix.

The simplest answer is to update to the latest minor/patch release of a given NodeJS major version if you want to avoid the Yarn 1.x issue. Or else install/use an unaffected version of npm with whatever arbitrary version of NodeJS. For instance, the latest npm (the latest npm 10.x version as of when I'm writing this comment), as well as the latest npm 9.x version, should both be unaffected. (Otherwise, if it is necessary to know the specific versions of npm which are affected, and/or which specific versions of npm were bundled with the various NodeJS versions over time, feel free to expand the sections I wrote above.)

So, to summarize... in terms of the versions of npm that were bundled with various NodeJS releases over time... which of those were affected?

  • NodeJS 16 and older were never affected ✅.
  • NodeJS 17 was never affected ✅.
  • NodeJS 18.8.0 is affected ❌ (NodeJS 18.9.0+ not affected ✅)
  • NodeJS 19 was never affected ✅.
  • NodeJS 20.4.0-20.9.0 are affected ❌ (NodeJS 20.10.0+ not affected ✅).
  • NodeJS 21.0.0 and 21.1.0 are affected ❌ (NodeJS 21.2.0+ not affected ✅).
  • NodeJS 22 appears unaffected ✅, as of when this comment was posted.

Take care that pretty much any version of npm can be installed or used with any version of NodeJS. The actual root determining factor is the npm version -- this comment is only about reviewing which versions of npm were bundled with which versions of NodeJS by default, "out of the box", for reference purposes.

(Any mistakes in this particular comment are mine, and be aware I am not affiliated with and do not represent either of the npm or NodeJS teams.)

@sag1v
Copy link

sag1v commented May 8, 2024

Thank you for the detailed answer 😃 🙏🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants