fixup: add tests and prefix for ps1 scripts

.github/workflows/ci.yml

@@ -150,3 +150,36 @@ jobs:
         run: node . test -w smoke-tests --ignore-scripts
       - name: Check Git Status
         run: node scripts/git-dirty.js
+
+  windows-shims:
+    name: Windows Shims Tests
+    runs-on: windows-latest
+    defaults:
+      run:
+        shell: cmd
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v3
+      - name: Setup Git User
+        run: |
+          git config --global user.email "npm-cli+bot@github.com"
+          git config --global user.name "npm CLI robot"
+      - name: Setup Node
+        uses: actions/setup-node@v3
+        with:
+          node-version: 18.x
+          cache: npm
+      - name: Reset Deps
+        run: node . run resetdeps
+      - name: Setup WSL
+        uses: Vampire/setup-wsl@v2.0.1
+      - name: Set up Cygwin
+        uses: egor-tensin/setup-cygwin@v4.0.1
+        with:
+          install-dir: C:\Windows\cygwin64
+      - name: Run Windows Shims Tests
+        run: node . test --ignore-scripts -- test/bin/windows-shims.js --no-coverage
+        env:
+          WINDOWS_SHIMS_TEST: fail
+      - name: Check Git Status
+        run: node scripts/git-dirty.js

bin/npm.ps1

@@ -9,18 +9,27 @@ if ($PSVersionTable.PSVersion -lt "6.0" -or $IsWindows) {
 }
 $ret=0
 
-$nodebin = $(Get-Command "node$exe" -ErrorAction SilentlyContinue -ErrorVariable F).Source
+$nodeexe = "node$exe"
+$nodebin = $(Get-Command $nodeexe -ErrorAction SilentlyContinue -ErrorVariable F).Source
 if ($nodebin -eq $null) {
-  Write-Host "node$exe not found."
+  Write-Host "$nodeexe not found."
   exit 1
 }
 $nodedir = $(New-Object -ComObject Scripting.FileSystemObject).GetFile("$nodebin").ParentFolder.Path
 
+$npmclijs="$nodedir/node_modules/npm/bin/npm-cli.js"
+$npmprefix=(& $nodeexe $npmclijs prefix -g)
+if ($LASTEXITCODE -ne 0) {
+  Write-Host "Could not determine Node.js install directory"
+  exit 1
+}
+$npmprefixclijs="$npmprefix/node_modules/npm/bin/npm-cli.js"
+
 # Support pipeline input
 if ($MyInvocation.ExpectingInput) {
-  $input | & "node$exe" "$nodedir/node_modules/npm/bin/npm-cli.js" $args
+  $input | & $nodeexe $npmprefixclijs $args
 } else {
-  & "node$exe" "$nodedir/node_modules/npm/bin/npm-cli.js" $args
+  & $nodeexe $npmprefixclijs $args
 }
 $ret=$LASTEXITCODE
 exit $ret

bin/npx.ps1

@@ -9,18 +9,27 @@ if ($PSVersionTable.PSVersion -lt "6.0" -or $IsWindows) {
 }
 $ret=0
 
-$nodebin = $(Get-Command "node$exe" -ErrorAction SilentlyContinue -ErrorVariable F).Source
+$nodeexe = "node$exe"
+$nodebin = $(Get-Command $nodeexe -ErrorAction SilentlyContinue -ErrorVariable F).Source
 if ($nodebin -eq $null) {
-  Write-Host "node$exe not found."
+  Write-Host "$nodeexe not found."
   exit 1
 }
 $nodedir = $(New-Object -ComObject Scripting.FileSystemObject).GetFile("$nodebin").ParentFolder.Path
 
+$npmclijs="$nodedir/node_modules/npm/bin/npm-cli.js"
+$npmprefix=(& $nodeexe $npmclijs prefix -g)
+if ($LASTEXITCODE -ne 0) {
+  Write-Host "Could not determine Node.js install directory"
+  exit 1
+}
+$npmprefixclijs="$npmprefix/node_modules/npm/bin/npx-cli.js"
+
 # Support pipeline input
 if ($MyInvocation.ExpectingInput) {
-  $input | & "node$exe" "$nodedir/node_modules/npm/bin/npx-cli.js" $args
+  $input | & $nodeexe $npmprefixclijs $args
 } else {
-  & "node$exe" "$nodedir/node_modules/npm/bin/npx-cli.js" $args
+  & $nodeexe $npmprefixclijs $args
 }
 $ret=$LASTEXITCODE
 exit $ret

scripts/template-oss/ci.yml

@@ -11,3 +11,24 @@
         run: {{rootNpmPath}} test -w smoke-tests --ignore-scripts
       - name: Check Git Status
         run: node scripts/git-dirty.js
