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`
npm · Jun 14, 2023 · 67aeffb · 67aeffb
1 parent 29622c1
commit 67aeffb
Show file tree

Hide file tree

Showing 8 changed files with 126 additions and 103 deletions.
diff --git a/bin/node-gyp-bin/node-gyp b/bin/node-gyp-bin/node-gyp
diff --git a/bin/node-gyp-bin/node-gyp.cmd b/bin/node-gyp-bin/node-gyp.cmd
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -1,9 +1,13 @@
 const t = require('tap')
+const spawn = require('@npmcli/promise-spawn')
+const { spawnSync } = require('child_process')
+const { resolve } = require('path')
+const { readFileSync, chmodSync } = require('fs')
+const Diff = require('diff')
+const { version } = require('../../package.json')
 
-if (process.platform !== 'win32') {
-  t.plan(0, 'test only relevant on windows')
-  process.exit(0)
-}
+const npmShim = resolve(__dirname, '../../bin/npm')
+const npxShim = resolve(__dirname, '../../bin/npx')
 
 const has = path => {
   try {
@@ -24,110 +28,137 @@ const has = path => {
   }
 }
 
-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,
+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()
 })
 
-const npmShim = resolve(__dirname, '../../bin/npm')
-const npxShim = resolve(__dirname, '../../bin/npx')
+t.test('basic', t => {
+  if (process.platform !== 'win32') {
+    t.plan(0, 'test only relevant on windows')
+    return
+  }
 
-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, '../..')),
+  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 => {
+    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,
+        })
       })
     })
-  })
-}
+  }
+})