From daaf4619c85ecf62346770735cfa8e2ddecbef8b Mon Sep 17 00:00:00 2001 From: Gar Date: Wed, 10 Aug 2022 09:34:24 -0700 Subject: [PATCH] fix: ignore global prefix if --prefix is used (#5291) When `--prefix` is used, both the local and global prefix values are set to be identical. This is functionally broken because their directory structures are inherently different (for instance, in posix the tree is in `lib/node_modules` in the global prefix). This commit makes npm exec ignore the global folders if it detects both local and global prefix are identical. --- lib/commands/exec.js | 9 +++++- test/lib/commands/exec.js | 43 ++++++++++++++++++++++++++++- workspaces/libnpmexec/lib/index.js | 6 ++-- workspaces/libnpmexec/test/index.js | 16 +++++++---- 4 files changed, 63 insertions(+), 11 deletions(-) diff --git a/lib/commands/exec.js b/lib/commands/exec.js index ddbb5f7cf9d7..a77a6326c00f 100644 --- a/lib/commands/exec.js +++ b/lib/commands/exec.js @@ -34,6 +34,7 @@ class Exec extends BaseCommand { const args = [..._args] const call = this.npm.config.get('call') + let globalPath const { flatOptions, localBin, @@ -44,6 +45,12 @@ class Exec extends BaseCommand { const scriptShell = this.npm.config.get('script-shell') || undefined const packages = this.npm.config.get('package') const yes = this.npm.config.get('yes') + // --prefix sets both of these to the same thing, meaning the global prefix + // is invalid (i.e. no lib/node_modules). This is not a trivial thing to + // untangle and fix so we work around it here. + if (this.npm.localPrefix !== this.npm.globalPrefix) { + globalPath = path.resolve(globalDir, '..') + } if (call && _args.length) { throw this.usageError() @@ -59,7 +66,7 @@ class Exec extends BaseCommand { localBin, locationMsg, globalBin, - globalPath: path.resolve(globalDir, '..'), + globalPath, output, packages, path: localPrefix, diff --git a/test/lib/commands/exec.js b/test/lib/commands/exec.js index 6e4c3e2246a6..f2bccf9e0d85 100644 --- a/test/lib/commands/exec.js +++ b/test/lib/commands/exec.js @@ -28,6 +28,7 @@ t.test('registry package', async t => { const { npm } = await loadMockNpm(t, { config: { + audit: false, yes: true, }, prefixDir: { @@ -43,7 +44,46 @@ t.test('registry package', async t => { }) await registry.package({ - times: 2, + manifest, + tarballs: { + '1.0.0': path.join(npm.prefix, 'npm-exec-test'), + } }) + + await npm.exec('exec', ['@npmcli/npx-test']) + const exists = await fs.stat(path.join(npm.prefix, 'npm-exec-test-success')) + t.ok(exists.isFile(), 'bin ran, creating file') +}) + +t.test('--prefix', async t => { + const registry = new MockRegistry({ + tap: t, + registry: 'https://registry.npmjs.org/', + }) + + const manifest = registry.manifest({ name: '@npmcli/npx-test' }) + manifest.versions['1.0.0'].bin = { 'npx-test': 'index.js' } + + const { npm } = await loadMockNpm(t, { + config: { + audit: false, + yes: true, + }, + prefixDir: { + 'npm-exec-test': { + 'package.json': JSON.stringify(manifest), + 'index.js': `#!/usr/bin/env node + require('fs').writeFileSync('npm-exec-test-success', '')`, + }, + }, + globals: ({ prefix }) => ({ + 'process.cwd': () => prefix, + }), + }) + + // This is what `--prefix` does + npm.globalPrefix = npm.localPrefix + + await registry.package({ manifest, tarballs: { '1.0.0': path.join(npm.prefix, 'npm-exec-test'), @@ -65,6 +105,7 @@ t.test('workspaces', async t => { const { npm } = await loadMockNpm(t, { config: { + audit: false, yes: true, workspace: ['workspace-a'], }, diff --git a/workspaces/libnpmexec/lib/index.js b/workspaces/libnpmexec/lib/index.js index 0b0565ca9207..15d5ba4eeca1 100644 --- a/workspaces/libnpmexec/lib/index.js +++ b/workspaces/libnpmexec/lib/index.js @@ -82,7 +82,7 @@ const exec = async (opts) => { localBin = resolve('./node_modules/.bin'), locationMsg = undefined, globalBin = '', - globalPath = '', + globalPath, output, // dereference values because we manipulate it later packages: [...packages] = [], @@ -120,7 +120,7 @@ const exec = async (opts) => { if (localBinPath) { binPaths.push(localBinPath) return await run() - } else if (await fileExists(`${globalBin}/${args[0]}`)) { + } else if (globalPath && await fileExists(`${globalBin}/${args[0]}`)) { binPaths.push(globalBin) return await run() } @@ -155,7 +155,7 @@ const exec = async (opts) => { args[0] = getBinFromManifest(commandManifest) - if (needInstall.length > 0) { + if (needInstall.length > 0 && globalPath) { // See if the package is installed globally, and run the translated bin const globalArb = new Arborist({ ...flatOptions, path: globalPath, global: true }) const globalTree = await globalArb.loadActual() diff --git a/workspaces/libnpmexec/test/index.js b/workspaces/libnpmexec/test/index.js index 578221cc070b..3e218b75cbe5 100644 --- a/workspaces/libnpmexec/test/index.js +++ b/workspaces/libnpmexec/test/index.js @@ -517,6 +517,7 @@ t.test('global space pkg', async t => { }, }) const globalBin = resolve(path, 'global/node_modules/.bin') + const globalPath = resolve(path, 'global') const runPath = path const executable = resolve(path, 'global/node_modules/a') @@ -531,6 +532,7 @@ t.test('global space pkg', async t => { ...baseOpts, args: ['a', 'resfile'], globalBin, + globalPath, path, runPath, }) @@ -588,12 +590,13 @@ t.test('run from registry - no local packages', async t => { const testdir = t.testdir({ cache: {}, npxCache: {}, + global: { + lib: {}, + bin: {}, + }, work: {}, }) const path = resolve(testdir, 'work') - const runPath = path - const cache = resolve(testdir, 'cache') - const npxCache = resolve(testdir, 'npxCache') t.throws( () => fs.statSync(resolve(path, 'index.js')), @@ -604,10 +607,11 @@ t.test('run from registry - no local packages', async t => { await libexec({ ...baseOpts, args: ['@ruyadorno/create-index'], - cache, - npxCache, + cache: resolve(testdir, 'cache'), + globalPath: resolve(testdir, 'global'), + npxCache: resolve(testdir, 'npxCache'), path, - runPath, + runPath: path, }) t.ok(fs.statSync(resolve(path, 'index.js')).isFile(), 'ran create pkg')