From dd6530eef99d2868b1fc4a652d1ad95ce613f38f Mon Sep 17 00:00:00 2001 From: Ruy Adorno Date: Wed, 30 Mar 2022 16:37:27 -0400 Subject: [PATCH] fix(libnpmexec): workspaces support Fixes the proper path location to use when targetting specific workspaces. Makes it so that libnpmexec is always reading from the Arborist actual tree instead of reading `node_modules` from the file system when retrieving local package data. Fixes: https://github.com/npm/cli/issues/3520 Relates to: https://github.com/npm/cli/issues/4619 Relates to: https://github.com/npm/statusboard/issues/403 --- lib/commands/exec.js | 8 +- test/lib/commands/exec.js | 274 +++++++++++++++--- workspaces/libnpmexec/lib/index.js | 78 +++-- workspaces/libnpmexec/lib/manifest-missing.js | 19 -- workspaces/libnpmexec/test/index.js | 76 +++++ .../libnpmexec/test/manifest-missing.js | 32 -- 6 files changed, 371 insertions(+), 116 deletions(-) delete mode 100644 workspaces/libnpmexec/lib/manifest-missing.js delete mode 100644 workspaces/libnpmexec/test/manifest-missing.js diff --git a/lib/commands/exec.js b/lib/commands/exec.js index 5e6a94296d287..f764cea528adb 100644 --- a/lib/commands/exec.js +++ b/lib/commands/exec.js @@ -48,10 +48,8 @@ class Exec extends BaseCommand { static ignoreImplicitWorkspace = false static isShellout = true - async exec (_args, { locationMsg, path, runPath } = {}) { - if (!path) { - path = this.npm.localPrefix - } + async exec (_args, { locationMsg, runPath } = {}) { + const path = this.npm.localPrefix if (!runPath) { runPath = process.cwd() @@ -95,7 +93,7 @@ class Exec extends BaseCommand { for (const path of this.workspacePaths) { const locationMsg = await getLocationMsg({ color, path }) - await this.exec(args, { locationMsg, path, runPath: path }) + await this.exec(args, { locationMsg, runPath: path }) } } } diff --git a/test/lib/commands/exec.js b/test/lib/commands/exec.js index 1f7230d25b654..6b57bf6666e1e 100644 --- a/test/lib/commands/exec.js +++ b/test/lib/commands/exec.js @@ -190,9 +190,14 @@ t.test('npx foo, bin already exists globally', async t => { t.test('npm exec foo, already present locally', async t => { const path = t.testdir() + const pkg = { name: 'foo', version: '1.2.3', bin: { foo: 'foo' } } npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map([['foo', { name: 'foo', version: '1.2.3' }]]), + inventory: { + query () { + return new Set([{ ...pkg, package: pkg }]) + }, + }, } MANIFESTS.foo = { name: 'foo', @@ -339,10 +344,18 @@ t.test('npm exec foo, not present locally or in central loc', async t => { const installDir = resolve('npx-cache-dir/f7fbba6e0636f890') npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } ARB_ACTUAL_TREE[installDir] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } MANIFESTS.foo = { name: 'foo', @@ -375,12 +388,21 @@ t.test('npm exec foo, not present locally or in central loc', async t => { t.test('npm exec foo, not present locally but in central loc', async t => { const path = t.testdir() const installDir = resolve('npx-cache-dir/f7fbba6e0636f890') + const pkg = { name: 'foo', version: '1.2.3', bin: { foo: 'foo' } } npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } ARB_ACTUAL_TREE[installDir] = { - children: new Map([['foo', { name: 'foo', version: '1.2.3' }]]), + inventory: { + query () { + return new Set([{ ...pkg, package: pkg }]) + }, + }, } MANIFESTS.foo = { name: 'foo', @@ -413,12 +435,21 @@ t.test('npm exec foo, not present locally but in central loc', async t => { t.test('npm exec foo, present locally but wrong version', async t => { const path = t.testdir() const installDir = resolve('npx-cache-dir/2badf4630f1cfaad') + const pkg = { name: 'foo', version: '1.2.3', bin: { foo: 'foo' } } npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } ARB_ACTUAL_TREE[installDir] = { - children: new Map([['foo', { name: 'foo', version: '1.2.3' }]]), + inventory: { + query () { + return new Set([{ ...pkg, package: pkg }]) + }, + }, } MANIFESTS['foo@2.x'] = { name: 'foo', @@ -448,11 +479,63 @@ t.test('npm exec foo, present locally but wrong version', async t => { ]) }) +t.test('npm exec foo, present locally but outdated version', async t => { + const path = t.testdir() + const installDir = resolve('npx-cache-dir/f7fbba6e0636f890') + const pkg = { name: 'foo', version: '1.2.3', bin: { foo: 'foo' } } + npm.localPrefix = path + ARB_ACTUAL_TREE[path] = { + inventory: { + query () { + return new Set() + }, + }, + } + ARB_ACTUAL_TREE[installDir] = { + inventory: { + query () { + return new Set([{ ...pkg, package: pkg }]) + }, + }, + } + MANIFESTS.foo = { + name: 'foo', + version: '2.3.4', + bin: { + foo: 'foo', + }, + _from: 'foo@2.x', + } + await exec.exec(['foo', 'one arg', 'two arg']) + t.strictSame(MKDIRPS, [installDir], 'need to make install dir') + t.match(ARB_CTOR, [{ path }]) + t.match(ARB_REIFY, [{ add: ['foo'], legacyPeerDeps: false }], 'need to add foo@2.x') + t.equal(PROGRESS_ENABLED, true, 'progress re-enabled') + const PATH = `${resolve(installDir, 'node_modules', '.bin')}${delimiter}${process.env.PATH}` + t.match(RUN_SCRIPTS, [ + { + pkg: { scripts: { npx: 'foo' } }, + args: ['one arg', 'two arg'], + banner: false, + path: process.cwd(), + stdioString: true, + event: 'npx', + env: { PATH }, + stdio: 'inherit', + }, + ]) +}) + t.test('npm exec --package=foo bar', async t => { const path = t.testdir() + const pkg = { name: 'foo', version: '1.2.3', bin: { foo: 'foo' } } npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map([['foo', { name: 'foo', version: '1.2.3' }]]), + inventory: { + query () { + return new Set([{ ...pkg, package: pkg }]) + }, + }, } MANIFESTS.foo = { name: 'foo', @@ -499,9 +582,18 @@ t.test('npm exec @foo/bar -- --some=arg, locally installed', async t => { }, }, }) + const pkg = { + name: '@foo/bar', + version: '1.2.3', + bin: { foo: 'foo', bar: 'bar' }, + } npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map([['@foo/bar', { name: '@foo/bar', version: '1.2.3' }]]), + inventory: { + query () { + return new Set([{ ...pkg, package: pkg }]) + }, + }, } MANIFESTS['@foo/bar'] = foobarManifest await exec.exec(['@foo/bar', '--some=arg']) @@ -526,7 +618,7 @@ t.test('npm exec @foo/bar -- --some=arg, locally installed', async t => { t.test( 'npm exec @foo/bar, with same bin alias and no unscoped named bin, locally installed', async t => { - const foobarManifest = { + const pkg = { name: '@foo/bar', version: '1.2.3', bin: { @@ -538,15 +630,19 @@ t.test( const path = t.testdir({ node_modules: { '@foo/bar': { - 'package.json': JSON.stringify(foobarManifest), + 'package.json': JSON.stringify(pkg), }, }, }) npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map([['@foo/bar', { name: '@foo/bar', version: '1.2.3' }]]), + inventory: { + query () { + return new Set([{ ...pkg, package: pkg }]) + }, + }, } - MANIFESTS['@foo/bar'] = foobarManifest + MANIFESTS['@foo/bar'] = pkg await exec.exec(['@foo/bar', 'one arg', 'two arg']) t.strictSame(MKDIRPS, [], 'no need to make any dirs') t.match(ARB_CTOR, [{ path }]) @@ -571,9 +667,22 @@ t.test( 'npm exec @foo/bar, with different bin alias and no unscoped named bin, locally installed', async t => { const path = t.testdir() + const pkg = { + name: '@foo/bar', + version: '1.2.3.', + bin: { foo: 'qux', corge: 'qux', baz: 'quux' }, + } npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map([['@foo/bar', { name: '@foo/bar', version: '1.2.3' }]]), + inventory: { + query () { + return new Set([{ + ...pkg, + package: pkg, + pkgid: `${pkg.name}@${pkg.version}`, + }]) + }, + }, } MANIFESTS['@foo/bar'] = { name: '@foo/bar', @@ -609,10 +718,18 @@ t.test('run command with 2 packages, need install, verify sort', async t => { const installDir = resolve('npx-cache-dir/07de77790e5f40f2') npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } ARB_ACTUAL_TREE[installDir] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } MANIFESTS.foo = { name: 'foo', @@ -653,10 +770,25 @@ t.test('run command with 2 packages, need install, verify sort', async t => { }) t.test('npm exec foo, no bin in package', async t => { - const path = t.testdir() + const pkg = { name: 'foo', version: '1.2.3' } + const path = t.testdir({ + node_modules: { + foo: { + 'package.json': JSON.stringify(pkg), + }, + }, + }) npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map([['foo', { name: 'foo', version: '1.2.3' }]]), + inventory: { + query () { + return new Set([{ + ...pkg, + package: pkg, + pkgid: `${pkg.name}@${pkg.version}`, + }]) + }, + }, } MANIFESTS.foo = { name: 'foo', @@ -672,9 +804,22 @@ t.test('npm exec foo, no bin in package', async t => { t.test('npm exec foo, many bins in package, none named foo', async t => { const path = t.testdir() + const pkg = { + name: 'foo', + version: '1.2.3', + bin: { bar: 'bar', baz: 'baz' }, + } npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map([['foo', { name: 'foo', version: '1.2.3' }]]), + inventory: { + query () { + return new Set([{ + ...pkg, + package: pkg, + pkgid: `${pkg.name}@${pkg.version}`, + }]) + }, + }, } MANIFESTS.foo = { name: 'foo', @@ -694,11 +839,16 @@ t.test('npm exec foo, many bins in package, none named foo', async t => { t.test('npm exec -p foo -c "ls -laF"', async t => { const path = t.testdir() + const pkg = { name: 'foo', version: '1.2.3' } npm.localPrefix = path config.package = ['foo'] config.call = 'ls -laF' ARB_ACTUAL_TREE[path] = { - children: new Map([['foo', { name: 'foo', version: '1.2.3' }]]), + inventory: { + query () { + return new Set([{ ...pkg, package: pkg }]) + }, + }, } MANIFESTS.foo = { name: 'foo', @@ -751,10 +901,18 @@ t.test('prompt when installs are needed if not already present and shell is a TT const installDir = resolve('npx-cache-dir/07de77790e5f40f2') npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } ARB_ACTUAL_TREE[installDir] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } MANIFESTS.foo = { name: 'foo', @@ -823,10 +981,18 @@ t.test( const installDir = resolve('npx-cache-dir/07de77790e5f40f2') npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } ARB_ACTUAL_TREE[installDir] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } MANIFESTS.foo = { name: 'foo', @@ -896,10 +1062,18 @@ t.test( const installDir = resolve('npx-cache-dir/f7fbba6e0636f890') npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } ARB_ACTUAL_TREE[installDir] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } MANIFESTS.foo = { name: 'foo', @@ -957,10 +1131,18 @@ t.test('abort if prompt rejected', async t => { const installDir = resolve('npx-cache-dir/07de77790e5f40f2') npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } ARB_ACTUAL_TREE[installDir] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } MANIFESTS.foo = { name: 'foo', @@ -1014,10 +1196,18 @@ t.test('abort if prompt false', async t => { const installDir = resolve('npx-cache-dir/07de77790e5f40f2') npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } ARB_ACTUAL_TREE[installDir] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } MANIFESTS.foo = { name: 'foo', @@ -1070,10 +1260,18 @@ t.test('abort if -n provided', async t => { const installDir = resolve('npx-cache-dir/07de77790e5f40f2') npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } ARB_ACTUAL_TREE[installDir] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } MANIFESTS.foo = { name: 'foo', @@ -1105,10 +1303,18 @@ t.test('forward legacyPeerDeps opt', async t => { const installDir = resolve('npx-cache-dir/f7fbba6e0636f890') npm.localPrefix = path ARB_ACTUAL_TREE[path] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } ARB_ACTUAL_TREE[installDir] = { - children: new Map(), + inventory: { + query () { + return new Set() + }, + }, } MANIFESTS.foo = { name: 'foo', diff --git a/workspaces/libnpmexec/lib/index.js b/workspaces/libnpmexec/lib/index.js index 81d152a20bd6e..c2014a4b53645 100644 --- a/workspaces/libnpmexec/lib/index.js +++ b/workspaces/libnpmexec/lib/index.js @@ -9,15 +9,14 @@ const npmlog = require('npmlog') const mkdirp = require('mkdirp-infer-owner') const npa = require('npm-package-arg') const pacote = require('pacote') -const readPackageJson = require('read-package-json-fast') const cacheInstallDir = require('./cache-install-dir.js') const { fileExists, localFileExists } = require('./file-exists.js') const getBinFromManifest = require('./get-bin-from-manifest.js') -const manifestMissing = require('./manifest-missing.js') const noTTY = require('./no-tty.js') const runScript = require('./run-script.js') const isWindows = require('./is-windows.js') +const _localManifest = Symbol('localManifest') /* istanbul ignore next */ const PATH = ( @@ -86,20 +85,47 @@ const exec = async (opts) => { packages.push(args[0]) } + // figure out whether we need to install stuff, or if local is fine + const localArb = new Arborist({ + ...flatOptions, + path, + }) + const localTree = await localArb.loadActual() + + const getLocalManifest = ({ tree, name }) => { + // look up the package name in the current tree inventory, + // if it's found then return that normalized pkg data + let node + const nodes = tree.inventory.query('packageName', name) + for (const n of nodes) { + node = n + break + } + + if (node) { + return { + _id: node.pkgid, + ...node.package, + [_localManifest]: true, + } + } + } + // If we do `npm exec foo`, and have a `foo` locally, then we'll // always use that, so we don't really need to fetch the manifest. // So: run npa on each packages entry, and if it is a name with a - // rawSpec==='', then try to readPackageJson at - // node_modules/${name}/package.json, and only pacote fetch if - // that fails. + // rawSpec==='', then try to find that node name in the tree inventory + // and only pacote fetch if that fails. const manis = await Promise.all(packages.map(async p => { const spec = npa(p, path) if (spec.type === 'tag' && spec.rawSpec === '') { - // fall through to the pacote.manifest() approach - try { - const pj = resolve(path, 'node_modules', spec.name, 'package.json') - return await readPackageJson(pj) - } catch (er) {} + const localManifest = getLocalManifest({ + tree: localTree, + name: spec.name, + }) + if (localManifest) { + return localManifest + } } // Force preferOnline to true so we are making sure to pull in the latest // This is especially useful if the user didn't give us a version, and @@ -114,16 +140,9 @@ const exec = async (opts) => { args[0] = getBinFromManifest(manis[0]) } - // figure out whether we need to install stuff, or if local is fine - const localArb = new Arborist({ - ...flatOptions, - path, - }) - const localTree = await localArb.loadActual() - - // do we have all the packages in manifest list? + // are all packages from the manifest list installed? const needInstall = - manis.some(manifest => manifestMissing({ tree: localTree, manifest })) + manis.some(manifest => !manifest[_localManifest]) if (needInstall) { const { npxCache } = flatOptions @@ -135,16 +154,23 @@ const exec = async (opts) => { }) const tree = await arb.loadActual() + // inspect the npx-space installed tree to check if the package is already + // there, if that's the case also check that it's version matches the same + // version expected by the user requested pkg returned by pacote.manifest + const filterMissingPackagesFromInstallDir = (mani) => { + const localManifest = getLocalManifest({ tree, name: mani.name }) + if (localManifest) { + return localManifest.version !== mani.version + } + return true + } + // at this point, we have to ensure that we get the exact same // version, because it's something that has only ever been installed // by npm exec in the cache install directory - const add = manis.filter(mani => manifestMissing({ - tree, - manifest: { - ...mani, - _from: `${mani.name}@${mani.version}`, - }, - })) + const add = manis + .filter(mani => !mani[_localManifest]) + .filter(filterMissingPackagesFromInstallDir) .map(mani => mani._from) .sort((a, b) => a.localeCompare(b, 'en')) diff --git a/workspaces/libnpmexec/lib/manifest-missing.js b/workspaces/libnpmexec/lib/manifest-missing.js deleted file mode 100644 index aec1281e3a4bf..0000000000000 --- a/workspaces/libnpmexec/lib/manifest-missing.js +++ /dev/null @@ -1,19 +0,0 @@ -const manifestMissing = ({ tree, manifest }) => { - // if the tree doesn't have a child by that name/version, return true - // true means we need to install it - const child = tree.children.get(manifest.name) - // if no child, we have to load it - if (!child) { - return true - } - - // if no version/tag specified, allow whatever's there - if (manifest._from === `${manifest.name}@`) { - return false - } - - // otherwise the version has to match what we WOULD get - return child.version !== manifest.version -} - -module.exports = manifestMissing diff --git a/workspaces/libnpmexec/test/index.js b/workspaces/libnpmexec/test/index.js index bd1b67cec3a5d..834dcff0a1ff6 100644 --- a/workspaces/libnpmexec/test/index.js +++ b/workspaces/libnpmexec/test/index.js @@ -121,6 +121,82 @@ t.test('local pkg, must not fetch manifest for avail pkg', async t => { t.equal(res, 'LOCAL PKG', 'should run local pkg bin script') }) +t.test('multiple local pkgs', async t => { + const foo = { + name: '@ruyadorno/create-foo', + version: '2.0.0', + bin: { + 'create-foo': './index.js', + }, + } + const bar = { + name: '@ruyadorno/create-bar', + version: '2.0.0', + bin: { + 'create-bar': './index.js', + }, + } + const path = t.testdir({ + cache: {}, + npxCache: {}, + node_modules: { + '.bin': {}, + '@ruyadorno': { + 'create-foo': { + 'package.json': JSON.stringify(foo), + 'index.js': `#!/usr/bin/env node + require('fs').writeFileSync(process.argv.slice(2)[0], 'foo')`, + }, + 'create-bar': { + 'package.json': JSON.stringify(bar), + 'index.js': `#!/usr/bin/env node + require('fs').writeFileSync(process.argv.slice(2)[0], 'bar')`, + }, + }, + }, + 'package.json': JSON.stringify({ + name: 'pkg', + dependencies: { + '@ruyadorno/create-foo': '^2.0.0', + '@ruyadorno/create-bar': '^2.0.0', + }, + }), + }) + const runPath = path + const cache = resolve(path, 'cache') + const npxCache = resolve(path, 'npxCache') + + const setupBins = async (pkg) => { + const executable = + resolve(path, `node_modules/${pkg.name}/index.js`) + fs.chmodSync(executable, 0o775) + + await binLinks({ + path: resolve(path, `node_modules/${pkg.name}`), + pkg, + }) + } + + await Promise.all([foo, bar] + .map(setupBins)) + + await libexec({ + ...baseOpts, + localBin: resolve(path, 'node_modules/.bin'), + cache, + npxCache, + packages: ['@ruyadorno/create-foo', '@ruyadorno/create-bar'], + call: 'create-foo resfile && create-bar bar', + path, + runPath, + }) + + const resFoo = fs.readFileSync(resolve(path, 'resfile')).toString() + t.equal(resFoo, 'foo', 'should run local pkg bin script') + const resBar = fs.readFileSync(resolve(path, 'bar')).toString() + t.equal(resBar, 'bar', 'should run local pkg bin script') +}) + t.test('local file system path', async t => { const path = t.testdir({ cache: {}, diff --git a/workspaces/libnpmexec/test/manifest-missing.js b/workspaces/libnpmexec/test/manifest-missing.js deleted file mode 100644 index e7ce1c851ab49..0000000000000 --- a/workspaces/libnpmexec/test/manifest-missing.js +++ /dev/null @@ -1,32 +0,0 @@ -const t = require('tap') -const Arborist = require('@npmcli/arborist') - -const manifestMissing = require('../lib/manifest-missing.js') - -t.test('missing version', async t => { - const path = t.testdir({ - node_modules: { - a: { - 'package.json': JSON.stringify({ - name: 'a', - version: '1.0.0', - }), - }, - }, - 'package.json': JSON.stringify({ - name: 'root', - dependencies: { - a: '^1.0.0', - }, - }), - }) - const arb = new Arborist({ - path, - }) - const tree = await arb.loadActual() - const manifest = { - name: 'a', - _from: 'a@', - } - t.notOk(manifestMissing({ tree, manifest }), 'manifest not missing') -})