From e69be2ac5c35e985732e2baa00b70d39332e4b9f Mon Sep 17 00:00:00 2001 From: isaacs Date: Thu, 25 Feb 2021 15:38:28 -0800 Subject: [PATCH] Get correct npm prefix on all Windows unix shells 1. Set the shebang to /usr/bin/env bash instead of /bin/sh (which might be dash or some other shell) 2. Use Unix-style line endings, not Windows-style (Cygwin accepts either, but mingw bash sometimes objects, and WSL bash always does) 3. Test against paths using wslpath if available, but still pass win32 paths to node.exe, since it is a Windows binary that only knows how to handle Windows paths. This makes npm as installed by the Node.js Windows MSI installer behave properly under WSL, Cygwin, MINGW Git Bash, and the internal MINGW Git Bash when posix CLI utilities are exposed to the cmd.exe shell. The test is not quite as comprehensive as I'd like. It runs on the various Windows bash implementations if they are found in their expected locations, skipping any that are not installed. Short of shipping mingw, cygwin, and wsl as test fixtures, I'm not sure how we could do much better, however. At least, we can use this test to assist debug and catch issues on Windows machines (ours or users who report problems). PR-URL: https://github.com/npm/cli/pull/2789 Credit: @isaacs Close: #2789 Reviewed-by: @nlf --- bin/npm | 45 +++++++------ bin/npx | 48 ++++++++------ test/bin/windows-shims.js | 135 ++++++++++++++++++++++++++++++++++++++ test/coverage-map.js | 4 ++ test/lib/doctor.js | 2 +- test/lib/npm.js | 13 ++-- 6 files changed, 198 insertions(+), 49 deletions(-) create mode 100644 test/bin/windows-shims.js diff --git a/bin/npm b/bin/npm index 4183703a7857e..a131a53543404 100755 --- a/bin/npm +++ b/bin/npm @@ -1,10 +1,10 @@ -#!/bin/sh +#!/usr/bin/env bash (set -o igncr) 2>/dev/null && set -o igncr; # cygwin encoding fix basedir=`dirname "$0"` case `uname` in - *CYGWIN*) basedir=`cygpath -w "$basedir"`;; + *CYGWIN*) basedir=`cygpath -w "$basedir"`;; esac NODE_EXE="$basedir/node.exe" @@ -15,23 +15,30 @@ if ! [ -x "$NODE_EXE" ]; then NODE_EXE=node fi -NPM_CLI_JS="$basedir/node_modules/npm/bin/npm-cli.js" +# 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" -case `uname` in - *MINGW*) - NPM_PREFIX=`"$NODE_EXE" "$NPM_CLI_JS" prefix -g` - NPM_PREFIX_NPM_CLI_JS="$NPM_PREFIX/node_modules/npm/bin/npm-cli.js" - if [ -f "$NPM_PREFIX_NPM_CLI_JS" ]; then - NPM_CLI_JS="$NPM_PREFIX_NPM_CLI_JS" - fi - ;; - *CYGWIN*) - NPM_PREFIX=`"$NODE_EXE" "$NPM_CLI_JS" prefix -g` - NPM_PREFIX_NPM_CLI_JS="$NPM_PREFIX/node_modules/npm/bin/npm-cli.js" - if [ -f "$NPM_PREFIX_NPM_CLI_JS" ]; then - NPM_CLI_JS="$NPM_PREFIX_NPM_CLI_JS" - fi - ;; -esac +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_PREFIX_NPM_CLI_JS="$NPM_PREFIX/node_modules/npm/bin/npm-cli.js" + +# a path that will fail -f test on any posix bash +NPM_WSL_PATH="/.." + +# WSL can run Windows binaries, so we have to give it the win32 path +# however, WSL bash tests against posix paths, so we need to construct that +# to know if npm is installed globally. +if [ `uname` = 'Linux' ] && type wslpath &>/dev/null ; then + NPM_WSL_PATH=`wslpath "$NPM_PREFIX_NPM_CLI_JS"` +fi +if [ -f "$NPM_PREFIX_NPM_CLI_JS" ] || [ -f "$NPM_WSL_PATH" ]; then + NPM_CLI_JS="$NPM_PREFIX_NPM_CLI_JS" +fi "$NODE_EXE" "$NPM_CLI_JS" "$@" diff --git a/bin/npx b/bin/npx index f43754d620a8a..4b58a104b9e42 100755 --- a/bin/npx +++ b/bin/npx @@ -1,4 +1,4 @@ -#!/bin/sh +#!/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. @@ -8,7 +8,7 @@ basedir=`dirname "$0"` case `uname` in - *CYGWIN*) basedir=`cygpath -w "$basedir"`;; + *CYGWIN*) basedir=`cygpath -w "$basedir"`;; esac NODE_EXE="$basedir/node.exe" @@ -16,24 +16,30 @@ if ! [ -x "$NODE_EXE" ]; then NODE_EXE=node fi -NPM_CLI_JS="$basedir/node_modules/npm/bin/npm-cli.js" -NPX_CLI_JS="$basedir/node_modules/npm/bin/npx-cli.js" - -case `uname` in - *MINGW*) - NPM_PREFIX=`"$NODE_EXE" "$NPM_CLI_JS" prefix -g` - NPM_PREFIX_NPX_CLI_JS="$NPM_PREFIX/node_modules/npm/bin/npx-cli.js" - if [ -f "$NPM_PREFIX_NPX_CLI_JS" ]; then - NPX_CLI_JS="$NPM_PREFIX_NPX_CLI_JS" - fi - ;; - *CYGWIN*) - NPM_PREFIX=`"$NODE_EXE" "$NPM_CLI_JS" prefix -g` - NPM_PREFIX_NPX_CLI_JS="$NPM_PREFIX/node_modules/npm/bin/npx-cli.js" - if [ -f "$NPM_PREFIX_NPX_CLI_JS" ]; then - NPX_CLI_JS="$NPM_PREFIX_NPX_CLI_JS" - fi - ;; -esac +# these paths are passed to node.exe, so they need 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)')" +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 +NPX_WSL_PATH="/.." + +# WSL can run Windows binaries, so we have to give it the win32 path +# however, WSL bash tests against posix paths, so we need to construct that +# to know if npm is installed globally. +if [ `uname` = 'Linux' ] && type wslpath &>/dev/null ; then + NPX_WSL_PATH=`wslpath "$NPM_PREFIX_NPX_CLI_JS"` +fi +if [ -f "$NPM_PREFIX_NPX_CLI_JS" ] || [ -f "$NPX_WSL_PATH" ]; then + NPX_CLI_JS="$NPM_PREFIX_NPX_CLI_JS" +fi "$NODE_EXE" "$NPX_CLI_JS" "$@" diff --git a/test/bin/windows-shims.js b/test/bin/windows-shims.js new file mode 100644 index 0000000000000..8d73e39f2c4d1 --- /dev/null +++ b/test/bin/windows-shims.js @@ -0,0 +1,135 @@ +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 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, '../..')), + }, + }, + // 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': ` + 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) + +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 + } + + 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 result = await spawn(bash, args, { + env: { PATH: path, npm_config_update_notifier: 'false' }, + cwd: path, + stdioString: true, + }) + 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'] + + const result = await spawn(bash, args, { + env: { PATH: path, npm_config_update_notifier: 'false' }, + cwd: path, + stdioString: true, + }) + 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, + }) + }) + }) +} diff --git a/test/coverage-map.js b/test/coverage-map.js index f247c051f0a2d..63f2a608e0ee3 100644 --- a/test/coverage-map.js +++ b/test/coverage-map.js @@ -7,6 +7,10 @@ const coverageMap = (filename) => { return glob.sync(`${dir}/**/*.js`) .map(f => relative(process.cwd(), f)) } + if (/windows-shims\.js$/.test(filename)) { + // this one doesn't provide any coverage nyc can track + return [] + } if (/^test\/(lib|bin)\//.test(filename)) return filename.replace(/^test\//, '') return [] diff --git a/test/lib/doctor.js b/test/lib/doctor.js index 8200493478fa1..eaa7ad72df8a5 100644 --- a/test/lib/doctor.js +++ b/test/lib/doctor.js @@ -487,7 +487,7 @@ test('node versions', t => { const dir = st.testdir({ cache: { one: 'one', - link: st.fixture('symlink', './one'), + link: st.fixture('symlink', './baddir'), unreadable: 'unreadable', baddir: {}, }, diff --git a/test/lib/npm.js b/test/lib/npm.js index 18bed36dec4e5..1f7a54e228a0e 100644 --- a/test/lib/npm.js +++ b/test/lib/npm.js @@ -1,5 +1,4 @@ const t = require('tap') -const fs = require('fs') // delete this so that we don't have configs from the fact that it // is being run by 'npm test' @@ -21,7 +20,7 @@ for (const env of Object.keys(process.env).filter(e => /^npm_/.test(e))) { delete process.env[env] } -const { resolve } = require('path') +const { resolve, dirname } = require('path') const actualPlatform = process.platform @@ -249,13 +248,11 @@ t.test('npm.load', t => { const node = actualPlatform === 'win32' ? 'node.exe' : 'node' const dir = t.testdir({ '.npmrc': 'foo = bar', + bin: t.fixture('symlink', dirname(process.execPath)), }) - // create manually to set the 'file' option in windows - fs.symlinkSync(process.execPath, resolve(dir, node), 'file') - const PATH = process.env.PATH || process.env.Path - process.env.PATH = dir + process.env.PATH = resolve(dir, 'bin') const { execPath, argv: processArgv } = process process.argv = [ node, @@ -294,7 +291,7 @@ t.test('npm.load', t => { [ 'verbose', 'node symlink', - resolve(dir, node), + resolve(dir, 'bin', node), ], [ 'timing', @@ -303,7 +300,7 @@ t.test('npm.load', t => { ], ]) logs.length = 0 - t.equal(process.execPath, resolve(dir, node)) + t.equal(process.execPath, resolve(dir, 'bin', node)) }) await npm.commands.ll([], (er) => {