fix: packages should be patched even when scripts are ignored (#4922)

.changeset/flat-bears-travel.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/calc-dep-state": patch
+---
+
+Calculate the cache key differently when scripts are ignored.

.changeset/tender-toes-fold.md

@@ -0,0 +1,7 @@
+---
+"@pnpm/build-modules": patch
+"@pnpm/core": patch
+"@pnpm/headless": patch
+---
+
+Patch packages even when scripts are ignored.

packages/build-modules/package.json

@@ -42,6 +42,7 @@
     "@pnpm/read-package-json": "workspace:6.0.5",
     "@pnpm/store-controller-types": "workspace:14.0.0",
     "@pnpm/types": "workspace:8.3.0",
+    "patch-package": "^6.4.7",
     "ramda": "^0.27.1",
     "run-groups": "^3.0.1"
   },

packages/build-modules/src/index.ts

@@ -7,6 +7,7 @@ import logger from '@pnpm/logger'
 import { fromDir as readPackageFromDir, safeReadPackageFromDir } from '@pnpm/read-package-json'
 import { StoreController } from '@pnpm/store-controller-types'
 import { DependencyManifest } from '@pnpm/types'
+import { applyPatch } from 'patch-package/dist/applyPatches'
 import runGroups from 'run-groups'
 import buildSequence, { DependenciesGraph, DependenciesGraphNode } from './buildSequence'
 
