Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
wraithgar committed Aug 10, 2022
1 parent a4808fb commit daaf461
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 11 deletions.
9 changes: 8 additions & 1 deletion lib/commands/exec.js
Expand Up @@ -34,6 +34,7 @@ class Exec extends BaseCommand {

const args = [..._args]
const call = this.npm.config.get('call')
let globalPath
const {
flatOptions,
localBin,
Expand All @@ -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()
Expand All @@ -59,7 +66,7 @@ class Exec extends BaseCommand {
localBin,
locationMsg,
globalBin,
globalPath: path.resolve(globalDir, '..'),
globalPath,
output,
packages,
path: localPrefix,
Expand Down
43 changes: 42 additions & 1 deletion test/lib/commands/exec.js
Expand Up @@ -28,6 +28,7 @@ t.test('registry package', async t => {

const { npm } = await loadMockNpm(t, {
config: {
audit: false,
yes: true,
},
prefixDir: {
Expand All @@ -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'),
Expand All @@ -65,6 +105,7 @@ t.test('workspaces', async t => {

const { npm } = await loadMockNpm(t, {
config: {
audit: false,
yes: true,
workspace: ['workspace-a'],
},
Expand Down
6 changes: 3 additions & 3 deletions workspaces/libnpmexec/lib/index.js
Expand Up @@ -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] = [],
Expand Down Expand Up @@ -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()
}
Expand Down Expand Up @@ -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()
Expand Down
16 changes: 10 additions & 6 deletions workspaces/libnpmexec/test/index.js
Expand Up @@ -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')
Expand All @@ -531,6 +532,7 @@ t.test('global space pkg', async t => {
...baseOpts,
args: ['a', 'resfile'],
globalBin,
globalPath,
path,
runPath,
})
Expand Down Expand Up @@ -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')),
Expand All @@ -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')
Expand Down

0 comments on commit daaf461

Please sign in to comment.