From ceec335e85ec127c55f389f0a0974df70024fd65 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Thu, 14 May 2026 16:44:16 +0200 Subject: [PATCH] fix(package-manager): link optionalDependencies siblings into slot (#526) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `create_symlink_layout` only iterated `snapshot.dependencies`, so a package whose CPU/OS-specific siblings live entirely under `optionalDependencies` (e.g. `@typescript/native-preview`, `@reflink/reflink`, every `*-darwin-arm64` / `*-linux-x64` family) ended up with a slot `node_modules//` containing only the parent package — no platform binary sibling. Consumers that do `require.resolve('@typescript/native-preview-darwin-arm64')` from inside `getExePath.js` walked parent directories and found nothing, so `tsgo --version` (and every other tool that delegates to a platform variant) crashed with `Unable to resolve … missing the package on disk`. Port upstream's `dependencies ∪ optionalDependencies` merge — the graph builder at https://github.com/pnpm/pnpm/blob/da65e6262/deps/graph-builder/src/lockfileToDepGraph.ts#L150-L156 unifies both maps into one `allDeps` for each node's children, and `linkAllModules` then symlinks every child with two short-circuits: `alias === depNode.name` (a snapshot referencing itself) and `!pkg.installable && pkg.optional` (a non-materialized optional). See https://github.com/pnpm/pnpm/blob/da65e6262/installing/deps-installer/src/install/link.ts#L521-L549. `create_symlink_layout` now takes both maps and a `SkippedSnapshots` reference, merges them, and applies both short-circuits. The skip set is threaded through `InstallPackageBySnapshot` (cold batch) and the warm-batch `CreateVirtualDirBySnapshot` call site in `CreateVirtualStore::run`, so a target dropped by the installability pass, by `--no-optional`, or by a swallowed optional fetch failure gets no dangling symlink. Five new unit tests in `create_symlink_layout/tests.rs` cover the matching-optional happy path, the skipped-optional dangling-link guard, the self-name guard for entries listed in either bucket, the both-buckets-absent no-op, and the alias-resolve path (aliased deps still link the alias filename while resolving the slot via the target's name). End-to-end verification: `pacquet install --frozen-lockfile` followed by `tsgo --version` in the pnpm v11 repo now succeeds; the matching `native-preview-darwin-arm64` sibling shows up in the slot's `node_modules/@typescript/`. --- Written by an agent (Claude Code, claude-opus-4-7). --- .../src/create_symlink_layout.rs | 77 ++++-- .../src/create_symlink_layout/tests.rs | 229 ++++++++++++++++++ .../src/create_virtual_dir_by_snapshot.rs | 29 ++- .../create_virtual_dir_by_snapshot/tests.rs | 2 + .../src/create_virtual_store.rs | 2 + .../src/install_package_by_snapshot.rs | 9 + 6 files changed, 319 insertions(+), 29 deletions(-) create mode 100644 pacquet/crates/package-manager/src/create_symlink_layout/tests.rs diff --git a/pacquet/crates/package-manager/src/create_symlink_layout.rs b/pacquet/crates/package-manager/src/create_symlink_layout.rs index 9f86ee9f1b..34655c0241 100644 --- a/pacquet/crates/package-manager/src/create_symlink_layout.rs +++ b/pacquet/crates/package-manager/src/create_symlink_layout.rs @@ -1,37 +1,69 @@ -use crate::{SymlinkPackageError, VirtualStoreLayout, symlink_package}; +use crate::{SkippedSnapshots, SymlinkPackageError, VirtualStoreLayout, symlink_package}; use pacquet_lockfile::{PkgName, SnapshotDepRef}; use std::{collections::HashMap, path::Path}; /// Create symlink layout of dependencies for a package in a virtual dir. /// -/// For npm-aliased dependencies (e.g. `string-width-cjs: string-width@4.2.3`), -/// the symlink filename under `node_modules/` uses the entry key (the alias), -/// while the virtual-store lookup uses the aliased target. +/// Mirrors upstream's `linkAllModules` child-selection rules at +/// +/// and the underlying `dependencies` ∪ `optionalDependencies` merge in +/// `lockfileToDepGraph` at +/// . /// -/// Child target paths come from the install-scoped [`VirtualStoreLayout`]: -/// `layout.slot_dir(&target)` returns either +/// For each entry in `dependencies ∪ optional_dependencies`: +/// - Skip when the alias matches the slot's own package name +/// (`alias === depNode.name`), so a package that lists itself as +/// a dep doesn't get a circular self-link inside its own slot. +/// - Skip when the target snapshot is in `skipped` — its slot was +/// never materialized (platform-mismatched optional, `--no-optional` +/// excluded, or a swallowed optional fetch failure). Mirrors +/// upstream's `!pkg || (!pkg.installable && pkg.optional)` guard. +/// - Otherwise create a symlink at +/// `/` pointing to the target's +/// slot under `/node_modules/`. +/// +/// For npm-aliased dependencies (e.g. +/// `string-width-cjs: string-width@4.2.3`), the symlink filename +/// under `node_modules/` uses the entry key (the alias), while the +/// virtual-store lookup uses the aliased target. +/// +/// Child target paths come from the install-scoped +/// [`VirtualStoreLayout`]: `layout.slot_dir(&target)` returns either /// `/` (legacy) or -/// `////` (GVS), so -/// the caller doesn't have to branch on which mode is in effect. +/// `////` (GVS), +/// so the caller doesn't have to branch on which mode is in effect. /// -/// `virtual_node_modules_dir` does not have to exist — `symlink_package` calls -/// `fs::create_dir_all` on the symlink path's parent before each link. Callers -/// that already know the directory exists (e.g. `CreateVirtualStore::run`, -/// which `mkdir`s it just before calling this function) just pay redundant -/// stat syscalls, which is cheap and matches pnpm's own redundant-mkdir shape. +/// `virtual_node_modules_dir` does not have to exist — +/// `symlink_package` calls `fs::create_dir_all` on the symlink path's +/// parent before each link. Callers that already know the directory +/// exists (e.g. `CreateVirtualStore::run`, which `mkdir`s it just +/// before calling this function) just pay redundant stat syscalls, +/// which is cheap and matches pnpm's own redundant-mkdir shape. pub fn create_symlink_layout( - dependencies: &HashMap, + dependencies: Option<&HashMap>, + optional_dependencies: Option<&HashMap>, + self_name: &PkgName, + skipped: &SkippedSnapshots, layout: &VirtualStoreLayout, virtual_node_modules_dir: &Path, ) -> Result<(), SymlinkPackageError> { - // Serial iteration: the symlink work per snapshot is small (a handful of - // entries), so fanning out to rayon here would just add task-scheduling - // overhead without a wider work queue to amortise it against. The - // single-caller policy upstream is to run this stage single-threaded on a - // `spawn_blocking` worker (see `CreateVirtualStore::run`), mirroring - // pnpm's `symlinkAllModules` in `worker/src/start.ts`. - dependencies.iter().try_for_each(|(alias_name, dep_ref)| { + // Serial iteration: the symlink work per snapshot is small (a + // handful of entries), so fanning out to rayon here would just add + // task-scheduling overhead without a wider work queue to amortise + // it against. The single-caller policy upstream is to run this + // stage single-threaded on a `spawn_blocking` worker (see + // `CreateVirtualStore::run`), mirroring pnpm's `symlinkAllModules` + // in `worker/src/start.ts`. + let deps = dependencies.into_iter().flatten(); + let opt_deps = optional_dependencies.into_iter().flatten(); + deps.chain(opt_deps).try_for_each(|(alias_name, dep_ref)| { + if alias_name == self_name { + return Ok(()); + } let target = dep_ref.resolve(alias_name); + if skipped.contains(&target) { + return Ok(()); + } let target_name_str = target.name.to_string(); let alias_name_str = alias_name.to_string(); symlink_package( @@ -40,3 +72,6 @@ pub fn create_symlink_layout( ) }) } + +#[cfg(test)] +mod tests; diff --git a/pacquet/crates/package-manager/src/create_symlink_layout/tests.rs b/pacquet/crates/package-manager/src/create_symlink_layout/tests.rs new file mode 100644 index 0000000000..fd55ce06e3 --- /dev/null +++ b/pacquet/crates/package-manager/src/create_symlink_layout/tests.rs @@ -0,0 +1,229 @@ +use crate::{SkippedSnapshots, VirtualStoreLayout, create_symlink_layout}; +use pacquet_lockfile::{PackageKey, PkgName, SnapshotDepRef}; +use pretty_assertions::assert_eq; +use std::{collections::HashMap, fs, path::PathBuf}; +use tempfile::tempdir; + +fn pkg_name(input: &str) -> PkgName { + PkgName::parse(input).expect("valid pkg name") +} + +fn dep_ref(input: &str) -> SnapshotDepRef { + input.parse().expect("valid snapshot dep ref") +} + +/// A symlink in the slot's `node_modules` matches the alias and points +/// to `/node_modules/`. Trivial +/// path-shape assertion that anchors the rest of the test cases. +fn assert_symlink_shape( + virtual_node_modules_dir: &std::path::Path, + alias: &str, + layout: &VirtualStoreLayout, + target_key: &PackageKey, +) { + let symlink_path = virtual_node_modules_dir.join(alias); + let read = fs::read_link(&symlink_path) + .unwrap_or_else(|err| panic!("read_link {symlink_path:?}: {err}")); + let expected = + layout.slot_dir(target_key).join("node_modules").join(target_key.name.to_string()); + assert_eq!(read, expected); +} + +/// `optionalDependencies` siblings whose target slot is **not** in +/// `skipped` get linked alongside the regular `dependencies` siblings. +/// This is the v11 install path: a snapshot like +/// `@typescript/native-preview` lists every platform variant under +/// `optionalDependencies`, and the installability pass leaves the +/// host-matching variant out of `skipped`. Without this, downstream +/// `getExePath`-style lookups fail because the matching binary slot +/// is missing from the consumer's slot `node_modules`. +#[test] +fn links_matching_optional_sibling_alongside_regular_deps() { + let tmp = tempdir().expect("tempdir"); + let virtual_store_dir = tmp.path().to_path_buf(); + let layout = VirtualStoreLayout::legacy(virtual_store_dir.clone()); + + let mut deps: HashMap = HashMap::new(); + deps.insert(pkg_name("plain-dep"), dep_ref("1.0.0")); + + let mut optional: HashMap = HashMap::new(); + optional.insert(pkg_name("matching-optional"), dep_ref("2.0.0")); + + let skipped = SkippedSnapshots::default(); + + let virtual_node_modules_dir = tmp.path().join("self/node_modules"); + fs::create_dir_all(&virtual_node_modules_dir).unwrap(); + + create_symlink_layout( + Some(&deps), + Some(&optional), + &pkg_name("self"), + &skipped, + &layout, + &virtual_node_modules_dir, + ) + .expect("create_symlink_layout should succeed"); + + assert_symlink_shape( + &virtual_node_modules_dir, + "plain-dep", + &layout, + &"plain-dep@1.0.0".parse().unwrap(), + ); + assert_symlink_shape( + &virtual_node_modules_dir, + "matching-optional", + &layout, + &"matching-optional@2.0.0".parse().unwrap(), + ); +} + +/// Optional siblings whose target snapshot is in `skipped` are +/// *not* linked — their slot was never materialized, so a symlink +/// would dangle. Mirrors upstream's `!pkg.installable && pkg.optional` +/// short-circuit at +/// . +#[test] +fn skips_optional_siblings_that_are_in_skipped() { + let tmp = tempdir().expect("tempdir"); + let virtual_store_dir = tmp.path().to_path_buf(); + let layout = VirtualStoreLayout::legacy(virtual_store_dir); + + let mut optional: HashMap = HashMap::new(); + optional.insert(pkg_name("matching-optional"), dep_ref("2.0.0")); + optional.insert(pkg_name("mismatched-optional"), dep_ref("3.0.0")); + + let mut skipped_set = std::collections::HashSet::::new(); + skipped_set.insert("mismatched-optional@3.0.0".parse().unwrap()); + let skipped = SkippedSnapshots::from_set(skipped_set); + + let virtual_node_modules_dir = tmp.path().join("self/node_modules"); + fs::create_dir_all(&virtual_node_modules_dir).unwrap(); + + create_symlink_layout( + None, + Some(&optional), + &pkg_name("self"), + &skipped, + &layout, + &virtual_node_modules_dir, + ) + .expect("create_symlink_layout should succeed"); + + // `symlink_metadata` reports the link itself, not the target — + // crucial for this assertion because the slot the link points to + // is never created in this test (the symlink is intentionally + // dangling). `Path::exists()` would follow the link and return + // false despite the link existing. + assert!( + fs::symlink_metadata(virtual_node_modules_dir.join("matching-optional")) + .is_ok_and(|m| m.is_symlink()), + "matching optional sibling must be linked", + ); + assert!( + fs::symlink_metadata(virtual_node_modules_dir.join("mismatched-optional")).is_err(), + "skipped optional sibling must not be linked (would dangle)", + ); +} + +/// A dep whose alias matches the slot's own package name is a +/// self-link that Node's resolver doesn't need (and upstream +/// explicitly excludes via `alias === depNode.name` at +/// ). +/// Tests both buckets — `dependencies` and `optionalDependencies` — +/// because either can list the self-name in the wild. +#[test] +fn skips_dep_entries_whose_alias_matches_self_name() { + let tmp = tempdir().expect("tempdir"); + let virtual_store_dir = tmp.path().to_path_buf(); + let layout = VirtualStoreLayout::legacy(virtual_store_dir); + + let mut deps: HashMap = HashMap::new(); + deps.insert(pkg_name("self"), dep_ref("1.0.0")); + + let mut optional: HashMap = HashMap::new(); + optional.insert(pkg_name("self"), dep_ref("1.0.0")); + + let skipped = SkippedSnapshots::default(); + let virtual_node_modules_dir = tmp.path().join("self/node_modules"); + fs::create_dir_all(&virtual_node_modules_dir).unwrap(); + + create_symlink_layout( + Some(&deps), + Some(&optional), + &pkg_name("self"), + &skipped, + &layout, + &virtual_node_modules_dir, + ) + .expect("create_symlink_layout should succeed"); + + let entries: Vec = fs::read_dir(&virtual_node_modules_dir) + .unwrap() + .map(|entry| entry.unwrap().path()) + .collect(); + assert!(entries.is_empty(), "self-named entries must not become symlinks; got {entries:?}"); +} + +/// Both `dependencies` and `optional_dependencies` absent is a +/// no-op — the empty-snapshot fast path matches what the legacy +/// `Option<&HashMap>` signature used to do for `dependencies = None`. +#[test] +fn both_dep_maps_absent_is_a_noop() { + let tmp = tempdir().expect("tempdir"); + let virtual_store_dir = tmp.path().to_path_buf(); + let layout = VirtualStoreLayout::legacy(virtual_store_dir); + let skipped = SkippedSnapshots::default(); + let virtual_node_modules_dir = tmp.path().join("self/node_modules"); + fs::create_dir_all(&virtual_node_modules_dir).unwrap(); + + create_symlink_layout( + None, + None, + &pkg_name("self"), + &skipped, + &layout, + &virtual_node_modules_dir, + ) + .expect("create_symlink_layout should succeed with no deps"); + + let entries: Vec<_> = fs::read_dir(&virtual_node_modules_dir).unwrap().collect(); + assert!(entries.is_empty(), "no symlinks should be created when both dep maps are absent"); +} + +/// Aliased `dependencies` entries — `: @` +/// shape — still link the alias filename in the slot's `node_modules` +/// while resolving the slot via the target's name. Guards the +/// `dep_ref.resolve(alias_name)` behavior since the merge change +/// rewrote the iteration shape; the alias path mustn't regress. +#[test] +fn alias_dep_links_under_alias_but_resolves_via_target() { + let tmp = tempdir().expect("tempdir"); + let virtual_store_dir = tmp.path().to_path_buf(); + let layout = VirtualStoreLayout::legacy(virtual_store_dir); + + let mut deps: HashMap = HashMap::new(); + deps.insert(pkg_name("string-width-cjs"), dep_ref("string-width@4.2.3")); + + let skipped = SkippedSnapshots::default(); + let virtual_node_modules_dir = tmp.path().join("self/node_modules"); + fs::create_dir_all(&virtual_node_modules_dir).unwrap(); + + create_symlink_layout( + Some(&deps), + None, + &pkg_name("self"), + &skipped, + &layout, + &virtual_node_modules_dir, + ) + .expect("create_symlink_layout should succeed"); + + let symlink_path = virtual_node_modules_dir.join("string-width-cjs"); + let read = fs::read_link(&symlink_path).expect("read_link"); + let expected = layout + .slot_dir(&"string-width@4.2.3".parse().unwrap()) + .join("node_modules") + .join("string-width"); + assert_eq!(read, expected); +} diff --git a/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot.rs b/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot.rs index a77f66dfca..88b17cfe4a 100644 --- a/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot.rs +++ b/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot.rs @@ -1,6 +1,6 @@ use crate::{ - ImportIndexedDirError, ImportIndexedDirOpts, SymlinkPackageError, VirtualStoreLayout, - create_symlink_layout, import_indexed_dir, + ImportIndexedDirError, ImportIndexedDirOpts, SkippedSnapshots, SymlinkPackageError, + VirtualStoreLayout, create_symlink_layout, import_indexed_dir, }; use derive_more::{Display, Error}; use miette::Diagnostic; @@ -47,6 +47,14 @@ pub struct CreateVirtualDirBySnapshot<'a> { pub package_id: &'a str, pub package_key: &'a PackageKey, pub snapshot: &'a SnapshotEntry, + /// Snapshots whose slots were not materialized on this host — + /// platform-mismatched optionals, `--no-optional` exclusions, and + /// swallowed optional fetch failures. `create_symlink_layout` + /// uses this to skip dangling symlinks to absent slots. Mirrors + /// upstream's `!pkg.installable && pkg.optional` short-circuit in + /// `linkAllModules` at + /// . + pub skipped: &'a SkippedSnapshots, } /// Error type of [`CreateVirtualDirBySnapshot`]. @@ -79,6 +87,7 @@ impl<'a> CreateVirtualDirBySnapshot<'a> { package_id: _package_id, package_key, snapshot, + skipped, } = self; let virtual_node_modules_dir = layout.slot_dir(package_key).join("node_modules"); @@ -109,12 +118,16 @@ impl<'a> CreateVirtualDirBySnapshot<'a> { ) .map_err(CreateVirtualDirError::ImportIndexedDir) }, - || match snapshot.dependencies.as_ref() { - Some(dependencies) => { - create_symlink_layout(dependencies, layout, &virtual_node_modules_dir) - .map_err(CreateVirtualDirError::SymlinkPackage) - } - None => Ok(()), + || { + create_symlink_layout( + snapshot.dependencies.as_ref(), + snapshot.optional_dependencies.as_ref(), + &package_key.name, + skipped, + layout, + &virtual_node_modules_dir, + ) + .map_err(CreateVirtualDirError::SymlinkPackage) }, ); cas_result?; diff --git a/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot/tests.rs b/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot/tests.rs index 9098c7eafd..227d2d2163 100644 --- a/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot/tests.rs +++ b/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot/tests.rs @@ -61,6 +61,7 @@ async fn run_emits_imported_event_after_import_indexed_dir() { // `.run()` directly here. The function itself is sync — only // the caller's runtime flavor matters. let layout = crate::VirtualStoreLayout::legacy(virtual_store_dir.clone()); + let skipped = crate::SkippedSnapshots::default(); CreateVirtualDirBySnapshot { layout: &layout, cas_paths: &cas_paths, @@ -70,6 +71,7 @@ async fn run_emits_imported_event_after_import_indexed_dir() { package_id: "react@18.0.0", package_key: &package_key, snapshot: &snapshot, + skipped: &skipped, } .run::() .expect("empty-cas-paths run should succeed"); diff --git a/pacquet/crates/package-manager/src/create_virtual_store.rs b/pacquet/crates/package-manager/src/create_virtual_store.rs index c93494b4c8..96c5ed67b7 100644 --- a/pacquet/crates/package-manager/src/create_virtual_store.rs +++ b/pacquet/crates/package-manager/src/create_virtual_store.rs @@ -647,6 +647,7 @@ impl<'a> CreateVirtualStore<'a> { package_id: &package_id, package_key: snapshot_key, snapshot, + skipped, } .run::() .map_err(|e| { @@ -725,6 +726,7 @@ impl<'a> CreateVirtualStore<'a> { metadata, snapshot, allow_build_policy, + skipped, node_linker, } .run::() diff --git a/pacquet/crates/package-manager/src/install_package_by_snapshot.rs b/pacquet/crates/package-manager/src/install_package_by_snapshot.rs index a03b613189..c1ac73b696 100644 --- a/pacquet/crates/package-manager/src/install_package_by_snapshot.rs +++ b/pacquet/crates/package-manager/src/install_package_by_snapshot.rs @@ -68,6 +68,13 @@ pub struct InstallPackageBySnapshot<'a> { /// [`crate::InstallFrozenLockfile::run`] and threaded through /// [`crate::CreateVirtualStore`]. pub allow_build_policy: &'a AllowBuildPolicy, + /// Snapshots whose slots were not materialized on this host — + /// threaded into [`CreateVirtualDirBySnapshot`] so the per-slot + /// `create_symlink_layout` step can skip optional siblings whose + /// target slot is absent (platform mismatch, `--no-optional` + /// exclusion, or swallowed optional fetch failure). See + /// [`crate::SkippedSnapshots`] for how it is built. + pub skipped: &'a crate::SkippedSnapshots, /// Selects between the isolated and hoisted install layouts. /// `Isolated` runs [`CreateVirtualDirBySnapshot`] at the end of /// the per-snapshot fetch to populate the virtual-store slot; @@ -200,6 +207,7 @@ impl<'a> InstallPackageBySnapshot<'a> { metadata, snapshot, allow_build_policy, + skipped, node_linker, } = self; @@ -434,6 +442,7 @@ impl<'a> InstallPackageBySnapshot<'a> { package_id: &package_id, package_key, snapshot, + skipped, } .run::() .map_err(InstallPackageBySnapshotError::CreateVirtualDir)?;