From 144b0d476116b93f0a5e61f756c4996261d1fc8e Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Wed, 21 Dec 2022 03:33:32 +0200 Subject: [PATCH 1/4] perf: do not build the same dependency multiple times when node-linker is hoisted --- exec/build-modules/package.json | 1 + exec/build-modules/src/index.ts | 18 ++++++++++++++++++ exec/build-modules/tsconfig.json | 3 +++ pkg-manager/headless/src/index.ts | 1 + pnpm-lock.yaml | 3 +++ 5 files changed, 26 insertions(+) diff --git a/exec/build-modules/package.json b/exec/build-modules/package.json index cd6d1d70c00..8f8935ae9f1 100644 --- a/exec/build-modules/package.json +++ b/exec/build-modules/package.json @@ -37,6 +37,7 @@ "@pnpm/calc-dep-state": "workspace:*", "@pnpm/core-loggers": "workspace:*", "@pnpm/error": "workspace:*", + "@pnpm/fs.hard-link-dir": "workspace:*", "@pnpm/graph-sequencer": "1.0.0", "@pnpm/lifecycle": "workspace:*", "@pnpm/link-bins": "workspace:*", diff --git a/exec/build-modules/src/index.ts b/exec/build-modules/src/index.ts index 82a8d35addd..9f2527619cc 100644 --- a/exec/build-modules/src/index.ts +++ b/exec/build-modules/src/index.ts @@ -5,6 +5,7 @@ import { PnpmError } from '@pnpm/error' import { runPostinstallHooks } from '@pnpm/lifecycle' import { linkBins, linkBinsOfPackages } from '@pnpm/link-bins' import { logger } from '@pnpm/logger' +import { hardLinkDir } from '@pnpm/fs.hard-link-dir' import { readPackageJsonFromDir, safeReadPackageJsonFromDir } from '@pnpm/read-package-json' import { StoreController } from '@pnpm/store-controller-types' import { DependencyManifest } from '@pnpm/types' @@ -38,12 +39,16 @@ export async function buildModules ( sideEffectsCacheWrite: boolean storeController: StoreController rootModulesDir: string + hoistedLocations?: Record } ) { const warn = (message: string) => logger.warn({ message, prefix: opts.lockfileDir }) // postinstall hooks const buildDepOpts = { ...opts, warn } + if (opts.hoistedLocations) { + buildDepOpts['builtHoistedDeps'] = new Set() + } const chunks = buildSequence(depGraph, rootDepPaths) const groups = chunks.map((chunk) => { chunk = chunk.filter((depPath) => { @@ -81,9 +86,12 @@ async function buildDependency ( sideEffectsCacheWrite: boolean storeController: StoreController unsafePerm: boolean + hoistedLocations?: Record + builtHoistedDeps?: Set warn: (message: string) => void } ) { + if (opts.builtHoistedDeps?.has(depPath)) return const depNode = depGraph[depPath] try { await linkBinsOfDependencies(depNode, depGraph, opts) @@ -147,6 +155,16 @@ async function buildDependency ( return } throw err + } finally { + const hoistedLocationsOfDep = opts.hoistedLocations?.[depPath] + if (hoistedLocationsOfDep) { + const rel = path.relative(opts.lockfileDir, depNode.dir) + const nonBuiltHoistedDeps = hoistedLocationsOfDep?.filter((hoistedLocation) => hoistedLocation !== rel) + await hardLinkDir(depNode.dir, nonBuiltHoistedDeps) + } + if (opts.builtHoistedDeps) { + opts.builtHoistedDeps.add(depPath) + } } } diff --git a/exec/build-modules/tsconfig.json b/exec/build-modules/tsconfig.json index 34f9e45d8cc..a6cbebe403e 100644 --- a/exec/build-modules/tsconfig.json +++ b/exec/build-modules/tsconfig.json @@ -9,6 +9,9 @@ "../../__typings__/**/*.d.ts" ], "references": [ + { + "path": "../../fs/hard-link-dir" + }, { "path": "../../packages/calc-dep-state" }, diff --git a/pkg-manager/headless/src/index.ts b/pkg-manager/headless/src/index.ts index 93212b013a8..0d670cca9ad 100644 --- a/pkg-manager/headless/src/index.ts +++ b/pkg-manager/headless/src/index.ts @@ -467,6 +467,7 @@ export async function headlessInstall (opts: HeadlessOptions) { extraEnv, depsStateCache, ignoreScripts: opts.ignoreScripts || opts.ignoreDepScripts, + hoistedLocations, lockfileDir, optional: opts.include.optionalDependencies, preferSymlinkedExecutables: opts.preferSymlinkedExecutables, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index b597a5c2aae..dc5c22f5e4b 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -857,6 +857,9 @@ importers: '@pnpm/error': specifier: workspace:* version: link:../../packages/error + '@pnpm/fs.hard-link-dir': + specifier: workspace:* + version: link:../../fs/hard-link-dir '@pnpm/graph-sequencer': specifier: 1.0.0 version: 1.0.0 From 888d126b4e2cde2051c4dd5033c3cd286df96ef5 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Wed, 21 Dec 2022 04:18:28 +0200 Subject: [PATCH 2/4] perf: do not build the same dependency multiple times when node-linker is hoisted --- exec/build-modules/package.json | 1 + exec/build-modules/src/index.ts | 15 ++- .../core/test/install/lifecycleScripts.ts | 110 +++++++++++++++++- pnpm-lock.yaml | 42 +++---- 4 files changed, 143 insertions(+), 25 deletions(-) diff --git a/exec/build-modules/package.json b/exec/build-modules/package.json index 8f8935ae9f1..f3b743cbee5 100644 --- a/exec/build-modules/package.json +++ b/exec/build-modules/package.json @@ -44,6 +44,7 @@ "@pnpm/read-package-json": "workspace:*", "@pnpm/store-controller-types": "workspace:*", "@pnpm/types": "workspace:*", + "p-defer": "^3.0.0", "patch-package": "^6.5.0", "ramda": "npm:@pnpm/ramda@0.28.1", "run-groups": "^3.0.1" diff --git a/exec/build-modules/src/index.ts b/exec/build-modules/src/index.ts index 9f2527619cc..5df84d8972e 100644 --- a/exec/build-modules/src/index.ts +++ b/exec/build-modules/src/index.ts @@ -9,6 +9,7 @@ import { hardLinkDir } from '@pnpm/fs.hard-link-dir' import { readPackageJsonFromDir, safeReadPackageJsonFromDir } from '@pnpm/read-package-json' import { StoreController } from '@pnpm/store-controller-types' import { DependencyManifest } from '@pnpm/types' +import pDefer, { DeferredPromise } from 'p-defer' import { applyPatch } from 'patch-package/dist/applyPatches' import pickBy from 'ramda/src/pickBy' import runGroups from 'run-groups' @@ -87,12 +88,18 @@ async function buildDependency ( storeController: StoreController unsafePerm: boolean hoistedLocations?: Record - builtHoistedDeps?: Set + builtHoistedDeps?: Record> warn: (message: string) => void } ) { - if (opts.builtHoistedDeps?.has(depPath)) return const depNode = depGraph[depPath] + if (opts.builtHoistedDeps) { + if (opts.builtHoistedDeps[depNode.depPath]) { + await opts.builtHoistedDeps[depNode.depPath].promise + return + } + opts.builtHoistedDeps[depNode.depPath] = pDefer() + } try { await linkBinsOfDependencies(depNode, depGraph, opts) const isPatched = depNode.patchFile?.path != null @@ -156,14 +163,14 @@ async function buildDependency ( } throw err } finally { - const hoistedLocationsOfDep = opts.hoistedLocations?.[depPath] + const hoistedLocationsOfDep = opts.hoistedLocations?.[depNode.depPath] if (hoistedLocationsOfDep) { const rel = path.relative(opts.lockfileDir, depNode.dir) const nonBuiltHoistedDeps = hoistedLocationsOfDep?.filter((hoistedLocation) => hoistedLocation !== rel) await hardLinkDir(depNode.dir, nonBuiltHoistedDeps) } if (opts.builtHoistedDeps) { - opts.builtHoistedDeps.add(depPath) + opts.builtHoistedDeps[depNode.depPath].resolve() } } } diff --git a/pkg-manager/core/test/install/lifecycleScripts.ts b/pkg-manager/core/test/install/lifecycleScripts.ts index 9fb31d803f9..2e056d4115c 100644 --- a/pkg-manager/core/test/install/lifecycleScripts.ts +++ b/pkg-manager/core/test/install/lifecycleScripts.ts @@ -1,11 +1,14 @@ import * as path from 'path' import { promises as fs } from 'fs' +import { assertProject } from '@pnpm/assert-project' import { LifecycleLog } from '@pnpm/core-loggers' -import { prepareEmpty } from '@pnpm/prepare' +import { prepareEmpty, preparePackages } from '@pnpm/prepare' import { addDependenciesToPackage, install, mutateModulesInSingleProject, + MutatedProject, + mutateModules, } from '@pnpm/core' import rimraf from '@zkochan/rimraf' import isWindows from 'is-windows' @@ -608,3 +611,108 @@ test('ignore-dep-scripts', async () => { expect(await exists('node_modules/@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-preinstall.js')).toBeFalsy() } }) + +test('run pre/postinstall scripts in a workspace that uses node-linker=hoisted', async () => { + const projects = preparePackages([ + { + location: 'project-1', + package: { name: 'project-1' }, + }, + { + location: 'project-2', + package: { name: 'project-2' }, + }, + { + location: 'project-3', + package: { name: 'project-3' }, + }, + { + location: 'project-4', + package: { name: 'project-4' }, + }, + ]) + const importers: MutatedProject[] = [ + { + mutation: 'install', + rootDir: path.resolve('project-1'), + }, + { + mutation: 'install', + rootDir: path.resolve('project-2'), + }, + { + mutation: 'install', + rootDir: path.resolve('project-3'), + }, + { + mutation: 'install', + rootDir: path.resolve('project-4'), + }, + ] + const allProjects = [ + { + buildIndex: 0, + manifest: { + name: 'project-1', + version: '1.0.0', + + dependencies: { + '@pnpm.e2e/pre-and-postinstall-scripts-example': '1', + }, + }, + rootDir: path.resolve('project-1'), + }, + { + buildIndex: 0, + manifest: { + name: 'project-2', + version: '1.0.0', + + dependencies: { + '@pnpm.e2e/pre-and-postinstall-scripts-example': '1', + }, + }, + rootDir: path.resolve('project-2'), + }, + { + buildIndex: 0, + manifest: { + name: 'project-3', + version: '1.0.0', + + dependencies: { + '@pnpm.e2e/pre-and-postinstall-scripts-example': '2', + }, + }, + rootDir: path.resolve('project-3'), + }, + { + buildIndex: 0, + manifest: { + name: 'project-4', + version: '1.0.0', + + dependencies: { + '@pnpm.e2e/pre-and-postinstall-scripts-example': '2', + }, + }, + rootDir: path.resolve('project-4'), + }, + ] + await mutateModules(importers, await testDefaults({ + allProjects, + fastUnpack: false, + nodeLinker: 'hoisted', + })) + const rootProject = assertProject(process.cwd()) + await rootProject.has('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-preinstall.js') + await rootProject.has('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-postinstall.js') + await projects['project-1'].hasNot('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-preinstall.js') + await projects['project-1'].hasNot('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-postinstall.js') + await projects['project-2'].hasNot('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-preinstall.js') + await projects['project-2'].hasNot('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-postinstall.js') + await projects['project-3'].has('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-preinstall.js') + await projects['project-3'].has('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-postinstall.js') + await projects['project-4'].has('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-preinstall.js') + await projects['project-4'].has('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-postinstall.js') +}) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index dc5c22f5e4b..924643d7976 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -881,6 +881,9 @@ importers: '@pnpm/types': specifier: workspace:* version: link:../../packages/types + p-defer: + specifier: ^3.0.0 + version: 3.0.0 patch-package: specifier: ^6.5.0 version: 6.5.0 @@ -6770,7 +6773,7 @@ packages: engines: {node: ^14.15.0 || ^16.10.0 || >=18.0.0} dependencies: '@jest/types': 29.3.1 - '@types/node': 18.11.9 + '@types/node': 18.11.17 chalk: 4.1.2 jest-message-util: 29.3.1 jest-util: 29.3.1 @@ -6791,14 +6794,14 @@ packages: '@jest/test-result': 29.3.1 '@jest/transform': 29.3.1_@babel+types@7.20.5 '@jest/types': 29.3.1 - '@types/node': 18.11.9 + '@types/node': 18.11.17 ansi-escapes: 4.3.2 chalk: 4.1.2 ci-info: 3.6.1 exit: 0.1.2 graceful-fs: 4.2.10 jest-changed-files: 29.2.0 - jest-config: 29.3.1_x4zmdo4imr2n2hhykfyzszwk6y + jest-config: 29.3.1_uvwivoxixlgocwjbikx7evbpf4 jest-haste-map: 29.3.1 jest-message-util: 29.3.1 jest-regex-util: 29.2.0 @@ -6886,7 +6889,7 @@ packages: '@jest/transform': 29.3.1_@babel+types@7.20.5 '@jest/types': 29.3.1 '@jridgewell/trace-mapping': 0.3.17 - '@types/node': 18.11.9 + '@types/node': 18.11.17 chalk: 4.1.2 collect-v8-coverage: 1.0.1 exit: 0.1.2 @@ -6976,7 +6979,7 @@ packages: '@jest/schemas': 29.0.0 '@types/istanbul-lib-coverage': 2.0.4 '@types/istanbul-reports': 3.0.1 - '@types/node': 18.11.9 + '@types/node': 18.11.17 '@types/yargs': 17.0.13 chalk: 4.1.2 dev: true @@ -6988,7 +6991,7 @@ packages: '@jest/schemas': 29.0.0 '@types/istanbul-lib-coverage': 2.0.4 '@types/istanbul-reports': 3.0.1 - '@types/node': 18.11.9 + '@types/node': 18.11.17 '@types/yargs': 17.0.13 chalk: 4.1.2 dev: true @@ -8472,7 +8475,7 @@ packages: dependencies: '@types/http-cache-semantics': 4.0.1 '@types/keyv': 3.1.4 - '@types/node': 18.11.9 + '@types/node': 18.11.17 '@types/responselike': 1.0.0 /@types/concat-stream/2.0.0: @@ -8500,7 +8503,7 @@ packages: resolution: {integrity: sha512-l6NQsDDyQUVeoTynNpC9uRvCUint/gSUXQA2euwmTuWGvPY5LSDUu6tkCtJB2SvGQlJQzLaKqcGZP4//7EDveA==} dependencies: '@types/minimatch': 5.1.2 - '@types/node': 18.11.9 + '@types/node': 18.11.17 dev: true /@types/graceful-fs/4.1.5: @@ -8570,7 +8573,7 @@ packages: /@types/keyv/3.1.4: resolution: {integrity: sha512-BQ5aZNSCpj7D6K2ksrRCTmKRLEpnPvWDiLPfoGyhZ++8YtiK9d/3DBKPJgry359X/P1PfruyYwvnvwFjuEiEIg==} dependencies: - '@types/node': 18.11.9 + '@types/node': 18.11.17 /@types/lodash/4.14.181: resolution: {integrity: sha512-n3tyKthHJbkiWhDZs3DkhkCzt2MexYHXlX0td5iMplyfwketaOeKboEVBqzceH7juqvEg3q5oUoBFxSLu7zFag==} @@ -8614,7 +8617,6 @@ packages: /@types/node/18.11.17: resolution: {integrity: sha512-HJSUJmni4BeDHhfzn6nF0sVmd1SMezP7/4F0Lq+aXzmp2xm9O7WXrUtHW/CHlYVtZUbByEvWidHqRtcJXGF2Ng==} - dev: true /@types/node/18.11.9: resolution: {integrity: sha512-CRpX21/kGdzjOpFsZSkcrXMGIBWMGNIHXXBVFSH+ggkftxg+XYP20TESbh+zFvFj3EQOl5byk0HTRn1IL6hbqg==} @@ -8651,7 +8653,7 @@ packages: /@types/responselike/1.0.0: resolution: {integrity: sha512-85Y2BjiufFzaMIlvJDvTTB8Fxl2xfLo4HgmHzVBz08w4wDePCTjYw66PdrolO0kzli3yam/YCgRufyo1DdQVTA==} dependencies: - '@types/node': 18.11.9 + '@types/node': 18.11.17 /@types/retry/0.12.2: resolution: {integrity: sha512-XISRgDJ2Tc5q4TRqvgJtzsRkFYNJzZrhTdtMoGVBttwzzQJkPnS3WWTFc7kuDRoPtPakl+T+OfdEUjYJj7Jbow==} @@ -9071,7 +9073,7 @@ packages: resolution: {integrity: sha512-xGj0G1xvOIRrQkB2aT/oBWscDqq20NAG4qSbM9gT7px58nyVAZvhBOSMyEEkVyTeHtJtPIMdADKbXawdZqnrVQ==} engines: {node: '>=14.15.0'} dependencies: - '@types/node': 18.11.9 + '@types/node': 18.11.17 '@yarnpkg/fslib': 3.0.0-rc.25 /@yarnpkg/shell/3.2.0-rc.8_typanion@3.12.1: @@ -12844,7 +12846,7 @@ packages: - supports-color dev: true - /jest-config/29.3.1_x4zmdo4imr2n2hhykfyzszwk6y: + /jest-config/29.3.1_uvwivoxixlgocwjbikx7evbpf4: resolution: {integrity: sha512-y0tFHdj2WnTEhxmGUK1T7fgLen7YK4RtfvpLFBXfQkh2eMJAQq24Vx9472lvn5wg0MAO6B+iPfJfzdR9hJYalg==} engines: {node: ^14.15.0 || ^16.10.0 || >=18.0.0} peerDependencies: @@ -12859,7 +12861,7 @@ packages: '@babel/core': 7.20.5 '@jest/test-sequencer': 29.3.1 '@jest/types': 29.3.1 - '@types/node': 18.11.9 + '@types/node': 18.11.17 babel-jest: 29.3.1_5zl3o24ezxc3cc3y3khgvpammi chalk: 4.1.2 ci-info: 3.6.1 @@ -12936,7 +12938,7 @@ packages: dependencies: '@jest/types': 29.3.1 '@types/graceful-fs': 4.1.5 - '@types/node': 18.11.9 + '@types/node': 18.11.17 anymatch: 3.1.2 fb-watchman: 2.0.2 graceful-fs: 4.2.10 @@ -13042,7 +13044,7 @@ packages: '@jest/test-result': 29.3.1 '@jest/transform': 29.3.1_@babel+types@7.20.5 '@jest/types': 29.3.1 - '@types/node': 18.11.9 + '@types/node': 18.11.17 chalk: 4.1.2 emittery: 0.13.1 graceful-fs: 4.2.10 @@ -13074,7 +13076,7 @@ packages: '@jest/test-result': 29.3.1 '@jest/transform': 29.3.1_@babel+types@7.20.5 '@jest/types': 29.3.1 - '@types/node': 18.11.9 + '@types/node': 18.11.17 chalk: 4.1.2 cjs-module-lexer: 1.2.2 collect-v8-coverage: 1.0.1 @@ -13131,7 +13133,7 @@ packages: engines: {node: ^14.15.0 || ^16.10.0 || >=18.0.0} dependencies: '@jest/types': 29.1.2 - '@types/node': 18.11.9 + '@types/node': 18.11.17 chalk: 4.1.2 ci-info: 3.6.1 graceful-fs: 4.2.10 @@ -13143,7 +13145,7 @@ packages: engines: {node: ^14.15.0 || ^16.10.0 || >=18.0.0} dependencies: '@jest/types': 29.3.1 - '@types/node': 18.11.9 + '@types/node': 18.11.17 chalk: 4.1.2 ci-info: 3.6.1 graceful-fs: 4.2.10 @@ -13168,7 +13170,7 @@ packages: dependencies: '@jest/test-result': 29.3.1 '@jest/types': 29.3.1 - '@types/node': 18.11.9 + '@types/node': 18.11.17 ansi-escapes: 4.3.2 chalk: 4.1.2 emittery: 0.13.1 From 83b60c5e0b576414cb499a54f81832b2f06b843b Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Wed, 21 Dec 2022 04:24:31 +0200 Subject: [PATCH 3/4] perf: do not build the same dependency multiple times when node-linker is hoisted --- exec/build-modules/src/index.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/exec/build-modules/src/index.ts b/exec/build-modules/src/index.ts index 5df84d8972e..980b50ce615 100644 --- a/exec/build-modules/src/index.ts +++ b/exec/build-modules/src/index.ts @@ -48,7 +48,7 @@ export async function buildModules ( const buildDepOpts = { ...opts, warn } if (opts.hoistedLocations) { - buildDepOpts['builtHoistedDeps'] = new Set() + buildDepOpts['builtHoistedDeps'] = {} } const chunks = buildSequence(depGraph, rootDepPaths) const groups = chunks.map((chunk) => { @@ -165,8 +165,10 @@ async function buildDependency ( } finally { const hoistedLocationsOfDep = opts.hoistedLocations?.[depNode.depPath] if (hoistedLocationsOfDep) { - const rel = path.relative(opts.lockfileDir, depNode.dir) - const nonBuiltHoistedDeps = hoistedLocationsOfDep?.filter((hoistedLocation) => hoistedLocation !== rel) + // There is no need to build the same package in every location. + // We just copy the built package to every location where it is present. + const currentHoistedLocation = path.relative(opts.lockfileDir, depNode.dir) + const nonBuiltHoistedDeps = hoistedLocationsOfDep?.filter((hoistedLocation) => hoistedLocation !== currentHoistedLocation) await hardLinkDir(depNode.dir, nonBuiltHoistedDeps) } if (opts.builtHoistedDeps) { From 97511cd7e73a685711e8d7077037827e037e2ead Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Wed, 21 Dec 2022 04:27:17 +0200 Subject: [PATCH 4/4] docs: add changesets --- .changeset/sour-cougars-deliver.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/sour-cougars-deliver.md diff --git a/.changeset/sour-cougars-deliver.md b/.changeset/sour-cougars-deliver.md new file mode 100644 index 00000000000..f10901fb0b7 --- /dev/null +++ b/.changeset/sour-cougars-deliver.md @@ -0,0 +1,6 @@ +--- +"@pnpm/build-modules": minor +"pnpm": minor +--- + +When the hoister node-linker is used, pnpm should not build the same package multiple times during installation. If a package is present at multipe locations because hoisting could not hoist them to a single directory, then the package should only built in one of the locations and copied to the rest.