From ab6c42d99e04233a44009fdf82816dd62f4736f3 Mon Sep 17 00:00:00 2001 From: palkim Date: Tue, 5 May 2026 10:17:11 +0900 Subject: [PATCH] fix: refresh ignored builds when allowBuilds changes (#11366) * fix: refresh ignored builds when allowBuilds changes * refactor: extract isBuildExplicitlyDisallowed into @pnpm/building.policy Removes the duplicated ignored-build filter from deps-installer and deps-restorer and exposes it as `isBuildExplicitlyDisallowed` on `@pnpm/building.policy`, alongside `createAllowBuildFunction`. * fix: respect ignoredWorkspaceStateSettings in allowBuilds stale-state check The fallback that flagged installs when allowBuilds went from unset to non-empty bypassed the ignoredSettings filter, so callers that explicitly opted out of allowBuilds tracking (via ignoredWorkspaceStateSettings) could still be forced into a redundant install. --------- Co-authored-by: Zoltan Kochan --- .changeset/quiet-geese-build.md | 10 ++++ building/policy/package.json | 1 + building/policy/src/index.ts | 10 +++- building/policy/test/index.ts | 21 ++++++- building/policy/tsconfig.json | 3 + deps/status/src/checkDepsStatus.ts | 17 +++++- deps/status/test/checkDepsStatus.test.ts | 58 +++++++++++++++++++ .../deps-installer/src/install/index.ts | 4 +- installing/deps-restorer/src/index.ts | 4 +- pnpm-lock.yaml | 3 + pnpm/test/install/lifecycleScripts.ts | 32 ++++++++++ workspace/state/src/types.ts | 1 + .../state/test/createWorkspaceState.test.ts | 6 ++ 13 files changed, 163 insertions(+), 7 deletions(-) create mode 100644 .changeset/quiet-geese-build.md diff --git a/.changeset/quiet-geese-build.md b/.changeset/quiet-geese-build.md new file mode 100644 index 0000000000..3b4c9399e1 --- /dev/null +++ b/.changeset/quiet-geese-build.md @@ -0,0 +1,10 @@ +--- +"@pnpm/building.policy": patch +"@pnpm/installing.deps-installer": patch +"@pnpm/installing.deps-restorer": patch +"@pnpm/deps.status": patch +"@pnpm/workspace.state": patch +"pnpm": patch +--- + +Treat `allowBuilds` as an install-state input and clear previously ignored builds when they are explicitly disallowed. diff --git a/building/policy/package.json b/building/policy/package.json index 729778a0b6..ebc3620542 100644 --- a/building/policy/package.json +++ b/building/policy/package.json @@ -32,6 +32,7 @@ }, "dependencies": { "@pnpm/config.version-policy": "workspace:*", + "@pnpm/deps.path": "workspace:*", "@pnpm/types": "workspace:*" }, "devDependencies": { diff --git a/building/policy/src/index.ts b/building/policy/src/index.ts index 9b7d2fee7c..d63afa9058 100644 --- a/building/policy/src/index.ts +++ b/building/policy/src/index.ts @@ -1,5 +1,13 @@ import { expandPackageVersionSpecs } from '@pnpm/config.version-policy' -import type { AllowBuild } from '@pnpm/types' +import * as dp from '@pnpm/deps.path' +import type { AllowBuild, DepPath } from '@pnpm/types' + +export function isBuildExplicitlyDisallowed (depPath: DepPath, allowBuild?: AllowBuild): boolean { + if (!allowBuild) return false + const { name, version } = dp.parse(depPath) + if (!name || !version) return false + return allowBuild(name, version) === false +} export function createAllowBuildFunction ( opts: { diff --git a/building/policy/test/index.ts b/building/policy/test/index.ts index 61a28bf51d..48ed94f283 100644 --- a/building/policy/test/index.ts +++ b/building/policy/test/index.ts @@ -1,5 +1,6 @@ import { expect, it } from '@jest/globals' -import { createAllowBuildFunction } from '@pnpm/building.policy' +import { createAllowBuildFunction, isBuildExplicitlyDisallowed } from '@pnpm/building.policy' +import type { DepPath } from '@pnpm/types' it('should allowBuilds with true value', () => { const allowBuild = createAllowBuildFunction({ @@ -42,3 +43,21 @@ it('should allow everything when dangerouslyAllowAllBuilds is true', () => { expect(typeof allowBuild).toBe('function') expect(allowBuild!('foo', '1.0.0')).toBeTruthy() }) + +it('isBuildExplicitlyDisallowed() flags only builds the policy explicitly forbids', () => { + const allowBuild = createAllowBuildFunction({ + allowBuilds: { foo: false, bar: true }, + }) + expect(isBuildExplicitlyDisallowed('foo@1.0.0' as DepPath, allowBuild)).toBe(true) + expect(isBuildExplicitlyDisallowed('bar@1.0.0' as DepPath, allowBuild)).toBe(false) + expect(isBuildExplicitlyDisallowed('baz@1.0.0' as DepPath, allowBuild)).toBe(false) +}) + +it('isBuildExplicitlyDisallowed() returns false when no policy is set', () => { + expect(isBuildExplicitlyDisallowed('foo@1.0.0' as DepPath, undefined)).toBe(false) +}) + +it('isBuildExplicitlyDisallowed() returns false for unparsable depPaths', () => { + const allowBuild = createAllowBuildFunction({ allowBuilds: { foo: false } }) + expect(isBuildExplicitlyDisallowed('not-a-valid-dep-path' as DepPath, allowBuild)).toBe(false) +}) diff --git a/building/policy/tsconfig.json b/building/policy/tsconfig.json index c33c0978bf..141caa1916 100644 --- a/building/policy/tsconfig.json +++ b/building/policy/tsconfig.json @@ -14,6 +14,9 @@ }, { "path": "../../core/types" + }, + { + "path": "../../deps/path" } ] } diff --git a/deps/status/src/checkDepsStatus.ts b/deps/status/src/checkDepsStatus.ts index 07fc265ca0..a36dbafbcb 100644 --- a/deps/status/src/checkDepsStatus.ts +++ b/deps/status/src/checkDepsStatus.ts @@ -134,7 +134,10 @@ async function _checkDepsStatus (opts: CheckDepsStatusOptions, workspaceState: W ignoredSettings.add('catalogs') // 'catalogs' is always ignored for (const [settingName, settingValue] of Object.entries(workspaceState.settings)) { if (ignoredSettings.has(settingName as keyof WorkspaceStateSettings)) continue - if (!equals(settingValue, opts[settingName as keyof WorkspaceStateSettings])) { + const currentSettingValue = settingName === 'allowBuilds' + ? opts.allowBuilds ?? {} + : opts[settingName as keyof WorkspaceStateSettings] + if (!equals(settingValue, currentSettingValue)) { return { upToDate: false, issue: `The value of the ${settingName} setting has changed`, @@ -142,6 +145,18 @@ async function _checkDepsStatus (opts: CheckDepsStatusOptions, workspaceState: W } } } + if ( + !ignoredSettings.has('allowBuilds') && + workspaceState.settings.allowBuilds == null && + opts.allowBuilds != null && + !isEmpty(opts.allowBuilds) + ) { + return { + upToDate: false, + issue: 'The value of the allowBuilds setting has changed', + workspaceState, + } + } } if ((opts.configDependencies != null || workspaceState.configDependencies != null) && !equals(opts.configDependencies ?? {}, workspaceState.configDependencies ?? {})) { return { diff --git a/deps/status/test/checkDepsStatus.test.ts b/deps/status/test/checkDepsStatus.test.ts index b1f37ee9fa..8118a37bb7 100644 --- a/deps/status/test/checkDepsStatus.test.ts +++ b/deps/status/test/checkDepsStatus.test.ts @@ -197,6 +197,64 @@ describe('checkDepsStatus - settings change detection', () => { expect(result.upToDate).toBe(false) expect(result.issue).toBe('The value of the peersSuffixMaxLength setting has changed') }) + + it('returns upToDate: false when allowBuilds have changed', async () => { + const lastValidatedTimestamp = Date.now() - 10_000 + const mockWorkspaceState: WorkspaceState = { + lastValidatedTimestamp, + pnpmfiles: [], + settings: { + excludeLinksFromLockfile: false, + linkWorkspacePackages: true, + preferWorkspacePackages: true, + }, + projects: {}, + filteredInstall: false, + } + + jest.mocked(loadWorkspaceState).mockReturnValue(mockWorkspaceState) + + const opts: CheckDepsStatusOptions = { + rootProjectManifest: {}, + rootProjectManifestDir: '/project', + pnpmfile: [], + ...mockWorkspaceState.settings, + allowBuilds: { sqlite3: false }, + } + const result = await checkDepsStatus(opts) + + expect(result.upToDate).toBe(false) + expect(result.issue).toBe('The value of the allowBuilds setting has changed') + }) + + it('skips the allowBuilds change detection when allowBuilds is in ignoredWorkspaceStateSettings', async () => { + const lastValidatedTimestamp = Date.now() - 10_000 + const mockWorkspaceState: WorkspaceState = { + lastValidatedTimestamp, + pnpmfiles: [], + settings: { + excludeLinksFromLockfile: false, + linkWorkspacePackages: true, + preferWorkspacePackages: true, + }, + projects: {}, + filteredInstall: false, + } + + jest.mocked(loadWorkspaceState).mockReturnValue(mockWorkspaceState) + + const opts: CheckDepsStatusOptions = { + rootProjectManifest: {}, + rootProjectManifestDir: '/project', + pnpmfile: [], + ...mockWorkspaceState.settings, + allowBuilds: { sqlite3: false }, + ignoredWorkspaceStateSettings: ['allowBuilds'], + } + const result = await checkDepsStatus(opts) + + expect(result.issue).not.toBe('The value of the allowBuilds setting has changed') + }) }) describe('checkDepsStatus - pnpmfile modification', () => { diff --git a/installing/deps-installer/src/install/index.ts b/installing/deps-installer/src/install/index.ts index 7a60fcef0f..e34612578d 100644 --- a/installing/deps-installer/src/install/index.ts +++ b/installing/deps-installer/src/install/index.ts @@ -3,7 +3,7 @@ import path from 'node:path' import { linkBins, linkBinsOfPackages } from '@pnpm/bins.linker' import { buildSelectedPkgs } from '@pnpm/building.after-install' import { buildModules, type DepsStateCache, linkBinsOfDependencies } from '@pnpm/building.during-install' -import { createAllowBuildFunction } from '@pnpm/building.policy' +import { createAllowBuildFunction, isBuildExplicitlyDisallowed } from '@pnpm/building.policy' import { parseCatalogProtocol } from '@pnpm/catalogs.protocol-parser' import { type CatalogResultMatcher, matchCatalogResolveResult, resolveFromCatalog } from '@pnpm/catalogs.resolver' import type { Catalogs } from '@pnpm/catalogs.types' @@ -1494,7 +1494,7 @@ const _installInContext: InstallFunction = async (projects, ctx, opts) => { if (ctx.modulesFile?.ignoredBuilds?.size) { ignoredBuilds ??= new Set() for (const ignoredBuild of ctx.modulesFile.ignoredBuilds.values()) { - if (result.currentLockfile.packages?.[ignoredBuild]) { + if (result.currentLockfile.packages?.[ignoredBuild] && !isBuildExplicitlyDisallowed(ignoredBuild, opts.allowBuild)) { ignoredBuilds.add(ignoredBuild) } } diff --git a/installing/deps-restorer/src/index.ts b/installing/deps-restorer/src/index.ts index 27ef939e8a..40dcb115c8 100644 --- a/installing/deps-restorer/src/index.ts +++ b/installing/deps-restorer/src/index.ts @@ -3,7 +3,7 @@ import path from 'node:path' import { linkBins, linkBinsOfPackages } from '@pnpm/bins.linker' import { buildModules } from '@pnpm/building.during-install' -import { createAllowBuildFunction } from '@pnpm/building.policy' +import { createAllowBuildFunction, isBuildExplicitlyDisallowed } from '@pnpm/building.policy' import { LAYOUT_VERSION, WANTED_LOCKFILE, @@ -594,7 +594,7 @@ export async function headlessInstall (opts: HeadlessOptions): Promise { + const project = prepare({}) + writeYamlFileSync('pnpm-workspace.yaml', { + optimisticRepeatInstall: true, + strictDepBuilds: true, + }) + + const firstResult = execPnpmSync(['add', '@pnpm.e2e/pre-and-postinstall-scripts-example@1.0.0']) + + expect(firstResult.status).toBe(1) + expect(firstResult.stdout.toString()).toContain('Ignored build scripts:') + expect(fs.existsSync('node_modules/.modules.yaml')).toBeTruthy() + + writeYamlFileSync('pnpm-workspace.yaml', { + allowBuilds: { + '@pnpm.e2e/pre-and-postinstall-scripts-example': false, + }, + optimisticRepeatInstall: true, + strictDepBuilds: true, + }) + + const secondResult = execPnpmSync(['install']) + + expect(secondResult.status).toBe(0) + expect(secondResult.stdout.toString()).not.toContain('Ignored build scripts:') + const modulesManifest = project.readModulesManifest() + expect(modulesManifest?.allowBuilds).toStrictEqual({ + '@pnpm.e2e/pre-and-postinstall-scripts-example': false, + }) + expect(Array.from(modulesManifest?.ignoredBuilds ?? [])).toStrictEqual([]) +}) + test('the list of ignored builds is preserved after a repeat install', async () => { const project = prepare({}) execPnpmSync(['add', '@pnpm.e2e/pre-and-postinstall-scripts-example@1.0.0', 'esbuild@0.25.0', '--config.optimistic-repeat-install=false']) diff --git a/workspace/state/src/types.ts b/workspace/state/src/types.ts index 910f4c4b12..6a0f803f52 100644 --- a/workspace/state/src/types.ts +++ b/workspace/state/src/types.ts @@ -16,6 +16,7 @@ export interface WorkspaceState { } export const WORKSPACE_STATE_SETTING_KEYS = [ + 'allowBuilds', 'autoInstallPeers', 'catalogs', 'dedupeDirectDeps', diff --git a/workspace/state/test/createWorkspaceState.test.ts b/workspace/state/test/createWorkspaceState.test.ts index 50d6e7cf9d..7a8b26d6a4 100644 --- a/workspace/state/test/createWorkspaceState.test.ts +++ b/workspace/state/test/createWorkspaceState.test.ts @@ -38,6 +38,9 @@ test('createWorkspaceState() saves lockfile-affecting settings', () => { pnpmfiles: [], filteredInstall: false, settings: { + allowBuilds: { + '@pnpm.e2e/pre-and-postinstall-scripts-example': false, + }, autoInstallPeers: true, dedupeDirectDeps: true, excludeLinksFromLockfile: false, @@ -58,6 +61,9 @@ test('createWorkspaceState() saves lockfile-affecting settings', () => { }, }) + expect(state.settings.allowBuilds).toStrictEqual({ + '@pnpm.e2e/pre-and-postinstall-scripts-example': false, + }) expect(state.settings.overrides).toStrictEqual({ foo: '1.0.0' }) expect(state.settings.packageExtensions).toStrictEqual({ bar: { dependencies: { baz: '2.0.0' } },