diff --git a/.changeset/hoisting-limits-setting.md b/.changeset/hoisting-limits-setting.md new file mode 100644 index 0000000000..5b00e411d6 --- /dev/null +++ b/.changeset/hoisting-limits-setting.md @@ -0,0 +1,8 @@ +--- +"@pnpm/installing.linking.real-hoist": minor +"@pnpm/config.reader": minor +"@pnpm/installing.commands": minor +"pnpm": minor +--- + +Added a new `hoistingLimits` setting for `nodeLinker: hoisted` installs, mirroring yarn's `nmHoistingLimits`. It accepts `none` (the default — hoist as far as possible), `workspaces` (hoist only as far as each workspace package), or `dependencies` (hoist only up to each workspace package's direct dependencies). Originally proposed in [#6468](https://github.com/pnpm/pnpm/pull/6468), closing [#6457](https://github.com/pnpm/pnpm/issues/6457). diff --git a/config/reader/src/Config.ts b/config/reader/src/Config.ts index 7e8f9591ed..a68b0e287e 100644 --- a/config/reader/src/Config.ts +++ b/config/reader/src/Config.ts @@ -176,6 +176,7 @@ export interface Config extends OptionsFromRootManifest { hoistPattern?: string[] publicHoistPattern?: string[] | string hoistWorkspacePackages?: boolean + hoistingLimits?: 'none' | 'workspaces' | 'dependencies' useStoreServer?: boolean useRunningStoreServer?: boolean workspaceConcurrency: number diff --git a/config/reader/src/configFileKey.ts b/config/reader/src/configFileKey.ts index a94cb64c63..95a6681375 100644 --- a/config/reader/src/configFileKey.ts +++ b/config/reader/src/configFileKey.ts @@ -102,6 +102,7 @@ export const excludedPnpmKeys = [ 'hoist', 'hoist-pattern', 'hoist-workspace-packages', + 'hoisting-limits', 'ignore-compatibility-db', 'ignore-pnpmfile', 'ignore-workspace', diff --git a/config/reader/src/types.ts b/config/reader/src/types.ts index 7cc351f71f..1869b6d395 100644 --- a/config/reader/src/types.ts +++ b/config/reader/src/types.ts @@ -48,6 +48,7 @@ export const pnpmTypes = { 'http-proxy': [null, String], 'hoist-pattern': Array, 'hoist-workspace-packages': Boolean, + 'hoisting-limits': ['none', 'workspaces', 'dependencies'], 'ignore-compatibility-db': Boolean, 'ignore-pnpmfile': Boolean, 'ignore-workspace': Boolean, diff --git a/installing/commands/src/add.ts b/installing/commands/src/add.ts index afbaffa033..40e261caff 100644 --- a/installing/commands/src/add.ts +++ b/installing/commands/src/add.ts @@ -42,6 +42,7 @@ export function rcOptionsTypes (): Record { 'global', 'hoist', 'hoist-pattern', + 'hoisting-limits', 'https-proxy', 'ignore-pnpmfile', 'ignore-scripts', diff --git a/installing/commands/src/install.ts b/installing/commands/src/install.ts index 05a4650bb6..d56a54af16 100644 --- a/installing/commands/src/install.ts +++ b/installing/commands/src/install.ts @@ -30,6 +30,7 @@ export function rcOptionsTypes (): Record { 'global', 'hoist', 'hoist-pattern', + 'hoisting-limits', 'https-proxy', 'ignore-pnpmfile', 'ignore-scripts', @@ -305,6 +306,7 @@ export type InstallCommandOptions = Pick { +test('hoistingLimits=dependencies should prevent packages from being hoisted past direct dependencies', async () => { prepareEmpty() - const hoistingLimits = new Map() - hoistingLimits.set('.@', new Set(['send'])) await install({ dependencies: { send: '0.17.2', }, }, testDefaults({ nodeLinker: 'hoisted', - hoistingLimits, + hoistingLimits: 'dependencies', })) expect(fs.existsSync('node_modules/ms')).toBeFalsy() diff --git a/installing/linking/real-hoist/src/index.ts b/installing/linking/real-hoist/src/index.ts index 105b1c6a1c..7a065374b6 100644 --- a/installing/linking/real-hoist/src/index.ts +++ b/installing/linking/real-hoist/src/index.ts @@ -7,10 +7,64 @@ import { } from '@pnpm/lockfile.utils' import { hoist as _hoist, HoisterDependencyKind, type HoisterResult, type HoisterTree } from '@yarnpkg/nm/hoist' -export type HoistingLimits = Map> +/** + * Controls how far dependencies are hoisted, mirroring yarn's + * `nmHoistingLimits`. Given workspace package `A` → `B` → `C`: + * + * - `'none'` (default): hoist as far as possible. + * - `/packages/A`, `/node_modules/B`, `/node_modules/C` + * - `'workspaces'`: hoist only as far as each workspace package. + * - `/packages/A`, `/packages/A/node_modules/B`, `/packages/A/node_modules/C` + * - `'dependencies'`: hoist only up to each workspace package's direct + * dependencies. + * - `/packages/A`, `/packages/A/node_modules/B`, `/packages/A/node_modules/B/node_modules/C` + */ +export type HoistingLimits = 'none' | 'workspaces' | 'dependencies' export type { HoisterResult } +/** + * Translate the user-facing {@link HoistingLimits} mode into the + * `@yarnpkg/nm` hoister's per-locator border map. A name in a + * locator's set is a hoisting border: that node's dependencies are + * not hoisted above it. Returns `undefined` for `'none'` (and when + * unset) so the hoister hoists as far as possible. + */ +export function getHoistingLimits (lockfile: Pick, mode: HoistingLimits | undefined): Map> | undefined { + if (!mode || mode === 'none') return undefined + + const hoistingLimits = new Map>() + const rootHoistingLimit = new Set() + + for (const [importerId, importer] of Object.entries(lockfile.importers)) { + const isWorkspaceRoot = importerId === '.' + const encodedId = encodeURIComponent(importerId) + if (!isWorkspaceRoot) { + rootHoistingLimit.add(encodedId) + if (mode !== 'dependencies') { + // In `'workspaces'` mode it's enough to border each workspace + // package at the root; their own direct deps don't need a + // per-importer border. + continue + } + } + + const reference = isWorkspaceRoot ? '' : `workspace:${importerId}` + const hoistingLimit = isWorkspaceRoot ? rootHoistingLimit : new Set() + + hoistingLimits.set(`${encodedId}@${reference}`, hoistingLimit) + + for (const deps of [importer.dependencies, importer.devDependencies, importer.optionalDependencies]) { + if (!deps) continue + for (const dep of Object.keys(deps)) { + hoistingLimit.add(dep) + } + } + } + + return hoistingLimits +} + export function hoist ( lockfile: LockfileObject, opts?: { @@ -65,7 +119,8 @@ export function hoist ( node.dependencies.add(importerNode) } - const hoisterResult = _hoist(node, opts) + const hoistingLimits = getHoistingLimits(lockfile, opts?.hoistingLimits) + const hoisterResult = _hoist(node, { ...opts, hoistingLimits }) if (opts?.externalDependencies) { for (const hoistedDep of hoisterResult.dependencies.values()) { if (opts.externalDependencies.has(hoistedDep.name)) { diff --git a/installing/linking/real-hoist/test/index.ts b/installing/linking/real-hoist/test/index.ts index a686e99631..317feb39ad 100644 --- a/installing/linking/real-hoist/test/index.ts +++ b/installing/linking/real-hoist/test/index.ts @@ -1,6 +1,8 @@ +// cspell:ignore Ffoo -- `%2F` percent-encoding of `packages/foo` in the hoisting-limits locator keys import { expect, test } from '@jest/globals' -import { hoist } from '@pnpm/installing.linking.real-hoist' +import { getHoistingLimits, hoist } from '@pnpm/installing.linking.real-hoist' import { readWantedLockfile } from '@pnpm/lockfile.fs' +import type { LockfileObject } from '@pnpm/lockfile.utils' import { fixtures } from '@pnpm/test-fixtures' import type { ProjectId } from '@pnpm/types' @@ -27,3 +29,38 @@ test('hoist throws an error if the lockfile is broken', () => { packages: {}, })).toThrow(/Broken lockfile/) }) + +const importersLockfile: Pick = { + importers: { + ['.' as ProjectId]: { + dependencies: { a: '1.0.0' }, + specifiers: { a: '1.0.0' }, + }, + ['packages/foo' as ProjectId]: { + dependencies: { b: '1.0.0' }, + specifiers: { b: '1.0.0' }, + }, + }, +} + +test('getHoistingLimits returns undefined for the default "none" mode', () => { + expect(getHoistingLimits(importersLockfile, undefined)).toBeUndefined() + expect(getHoistingLimits(importersLockfile, 'none')).toBeUndefined() +}) + +test('getHoistingLimits in "workspaces" mode borders each workspace package at the root', () => { + const limits = getHoistingLimits(importersLockfile, 'workspaces') + // Only the root locator gets a border: its direct deps plus every + // (URI-encoded) workspace package id. No per-importer border. + expect([...limits!.keys()]).toStrictEqual(['.@']) + expect(limits!.get('.@')).toStrictEqual(new Set(['a', 'packages%2Ffoo'])) +}) + +test('getHoistingLimits in "dependencies" mode additionally borders each importer\'s direct deps', () => { + const limits = getHoistingLimits(importersLockfile, 'dependencies') + expect([...limits!.keys()].sort()).toStrictEqual(['.@', 'packages%2Ffoo@workspace:packages/foo']) + expect(limits!.get('.@')).toStrictEqual(new Set(['a', 'packages%2Ffoo'])) + // Each non-root importer borders its own direct deps so their + // transitives stay nested under them. + expect(limits!.get('packages%2Ffoo@workspace:packages/foo')).toStrictEqual(new Set(['b'])) +}) diff --git a/pacquet/crates/cli/tests/hoist.rs b/pacquet/crates/cli/tests/hoist.rs index 43947ba2ea..f9753d2df1 100644 --- a/pacquet/crates/cli/tests/hoist.rs +++ b/pacquet/crates/cli/tests/hoist.rs @@ -479,6 +479,77 @@ fn workspace_hoist_walks_every_importer() { drop((root, mock_instance)); } +/// `nodeLinker: hoisted` on the fresh-lockfile path (no checked-in +/// lockfile, no `--frozen-lockfile`) must lay every dependency out as +/// a **real directory** flat under the project's `node_modules/`, not +/// as a symlink into a `.pnpm` virtual store. Closes +/// [#11871](https://github.com/pnpm/pnpm/issues/11871): the fresh +/// path used to hard-refuse the combination. +/// +/// Uses `@pnpm.e2e/hello-world-js-bin-parent` (a direct dep) which +/// pulls in `@pnpm.e2e/hello-world-js-bin` as a transitive — under +/// the hoisted linker both land at the top level as real dirs. +#[test] +fn fresh_install_hoisted_node_linker_lands_real_directories() { + let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + write_manifest( + &workspace, + serde_json::json!({ "@pnpm.e2e/hello-world-js-bin-parent": "1.0.0" }), + ); + write_workspace_yaml(&workspace, "nodeLinker: hoisted\n"); + + // No `generate_lockfile` and no `--frozen-lockfile`: this drives + // the fresh-resolve path. + pacquet.with_args(["install"]).assert().success(); + + let is_real_dir = |relative: &str| -> bool { + let path = workspace.join(relative); + path.is_dir() && !is_symlink_or_junction(&path).unwrap() + }; + + // Direct dep is a real directory carrying its own manifest. + assert!( + is_real_dir("node_modules/@pnpm.e2e/hello-world-js-bin-parent"), + "direct dep should be a real directory under node_modules/, not a symlink", + ); + assert!( + workspace.join("node_modules/@pnpm.e2e/hello-world-js-bin-parent/package.json").is_file(), + "real directory should contain the package's package.json", + ); + // Transitive dep is hoisted flat to the top level as a real dir. + assert!( + is_real_dir("node_modules/@pnpm.e2e/hello-world-js-bin"), + "transitive dep should be hoisted to a real directory at the top level", + ); + // The hoisted linker writes no virtual-store slot directories. + // (`node_modules/.pnpm` itself may exist to hold the current + // `lock.yaml`, matching pnpm, but no per-package slot is laid + // down — that's the isolated linker's shape.) + assert!( + !workspace.join("node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0").exists(), + "hoisted linker must not materialize a virtual-store slot for the package", + ); + assert!( + !workspace.join("node_modules/.pnpm/node_modules").exists(), + "hoisted linker must not create a private-hoist `.pnpm/node_modules` tree", + ); + // The transitive's bin is shimmed into the top-level `.bin`. + assert!( + workspace.join("node_modules/.bin/hello-world-js-bin").exists(), + "hoisted linker should link the transitive's bin into node_modules/.bin", + ); + // A fresh install writes a `pnpm-lock.yaml` for the next run. + assert!( + workspace.join("pnpm-lock.yaml").is_file(), + "fresh install should write a wanted lockfile", + ); + + drop((root, mock_instance)); +} + mod known_failures { //! Test ports of upstream `hoist.ts` cases blocked on features //! pacquet hasn't built yet. Each entry stubs the not-yet-built diff --git a/pacquet/crates/cli/tests/hoisted_node_linker.rs b/pacquet/crates/cli/tests/hoisted_node_linker.rs new file mode 100644 index 0000000000..7be274c063 --- /dev/null +++ b/pacquet/crates/cli/tests/hoisted_node_linker.rs @@ -0,0 +1,300 @@ +//! End-to-end coverage for `nodeLinker: hoisted` on the +//! **fresh-lockfile** install path (no checked-in lockfile, not +//! `--frozen-lockfile`). pnpm/pnpm#11871 enabled this path; before +//! it, `pacquet install` hard-refused the combination. +//! +//! Each test writes a `package.json` (and a `pnpm-workspace.yaml` +//! carrying `nodeLinker: hoisted` plus any feature knob under test), +//! then runs `pacquet install` so the fresh resolver builds the +//! lockfile in memory and the hoisted linker materializes a flat +//! `node_modules/` of **real directories**. +//! +//! Test ports of upstream's +//! [`installing/deps-installer/test/hoistedNodeLinker/install.ts`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/hoistedNodeLinker/install.ts). +//! Cases that depend on features pacquet hasn't built yet — `pnpm add` +//! / update manifest mutation (pnpm/pacquet#433), lifecycle scripts + +//! bin linking on the fresh path (#11870) — live in [`known_failures`] +//! below with [`pacquet_testing_utils::allow_known_failure`] gating the +//! assertion against the not-yet-implemented subject under test. + +#![cfg(unix)] // hoisted bin shims + real-dir-vs-junction checks are unix-shaped here. + +pub mod _utils; +pub use _utils::*; + +use assert_cmd::prelude::*; +use command_extra::CommandExtra; +use pacquet_testing_utils::{ + bin::{AddMockedRegistry, CommandTempCwd}, + fs::is_symlink_or_junction, +}; +use std::{fs, path::Path}; + +/// Replace the `pnpm-workspace.yaml` written by `add_mocked_registry` +/// with one that keeps the mock's `storeDir` / `cacheDir` and appends +/// `extra` (e.g. `nodeLinker: hoisted`). +fn write_workspace_yaml(workspace: &Path, extra: &str) { + let yaml = format!("storeDir: ../pacquet-store\ncacheDir: ../pacquet-cache\n{extra}"); + fs::write(workspace.join("pnpm-workspace.yaml"), yaml).expect("write pnpm-workspace.yaml"); +} + +/// Write a `package.json` with the given `dependencies` object. +fn write_manifest(workspace: &Path, deps: serde_json::Value) { + let manifest = serde_json::json!({ "dependencies": deps }); + fs::write(workspace.join("package.json"), manifest.to_string()).expect("write package.json"); +} + +/// `true` when `relative` resolves to a real directory (not a symlink +/// or junction) under `workspace`. This is the hoisted-linker +/// contract: regular deps are materialized as real directories, not +/// symlinks into a virtual store. Mirrors upstream's +/// `realpathSync(p) === resolve(p)` check. +fn is_real_dir(workspace: &Path, relative: &str) -> bool { + let path = workspace.join(relative); + path.is_dir() && !is_symlink_or_junction(&path).unwrap() +} + +/// Upstream: [`install.ts:16` "installing with hoisted node-linker"](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/hoistedNodeLinker/install.ts#L16). +/// +/// Direct deps land as real directories at the project root and a +/// version-conflicting transitive nests under its consumer. `send` +/// pulls `ms@2.x` while the root pins `ms@1.0.0`, so the root keeps +/// `1.0.0` and `send` nests its own `ms`. `.modules.yaml` records the +/// hoisted linker. +/// +/// The upstream test also removes `node_modules/send` and reinstalls +/// to assert it is re-added; that re-add is the partial-install path +/// (pnpm/pacquet#433) and is omitted here. +#[test] +fn installing_with_hoisted_node_linker() { + let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + write_manifest( + &workspace, + serde_json::json!({ "send": "0.17.2", "has-flag": "1.0.0", "ms": "1.0.0" }), + ); + write_workspace_yaml(&workspace, "nodeLinker: hoisted\n"); + + pacquet.with_args(["install"]).assert().success(); + + assert!(is_real_dir(&workspace, "node_modules/send"), "send should be a real directory"); + assert!( + is_real_dir(&workspace, "node_modules/has-flag"), + "has-flag should be a real directory", + ); + assert!(is_real_dir(&workspace, "node_modules/ms"), "ms should be a real directory"); + // Version conflict: send needs ms@2.x, the root pins ms@1.0.0, so + // send keeps its own copy nested. + assert!( + workspace.join("node_modules/send/node_modules/ms").exists(), + "send's conflicting ms should nest under send/node_modules/ms", + ); + + // `.modules.yaml` is written JSON-with-quoted-keys (valid YAML); + // a substring match avoids dragging in a YAML parser, matching the + // convention in the sibling `hoist.rs` tests. + let modules_yaml = fs::read_to_string(workspace.join("node_modules/.modules.yaml")) + .expect("read .modules.yaml"); + assert!( + modules_yaml.contains(r#""nodeLinker": "hoisted""#), + ".modules.yaml should record the hoisted linker; got:\n{modules_yaml}", + ); + + drop((root, mock_instance)); +} + +/// Upstream: [`install.ts:45` "installing with hoisted node-linker and no lockfile"](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/hoistedNodeLinker/install.ts#L45). +/// +/// With `lockfile: false` the hoisted install still materializes a +/// real directory and writes no `pnpm-lock.yaml`. +#[test] +fn installing_with_hoisted_node_linker_and_no_lockfile() { + let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + write_manifest(&workspace, serde_json::json!({ "ms": "1.0.0" })); + write_workspace_yaml(&workspace, "nodeLinker: hoisted\nlockfile: false\n"); + + pacquet.with_args(["install"]).assert().success(); + + assert!(is_real_dir(&workspace, "node_modules/ms"), "ms should be a real directory"); + assert!( + !workspace.join("pnpm-lock.yaml").exists(), + "no lockfile should be written when lockfile: false", + ); + + drop((root, mock_instance)); +} + +/// Upstream: [`install.ts:229` "hoistingLimits should prevent packages to be hoisted"](https://github.com/pnpm/pnpm/blob/89812a9353/installing/deps-installer/test/hoistedNodeLinker/install.ts#L229). +/// +/// `hoistingLimits: dependencies` borders each direct dependency, so +/// `send`'s transitive `ms` stays nested under `send` instead of +/// hoisting to the root. +#[test] +fn hoisting_limits_prevents_hoisting() { + let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + write_manifest(&workspace, serde_json::json!({ "send": "0.17.2" })); + write_workspace_yaml(&workspace, "nodeLinker: hoisted\nhoistingLimits: dependencies\n"); + + pacquet.with_args(["install"]).assert().success(); + + assert!( + !workspace.join("node_modules/ms").exists(), + "ms should not be hoisted to the root when send's deps are bordered", + ); + assert!( + workspace.join("node_modules/send/node_modules/ms").exists(), + "ms should stay nested under send", + ); + + drop((root, mock_instance)); +} + +/// Upstream: [`install.ts:247` "externalDependencies should prevent package from being hoisted to the root"](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/hoistedNodeLinker/install.ts#L247). +/// +/// `externalDependencies: [ms]` reserves the root `ms` slot for an +/// external linker, so `ms` is not hoisted to the root and stays +/// nested under `send`. +#[test] +fn external_dependencies_prevents_hoisting_to_root() { + let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + write_manifest(&workspace, serde_json::json!({ "send": "0.17.2" })); + write_workspace_yaml(&workspace, "nodeLinker: hoisted\nexternalDependencies:\n - ms\n"); + + pacquet.with_args(["install"]).assert().success(); + + assert!( + !workspace.join("node_modules/ms").exists(), + "ms should not be hoisted to the root when declared external", + ); + assert!( + workspace.join("node_modules/send/node_modules/ms").exists(), + "ms should stay nested under send", + ); + + drop((root, mock_instance)); +} + +/// Upstream: [`install.ts:314` "peerDependencies should be installed when autoInstallPeers is set to true and nodeLinker is set to hoisted"](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/hoistedNodeLinker/install.ts#L314). +/// +/// With `autoInstallPeers: true`, `react-dom`'s `react` peer is +/// resolved and lands as a real directory at the hoisted root. +#[test] +fn peer_dependencies_installed_with_auto_install_peers() { + let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + write_manifest(&workspace, serde_json::json!({ "react-dom": "18.2.0" })); + write_workspace_yaml(&workspace, "nodeLinker: hoisted\nautoInstallPeers: true\n"); + + pacquet.with_args(["install"]).assert().success(); + + assert!( + workspace.join("node_modules/react").exists(), + "react peer should be installed under the hoisted root", + ); + + drop((root, mock_instance)); +} + +mod known_failures { + //! Ports of upstream `hoistedNodeLinker/install.ts` cases blocked + //! on features pacquet hasn't built yet. Each stubs the + //! not-yet-built subject through + //! [`pacquet_testing_utils::allow_known_failure`] so the test exits + //! early rather than masking a real bug. + + use pacquet_testing_utils::{ + allow_known_failure, + known_failure::{KnownFailure, KnownResult}, + }; + + fn manifest_mutation_via_pnpm_add() -> KnownResult<()> { + Err(KnownFailure::new( + "Pacquet doesn't yet implement the `pnpm add` / update \ + manifest-mutation flow these tests exercise (add a dep, or \ + bump a dist-tag, then reinstall). Partial install / re-hoist \ + across runs is tracked by pnpm/pacquet#433.", + )) + } + + fn lifecycle_scripts_on_fresh_path() -> KnownResult<()> { + Err(KnownFailure::new( + "The fresh-lockfile install path doesn't run lifecycle \ + scripts or link per-node_modules bins yet — that's the \ + `BuildModules` port tracked by #11870. Until it lands, \ + pre/postinstall-script and local-bin assertions under the \ + hoisted linker can't be exercised on the fresh path.", + )) + } + + /// Upstream: [`install.ts:61` "overwriting (is-positive@3.0.0 with is-positive@latest)"](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/hoistedNodeLinker/install.ts#L61). + #[test] + fn overwriting_is_positive_with_latest() { + allow_known_failure!(manifest_mutation_via_pnpm_add()); + } + + /// Upstream: [`install.ts:83` "overwriting existing files in node_modules"](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/hoistedNodeLinker/install.ts#L83). + #[test] + fn overwriting_existing_files_in_node_modules() { + allow_known_failure!(manifest_mutation_via_pnpm_add()); + } + + /// Upstream: [`install.ts:97` "preserve subdeps on update"](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/hoistedNodeLinker/install.ts#L97). + #[test] + fn preserve_subdeps_on_update() { + allow_known_failure!(manifest_mutation_via_pnpm_add()); + } + + /// Upstream: [`install.ts:119` "adding a new dependency to one of the workspace projects"](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/hoistedNodeLinker/install.ts#L119). + #[test] + fn adding_a_new_dependency_to_a_workspace_project() { + allow_known_failure!(manifest_mutation_via_pnpm_add()); + } + + /// Upstream: [`install.ts:172` "installing the same package with alias and no alias"](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/hoistedNodeLinker/install.ts#L172). + /// Relies on `pnpm add` of multiple specifiers plus a dist-tag + /// bump to pin the aliased and unaliased copies to the same + /// version. + #[test] + fn installing_same_package_with_alias_and_no_alias() { + allow_known_failure!(manifest_mutation_via_pnpm_add()); + } + + /// Upstream: [`install.ts:329` "installing with hoisted node-linker a package that is a peer dependency of itself"](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/hoistedNodeLinker/install.ts#L329). + /// Adds the dep via `pnpm add --save` and then introspects the + /// written lockfile's `peerDependencies` entry. + #[test] + fn package_that_is_peer_dependency_of_itself() { + allow_known_failure!(manifest_mutation_via_pnpm_add()); + } + + /// Upstream: [`install.ts:187` "run pre/postinstall scripts. bin files should be linked in a hoisted node_modules"](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/hoistedNodeLinker/install.ts#L187). + #[test] + fn run_pre_and_postinstall_scripts_and_link_bins() { + allow_known_failure!(lifecycle_scripts_on_fresh_path()); + } + + /// Upstream: [`install.ts:210` "running install scripts in a workspace that has no root project"](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/hoistedNodeLinker/install.ts#L210). + #[test] + fn running_install_scripts_in_workspace_without_root_project() { + allow_known_failure!(lifecycle_scripts_on_fresh_path()); + } + + /// Upstream: [`install.ts:264` "linking bins of local projects when node-linker is set to hoisted"](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/hoistedNodeLinker/install.ts#L264). + #[test] + fn linking_bins_of_local_projects() { + allow_known_failure!(lifecycle_scripts_on_fresh_path()); + } +} diff --git a/pacquet/crates/config/src/env_overlay.rs b/pacquet/crates/config/src/env_overlay.rs index 9a2c4007ac..7333dffa90 100644 --- a/pacquet/crates/config/src/env_overlay.rs +++ b/pacquet/crates/config/src/env_overlay.rs @@ -17,8 +17,8 @@ //! [`config/reader/src/index.ts:719-722`](https://github.com/pnpm/pnpm/blob/2a9bd897bf/config/reader/src/index.ts#L719-L722). use crate::{ - NodeLinker, PackageImportMethod, ScriptsPrependNodePath, TrustPolicy, WorkspaceSettings, - api::EnvVar, + HoistingLimits, NodeLinker, PackageImportMethod, ScriptsPrependNodePath, TrustPolicy, + WorkspaceSettings, api::EnvVar, }; use serde::de::DeserializeOwned; @@ -141,7 +141,7 @@ impl WorkspaceSettings { json_field!(auto_install_peers_from_highest_match, "AUTO_INSTALL_PEERS_FROM_HIGHEST_MATCH"); json_field!(exclude_links_from_lockfile, "EXCLUDE_LINKS_FROM_LOCKFILE"); json_field!(hoist_workspace_packages, "HOIST_WORKSPACE_PACKAGES"); - json_field!(hoisting_limits, "HOISTING_LIMITS"); + enum_field!(hoisting_limits, "HOISTING_LIMITS", HoistingLimits); json_field!(external_dependencies, "EXTERNAL_DEPENDENCIES"); json_field!(dedupe_peer_dependents, "DEDUPE_PEER_DEPENDENTS"); json_field!(dedupe_peers, "DEDUPE_PEERS"); diff --git a/pacquet/crates/config/src/lib.rs b/pacquet/crates/config/src/lib.rs index 2be9557357..6724dc058a 100644 --- a/pacquet/crates/config/src/lib.rs +++ b/pacquet/crates/config/src/lib.rs @@ -55,6 +55,32 @@ pub enum NodeLinker { Pnp, } +/// Controls how far dependencies are hoisted under +/// `nodeLinker: hoisted`, mirroring yarn's `nmHoistingLimits`. +/// +/// Given workspace package `A` → `B` → `C`: +/// - [`HoistingLimits::None`] (default): hoist as far as possible +/// (`/node_modules/B`, `/node_modules/C`). +/// - [`HoistingLimits::Workspaces`]: hoist only as far as each +/// workspace package (`/packages/A/node_modules/{B,C}`). +/// - [`HoistingLimits::Dependencies`]: hoist only up to each +/// workspace package's direct dependencies +/// (`/packages/A/node_modules/B/node_modules/C`). +/// +/// Mirrors pnpm's +/// [`HoistingLimits`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/linking/real-hoist/src/index.ts#L10). +/// No effect under `nodeLinker: isolated`. The user-facing mode is +/// translated into the per-locator border map the hoister consumes +/// by `crate::get_hoisting_limits` in `pacquet-package-manager`. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub enum HoistingLimits { + #[default] + None, + Workspaces, + Dependencies, +} + /// Supply-chain trust policy applied to lockfile entries. /// /// Mirrors pnpm's @@ -581,21 +607,15 @@ pub struct Config { /// Per-importer block-list of package aliases that may NOT be /// hoisted past that importer's slot. Outer key is the /// importer locator (e.g. `'.@'` for the root project, or the - /// percent-encoded importer id with the `@` slot suffix); - /// inner set is the alias names whose hoisting is bordered. - /// - /// Programmatic-only upstream — pnpm exposes it through the - /// embedded API and Bit CLI rather than `pnpm-workspace.yaml`, - /// because the ergonomics of the locator-keyed map don't - /// translate cleanly to a yaml setting. Pacquet exposes it - /// via `HoistOpts::hoisting_limits` (in `pacquet-real-hoist`) - /// and reads the same yaml shape (`hoistingLimits: { ".@": [...] }`) - /// for parity. - /// - /// Default empty (no aliases bordered). Mirrors upstream's - /// [`hoistingLimits`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/linking/real-hoist/src/index.ts#L10). - /// No effect under `nodeLinker: isolated`. - pub hoisting_limits: BTreeMap>, + /// `hoistingLimits` from `pnpm-workspace.yaml`. Controls how far + /// dependencies are hoisted under `nodeLinker: hoisted`. See + /// [`HoistingLimits`] for the `none` / `workspaces` / + /// `dependencies` semantics. Default [`HoistingLimits::None`] + /// (hoist as far as possible). Translated into the hoister's + /// per-locator border map by `crate::get_hoisting_limits` in + /// `pacquet-package-manager`. No effect under + /// `nodeLinker: isolated`. + pub hoisting_limits: HoistingLimits, /// `linkWorkspacePackages` from `pnpm-workspace.yaml`. Controls /// whether the npm resolver consults the workspace map when diff --git a/pacquet/crates/config/src/workspace_yaml.rs b/pacquet/crates/config/src/workspace_yaml.rs index 2dadb925b8..4933fbaad9 100644 --- a/pacquet/crates/config/src/workspace_yaml.rs +++ b/pacquet/crates/config/src/workspace_yaml.rs @@ -1,6 +1,6 @@ use crate::{ - Config, LinkWorkspacePackages, NodeLinker, PackageImportMethod, ScriptsPrependNodePath, - TrustPolicy, api::EnvVar, resolve_child_concurrency, + Config, HoistingLimits, LinkWorkspacePackages, NodeLinker, PackageImportMethod, + ScriptsPrependNodePath, TrustPolicy, api::EnvVar, resolve_child_concurrency, }; use derive_more::{Display, Error}; use indexmap::IndexMap; @@ -149,12 +149,11 @@ pub struct WorkspaceSettings { /// `file:` (hard-linked copy) instead of a `link:` symlink. See /// [`Config::inject_workspace_packages`]. pub inject_workspace_packages: Option, - /// `hoistingLimits` from `pnpm-workspace.yaml`. Outer key is - /// the importer locator (e.g. `'.@'`); inner list is the - /// alias names whose hoisting is bordered. Mirrors upstream's - /// programmatic-only knob shape, exposed here as yaml for - /// parity. Empty / missing → no limits. - pub hoisting_limits: Option>>, + /// `hoistingLimits` from `pnpm-workspace.yaml`. One of `none`, + /// `workspaces`, or `dependencies` — see + /// [`crate::HoistingLimits`]. Missing → default + /// [`crate::HoistingLimits::None`]. + pub hoisting_limits: Option, /// `externalDependencies` from `pnpm-workspace.yaml`. Names /// whose top-level slot is reserved for an external linker /// and stripped from the hoist tree. Mirrors upstream's diff --git a/pacquet/crates/config/src/workspace_yaml/tests.rs b/pacquet/crates/config/src/workspace_yaml/tests.rs index 467de3bb82..ebed93cbfd 100644 --- a/pacquet/crates/config/src/workspace_yaml/tests.rs +++ b/pacquet/crates/config/src/workspace_yaml/tests.rs @@ -1,6 +1,7 @@ use super::{LoadWorkspaceYamlError, WORKSPACE_MANIFEST_FILENAME, WorkspaceSettings}; use crate::{ - Config, LinkWorkspacePackages, NodeLinker, ScriptsPrependNodePath, TrustPolicy, api::EnvVar, + Config, HoistingLimits, LinkWorkspacePackages, NodeLinker, ScriptsPrependNodePath, TrustPolicy, + api::EnvVar, }; use pacquet_store_dir::StoreDir; use pipe_trait::Pipe; @@ -972,30 +973,23 @@ fn empty_package_extensions_map_collapses_to_none() { assert!(config.package_extensions.is_none(), "empty map collapses to None"); } -/// `hoistingLimits` deserializes as a map keyed by importer -/// locator (e.g. `'.@'`); inner value is a list of alias names. -/// Mirrors upstream's [`HoistingLimits`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/linking/real-hoist/src/index.ts#L10) -/// shape and threads straight into [`pacquet_real_hoist::HoistOpts`] -/// via the install pipeline. Yaml-empty / missing keeps the -/// `Config` field at its `BTreeMap::default()` empty value. +/// `hoistingLimits` deserializes as one of the `none` / `workspaces` +/// / `dependencies` modes. Mirrors upstream's +/// [`HoistingLimits`](https://github.com/pnpm/pnpm/blob/89812a9353/installing/linking/real-hoist/src/index.ts) +/// shape; the install pipeline translates the mode into the +/// per-locator border map via `pacquet_package_manager::get_hoisting_limits`. +/// Yaml-empty / missing keeps the `Config` field at its +/// [`HoistingLimits::None`] default. #[test] fn parses_hoisting_limits_from_yaml_and_applies() { - let yaml = r#" -hoistingLimits: - ".@": - - foo - - bar -"#; + let yaml = "hoistingLimits: dependencies\n"; let settings: WorkspaceSettings = serde_saphyr::from_str(yaml).unwrap(); - let raw = settings.hoisting_limits.clone().expect("field present"); - let aliases = raw.get(".@").expect("locator present"); - assert!(aliases.contains("foo") && aliases.contains("bar")); + assert_eq!(settings.hoisting_limits, Some(HoistingLimits::Dependencies)); let mut config = Config::new(); - assert!(config.hoisting_limits.is_empty(), "default is empty"); + assert_eq!(config.hoisting_limits, HoistingLimits::None, "default is None"); settings.apply_to(&mut config, Path::new("/irrelevant")); - let aliases = config.hoisting_limits.get(".@").expect("locator present in config"); - assert!(aliases.contains("foo") && aliases.contains("bar")); + assert_eq!(config.hoisting_limits, HoistingLimits::Dependencies); } /// `externalDependencies` deserializes as a flat list of names. @@ -1032,7 +1026,7 @@ fn omitting_hoisting_limits_and_external_dependencies_keeps_defaults() { let mut config = Config::new(); settings.apply_to(&mut config, Path::new("/irrelevant")); - assert!(config.hoisting_limits.is_empty()); + assert_eq!(config.hoisting_limits, HoistingLimits::None); assert!(config.external_dependencies.is_empty()); } diff --git a/pacquet/crates/package-manager/src/hoisted_dep_graph.rs b/pacquet/crates/package-manager/src/hoisted_dep_graph.rs index 120d6ee2fc..813430b775 100644 --- a/pacquet/crates/package-manager/src/hoisted_dep_graph.rs +++ b/pacquet/crates/package-manager/src/hoisted_dep_graph.rs @@ -345,9 +345,11 @@ pub enum HoistedDepGraphError { /// populated by Slice 5's linker, which kicks off the actual /// store fetches when it has a real consumer for the handles. /// -/// Single-importer only today — multi-importer (workspace) -/// lockfiles surface as `HoistError::UnsupportedWorkspace` via -/// the hoister. +/// Multi-importer (workspace) lockfiles are supported: the hoister +/// ([`pacquet_real_hoist::hoist`]) attaches each non-root importer as +/// a workspace child of the virtual `.` root when +/// `hoist_workspace_packages` is enabled. Per-importer hoisting roots +/// (upstream's multi-level output shape) are not modelled yet. pub fn lockfile_to_hoisted_dep_graph( lockfile: &Lockfile, current_lockfile: Option<&Lockfile>, diff --git a/pacquet/crates/package-manager/src/hoisting_limits.rs b/pacquet/crates/package-manager/src/hoisting_limits.rs new file mode 100644 index 0000000000..b0dfd53594 --- /dev/null +++ b/pacquet/crates/package-manager/src/hoisting_limits.rs @@ -0,0 +1,176 @@ +use pacquet_config::HoistingLimits; +use pacquet_lockfile::{Lockfile, ProjectSnapshot, ResolvedDependencyMap}; +use pacquet_real_hoist::percent_encode_path; +use std::collections::{BTreeSet, HashMap}; + +/// Translate the user-facing [`HoistingLimits`] mode into the +/// `@yarnpkg/nm` hoister's per-locator border map (the shape +/// [`pacquet_real_hoist::HoistOpts::hoisting_limits`] consumes). A +/// name in a locator's set is a hoisting border: that node's +/// dependencies are not hoisted above it. +/// +/// Ports pnpm's +/// [`getHoistingLimits`](https://github.com/pnpm/pnpm/blob/89812a9353/installing/linking/real-hoist/src/index.ts): +/// +/// - [`HoistingLimits::None`] → empty map (hoist as far as possible). +/// - [`HoistingLimits::Workspaces`] → border every workspace package +/// (and the root's direct deps) at the root locator, so each +/// project's dependencies stay within that project. +/// - [`HoistingLimits::Dependencies`] → additionally border each +/// workspace package's own direct dependencies, so their +/// transitives stay nested beneath them. +/// +/// Pacquet's hoister currently hoists into the single root importer +/// only, so it consults the `.@` entry; the per-importer entries the +/// `dependencies` mode emits are produced for parity and become +/// load-bearing once multi-level hoisting lands. +pub fn get_hoisting_limits( + importers: &HashMap, + mode: HoistingLimits, +) -> pacquet_real_hoist::HoistingLimits { + let mut limits = pacquet_real_hoist::HoistingLimits::new(); + if matches!(mode, HoistingLimits::None) { + return limits; + } + + // The root border accumulates the root's own direct deps plus + // every (encoded) non-root importer id, regardless of iteration + // order — `BTreeSet` makes the result deterministic even though + // `importers` is a `HashMap`. Only stored under `.@` when a root + // importer is present, matching upstream. + let mut root_border: BTreeSet = BTreeSet::new(); + let mut root_present = false; + + for (importer_id, importer) in importers { + if importer_id == Lockfile::ROOT_IMPORTER_KEY { + root_present = true; + collect_direct_dep_names(importer, &mut root_border); + continue; + } + + root_border.insert(percent_encode_path(importer_id)); + if !matches!(mode, HoistingLimits::Dependencies) { + // `workspaces` mode borders each package at the root only; + // their own direct deps don't get a per-importer border. + continue; + } + + let mut importer_border: BTreeSet = BTreeSet::new(); + collect_direct_dep_names(importer, &mut importer_border); + limits.insert( + format!("{}@workspace:{importer_id}", percent_encode_path(importer_id)), + importer_border, + ); + } + + if root_present { + limits.insert(format!("{}@", Lockfile::ROOT_IMPORTER_KEY), root_border); + } + + limits +} + +/// Collect every direct-dependency alias of `importer` (across the +/// regular, dev, and optional groups) into `out`. Aliases are the +/// node names the hoister matches borders against. +fn collect_direct_dep_names(importer: &ProjectSnapshot, out: &mut BTreeSet) { + for group in [ + importer.dependencies.as_ref(), + importer.dev_dependencies.as_ref(), + importer.optional_dependencies.as_ref(), + ] { + let Some(deps) = group else { continue }; + add_alias_names(deps, out); + } +} + +fn add_alias_names(deps: &ResolvedDependencyMap, out: &mut BTreeSet) { + for alias in deps.keys() { + out.insert(alias.to_string()); + } +} + +#[cfg(test)] +mod tests { + use super::get_hoisting_limits; + use pacquet_config::HoistingLimits; + use pacquet_lockfile::{ + Lockfile, PkgName, PkgVerPeer, ProjectSnapshot, ResolvedDependencyMap, + ResolvedDependencySpec, + }; + use std::collections::{BTreeSet, HashMap}; + + fn project_with_deps(names: &[&str]) -> ProjectSnapshot { + let mut deps = ResolvedDependencyMap::new(); + for name in names { + // `get_hoisting_limits` reads only the alias keys; the spec + // value is filled in just to satisfy the map's value type. + deps.insert( + name.parse::().expect("valid pkg name"), + ResolvedDependencySpec { + specifier: "1.0.0".to_string(), + version: "1.0.0".parse::().expect("parse version").into(), + }, + ); + } + ProjectSnapshot { dependencies: Some(deps), ..ProjectSnapshot::default() } + } + + fn root_only() -> HashMap { + let mut importers = HashMap::new(); + importers.insert(Lockfile::ROOT_IMPORTER_KEY.to_string(), project_with_deps(&["a", "b"])); + importers + } + + /// `none` (the default) produces no borders, so the hoister + /// flattens as far as possible. + #[test] + fn none_mode_yields_no_borders() { + assert!(get_hoisting_limits(&root_only(), HoistingLimits::None).is_empty()); + } + + /// For a single root project, every mode that limits hoisting + /// borders the root's direct deps at the `.@` locator. + #[test] + fn root_direct_deps_are_bordered_under_dependencies_mode() { + let limits = get_hoisting_limits(&root_only(), HoistingLimits::Dependencies); + assert_eq!(limits.keys().cloned().collect::>(), vec![".@".to_string()]); + assert_eq!(limits[".@"], BTreeSet::from(["a".to_string(), "b".to_string()])); + } + + /// `workspaces` mode borders each workspace package (encoded id) + /// and the root's direct deps at the root locator, with no + /// per-importer entry. + #[test] + fn workspaces_mode_borders_packages_at_root() { + let mut importers = HashMap::new(); + importers.insert(Lockfile::ROOT_IMPORTER_KEY.to_string(), project_with_deps(&["a"])); + importers.insert("packages/foo".to_string(), project_with_deps(&["b"])); + + let limits = get_hoisting_limits(&importers, HoistingLimits::Workspaces); + assert_eq!(limits.keys().cloned().collect::>(), vec![".@".to_string()]); + assert_eq!(limits[".@"], BTreeSet::from(["a".to_string(), "packages%2Ffoo".to_string()])); + } + + /// `dependencies` mode additionally borders each non-root + /// importer's own direct deps under its workspace locator. + #[test] + fn dependencies_mode_borders_each_importer() { + let mut importers = HashMap::new(); + importers.insert(Lockfile::ROOT_IMPORTER_KEY.to_string(), project_with_deps(&["a"])); + importers.insert("packages/foo".to_string(), project_with_deps(&["b"])); + + let limits = get_hoisting_limits(&importers, HoistingLimits::Dependencies); + let mut keys = limits.keys().cloned().collect::>(); + keys.sort(); + assert_eq!( + keys, + vec![".@".to_string(), "packages%2Ffoo@workspace:packages/foo".to_string()], + ); + assert_eq!(limits[".@"], BTreeSet::from(["a".to_string(), "packages%2Ffoo".to_string()])); + assert_eq!( + limits["packages%2Ffoo@workspace:packages/foo"], + BTreeSet::from(["b".to_string()]), + ); + } +} diff --git a/pacquet/crates/package-manager/src/install.rs b/pacquet/crates/package-manager/src/install.rs index 0ce73910a3..e794557ffe 100644 --- a/pacquet/crates/package-manager/src/install.rs +++ b/pacquet/crates/package-manager/src/install.rs @@ -151,22 +151,6 @@ pub enum InstallError { #[diagnostic(transparent)] WithFreshLockfile(#[error(source)] InstallWithFreshLockfileError), - /// Requested `nodeLinker` value isn't supported on the - /// fresh-lockfile path yet. Pacquet's hoist pass runs only over - /// a loaded lockfile's snapshots (`link_hoisted_modules`); a - /// non-frozen install with `nodeLinker: hoisted` would produce - /// an isolated layout silently, which doesn't match the user's - /// intent. Re-run with `--frozen-lockfile`, or set - /// `nodeLinker: isolated`. - #[display( - "nodeLinker: {node_linker:?} is not supported without --frozen-lockfile yet. Re-run with --frozen-lockfile against an existing pnpm-lock.yaml, or set nodeLinker: isolated." - )] - #[diagnostic(code(pacquet_package_manager::unsupported_fresh_install_node_linker))] - UnsupportedFreshInstallNodeLinker { - #[error(not(source))] - node_linker: NodeLinker, - }, - /// `--no-runtime` (or `config.skip_runtimes`) is honored only on /// the frozen-lockfile path today, where the runtime filter runs /// against the loaded lockfile's `packages:` map. A non-frozen @@ -778,21 +762,12 @@ where // auto-frozen install (state 2 of [`Install::run`]) doesn't // get rejected up front: // - // - `nodeLinker: hoisted` on the fresh path would need a - // port of upstream's hoist pass against the freshly-built - // graph (the frozen path uses `link_hoisted_modules` over - // the lockfile's snapshots). Falling through to the - // isolated linker would lay out `node_modules` in the - // wrong shape, so refuse the install instead. // - `skip_runtimes` (CLI `--no-runtime`) on the fresh path // would need a runtime-filter at the materialization step // matching the frozen path's // [`installing/deps-installer/src/install/index.ts:1374-1387`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/index.ts#L1374-L1387) // filter. Without it, runtime archives get fetched + // materialized despite the opt-out. - if matches!(node_linker, NodeLinker::Hoisted) { - return Err(InstallError::UnsupportedFreshInstallNodeLinker { node_linker }); - } if skip_runtimes { return Err(InstallError::UnsupportedFreshInstallSkipRuntimes); } @@ -865,6 +840,8 @@ where // upstream's `update: false` mode. State 4 (no // lockfile) passes `None`. wanted_lockfile: lockfile, + node_linker, + supported_architectures: supported_architectures.as_ref(), } .run::() .await @@ -872,7 +849,7 @@ where ( fresh_result.hoisted_dependencies, - BTreeMap::new(), + fresh_result.hoisted_locations, crate::SkippedSnapshots::new(), fresh_result.wanted_lockfile, ) diff --git a/pacquet/crates/package-manager/src/install/tests.rs b/pacquet/crates/package-manager/src/install/tests.rs index f0731822f4..85171c9d5a 100644 --- a/pacquet/crates/package-manager/src/install/tests.rs +++ b/pacquet/crates/package-manager/src/install/tests.rs @@ -4428,30 +4428,32 @@ async fn fresh_install_marks_optional_snapshots_in_pnpm_lock_yaml() { drop((dir, mock_instance)); } -/// `nodeLinker: hoisted` on the fresh-lockfile path is refused up -/// front: pacquet's hoist pass runs only against a loaded lockfile's -/// snapshots, so a from-scratch install with the flag would silently -/// produce an isolated layout. The error must fire *before* any -/// state file lands so a follow-up `--frozen-lockfile` retry can -/// reuse the workspace. +/// `nodeLinker: hoisted` on the fresh-lockfile path (no lockfile, +/// not frozen) installs successfully and records the hoisted linker +/// in `.modules.yaml`. With an empty manifest there is nothing to +/// materialize, so the assertion focuses on the dispatch reaching +/// the hoisted-linker pipeline rather than bailing — the previous +/// hard-refusal at this site (#11871) is gone. #[tokio::test] -async fn fresh_install_refuses_hoisted_node_linker_before_writing_state() { +async fn fresh_install_hoisted_node_linker_records_modules_yaml() { let dir = tempdir().unwrap(); let store_dir = dir.path().join("pacquet-store"); let project_root = dir.path().join("project"); let modules_dir = project_root.join("node_modules"); let virtual_store_dir = modules_dir.join(".pacquet"); - let manifest_path = dir.path().join("package.json"); + std::fs::create_dir_all(&project_root).expect("create project root"); + let manifest_path = project_root.join("package.json"); let manifest = PackageManifest::create_if_needed(manifest_path).unwrap(); let mut config = Config::new(); + config.lockfile = false; config.store_dir = store_dir.into(); - config.modules_dir = modules_dir.to_path_buf(); + config.modules_dir = modules_dir.clone(); config.virtual_store_dir = virtual_store_dir.clone(); let config = config.leak(); - let result = Install { + Install { tarball_mem_cache: Default::default(), http_client: &Default::default(), http_client_arc: std::sync::Arc::new(Default::default()), @@ -4471,12 +4473,28 @@ async fn fresh_install_refuses_hoisted_node_linker_before_writing_state() { resolved_packages: &Default::default(), } .run::() - .await; + .await + .expect("fresh hoisted-linker install should succeed"); - assert!(matches!(result, Err(InstallError::UnsupportedFreshInstallNodeLinker { .. }))); - assert!(!dir.path().join(Lockfile::FILE_NAME).exists(), "no wanted lockfile written"); - assert!(!virtual_store_dir.join(Lockfile::CURRENT_FILE_NAME).exists(), "no current lockfile"); - assert!(!modules_dir.join(".modules.yaml").exists(), "no modules manifest"); + let written = modules_dir + .pipe_as_ref(read_modules_manifest::) + .expect("read .modules.yaml") + .expect("modules manifest exists"); + + assert_eq!(written.node_linker, Some(NodeLinker::Hoisted)); + // Empty manifest → no packages, so the hoisted linker records no + // locations. The field is `None`-when-empty so a stale + // `hoistedLocations: {}` key isn't written. + assert!( + written.hoisted_locations.is_none(), + "empty manifest produces no hoisted_locations: {:?}", + written.hoisted_locations, + ); + // Hoisted skips the virtual store entirely. + assert!( + !virtual_store_dir.exists(), + "hoisted install must not materialize the virtual-store root at {virtual_store_dir:?}", + ); drop(dir); } diff --git a/pacquet/crates/package-manager/src/install_frozen_lockfile.rs b/pacquet/crates/package-manager/src/install_frozen_lockfile.rs index 77885b6efd..94e4810cbf 100644 --- a/pacquet/crates/package-manager/src/install_frozen_lockfile.rs +++ b/pacquet/crates/package-manager/src/install_frozen_lockfile.rs @@ -714,125 +714,25 @@ where // pick. `None` (and an empty `hoisted_locations`) for the // isolated linker. let HoistedLinkerOutput { hoisted_locations, hoisted_pkg_root_by_key } = if is_hoisted { - // Walker installability inputs come straight from the - // optional `host_node` we built earlier for the - // `compute_skipped_snapshots` pass. When `host_node` is - // `None` no per-snapshot constraint exists, so the - // host triple values pass through as defaults that the - // walker won't actually consult. - let host_for_walker = host_node.as_ref(); - let walker_skipped: BTreeSet = - skipped.iter().map(|key| key.to_string()).collect(); - let walker_opts = LockfileToHoistedDepGraphOptions { - lockfile_dir: workspace_root.to_path_buf(), - auto_install_peers: config.auto_install_peers, - skipped: walker_skipped.clone(), - force: false, - // Pacquet's [`Config`] does not yet expose - // `engineStrict` (tracked separately); default to - // `false` so the walker matches - // `compute_skipped_snapshots` upthread, which uses - // [`InstallabilityHost::detect`]'s `false` default. - // Promotes engine mismatches to skip-optional rather - // than hard errors, in line with pacquet's - // production posture. - engine_strict: false, - current_node_version: host_for_walker - .map(|(_, ver)| ver.clone()) - .unwrap_or_default(), - current_os: pacquet_graph_hasher::host_platform().to_string(), - current_cpu: pacquet_graph_hasher::host_arch().to_string(), - current_libc: pacquet_graph_hasher::host_libc().to_string(), - supported_architectures: supported_architectures.cloned(), - hoist_workspace_packages: config.hoist_workspace_packages, - hoisting_limits: config.hoisting_limits.clone(), - external_dependencies: config.external_dependencies.clone(), - }; - let walker_result = - lockfile_to_hoisted_dep_graph(lockfile, current_lockfile, &walker_opts) - .map_err(InstallFrozenLockfileError::HoistedDepGraph)?; - // Augment the live skip set with the walker's *new* - // skips only — entries already in `walker_skipped` came - // from the input `SkippedSnapshots`, where each one - // already lives in its proper subset - // (installability / fetch-failed / optional-excluded). - // Re-inserting them as installability would promote - // transient `fetch_failed` / `optional_excluded` - // entries into the persisted-on-disk - // `.modules.yaml.skipped` set, which would survive into - // the next install — exactly the contract those - // subsets exist to prevent. Diffing against the input - // set keeps the persistence boundary intact: only - // walker-discovered installability skips (optional + - // unsupported platform) flow into - // [`SkippedSnapshots::insert_installability`]. - for skipped_dep_path in walker_result.skipped.difference(&walker_skipped) { - if let Ok(key) = skipped_dep_path.parse::() { - skipped.insert_installability(key); - } - } - // Empty CAS index → linker would refuse every - // non-optional node. Only happens when the install - // has no snapshots, in which case the linker is a no-op. - let cas_index = - cas_paths_by_pkg_id.expect("hoisted CreateVirtualStore populates cas_paths"); - let link_opts = LinkHoistedModulesOpts { - graph: &walker_result.graph, - prev_graph: walker_result.prev_graph.as_ref(), - hierarchy: &walker_result.hierarchy, - cas_paths_by_pkg_id: &cas_index, - import_method: config.package_import_method, - logged_methods, - requester, - }; - link_hoisted_modules::(&link_opts) - .map_err(InstallFrozenLockfileError::LinkHoistedModules)?; - // Workspace `link:` deps still need symlinks under - // each importer's `node_modules/` even though - // the regular deps now live as real directories. The - // hoisted dep-graph walker skips `workspace:`-prefixed - // references entirely (they're not in the hoist tree), - // so without this pass workspace siblings would be - // missing from each project's `node_modules/`. - // `link_only: true` filters every other dep out so - // the call doesn't try to re-create symlinks for - // packages that the hoisted linker already wrote as - // real dirs. Mirrors upstream's hoisted branch at - // [`installing/deps-restorer/src/index.ts:411-440`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/src/index.ts#L411-L440). - SymlinkDirectDependencies { - config, - layout: &layout, - importers, - dependency_groups: dependency_groups.iter().copied(), - workspace_root, - skipped: &skipped, - link_only: true, - // Hoisted-linker path has no public-hoist virtual store - // to dedupe against; the real-directory tree is the - // hoist layout. - public_hoist_targets: None, - } - .run::() - .map_err(InstallFrozenLockfileError::SymlinkDirectDependencies)?; - // Map snapshot key → first recorded directory. The - // walker can emit multiple [`crate::DependenciesGraphNode`]s - // with the same `dep_path` when the package nests under - // a sibling (version conflict). Postinstall scripts and - // the side-effects-cache key both depend only on the - // package contents (identical across locations), so - // running once at the first dir matches upstream's - // `pkgRoots[0]` pick at - // [`after-install:348`](https://github.com/pnpm/pnpm/blob/94240bc046/building/after-install/src/index.ts#L348). - let mut pkg_root_by_key: HashMap = HashMap::new(); - for node in walker_result.graph.values() { - if let Ok(key) = node.dep_path.as_str().parse::() { - pkg_root_by_key.entry(key).or_insert_with(|| node.dir.clone()); - } - } - HoistedLinkerOutput { - hoisted_locations: walker_result.hoisted_locations, - hoisted_pkg_root_by_key: Some(pkg_root_by_key), - } + run_hoisted_linker::( + HoistedLinkerInputs { + config, + lockfile, + current_lockfile, + layout: &layout, + importers, + dependency_groups: &dependency_groups, + walker_lockfile_dir: workspace_root, + symlink_workspace_root: workspace_root, + host_node: host_node.as_ref(), + supported_architectures, + cas_paths_by_pkg_id, + logged_methods, + requester, + }, + &mut skipped, + ) + .map_err(InstallFrozenLockfileError::from)? } else { HoistedLinkerOutput::default() }; @@ -1190,16 +1090,224 @@ pub struct InstallFrozenLockfileOutput { /// `clippy::type_complexity`. Always [`Default`]-empty for the /// isolated linker. #[derive(Debug, Default)] -struct HoistedLinkerOutput { +pub(crate) struct HoistedLinkerOutput { /// `LockfileToDepGraphResult::hoisted_locations` from the slice /// 4 walker. Persisted into `.modules.yaml.hoisted_locations` /// when non-empty. - hoisted_locations: BTreeMap>, + pub(crate) hoisted_locations: BTreeMap>, /// Per-snapshot `pkgRoot` override for the build phase — /// snapshot key → its first recorded directory in the hoisted /// graph. `None` for the isolated linker (the layout-based /// lookup in `BuildModules` is used instead). - hoisted_pkg_root_by_key: Option>, + pub(crate) hoisted_pkg_root_by_key: Option>, +} + +/// Inputs to [`run_hoisted_linker`]. Bundled so the two install +/// paths (`InstallFrozenLockfile` and `InstallWithFreshLockfile`) +/// can feed the shared hoisted-linker materialization without a +/// long positional argument list. The frozen path passes the +/// loaded `pnpm-lock.yaml`; the fresh path passes the freshly-built +/// lockfile and `current_lockfile: None`. +pub(crate) struct HoistedLinkerInputs<'a> { + pub(crate) config: &'static Config, + /// Lockfile the walker reads `snapshots:` / `packages:` / + /// `importers:` from. `&built_lockfile` on the fresh path, + /// the loaded wanted lockfile on the frozen path. + pub(crate) lockfile: &'a Lockfile, + /// Previous install's `/lock.yaml`, used by the + /// walker to diff orphans. `None` on the fresh path (no analogue + /// yet). + pub(crate) current_lockfile: Option<&'a Lockfile>, + pub(crate) layout: &'a VirtualStoreLayout, + pub(crate) importers: &'a HashMap, + pub(crate) dependency_groups: &'a [DependencyGroup], + /// Lockfile root the walker resolves hoisted directories against. + pub(crate) walker_lockfile_dir: &'a Path, + /// Anchor for [`crate::SymlinkDirectDependencies`]'s per-importer + /// `node_modules` lookup. Equals `walker_lockfile_dir` on the + /// frozen path; the fresh path passes `config.modules_dir.parent()` + /// so relocated `modules_dir` test configs land symlinks where the + /// rest of the install writes. + pub(crate) symlink_workspace_root: &'a Path, + /// `(node_detected, node_version)` from the installability host + /// probe. `None` when no installability check ran (the fresh + /// path, and constraint-free frozen lockfiles). + pub(crate) host_node: Option<&'a (bool, String)>, + pub(crate) supported_architectures: + Option<&'a pacquet_package_is_installable::SupportedArchitectures>, + /// Per-package CAS index produced by [`crate::CreateVirtualStore`] + /// under `node_linker == Hoisted`. The linker imports files from + /// these paths into the on-disk hoisted tree. + pub(crate) cas_paths_by_pkg_id: Option, + pub(crate) logged_methods: &'a AtomicU8, + pub(crate) requester: &'a str, +} + +/// Error type of [`run_hoisted_linker`]. Each install path maps these +/// back onto its own error enum's matching variant so the user-facing +/// error code is identical regardless of which path drove the hoist. +#[derive(Debug, Display, Error, Diagnostic)] +pub(crate) enum HoistedLinkerError { + #[diagnostic(transparent)] + HoistedDepGraph(#[error(source)] HoistedDepGraphError), + #[diagnostic(transparent)] + LinkHoistedModules(#[error(source)] LinkHoistedModulesError), + #[diagnostic(transparent)] + SymlinkDirectDependencies(#[error(source)] SymlinkDirectDependenciesError), +} + +impl From for InstallFrozenLockfileError { + fn from(error: HoistedLinkerError) -> Self { + match error { + HoistedLinkerError::HoistedDepGraph(error) => { + InstallFrozenLockfileError::HoistedDepGraph(error) + } + HoistedLinkerError::LinkHoistedModules(error) => { + InstallFrozenLockfileError::LinkHoistedModules(error) + } + HoistedLinkerError::SymlinkDirectDependencies(error) => { + InstallFrozenLockfileError::SymlinkDirectDependencies(error) + } + } + } +} + +/// Materialize the `nodeLinker: hoisted` on-disk tree from a lockfile. +/// +/// Runs the [`crate::lockfile_to_hoisted_dep_graph`] walker over the +/// lockfile's snapshots, materializes the resulting graph with +/// [`crate::link_hoisted_modules()`] (real directories under each +/// importer's tree, fed from `cas_paths_by_pkg_id`), then layers +/// [`crate::SymlinkDirectDependencies`] with `link_only: true` to wire +/// `workspace:` / `link:` deps the hoist walker skips. Folds the +/// walker's newly-discovered installability skips into `skipped`. +/// +/// Shared by both install paths so the hoisted layout, skip-set +/// accounting, and `pkg_root_by_key` derivation stay identical. +/// Mirrors upstream's hoisted branch at +/// [`installing/deps-restorer/src/index.ts:369-440`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/src/index.ts#L369-L440). +pub(crate) fn run_hoisted_linker( + inputs: HoistedLinkerInputs<'_>, + skipped: &mut SkippedSnapshots, +) -> Result { + let HoistedLinkerInputs { + config, + lockfile, + current_lockfile, + layout, + importers, + dependency_groups, + walker_lockfile_dir, + symlink_workspace_root, + host_node, + supported_architectures, + cas_paths_by_pkg_id, + logged_methods, + requester, + } = inputs; + + // Walker installability inputs come straight from the optional + // `host_node` the caller built for the `compute_skipped_snapshots` + // pass. When `host_node` is `None` no per-snapshot constraint + // exists, so the host triple values pass through as defaults the + // walker won't actually consult. + let walker_skipped: BTreeSet = skipped.iter().map(|key| key.to_string()).collect(); + let walker_opts = LockfileToHoistedDepGraphOptions { + lockfile_dir: walker_lockfile_dir.to_path_buf(), + auto_install_peers: config.auto_install_peers, + skipped: walker_skipped.clone(), + force: false, + // Pacquet's [`Config`] does not yet expose `engineStrict` + // (tracked separately); default to `false` so the walker + // matches `compute_skipped_snapshots` upthread, which uses + // [`crate::InstallabilityHost::detect`]'s `false` default. + // Promotes engine mismatches to skip-optional rather than + // hard errors, in line with pacquet's production posture. + engine_strict: false, + current_node_version: host_node.map(|(_, ver)| ver.clone()).unwrap_or_default(), + current_os: pacquet_graph_hasher::host_platform().to_string(), + current_cpu: pacquet_graph_hasher::host_arch().to_string(), + current_libc: pacquet_graph_hasher::host_libc().to_string(), + supported_architectures: supported_architectures.cloned(), + hoist_workspace_packages: config.hoist_workspace_packages, + hoisting_limits: crate::get_hoisting_limits(&lockfile.importers, config.hoisting_limits), + external_dependencies: config.external_dependencies.clone(), + }; + let walker_result = lockfile_to_hoisted_dep_graph(lockfile, current_lockfile, &walker_opts) + .map_err(HoistedLinkerError::HoistedDepGraph)?; + // Augment the live skip set with the walker's *new* skips only — + // entries already in `walker_skipped` came from the input + // `SkippedSnapshots`, where each one already lives in its proper + // subset (installability / fetch-failed / optional-excluded). + // Re-inserting them as installability would promote transient + // `fetch_failed` / `optional_excluded` entries into the + // persisted-on-disk `.modules.yaml.skipped` set, which would + // survive into the next install — exactly the contract those + // subsets exist to prevent. Diffing against the input set keeps + // the persistence boundary intact: only walker-discovered + // installability skips (optional + unsupported platform) flow + // into [`SkippedSnapshots::insert_installability`]. + for skipped_dep_path in walker_result.skipped.difference(&walker_skipped) { + if let Ok(key) = skipped_dep_path.parse::() { + skipped.insert_installability(key); + } + } + // Empty CAS index → linker would refuse every non-optional node. + // Only happens when the install has no snapshots, in which case + // the linker is a no-op. + let cas_index = cas_paths_by_pkg_id.expect("hoisted CreateVirtualStore populates cas_paths"); + let link_opts = LinkHoistedModulesOpts { + graph: &walker_result.graph, + prev_graph: walker_result.prev_graph.as_ref(), + hierarchy: &walker_result.hierarchy, + cas_paths_by_pkg_id: &cas_index, + import_method: config.package_import_method, + logged_methods, + requester, + }; + link_hoisted_modules::(&link_opts).map_err(HoistedLinkerError::LinkHoistedModules)?; + // Workspace `link:` deps still need symlinks under each importer's + // `node_modules/` even though the regular deps now live as + // real directories. The hoisted dep-graph walker skips + // `workspace:`-prefixed references entirely (they're not in the + // hoist tree), so without this pass workspace siblings would be + // missing from each project's `node_modules/`. `link_only: true` + // filters every other dep out so the call doesn't try to re-create + // symlinks for packages that the hoisted linker already wrote as + // real dirs. Mirrors upstream's hoisted branch at + // [`installing/deps-restorer/src/index.ts:411-440`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/src/index.ts#L411-L440). + SymlinkDirectDependencies { + config, + layout, + importers, + dependency_groups: dependency_groups.iter().copied(), + workspace_root: symlink_workspace_root, + skipped: &*skipped, + link_only: true, + // Hoisted-linker path has no public-hoist virtual store to + // dedupe against; the real-directory tree is the hoist layout. + public_hoist_targets: None, + } + .run::() + .map_err(HoistedLinkerError::SymlinkDirectDependencies)?; + // Map snapshot key → first recorded directory. The walker can emit + // multiple [`crate::DependenciesGraphNode`]s with the same + // `dep_path` when the package nests under a sibling (version + // conflict). Postinstall scripts and the side-effects-cache key + // both depend only on the package contents (identical across + // locations), so running once at the first dir matches upstream's + // `pkgRoots[0]` pick at + // [`after-install:348`](https://github.com/pnpm/pnpm/blob/94240bc046/building/after-install/src/index.ts#L348). + let mut pkg_root_by_key: HashMap = HashMap::new(); + for node in walker_result.graph.values() { + if let Ok(key) = node.dep_path.as_str().parse::() { + pkg_root_by_key.entry(key).or_insert_with(|| node.dir.clone()); + } + } + Ok(HoistedLinkerOutput { + hoisted_locations: walker_result.hoisted_locations, + hoisted_pkg_root_by_key: Some(pkg_root_by_key), + }) } /// Pre-computed hoist plan threaded across the install pipeline so diff --git a/pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs b/pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs index ecba6265f2..69fe63ba53 100644 --- a/pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs +++ b/pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs @@ -145,6 +145,18 @@ pub struct InstallWithFreshLockfile<'a, DependencyGroupList> { /// from it to skip duplicate fetches when both touch the same /// `(registry, name)`. pub meta_cache: Arc, + /// Resolved [`pacquet_config::Config::node_linker`]. Selects the + /// materialization shape after the virtual store is populated: + /// under [`NodeLinker::Hoisted`] the freshly-built lockfile is + /// routed through [`crate::lockfile_to_hoisted_dep_graph`] + + /// [`crate::link_hoisted_modules()`] instead of the isolated + /// symlink layout. + pub node_linker: NodeLinker, + /// CLI-merged `supportedArchitectures` (`pnpm-workspace.yaml` + + /// `--cpu`/`--os`/`--libc`). Threaded into the hoisted-linker + /// walker so its installability filter honors user-supplied + /// accept lists. `None` when no architectures are configured. + pub supported_architectures: Option<&'a pacquet_package_is_installable::SupportedArchitectures>, } /// Error type of [`InstallWithFreshLockfile`]. @@ -159,6 +171,20 @@ pub enum InstallWithFreshLockfileError { #[diagnostic(transparent)] SymlinkDirectDependencies(#[error(source)] SymlinkDirectDependenciesError), + /// Surfaces failures from [`crate::lockfile_to_hoisted_dep_graph`] + /// when a fresh install runs under `nodeLinker: hoisted`. Same + /// shape the frozen-lockfile path surfaces — see + /// `InstallFrozenLockfileError::HoistedDepGraph`. + #[diagnostic(transparent)] + HoistedDepGraph(#[error(source)] crate::HoistedDepGraphError), + + /// Surfaces failures from [`crate::link_hoisted_modules()`] while + /// materializing the on-disk hoisted tree on the fresh path. Same + /// shape the frozen-lockfile path surfaces — see + /// `InstallFrozenLockfileError::LinkHoistedModules`. + #[diagnostic(transparent)] + LinkHoistedModules(#[error(source)] crate::LinkHoistedModulesError), + #[diagnostic(transparent)] LinkBins(#[error(source)] LinkBinsError), @@ -253,6 +279,23 @@ pub enum InstallWithFreshLockfileError { SaveWantedLockfile(#[error(source)] SaveLockfileError), } +impl From for InstallWithFreshLockfileError { + fn from(error: crate::install_frozen_lockfile::HoistedLinkerError) -> Self { + use crate::install_frozen_lockfile::HoistedLinkerError; + match error { + HoistedLinkerError::HoistedDepGraph(error) => { + InstallWithFreshLockfileError::HoistedDepGraph(error) + } + HoistedLinkerError::LinkHoistedModules(error) => { + InstallWithFreshLockfileError::LinkHoistedModules(error) + } + HoistedLinkerError::SymlinkDirectDependencies(error) => { + InstallWithFreshLockfileError::SymlinkDirectDependencies(error) + } + } + } +} + /// Output of [`InstallWithFreshLockfile::run`]. /// /// Returns the hoist-graph slot the dispatch already consumed plus the @@ -264,6 +307,14 @@ pub enum InstallWithFreshLockfileError { #[must_use] pub struct InstallWithFreshLockfileResult { pub hoisted_dependencies: HoistedDependencies, + /// Per-depPath list of lockfile-relative directory paths the + /// hoisted linker placed each package at. Empty under the + /// isolated linker (the field is hoisted-only on disk). The + /// caller persists it into + /// [`pacquet_modules_yaml::Modules::hoisted_locations`] so a + /// follow-up install or rebuild can locate every package without + /// re-running the walker. + pub hoisted_locations: BTreeMap>, /// `Some` when the install resolved a graph that was written to /// `pnpm-lock.yaml`; `None` when the write was skipped (today: only /// `config.lockfile=false`). The caller mirrors the same gate when @@ -274,13 +325,11 @@ pub struct InstallWithFreshLockfileResult { impl<'a, DependencyGroupList> InstallWithFreshLockfile<'a, DependencyGroupList> { /// Execute the subroutine. /// - /// The fresh-lockfile path's [`HoistedDependencies`] slot is always - /// empty. Hoisting needs the resolved snapshot graph the lockfile - /// carries; this path serializes the graph into `pnpm-lock.yaml` - /// itself, but the hoist pass still runs only inside the - /// frozen-lockfile install ([`crate::InstallFrozenLockfile::run`]). - /// The signature symmetry keeps `Install::run` from branching on - /// which sub-path produced the result. + /// Under the isolated linker the [`HoistedDependencies`] result + /// carries the publicly/privately-hoisted alias map; under + /// `nodeLinker: hoisted` it is empty (the hoisted linker writes the + /// on-disk tree directly and reports its placements through + /// [`InstallWithFreshLockfileResult::hoisted_locations`] instead). pub async fn run( self, ) -> Result @@ -314,7 +363,10 @@ impl<'a, DependencyGroupList> InstallWithFreshLockfile<'a, DependencyGroupList> update_checksums, wanted_lockfile, meta_cache, + node_linker, + supported_architectures, } = self; + let is_hoisted = matches!(node_linker, NodeLinker::Hoisted); // Materialise the caller's iterator into a `Vec` so the same // group set can be replayed into both the resolver (consumes // the iterator) and `SymlinkDirectDependencies` (needs to walk @@ -826,15 +878,21 @@ impl<'a, DependencyGroupList> InstallWithFreshLockfile<'a, DependencyGroupList> // The fresh-lockfile path has no installability check yet // (the resolver's `PackageVersion` deserializer doesn't carry // engine / cpu / os / libc constraints to gate on), so the - // skip set is empty. A future port of `compute_skipped_snapshots` - // for fresh-lockfile would route through here too. - let empty_skipped = SkippedSnapshots::new(); + // skip set starts empty. A future port of + // `compute_skipped_snapshots` for fresh-lockfile would route + // through here too. Under `nodeLinker: hoisted` the + // hoisted-linker walker may fold its own installability skips + // into this set, so it is `mut`. + let mut skipped = SkippedSnapshots::new(); let phase_start = std::time::Instant::now(); let CreateVirtualStoreOutput { package_manifests, side_effects_maps_by_snapshot: _, fetch_failed: _, - cas_paths_by_pkg_id: _, + // Populated only under `node_linker == Hoisted`; consumed by + // the hoisted-linker pass below to materialize the on-disk + // tree. `None` for the isolated linker. + cas_paths_by_pkg_id, } = CreateVirtualStore { http_client, config, @@ -847,9 +905,9 @@ impl<'a, DependencyGroupList> InstallWithFreshLockfile<'a, DependencyGroupList> requester, store_index_writer: &store_index_writer, allow_build_policy: &allow_build_policy, - skipped: &empty_skipped, + skipped: &skipped, workspace_root: lockfile_dir, - node_linker: NodeLinker::Isolated, + node_linker, } .run::() .await @@ -904,22 +962,63 @@ impl<'a, DependencyGroupList> InstallWithFreshLockfile<'a, DependencyGroupList> // writes. let symlink_root: &Path = config.modules_dir.parent().unwrap_or(lockfile_dir); - // Pre-compute the hoist plan so the dedupe pass in - // `SymlinkDirectDependencies` can fold publicly-hoisted - // aliases into root's target map — same shape as the - // frozen-lockfile path. The `HoistResult` is reused for - // the on-disk hoist phase below, so the BFS runs once. - let pre_hoist = crate::install_frozen_lockfile::compute_hoist_plan( - config, - built_lockfile.snapshots.as_ref(), - built_lockfile.packages.as_ref(), - &built_lockfile.importers, - &dependency_groups, - &empty_skipped, - false, - ); - let public_hoist_targets: Option> = - pre_hoist.as_ref().map(|plan| { + // Under `nodeLinker: hoisted` the regular deps live as real + // directories materialized by the hoisted linker, not as + // symlinks into the virtual store. Route through the same + // walker + linker + `link_only` symlink pass the frozen path + // uses (shared via `run_hoisted_linker`), then skip the + // isolated-linker public/private hoist and `LinkVirtualStoreBins` + // passes entirely — the hoisted linker writes per-`node_modules` + // bins while walking the hierarchy. `hoisted_dependencies` stays + // empty (the hoisted linker has no isolated-mode alias→kind + // adapter shape); `hoisted_locations` carries the walker's + // placements so `.modules.yaml` round-trips them. + let (hoisted_dependencies, hoisted_locations) = if is_hoisted { + let output = crate::install_frozen_lockfile::run_hoisted_linker::( + crate::install_frozen_lockfile::HoistedLinkerInputs { + config, + lockfile: &built_lockfile, + // No previous-install `/lock.yaml` + // is threaded into the fresh path yet (#11871), so the + // walker runs without an orphan diff. + current_lockfile: None, + layout: &layout, + importers: &built_lockfile.importers, + dependency_groups: &dependency_groups, + walker_lockfile_dir: lockfile_dir, + symlink_workspace_root: symlink_root, + // No installability host was probed on the fresh + // path (the resolver's `PackageVersion` carries no + // engine/cpu/os/libc constraints), so the walker + // falls back to its default host triple. + host_node: None, + supported_architectures, + cas_paths_by_pkg_id, + logged_methods, + requester, + }, + &mut skipped, + ) + .map_err(InstallWithFreshLockfileError::from)?; + (HoistedDependencies::new(), output.hoisted_locations) + } else { + // Pre-compute the hoist plan so the dedupe pass in + // `SymlinkDirectDependencies` can fold publicly-hoisted + // aliases into root's target map — same shape as the + // frozen-lockfile path. The `HoistResult` is reused for + // the on-disk hoist phase below, so the BFS runs once. + let pre_hoist = crate::install_frozen_lockfile::compute_hoist_plan( + config, + built_lockfile.snapshots.as_ref(), + built_lockfile.packages.as_ref(), + &built_lockfile.importers, + &dependency_groups, + &skipped, + false, + ); + let public_hoist_targets: Option< + std::collections::BTreeMap, + > = pre_hoist.as_ref().map(|plan| { crate::install_frozen_lockfile::collect_public_hoist_targets( &plan.result, &plan.graph, @@ -928,106 +1027,112 @@ impl<'a, DependencyGroupList> InstallWithFreshLockfile<'a, DependencyGroupList> ) }); - SymlinkDirectDependencies { - config, - layout: &layout, - importers: &built_lockfile.importers, - dependency_groups: dependency_groups.iter().copied(), - workspace_root: symlink_root, - skipped: &empty_skipped, - link_only: false, - public_hoist_targets: public_hoist_targets.as_ref(), - } - .run::() - .map_err(InstallWithFreshLockfileError::SymlinkDirectDependencies)?; + SymlinkDirectDependencies { + config, + layout: &layout, + importers: &built_lockfile.importers, + dependency_groups: dependency_groups.iter().copied(), + workspace_root: symlink_root, + skipped: &skipped, + link_only: false, + public_hoist_targets: public_hoist_targets.as_ref(), + } + .run::() + .map_err(InstallWithFreshLockfileError::SymlinkDirectDependencies)?; - // On-disk hoist phase. Mirrors the frozen-install block at - // `install_frozen_lockfile.rs`: symlink the publicly + privately - // hoisted aliases into their target dirs, then link private-side - // bins into `/node_modules/.bin`. Public-side bin precedence - // is handled implicitly by the per-importer `link_bins` pass below, - // which now walks both direct-dep and public-hoist symlinks in - // root's `node_modules/`. - let hoisted_dependencies = if let Some(plan) = pre_hoist { - let crate::install_frozen_lockfile::HoistPlan { - graph, - result, - skipped: hoist_skipped, - .. - } = plan; - let private_dir = config.virtual_store_dir.join("node_modules"); - let public_dir = config.modules_dir.clone(); - crate::symlink_hoisted_dependencies( - &result.hoisted_dependencies_by_node_id, - &graph, - &layout, - &private_dir, - &public_dir, - &hoist_skipped, - ) - .map_err(InstallWithFreshLockfileError::HoistSymlink)?; - crate::link_direct_dep_bins(&private_dir, &result.hoisted_aliases_with_bins) - .map_err(InstallWithFreshLockfileError::HoistLinkBins)?; - result.hoisted_dependencies - } else { - HoistedDependencies::new() + // On-disk hoist phase. Mirrors the frozen-install block at + // `install_frozen_lockfile.rs`: symlink the publicly + + // privately hoisted aliases into their target dirs, then + // link private-side bins into `/node_modules/.bin`. + // Public-side bin precedence is handled implicitly by the + // per-importer `link_bins` pass below, which now walks both + // direct-dep and public-hoist symlinks in root's + // `node_modules/`. + let hoisted_dependencies = if let Some(plan) = pre_hoist { + let crate::install_frozen_lockfile::HoistPlan { + graph, + result, + skipped: hoist_skipped, + .. + } = plan; + let private_dir = config.virtual_store_dir.join("node_modules"); + let public_dir = config.modules_dir.clone(); + crate::symlink_hoisted_dependencies( + &result.hoisted_dependencies_by_node_id, + &graph, + &layout, + &private_dir, + &public_dir, + &hoist_skipped, + ) + .map_err(InstallWithFreshLockfileError::HoistSymlink)?; + crate::link_direct_dep_bins(&private_dir, &result.hoisted_aliases_with_bins) + .map_err(InstallWithFreshLockfileError::HoistLinkBins)?; + result.hoisted_dependencies + } else { + HoistedDependencies::new() + }; + + // Link bins. Direct dependencies first (each importer's + // `node_modules/.bin`) and then per-slot children inside the + // virtual store. Mirrors the same two-call shape as + // `install_frozen_lockfile.rs`. We re-walk `` + // instead of replaying the manifest because the + // `dependency_groups` iterator was already consumed above; + // pnpm's own `linkBins(modulesDir, binsDir)` overload uses + // the same strategy. One pass per importer so sibling + // workspace projects get their own `.bin/` populated, + // mirroring upstream's per-importer `linkBinsOfImporter` at + // . + let modules_basename = config + .modules_dir + .file_name() + .map(std::ffi::OsStr::to_os_string) + .unwrap_or_else(|| std::ffi::OsString::from("node_modules")); + for importer_id in importer_manifests.keys() { + let project_dir = crate::symlink_direct_dependencies::importer_root_dir( + symlink_root, + importer_id, + ); + let modules_dir = project_dir.join(&modules_basename); + let bins_dir = modules_dir.join(".bin"); + link_bins::(&modules_dir, &bins_dir) + .map_err(InstallWithFreshLockfileError::LinkBins)?; + } + + // Drive the lockfile-driven `LinkVirtualStoreBins` path. The + // bin linker iterates `snapshots:` (no per-slot `read_dir`) + // and reads each child's manifest from `package_manifests` + // (no per-child `package.json` disk read on warm hits). + // `package_manifests` is now produced by `CreateVirtualStore` + // directly — its prefetch + cold-batch passes both feed into + // the same map. + // + // `packages: None` on purpose: the freshly-built lockfile's + // `packages:` rows carry an incomplete `has_bin` because the + // resolver's `PackageVersion` deserializer does not include + // the `bin` field. Trusting the empty-by-omission + // `has_bin_set` here would filter out every child and skip + // bin linking entirely. With `packages: None` the bin linker + // falls through to "process every child" and lets each + // child's actual manifest (`bin` present or not) decide. + // Threading `bin` through `PackageVersion` is the proper + // fix; once that lands, pass + // `built_lockfile.packages.as_ref()` here to recover the + // ~95% slot short-circuit the frozen path enjoys. + LinkVirtualStoreBins { + layout: &layout, + snapshots: built_lockfile.snapshots.as_ref(), + packages: None, + package_manifests: &package_manifests, + skipped: &skipped, + } + .run() + .map_err(InstallWithFreshLockfileError::LinkVirtualStoreBins)?; + + (hoisted_dependencies, BTreeMap::new()) }; - // Link bins. Direct dependencies first (each importer's - // `node_modules/.bin`) and then per-slot children inside the - // virtual store. Mirrors the same two-call shape as - // `install_frozen_lockfile.rs`. We re-walk `` instead - // of replaying the manifest because the `dependency_groups` - // iterator was already consumed above; pnpm's own - // `linkBins(modulesDir, binsDir)` overload uses the same - // strategy. One pass per importer so sibling workspace projects - // get their own `.bin/` populated, mirroring upstream's - // per-importer `linkBinsOfImporter` at - // . - let modules_basename = config - .modules_dir - .file_name() - .map(std::ffi::OsStr::to_os_string) - .unwrap_or_else(|| std::ffi::OsString::from("node_modules")); - for importer_id in importer_manifests.keys() { - let project_dir = - crate::symlink_direct_dependencies::importer_root_dir(symlink_root, importer_id); - let modules_dir = project_dir.join(&modules_basename); - let bins_dir = modules_dir.join(".bin"); - link_bins::(&modules_dir, &bins_dir) - .map_err(InstallWithFreshLockfileError::LinkBins)?; - } - - // Drive the lockfile-driven `LinkVirtualStoreBins` path. The - // bin linker iterates `snapshots:` (no per-slot `read_dir`) - // and reads each child's manifest from `package_manifests` - // (no per-child `package.json` disk read on warm hits). - // `package_manifests` is now produced by `CreateVirtualStore` - // directly — its prefetch + cold-batch passes both feed into - // the same map. - // - // `packages: None` on purpose: the freshly-built lockfile's - // `packages:` rows carry an incomplete `has_bin` because the - // resolver's `PackageVersion` deserializer does not include - // the `bin` field. Trusting the empty-by-omission - // `has_bin_set` here would filter out every child and skip - // bin linking entirely. With `packages: None` the bin linker - // falls through to "process every child" and lets each - // child's actual manifest (`bin` present or not) decide. - // Threading `bin` through `PackageVersion` is the proper - // fix; once that lands, pass - // `built_lockfile.packages.as_ref()` here to recover the - // ~95% slot short-circuit the frozen path enjoys. - LinkVirtualStoreBins { - layout: &layout, - snapshots: built_lockfile.snapshots.as_ref(), - packages: None, - package_manifests: &package_manifests, - skipped: &empty_skipped, - } - .run() - .map_err(InstallWithFreshLockfileError::LinkVirtualStoreBins)?; - // Write `pnpm-lock.yaml` from the resolved graph. Mirrors // upstream's // [`writeLockfiles`](https://github.com/pnpm/pnpm/blob/094aa6e57b/lockfile/fs/src/write.ts#L133) @@ -1068,7 +1173,11 @@ impl<'a, DependencyGroupList> InstallWithFreshLockfile<'a, DependencyGroupList> stage: Stage::ImportingDone, })); - Ok(InstallWithFreshLockfileResult { hoisted_dependencies, wanted_lockfile }) + Ok(InstallWithFreshLockfileResult { + hoisted_dependencies, + hoisted_locations, + wanted_lockfile, + }) } } diff --git a/pacquet/crates/package-manager/src/lib.rs b/pacquet/crates/package-manager/src/lib.rs index 9dde869d31..8003697a0d 100644 --- a/pacquet/crates/package-manager/src/lib.rs +++ b/pacquet/crates/package-manager/src/lib.rs @@ -12,6 +12,7 @@ mod deps_graph; mod graph_sequencer; mod hoist; mod hoisted_dep_graph; +mod hoisting_limits; mod import_indexed_dir; mod install; pub(crate) mod install_frozen_lockfile; @@ -47,6 +48,7 @@ pub use deps_graph::*; pub use graph_sequencer::*; pub use hoist::*; pub use hoisted_dep_graph::*; +pub use hoisting_limits::*; pub use import_indexed_dir::*; pub use install::*; pub use install_frozen_lockfile::*; diff --git a/pacquet/crates/real-hoist/src/lib.rs b/pacquet/crates/real-hoist/src/lib.rs index acb75537e4..fc3fdc87fa 100644 --- a/pacquet/crates/real-hoist/src/lib.rs +++ b/pacquet/crates/real-hoist/src/lib.rs @@ -515,15 +515,17 @@ fn collect_snapshot_deps( Ok(()) } -/// Encode an importer id for use as a child node's `name`. Upstream -/// uses `encodeURIComponent`, which percent-encodes everything -/// except `A-Z a-z 0-9 - _ . ! ~ * ' ( )`. Pacquet workspace -/// importers are filesystem-relative paths, so the common case is -/// alphanumeric + `/` + `-` + `_`. Encode `/` (since it would -/// confuse `node_modules` directory parsing) and pass the rest -/// through; if a richer set ever shows up the function can switch -/// to a full encoder without touching call sites. -fn percent_encode_path(text: &str) -> String { +/// Encode an importer id for use as a child node's `name` (and in +/// the hoisting-limits locator keys built by +/// `pacquet_package_manager::get_hoisting_limits`). Upstream uses +/// `encodeURIComponent`, which percent-encodes everything except +/// `A-Z a-z 0-9 - _ . ! ~ * ' ( )`. Pacquet workspace importers are +/// filesystem-relative paths, so the common case is alphanumeric + +/// `/` + `-` + `_`. Encode `/` (since it would confuse +/// `node_modules` directory parsing) and pass the rest through; if a +/// richer set ever shows up the function can switch to a full +/// encoder without touching call sites. +pub fn percent_encode_path(text: &str) -> String { let mut out = String::with_capacity(text.len()); for ch in text.chars() { match ch { @@ -593,8 +595,9 @@ fn percent_encode_path(text: &str) -> String { /// What this models today (continued): /// /// * `hoistingLimits` borders. Names in -/// `opts.hoisting_limits[root_locator]` are kept out of the -/// root's `node_modules` (`AbsorbDecision::Border`). Mirrors +/// `opts.hoisting_limits[root_locator]` mark hoisting borders: a +/// bordered node's descendants stay nested beneath it rather than +/// hoisting to the root (`AbsorbDecision::Border`). Mirrors /// upstream's `isHoistBorder` flag. /// * `externalDependencies` placeholders — the wrapper adds them /// as zero-children `ExternalSoftLink` nodes at the root, and @@ -608,10 +611,12 @@ fn percent_encode_path(text: &str) -> String { /// alias, pacquet picks the first-visited; upstream picks the /// one with more incoming references. Outcome differs for the /// handful of upstream test cases that exercise the tie-break. -/// * Multi-importer (workspace) hoist trees — pacquet's wrapper -/// refuses lockfiles with non-root importers upfront via -/// `UnsupportedWorkspace`. Workspace-aware hoisting requires -/// per-importer roots and a different output shape. +/// * Per-importer roots and the multi-level output shape upstream +/// produces for workspaces. [`hoist`] does attach every non-root +/// importer as a `Workspace`-kind child of the virtual `.` root +/// when [`HoistOpts::hoist_workspace_packages`] is enabled, but the +/// algorithm still hoists into that single `.` root rather than +/// giving each importer its own hoisting root. /// * `ExternalSoftLink` descendants — pacquet creates soft-links /// only as zero-children placeholders, so upstream's /// "only-hoist-when-all-descendants-hoist" rule has nothing to @@ -666,12 +671,14 @@ enum AbsorbDecision { /// [peer-shadow-root]: https://github.com/yarnpkg/berry/blob/4287909fa6a0a1ec976a55776bff606864b31990/packages/yarnpkg-nm/sources/hoist.ts#L414 /// [peer-path]: https://github.com/yarnpkg/berry/blob/4287909fa6a0a1ec976a55776bff606864b31990/packages/yarnpkg-nm/sources/hoist.ts#L454-L479 PeerShadow, - /// The candidate's name is in `opts.hoisting_limits` for the - /// current root locator. The caller asked us to keep this name - /// out of the root's `node_modules`, so the candidate stays - /// nested under its parent. Mirrors upstream's `isHoistBorder` - /// flag set during `cloneTree` from - /// [`hoist.ts:707`][hoist-border]. + /// The candidate sits beneath a hoisting border — its parent (or + /// a higher ancestor) has a name listed in + /// `opts.hoisting_limits` for the root locator. A bordered node's + /// descendants stay nested beneath it rather than hoisting to the + /// root, so the candidate stays under its parent. Mirrors + /// upstream's `isHoistBorder` flag set during `cloneTree` from + /// [`hoist.ts:707`][hoist-border], which blocks a bordered node's + /// children from hoisting past it (not the bordered node itself). /// /// [hoist-border]: https://github.com/yarnpkg/berry/blob/4287909fa6a0a1ec976a55776bff606864b31990/packages/yarnpkg-nm/sources/hoist.ts#L707 Border, @@ -714,19 +721,21 @@ fn hoist_into_root(root: &Rc, root_locator: &str, opts: &HoistOpt let mut root_index: HashMap> = root.dependencies.borrow().iter().map(|dep| (dep.0.name.clone(), dep.clone())).collect(); - // Look up the names the caller asked us not to hoist to *this* - // root. Upstream stores this on each child as `isHoistBorder` - // during `cloneTree`; pacquet stays DAG-shaped and looks the - // names up by-name at decision time, which is equivalent since - // there's only one root locator. An empty fallback set means - // the check is effectively a no-op when no limits are configured. + // Look up the border names for *this* root locator: a node whose + // name is in this set is a hoisting border, so its descendants + // stay nested beneath it. Upstream stores the flag on each node + // as `isHoistBorder` during `cloneTree`; pacquet stays DAG-shaped + // and looks the names up by-name at decision time, which is + // equivalent since there's only one root locator. An empty + // fallback set means the check is a no-op when no limits are set. let empty_set: BTreeSet = BTreeSet::new(); let border_names: &BTreeSet = opts.hoisting_limits.get(root_locator).unwrap_or(&empty_set); loop { let mut visited: HashSet<*const HoisterResult> = HashSet::new(); - let changed = hoist_subtree(root, &[], root, &mut root_index, &mut visited, border_names); + let changed = + hoist_subtree(root, &[], root, &mut root_index, &mut visited, border_names, false); if !changed { break; } @@ -746,6 +755,7 @@ fn hoist_subtree( root_index: &mut HashMap>, visited: &mut HashSet<*const HoisterResult>, border_names: &BTreeSet, + under_border: bool, ) -> bool { let root_ptr = Rc::as_ptr(root); if !visited.insert(Rc::as_ptr(node)) { @@ -753,6 +763,16 @@ fn hoist_subtree( } let mut changed_in_subtree = false; + // A node whose name is in `border_names` is a hoisting border: + // its descendants are kept nested beneath it rather than hoisted + // to the root. `under_border` carries that boundary down the + // recursion — once any proper ancestor of a node is a border, + // the node (and everything below it) stays put. Mirrors + // upstream's `isHoistBorder` flag, which blocks a bordered + // node's *children* from hoisting past it, not the bordered + // node itself. + let children_blocked = under_border || border_names.contains(&node.name); + // Snapshot the current children so we can mutate // `node.dependencies` mid-iteration without invalidating the // borrow. `RcByPtr::clone` just bumps refcounts. @@ -775,19 +795,21 @@ fn hoist_subtree( continue; } - let mut decision = match root_index.get(&child.0.name) { - None => AbsorbDecision::Free, - Some(existing) if Rc::ptr_eq(&existing.0, &child.0) => AbsorbDecision::SameNode, - Some(_) => AbsorbDecision::Conflict, + // A hoisting border on this `node` (or any ancestor) keeps + // every descendant nested, so the child stays under its + // parent regardless of whether the root slot is free. Decided + // before the free/dedup/conflict lookup because the border + // wins outright. + let mut decision = if children_blocked { + AbsorbDecision::Border + } else { + match root_index.get(&child.0.name) { + None => AbsorbDecision::Free, + Some(existing) if Rc::ptr_eq(&existing.0, &child.0) => AbsorbDecision::SameNode, + Some(_) => AbsorbDecision::Conflict, + } }; - // Hoisting limits ride on top of the basic decision: even - // if the slot is free and no peer would shadow, the caller - // may have asked us to keep this name out of the root. - if matches!(decision, AbsorbDecision::Free) && border_names.contains(&child.0.name) { - decision = AbsorbDecision::Border; - } - // Peer-aware refusal layered on top of the basic // free / dedup / conflict decision. `Conflict` already // leaves the candidate in place and `SameNode` dedups @@ -833,19 +855,27 @@ fn hoist_subtree( AbsorbDecision::Conflict | AbsorbDecision::PeerShadow | AbsorbDecision::Border => { // Stays at the current parent. The version // already at root wins the slot, hoisting would - // shadow a peer dependency, or the caller's - // `hoisting_limits` blocked the name. Child's + // shadow a peer dependency, or the candidate sits + // beneath a `hoisting_limits` border. Child's // ancestor path is the path through `node`; a // later round may revisit this candidate with a - // different peer / conflict context (limits are - // fixed so the Border verdict won't change). + // different peer / conflict context (the border + // boundary is fixed so the Border verdict won't + // change). path_for_children.clone() } } }; - let child_changed = - hoist_subtree(&child.0, &child_recursion_path, root, root_index, visited, border_names); + let child_changed = hoist_subtree( + &child.0, + &child_recursion_path, + root, + root_index, + visited, + border_names, + children_blocked, + ); changed_in_subtree |= child_changed; } changed_in_subtree diff --git a/pacquet/crates/real-hoist/src/tests.rs b/pacquet/crates/real-hoist/src/tests.rs index 622c7301c9..e51a0e4748 100644 --- a/pacquet/crates/real-hoist/src/tests.rs +++ b/pacquet/crates/real-hoist/src/tests.rs @@ -757,13 +757,16 @@ fn multi_round_unlocks_peer_friendly_hoist_after_blocker_moves() { assert!(app.dependencies.borrow().is_empty(), "app stripped after multi-round: {app:#?}"); } -/// `hoisting_limits` blocks a single name from hoisting to root. -/// Ports the spirit of upstream's `should not hoist packages past -/// hoist boundary`. Setup: `root → a → b`. With no limits, `b` +/// A `hoisting_limits` border keeps a bordered node's descendants +/// nested. Ports the spirit of upstream's `should not hoist packages +/// past hoist boundary`. Setup: `root → a → b`. With no limits, `b` /// would flatten to root (see `one_transitive_dep_hoists_to_root`). -/// With `hoisting_limits[".@"] = {b}`, `b` stays under `a`. +/// With `hoisting_limits[".@"] = {a}`, `a` is a border, so its +/// descendant `b` stays nested under `a`. The border node `a` itself +/// still sits at root (a border blocks a node's children, not the +/// node). #[test] -fn hoisting_limits_keeps_blocked_name_at_parent() { +fn hoisting_limits_border_keeps_descendants_nested() { let mut importers = HashMap::new(); let mut root_deps = ResolvedDependencyMap::new(); root_deps.insert(pkg_name("a"), resolved_dep("1.0.0")); @@ -793,7 +796,7 @@ fn hoisting_limits_keeps_blocked_name_at_parent() { }; let mut blocked = BTreeSet::new(); - blocked.insert("b".to_string()); + blocked.insert("a".to_string()); let mut opts = HoistOpts::default(); opts.hoisting_limits.insert(".@".to_string(), blocked); @@ -801,18 +804,20 @@ fn hoisting_limits_keeps_blocked_name_at_parent() { let root_children = result.dependencies.borrow(); let mut names: Vec<&str> = root_children.iter().map(|dep| dep.0.name.as_str()).collect(); names.sort(); - assert_eq!(names, ["a"], "b stayed below the limit: {result:#?}"); + assert_eq!(names, ["a"], "border node a sits at root; b did not flatten: {result:#?}"); let a = Rc::clone(&root_children.iter().find(|dep| dep.0.name == "a").unwrap().0); let a_deps = a.dependencies.borrow(); let a_names: Vec<&str> = a_deps.iter().map(|dep| dep.0.name.as_str()).collect(); - assert_eq!(a_names, ["b"], "b remains under a: {a_names:?}"); + assert_eq!(a_names, ["b"], "b stays nested under the border a: {a_names:?}"); } -/// Multiple blocked names work the same way — each one stays at -/// its declaring parent. Ports the spirit of upstream's `should -/// not hoist multiple package past nohoist root`. +/// A border keeps *every* descendant of the bordered node nested, +/// not just the first. Ports the spirit of upstream's `should not +/// hoist multiple package past nohoist root`. Setup: `root → a → +/// {b, c, d}` with `hoisting_limits[".@"] = {a}`. All three of a's +/// deps stay under a. #[test] -fn hoisting_limits_blocks_multiple_names() { +fn hoisting_limits_border_keeps_all_descendants_nested() { let mut importers = HashMap::new(); let mut root_deps = ResolvedDependencyMap::new(); root_deps.insert(pkg_name("a"), resolved_dep("1.0.0")); @@ -846,8 +851,7 @@ fn hoisting_limits_blocks_multiple_names() { }; let mut blocked = BTreeSet::new(); - blocked.insert("b".to_string()); - blocked.insert("c".to_string()); + blocked.insert("a".to_string()); let mut opts = HoistOpts::default(); opts.hoisting_limits.insert(".@".to_string(), blocked); @@ -855,14 +859,18 @@ fn hoisting_limits_blocks_multiple_names() { let root_children = result.dependencies.borrow(); let mut names: Vec<&str> = root_children.iter().map(|dep| dep.0.name.as_str()).collect(); names.sort(); - // Only `a` (direct dep) and `d` (not blocked) sit at root; b - // and c stay nested under a. - assert_eq!(names, ["a", "d"], "blocked names stayed at a: {result:#?}"); + // Only the border node `a` sits at root; all of its deps stay + // nested beneath it. + assert_eq!(names, ["a"], "only the border a sits at root: {result:#?}"); let a = Rc::clone(&root_children.iter().find(|dep| dep.0.name == "a").unwrap().0); let a_deps = a.dependencies.borrow(); let mut a_names: Vec<&str> = a_deps.iter().map(|dep| dep.0.name.as_str()).collect(); a_names.sort(); - assert_eq!(a_names, ["b", "c"], "a kept its blocked deps: {a_names:?}"); + assert_eq!( + a_names, + ["b", "c", "d"], + "all of a's deps stay nested under the border: {a_names:?}", + ); } /// `hoisting_limits` keyed on a different importer (one we don't diff --git a/pacquet/plans/TEST_PORTING.md b/pacquet/plans/TEST_PORTING.md index 38f5b9f9ca..f24052513d 100644 --- a/pacquet/plans/TEST_PORTING.md +++ b/pacquet/plans/TEST_PORTING.md @@ -259,20 +259,20 @@ Rust port notes: Primary tests: -- [ ] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:16` `installing with hoisted node-linker` -- [ ] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:45` `installing with hoisted node-linker and no lockfile` -- [ ] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:61` `overwriting (is-positive@3.0.0 with is-positive@latest)` -- [ ] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:83` `overwriting existing files in node_modules` -- [ ] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:97` `preserve subdeps on update` -- [ ] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:119` `adding a new dependency to one of the workspace projects` -- [ ] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:172` `installing the same package with alias and no alias` -- [ ] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:187` `run pre/postinstall scripts. bin files should be linked in a hoisted node_modules` -- [ ] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:210` `running install scripts in a workspace that has no root project` -- [ ] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:229` `hoistingLimits should prevent packages to be hoisted` -- [ ] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:247` `externalDependencies should prevent package from being hoisted to the root` -- [ ] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:264` `linking bins of local projects when node-linker is set to hoisted` -- [ ] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:314` `peerDependencies should be installed when autoInstallPeers is set to true and nodeLinker is set to hoisted` -- [ ] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:329` `installing with hoisted node-linker a package that is a peer dependency of itself` +- [x] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:16` `installing with hoisted node-linker`. Ported as `installing_with_hoisted_node_linker` in `crates/cli/tests/hoisted_node_linker.rs` (real dirs at root + version-conflict nesting + `.modules.yaml` linker). The rimraf-then-reinstall re-add tail is the partial-install path (pnpm/pacquet#433) and is omitted. +- [x] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:45` `installing with hoisted node-linker and no lockfile`. Ported as `installing_with_hoisted_node_linker_and_no_lockfile` (real dir + no `pnpm-lock.yaml` when `lockfile: false`). +- [x] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:61` `overwriting (is-positive@3.0.0 with is-positive@latest)`. Stubbed in `known_failures::overwriting_is_positive_with_latest` — needs `pnpm add` / update manifest mutation (pnpm/pacquet#433). +- [x] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:83` `overwriting existing files in node_modules`. Stubbed in `known_failures::overwriting_existing_files_in_node_modules` (#433). +- [x] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:97` `preserve subdeps on update`. Stubbed in `known_failures::preserve_subdeps_on_update` (#433). +- [x] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:119` `adding a new dependency to one of the workspace projects`. Stubbed in `known_failures::adding_a_new_dependency_to_a_workspace_project` (#433). +- [x] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:172` `installing the same package with alias and no alias`. Stubbed in `known_failures::installing_same_package_with_alias_and_no_alias` — needs `pnpm add` of multiple specifiers + a dist-tag bump (#433). +- [x] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:187` `run pre/postinstall scripts. bin files should be linked in a hoisted node_modules`. Stubbed in `known_failures::run_pre_and_postinstall_scripts_and_link_bins` — lifecycle scripts + bin linking on the fresh path (#11870). +- [x] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:210` `running install scripts in a workspace that has no root project`. Stubbed in `known_failures::running_install_scripts_in_workspace_without_root_project` (#11870). +- [x] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:229` `hoistingLimits should prevent packages to be hoisted`. Ported as `hoisting_limits_prevents_hoisting` (`hoistingLimits: dependencies`). Pacquet's `hoistingLimits` config was migrated from the raw locator map to the `none`/`workspaces`/`dependencies` enum to match the pnpm CLI setting, and `real-hoist`'s border semantics were corrected (a name in the limits is a subtree border whose descendants stay nested, matching the `@yarnpkg/nm` hoister). +- [x] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:247` `externalDependencies should prevent package from being hoisted to the root`. Ported as `external_dependencies_prevents_hoisting_to_root`. +- [x] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:264` `linking bins of local projects when node-linker is set to hoisted`. Stubbed in `known_failures::linking_bins_of_local_projects` (#11870 — bin linking on the fresh path). +- [x] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:314` `peerDependencies should be installed when autoInstallPeers is set to true and nodeLinker is set to hoisted`. Ported as `peer_dependencies_installed_with_auto_install_peers`. +- [x] `TypeScript repo: installing/deps-installer/test/hoistedNodeLinker/install.ts:329` `installing with hoisted node-linker a package that is a peer dependency of itself`. Stubbed in `known_failures::package_that_is_peer_dependency_of_itself` — needs `pnpm add --save` + lockfile `peerDependencies` introspection (#433). - [ ] `TypeScript repo: installing/deps-installer/test/install/multipleImporters.ts:87` `install only the dependencies of the specified importer, when node-linker is hoisted` is workspace subset coverage for hoisted linker. Frozen/headless cross-coverage: