Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: do not build the same dependency multiple times when node-linker is hoisted #5814

Merged
merged 4 commits into from
Dec 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/sour-cougars-deliver.md
Original file line number Diff line number Diff line change
@@ -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.
2 changes: 2 additions & 0 deletions exec/build-modules/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,14 @@
"@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:*",
"@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"
Expand Down
27 changes: 27 additions & 0 deletions exec/build-modules/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ 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'
import pDefer, { DeferredPromise } from 'p-defer'
import { applyPatch } from 'patch-package/dist/applyPatches'
import pickBy from 'ramda/src/pickBy'
import runGroups from 'run-groups'
Expand Down Expand Up @@ -38,12 +40,16 @@ export async function buildModules (
sideEffectsCacheWrite: boolean
storeController: StoreController
rootModulesDir: string
hoistedLocations?: Record<string, string[]>
}
) {
const warn = (message: string) => logger.warn({ message, prefix: opts.lockfileDir })
// postinstall hooks

const buildDepOpts = { ...opts, warn }
if (opts.hoistedLocations) {
buildDepOpts['builtHoistedDeps'] = {}
}
const chunks = buildSequence(depGraph, rootDepPaths)
const groups = chunks.map((chunk) => {
chunk = chunk.filter((depPath) => {
Expand Down Expand Up @@ -81,10 +87,19 @@ async function buildDependency (
sideEffectsCacheWrite: boolean
storeController: StoreController
unsafePerm: boolean
hoistedLocations?: Record<string, string[]>
builtHoistedDeps?: Record<string, DeferredPromise<void>>
warn: (message: string) => void
}
) {
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
Expand Down Expand Up @@ -147,6 +162,18 @@ async function buildDependency (
return
}
throw err
} finally {
const hoistedLocationsOfDep = opts.hoistedLocations?.[depNode.depPath]
if (hoistedLocationsOfDep) {
// 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) {
opts.builtHoistedDeps[depNode.depPath].resolve()
}
}
}

Expand Down
3 changes: 3 additions & 0 deletions exec/build-modules/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
"../../__typings__/**/*.d.ts"
],
"references": [
{
"path": "../../fs/hard-link-dir"
},
{
"path": "../../packages/calc-dep-state"
},
Expand Down
110 changes: 109 additions & 1 deletion pkg-manager/core/test/install/lifecycleScripts.ts
Original file line number Diff line number Diff line change
@@ -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'
Expand Down Expand Up @@ -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')
})
1 change: 1 addition & 0 deletions pkg-manager/headless/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down