@@ -22,6 +23,7 @@ export default async (
     extraBinPaths?: string[]
     extraNodePaths?: string[]
     extraEnv?: Record<string, string>
+    ignoreScripts?: boolean
     lockfileDir: string
     optional: boolean
     rawConfig: object
@@ -61,6 +63,7 @@ async function buildDependency (
     extraNodePaths?: string[]
     extraEnv?: Record<string, string>
     depsStateCache: DepsStateCache
+    ignoreScripts?: boolean
     lockfileDir: string
     optional: boolean
     rawConfig: object
@@ -77,13 +80,16 @@ async function buildDependency (
   const depNode = depGraph[depPath]
   try {
     await linkBinsOfDependencies(depNode, depGraph, opts)
-    const hasSideEffects = await runPostinstallHooks({
+    const isPatched = depNode.patchFile?.path != null
+    if (isPatched) {
+      applyPatchToDep(depNode.dir, depNode.patchFile!.path)
+    }
+    const hasSideEffects = !opts.ignoreScripts && await runPostinstallHooks({
       depPath,
       extraBinPaths: opts.extraBinPaths,
       extraEnv: opts.extraEnv,
       initCwd: opts.lockfileDir,
       optional: depNode.optional,
-      patchPath: depNode.patchFile?.path,
       pkgRoot: depNode.dir,
       rawConfig: opts.rawConfig,
       rootModulesDir: opts.rootModulesDir,
@@ -92,9 +98,12 @@ async function buildDependency (
       shellEmulator: opts.shellEmulator,
       unsafePerm: opts.unsafePerm || false,
     })
-    if (hasSideEffects && opts.sideEffectsCacheWrite) {
+    if ((isPatched || hasSideEffects) && opts.sideEffectsCacheWrite) {
       try {
-        const sideEffectsCacheKey = calcDepState(depGraph, opts.depsStateCache, depPath, depNode.patchFile?.hash)
+        const sideEffectsCacheKey = calcDepState(depGraph, opts.depsStateCache, depPath, {
+          patchFileHash: depNode.patchFile?.hash,
+          ignoreScripts: Boolean(opts.ignoreScripts),
+        })
         await opts.storeController.upload(depNode.dir, {
           sideEffectsCacheKey,
           filesIndexFile: depNode.filesIndexFile,
@@ -134,6 +143,18 @@ async function buildDependency (
   }
 }
 
+function applyPatchToDep (patchDir: string, patchFilePath: string) {
+  // Ideally, we would just run "patch" or "git apply".
+  // However, "patch" is not available on Windows and "git apply" is hard to execute on a subdirectory of an existing repository
+  const cwd = process.cwd()
+  process.chdir(patchDir)
+  applyPatch({
+    patchFilePath,
+    patchDir: patchDir,
+  })
+  process.chdir(cwd)
+}
+
 export async function linkBinsOfDependencies (
   depNode: DependenciesGraphNode,
   depGraph: DependenciesGraph,

packages/calc-dep-state/src/index.ts

@@ -22,12 +22,18 @@ export function calcDepState (
   depsGraph: DepsGraph,
   cache: DepsStateCache,
   depPath: string,
-  patchFileHash?: string
+  opts: {
+    patchFileHash?: string
+    ignoreScripts: boolean
+  }
 ): string {
-  const depStateObj = calcDepStateObj(depPath, depsGraph, cache, new Set())
-  let result = `${ENGINE_NAME}-${JSON.stringify(depStateObj)}`
-  if (patchFileHash) {
-    result += `-${patchFileHash}`
+  let result = ENGINE_NAME
+  if (!opts.ignoreScripts) {
+    const depStateObj = calcDepStateObj(depPath, depsGraph, cache, new Set())
+    result += `-${JSON.stringify(depStateObj)}`
+  }
+  if (opts.patchFileHash) {
+    result += `-${opts.patchFileHash}`
   }
   return result
 }

packages/calc-dep-state/test/index.ts

@@ -1,19 +1,29 @@
 import { calcDepState } from '@pnpm/calc-dep-state'
 import { ENGINE_NAME } from '@pnpm/constants'
 
+const depsGraph = {
+  'registry/foo/1.0.0': {
+    depPath: '/foo/1.0.0',
+    children: {
+      bar: 'registry/bar/1.0.0',
+    },
+  },
+  'registry/bar/1.0.0': {
+    depPath: '/bar/1.0.0',
+    children: {
+      foo: 'registry/foo/1.0.0',
+    },
+  },
+}
+
 test('calcDepState()', () => {
-  expect(calcDepState({
-    'registry/foo/1.0.0': {
-      depPath: '/foo/1.0.0',
-      children: {
-        bar: 'registry/bar/1.0.0',
-      },
-    },
-    'registry/bar/1.0.0': {
-      depPath: '/bar/1.0.0',
-      children: {
-        foo: 'registry/foo/1.0.0',
-      },
-    },
-  }, {}, '/registry/foo/1.0.0')).toBe(`${ENGINE_NAME}-{}`)
+  expect(calcDepState(depsGraph, {}, '/registry/foo/1.0.0', {
+    ignoreScripts: false,
+  })).toBe(`${ENGINE_NAME}-{}`)
+})
+
+test('calcDepState() when scripts are ignored', () => {
+  expect(calcDepState(depsGraph, {}, '/registry/foo/1.0.0', {
+    ignoreScripts: true,
+  })).toBe(ENGINE_NAME)
 })

packages/core/src/install/index.ts

@@ -839,6 +839,7 @@ const _installInContext: InstallFunction = async (projects, ctx, opts) => {
         hoistedDependencies: ctx.hoistedDependencies,
         hoistedModulesDir: ctx.hoistedModulesDir,
         hoistPattern: ctx.hoistPattern,
+        ignoreScripts: opts.ignoreScripts,
         include: opts.include,
         linkedDependenciesByProjectId,
         lockfileDir: opts.lockfileDir,
@@ -874,46 +875,50 @@ const _installInContext: InstallFunction = async (projects, ctx, opts) => {
     ctx.pendingBuilds = ctx.pendingBuilds
       .filter((relDepPath) => !result.removedDepPaths.has(relDepPath))
 
-    if (opts.ignoreScripts) {
-      // we can use concat here because we always only append new packages, which are guaranteed to not be there by definition
-      ctx.pendingBuilds = ctx.pendingBuilds
-        .concat(
-          await pFilter(result.newDepPaths,
-            (depPath) => {
-              const requiresBuild = dependenciesGraph[depPath].requiresBuild
-              if (typeof requiresBuild === 'function') return requiresBuild()
-              return requiresBuild
-            }
+    if (result.newDepPaths?.length) {
+      if (opts.ignoreScripts) {
+        // we can use concat here because we always only append new packages, which are guaranteed to not be there by definition
+        ctx.pendingBuilds = ctx.pendingBuilds
+          .concat(
+            await pFilter(result.newDepPaths,
+              (depPath) => {
+                const requiresBuild = dependenciesGraph[depPath].requiresBuild
+                if (typeof requiresBuild === 'function') return requiresBuild()
+                return requiresBuild
+              }
+            )
           )
-        )
-    } else if (result.newDepPaths?.length) {
-      // postinstall hooks
-      const depPaths = Object.keys(dependenciesGraph)
-      const rootNodes = depPaths.filter((depPath) => dependenciesGraph[depPath].depth === 0)
-
-      let extraEnv: Record<string, string> | undefined
-      if (opts.enablePnp) {
-        extraEnv = makeNodeRequireOption(path.join(opts.lockfileDir, '.pnp.cjs'))
       }
-      await buildModules(dependenciesGraph, rootNodes, {
-        childConcurrency: opts.childConcurrency,
-        depsStateCache,
-        depsToBuild: new Set(result.newDepPaths),
-        extraBinPaths: ctx.extraBinPaths,
-        extraNodePaths: ctx.extraNodePaths,
-        extraEnv,
-        lockfileDir: ctx.lockfileDir,
-        optional: opts.include.optionalDependencies,
-        rawConfig: opts.rawConfig,
-        rootModulesDir: ctx.virtualStoreDir,
-        scriptsPrependNodePath: opts.scriptsPrependNodePath,
-        scriptShell: opts.scriptShell,
-        shellEmulator: opts.shellEmulator,
-        sideEffectsCacheWrite: opts.sideEffectsCacheWrite,
-        storeController: opts.storeController,
-        unsafePerm: opts.unsafePerm,
-        userAgent: opts.userAgent,
-      })
+      if (!opts.ignoreScripts || Object.keys(opts.patchedDependencies ?? {}).length > 0) {
+        // postinstall hooks
+        const depPaths = Object.keys(dependenciesGraph)
+        const rootNodes = depPaths.filter((depPath) => dependenciesGraph[depPath].depth === 0)
+
+        let extraEnv: Record<string, string> | undefined
+        if (opts.enablePnp) {
+          extraEnv = makeNodeRequireOption(path.join(opts.lockfileDir, '.pnp.cjs'))
+        }
+        await buildModules(dependenciesGraph, rootNodes, {
+          childConcurrency: opts.childConcurrency,
+          depsStateCache,
+          depsToBuild: new Set(result.newDepPaths),
+          extraBinPaths: ctx.extraBinPaths,
+          extraNodePaths: ctx.extraNodePaths,
+          extraEnv,
+          ignoreScripts: opts.ignoreScripts,
+          lockfileDir: ctx.lockfileDir,
+          optional: opts.include.optionalDependencies,
+          rawConfig: opts.rawConfig,
+          rootModulesDir: ctx.virtualStoreDir,
+          scriptsPrependNodePath: opts.scriptsPrependNodePath,
+          scriptShell: opts.scriptShell,
+          shellEmulator: opts.shellEmulator,
+          sideEffectsCacheWrite: opts.sideEffectsCacheWrite,
+          storeController: opts.storeController,
+          unsafePerm: opts.unsafePerm,
+          userAgent: opts.userAgent,
+        })
+      }
     }
 
     const binWarn = (prefix: string, message: string) => logger.info({ message, prefix })

packages/core/src/install/link.ts

@@ -52,6 +52,7 @@ export default async function linkPackages (
     hoistedDependencies: HoistedDependencies
     hoistedModulesDir: string
     hoistPattern?: string[]
+    ignoreScripts: boolean
     publicHoistPattern?: string[]
     include: IncludedDependencies
     linkedDependenciesByProjectId: Record<string, LinkedDependency[]>
@@ -140,6 +141,7 @@ export default async function linkPackages (
     {
       force: opts.force,
       depsStateCache: opts.depsStateCache,
+      ignoreScripts: opts.ignoreScripts,
       lockfileDir: opts.lockfileDir,
       optional: opts.include.optionalDependencies,
       sideEffectsCacheRead: opts.sideEffectsCacheRead,
@@ -286,6 +288,7 @@ async function linkNewPackages (
     depsStateCache: DepsStateCache
     force: boolean
     optional: boolean
+    ignoreScripts: boolean
     lockfileDir: string
     sideEffectsCacheRead: boolean
     symlink: boolean
@@ -348,6 +351,7 @@ async function linkNewPackages (
       depGraph,
       depsStateCache: opts.depsStateCache,
       force: opts.force,
+      ignoreScripts: opts.ignoreScripts,
       lockfileDir: opts.lockfileDir,
       sideEffectsCacheRead: opts.sideEffectsCacheRead,
     }),
@@ -396,6 +400,7 @@ async function linkAllPkgs (
     depGraph: DependenciesGraph
     depsStateCache: DepsStateCache
     force: boolean
+    ignoreScripts: boolean
     lockfileDir: string
     sideEffectsCacheRead: boolean
   }
@@ -406,7 +411,10 @@ async function linkAllPkgs (
 
       let sideEffectsCacheKey: string | undefined
       if (opts.sideEffectsCacheRead && filesResponse.sideEffects && !isEmpty(filesResponse.sideEffects)) {
-        sideEffectsCacheKey = calcDepState(opts.depGraph, opts.depsStateCache, depNode.depPath, depNode.patchFile?.hash)
+        sideEffectsCacheKey = calcDepState(opts.depGraph, opts.depsStateCache, depNode.depPath, {
+          ignoreScripts: opts.ignoreScripts,
+          patchFileHash: depNode.patchFile?.hash,
+        })
       }
       if (typeof depNode.requiresBuild === 'function') {
         depNode.requiresBuild = await depNode.requiresBuild()

packages/core/test/install/patch.ts

@@ -146,3 +146,90 @@ test('the patched package is updated if the patch is modified', async () => {
   await install(manifest, opts)
   expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).toContain('// edited patch')
 })
+
+test('patch package when scripts are ignored', 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,
+    ignoreScripts: true,
+    sideEffectsCacheRead: true,
+    sideEffectsCacheWrite: true,
+    patchedDependencies,
+  }, {}, {}, { 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<PackageFilesIndex>(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,
+    ignoreScripts: true,
+    sideEffectsCacheRead: true,
+    sideEffectsCacheWrite: true,
+    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')
+})

packages/headless/src/index.ts

@@ -302,6 +302,7 @@ export default async (opts: HeadlessOptions) => {
     await linkHoistedModules(opts.storeController, graph, prevGraph, hierarchy, {
       depsStateCache,
       force: opts.force,
+      ignoreScripts: opts.ignoreScripts,
       lockfileDir: opts.lockfileDir,
       sideEffectsCacheRead: opts.sideEffectsCacheRead,
     })
@@ -331,6 +332,7 @@ export default async (opts: HeadlessOptions) => {
         force: opts.force,
         depGraph: graph,
         depsStateCache,
+        ignoreScripts: opts.ignoreScripts,
         lockfileDir: opts.lockfileDir,
         sideEffectsCacheRead: opts.sideEffectsCacheRead,
       }),
@@ -404,7 +406,8 @@ export default async (opts: HeadlessOptions) => {
           .filter(({ requiresBuild }) => requiresBuild)
           .map(({ depPath }) => depPath)
       )
-  } else {
+  }
+  if (!opts.ignoreScripts || Object.keys(opts.patchedDependencies ?? {}).length > 0) {
     const directNodes = new Set<string>()
     for (const id of union(importerIds, ['.'])) {
       Object
@@ -427,6 +430,7 @@ export default async (opts: HeadlessOptions) => {
       extraBinPaths,
       extraEnv,
       depsStateCache,
+      ignoreScripts: opts.ignoreScripts,
       lockfileDir,
       optional: opts.include.optionalDependencies,
       rawConfig: opts.rawConfig,
@@ -667,6 +671,7 @@ async function linkAllPkgs (
     depGraph: DependenciesGraph
     depsStateCache: DepsStateCache
     force: boolean
+    ignoreScripts: boolean
     lockfileDir: string
     sideEffectsCacheRead: boolean
   }
@@ -683,7 +688,10 @@ async function linkAllPkgs (
 
       let sideEffectsCacheKey: string | undefined
       if (opts.sideEffectsCacheRead && filesResponse.sideEffects && !isEmpty(filesResponse.sideEffects)) {
-        sideEffectsCacheKey = calcDepState(opts.depGraph, opts.depsStateCache, depNode.dir, depNode.patchFile?.hash)
+        sideEffectsCacheKey = calcDepState(opts.depGraph, opts.depsStateCache, depNode.dir, {
+          ignoreScripts: opts.ignoreScripts,
+          patchFileHash: depNode.patchFile?.hash,
+        })
       }
       const { importMethod, isBuilt } = await storeController.importPackage(depNode.dir, {
         filesResponse,

packages/headless/src/linkHoistedModules.ts

@@ -27,6 +27,7 @@ export default async function linkHoistedModules (
   opts: {
     depsStateCache: DepsStateCache
     force: boolean
+    ignoreScripts: boolean
     lockfileDir: string
     sideEffectsCacheRead: boolean
   }
@@ -82,6 +83,7 @@ async function linkAllPkgsInOrder (
   opts: {
     depsStateCache: DepsStateCache
     force: boolean
+    ignoreScripts: boolean
     lockfileDir: string
     sideEffectsCacheRead: boolean
     warn: (message: string) => void
@@ -101,7 +103,10 @@ async function linkAllPkgsInOrder (
 
       let sideEffectsCacheKey: string | undefined
       if (opts.sideEffectsCacheRead && filesResponse.sideEffects && !isEmpty(filesResponse.sideEffects)) {
-        sideEffectsCacheKey = _calcDepState(dir, depNode.patchFile?.hash)
+        sideEffectsCacheKey = _calcDepState(dir, {
+          patchFileHash: depNode.patchFile?.hash,
+          ignoreScripts: opts.ignoreScripts,
+        })
       }
       const { importMethod, isBuilt } = await storeController.importPackage(depNode.dir, {
         filesResponse,

packages/lifecycle/package.json

@@ -41,7 +41,6 @@
     "@pnpm/read-package-json": "workspace:6.0.5",
     "@pnpm/store-controller-types": "workspace:14.0.0",
     "@pnpm/types": "workspace:8.3.0",
-    "patch-package": "^6.4.7",
     "path-exists": "^4.0.0",
     "run-groups": "^3.0.1"
   },

packages/lifecycle/src/index.ts

@@ -3,7 +3,6 @@ import { safeReadPackageFromDir } from '@pnpm/read-package-json'
 import exists from 'path-exists'
 import runLifecycleHook, { RunLifecycleHookOptions } from './runLifecycleHook'
 import runLifecycleHooksConcurrently, { RunLifecycleHooksConcurrentlyOptions } from './runLifecycleHooksConcurrently'
-import { applyPatch } from 'patch-package/dist/applyPatches'
 
 export function makeNodeRequireOption (modulePath: string) {
   let { NODE_OPTIONS } = process.env
@@ -26,17 +25,6 @@ export async function runPostinstallHooks (
   if (pkg.scripts == null) {
     pkg.scripts = {}
   }
-  if (opts.patchPath) {
-    // Ideally, we would just run "patch" or "git apply".
-    // However, "patch" is not available on Windows and "git apply" is hard to execute on a subdirectory of an existing repository
-    const cwd = process.cwd()
-    process.chdir(opts.pkgRoot)
-    applyPatch({
-      patchFilePath: opts.patchPath,
-      patchDir: opts.pkgRoot,
-    })
-    process.chdir(cwd)
-  }
 
   if (!pkg.scripts.install) {
     await checkBindingGyp(opts.pkgRoot, pkg.scripts)
@@ -54,8 +42,7 @@ export async function runPostinstallHooks (
 
   return pkg.scripts.preinstall != null ||
     pkg.scripts.install != null ||
-    pkg.scripts.postinstall != null ||
-    Boolean(opts.patchPath)
+    pkg.scripts.postinstall != null
 }
 
 /**

packages/lifecycle/src/runLifecycleHook.ts

@@ -12,7 +12,6 @@ export interface RunLifecycleHookOptions {
   extraEnv?: Record<string, string>
   initCwd?: string
   optional?: boolean
-  patchPath?: string
   pkgRoot: string
   rawConfig: object
   rootModulesDir: string

pnpm-lock.yaml

@@ -184,6 +184,7 @@ importers:
       '@pnpm/store-controller-types': workspace:14.0.0
       '@pnpm/types': workspace:8.3.0
       '@types/ramda': 0.27.39
+      patch-package: ^6.4.7
       ramda: ^0.27.1
       run-groups: ^3.0.1
     dependencies:
@@ -195,6 +196,7 @@ importers:
       '@pnpm/read-package-json': link:../read-package-json
       '@pnpm/store-controller-types': link:../store-controller-types
       '@pnpm/types': link:../types
+      patch-package: 6.4.7
       ramda: 0.27.2
       run-groups: 3.0.1
     devDependencies:
@@ -1266,7 +1268,6 @@ importers:
       '@zkochan/rimraf': ^2.1.2
       json-append: 1.1.1
       load-json-file: ^6.2.0
-      patch-package: ^6.4.7
       path-exists: ^4.0.0
       run-groups: ^3.0.1
     dependencies:
@@ -1276,7 +1277,6 @@ importers:
       '@pnpm/read-package-json': link:../read-package-json
       '@pnpm/store-controller-types': link:../store-controller-types
       '@pnpm/types': link:../types
-      patch-package: 6.4.7
       path-exists: 4.0.0
       run-groups: 3.0.1
     devDependencies: