-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Conversation
67eb0a3
to
67aeffb
Compare
2145820
to
fbfd2ca
Compare
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`
fbfd2ca
to
62b65d3
Compare
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). |
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 |
@lukekarrys Thanks. What version of node is this? @gauthierm Thank you! 🙏 your comment should get much more attention. |
Perhaps you are asking what version of NodeJS bundled npm with the fix? First, the affected versions of npm (click to expand, for details):
Then for the NodeJS releases that were at one point affected (click to expand, for details):
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?
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.) |
Thank you for the detailed answer 😃 🙏🏽 |
This removes
directories.bin
from npm'spackage.json
since itinstead uses the
bin
object syntax to create the bins fornpm
andnpx
. The non-JS files in that directory are used by the Nodeinstaller, and are not actually bin files that npm is responsible for
linking.
This also does a few items of cleanup around those
bin/
files:node-gyp-bin
files. Those are remnants frombefore
@npmcli/run-script
was introduced innpm@7
. Now thatpackage is responsible for setting
PATH
with the appropriatenode-gyp
bin.bin/npx
where the exit code was not being readfrom the
npx prefix
call.bin/(npm|npx)(.cmd)?
files to ensure theonly differences between them are
npm -> npx