From 3a7378d889707d2a4c1f8a6397dda87825e9f5a3 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Tue, 13 Jun 2023 20:47:33 -0700 Subject: [PATCH] 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` --- DEPENDENCIES.md | 1 + bin/node-gyp-bin/node-gyp | 6 - bin/node-gyp-bin/node-gyp.cmd | 5 - bin/npm | 5 +- bin/npx | 8 +- package-lock.json | 1 + package.json | 2 +- .../test/lib/commands/publish.js.test.cjs | 1 - test/bin/windows-shims.js | 233 ++++++++++-------- 9 files changed, 143 insertions(+), 119 deletions(-) delete mode 100755 bin/node-gyp-bin/node-gyp delete mode 100755 bin/node-gyp-bin/node-gyp.cmd diff --git a/DEPENDENCIES.md b/DEPENDENCIES.md index 5e91aebe1dfe9..e1e689bbd8f81 100644 --- a/DEPENDENCIES.md +++ b/DEPENDENCIES.md @@ -510,6 +510,7 @@ graph LR; npm-->cli-columns; npm-->cli-table3; npm-->columnify; + npm-->diff; npm-->fastest-levenshtein; npm-->fs-minipass; npm-->glob; diff --git a/bin/node-gyp-bin/node-gyp b/bin/node-gyp-bin/node-gyp deleted file mode 100755 index 70efb6f339f76..0000000000000 --- a/bin/node-gyp-bin/node-gyp +++ /dev/null @@ -1,6 +0,0 @@ -#!/usr/bin/env sh -if [ "x$npm_config_node_gyp" = "x" ]; then - node "`dirname "$0"`/../../node_modules/node-gyp/bin/node-gyp.js" "$@" -else - "$npm_config_node_gyp" "$@" -fi diff --git a/bin/node-gyp-bin/node-gyp.cmd b/bin/node-gyp-bin/node-gyp.cmd deleted file mode 100755 index 1ef2ae0c68fc4..0000000000000 --- a/bin/node-gyp-bin/node-gyp.cmd +++ /dev/null @@ -1,5 +0,0 @@ -if not defined npm_config_node_gyp ( - node "%~dp0\..\..\node_modules\node-gyp\bin\node-gyp.js" %* -) else ( - node "%npm_config_node_gyp%" %* -) diff --git a/bin/npm b/bin/npm index a131a53543404..a08b4d113c444 100755 --- a/bin/npm +++ b/bin/npm @@ -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"` @@ -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 diff --git a/bin/npx b/bin/npx index a34e3459b5a70..c51ad45cb68ae 100755 --- a/bin/npx +++ b/bin/npx @@ -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 diff --git a/package-lock.json b/package-lock.json index ea9ce92c57c58..2f8e49bceb07f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -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", diff --git a/package.json b/package.json index f417d60fbab8c..70536a8505a94 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,6 @@ "url": "https://github.com/npm/cli/issues" }, "directories": { - "bin": "./bin", "doc": "./doc", "lib": "./lib", "man": "./man" @@ -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", diff --git a/tap-snapshots/test/lib/commands/publish.js.test.cjs b/tap-snapshots/test/lib/commands/publish.js.test.cjs index 60717193b3cef..7465dd4e92198 100644 --- a/tap-snapshots/test/lib/commands/publish.js.test.cjs +++ b/tap-snapshots/test/lib/commands/publish.js.test.cjs @@ -114,7 +114,6 @@ Object { }, "description": "a package manager for JavaScript", "directories": Object { - "bin": "./bin", "doc": "./doc", "lib": "./lib", "man": "./man", diff --git a/test/bin/windows-shims.js b/test/bin/windows-shims.js index e06e41bf8901d..c41569f2d24f1 100644 --- a/test/bin/windows-shims.js +++ b/test/bin/windows-shims.js @@ -1,70 +1,100 @@ 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') @@ -72,62 +102,63 @@ const path = t.testdir({ 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, + }) }) }) - }) -} + } +})