fix: built packages should not modify the original files in the store (…

…#4898)
pnpm · Jun 18, 2022 · 79f2843 · 79f2843
1 parent 9963135
commit 79f2843
Show file tree

Hide file tree

Showing 36 changed files with 772 additions and 646 deletions.
diff --git a/.changeset/good-taxis-laugh.md b/.changeset/good-taxis-laugh.md
@@ -0,0 +1,5 @@
+---
+"@pnpm/build-modules": patch
+---
+
+`requiresBuild` may be of any value. This is just a workaround to a typing issue. `requiresBuild` will always be boolean.
diff --git a/.changeset/hungry-dogs-draw.md b/.changeset/hungry-dogs-draw.md
@@ -0,0 +1,5 @@
+---
+"@pnpm/resolve-dependencies": major
+---
+
+`requiresBuild` is sometimes a function that return a boolean promise.
diff --git a/.changeset/seven-cats-lie.md b/.changeset/seven-cats-lie.md
@@ -0,0 +1,7 @@
+---
+"@pnpm/core": patch
+"@pnpm/headless": patch
+"pnpm": patch
+---
+
+Packages that should be built are always cloned or copied from the store. This is required to prevent the postinstall scripts from modifying the original source files of the package.
diff --git a/.changeset/sour-stingrays-tell.md b/.changeset/sour-stingrays-tell.md
@@ -0,0 +1,5 @@
+---
+"@pnpm/package-requester": patch
+---
+
+Use `safe-promise-defer`.
diff --git a/.changeset/stale-cheetahs-warn.md b/.changeset/stale-cheetahs-warn.md
@@ -0,0 +1,6 @@
+---
+"@pnpm/create-cafs-store": minor
+"@pnpm/fetcher-base": minor
+---
+
+New optional option added to package importer: `requiresBuild`. When `requiresBuild` is `true`, the package should only be imported using cloning or copying.
diff --git a/.changeset/tiny-ravens-brake.md b/.changeset/tiny-ravens-brake.md
@@ -0,0 +1,5 @@
+---
+"@pnpm/create-cafs-store": minor
+---
+
+New import method added: `clone-or-copy`.
diff --git a/package.json b/package.json
@@ -38,7 +38,7 @@
     "@commitlint/prompt-cli": "^16.3.0",
     "@pnpm/eslint-config": "workspace:*",
     "@pnpm/meta-updater": "0.0.6",
-    "@pnpm/registry-mock": "2.19.0",
+    "@pnpm/registry-mock": "2.20.0",
     "@pnpm/tsconfig": "workspace:*",
     "@types/jest": "^27.5.2",
     "@types/node": "^14.18.20",

diff --git a/packages/build-modules/src/buildSequence.ts b/packages/build-modules/src/buildSequence.ts
@@ -14,7 +14,8 @@ export interface DependenciesGraphNode {
   isBuilt?: boolean
   optional: boolean
   optionalDependencies: Set<string>
-  requiresBuild?: boolean
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  requiresBuild?: boolean | any // this is a durty workaround added in https://github.com/pnpm/pnpm/pull/4898
 }
 
 export interface DependenciesGraph {

diff --git a/packages/core/package.json b/packages/core/package.json
@@ -75,7 +75,7 @@
     "@pnpm/logger": "^4.0.0",
     "@pnpm/package-store": "workspace:12.1.16",
     "@pnpm/prepare": "workspace:*",
-    "@pnpm/registry-mock": "2.19.0",
+    "@pnpm/registry-mock": "2.20.0",
     "@pnpm/store-path": "^5.0.0",
     "@pnpm/test-fixtures": "workspace:*",
     "@types/fs-extra": "^9.0.13",

diff --git a/packages/core/src/install/index.ts b/packages/core/src/install/index.ts
@@ -831,8 +831,13 @@ 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(
-          result.newDepPaths
-            .filter((depPath) => dependenciesGraph[depPath].requiresBuild)
+          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

diff --git a/packages/core/src/install/link.ts b/packages/core/src/install/link.ts
@@ -409,10 +409,14 @@ async function linkAllPkgs (
       if (opts.sideEffectsCacheRead && filesResponse.sideEffects && !isEmpty(filesResponse.sideEffects)) {
         targetEngine = calcDepState(depNode.depPath, opts.depGraph, opts.depsStateCache)
       }
+      if (typeof depNode.requiresBuild === 'function') {
+        depNode.requiresBuild = await depNode.requiresBuild()
+      }
       const { importMethod, isBuilt } = await storeController.importPackage(depNode.dir, {
         filesResponse,
         force: opts.force,
         targetEngine,
+        requiresBuild: depNode.requiresBuild,
       })
       if (importMethod) {
         progressLogger.debug({

diff --git a/packages/core/test/install/sideEffects.ts b/packages/core/test/install/sideEffects.ts
@@ -1,8 +1,8 @@
-import { promises as fs } from 'fs'
+import { promises as fs, readFileSync } from 'fs'
 import path from 'path'
 import { addDependenciesToPackage } from '@pnpm/core'
-import { PackageFilesIndex } from '@pnpm/cafs'
-import { REGISTRY_MOCK_PORT } from '@pnpm/registry-mock'
+import { getFilePathInCafs, PackageFilesIndex } from '@pnpm/cafs'
+import { getIntegrity, REGISTRY_MOCK_PORT } from '@pnpm/registry-mock'
 import { prepareEmpty } from '@pnpm/prepare'
 import { ENGINE_NAME } from '@pnpm/constants'
 import rimraf from '@zkochan/rimraf'
@@ -159,3 +159,28 @@ test('uploading errors do not interrupt installation', async () => {
   const filesIndex = await loadJsonFile<PackageFilesIndex>(filesIndexFile)
   expect(filesIndex.sideEffects).toBeFalsy()
 })
+
+test('a postinstall script does not modify the original sources added to the store', async () => {
+  prepareEmpty()
+
+  const opts = await testDefaults({
+    fastUnpack: false,
+    sideEffectsCacheRead: true,
+    sideEffectsCacheWrite: true,
+  }, {}, {}, { packageImportMethod: 'hardlink' })
+  await addDependenciesToPackage({}, ['@pnpm/postinstall-modifies-source@1.0.0'], opts)
+
+  expect(readFileSync('node_modules/@pnpm/postinstall-modifies-source/empty-file.txt', 'utf8')).toContain('hello')
+
+  const cafsDir = path.join(opts.storeDir, 'files')
+  const filesIndexFile = getFilePathInCafs(cafsDir, getIntegrity('@pnpm/postinstall-modifies-source', '1.0.0'), 'index')
+  const filesIndex = await loadJsonFile<PackageFilesIndex>(filesIndexFile)
+  const patchedFileIntegrity = filesIndex.sideEffects?.[`${ENGINE_NAME}-{}`]['empty-file.txt']?.integrity
+  expect(patchedFileIntegrity).toBeTruthy()
+  const originalFileIntegrity = filesIndex.files['empty-file.txt'].integrity
+  expect(originalFileIntegrity).toBeTruthy()
+  // The integrity of the original file differs from the integrity of the patched file
+  expect(originalFileIntegrity).not.toEqual(patchedFileIntegrity)
+
+  expect(readFileSync(getFilePathInCafs(cafsDir, originalFileIntegrity, 'nonexec'), 'utf8')).toEqual('')
+})
diff --git a/packages/fetcher-base/src/index.ts b/packages/fetcher-base/src/index.ts
@@ -21,13 +21,16 @@ export type PackageFilesResponse = {
   filesIndex: Record<string, PackageFileInfo>
 })
 
+export interface ImportPackageOpts {
+  requiresBuild?: boolean
+  targetEngine?: string
+  filesResponse: PackageFilesResponse
+  force: boolean
+}
+
 export type ImportPackageFunction = (
   to: string,
-  opts: {
-    targetEngine?: string
-    filesResponse: PackageFilesResponse
-    force: boolean
-  }
+  opts: ImportPackageOpts
 ) => Promise<{ isBuilt: boolean, importMethod: undefined | string }>
 
 export interface Cafs {

diff --git a/packages/headless/package.json b/packages/headless/package.json
@@ -22,7 +22,7 @@
     "@pnpm/package-store": "workspace:12.1.16",
     "@pnpm/prepare": "workspace:*",
     "@pnpm/read-projects-context": "workspace:5.0.19",
-    "@pnpm/registry-mock": "2.19.0",
+    "@pnpm/registry-mock": "2.20.0",
     "@pnpm/store-path": "^5.0.0",
     "@pnpm/test-fixtures": "workspace:*",
     "@types/fs-extra": "^9.0.13",

diff --git a/packages/headless/src/index.ts b/packages/headless/src/index.ts
@@ -683,6 +683,7 @@ async function linkAllPkgs (
       const { importMethod, isBuilt } = await storeController.importPackage(depNode.dir, {
         filesResponse,
         force: opts.force,
+        requiresBuild: depNode.requiresBuild,
         targetEngine,
       })
       if (importMethod) {

diff --git a/packages/headless/src/linkHoistedModules.ts b/packages/headless/src/linkHoistedModules.ts
@@ -107,6 +107,7 @@ async function linkAllPkgsInOrder (
       const { importMethod, isBuilt } = await storeController.importPackage(depNode.dir, {
         filesResponse,
         force: opts.force || depNode.depPath !== prevGraph[dir]?.depPath,
+        requiresBuild: depNode.requiresBuild,
         targetEngine,
       })
       if (importMethod) {

diff --git a/packages/package-requester/package.json b/packages/package-requester/package.json
@@ -57,6 +57,7 @@
     "promise-share": "^1.0.0",
     "ramda": "^0.27.2",
     "rename-overwrite": "^4.0.2",
+    "safe-promise-defer": "^1.0.1",
     "semver": "^7.3.7",
     "ssri": "^8.0.1"
   },
@@ -65,7 +66,7 @@
     "@pnpm/logger": "^4.0.0",
     "@pnpm/package-requester": "workspace:17.0.4",
     "@pnpm/package-store": "workspace:12.1.16",
-    "@pnpm/registry-mock": "2.19.0",
+    "@pnpm/registry-mock": "2.20.0",
     "@pnpm/test-fixtures": "workspace:*",
     "@types/normalize-path": "^3.0.0",
     "@types/ramda": "0.27.39",

diff --git a/packages/package-requester/src/packageRequester.ts b/packages/package-requester/src/packageRequester.ts
@@ -51,7 +51,7 @@ import renameOverwrite from 'rename-overwrite'
 import semver from 'semver'
 import ssri from 'ssri'
 import equalOrSemverEqual from './equalOrSemverEqual'
-import safeDeferredPromise from './safeDeferredPromise'
+import safePromiseDefer from 'safe-promise-defer'
 
 const TARBALL_INTEGRITY_FILENAME = 'tarball-integrity'
 const packageRequestLogger = logger('package-requester')
@@ -441,7 +441,7 @@ function fetchToStore (
 
         if ((pkgFilesIndex?.files) != null) {
           const manifest = opts.fetchRawManifest
-            ? safeDeferredPromise<DependencyManifest | undefined>()
+            ? safePromiseDefer<DependencyManifest | undefined>()
             : undefined
           if (
             (
@@ -498,7 +498,7 @@ Actual package in the store by the given integrity: ${pkgFilesIndex.name}@${pkgF
       const priority = (++ctx.requestsQueue['counter'] % ctx.requestsQueue['concurrency'] === 0 ? -1 : 1) * 1000 // eslint-disable-line
 
       const fetchManifest = opts.fetchRawManifest
-        ? safeDeferredPromise<DependencyManifest | undefined>()
+        ? safePromiseDefer<DependencyManifest | undefined>()
         : undefined
       if (fetchManifest != null) {
         fetchManifest()

diff --git a/packages/package-requester/src/safeDeferredPromise.ts b/packages/package-requester/src/safeDeferredPromise.ts
diff --git a/packages/package-store/src/storeController/createImportPackage.ts b/packages/package-store/src/storeController/createImportPackage.ts
@@ -19,13 +19,13 @@ interface ImportOptions {
 type ImportFunction = (to: string, opts: ImportOptions) => Promise<string | undefined>
 
 export default (
-  packageImportMethod?: 'auto' | 'hardlink' | 'copy' | 'clone'
+  packageImportMethod?: 'auto' | 'hardlink' | 'copy' | 'clone' | 'clone-or-copy'
 ): ImportFunction => {
   const importPackage = createImportPackage(packageImportMethod)
   return async (to, opts) => limitLinking(async () => importPackage(to, opts))
 }
 
-function createImportPackage (packageImportMethod?: 'auto' | 'hardlink' | 'copy' | 'clone') {
+function createImportPackage (packageImportMethod?: 'auto' | 'hardlink' | 'copy' | 'clone' | 'clone-or-copy') {
   // this works in the following way:
   // - hardlink: hardlink the packages, no fallback
   // - clone: clone the packages, no fallback
@@ -41,6 +41,8 @@ function createImportPackage (packageImportMethod?: 'auto' | 'hardlink' | 'copy'
   case 'auto': {
     return createAutoImporter()
   }
+  case 'clone-or-copy':
+    return createCloneOrCopyImporter()
   case 'copy':
     packageImportMethodLogger.debug({ method: 'copy' })
     return copyPkg
@@ -87,6 +89,29 @@ function createAutoImporter (): ImportFunction {
   }
 }
 
+function createCloneOrCopyImporter (): ImportFunction {
+  let auto = initialAuto
+
+  return async (to, opts) => auto(to, opts)
+
+  async function initialAuto (
+    to: string,
+    opts: ImportOptions
+  ): Promise<string | undefined> {
+    try {
+      if (!await clonePkg(to, opts)) return undefined
+      packageImportMethodLogger.debug({ method: 'clone' })
+      auto = clonePkg
+      return 'clone'
+    } catch (err: any) { // eslint-disable-line
+      // ignore
+    }
+    packageImportMethodLogger.debug({ method: 'copy' })
+    auto = copyPkg
+    return auto(to, opts)
+  }
+}
+
 async function clonePkg (
   to: string,
   opts: ImportOptions

diff --git a/packages/package-store/src/storeController/index.ts b/packages/package-store/src/storeController/index.ts
@@ -30,7 +30,10 @@ function createPackageImporter (
   const gfm = getFlatMap.bind(null, opts.cafsDir)
   return async (to, opts) => {
     const { filesMap, isBuilt } = gfm(opts.filesResponse, opts.targetEngine)
-    const impPkg = cachedImporterCreator(opts.filesResponse.packageImportMethod ?? packageImportMethod)
+    const pkgImportMethod = (opts.requiresBuild && !isBuilt)
+      ? 'clone-or-copy'
+      : (opts.filesResponse.packageImportMethod ?? packageImportMethod)
+    const impPkg = cachedImporterCreator(pkgImportMethod)
     const importMethod = await impPkg(to, { filesMap, fromStore: opts.filesResponse.fromStore, force: opts.force })
     return { importMethod, isBuilt }
   }

diff --git a/packages/plugin-commands-installation/package.json b/packages/plugin-commands-installation/package.json
@@ -39,7 +39,7 @@
     "@pnpm/modules-yaml": "workspace:9.1.1",
     "@pnpm/plugin-commands-installation": "workspace:8.4.23",
     "@pnpm/prepare": "workspace:*",
-    "@pnpm/registry-mock": "2.19.0",
+    "@pnpm/registry-mock": "2.20.0",
     "@pnpm/test-fixtures": "workspace:*",
     "@types/is-ci": "^3.0.0",
     "@types/proxyquire": "^1.3.28",

diff --git a/packages/plugin-commands-listing/package.json b/packages/plugin-commands-listing/package.json
@@ -38,7 +38,7 @@
     "@pnpm/plugin-commands-installation": "workspace:8.4.23",
     "@pnpm/plugin-commands-listing": "workspace:4.1.22",
     "@pnpm/prepare": "workspace:*",
-    "@pnpm/registry-mock": "2.19.0",
+    "@pnpm/registry-mock": "2.20.0",
     "@types/ramda": "0.27.39",
     "execa": "npm:safe-execa@^0.1.1",
     "strip-ansi": "^6.0.1",

diff --git a/packages/plugin-commands-outdated/package.json b/packages/plugin-commands-outdated/package.json
@@ -38,7 +38,7 @@
     "@pnpm/plugin-commands-installation": "workspace:8.4.23",
     "@pnpm/plugin-commands-outdated": "workspace:5.1.21",
     "@pnpm/prepare": "workspace:*",
-    "@pnpm/registry-mock": "2.19.0",
+    "@pnpm/registry-mock": "2.20.0",
     "@types/lru-cache": "^5.1.1",
     "@types/ramda": "0.27.39",
     "@types/wrap-ansi": "^3.0.0",

diff --git a/packages/plugin-commands-publishing/package.json b/packages/plugin-commands-publishing/package.json
@@ -39,7 +39,7 @@
     "@pnpm/logger": "^4.0.0",
     "@pnpm/plugin-commands-publishing": "workspace:4.5.15",
     "@pnpm/prepare": "workspace:*",
-    "@pnpm/registry-mock": "2.19.0",
+    "@pnpm/registry-mock": "2.20.0",
     "@types/cross-spawn": "^6.0.2",
     "@types/is-ci": "^3.0.0",
     "@types/is-windows": "^1.0.0",

diff --git a/packages/plugin-commands-rebuild/package.json b/packages/plugin-commands-rebuild/package.json
@@ -37,7 +37,7 @@
     "@pnpm/logger": "^4.0.0",
     "@pnpm/plugin-commands-rebuild": "workspace:5.4.28",
     "@pnpm/prepare": "workspace:*",
-    "@pnpm/registry-mock": "2.19.0",
+    "@pnpm/registry-mock": "2.20.0",
     "@pnpm/test-fixtures": "workspace:*",
     "@types/ramda": "0.27.39",
     "@types/semver": "^7.3.9",

diff --git a/packages/plugin-commands-script-runners/package.json b/packages/plugin-commands-script-runners/package.json
@@ -38,7 +38,7 @@
     "@pnpm/logger": "^4.0.0",
     "@pnpm/plugin-commands-script-runners": "workspace:4.6.17",
     "@pnpm/prepare": "workspace:*",
-    "@pnpm/registry-mock": "2.19.0",
+    "@pnpm/registry-mock": "2.20.0",
     "@types/is-windows": "^1.0.0",
     "@types/ramda": "0.27.39",
     "is-windows": "^1.0.2",

diff --git a/packages/plugin-commands-store/package.json b/packages/plugin-commands-store/package.json
@@ -38,7 +38,7 @@
     "@pnpm/logger": "^4.0.0",
     "@pnpm/plugin-commands-store": "workspace:4.1.26",
     "@pnpm/prepare": "workspace:*",
-    "@pnpm/registry-mock": "2.19.0",
+    "@pnpm/registry-mock": "2.20.0",
     "@types/archy": "0.0.31",
     "@types/ramda": "0.27.39",
     "@types/ssri": "^7.1.1",

diff --git a/packages/pnpm/package.json b/packages/pnpm/package.json
@@ -54,7 +54,7 @@
     "@pnpm/prepare": "workspace:*",
     "@pnpm/read-package-json": "workspace:5.0.12",
     "@pnpm/read-project-manifest": "workspace:2.0.13",
-    "@pnpm/registry-mock": "2.19.0",
+    "@pnpm/registry-mock": "2.20.0",
     "@pnpm/run-npm": "workspace:3.1.1",
     "@pnpm/store-path": "^5.0.0",
     "@pnpm/tabtab": "^0.1.2",

diff --git a/packages/resolve-dependencies/package.json b/packages/resolve-dependencies/package.json
@@ -51,9 +51,11 @@
     "is-inner-link": "^4.0.0",
     "is-subdir": "^1.2.0",
     "path-exists": "^4.0.0",
+    "promise-share": "^1.0.0",
     "ramda": "^0.27.2",
     "rename-overwrite": "^4.0.2",
     "replace-string": "^3.1.0",
+    "safe-promise-defer": "^1.0.1",
     "semver": "^7.3.7",
     "semver-range-intersect": "^0.3.1",
     "version-selector-type": "^3.0.0"