Skip to content

Commit

Permalink
Get correct npm prefix on all Windows unix shells
Browse files Browse the repository at this point in the history
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: #2789
Credit: @isaacs
Close: #2789
Reviewed-by: @nlf
  • Loading branch information
isaacs authored and ruyadorno committed Mar 4, 2021
1 parent 4a5dd3a commit e69be2a
Show file tree
Hide file tree
Showing 6 changed files with 198 additions and 49 deletions.
45 changes: 26 additions & 19 deletions 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"
Expand All @@ -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" "$@"
48 changes: 27 additions & 21 deletions 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.
Expand All @@ -8,32 +8,38 @@
basedir=`dirname "$0"`

case `uname` in
*CYGWIN*) basedir=`cygpath -w "$basedir"`;;
*CYGWIN*) basedir=`cygpath -w "$basedir"`;;
esac

NODE_EXE="$basedir/node.exe"
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" "$@"
135 changes: 135 additions & 0 deletions 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,
})
})
})
}
4 changes: 4 additions & 0 deletions test/coverage-map.js
Expand Up @@ -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 []
Expand Down
2 changes: 1 addition & 1 deletion test/lib/doctor.js
Expand Up @@ -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: {},
},
Expand Down
13 changes: 5 additions & 8 deletions 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'
Expand All @@ -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

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -294,7 +291,7 @@ t.test('npm.load', t => {
[
'verbose',
'node symlink',
resolve(dir, node),
resolve(dir, 'bin', node),
],
[
'timing',
Expand All @@ -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) => {
Expand Down

0 comments on commit e69be2a

Please sign in to comment.