Skip to content

Commit 67aeffb

Browse files
committed
fix: cleanup bin contents
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`
1 parent 29622c1 commit 67aeffb

File tree

8 files changed

+126
-103
lines changed

8 files changed

+126
-103
lines changed

bin/node-gyp-bin/node-gyp

-6
This file was deleted.

bin/node-gyp-bin/node-gyp.cmd

-5
This file was deleted.

bin/npm

+4-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
11
#!/usr/bin/env bash
2+
3+
# This is used by the Node.js installer, which expects the cygwin/mingw
4+
# shell script to already be present in the npm dependency folder.
5+
26
(set -o igncr) 2>/dev/null && set -o igncr; # cygwin encoding fix
37

48
basedir=`dirname "$0"`
@@ -19,7 +23,6 @@ fi
1923
# kind of paths Node.js thinks it's using, typically win32 paths.
2024
CLI_BASEDIR="$("$NODE_EXE" -p 'require("path").dirname(process.execPath)')"
2125
NPM_CLI_JS="$CLI_BASEDIR/node_modules/npm/bin/npm-cli.js"
22-
2326
NPM_PREFIX=`"$NODE_EXE" "$NPM_CLI_JS" prefix -g`
2427
if [ $? -ne 0 ]; then
2528
# if this didn't work, then everything else below will fail

bin/npx

+4-4
Original file line numberDiff line numberDiff line change
@@ -19,17 +19,17 @@ if ! [ -x "$NODE_EXE" ]; then
1919
NODE_EXE=node
2020
fi
2121

22-
# these paths are passed to node.exe, so they need to match whatever
22+
# this path is passed to node.exe, so it needs to match whatever
2323
# kind of paths Node.js thinks it's using, typically win32 paths.
2424
CLI_BASEDIR="$("$NODE_EXE" -p 'require("path").dirname(process.execPath)')"
25+
NPM_CLI_JS="$CLI_BASEDIR/node_modules/npm/bin/npm-cli.js"
26+
NPX_CLI_JS="$CLI_BASEDIR/node_modules/npm/bin/npx-cli.js"
27+
NPM_PREFIX=`"$NODE_EXE" "$NPM_CLI_JS" prefix -g`
2528
if [ $? -ne 0 ]; then
2629
# if this didn't work, then everything else below will fail
2730
echo "Could not determine Node.js install directory" >&2
2831
exit 1
2932
fi
30-
NPM_CLI_JS="$CLI_BASEDIR/node_modules/npm/bin/npm-cli.js"
31-
NPX_CLI_JS="$CLI_BASEDIR/node_modules/npm/bin/npx-cli.js"
32-
NPM_PREFIX=`"$NODE_EXE" "$NPM_CLI_JS" prefix -g`
3333
NPM_PREFIX_NPX_CLI_JS="$NPM_PREFIX/node_modules/npm/bin/npx-cli.js"
3434

3535
# a path that will fail -f test on any posix bash

package-lock.json

+1
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,7 @@
163163
"@npmcli/promise-spawn": "^6.0.2",
164164
"@npmcli/template-oss": "4.14.1",
165165
"@tufjs/repo-mock": "^1.3.1",
166+
"diff": "^5.1.0",
166167
"licensee": "^10.0.0",
167168
"nock": "^13.3.0",
168169
"npm-packlist": "^7.0.4",

package.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@
3333
"url": "https://github.com/npm/cli/issues"
3434
},
3535
"directories": {
36-
"bin": "./bin",
3736
"doc": "./doc",
3837
"lib": "./lib",
3938
"man": "./man"
@@ -196,6 +195,7 @@
196195
"@npmcli/promise-spawn": "^6.0.2",
197196
"@npmcli/template-oss": "4.14.1",
198197
"@tufjs/repo-mock": "^1.3.1",
198+
"diff": "^5.1.0",
199199
"licensee": "^10.0.0",
200200
"nock": "^13.3.0",
201201
"npm-packlist": "^7.0.4",

tap-snapshots/test/lib/commands/publish.js.test.cjs

-1
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,6 @@ Object {
114114
},
115115
"description": "a package manager for JavaScript",
116116
"directories": Object {
117-
"bin": "./bin",
118117
"doc": "./doc",
119118
"lib": "./lib",
120119
"man": "./man",

test/bin/windows-shims.js

+116-85
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,13 @@
11
const t = require('tap')
2+
const spawn = require('@npmcli/promise-spawn')
3+
const { spawnSync } = require('child_process')
4+
const { resolve } = require('path')
5+
const { readFileSync, chmodSync } = require('fs')
6+
const Diff = require('diff')
7+
const { version } = require('../../package.json')
28

3-
if (process.platform !== 'win32') {
4-
t.plan(0, 'test only relevant on windows')
5-
process.exit(0)
6-
}
9+
const npmShim = resolve(__dirname, '../../bin/npm')
10+
const npxShim = resolve(__dirname, '../../bin/npx')
711

812
const has = path => {
913
try {
@@ -24,110 +28,137 @@ const has = path => {
2428
}
2529
}
2630

27-
const { version } = require('../../package.json')
28-
const spawn = require('@npmcli/promise-spawn')
29-
const { spawnSync } = require('child_process')
30-
const { resolve } = require('path')
31-
const { ProgramFiles, SystemRoot } = process.env
32-
const { readFileSync, chmodSync } = require('fs')
33-
const gitBash = resolve(ProgramFiles, 'Git', 'bin', 'bash.exe')
34-
const gitUsrBinBash = resolve(ProgramFiles, 'Git', 'usr', 'bin', 'bash.exe')
35-
const wslBash = resolve(SystemRoot, 'System32', 'bash.exe')
36-
const cygwinBash = resolve(SystemRoot, '/', 'cygwin64', 'bin', 'bash.exe')
37-
38-
const bashes = Object.entries({
39-
'wsl bash': wslBash,
40-
'git bash': gitBash,
41-
'git internal bash': gitUsrBinBash,
42-
'cygwin bash': cygwinBash,
31+
t.test('npm vs npx', t => {
32+
// these scripts should be kept in sync so this tests the contents of each
33+
// and does a diff to ensure the only differences between them are necessary
34+
const diffFiles = (ext = '') => Diff.diffChars(
35+
readFileSync(`${npmShim}${ext}`, 'utf8'),
36+
readFileSync(`${npxShim}${ext}`, 'utf8')
37+
).filter(v => v.added || v.removed).map((v, i) => i === 0 ? v.value : v.value.toUpperCase())
38+
39+
t.test('bash', t => {
40+
const [npxCli, ...changes] = diffFiles()
41+
const npxCliLine = npxCli.split('\n').reverse().join('')
42+
t.match(npxCliLine, /^NPX_CLI_JS=/, 'has NPX_CLI')
43+
t.equal(changes.length, 20)
44+
t.strictSame([...new Set(changes)], ['M', 'X'], 'all other changes are m->x')
45+
t.end()
46+
})
47+
48+
t.test('cmd', t => {
49+
const [npxCli, ...changes] = diffFiles('.cmd')
50+
t.match(npxCli, /^SET "NPX_CLI_JS=/, 'has NPX_CLI')
51+
t.equal(changes.length, 12)
52+
t.strictSame([...new Set(changes)], ['M', 'X'], 'all other changes are m->x')
53+
t.end()
54+
})
55+
56+
t.end()
4357
})
4458

45-
const npmShim = resolve(__dirname, '../../bin/npm')
46-
const npxShim = resolve(__dirname, '../../bin/npx')
59+
t.test('basic', t => {
60+
if (process.platform !== 'win32') {
61+
t.plan(0, 'test only relevant on windows')
62+
return
63+
}
4764

48-
const path = t.testdir({
49-
'node.exe': readFileSync(process.execPath),
50-
npm: readFileSync(npmShim),
51-
npx: readFileSync(npxShim),
52-
// simulate the state where one version of npm is installed
53-
// with node, but we should load the globally installed one
54-
'global-prefix': {
55-
node_modules: {
56-
npm: t.fixture('symlink', resolve(__dirname, '../..')),
65+
const { ProgramFiles, SystemRoot } = process.env
66+
const gitBash = resolve(ProgramFiles, 'Git', 'bin', 'bash.exe')
67+
const gitUsrBinBash = resolve(ProgramFiles, 'Git', 'usr', 'bin', 'bash.exe')
68+
const wslBash = resolve(SystemRoot, 'System32', 'bash.exe')
69+
const cygwinBash = resolve(SystemRoot, '/', 'cygwin64', 'bin', 'bash.exe')
70+
71+
const bashes = Object.entries({
72+
'wsl bash': wslBash,
73+
'git bash': gitBash,
74+
'git internal bash': gitUsrBinBash,
75+
'cygwin bash': cygwinBash,
76+
})
77+
78+
const path = t.testdir({
79+
'node.exe': readFileSync(process.execPath),
80+
npm: readFileSync(npmShim),
81+
npx: readFileSync(npxShim),
82+
// simulate the state where one version of npm is installed
83+
// with node, but we should load the globally installed one
84+
'global-prefix': {
85+
node_modules: {
86+
npm: t.fixture('symlink', resolve(__dirname, '../..')),
87+
},
5788
},
58-
},
59-
// put in a shim that ONLY prints the intended global prefix,
60-
// and should not be used for anything else.
61-
node_modules: {
62-
npm: {
63-
bin: {
64-
'npx-cli.js': `
89+
// put in a shim that ONLY prints the intended global prefix,
90+
// and should not be used for anything else.
91+
node_modules: {
92+
npm: {
93+
bin: {
94+
'npx-cli.js': `
6595
throw new Error('this should not be called')
6696
`,
67-
'npm-cli.js': `
97+
'npm-cli.js': `
6898
const assert = require('assert')
6999
const args = process.argv.slice(2)
70100
assert.equal(args[0], 'prefix')
71101
assert.equal(args[1], '-g')
72102
const { resolve } = require('path')
73103
console.log(resolve(__dirname, '../../../global-prefix'))
74104
`,
105+
},
75106
},
76107
},
77-
},
78-
})
79-
chmodSync(resolve(path, 'npm'), 0o755)
80-
chmodSync(resolve(path, 'npx'), 0o755)
108+
})
109+
chmodSync(resolve(path, 'npm'), 0o755)
110+
chmodSync(resolve(path, 'npx'), 0o755)
81111

82-
for (const [name, bash] of bashes) {
83-
if (!has(bash)) {
84-
t.skip(`${name} not installed`, { bin: bash, diagnostic: true })
85-
continue
86-
}
112+
for (const [name, bash] of bashes) {
113+
if (!has(bash)) {
114+
t.skip(`${name} not installed`, { bin: bash, diagnostic: true })
115+
continue
116+
}
87117

88-
if (bash === cygwinBash && process.env.NYC_CONFIG) {
89-
t.skip('Cygwin does not play nicely with NYC, run without coverage')
90-
continue
91-
}
118+
if (bash === cygwinBash && process.env.NYC_CONFIG) {
119+
t.skip('Cygwin does not play nicely with NYC, run without coverage')
120+
continue
121+
}
92122

93-
t.test(name, async t => {
94-
t.plan(2)
95-
t.test('npm', async t => {
123+
t.test(name, async t => {
124+
t.plan(2)
125+
t.test('npm', async t => {
96126
// only cygwin *requires* the -l, but the others are ok with it
97127
// don't hit the registry for the update check
98-
const args = ['-l', 'npm', 'help']
128+
const args = ['-l', 'npm', 'help']
99129

100-
const result = await spawn(bash, args, {
101-
env: { PATH: path, npm_config_update_notifier: 'false' },
102-
cwd: path,
130+
const result = await spawn(bash, args, {
131+
env: { PATH: path, npm_config_update_notifier: 'false' },
132+
cwd: path,
133+
})
134+
t.match(result, {
135+
cmd: bash,
136+
args: ['-l', 'npm', 'help'],
137+
code: 0,
138+
signal: null,
139+
stderr: String,
140+
// should have loaded this instance of npm we symlinked in
141+
stdout: `npm@${version} ${resolve(__dirname, '../..')}`,
142+
})
103143
})
104-
t.match(result, {
105-
cmd: bash,
106-
args: ['-l', 'npm', 'help'],
107-
code: 0,
108-
signal: null,
109-
stderr: String,
110-
// should have loaded this instance of npm we symlinked in
111-
stdout: `npm@${version} ${resolve(__dirname, '../..')}`,
112-
})
113-
})
114144

115-
t.test('npx', async t => {
116-
const args = ['-l', 'npx', '--version']
145+
t.test('npx', async t => {
146+
const args = ['-l', 'npx', '--version']
117147

118-
const result = await spawn(bash, args, {
119-
env: { PATH: path, npm_config_update_notifier: 'false' },
120-
cwd: path,
121-
})
122-
t.match(result, {
123-
cmd: bash,
124-
args: ['-l', 'npx', '--version'],
125-
code: 0,
126-
signal: null,
127-
stderr: String,
128-
// should have loaded this instance of npm we symlinked in
129-
stdout: version,
148+
const result = await spawn(bash, args, {
149+
env: { PATH: path, npm_config_update_notifier: 'false' },
150+
cwd: path,
151+
})
152+
t.match(result, {
153+
cmd: bash,
154+
args: ['-l', 'npx', '--version'],
155+
code: 0,
156+
signal: null,
157+
stderr: String,
158+
// should have loaded this instance of npm we symlinked in
159+
stdout: version,
160+
})
130161
})
131162
})
132-
})
133-
}
163+
}
164+
})

0 commit comments

Comments
 (0)