Skip to content

Commit

Permalink
fix: cleanup bin contents
Browse files Browse the repository at this point in the history
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`
  • Loading branch information
lukekarrys committed Jun 14, 2023
1 parent 29622c1 commit fbfd2ca
Show file tree
Hide file tree
Showing 9 changed files with 143 additions and 119 deletions.
1 change: 1 addition & 0 deletions DEPENDENCIES.md
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,7 @@ graph LR;
npm-->cli-columns;
npm-->cli-table3;
npm-->columnify;
npm-->diff;
npm-->fastest-levenshtein;
npm-->fs-minipass;
npm-->glob;
Expand Down
6 changes: 0 additions & 6 deletions bin/node-gyp-bin/node-gyp

This file was deleted.

5 changes: 0 additions & 5 deletions bin/node-gyp-bin/node-gyp.cmd

This file was deleted.

5 changes: 4 additions & 1 deletion bin/npm
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
#!/usr/bin/env bash

# This is used by the Node.js installer, which expects the cygwin/mingw
# shell script to already be present in the npm dependency folder.

(set -o igncr) 2>/dev/null && set -o igncr; # cygwin encoding fix

basedir=`dirname "$0"`
Expand All @@ -19,7 +23,6 @@ fi
# kind of paths Node.js thinks it's using, typically win32 paths.
CLI_BASEDIR="$("$NODE_EXE" -p 'require("path").dirname(process.execPath)')"
NPM_CLI_JS="$CLI_BASEDIR/node_modules/npm/bin/npm-cli.js"

NPM_PREFIX=`"$NODE_EXE" "$NPM_CLI_JS" prefix -g`
if [ $? -ne 0 ]; then
# if this didn't work, then everything else below will fail
Expand Down
8 changes: 4 additions & 4 deletions bin/npx
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,17 @@ if ! [ -x "$NODE_EXE" ]; then
NODE_EXE=node
fi

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

# a path that will fail -f test on any posix bash
Expand Down
1 change: 1 addition & 0 deletions package-lock.json
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@
"@npmcli/promise-spawn": "^6.0.2",
"@npmcli/template-oss": "4.14.1",
"@tufjs/repo-mock": "^1.3.1",
"diff": "^5.1.0",
"licensee": "^10.0.0",
"nock": "^13.3.0",
"npm-packlist": "^7.0.4",
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
"url": "https://github.com/npm/cli/issues"
},
"directories": {
"bin": "./bin",
"doc": "./doc",
"lib": "./lib",
"man": "./man"
Expand Down Expand Up @@ -196,6 +195,7 @@
"@npmcli/promise-spawn": "^6.0.2",
"@npmcli/template-oss": "4.14.1",
"@tufjs/repo-mock": "^1.3.1",
"diff": "^5.1.0",
"licensee": "^10.0.0",
"nock": "^13.3.0",
"npm-packlist": "^7.0.4",
Expand Down
1 change: 0 additions & 1 deletion tap-snapshots/test/lib/commands/publish.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ Object {
},
"description": "a package manager for JavaScript",
"directories": Object {
"bin": "./bin",
"doc": "./doc",
"lib": "./lib",
"man": "./man",
Expand Down
233 changes: 132 additions & 101 deletions test/bin/windows-shims.js
Original file line number Diff line number Diff line change
@@ -1,133 +1,164 @@
const t = require('tap')

if (process.platform !== 'win32') {
t.plan(0, 'test only relevant on windows')
process.exit(0)
}

const has = path => {
try {
// If WSL is installed, it *has* a bash.exe, but it fails if
// there is no distro installed, so we need to detect that.
const result = spawnSync(path, ['-l', '-c', 'exit 0'])
if (result.status === 0) {
return true
} else {
// print whatever error we got
throw result.error || Object.assign(new Error(String(result.stderr)), {
code: result.status,
})
}
} catch (er) {
t.comment(`not installed: ${path}`, er)
return false
}
}

const { version } = require('../../package.json')
const spawn = require('@npmcli/promise-spawn')
const { spawnSync } = require('child_process')
const { resolve } = require('path')
const { ProgramFiles, SystemRoot } = process.env
const { readFileSync, chmodSync } = require('fs')
const gitBash = resolve(ProgramFiles, 'Git', 'bin', 'bash.exe')
const gitUsrBinBash = resolve(ProgramFiles, 'Git', 'usr', 'bin', 'bash.exe')
const wslBash = resolve(SystemRoot, 'System32', 'bash.exe')
const cygwinBash = resolve(SystemRoot, '/', 'cygwin64', 'bin', 'bash.exe')

const bashes = Object.entries({
'wsl bash': wslBash,
'git bash': gitBash,
'git internal bash': gitUsrBinBash,
'cygwin bash': cygwinBash,
})
const Diff = require('diff')
const { version } = require('../../package.json')

const npmShim = resolve(__dirname, '../../bin/npm')
const npxShim = resolve(__dirname, '../../bin/npx')

const path = t.testdir({
'node.exe': readFileSync(process.execPath),
npm: readFileSync(npmShim),
npx: readFileSync(npxShim),
// simulate the state where one version of npm is installed
// with node, but we should load the globally installed one
'global-prefix': {
node_modules: {
npm: t.fixture('symlink', resolve(__dirname, '../..')),
t.test('npm vs npx', t => {
// these scripts should be kept in sync so this tests the contents of each
// and does a diff to ensure the only differences between them are necessary
const diffFiles = (ext = '') => Diff.diffChars(
readFileSync(`${npmShim}${ext}`, 'utf8'),
readFileSync(`${npxShim}${ext}`, 'utf8')
).filter(v => v.added || v.removed).map((v, i) => i === 0 ? v.value : v.value.toUpperCase())

t.test('bash', t => {
const [npxCli, ...changes] = diffFiles()
const npxCliLine = npxCli.split('\n').reverse().join('')
t.match(npxCliLine, /^NPX_CLI_JS=/, 'has NPX_CLI')
t.equal(changes.length, 20)
t.strictSame([...new Set(changes)], ['M', 'X'], 'all other changes are m->x')
t.end()
})

t.test('cmd', t => {
const [npxCli, ...changes] = diffFiles('.cmd')
t.match(npxCli, /^SET "NPX_CLI_JS=/, 'has NPX_CLI')
t.equal(changes.length, 12)
t.strictSame([...new Set(changes)], ['M', 'X'], 'all other changes are m->x')
t.end()
})

t.end()
})

t.test('basic', async t => {
if (process.platform !== 'win32') {
t.plan(0, 'test only relevant on windows')
return
}

const has = path => {
try {
// If WSL is installed, it *has* a bash.exe, but it fails if
// there is no distro installed, so we need to detect that.
const result = spawnSync(path, ['-l', '-c', 'exit 0'])
if (result.status === 0) {
return true
} else {
// print whatever error we got
throw result.error || Object.assign(new Error(String(result.stderr)), {
code: result.status,
})
}
} catch (er) {
t.comment(`not installed: ${path}`, er)
return false
}
}

const { ProgramFiles, SystemRoot } = process.env
const gitBash = resolve(ProgramFiles, 'Git', 'bin', 'bash.exe')
const gitUsrBinBash = resolve(ProgramFiles, 'Git', 'usr', 'bin', 'bash.exe')
const wslBash = resolve(SystemRoot, 'System32', 'bash.exe')
const cygwinBash = resolve(SystemRoot, '/', 'cygwin64', 'bin', 'bash.exe')

const bashes = Object.entries({
'wsl bash': wslBash,
'git bash': gitBash,
'git internal bash': gitUsrBinBash,
'cygwin bash': cygwinBash,
})

const path = t.testdir({
'node.exe': readFileSync(process.execPath),
npm: readFileSync(npmShim),
npx: readFileSync(npxShim),
// simulate the state where one version of npm is installed
// with node, but we should load the globally installed one
'global-prefix': {
node_modules: {
npm: t.fixture('symlink', resolve(__dirname, '../..')),
},
},
},
// put in a shim that ONLY prints the intended global prefix,
// and should not be used for anything else.
node_modules: {
npm: {
bin: {
'npx-cli.js': `
// put in a shim that ONLY prints the intended global prefix,
// and should not be used for anything else.
node_modules: {
npm: {
bin: {
'npx-cli.js': `
throw new Error('this should not be called')
`,
'npm-cli.js': `
'npm-cli.js': `
const assert = require('assert')
const args = process.argv.slice(2)
assert.equal(args[0], 'prefix')
assert.equal(args[1], '-g')
const { resolve } = require('path')
console.log(resolve(__dirname, '../../../global-prefix'))
`,
},
},
},
},
})
chmodSync(resolve(path, 'npm'), 0o755)
chmodSync(resolve(path, 'npx'), 0o755)
})
chmodSync(resolve(path, 'npm'), 0o755)
chmodSync(resolve(path, 'npx'), 0o755)

for (const [name, bash] of bashes) {
if (!has(bash)) {
t.skip(`${name} not installed`, { bin: bash, diagnostic: true })
continue
}
for (const [name, bash] of bashes) {
if (!has(bash)) {
t.skip(`${name} not installed`, { bin: bash, diagnostic: true })
continue
}

if (bash === cygwinBash && process.env.NYC_CONFIG) {
t.skip('Cygwin does not play nicely with NYC, run without coverage')
continue
}
if (bash === cygwinBash && process.env.NYC_CONFIG) {
t.skip('Cygwin does not play nicely with NYC, run without coverage')
continue
}

t.test(name, async t => {
t.plan(2)
t.test('npm', async t => {
await t.test(name, async t => {
t.plan(2)
t.test('npm', async t => {
// only cygwin *requires* the -l, but the others are ok with it
// don't hit the registry for the update check
const args = ['-l', 'npm', 'help']
const args = ['-l', 'npm', 'help']

const result = await spawn(bash, args, {
env: { PATH: path, npm_config_update_notifier: 'false' },
cwd: path,
const result = await spawn(bash, args, {
env: { PATH: path, npm_config_update_notifier: 'false' },
cwd: path,
})
t.match(result, {
cmd: bash,
args: ['-l', 'npm', 'help'],
code: 0,
signal: null,
stderr: String,
// should have loaded this instance of npm we symlinked in
stdout: `npm@${version} ${resolve(__dirname, '../..')}`,
})
})
t.match(result, {
cmd: bash,
args: ['-l', 'npm', 'help'],
code: 0,
signal: null,
stderr: String,
// should have loaded this instance of npm we symlinked in
stdout: `npm@${version} ${resolve(__dirname, '../..')}`,
})
})

t.test('npx', async t => {
const args = ['-l', 'npx', '--version']
t.test('npx', async t => {
const args = ['-l', 'npx', '--version']

const result = await spawn(bash, args, {
env: { PATH: path, npm_config_update_notifier: 'false' },
cwd: path,
})
t.match(result, {
cmd: bash,
args: ['-l', 'npx', '--version'],
code: 0,
signal: null,
stderr: String,
// should have loaded this instance of npm we symlinked in
stdout: version,
const result = await spawn(bash, args, {
env: { PATH: path, npm_config_update_notifier: 'false' },
cwd: path,
})
t.match(result, {
cmd: bash,
args: ['-l', 'npx', '--version'],
code: 0,
signal: null,
stderr: String,
// should have loaded this instance of npm we symlinked in
stdout: version,
})
})
})
})
}
}
})

0 comments on commit fbfd2ca

Please sign in to comment.