diff --git a/.changeset/fix-cached-builds-bypass-allowbuilds.md b/.changeset/fix-cached-builds-bypass-allowbuilds.md new file mode 100644 index 0000000000..b029a5e629 --- /dev/null +++ b/.changeset/fix-cached-builds-bypass-allowbuilds.md @@ -0,0 +1,7 @@ +--- +"@pnpm/building.during-install": patch +"@pnpm/installing.deps-installer": patch +"pnpm": patch +--- + +Fixed `strictDepBuilds` and `allowBuilds` checks being bypassed when a package's build side-effects are cached in the store. Packages with cached builds were skipped by `buildModules` (`isBuilt: true`) and never reached the `allowBuild` check. Now checks `allowBuild` for all packages with `requiresBuild` regardless of `isBuilt` state. Also detects packages whose build approval was revoked between installs. diff --git a/AGENTS.md b/AGENTS.md index 80f99fa516..6ce87df985 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -49,6 +49,20 @@ pnpm install pnpm run compile ``` +To compile a specific package: + +```bash +pnpm --filter run compile +``` + +**Important:** The pnpm CLI e2e tests (in `pnpm/test/`) use the **bundled** `pnpm/dist/pnpm.mjs`, not the individual package `lib/` outputs. After changing any package, you must rebuild the bundle before running e2e tests: + +```bash +pnpm --filter pnpm run compile +``` + +This runs `tsgo --build`, linting, and `pnpm run bundle` (which bundles all packages into `pnpm/dist/pnpm.mjs`). Without this step, e2e tests will use a stale bundle and your changes won't be tested. + ## Testing Never run all tests in the repository as it takes a lot of time. diff --git a/building/after-install/src/index.ts b/building/after-install/src/index.ts index d65fd40db4..d89f1d4437 100644 --- a/building/after-install/src/index.ts +++ b/building/after-install/src/index.ts @@ -28,7 +28,7 @@ import { type PackageSnapshots, } from '@pnpm/lockfile.utils' import { lockfileWalker, type LockfileWalkerStep } from '@pnpm/lockfile.walker' -import { logger, streamParser } from '@pnpm/logger' +import { globalInfo, logger, streamParser } from '@pnpm/logger' import npa from '@pnpm/npm-package-arg' import { safeReadPackageJsonFromDir } from '@pnpm/pkg-manifest.reader' import type { PackageFilesIndex } from '@pnpm/store.cafs' @@ -153,9 +153,10 @@ export async function buildSelectedPkgs ( publicHoistPattern: ctx.publicHoistPattern, registries: ctx.registries, skipped: Array.from(ctx.skipped), - storeDir: ctx.storeDir, - virtualStoreDir: ctx.virtualStoreDir, - virtualStoreDirMaxLength: ctx.virtualStoreDirMaxLength, + storeDir: ctx.modulesFile?.storeDir ?? ctx.storeDir, + virtualStoreDir: ctx.modulesFile?.virtualStoreDir ?? ctx.virtualStoreDir, + virtualStoreDirMaxLength: ctx.modulesFile?.virtualStoreDirMaxLength ?? ctx.virtualStoreDirMaxLength, + allowBuilds: opts.allowBuilds, }) return { ignoredBuilds: ignoredPkgs, @@ -366,6 +367,7 @@ async function _rebuild ( includeDepGraphHash: true, }) if (pkgFilesIndex.sideEffects?.has(sideEffectsCacheKey)) { + globalInfo(`${pkgId}: reused from store cache`) pkgsThatWereRebuilt.add(depPath) return } @@ -379,7 +381,17 @@ async function _rebuild ( requiresBuild = pkgRequiresBuild(pgkManifest, new Map()) } - const hasSideEffects = requiresBuild && allowBuild(pkgInfo.name, pkgInfo.version, depPath) && await runPostinstallHooks({ + if (!requiresBuild) { + globalInfo(`${pkgId}: skipped (no build scripts)`) + pkgsThatWereRebuilt.add(depPath) + return + } + if (!allowBuild(pkgInfo.name, pkgInfo.version, depPath)) { + globalInfo(`${pkgId}: skipped (not allowed)`) + pkgsThatWereRebuilt.add(depPath) + return + } + const hasSideEffects = await runPostinstallHooks({ depPath, extraBinPaths, extraEnv: opts.extraEnv, @@ -391,6 +403,7 @@ async function _rebuild ( shellEmulator: opts.shellEmulator, unsafePerm: opts.unsafePerm || false, }) + globalInfo(`${pkgId}: built successfully`) if (hasSideEffects && (opts.sideEffectsCacheWrite ?? true) && resolution.integrity) { builtDepPaths.add(depPath) const filesIndexFile = storeIndexKey(resolution.integrity!.toString(), pkgId) diff --git a/building/during-install/src/index.ts b/building/during-install/src/index.ts index b1c870f412..863063d7e1 100644 --- a/building/during-install/src/index.ts +++ b/building/during-install/src/index.ts @@ -9,7 +9,7 @@ import { skippedOptionalDependencyLogger } from '@pnpm/core-loggers' import { calcDepState, type DepsStateCache } from '@pnpm/deps.graph-hasher' import { PnpmError } from '@pnpm/error' import { runPostinstallHooks } from '@pnpm/exec.lifecycle' -import { logger } from '@pnpm/logger' +import { globalInfo, logger } from '@pnpm/logger' import { applyPatchToDir } from '@pnpm/patching.apply-patch' import { safeReadPackageJsonFromDir } from '@pnpm/pkg-manifest.reader' import type { StoreController } from '@pnpm/store.controller-types' @@ -74,6 +74,9 @@ export async function buildModules ( const groups = chunks.map((chunk) => { chunk = chunk.filter((depPath) => { const node = depGraph[depPath] + if ((node.requiresBuild || node.patch != null) && node.isBuilt) { + globalInfo(`${node.name}@${node.version}: reused from store (side effects cache)`) + } return (node.requiresBuild || node.patch != null) && !node.isBuilt }) if (opts.depsToBuild != null) { diff --git a/installing/deps-installer/src/install/index.ts b/installing/deps-installer/src/install/index.ts index d44b9a73b7..43fc136dc5 100644 --- a/installing/deps-installer/src/install/index.ts +++ b/installing/deps-installer/src/install/index.ts @@ -371,6 +371,29 @@ export async function mutateModules ( if (!opts.ignoreScripts && ignoredBuilds?.size) { ignoredBuilds = await runUnignoredDependencyBuilds(opts, ignoredBuilds, allowBuild) } + // Detect packages whose build approval was revoked between the previous + // and current install. A package is considered revoked when it was + // previously allowed (true) but is now undecided (undefined). Packages + // explicitly denied (false) are not added to ignoredBuilds, consistent + // with how buildModules treats them. + if ( + ctx.modulesFile?.allowBuilds && + ctx.wantedLockfile.packages && + Object.values(ctx.modulesFile.allowBuilds).some((v) => v === true) + ) { + const oldAllowBuild = createAllowBuildFunction({ allowBuilds: ctx.modulesFile.allowBuilds }) + if (oldAllowBuild) { + for (const depPath of Object.keys(ctx.wantedLockfile.packages) as DepPath[]) { + if (ignoredBuilds?.has(depPath)) continue + const { name, version } = dp.parse(depPath) + if (!name || !version) continue + if (oldAllowBuild(name, version) === true && allowBuild?.(name, version) === undefined) { + ignoredBuilds ??= new Set() + ignoredBuilds.add(depPath) + } + } + } + } ignoredScriptsLogger.debug({ packageNames: ignoredBuilds ? dedupePackageNamesFromIgnoredBuilds(ignoredBuilds) : [], }) diff --git a/installing/deps-installer/src/install/link.ts b/installing/deps-installer/src/install/link.ts index d9981eebcb..37096495bf 100644 --- a/installing/deps-installer/src/install/link.ts +++ b/installing/deps-installer/src/install/link.ts @@ -472,7 +472,7 @@ async function linkAllPkgs ( depNode.requiresBuild = files.requiresBuild let sideEffectsCacheKey: string | undefined if (opts.sideEffectsCacheRead && files.sideEffectsMaps && !isEmpty(files.sideEffectsMaps)) { - if (opts?.allowBuild?.(depNode.name, depNode.version) !== false) { + if (opts.allowBuild?.(depNode.name, depNode.version) === true) { sideEffectsCacheKey = calcDepState(opts.depGraph, opts.depsStateCache, depNode.depPath, { includeDepGraphHash: !opts.ignoreScripts && depNode.requiresBuild, // true when is built patchFileHash: depNode.patch?.hash, diff --git a/installing/deps-restorer/src/index.ts b/installing/deps-restorer/src/index.ts index 61296f7080..596cbb2036 100644 --- a/installing/deps-restorer/src/index.ts +++ b/installing/deps-restorer/src/index.ts @@ -908,7 +908,7 @@ async function linkAllPkgs ( depNode.requiresBuild = filesResponse.requiresBuild let sideEffectsCacheKey: string | undefined if (opts.sideEffectsCacheRead && filesResponse.sideEffectsMaps && !isEmpty(filesResponse.sideEffectsMaps)) { - if (opts?.allowBuild?.(depNode.name, depNode.version) !== false) { + if (opts.allowBuild?.(depNode.name, depNode.version) === true) { sideEffectsCacheKey = calcDepState(opts.depGraph, opts.depsStateCache, depNode.dir, { includeDepGraphHash: !opts.ignoreScripts && depNode.requiresBuild, // true when is built patchFileHash: depNode.patch?.hash, diff --git a/installing/deps-restorer/src/linkHoistedModules.ts b/installing/deps-restorer/src/linkHoistedModules.ts index 385fb6ab9b..fab94f6768 100644 --- a/installing/deps-restorer/src/linkHoistedModules.ts +++ b/installing/deps-restorer/src/linkHoistedModules.ts @@ -116,7 +116,7 @@ async function linkAllPkgsInOrder ( depNode.requiresBuild = filesResponse.requiresBuild let sideEffectsCacheKey: string | undefined if (opts.sideEffectsCacheRead && filesResponse.sideEffectsMaps && !isEmpty(filesResponse.sideEffectsMaps)) { - if (opts?.allowBuild?.(depNode.name, depNode.version) !== false) { + if (opts.allowBuild?.(depNode.name, depNode.version) === true) { sideEffectsCacheKey = _calcDepState(dir, { includeDepGraphHash: !opts.ignoreScripts && depNode.requiresBuild, // true when is built patchFileHash: depNode.patch?.hash, diff --git a/pnpm/test/install/lifecycleScripts.ts b/pnpm/test/install/lifecycleScripts.ts index 956b6f330d..f1a71ea110 100644 --- a/pnpm/test/install/lifecycleScripts.ts +++ b/pnpm/test/install/lifecycleScripts.ts @@ -299,6 +299,39 @@ test('selective rebuild preserves ignoredBuilds for packages not being rebuilt', expect(afterRebuild!.ignoredBuilds).toBeDefined() }) +test('strictDepBuilds fails for packages with cached side-effects (#11035)', async () => { + prepare({ + dependencies: { + '@pnpm.e2e/pre-and-postinstall-scripts-example': '1.0.0', + }, + }) + const storeDir = path.resolve('isolated-store') + + // First install: allow the build so side-effects get cached in the store + writeYamlFileSync('pnpm-workspace.yaml', { + storeDir, + enableGlobalVirtualStore: false, + allowBuilds: { + '@pnpm.e2e/pre-and-postinstall-scripts-example': true, + }, + }) + const firstResult = execPnpmSync(['install']) + expect(firstResult.status).toBe(0) + expect(fs.existsSync('node_modules/@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-postinstall.js')).toBeTruthy() + + // Second install: remove the approval. Side-effects are cached in the store + // but strictDepBuilds should still fail. + writeYamlFileSync('pnpm-workspace.yaml', { + storeDir, + enableGlobalVirtualStore: false, + strictDepBuilds: true, + optimisticRepeatInstall: false, + }) + const secondResult = execPnpmSync(['install']) + expect(secondResult.status).toBe(1) + expect(secondResult.stdout.toString()).toContain('Ignored build scripts:') +}) + test('git dependencies with preparation scripts should be installed when dangerouslyAllowAllBuilds is true', async () => { prepare({}) writeYamlFileSync('pnpm-workspace.yaml', { dangerouslyAllowAllBuilds: true })