From d01c323559aa3e087445dbc45e0a74cb03009381 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Fri, 24 Jun 2022 21:44:25 +0300 Subject: [PATCH] fix: patch package even if it is not in the onlyBuiltDependencies list (#4925) close #4914 --- .changeset/nasty-shoes-attack.md | 5 ++ packages/build-modules/src/buildSequence.ts | 10 ++- packages/build-modules/src/index.ts | 7 +- packages/calc-dep-state/src/index.ts | 4 +- packages/calc-dep-state/test/index.ts | 4 +- packages/core/src/install/link.ts | 10 +-- packages/core/test/install/patch.ts | 89 ++++++++++++++++++- packages/headless/src/index.ts | 4 +- packages/headless/src/linkHoistedModules.ts | 4 +- packages/lockfile-types/src/index.ts | 1 + packages/resolve-dependencies/src/index.ts | 3 +- .../src/updateLockfile.ts | 3 + 12 files changed, 122 insertions(+), 22 deletions(-) create mode 100644 .changeset/nasty-shoes-attack.md diff --git a/.changeset/nasty-shoes-attack.md b/.changeset/nasty-shoes-attack.md new file mode 100644 index 0000000000..c868669c67 --- /dev/null +++ b/.changeset/nasty-shoes-attack.md @@ -0,0 +1,5 @@ +--- +"@pnpm/lockfile-types": minor +--- + +Add optional "patched" field to package object in the lockfile. diff --git a/packages/build-modules/src/buildSequence.ts b/packages/build-modules/src/buildSequence.ts index d0ac7e8514..d07c21e8bc 100644 --- a/packages/build-modules/src/buildSequence.ts +++ b/packages/build-modules/src/buildSequence.ts @@ -44,18 +44,20 @@ export default function buildSequence ( } function getSubgraphToBuild ( - graph: Record>, + graph: Record>, entryNodes: string[], nodesToBuild: Set, walked: Set ) { let currentShouldBeBuilt = false for (const depPath of entryNodes) { - if (!graph[depPath]) continue // packages that are already in node_modules are skipped + const node = graph[depPath] + if (!node) continue // packages that are already in node_modules are skipped if (walked.has(depPath)) continue walked.add(depPath) - const childShouldBeBuilt = getSubgraphToBuild(graph, Object.values(graph[depPath].children), nodesToBuild, walked) || - graph[depPath].requiresBuild + const childShouldBeBuilt = getSubgraphToBuild(graph, Object.values(node.children), nodesToBuild, walked) || + node.requiresBuild || + node.patchFile != null if (childShouldBeBuilt) { nodesToBuild.add(depPath) currentShouldBeBuilt = true diff --git a/packages/build-modules/src/index.ts b/packages/build-modules/src/index.ts index b98c64228d..e6851e2089 100644 --- a/packages/build-modules/src/index.ts +++ b/packages/build-modules/src/index.ts @@ -43,7 +43,10 @@ export default async ( const buildDepOpts = { ...opts, warn } const chunks = buildSequence(depGraph, rootDepPaths) const groups = chunks.map((chunk) => { - chunk = chunk.filter((depPath) => depGraph[depPath].requiresBuild && !depGraph[depPath].isBuilt) + chunk = chunk.filter((depPath) => { + const node = depGraph[depPath] + return (node.requiresBuild || node.patchFile != null) && !node.isBuilt + }) if (opts.depsToBuild != null) { chunk = chunk.filter((depPath) => opts.depsToBuild!.has(depPath)) } @@ -102,7 +105,7 @@ async function buildDependency ( try { const sideEffectsCacheKey = calcDepState(depGraph, opts.depsStateCache, depPath, { patchFileHash: depNode.patchFile?.hash, - ignoreScripts: Boolean(opts.ignoreScripts), + isBuilt: hasSideEffects, }) await opts.storeController.upload(depNode.dir, { sideEffectsCacheKey, diff --git a/packages/calc-dep-state/src/index.ts b/packages/calc-dep-state/src/index.ts index e6d77736c5..f11f27ce6e 100644 --- a/packages/calc-dep-state/src/index.ts +++ b/packages/calc-dep-state/src/index.ts @@ -24,11 +24,11 @@ export function calcDepState ( depPath: string, opts: { patchFileHash?: string - ignoreScripts: boolean + isBuilt: boolean } ): string { let result = ENGINE_NAME - if (!opts.ignoreScripts) { + if (opts.isBuilt) { const depStateObj = calcDepStateObj(depPath, depsGraph, cache, new Set()) result += `-${JSON.stringify(depStateObj)}` } diff --git a/packages/calc-dep-state/test/index.ts b/packages/calc-dep-state/test/index.ts index e2dfc69908..fb0836a557 100644 --- a/packages/calc-dep-state/test/index.ts +++ b/packages/calc-dep-state/test/index.ts @@ -18,12 +18,12 @@ const depsGraph = { test('calcDepState()', () => { expect(calcDepState(depsGraph, {}, '/registry/foo/1.0.0', { - ignoreScripts: false, + isBuilt: true, })).toBe(`${ENGINE_NAME}-{}`) }) test('calcDepState() when scripts are ignored', () => { expect(calcDepState(depsGraph, {}, '/registry/foo/1.0.0', { - ignoreScripts: true, + isBuilt: false, })).toBe(ENGINE_NAME) }) diff --git a/packages/core/src/install/link.ts b/packages/core/src/install/link.ts index a76b615cba..f116b411e5 100644 --- a/packages/core/src/install/link.ts +++ b/packages/core/src/install/link.ts @@ -409,21 +409,21 @@ async function linkAllPkgs ( depNodes.map(async (depNode) => { const filesResponse = await depNode.fetchingFiles() + if (typeof depNode.requiresBuild === 'function') { + depNode.requiresBuild = await depNode.requiresBuild() + } let sideEffectsCacheKey: string | undefined if (opts.sideEffectsCacheRead && filesResponse.sideEffects && !isEmpty(filesResponse.sideEffects)) { sideEffectsCacheKey = calcDepState(opts.depGraph, opts.depsStateCache, depNode.depPath, { - ignoreScripts: opts.ignoreScripts, + isBuilt: !opts.ignoreScripts && depNode.requiresBuild, patchFileHash: depNode.patchFile?.hash, }) } - if (typeof depNode.requiresBuild === 'function') { - depNode.requiresBuild = await depNode.requiresBuild() - } const { importMethod, isBuilt } = await storeController.importPackage(depNode.dir, { filesResponse, force: opts.force, sideEffectsCacheKey, - requiresBuild: depNode.requiresBuild, + requiresBuild: depNode.requiresBuild || depNode.patchFile != null, }) if (importMethod) { progressLogger.debug({ diff --git a/packages/core/test/install/patch.ts b/packages/core/test/install/patch.ts index ac5d20fe73..a1aec671d3 100644 --- a/packages/core/test/install/patch.ts +++ b/packages/core/test/install/patch.ts @@ -44,7 +44,7 @@ test('patch package', async () => { const filesIndexFile = path.join(opts.storeDir, 'files/c7/1ccf199e0fdae37aad13946b937d67bcd35fa111b84d21b3a19439cfdc2812c5d8da8a735e94c2a1ccb77b4583808ee8405313951e7146ac83ede3671dc292-index.json') const filesIndex = await loadJsonFile(filesIndexFile) - const sideEffectsKey = `${ENGINE_NAME}-{}-${patchFileHash}` + const sideEffectsKey = `${ENGINE_NAME}-${patchFileHash}` const patchedFileIntegrity = filesIndex.sideEffects?.[sideEffectsKey]['index.js']?.integrity expect(patchedFileIntegrity).toBeTruthy() const originalFileIntegrity = filesIndex.files['index.js'].integrity @@ -233,3 +233,90 @@ test('patch package when scripts are ignored', async () => { // The original file did not break, when a patched version was created expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).not.toContain('// patched') }) + +test('patch package when the package is not in onlyBuiltDependencies list', async () => { + const project = prepareEmpty() + const patchPath = path.join(f.find('patch-pkg'), 'is-positive@1.0.0.patch') + + const patchedDependencies = { + 'is-positive@1.0.0': path.relative(process.cwd(), patchPath), + } + const opts = await testDefaults({ + fastUnpack: false, + sideEffectsCacheRead: true, + sideEffectsCacheWrite: true, + patchedDependencies, + onlyBuiltDependencies: [], + }, {}, {}, { packageImportMethod: 'hardlink' }) + await install({ + dependencies: { + 'is-positive': '1.0.0', + }, + }, opts) + + expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).toContain('// patched') + + const patchFileHash = 'jnbpamcxayl5i4ehrkoext3any' + const lockfile = await project.readLockfile() + expect(lockfile.patchedDependencies).toStrictEqual({ + 'is-positive@1.0.0': { + path: patchedDependencies['is-positive@1.0.0'], + hash: patchFileHash, + }, + }) + expect(lockfile.packages[`/is-positive/1.0.0_${patchFileHash}`]).toBeTruthy() + + const filesIndexFile = path.join(opts.storeDir, 'files/c7/1ccf199e0fdae37aad13946b937d67bcd35fa111b84d21b3a19439cfdc2812c5d8da8a735e94c2a1ccb77b4583808ee8405313951e7146ac83ede3671dc292-index.json') + const filesIndex = await loadJsonFile(filesIndexFile) + const sideEffectsKey = `${ENGINE_NAME}-${patchFileHash}` + const patchedFileIntegrity = filesIndex.sideEffects?.[sideEffectsKey]['index.js']?.integrity + expect(patchedFileIntegrity).toBeTruthy() + const originalFileIntegrity = filesIndex.files['index.js'].integrity + expect(originalFileIntegrity).toBeTruthy() + // The integrity of the original file differs from the integrity of the patched file + expect(originalFileIntegrity).not.toEqual(patchedFileIntegrity) + + // The same with frozen lockfile + await rimraf('node_modules') + await install({ + dependencies: { + 'is-positive': '1.0.0', + }, + }, { + ...opts, + frozenLockfile: true, + }) + expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).toContain('// patched') + + // The same with frozen lockfile and hoisted node_modules + await rimraf('node_modules') + await install({ + dependencies: { + 'is-positive': '1.0.0', + }, + }, { + ...opts, + frozenLockfile: true, + nodeLinker: 'hoisted', + }) + expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).toContain('// patched') + + process.chdir('..') + fs.mkdirSync('project2') + process.chdir('project2') + + await install({ + dependencies: { + 'is-positive': '1.0.0', + }, + }, await testDefaults({ + fastUnpack: false, + sideEffectsCacheRead: true, + sideEffectsCacheWrite: true, + onlyBuiltDependencies: [], + offline: true, + }, {}, {}, { packageImportMethod: 'hardlink' })) + + // The original file did not break, when a patched version was created + expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).not.toContain('// patched') +}) diff --git a/packages/headless/src/index.ts b/packages/headless/src/index.ts index 8787a59117..4092481081 100644 --- a/packages/headless/src/index.ts +++ b/packages/headless/src/index.ts @@ -689,14 +689,14 @@ async function linkAllPkgs ( let sideEffectsCacheKey: string | undefined if (opts.sideEffectsCacheRead && filesResponse.sideEffects && !isEmpty(filesResponse.sideEffects)) { sideEffectsCacheKey = calcDepState(opts.depGraph, opts.depsStateCache, depNode.dir, { - ignoreScripts: opts.ignoreScripts, + isBuilt: !opts.ignoreScripts && depNode.requiresBuild, patchFileHash: depNode.patchFile?.hash, }) } const { importMethod, isBuilt } = await storeController.importPackage(depNode.dir, { filesResponse, force: opts.force, - requiresBuild: depNode.requiresBuild, + requiresBuild: depNode.requiresBuild || depNode.patchFile != null, sideEffectsCacheKey, }) if (importMethod) { diff --git a/packages/headless/src/linkHoistedModules.ts b/packages/headless/src/linkHoistedModules.ts index b60bc22e97..e715e24289 100644 --- a/packages/headless/src/linkHoistedModules.ts +++ b/packages/headless/src/linkHoistedModules.ts @@ -104,14 +104,14 @@ async function linkAllPkgsInOrder ( let sideEffectsCacheKey: string | undefined if (opts.sideEffectsCacheRead && filesResponse.sideEffects && !isEmpty(filesResponse.sideEffects)) { sideEffectsCacheKey = _calcDepState(dir, { + isBuilt: !opts.ignoreScripts && depNode.requiresBuild, patchFileHash: depNode.patchFile?.hash, - ignoreScripts: opts.ignoreScripts, }) } const { importMethod, isBuilt } = await storeController.importPackage(depNode.dir, { filesResponse, force: opts.force || depNode.depPath !== prevGraph[dir]?.depPath, - requiresBuild: depNode.requiresBuild, + requiresBuild: depNode.requiresBuild || depNode.patchFile != null, sideEffectsCacheKey, }) if (importMethod) { diff --git a/packages/lockfile-types/src/index.ts b/packages/lockfile-types/src/index.ts index 4a090d8f70..46865d771d 100644 --- a/packages/lockfile-types/src/index.ts +++ b/packages/lockfile-types/src/index.ts @@ -69,6 +69,7 @@ export interface PackageSnapshot { dev?: true | false optional?: true requiresBuild?: true + patched?: true prepare?: true hasBin?: true // name and version are only needed diff --git a/packages/resolve-dependencies/src/index.ts b/packages/resolve-dependencies/src/index.ts index 09fe6706bb..1192513d02 100644 --- a/packages/resolve-dependencies/src/index.ts +++ b/packages/resolve-dependencies/src/index.ts @@ -271,8 +271,7 @@ async function finishLockfileUpdates ( Boolean(pkgJson.scripts.postinstall) ) || filesResponse.filesIndex['binding.gyp'] || - Object.keys(filesResponse.filesIndex).some((filename) => !(filename.match(/^[.]hooks[\\/]/) == null)) || // TODO: optimize this - depNode.patchFile != null + Object.keys(filesResponse.filesIndex).some((filename) => !(filename.match(/^[.]hooks[\\/]/) == null)) // TODO: optimize this ) } else { // This should never ever happen diff --git a/packages/resolve-dependencies/src/updateLockfile.ts b/packages/resolve-dependencies/src/updateLockfile.ts index 86271fffba..0cb684e282 100644 --- a/packages/resolve-dependencies/src/updateLockfile.ts +++ b/packages/resolve-dependencies/src/updateLockfile.ts @@ -156,6 +156,9 @@ function toLockfileDependency ( if (pkg.hasBin) { result['hasBin'] = true } + if (pkg.patchFile) { + result['patched'] = true + } const requiresBuildIsKnown = typeof pkg.requiresBuild === 'boolean' let pending = false if (requiresBuildIsKnown) {