From 9adb8f1e7a634484251696d45d17d3678f6f7025 Mon Sep 17 00:00:00 2001 From: Ruy Adorno Date: Wed, 30 Mar 2022 16:37:27 -0400 Subject: [PATCH] fix(libnpmexec): fix read mixed local/registry pkg Refactor / clean up of the logic around reading installed packages. Fixes reading packages from mixed sources (one being from the local installed tree and the other from the registry using pacote.manifest). 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/3668 Relates to: https://github.com/npm/cli/pull/4643 Relates to: https://github.com/npm/cli/issues/4619 Relates to: https://github.com/npm/statusboard/issues/403 --- test/lib/commands/exec.js | 274 +++++++++++++++--- workspaces/libnpmexec/lib/index.js | 73 +++-- workspaces/libnpmexec/lib/manifest-missing.js | 19 -- .../libnpmexec/test/manifest-missing.js | 32 -- 4 files changed, 287 insertions(+), 111 deletions(-) delete mode 100644 workspaces/libnpmexec/lib/manifest-missing.js delete mode 100644 workspaces/libnpmexec/test/manifest-missing.js 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..fbe5c5520c381 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,42 @@ 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 + const [node] = tree.inventory.query('packageName', name) + + 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 +135,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 +149,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/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') -})