feat!: remove requiresBuild from the lockfile

pnpm · Feb 24, 2024 · 8c681d1 · 8c681d1
1 parent 75840d6
commit 8c681d1
Show file tree

Hide file tree

Showing 31 changed files with 133 additions and 182 deletions.
diff --git a/deps/graph-builder/src/lockfileToDepGraph.ts b/deps/graph-builder/src/lockfileToDepGraph.ts
@@ -41,8 +41,7 @@ export interface DependenciesGraphNode {
   optional: boolean
   depPath: string // this option is only needed for saving pendingBuild when running with --ignore-scripts flag
   isBuilt?: boolean
-  requiresBuild: boolean
-  prepare: boolean
+  requiresBuild?: boolean
   hasBin: boolean
   filesIndexFile?: string
   patchFile?: PatchFile
@@ -195,8 +194,6 @@ export async function lockfileToDepGraph (
           name: pkgName,
           optional: !!pkgSnapshot.optional,
           optionalDependencies: new Set(Object.keys(pkgSnapshot.optionalDependencies ?? {})),
-          prepare: pkgSnapshot.prepare === true,
-          requiresBuild: pkgSnapshot.requiresBuild === true,
           patchFile: opts.patchedDependencies?.[`${pkgName}@${pkgVersion}`],
         }
         pkgSnapshotByLocation[dir] = pkgSnapshot

diff --git a/env/node.fetcher/src/index.ts b/env/node.fetcher/src/index.ts
@@ -50,6 +50,7 @@ export async function fetchNode (fetch: FetchFromRegistry, version: string, targ
     filesResponse: {
       filesIndex: filesIndex as Record<string, string>,
       resolvedFrom: 'remote',
+      requiresBuild: false,
     },
     force: true,
   })

diff --git a/fetching/directory-fetcher/package.json b/fetching/directory-fetcher/package.json
@@ -34,6 +34,7 @@
     "@pnpm/logger": "^5.0.0"
   },
   "dependencies": {
+    "@pnpm/exec.files-include-install-scripts": "1.0.0",
     "@pnpm/fetcher-base": "workspace:*",
     "@pnpm/fs.packlist": "workspace:*",
     "@pnpm/read-project-manifest": "workspace:*",

diff --git a/fetching/directory-fetcher/src/index.ts b/fetching/directory-fetcher/src/index.ts
@@ -1,5 +1,6 @@
 import { promises as fs, type Stats } from 'fs'
 import path from 'path'
+import { filesIncludeInstallScripts } from '@pnpm/exec.files-include-install-scripts'
 import type { DirectoryFetcher, DirectoryFetcherOptions } from '@pnpm/fetcher-base'
 import { logger } from '@pnpm/logger'
 import { packlist } from '@pnpm/fs.packlist'
@@ -55,11 +56,20 @@ async function fetchAllFilesFromDir (
     // Related PR in Bit: https://github.com/teambit/bit/pull/5251
     manifest = await safeReadProjectManifestOnly(dir) as DependencyManifest ?? undefined
   }
+  const requiresBuild = Boolean(
+    manifest?.scripts != null && (
+      Boolean(manifest.scripts.preinstall) ||
+      Boolean(manifest.scripts.install) ||
+      Boolean(manifest.scripts.postinstall)
+    ) ||
+    filesIncludeInstallScripts(filesIndex)
+  )
   return {
     local: true as const,
     filesIndex,
     packageImportMethod: 'hardlink' as const,
     manifest,
+    requiresBuild,
   }
 }
 
@@ -137,10 +147,19 @@ async function fetchPackageFilesFromDir (
     // Related PR in Bit: https://github.com/teambit/bit/pull/5251
     manifest = await safeReadProjectManifestOnly(dir) as DependencyManifest ?? undefined
   }
+  const requiresBuild = Boolean(
+    manifest?.scripts != null && (
+      Boolean(manifest.scripts.preinstall) ||
+      Boolean(manifest.scripts.install) ||
+      Boolean(manifest.scripts.postinstall)
+    ) ||
+    filesIncludeInstallScripts(filesIndex)
+  )
   return {
     local: true as const,
     filesIndex,
     packageImportMethod: 'hardlink' as const,
     manifest,
+    requiresBuild,
   }
 }