+
+  windows-shims:
+    name: Windows Shims Tests
+    runs-on: windows-latest
+    defaults:
+      run:
+        shell: cmd
+    steps:
+      {{> stepsSetup }}
+      - name: Setup WSL
+        uses: Vampire/setup-wsl@v2.0.1
+      - name: Set up Cygwin
+        uses: egor-tensin/setup-cygwin@v4.0.1
+        with:
+          install-dir: C:\Windows\cygwin64
+      - name: Run Windows Shims Tests
+        run: {{rootNpmPath}} test --ignore-scripts -- test/bin/windows-shims.js --no-coverage
+        env:
+          WINDOWS_SHIMS_TEST: fail
+      - name: Check Git Status
+        run: node scripts/git-dirty.js

test/bin/windows-shims.js

@@ -1,68 +1,83 @@
 const t = require('tap')
-const spawn = require('@npmcli/promise-spawn')
 const { spawnSync } = require('child_process')
-const { resolve, join } = require('path')
-const { readFileSync, chmodSync } = require('fs')
+const { resolve, join, extname, sep } = require('path')
+const { readFileSync, chmodSync, readdirSync } = require('fs')
 const Diff = require('diff')
+const { sync: which } = require('which')
 const { version } = require('../../package.json')
 
-const root = resolve(__dirname, '../..')
-const npmShim = join(root, 'bin/npm')
-const npxShim = join(root, 'bin/npx')
+const ROOT = resolve(__dirname, '../..')
+const BIN = join(ROOT, 'bin')
+const SHIMS = readdirSync(BIN).reduce((acc, shim) => {
+  if (extname(shim) !== '.js') {
+    acc[shim] = readFileSync(join(BIN, shim), 'utf-8')
+  }
+  return acc
+}, {})
+
+// windows requires each segment of a command path to be quoted when using shell: true
+const quoteWhich = (cmd) => which(cmd)
+  .split(sep)
+  .map(p => p.includes(' ') ? `"${p}"` : p)
+  .join(sep)
 
-t.test('npm vs npx', t => {
+t.test('shim contents', 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())
+  const diffFiles = (npm, npx) => Diff.diffChars(npm, npx)
+    .filter(v => v.added || v.removed)
+    .reduce((acc, v) => {
+      if (v.value.length === 1) {
+        acc.letters.add(v.value.toUpperCase())
+      } else {
+        acc.diff.push(v.value)
+      }
+      return acc
+    }, { diff: [], letters: new Set() })
+
+  t.plan(3)
 
   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')
+    const { diff, letters } = diffFiles(SHIMS.npm, SHIMS.npx)
+    t.match(diff[0].split('\n').reverse().join(''), /^NPX_CLI_JS=/, 'has NPX_CLI')
+    t.equal(diff.length, 1)
+    t.strictSame([...letters], ['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')
+    const { diff, letters } = diffFiles(SHIMS['npm.cmd'], SHIMS['npx.cmd'])
+    t.match(diff[0], /^SET "NPX_CLI_JS=/, 'has NPX_CLI')
+    t.equal(diff.length, 1)
+    t.strictSame([...letters], ['M', 'X'], 'all other changes are m->x')
     t.end()
   })
 
-  t.end()
+  t.test('pwsh', t => {
+    const { diff, letters } = diffFiles(SHIMS['npm.ps1'], SHIMS['npx.ps1'])
+    t.equal(diff.length, 0)
+    t.strictSame([...letters], ['M', 'X'], 'all other changes are m->x')
+    t.end()
+  })
 })
 
