From d3517fd4a0e21bbc2e9d729cd88666a829de59fc Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Tue, 27 Sep 2022 12:21:48 -0700 Subject: [PATCH] feat: pacote now optionally takes a tree when preparing directories If a tree is not passed in, then pacote will create one with `loadActual` and the resolved path to the directory it is preparing. BREAKING CHANGE: `pacote` now has a peer dependency on `@npmcli/arborist`. --- lib/dir.js | 9 ++++- lib/fetcher.js | 1 + package.json | 6 ++- test/dir.js | 39 ++++++++++-------- test/fixtures/npm-mock.js | 28 +++++++++++++ test/git.js | 84 +++++++++++++++++---------------------- 6 files changed, 101 insertions(+), 66 deletions(-) create mode 100644 test/fixtures/npm-mock.js diff --git a/lib/dir.js b/lib/dir.js index 50237981..922298e3 100644 --- a/lib/dir.js +++ b/lib/dir.js @@ -3,6 +3,7 @@ const FileFetcher = require('./file.js') const Minipass = require('minipass') const tarCreateOptions = require('./util/tar-create-options.js') const packlist = require('npm-packlist') +const Arborist = require('@npmcli/arborist') const tar = require('tar') const _prepareDir = Symbol('_prepareDir') const { resolve } = require('path') @@ -68,7 +69,13 @@ class DirFetcher extends Fetcher { // run the prepare script, get the list of files, and tar it up // pipe to the stream, and proxy errors the chain. this[_prepareDir]() - .then(() => packlist({ path: this.resolved, prefix, workspaces })) + .then(async () => { + if (!this.tree) { + const arb = new Arborist({ path: this.resolved }) + this.tree = await arb.loadActual() + } + return packlist(this.tree, { path: this.resolved, prefix, workspaces }) + }) .then(files => tar.c(tarCreateOptions(this.package), files) .on('error', er => stream.emit('error', er)).pipe(stream)) .catch(er => stream.emit('error', er)) diff --git a/lib/fetcher.js b/lib/fetcher.js index 95b6d3ee..566acf67 100644 --- a/lib/fetcher.js +++ b/lib/fetcher.js @@ -72,6 +72,7 @@ class FetcherBase { this.cache = opts.cache || cacheDir() this.resolved = opts.resolved || null + this.tree = opts.tree || null // default to caching/verifying with sha512, that's what we usually have // need to change this default, or start overriding it, when sha512 diff --git a/package.json b/package.json index ae4ff2f7..b1e29b04 100644 --- a/package.json +++ b/package.json @@ -25,6 +25,7 @@ ] }, "devDependencies": { + "@npmcli/arborist": "^6.0.0 || ^6.0.0-pre.0", "@npmcli/eslint-config": "^3.1.0", "@npmcli/template-oss": "4.4.2", "hosted-git-info": "^5.0.0", @@ -54,7 +55,7 @@ "minipass": "^3.1.6", "mkdirp": "^1.0.4", "npm-package-arg": "^9.0.0", - "npm-packlist": "^6.0.0", + "npm-packlist": "^7.0.0 || ^7.0.0-pre.0", "npm-pick-manifest": "^7.0.0", "npm-registry-fetch": "^13.0.1", "proc-log": "^2.0.0", @@ -65,6 +66,9 @@ "ssri": "^9.0.0", "tar": "^6.1.11" }, + "peerDependencies": { + "@npmcli/arborist": "^6.0.0 || ^6.0.0-pre.0" + }, "engines": { "node": "^14.17.0 || ^16.13.0 || >=18.0.0" }, diff --git a/test/dir.js b/test/dir.js index 93b9ecce..bec24871 100644 --- a/test/dir.js +++ b/test/dir.js @@ -1,6 +1,13 @@ const runScript = require('@npmcli/run-script') const RUNS = [] const t = require('tap') +const Arborist = require('@npmcli/arborist') + +const loadActual = async (path) => { + const arb = new Arborist({ path }) + const tree = await arb.loadActual() + return tree +} const DirFetcher = t.mock('../lib/dir.js', { '@npmcli/run-script': (opts) => { @@ -20,8 +27,8 @@ t.cleanSnapshot = str => str.split(process.cwd()).join('${CWD}') const abbrev = resolve(__dirname, 'fixtures/abbrev') const abbrevspec = `file:${relative(process.cwd(), abbrev)}` -t.test('basic', t => { - const f = new DirFetcher(abbrevspec, {}) +t.test('basic', async t => { + const f = new DirFetcher(abbrevspec, { tree: await loadActual(abbrev) }) t.same(f.types, ['directory']) t.resolveMatchSnapshot(f.packument(), 'packument') t.resolveMatchSnapshot(f.manifest(), 'manifest') @@ -32,21 +39,21 @@ t.test('basic', t => { .then(() => f.manifest().then(mani => t.equal(mani, f.package))) }) -t.test('dir with integrity', t => { +t.test('dir with integrity', async t => { const f = new DirFetcher(abbrevspec, { integrity: 'sha512-whatever-this-is-only-checked-if-we-extract-it', + tree: await loadActual(abbrev), }) t.same(f.types, ['directory']) - t.resolveMatchSnapshot(f.packument(), 'packument') - return t.end() + return t.resolveMatchSnapshot(f.packument(), 'packument') }) const prepare = resolve(__dirname, 'fixtures/prepare-script') const preparespec = `file:${relative(process.cwd(), prepare)}` -t.test('with prepare script', t => { +t.test('with prepare script', async t => { RUNS.length = 0 - const f = new DirFetcher(preparespec, {}) + const f = new DirFetcher(preparespec, { tree: await loadActual(prepare) }) t.resolveMatchSnapshot(f.packument(), 'packument') t.resolveMatchSnapshot(f.manifest(), 'manifest') const index = me + '/prepare/index.js' @@ -59,9 +66,9 @@ t.test('with prepare script', t => { }, 'should run in background')) }) -t.test('responds to foregroundScripts: true', t => { +t.test('responds to foregroundScripts: true', async t => { RUNS.length = 0 - const opt = { foregroundScripts: true } + const opt = { foregroundScripts: true, tree: await loadActual(prepare) } const f = new DirFetcher(preparespec, opt) t.resolveMatchSnapshot(f.packument(), 'packument') t.resolveMatchSnapshot(f.manifest(), 'manifest') @@ -75,9 +82,9 @@ t.test('responds to foregroundScripts: true', t => { }, 'should run in foreground')) }) -t.test('responds to foregroundScripts: true and silent: true', t => { +t.test('responds to foregroundScripts: true and silent: true', async t => { RUNS.length = 0 - const opt = { foregroundScripts: true, silent: true } + const opt = { foregroundScripts: true, silent: true, tree: await loadActual(prepare) } const f = new DirFetcher(preparespec, opt) t.resolveMatchSnapshot(f.packument(), 'packument') t.resolveMatchSnapshot(f.manifest(), 'manifest') @@ -91,8 +98,8 @@ t.test('responds to foregroundScripts: true and silent: true', t => { }, 'should run in foreground, but without banner')) }) -t.test('missing dir cannot be packed', t => { - const f = new DirFetcher('file:/this/dir/doesnt/exist', {}) +t.test('missing dir cannot be packed', async t => { + const f = new DirFetcher('file:/this/dir/doesnt/exist', { tree: await loadActual() }) return t.rejects(f.extract(me + '/nope'), { message: `no such file or directory, open '/this/dir/doesnt/exist/package.json`, errno: Number, @@ -102,19 +109,19 @@ t.test('missing dir cannot be packed', t => { }) }) -t.test('when read fails', t => { +t.test('when read fails', async t => { const read = fs.read t.teardown(() => fs.read = read) const poop = new Error('poop') fs.read = (...args) => setTimeout(() => args[args.length - 1](poop)) - const f = new DirFetcher(preparespec, {}) + const f = new DirFetcher(preparespec, { tree: await loadActual() }) return t.rejects(f.extract(me + '/nope'), poop) }) t.test('make bins executable', async t => { const file = resolve(__dirname, 'fixtures/bin-object') const spec = `file:${relative(process.cwd(), file)}` - const f = new DirFetcher(spec, {}) + const f = new DirFetcher(spec, { tree: await loadActual(file) }) const target = resolve(me, basename(file)) const res = await f.extract(target) // node v13.8 swapped out their zlib implementation with chromium's diff --git a/test/fixtures/npm-mock.js b/test/fixtures/npm-mock.js new file mode 100644 index 00000000..11879c1a --- /dev/null +++ b/test/fixtures/npm-mock.js @@ -0,0 +1,28 @@ +#!/usr/bin/env node +const { argv, env } = process + +const pnp = env._PACOTE_NO_PREPARE_ || '' +const pacotePath = env._PACOTE_TEST_PATH_ +const pacoteOpts = env._PACOTE_TEST_OPTS_ + +const data = { + argv, + noPrepare: pnp ? pnp.split('\\n') : [], + cwd: process.cwd(), +} + +if (data.noPrepare.length > 5) { + throw new Error('infinite regress detected!') +} + +// just an incredibly rudimentary package manager +const pkg = require(process.cwd() + '/package.json') +const pacote = require(pacotePath) +for (const [name, spec] of Object.entries(pkg.dependencies)) { + pacote.extract(spec, process.cwd() + '/' + name, { + npmBin: __filename, + ...JSON.parse(pacoteOpts), + }) +} + +require('fs').writeFileSync('log', JSON.stringify(data, 0, 2)) diff --git a/test/git.js b/test/git.js index b5e0b283..15cc5d41 100644 --- a/test/git.js +++ b/test/git.js @@ -78,6 +78,8 @@ const cycleB = resolve(me, 'cycle-b') const abbrevSpec = `file:${abbrev}` +const opts = { cache } + const spawnGit = require('@npmcli/git').spawn const { spawn } = require('child_process') const spawnNpm = require('../lib/util/npm.js') @@ -381,13 +383,13 @@ t.test('setup', { bail: true }, t => { }) t.test('basic stuff', async t => { - const g = new GitFetcher(remote, { cache }) + const g = new GitFetcher(remote, opts) t.same(g.types, ['git']) t.equal(g.resolved, null) - const r = new GitFetcher(remote + '#' + REPO_HEAD, { cache }) + const r = new GitFetcher(remote + '#' + REPO_HEAD, opts) t.equal(r.resolved, remote + '#' + REPO_HEAD) await g.resolve().then(resolved => t.equal(resolved, r.resolved)) - const s = new GitFetcher(remote + '#semver:1.x', { cache }) + const s = new GitFetcher(remote + '#semver:1.x', opts) t.equal(s.resolved, null) await s.resolve().then(resolved => t.not(resolved, r.resolved)) @@ -418,7 +420,7 @@ t.test('basic stuff', async t => { t.equal(await g.manifest(), gm, 'cached manifest') // one that doesn't have an npm install step, but does have submods - const subs = new GitFetcher(submodsRemote, { cache }) + const subs = new GitFetcher(submodsRemote, opts) await subs.extract(me + '/s') fs.statSync(me + '/s/fooblz/package.json') }) @@ -436,7 +438,8 @@ t.test('ignores integrity for git deps', async (t) => { }) // known invalid integrity - const fetcher = new GitFetcher(remote + '#' + REPO_HEAD, { cache, integrity: 'sha512-beeffeed' }) + const fetcher = new GitFetcher(remote + '#' + REPO_HEAD, + { ...opts, integrity: 'sha512-beeffeed' }) const manifest = await fetcher.manifest() t.match(manifest, { _id: 'repo@1.0.0', @@ -470,13 +473,13 @@ t.test('weird hosted that doesnt provide any fetch targets', t => { const spec = npa(remote) spec.hosted = hosted - t.rejects(new GitFetcher(Object.assign(spec, { hosted }), { cache }).resolve(), { + t.rejects(new GitFetcher(Object.assign(spec, { hosted }), opts).resolve(), { message: `No git url for ${remote}`, }) const resSpec = npa(`${remote}#${REPO_HEAD}`) resSpec.hosted = hosted - t.rejects(new GitFetcher(Object.assign(resSpec, { hosted }), { cache }) + t.rejects(new GitFetcher(Object.assign(resSpec, { hosted }), opts) .extract(`${me}/weird-hosted-extract`), { message: `No git url for ${remote}`, }) @@ -494,7 +497,7 @@ t.test('extract from tarball from hosted git service', async t => { t.test(domain, async t => { const runTest = nameat => async t => { const spec = npa(`${nameat}${domain}:repo/x#${REPO_HEAD}`) - const g = new GitFetcher(spec, { cache }) + const g = new GitFetcher(spec, opts) const m = await g.manifest() t.match(m, { name: 'repo', @@ -542,9 +545,8 @@ t.test('extract from tarball from hosted git service', async t => { }) t.test('include auth with hosted https when provided', async t => { - const opt = { cache } const spec = `git+https://user:pass@127.0.0.1/repo` - const g = new GitFetcher(spec, opt) + const g = new GitFetcher(spec, opts) const resolved = await g.resolve() // this weird url is because our fakey mock hosted service's // https() method returns a git:// url, since the git daemon we @@ -559,7 +561,7 @@ t.test('include auth with hosted https when provided', async t => { t.test('fail, but do not fall through to sshurl', async t => { const badSpec = `git+https://user:pass@127.0.0.1/no-repo-here` const failer = new GitFetcher(badSpec, { - cache, + ...opts, resolved: resolved.replace(/\/repo/, '/no-repo-here'), }) t.equal(failer.spec.hosted.shortcut(), 'localhosthttps:no-repo-here/x', @@ -575,7 +577,7 @@ t.test('include .gitignore in hosted tarballs for preparation', async t => { const spec = npa(`localhost:foo/y#${REPO_HEAD}`) spec.hosted.tarball = () => `http://localhost:${httpPort}/prepare-requires-gitignore-1.2.3.tgz` - const g = new GitFetcher(spec, { cache }) + const g = new GitFetcher(spec, opts) const dir = t.testdir() await g.extract(dir) t.strictSame(fs.readdirSync(dir).sort((a, b) => a.localeCompare(b)), [ @@ -586,14 +588,14 @@ t.test('include .gitignore in hosted tarballs for preparation', async t => { }) t.test('add git sha to hosted git shorthand', t => - new GitFetcher('localhost:repo/x', { cache }) + new GitFetcher('localhost:repo/x', opts) // it adds the git+ because it thinks it's https .resolve().then(r => t.equal(r, `git+${remoteHosted}#${REPO_HEAD}`))) t.test('fetch a weird ref', t => { let head3 = '' t.test('hosted', async t => { - const result = await new GitFetcher('localhost:repo/x#HEAD~3', { cache }).extract(me + '/h3h') + const result = await new GitFetcher('localhost:repo/x#HEAD~3', opts).extract(me + '/h3h') head3 = result.resolved.split('#').pop() t.match(result.resolved, /^git\+git:\/\/127\.0\.0\.1:[0-9]+\/repo#[a-z0-9]{40}$/, 'got git url as resolved value') @@ -602,7 +604,7 @@ t.test('fetch a weird ref', t => { }) t.test('regular', async t => { - const result = await new GitFetcher(`${remote}#HEAD~3`, { cache }).extract(me + '/h3r') + const result = await new GitFetcher(`${remote}#HEAD~3`, opts).extract(me + '/h3r') t.equal(result.resolved, `${remote}#${head3}`, 'got the same HEAD~3 sha as before') }) @@ -610,14 +612,14 @@ t.test('fetch a weird ref', t => { }) t.test('fetch a private repo where the tgz is a 404', t => { - const gf = new GitFetcher(`localhost:repo/x#${REPO_HEAD}`, { cache }) + const gf = new GitFetcher(`localhost:repo/x#${REPO_HEAD}`, opts) gf.spec.hosted.tarball = () => `${hostedUrl}/not-found.tgz` // should fetch it by falling back to ssh when it gets an http error return gf.extract(me + '/no-tgz') }) t.test('fetch a private repo where the tgz is not a tarball', t => { - const gf = new GitFetcher(`localhost:repo/x#${REPO_HEAD}`, { cache }) + const gf = new GitFetcher(`localhost:repo/x#${REPO_HEAD}`, opts) gf.spec.hosted.tarball = () => `${hostedUrl}/not-tar.tgz` // should NOT retry, because the error was not an HTTP fetch error return t.rejects(gf.extract(me + '/bad-tgz'), { @@ -627,7 +629,7 @@ t.test('fetch a private repo where the tgz is not a tarball', t => { t.test('resolved is a git+ssh url for hosted repos that support it', t => { const hash = '0000000000000000000000000000000000000000' - const gf = new GitFetcher(`github:x/y#${hash}`, {}) + const gf = new GitFetcher(`github:x/y#${hash}`, { ...opts, cache: null }) t.equal(gf.resolved, `git+ssh://git@github.com/x/y.git#${hash}`) t.end() }) @@ -635,13 +637,13 @@ t.test('resolved is a git+ssh url for hosted repos that support it', t => { t.test('resolved preserves git+ssh url for non-hosted repos', t => { const hash = '0000000000000000000000000000000000000000' const url = `git+ssh://git@test/x/y.git#${hash}` - const gf = new GitFetcher(url, {}) + const gf = new GitFetcher(url, { ...opts, cache: null }) t.equal(gf.resolved, url) t.end() }) t.test('fall back to ssh url if https url fails or is missing', t => { - const gf = new GitFetcher(`localhostssh:repo/x`, { cache }) + const gf = new GitFetcher(`localhostssh:repo/x`, opts) return gf.extract(`${me}/localhostssh`).then(({ resolved }) => t.equal(resolved, `git+git://127.0.0.1:${gitPort}/repo#${REPO_HEAD}`)) }) @@ -667,36 +669,22 @@ t.test('handle it when prepared git deps depend on each other', async t => { // now we've created both repos, and they each depend on the other // our mocked npm bin should safely prevent an infinite regress if // this test fails, and just log that the appropriate env got set. - const npmBin = resolve(t.testdir(), 'npm-mock.js') const path = t.testdir({ - 'npm-mock.js': `#!/usr/bin/env node -const pnp = process.env._PACOTE_NO_PREPARE_ || '' -const { argv } = process -const data = { - argv, - noPrepare: pnp ? pnp.split('\\n') : [], - cwd: process.cwd(), -} -if (data.noPrepare.length > 5) { - throw new Error('infinite regress detected!') -} - -// just an incredibly rudimentary package manager -const pkg = require(process.cwd() + '/package.json') -const pacote = require(${JSON.stringify(dirname(__dirname))}) -for (const [name, spec] of Object.entries(pkg.dependencies)) { - pacote.extract(spec, process.cwd() + '/' + name, { - npmBin: ${JSON.stringify(npmBin)}, - cache: ${JSON.stringify(cache)}, + 'npm-mock.js': fs.readFileSync(resolve(fixtures, 'npm-mock.js')).toString(), }) -} -require('fs').writeFileSync('log', JSON.stringify(data,0,2)) -`, + + process.env._PACOTE_TEST_PATH_ = dirname(__dirname) + process.env._PACOTE_TEST_OPTS_ = JSON.stringify(opts) + t.teardown(() => { + delete process.env._PACOTE_TEST_PATH_ + delete process.env._PACOTE_TEST_OPTS_ }) + for (const project of ['cycle-a', 'cycle-b']) { const localRemote = `git://localhost:${gitPort}/${project}` const local = `${path}/${project}` - const g = new GitFetcher(localRemote, { cache, npmBin }) + const npmBin = `${path}/npm-mock.js` + const g = new GitFetcher(localRemote, { ...opts, npmBin }) await g.extract(local) const log = JSON.parse(fs.readFileSync(`${local}/log`, 'utf8')) t.match(log, { @@ -726,7 +714,7 @@ t.test('missing branch name throws pathspec error', async (t) => { await t.rejects( new GitFetcher( `${domain}:repo/x#this-branch-does-not-exist`, - { cache } + opts ).resolve(), { constructor: /GitPathspecError/, @@ -736,7 +724,7 @@ t.test('missing branch name throws pathspec error', async (t) => { await t.rejects( new GitFetcher( `${domain}:repo/x#this-branch-does-not-exist`, - { cache } + opts ).manifest(), { constructor: /GitPathspecError/, @@ -745,7 +733,7 @@ t.test('missing branch name throws pathspec error', async (t) => { }) t.test('simple repo with workspaces', async t => { - const ws = new GitFetcher(workspacesRemote, { cache }) + const ws = new GitFetcher(workspacesRemote, opts) const extract = resolve(me, 'extract-workspaces') await ws.extract(extract) // the file ./a/foo does not exist in the original repo @@ -757,7 +745,7 @@ t.test('simple repo with workspaces', async t => { }) t.test('simple repo with only a prepack script', async t => { - const ws = new GitFetcher(prepackRemote, { cache }) + const ws = new GitFetcher(prepackRemote, opts) const extract = resolve(me, 'extract-prepack') await ws.extract(extract) // the file ./foo does not exist in the original repo