diff --git a/fetching/fetcher-base/src/index.ts b/fetching/fetcher-base/src/index.ts
@@ -26,6 +26,7 @@ export interface FetchResult {
   local?: boolean
   manifest?: DependencyManifest
   filesIndex: Record<string, string>
+  requiresBuild: boolean
 }
 
 export interface GitFetcherOptions {
@@ -34,7 +35,13 @@ export interface GitFetcherOptions {
   pkg?: PkgNameVersion
 }
 
-export type GitFetcher = FetchFunction<GitResolution, GitFetcherOptions, { filesIndex: Record<string, string>, manifest?: DependencyManifest }>
+export interface GitFetcherResult {
+  filesIndex: Record<string, string>
+  manifest?: DependencyManifest
+  requiresBuild: boolean
+}
+
+export type GitFetcher = FetchFunction<GitResolution, GitFetcherOptions, GitFetcherResult>
 
 export interface DirectoryFetcherOptions {
   lockfileDir: string
@@ -46,6 +53,7 @@ export interface DirectoryFetcherResult {
   filesIndex: Record<string, string>
   packageImportMethod: 'hardlink'
   manifest?: DependencyManifest
+  requiresBuild: boolean
 }
 
 export type DirectoryFetcher = FetchFunction<DirectoryResolution, DirectoryFetcherOptions, DirectoryFetcherResult>

diff --git a/fetching/tarball-fetcher/src/gitHostedTarballFetcher.ts b/fetching/tarball-fetcher/src/gitHostedTarballFetcher.ts
@@ -25,7 +25,7 @@ export interface CreateGitHostedTarballFetcher {
 export function createGitHostedTarballFetcher (fetchRemoteTarball: FetchFunction, fetcherOpts: CreateGitHostedTarballFetcher): FetchFunction {
   const fetch = async (cafs: Cafs, resolution: Resolution, opts: FetchOptions) => {
     const tempIndexFile = pathTemp(opts.filesIndexFile)
-    const { filesIndex, manifest } = await fetchRemoteTarball(cafs, resolution, {
+    const { filesIndex, manifest, requiresBuild } = await fetchRemoteTarball(cafs, resolution, {
       ...opts,
       filesIndexFile: tempIndexFile,
     })
@@ -37,6 +37,7 @@ export function createGitHostedTarballFetcher (fetchRemoteTarball: FetchFunction
       return {
         filesIndex: prepareResult.filesIndex,
         manifest: prepareResult.manifest ?? manifest,
+        requiresBuild,
       }
     } catch (err: any) { // eslint-disable-line
       err.message = `Failed to prepare git-hosted package fetched from "${resolution.tarball}": ${err.message}`
@@ -67,6 +68,7 @@ async function prepareGitHostedPkg (
     filesResponse: {
       filesIndex,
       resolvedFrom: 'remote',
+      requiresBuild: false,
     },
     force: true,
   })

diff --git a/lockfile/lockfile-types/src/index.ts b/lockfile/lockfile-types/src/index.ts
@@ -77,9 +77,7 @@ export interface PackageSnapshot {
   id?: string
   dev?: true | false
   optional?: true
-  requiresBuild?: true
   patched?: true
-  prepare?: true
   hasBin?: true
   // name and version are only needed
   // for packages that are hosted not in the npm registry

diff --git a/pkg-manager/core/src/install/index.ts b/pkg-manager/core/src/install/index.ts
@@ -994,7 +994,6 @@ const _installInContext: InstallFunction = async (projects, ctx, opts) => {
   let {
     dependenciesGraph,
     dependenciesByProjectId,
-    finishLockfileUpdates,
     linkedDependenciesByProjectId,
     newLockfile,
     outdatedDependencies,
@@ -1129,7 +1128,6 @@ const _installInContext: InstallFunction = async (projects, ctx, opts) => {
       }
     )
     stats = result.stats
-    await finishLockfileUpdates()
     if (opts.enablePnp) {
       const importerNames = Object.fromEntries(
         projects.map(({ manifest, id }) => [id, manifest.name ?? id])
@@ -1150,13 +1148,10 @@ const _installInContext: InstallFunction = async (projects, ctx, opts) => {
         // 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
-              }
-            )
+            result.newDepPaths.filter((depPath) => {
+              const requiresBuild = dependenciesGraph[depPath].requiresBuild
+              return requiresBuild
+            })
           )
       }
       if (!opts.ignoreScripts || Object.keys(opts.patchedDependencies ?? {}).length > 0) {
@@ -1331,7 +1326,6 @@ const _installInContext: InstallFunction = async (projects, ctx, opts) => {
       )
     }
   } else {
-    await finishLockfileUpdates()
     if (opts.useLockfile && !isInstallationOnlyForLockfileCheck) {
       await writeWantedLockfile(ctx.lockfileDir, newLockfile, lockfileOpts)
     }

diff --git a/pkg-manager/core/src/install/link.ts b/pkg-manager/core/src/install/link.ts
@@ -447,9 +447,7 @@ async function linkAllPkgs (
     depNodes.map(async (depNode) => {
       const { files } = await depNode.fetching()
 
-      if (typeof depNode.requiresBuild === 'function') {
-        depNode.requiresBuild = await depNode.requiresBuild()
-      }
+      depNode.requiresBuild = files.requiresBuild
       let sideEffectsCacheKey: string | undefined
       if (opts.sideEffectsCacheRead && files.sideEffects && !isEmpty(files.sideEffects)) {
         sideEffectsCacheKey = calcDepState(opts.depGraph, opts.depsStateCache, depNode.depPath, {
@@ -462,7 +460,7 @@ async function linkAllPkgs (
         filesResponse: files,
         force: opts.force,
         sideEffectsCacheKey,
-        requiresBuild: depNode.patchFile != null || (depNode.optional ? (depNode.requiresBuild ? undefined : false) : depNode.requiresBuild),
+        requiresBuild: depNode.patchFile != null || files.requiresBuild,
       })
       if (importMethod) {
         progressLogger.debug({

diff --git a/pkg-manager/core/test/install/fixLockfile.ts b/pkg-manager/core/test/install/fixLockfile.ts
@@ -34,7 +34,6 @@ test('fix broken lockfile with --fix-lockfile', async () => {
         resolution: {
           integrity: 'sha512-oxKe64UH049mJqrKkynWp6Vu0Rlm/BTXO/bJZuN2mmR3RtOFNepLlSWDd1eo16PzHpQAoNG97rLU1V/YxesJjw==',
         },
-        // requiresBuild: true,
         // dev: true
       },
     },
@@ -59,7 +58,6 @@ test('fix broken lockfile with --fix-lockfile', async () => {
   expect(lockfile.packages?.['/core-js-pure@3.16.2']?.resolution).toEqual({
     integrity: 'sha512-oxKe64UH049mJqrKkynWp6Vu0Rlm/BTXO/bJZuN2mmR3RtOFNepLlSWDd1eo16PzHpQAoNG97rLU1V/YxesJjw==',
   })
-  expect(lockfile.packages?.['/core-js-pure@3.16.2']?.requiresBuild).toBeTruthy()
   expect(lockfile.packages?.['/core-js-pure@3.16.2']?.dev).toBeTruthy()
 })
 
@@ -141,12 +139,10 @@ test('--fix-lockfile should preserve all locked dependencies version', async ()
       },
       '/core-js-pure@3.17.2': {
         resolution: { integrity: 'sha512-2VV7DlIbooyTI7Bh+yzOOWL9tGwLnQKHno7qATE+fqZzDKYr6llVjVQOzpD/QLZFgXDPb8T71pJokHEZHEYJhQ==' },
-        // requiresBuild: true,
         dev: false,
       },
       '/core-js-pure@3.17.3': {
       // resolution: { integrity: 'sha512-YusrqwiOTTn8058JDa0cv9unbXdIiIgcgI9gXso0ey4WgkFLd3lYlV9rp9n7nDCsYxXsMDTjA4m1h3T348mdlQ==' },
-      // requiresBuild: true,
       // dev: false
       },
       '/regenerator-runtime@0.13.9': {
@@ -217,14 +213,12 @@ test('--fix-lockfile should preserve all locked dependencies version', async ()
 
   expect(lockfile.packages?.['/core-js-pure@3.17.2']).toBeTruthy()
   expect(lockfile.packages?.['/core-js-pure@3.17.2']?.resolution).toHaveProperty('integrity', 'sha512-2VV7DlIbooyTI7Bh+yzOOWL9tGwLnQKHno7qATE+fqZzDKYr6llVjVQOzpD/QLZFgXDPb8T71pJokHEZHEYJhQ==')
-  expect(lockfile.packages?.['/core-js-pure@3.17.2']?.requiresBuild).toBeTruthy()
   expect(lockfile.packages?.['/core-js-pure@3.17.2']?.dev).toBeFalsy()
 
   expect(lockfile.packages?.['/core-js-pure@3.17.3']).toBeTruthy()
   expect(lockfile.packages?.['/core-js-pure@3.17.3']?.resolution).toEqual({
     integrity: 'sha512-YusrqwiOTTn8058JDa0cv9unbXdIiIgcgI9gXso0ey4WgkFLd3lYlV9rp9n7nDCsYxXsMDTjA4m1h3T348mdlQ==',
   })
-  expect(lockfile.packages?.['/core-js-pure@3.17.3']?.requiresBuild).toBeTruthy()
   expect(lockfile.packages?.['/core-js-pure@3.17.3']?.dev).toBeFalsy()
 
   expect(lockfile.packages?.['/regenerator-runtime@0.13.9']).toBeTruthy()

diff --git a/pkg-manager/core/test/install/lifecycleScripts.ts b/pkg-manager/core/test/install/lifecycleScripts.ts
@@ -53,9 +53,6 @@ test('run pre/postinstall scripts', async () => {
     const generatedByPostinstall = project.requireModule('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-postinstall')
     expect(typeof generatedByPostinstall).toBe('function')
   }
-
-  const lockfile = project.readLockfile()
-  expect(lockfile.packages['/@pnpm.e2e/pre-and-postinstall-scripts-example@1.0.0'].requiresBuild)
 })
 
 test('run pre/postinstall scripts, when PnP is used and no symlinks', async () => {
@@ -291,9 +288,6 @@ test('run prepare script for git-hosted dependencies', async () => {
     'install',
     'postinstall',
   ])
-
-  const lockfile = project.readLockfile()
-  expect(lockfile.packages['github.com/pnpm/test-git-fetch/d222f6bfbdea55c032fdb5f0538d52b2a484bbbf'].prepare === true).toBeTruthy()
 })
 
 test('lifecycle scripts run before linking bins', async () => {
@@ -424,8 +418,6 @@ test('selectively ignore scripts in some dependencies by neverBuiltDependencies'
 
   const lockfile = project.readLockfile()
   expect(lockfile.neverBuiltDependencies).toStrictEqual(neverBuiltDependencies)
-  expect(lockfile.packages['/@pnpm.e2e/pre-and-postinstall-scripts-example@1.0.0'].requiresBuild).toBe(undefined)
-  expect(lockfile.packages['/@pnpm.e2e/install-script-example@1.0.0'].requiresBuild).toBeTruthy()
 
   rimraf('node_modules')
 
@@ -462,8 +454,6 @@ test('selectively allow scripts in some dependencies by onlyBuiltDependencies',
 
   const lockfile = project.readLockfile()
   expect(lockfile.onlyBuiltDependencies).toStrictEqual(onlyBuiltDependencies)
-  expect(lockfile.packages['/@pnpm.e2e/pre-and-postinstall-scripts-example@1.0.0'].requiresBuild).toBe(undefined)
-  expect(lockfile.packages['/@pnpm.e2e/install-script-example@1.0.0'].requiresBuild).toBe(true)
 
   rimraf('node_modules')
 
@@ -527,8 +517,6 @@ test('lockfile is updated if neverBuiltDependencies is changed', async () => {
   {
     const lockfile = project.readLockfile()
     expect(lockfile.neverBuiltDependencies).toBeFalsy()
-    expect(lockfile.packages['/@pnpm.e2e/pre-and-postinstall-scripts-example@1.0.0'].requiresBuild).toBeTruthy()
-    expect(lockfile.packages['/@pnpm.e2e/install-script-example@1.0.0'].requiresBuild).toBeTruthy()
   }
 
   const neverBuiltDependencies = ['@pnpm.e2e/pre-and-postinstall-scripts-example']
@@ -541,8 +529,6 @@ test('lockfile is updated if neverBuiltDependencies is changed', async () => {
   {
     const lockfile = project.readLockfile()
     expect(lockfile.neverBuiltDependencies).toStrictEqual(neverBuiltDependencies)
-    expect(lockfile.packages['/@pnpm.e2e/pre-and-postinstall-scripts-example@1.0.0'].requiresBuild).toBe(undefined)
-    expect(lockfile.packages['/@pnpm.e2e/install-script-example@1.0.0'].requiresBuild).toBeTruthy()
   }
 })
 
@@ -556,8 +542,6 @@ test('lockfile is updated if onlyBuiltDependencies is changed', async () => {
   {
     const lockfile = project.readLockfile()
     expect(lockfile.onlyBuiltDependencies).toBeFalsy()
-    expect(lockfile.packages['/@pnpm.e2e/pre-and-postinstall-scripts-example@1.0.0'].requiresBuild).toBeTruthy()
-    expect(lockfile.packages['/@pnpm.e2e/install-script-example@1.0.0'].requiresBuild).toBeTruthy()
   }
 
   const onlyBuiltDependencies: string[] = []
@@ -570,8 +554,6 @@ test('lockfile is updated if onlyBuiltDependencies is changed', async () => {
   {
     const lockfile = project.readLockfile()
     expect(lockfile.onlyBuiltDependencies).toStrictEqual(onlyBuiltDependencies)
-    expect(lockfile.packages['/@pnpm.e2e/pre-and-postinstall-scripts-example@1.0.0'].requiresBuild).toBe(undefined)
-    expect(lockfile.packages['/@pnpm.e2e/install-script-example@1.0.0'].requiresBuild).toBe(undefined)
   }
 
   onlyBuiltDependencies.push('@pnpm.e2e/pre-and-postinstall-scripts-example')
@@ -584,8 +566,6 @@ test('lockfile is updated if onlyBuiltDependencies is changed', async () => {
   {
     const lockfile = project.readLockfile()
     expect(lockfile.onlyBuiltDependencies).toStrictEqual(onlyBuiltDependencies)
-    expect(lockfile.packages['/@pnpm.e2e/pre-and-postinstall-scripts-example@1.0.0'].requiresBuild).toBe(true)
-    expect(lockfile.packages['/@pnpm.e2e/install-script-example@1.0.0'].requiresBuild).toBe(undefined)
   }
 })
 

diff --git a/pkg-manager/core/test/install/misc.ts b/pkg-manager/core/test/install/misc.ts
@@ -546,14 +546,11 @@ test('bin specified in the directories property symlinked to .bin folder when pr
 })
 
 testOnNonWindows('building native addons', async () => {
-  const project = prepareEmpty()
+  prepareEmpty()
 
   await addDependenciesToPackage({}, ['diskusage@1.1.3'], testDefaults({ fastUnpack: false }))
 
   expect(fs.existsSync('node_modules/diskusage/build')).toBeTruthy()
-
-  const lockfile = project.readLockfile()
-  expect(lockfile.packages).toHaveProperty(['/diskusage@1.1.3', 'requiresBuild'], true)
 })
 
 test('should update subdep on second install', async () => {

diff --git a/pkg-manager/core/test/install/optionalDependencies.ts b/pkg-manager/core/test/install/optionalDependencies.ts
@@ -75,10 +75,6 @@ test('skip optional dependency that does not support the current OS', async () =
   const lockfile = project.readLockfile()
   expect(lockfile.packages['/@pnpm.e2e/not-compatible-with-any-os@1.0.0']).toBeTruthy()
 
-  // optional dependencies always get requiresBuild: true
-  // this is to resolve https://github.com/pnpm/pnpm/issues/2038
-  expect(lockfile.packages['/@pnpm.e2e/not-compatible-with-any-os@1.0.0'].requiresBuild).toBeTruthy()
-
   expect(lockfile.packages['/@pnpm.e2e/dep-of-optional-pkg@1.0.0']).toBeTruthy()
 
   const currentLockfile = project.readCurrentLockfile()

diff --git a/pkg-manager/headless/src/index.ts b/pkg-manager/headless/src/index.ts
@@ -828,6 +828,7 @@ async function linkAllPkgs (
         throw err
       }
 
+      depNode.requiresBuild = filesResponse.requiresBuild
       let sideEffectsCacheKey: string | undefined
       if (opts.sideEffectsCacheRead && filesResponse.sideEffects && !isEmpty(filesResponse.sideEffects)) {
         sideEffectsCacheKey = calcDepState(opts.depGraph, opts.depsStateCache, depNode.dir, {
@@ -839,7 +840,7 @@ async function linkAllPkgs (
         filesResponse,
         force: opts.force,
         disableRelinkLocalDirDeps: opts.disableRelinkLocalDirDeps,
-        requiresBuild: depNode.patchFile != null || (depNode.optional ? (depNode.requiresBuild ? undefined : false) : depNode.requiresBuild),
+        requiresBuild: depNode.patchFile != null || filesResponse.requiresBuild,
         sideEffectsCacheKey,
       })
       if (importMethod) {

diff --git a/pkg-manager/headless/src/linkHoistedModules.ts b/pkg-manager/headless/src/linkHoistedModules.ts
@@ -111,6 +111,7 @@ async function linkAllPkgsInOrder (
           throw err
         }
 
+        depNode.requiresBuild = filesResponse.requiresBuild
         let sideEffectsCacheKey: string | undefined
         if (opts.sideEffectsCacheRead && filesResponse.sideEffects && !isEmpty(filesResponse.sideEffects)) {
           sideEffectsCacheKey = _calcDepState(dir, {
@@ -127,7 +128,7 @@ async function linkAllPkgsInOrder (
             force: true,
             disableRelinkLocalDirDeps: opts.disableRelinkLocalDirDeps,
             keepModulesDir: true,
-            requiresBuild: depNode.patchFile != null || (depNode.optional ? (depNode.requiresBuild ? undefined : false) : depNode.requiresBuild),
+            requiresBuild: depNode.patchFile != null || filesResponse.requiresBuild,
             sideEffectsCacheKey,
           })
           if (importMethod) {