From 26a0b811ff4d976c991b9bf54126793226fc5fcb Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Tue, 20 Jun 2023 17:52:43 -0700 Subject: [PATCH] fixup: add tests and prefix for ps1 scripts --- .github/workflows/ci.yml | 33 +++++ bin/npm.ps1 | 17 ++- bin/npx.ps1 | 17 ++- scripts/template-oss/ci.yml | 21 ++++ test/bin/windows-shims.js | 238 ++++++++++++++++++++++-------------- 5 files changed, 229 insertions(+), 97 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 485b9bc9245c9..cadbe0de095a8 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/bin/npm.ps1 b/bin/npm.ps1 index 419992b62c36f..f2f236adc23db 100644 --- a/bin/npm.ps1 +++ b/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 diff --git a/bin/npx.ps1 b/bin/npx.ps1 index e6038695c5a92..437e2a7b74c3a 100644 --- a/bin/npx.ps1 +++ b/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 diff --git a/scripts/template-oss/ci.yml b/scripts/template-oss/ci.yml index ec8e9540d648d..7aab115076806 100644 --- a/scripts/template-oss/ci.yml +++ b/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:\cygwin64 + - name: Run Windows Shims Tests + run: {{rootNpmPath}} test --ignore-scripts -- test/bin/windows-shims.js --no-coverage + env: + WINDOWS_SHIMS_TEST: true + - name: Check Git Status + run: node scripts/git-dirty.js diff --git a/test/bin/windows-shims.js b/test/bin/windows-shims.js index 13005ccf642ee..d855a9b3f9b31 100644 --- a/test/bin/windows-shims.js +++ b/test/bin/windows-shims.js @@ -1,58 +1,75 @@ 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, basename, 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 quotePath = (cmd) => 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, @@ -60,9 +77,7 @@ t.test('basic', async t => { 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,115 @@ 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 { - 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) { - throw new Error('not installed') + for (const shim of Object.keys(SHIMS)) { + chmodSync(join(path, shim), 0o755) + } + + const { ProgramFiles = '/', SystemRoot = '/', NYC_CONFIG, WINDOWS_SHIMS_TEST } = process.env + const skipDefault = WINDOWS_SHIMS_TEST || process.platform === 'win32' + ? null : 'test not relevant on platform' + + const shells = Object.entries({ + cmd: 'cmd', + pwsh: 'pwsh', + git: join(ProgramFiles, 'Git', 'bin', 'bash.exe'), + 'user git': join(ProgramFiles, 'Git', 'usr', 'bin', 'bash.exe'), + wsl: join(SystemRoot, 'System32', 'bash.exe'), + cygwin: resolve(SystemRoot, '/', 'cygwin64', 'bin', 'bash.exe'), + }).map(([name, cmd]) => { + let skip = skipDefault + const isBash = cmd.endsWith('bash.exe') + const testName = `${name} ${isBash ? 'bash' : ''}`.trim() + + if (!skip) { + if (isBash) { + 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(cmd, ['-l', '-c', 'exit 0']).status !== 0) { + throw new Error('not installed') + } + if (cmd.includes('cygwin') && NYC_CONFIG) { + throw new Error('does not play nicely with nyc') + } + } catch (err) { + skip = err.message + } + } else { + try { + cmd = which(cmd) + } catch { + skip = 'not installed' } - } catch { - skip = 'not installed' } } - return { name, bash, skip } + + return { + cmd, + name: testName, + skip: skip ? `${testName} - ${skip}` : null, + } }) - for (const { name, bash, skip } of bashes) { - if (skip) { - t.skip(name, { diagnostic: true, bin: bash, reason: skip }) - continue + // ensure that all tests are either run or skipped + t.plan(shells.length) + + 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 + } + + const matchSpawn = (t, cmd, bin = '') => { + const args = [] + const opts = {} + + switch (basename(cmd).toLowerCase()) { + case 'cmd.exe': + cmd = `${bin}.cmd` + break + case 'pwsh.exe': + cmd = quotePath(cmd) + args.push(`${bin}.ps1`) + opts.shell = true + break + case 'bash.exe': + // only cygwin *requires* the -l, but the others are ok with it + args.push('-l', bin) + break + default: + throw new Error('unknown shell') } - 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, - }) - }) + const isNpm = bin === 'npm' + t.match(spawn(cmd, [...args, isNpm ? 'help' : '--version'], opts), { + status: 0, + signal: null, + stderr: '', + stdout: isNpm ? `npm@${version} ${ROOT}` : version, + }, 'command result') + } + + for (const { cmd, skip, name } of shells) { + t.test(name, t => { + if (skip) { + if (WINDOWS_SHIMS_TEST) { + t.fail(skip) + } else { + t.skip(skip) + } + return t.end() } + t.plan(2) + matchSpawn(t, cmd, 'npm') + matchSpawn(t, cmd, 'npx') }) } })