-t.test('basic', async t => {
-  if (process.platform !== 'win32') {
-    t.comment('test only relevant on windows')
-    return
-  }
-
+t.test('run shims', t => {
   const path = t.testdir({
+    ...SHIMS,
     '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', root),
+        npm: t.fixture('symlink', ROOT),
       },
     },
     // 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')
-          `,
+          'npx-cli.js': `throw new Error('this should not be called')`,
           'npm-cli.js': `
             const assert = require('assert')
             const args = process.argv.slice(2)
@@ -76,70 +91,96 @@ t.test('basic', async t => {
     },
   })
 
-  chmodSync(join(path, 'npm'), 0o755)
-  chmodSync(join(path, 'npx'), 0o755)
-
-  const { ProgramFiles, SystemRoot, NYC_CONFIG } = process.env
-  const gitBash = join(ProgramFiles, 'Git', 'bin', 'bash.exe')
-  const gitUsrBinBash = join(ProgramFiles, 'Git', 'usr', 'bin', 'bash.exe')
-  const wslBash = join(SystemRoot, 'System32', 'bash.exe')
-  const cygwinBash = join(SystemRoot, '/', 'cygwin64', 'bin', 'bash.exe')
-
-  const bashes = Object.entries({
-    'wsl bash': wslBash,
-    'git bash': gitBash,
-    'git internal bash': gitUsrBinBash,
-    'cygwin bash': cygwinBash,
-  }).map(([name, bash]) => {
-    let skip
-    if (bash === cygwinBash && NYC_CONFIG) {
-      skip = 'does not play nicely with NYC, run without coverage'
-    } else {
+  const spawn = (cmd, args, opts) => {
+    const result = spawnSync(cmd, args, {
+      // don't hit the registry for the update check
+      env: { PATH: path, npm_config_update_notifier: 'false' },
+      cwd: path,
+      windowsHide: true,
+      ...opts,
+    })
+    result.stdout = result.stdout.toString().trim()
+    result.stderr = result.stderr.toString().trim()
+    return result
+  }
+
+  for (const shim of Object.keys(SHIMS)) {
+    chmodSync(join(path, shim), 0o755)
+  }
+
+  const { ProgramFiles = '', SystemRoot = '', NYC_CONFIG, WINDOWS_SHIMS_TEST } = process.env
+  const failOnMissing = WINDOWS_SHIMS_TEST === 'fail'
+  const defaultSkip = process.platform === 'win32' ? null : 'test on relevant on windows'
+
+  const matchSpawn = (t, cmd, bin = '', { skip = defaultSkip, name } = {}) => {
+    const testName = `${name || cmd} ${bin}`.trim()
+    if (skip) {
+      if (failOnMissing) {
+        t.fail(testName)
+      } else {
+        t.skip(`${testName} - ${skip}`)
+      }
+      return
+    }
+    t.test(testName, t => {
+      t.plan(1)
+      const isNpm = testName.includes('npm')
+      const binArg = isNpm ? 'help' : '--version'
+      const args = []
+      const opts = {}
+      if (cmd.endsWith('.cmd')) {
+        args.push(binArg)
+      } else if (cmd === 'pwsh') {
+        cmd = quoteWhich(cmd)
+        args.push(`${bin}.ps1`, binArg)
+        opts.shell = true
+      } else if (cmd.endsWith('bash.exe')) {
+        // only cygwin *requires* the -l, but the others are ok with it
+        args.push('-l', bin, binArg)
+      }
+      t.match(spawn(cmd, args, opts), {
+        status: 0,
+        signal: null,
+        stderr: '',
+        stdout: isNpm ? `npm@${version} ${ROOT}` : version,
+      }, 'command output is correct')
+    })
+  }
+
+  // ensure that all tests are either run or skipped
+  t.plan(12)
+
+  matchSpawn(t, 'npm.cmd')
+  matchSpawn(t, 'npx.cmd')
+  matchSpawn(t, 'pwsh', 'npm')
+  matchSpawn(t, 'pwsh', 'npx')
+
+  const bashes = [
+    { name: 'git', cmd: join(ProgramFiles, 'Git', 'bin', 'bash.exe') },
+    { name: 'user git', cmd: join(ProgramFiles, 'Git', 'usr', 'bin', 'bash.exe') },
+    { name: 'wsl', cmd: join(SystemRoot, 'System32', 'bash.exe') },
+    {
+      name: 'cygwin',
+      cmd: join(SystemRoot, '/', 'cygwin64', 'bin', 'bash.exe'),
+      skip: NYC_CONFIG ? 'does not play nicely with nyc' : undefined,
+    },
+  ].map(({ name, cmd, skip = defaultSkip }) => {
+    if (!skip) {
       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.
-        if (spawnSync(bash, ['-l', '-c', 'exit 0']).status !== 0) {
+        if (spawnSync(cmd, ['-l', '-c', 'exit 0']).status !== 0) {
           throw new Error('not installed')
         }
-      } catch {
-        skip = 'not installed'
+      } catch (err) {
+        skip = err.message
       }
     }
-    return { name, bash, skip }
+    return { cmd, skip, name: `${name} bash` }
   })
 
-  for (const { name, bash, skip } of bashes) {
-    if (skip) {
-      t.skip(name, { diagnostic: true, bin: bash, reason: skip })
-      continue
-    }
-
-    await t.test(name, async t => {
-      const bins = Object.entries({
-        // should have loaded this instance of npm we symlinked in
-        npm: [['help'], `npm@${version} ${root}`],
-        npx: [['--version'], version],
-      })
-
-      for (const [binName, [cmdArgs, stdout]] of bins) {
-        await t.test(binName, async t => {
-          // only cygwin *requires* the -l, but the others are ok with it
-          const args = ['-l', binName, ...cmdArgs]
-          const result = await spawn(bash, args, {
-            // don't hit the registry for the update check
-            env: { PATH: path, npm_config_update_notifier: 'false' },
-            cwd: path,
-          })
-          t.match(result, {
-            cmd: bash,
-            args: args,
-            code: 0,
-            signal: null,
-            stderr: String,
-            stdout,
-          })
-        })
-      }
-    })
+  for (const { cmd, skip, name } of bashes) {
+    matchSpawn(t, cmd, 'npm', { name, skip })
+    matchSpawn(t, cmd, 'npx', { name, skip })
   }
 })