diff --git a/.changeset/hoisted-file-dep-peer-variants-stay-apart.md b/.changeset/hoisted-file-dep-peer-variants-stay-apart.md new file mode 100644 index 0000000000..493b5a5d43 --- /dev/null +++ b/.changeset/hoisted-file-dep-peer-variants-stay-apart.md @@ -0,0 +1,8 @@ +--- +"@pnpm/installing.deps-restorer": patch +"@pnpm/installing.linking.real-hoist": patch +"pacquet": patch +"pnpm": patch +--- + +Under `nodeLinker: hoisted`, peer-resolution variants of an injected directory dependency (a `file:` snapshot) are materialized as separate copies again instead of collapsing onto the first-seen variant. Each copy keeps its own peer-resolved dependency set, so a project pinning one peer version no longer resolves another project's variant — Bit root components with conflicting peers across injected copies rely on this. diff --git a/pnpm/crates/deps-restorer/src/hoisted_dep_graph/tests.rs b/pnpm/crates/deps-restorer/src/hoisted_dep_graph/tests.rs index 91697d7cc8..9f12d96125 100644 --- a/pnpm/crates/deps-restorer/src/hoisted_dep_graph/tests.rs +++ b/pnpm/crates/deps-restorer/src/hoisted_dep_graph/tests.rs @@ -1175,3 +1175,88 @@ fn walker_wires_edges_declared_against_a_collapsed_peer_variant() { "a snapshot edge on the collapsed variant resolves to the surviving copy", ); } + +/// Peer variants of an injected directory dependency are exempt from +/// the collapse (see [`pnpm_real_hoist::pkg_id`]), so the walk has to +/// keep a location — and a direct-dependency entry — per variant, +/// where every collapsed package funnels into one. +#[test] +fn walker_keeps_file_dep_peer_variants_apart() { + let mut r1_deps = ResolvedDependencyMap::new(); + r1_deps.insert( + pkg_name("comp"), + ResolvedDependencySpec { + specifier: "workspace:*".to_string(), + version: ver_peer("file:comp(peer@1.0.0)").into(), + }, + ); + r1_deps.insert(pkg_name("peer"), resolved_dep("1.0.0")); + let mut r2_deps = ResolvedDependencyMap::new(); + r2_deps.insert( + pkg_name("comp"), + ResolvedDependencySpec { + specifier: "workspace:*".to_string(), + version: ver_peer("file:comp(peer@2.0.0)").into(), + }, + ); + r2_deps.insert(pkg_name("peer"), resolved_dep("2.0.0")); + + let mut packages = HashMap::new(); + packages.insert( + dep_key("comp", "file:comp"), + PackageMetadata { resolution: directory_resolution("comp"), ..metadata_stub() }, + ); + packages.insert(dep_key("peer", "1.0.0"), metadata_stub()); + packages.insert(dep_key("peer", "2.0.0"), metadata_stub()); + + let mut snapshots = HashMap::new(); + for peer_version in ["1.0.0", "2.0.0"] { + let mut comp_deps = HashMap::new(); + comp_deps.insert(pkg_name("peer"), SnapshotDepRef::Plain(ver_peer(peer_version))); + snapshots.insert( + dep_key("comp", &format!("file:comp(peer@{peer_version})")), + SnapshotEntry { dependencies: Some(comp_deps), ..SnapshotEntry::default() }, + ); + snapshots.insert(dep_key("peer", peer_version), SnapshotEntry::default()); + } + + let lockfile = workspace_lockfile( + vec![ + (Lockfile::ROOT_IMPORTER_KEY, ResolvedDependencyMap::new()), + ("node_modules/.bit_roots/r1", r1_deps), + ("node_modules/.bit_roots/r2", r2_deps), + ], + packages, + snapshots, + ); + let opts = LockfileToHoistedDepGraphOptions { + lockfile_dir: PathBuf::from("/repo"), + ..LockfileToHoistedDepGraphOptions::default() + }; + let result = lockfile_to_hoisted_dep_graph(&lockfile, None, &opts).expect("walker succeeds"); + + let r1_comp = result.direct_dependencies_by_importer_id["node_modules/.bit_roots/r1"] + .get("comp") + .expect("r1 keeps its comp direct dependency") + .clone(); + let r2_comp = result.direct_dependencies_by_importer_id["node_modules/.bit_roots/r2"] + .get("comp") + .expect("r2 keeps its comp direct dependency") + .clone(); + assert_ne!( + r1_comp, r2_comp, + "each importer's direct dependency must be its own variant's copy", + ); + let r1_peer = result.graph[&r1_comp].children.get("peer").expect("r1's copy resolves peer"); + let r2_peer = result.graph[&r2_comp].children.get("peer").expect("r2's copy resolves peer"); + assert_eq!( + result.graph[r1_peer].dep_path, + DepPath::from("peer@1.0.0".to_string()), + "r1's copy must resolve the peer version r1 pinned", + ); + assert_eq!( + result.graph[r2_peer].dep_path, + DepPath::from("peer@2.0.0".to_string()), + "r2's copy must resolve the peer version r2 pinned", + ); +} diff --git a/pnpm/crates/lockfile/src/lib.rs b/pnpm/crates/lockfile/src/lib.rs index e0390ebd99..67e06e4c4a 100644 --- a/pnpm/crates/lockfile/src/lib.rs +++ b/pnpm/crates/lockfile/src/lib.rs @@ -297,9 +297,12 @@ impl Lockfile { } /// Whether `path` ends in a tarball extension (`.tgz`, `.tar.gz`, or -/// `.tar`, case-insensitively), so the directory-vs-tarball boundary -/// applied here matches the resolver's at resolve time. -fn is_local_tarball_path(path: &str) -> bool { +/// `.tar`, case-insensitively) — the directory-vs-tarball boundary the +/// resolver applies to a `file:` spec at resolve time. Public so +/// consumers classifying a `file:` snapshot key (such as the hoister's +/// identity function) draw the same line. +#[must_use] +pub fn is_local_tarball_path(path: &str) -> bool { let lower = path.as_bytes(); let ends_with_ci = |suffix: &str| { let bytes = suffix.as_bytes(); diff --git a/pnpm/crates/real-hoist/src/lib.rs b/pnpm/crates/real-hoist/src/lib.rs index 5a6016f43e..d8b42bbeca 100644 --- a/pnpm/crates/real-hoist/src/lib.rs +++ b/pnpm/crates/real-hoist/src/lib.rs @@ -40,7 +40,9 @@ use derive_more::{Display, Error}; use indexmap::{IndexMap, IndexSet}; use miette::Diagnostic; -use pnpm_lockfile::{Lockfile, PkgName, PkgNameVerPeer, ProjectSnapshot, SnapshotEntry}; +use pnpm_lockfile::{ + Lockfile, PkgName, PkgNameVerPeer, ProjectSnapshot, SnapshotEntry, VersionPart, +}; use std::{ cell::{Cell, RefCell}, collections::{BTreeMap, BTreeSet, HashMap, HashSet, VecDeque}, @@ -605,8 +607,27 @@ fn collect_snapshot_deps( /// package under a second name gets a node, and a directory, of its /// own. An index over the result holds the list and resolves an edge /// to its first entry. +/// +/// An injected directory dependency (a `file:` version that is not a +/// local tarball) keeps its peer suffix: every variant of it is a +/// separate on-disk copy of the local package, materialized with its +/// own peer-resolved dependency set, so collapsing the variants would +/// rewire every dependent of the losing one onto the survivor's +/// children (Bit's root components pin conflicting peers across such +/// copies on purpose). The registry collapse exists to stop +/// peer-variant explosion on large lockfiles; directory snapshots are +/// one per injected workspace package and cannot explode that way. A +/// local tarball (`file:foo.tgz`) unpacks the same archive for every +/// variant like a registry package, so it collapses like one — the +/// boundary is [`pnpm_lockfile::is_local_tarball_path`], the same one +/// the lockfile itself draws for `file:` resolutions. #[must_use] pub fn pkg_id(dep_key: &PkgNameVerPeer) -> String { + if let VersionPart::File(path) = dep_key.suffix.version() + && !pnpm_lockfile::is_local_tarball_path(path) + { + return dep_key.to_string(); + } dep_key.without_peer().to_string() } diff --git a/pnpm/crates/real-hoist/src/tests.rs b/pnpm/crates/real-hoist/src/tests.rs index ae260f05f5..5bd7948f69 100644 --- a/pnpm/crates/real-hoist/src/tests.rs +++ b/pnpm/crates/real-hoist/src/tests.rs @@ -1,5 +1,6 @@ use super::{ - HoistError, HoistOpts, HoisterResult, RcByPtr, build_hoist_ident_map, hoist, is_preferred_ident, + HoistError, HoistOpts, HoisterResult, RcByPtr, build_hoist_ident_map, hoist, + is_preferred_ident, percent_encode_path, }; use indexmap::IndexSet; use pnpm_lockfile::{ @@ -1316,6 +1317,131 @@ fn peer_suffix_variants_collapse_to_one_hoisted_copy() { ); } +/// Peer variants of an injected directory dependency are exempt from +/// the collapse — see [`pkg_id`] for why. The fixture mirrors the +/// teambit/bit root-components layout that caught the regression: two +/// importers on the same `file:` package, pinning conflicting peers. +#[test] +fn file_dep_peer_variants_keep_their_own_copies() { + let mut importers = HashMap::new(); + importers.insert(Lockfile::ROOT_IMPORTER_KEY.to_string(), ProjectSnapshot::default()); + for (importer_id, peer_ver) in + [("node_modules/.bit_roots/r1", "1.0.0"), ("node_modules/.bit_roots/r2", "2.0.0")] + { + let mut deps = ResolvedDependencyMap::new(); + deps.insert( + pkg_name("comp"), + ResolvedDependencySpec { + specifier: "workspace:*".to_string(), + version: ver_peer(&format!("file:comp(p@{peer_ver})")).into(), + }, + ); + deps.insert(pkg_name("p"), resolved_dep(peer_ver)); + importers.insert( + importer_id.to_string(), + ProjectSnapshot { dependencies: Some(deps), ..ProjectSnapshot::default() }, + ); + } + + let mut snapshots = HashMap::new(); + for peer_ver in ["1.0.0", "2.0.0"] { + let mut comp_deps = HashMap::new(); + comp_deps.insert(pkg_name("p"), SnapshotDepRef::Plain(ver_peer(peer_ver))); + snapshots.insert( + dep_key("comp", &format!("file:comp(p@{peer_ver})")), + SnapshotEntry { dependencies: Some(comp_deps), ..SnapshotEntry::default() }, + ); + snapshots.insert(dep_key("p", peer_ver), SnapshotEntry::default()); + } + + let lockfile = Lockfile { importers, snapshots: Some(snapshots), ..empty_lockfile() }; + + let result = + hoist(&lockfile, &HoistOpts::default()).expect("file-variant hoist should succeed"); + + // Node resolution walks up from the importer, so the copy an + // importer sees is the one nested in its own subtree — or the + // root's, when its variant was hoisted there. Either placement is + // correct only if the reference reached this way is the variant + // the importer declared. + let root_children = result.dependencies.borrow(); + let comp_reference_seen_by = |importer: &str| -> String { + let importer_name = percent_encode_path(importer); + let importer_node = + &root_children.iter().find(|dep| dep.0.name == importer_name).unwrap().0; + let importer_children = importer_node.dependencies.borrow(); + let comp = importer_children + .iter() + .find(|dep| dep.0.name == "comp") + .or_else(|| root_children.iter().find(|dep| dep.0.name == "comp")) + .unwrap_or_else(|| panic!("no comp reachable from {importer}")); + comp.0.references.borrow().iter().next().cloned().unwrap_or_default() + }; + assert_eq!( + comp_reference_seen_by("node_modules/.bit_roots/r1"), + "comp@file:comp(p@1.0.0)", + "r1 must resolve the copy carrying its own peer variant", + ); + assert_eq!( + comp_reference_seen_by("node_modules/.bit_roots/r2"), + "comp@file:comp(p@2.0.0)", + "r2 must resolve the copy carrying its own peer variant", + ); +} + +/// The directory exemption in [`pkg_id`] must not widen to local +/// tarballs: a `file:*.tgz` dependency collapses its peer variants +/// like a registry package. +#[test] +fn file_tarball_peer_variants_collapse_like_registry_packages() { + let mut importers = HashMap::new(); + importers.insert(Lockfile::ROOT_IMPORTER_KEY.to_string(), ProjectSnapshot::default()); + for (importer_id, peer_ver) in [("packages/a", "1.0.0"), ("packages/b", "2.0.0")] { + let mut deps = ResolvedDependencyMap::new(); + deps.insert( + pkg_name("tarpkg"), + ResolvedDependencySpec { + specifier: "file:tarpkg.tgz".to_string(), + version: ver_peer(&format!("file:tarpkg.tgz(p@{peer_ver})")).into(), + }, + ); + deps.insert(pkg_name("p"), resolved_dep(peer_ver)); + importers.insert( + importer_id.to_string(), + ProjectSnapshot { dependencies: Some(deps), ..ProjectSnapshot::default() }, + ); + } + + let mut snapshots = HashMap::new(); + for peer_ver in ["1.0.0", "2.0.0"] { + let mut tar_deps = HashMap::new(); + tar_deps.insert(pkg_name("p"), SnapshotDepRef::Plain(ver_peer(peer_ver))); + snapshots.insert( + dep_key("tarpkg", &format!("file:tarpkg.tgz(p@{peer_ver})")), + SnapshotEntry { dependencies: Some(tar_deps), ..SnapshotEntry::default() }, + ); + snapshots.insert(dep_key("p", peer_ver), SnapshotEntry::default()); + } + + let lockfile = Lockfile { importers, snapshots: Some(snapshots), ..empty_lockfile() }; + + let result = + hoist(&lockfile, &HoistOpts::default()).expect("tarball-variant hoist should succeed"); + let root_children = result.dependencies.borrow(); + let tarpkg = &root_children.iter().find(|dep| dep.0.name == "tarpkg").unwrap().0; + assert!( + tarpkg.references.borrow().contains("tarpkg@file:tarpkg.tgz(p@1.0.0)"), + "the first-seen variant is the canonical reference: {tarpkg:#?}", + ); + for importer in ["packages%2Fa", "packages%2Fb"] { + let importer_node = &root_children.iter().find(|dep| dep.0.name == importer).unwrap().0; + assert!( + !importer_node.dependencies.borrow().iter().any(|dep| dep.0.name == "tarpkg"), + "a tarball peer variant must dedup against the root copy: {importer_node:#?}", + ); + } +} + /// A nearer ancestor holding a different version of a name blocks /// both hoisting and same-node dedup for that name (upstream's /// "filled by parent" scan): removing the edge would make this diff --git a/pnpm11/installing/deps-restorer/test/lockfileToHoistedDepGraph.test.ts b/pnpm11/installing/deps-restorer/test/lockfileToHoistedDepGraph.test.ts index 959347c53b..9fa5985eca 100644 --- a/pnpm11/installing/deps-restorer/test/lockfileToHoistedDepGraph.test.ts +++ b/pnpm11/installing/deps-restorer/test/lockfileToHoistedDepGraph.test.ts @@ -134,3 +134,57 @@ function peerVariantLockfile (): LockfileObject { }, } as unknown as LockfileObject } + +// Peer variants of an injected directory dependency are exempt from the +// collapse (see `getHoisterPkgId`), so the walk has to keep a location +// per variant, where every collapsed package funnels into one. +test('lockfileToHoistedDepGraph keeps file-dep peer variants apart', async () => { + const dir = tempDir(false) + const opts = hoistedOpts(dir) + opts.storeController = { + fetchPackage: () => ({ filesIndexFile: '' }), + getFilesIndexFilePath: () => ({ filesIndexFile: '' }), + } as unknown as typeof opts.storeController + + const { graph } = await lockfileToHoistedDepGraph(fileVariantLockfile(), null, opts) + + const compDirs = Object.keys(graph).filter((dir) => path.basename(dir) === 'comp') + expect(compDirs).toHaveLength(2) + const peerVersionByVariant = Object.fromEntries(compDirs.map((dir) => { + const variant = graph[dir].depPath + const peerDir = graph[dir].children.peer + return [variant, graph[peerDir].depPath] + })) + expect(peerVersionByVariant).toStrictEqual({ + 'comp@file:comp(peer@1.0.0)': 'peer@1.0.0', + 'comp@file:comp(peer@2.0.0)': 'peer@2.0.0', + }) +}) + +function fileVariantLockfile (): LockfileObject { + const importer = (peerVersion: string) => ({ + dependencies: { + comp: `file:comp(peer@${peerVersion})`, + peer: peerVersion, + }, + specifiers: { comp: 'workspace:*', peer: peerVersion }, + }) + const compVariant = (peerVersion: string) => ({ + resolution: { directory: 'comp', type: 'directory' }, + dependencies: { peer: peerVersion }, + }) + return { + lockfileVersion: '9.0', + importers: { + '.': { specifiers: {} }, + 'node_modules/.bit_roots/r1': importer('1.0.0'), + 'node_modules/.bit_roots/r2': importer('2.0.0'), + }, + packages: { + 'comp@file:comp(peer@1.0.0)': compVariant('1.0.0'), + 'comp@file:comp(peer@2.0.0)': compVariant('2.0.0'), + 'peer@1.0.0': { resolution: { integrity: 'sha512-deadbeef' } }, + 'peer@2.0.0': { resolution: { integrity: 'sha512-deadbeef' } }, + }, + } as unknown as LockfileObject +} diff --git a/pnpm11/installing/linking/real-hoist/src/index.ts b/pnpm11/installing/linking/real-hoist/src/index.ts index 505c73e425..f1521ed05d 100644 --- a/pnpm11/installing/linking/real-hoist/src/index.ts +++ b/pnpm11/installing/linking/real-hoist/src/index.ts @@ -40,8 +40,25 @@ export type { HoisterResult } * `(alias, depPath)`, so an alias exposing a package under a second * name gets a node, and a directory, of its own. An index over the * result holds the list and resolves an edge to its first entry. + * + * An injected directory dependency (a `directory` resolution) keeps + * its peer suffix: every variant of it is a separate on-disk copy of + * the local package, materialized with its own peer-resolved + * dependency set, so collapsing the variants would rewire every + * dependent of the losing one onto the survivor's children (Bit's + * root components pin conflicting peers across such copies on + * purpose). The registry collapse exists to stop peer-variant + * explosion on large lockfiles; directory snapshots are one per + * injected workspace package and cannot explode that way. */ export function getHoisterPkgId (depPath: string, pkgSnapshot: PackageSnapshot): string { + // `resolution` is typed as required, but the lockfile is parsed from + // untyped YAML — guard so a malformed snapshot degrades to the + // collapsed identity instead of a TypeError here. + const resolution = pkgSnapshot.resolution as PackageSnapshot['resolution'] | undefined + if (resolution != null && 'directory' in resolution && resolution.directory != null) { + return depPath + } const { name, version } = nameVerFromPkgSnapshot(depPath, pkgSnapshot) return `${name}@${version}` }