From 2dabb31dcefbb58ef671c60c8f7cf3e6f100fce1 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Sun, 5 Jul 2026 08:42:49 +0200 Subject: [PATCH] fix(pacquet): skip unsupported optional deps on fresh install (#12809) Rust pnpm was resolving optional dependencies and starting prefetch downloads before it computed installability on the fresh-lockfile path. That meant platform-specific optional packages for other OS/CPU combinations could be downloaded even though they would never be linked or built. Run the same installability skip computation for fresh lockfiles before virtual-store materialization, thread those skips back to the modules manifest writer, and make the prefetch resolver avoid platform-incompatible optional tarballs. The prefetch shortcut only checks platform compatibility; the full installability pass still handles engine checks with the real host Node version. This matches the TypeScript requester behavior, which skips fetching unsupported optional packages after resolution. --- Cargo.lock | 2 + pacquet/crates/cli/src/cli_args/pipelines.rs | 2 +- .../src/check_platform.rs | 152 ++++----- .../crates/package-is-installable/src/lib.rs | 6 +- pacquet/crates/package-manager/Cargo.toml | 4 +- .../package-manager/src/current_lockfile.rs | 59 ++-- .../src/current_lockfile/tests.rs | 102 ++++-- pacquet/crates/package-manager/src/install.rs | 23 +- .../package-manager/src/install/tests.rs | 118 +++++++ .../src/install_with_fresh_lockfile.rs | 81 +++-- .../package-manager/src/installability.rs | 151 +++++++-- .../src/installability/tests.rs | 84 ++++- .../src/prefetching_resolver.rs | 51 ++- .../src/prefetching_resolver/tests.rs | 300 ++++++++++++++++++ 14 files changed, 951 insertions(+), 184 deletions(-) create mode 100644 pacquet/crates/package-manager/src/prefetching_resolver/tests.rs diff --git a/Cargo.lock b/Cargo.lock index 70b7f90238..e658d5801d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4283,6 +4283,7 @@ dependencies = [ "dashmap", "derive_more", "dunce", + "flate2", "futures-util", "httpdate", "indexmap 2.14.0", @@ -4343,6 +4344,7 @@ dependencies = [ "serde_json", "sha2", "ssri", + "tar", "tempfile", "text-block-macros", "tokio", diff --git a/pacquet/crates/cli/src/cli_args/pipelines.rs b/pacquet/crates/cli/src/cli_args/pipelines.rs index 61a2ad0c19..2e8cead8e7 100644 --- a/pacquet/crates/cli/src/cli_args/pipelines.rs +++ b/pacquet/crates/cli/src/cli_args/pipelines.rs @@ -51,7 +51,7 @@ impl InstallPipeline { let cfg: &'static Config = cfg; let state = State::init(manifest_path, cfg, require_lockfile).wrap_err("initialize the state")?; - args.run::(state).await + Box::pin(args.run::(state)).await } } diff --git a/pacquet/crates/package-is-installable/src/check_platform.rs b/pacquet/crates/package-is-installable/src/check_platform.rs index 246c84ac29..59c4fd9228 100644 --- a/pacquet/crates/package-is-installable/src/check_platform.rs +++ b/pacquet/crates/package-is-installable/src/check_platform.rs @@ -8,8 +8,7 @@ use serde::{Deserialize, Serialize}; /// against which a package's wanted platform is evaluated. Each list /// defaults to `['current']` at the call site (read from the config /// setting, falling back to `['current']` if absent). The `'current'` -/// sentinel is replaced with the host triple via `dedupe_current` -/// before the `os` / `cpu` / `libc` lists are compared. +/// sentinel is compared as the concrete host triple. #[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct SupportedArchitectures { #[serde(skip_serializing_if = "Option::is_none")] @@ -116,7 +115,7 @@ fn json_string_array(values: &[String]) -> String { /// Returns `None` when the package is compatible, or /// `Some(UnsupportedPlatformError)` when any constraint rejects the /// host. Negation entries (`!foo`) and the special `any` sentinel are -/// honored (see `check_list` in this module). +/// honored. /// /// The wanted axes are taken as `Option<&[String]>` slices so the /// hot path doesn't allocate a [`WantedPlatform`] per snapshot — @@ -125,9 +124,7 @@ fn json_string_array(values: &[String]) -> String { /// (for diagnostic display via the /// [`UnsupportedPlatformError`]). /// -/// `supported_architectures` substitutes for `['current']` per axis; -/// `'current'` entries are replaced with the host value before -/// comparison (see `dedupe_current` in this module). +/// `supported_architectures` substitutes for `['current']` per axis. /// /// `current_os`, `current_cpu`, and `current_libc` are passed in /// rather than read from the environment so this function stays @@ -140,83 +137,92 @@ pub fn check_platform( current_cpu: &str, current_libc: &str, ) -> Option { - let default_current = vec!["current".to_string()]; - let os_supp = supported.and_then(|supported| supported.os.as_ref()).unwrap_or(&default_current); - let cpu_supp = - supported.and_then(|supported| supported.cpu.as_ref()).unwrap_or(&default_current); - let libc_supp = - supported.and_then(|supported| supported.libc.as_ref()).unwrap_or(&default_current); + if platform_is_supported(wanted, supported, current_os, current_cpu, current_libc) { + return None; + } - let current = Platform { - os: dedupe_current(current_os, os_supp), - cpu: dedupe_current(current_cpu, cpu_supp), - libc: dedupe_current(current_libc, libc_supp), + let owned_wanted = WantedPlatform { + os: wanted.os.map(<[String]>::to_vec), + cpu: wanted.cpu.map(<[String]>::to_vec), + libc: wanted.libc.map(<[String]>::to_vec), }; - - let mut os_ok = true; - let mut cpu_ok = true; - let mut libc_ok = true; - - if let Some(wanted_os) = wanted.os { - os_ok = check_list(¤t.os, wanted_os); - } - if let Some(wanted_cpu) = wanted.cpu { - cpu_ok = check_list(¤t.cpu, wanted_cpu); - } - if let Some(wanted_libc) = wanted.libc - && current_libc != "unknown" - { - libc_ok = check_list(¤t.libc, wanted_libc); - } - - if !os_ok || !cpu_ok || !libc_ok { - // Cold path. Only here do we materialise the owned - // `WantedPlatform` for the error payload — the rest of the - // pass borrows. - let owned_wanted = WantedPlatform { - os: wanted.os.map(<[String]>::to_vec), - cpu: wanted.cpu.map(<[String]>::to_vec), - libc: wanted.libc.map(<[String]>::to_vec), - }; - let real_current = Platform { - os: vec![current_os.to_string()], - cpu: vec![current_cpu.to_string()], - libc: vec![current_libc.to_string()], - }; - return Some(UnsupportedPlatformError::new( - package_id.to_string(), - owned_wanted, - real_current, - )); - } - None + let real_current = Platform { + os: vec![current_os.to_string()], + cpu: vec![current_cpu.to_string()], + libc: vec![current_libc.to_string()], + }; + Some(UnsupportedPlatformError::new(package_id.to_string(), owned_wanted, real_current)) } -/// Replace the literal `current` sentinel in `supported` with the -/// concrete host value. -fn dedupe_current(current: &str, supported: &[String]) -> Vec { - supported - .iter() - .map(|item| if item == "current" { current.to_string() } else { item.clone() }) - .collect() +#[must_use] +pub fn platform_is_supported( + wanted: WantedPlatformRef<'_>, + supported: Option<&SupportedArchitectures>, + current_os: &str, + current_cpu: &str, + current_libc: &str, +) -> bool { + wanted.os.is_none_or(|wanted_os| { + axis_is_supported( + current_os, + supported.and_then(|supported| supported.os.as_deref()), + wanted_os, + ) + }) && wanted.cpu.is_none_or(|wanted_cpu| { + axis_is_supported( + current_cpu, + supported.and_then(|supported| supported.cpu.as_deref()), + wanted_cpu, + ) + }) && wanted.libc.is_none_or(|wanted_libc| { + current_libc == "unknown" + || axis_is_supported( + current_libc, + supported.and_then(|supported| supported.libc.as_deref()), + wanted_libc, + ) + }) } -/// Decide whether any element of `value` is allowed by `list`. -fn check_list(value: &[String], list: &[String]) -> bool { - if list.len() == 1 && list[0] == "any" { +fn axis_is_supported(current: &str, supported: Option<&[String]>, wanted: &[String]) -> bool { + if wanted.len() == 1 && wanted[0] == "any" { return true; } + let mut matched = false; - for v in value { - for entry in list { - if let Some(stripped) = entry.strip_prefix('!') { - if stripped == v { - return false; - } - } else if entry == v { - matched = true; + if let Some(supported) = supported { + for value in supported { + match platform_value_match(if value == "current" { current } else { value }, wanted) { + PlatformValueMatch::Rejected => return false, + PlatformValueMatch::Matched => matched = true, + PlatformValueMatch::NoMatch => {} } } + } else { + match platform_value_match(current, wanted) { + PlatformValueMatch::Rejected => return false, + PlatformValueMatch::Matched => matched = true, + PlatformValueMatch::NoMatch => {} + } } - matched || list.iter().all(|e| e.starts_with('!')) + matched || wanted.iter().all(|entry| entry.starts_with('!')) +} + +enum PlatformValueMatch { + Rejected, + Matched, + NoMatch, +} + +fn platform_value_match(value: &str, wanted: &[String]) -> PlatformValueMatch { + for entry in wanted { + if let Some(stripped) = entry.strip_prefix('!') { + if stripped == value { + return PlatformValueMatch::Rejected; + } + } else if entry == value { + return PlatformValueMatch::Matched; + } + } + PlatformValueMatch::NoMatch } diff --git a/pacquet/crates/package-is-installable/src/lib.rs b/pacquet/crates/package-is-installable/src/lib.rs index 2ee9cff86a..d53f8edbf7 100644 --- a/pacquet/crates/package-is-installable/src/lib.rs +++ b/pacquet/crates/package-is-installable/src/lib.rs @@ -1,11 +1,13 @@ //! Evaluates whether a package can be installed on the current host. //! -//! Three exported functions: +//! Exported functions: //! - [`check_engine()`] — evaluates `engines.node` / `engines.pnpm` against //! the current runtime. //! - [`check_platform()`] — evaluates a package's `os` / `cpu` / `libc` //! triple against the host (or a caller-supplied //! [`SupportedArchitectures`] override). +//! - [`platform_is_supported()`] — the allocation-light boolean form of +//! the same platform check. //! - [`package_is_installable()`] — composes the two and produces a //! tri-state verdict: compatible, skip-as-optional, or //! proceed-with-warning. Caller handles emitting `pnpm:install-check` @@ -24,7 +26,7 @@ pub use check_engine::{ }; pub use check_platform::{ Platform, SupportedArchitectures, UnsupportedPlatformError, WantedPlatform, WantedPlatformRef, - check_platform, + check_platform, platform_is_supported, }; pub use infer_platform_from_package_name::{infer_platform_from_package_name, inferred_platform}; pub use package_is_installable::{ diff --git a/pacquet/crates/package-manager/Cargo.toml b/pacquet/crates/package-manager/Cargo.toml index 575dacac19..0e105652a7 100644 --- a/pacquet/crates/package-manager/Cargo.toml +++ b/pacquet/crates/package-manager/Cargo.toml @@ -77,15 +77,17 @@ miette = { workspace = true } pacquet-testing-utils = { workspace = true } async-trait = { workspace = true } -node-semver = { workspace = true } dunce = { workspace = true } +flate2 = { workspace = true } insta = { workspace = true } mockito = { workspace = true } +node-semver = { workspace = true } pathdiff = { workspace = true } pretty_assertions = { workspace = true } serde-saphyr = { workspace = true } sha2 = { workspace = true } ssri = { workspace = true } +tar = { workspace = true } tempfile = { workspace = true } text-block-macros = { workspace = true } walkdir = { workspace = true } diff --git a/pacquet/crates/package-manager/src/current_lockfile.rs b/pacquet/crates/package-manager/src/current_lockfile.rs index 9021b9848a..66c79f3dbf 100644 --- a/pacquet/crates/package-manager/src/current_lockfile.rs +++ b/pacquet/crates/package-manager/src/current_lockfile.rs @@ -3,10 +3,9 @@ //! //! Rather than re-running the engine + `supportedArchitectures` + //! `skipped` checks at filter time, reuse the [`SkippedSnapshots`] -//! set produced during install. Its union of `installability` -//! (slice 1) + `fetch_failed` (slice 4) + `optional_excluded` -//! (slice 5) is the exact set the filter would drop — just -//! precomputed during the install pipeline, no duplicated walk. +//! set produced during install. The full set filters materialized +//! snapshots; the package metadata map only drops user exclusions +//! such as `--no-optional` and `--no-runtime`. //! //! The output drives the **next** install's diff. Without this //! filter, pacquet's current lockfile recorded every snapshot the @@ -30,15 +29,24 @@ use crate::SkippedSnapshots; /// /// Importers lose dep maps whose `include` flag is false; importer /// `optionalDependencies` lose entries whose resolved snapshot got -/// skipped; the snapshot + package maps are pruned to the transitive -/// closure reachable from the surviving importer roots. +/// skipped; the snapshot map is pruned to the transitive closure +/// reachable from the surviving importer roots. The package metadata +/// map preserves installability-skipped entries, matching pnpm's +/// current-lockfile shape while `.modules.yaml.skipped` records the +/// materialization skip. #[must_use] pub fn filter_lockfile_for_current( lockfile: &Lockfile, included: IncludedDependencies, skipped: &SkippedSnapshots, ) -> Lockfile { - let reachable = collect_reachable(&lockfile.importers, lockfile.snapshots.as_ref(), skipped); + let reachable = collect_reachable(&lockfile.importers, lockfile.snapshots.as_ref(), |key| { + skipped.contains(key) + }); + let metadata_reachable = + collect_reachable(&lockfile.importers, lockfile.snapshots.as_ref(), |key| { + skipped.contains_optional_excluded(key) + }); // Reachable metadata keys: snapshot keys without the peer suffix. // The `packages:` map is keyed by `metadata_key` (e.g. @@ -48,7 +56,7 @@ pub fn filter_lockfile_for_current( // union of `without_peer()` keys so peer-variant survivors keep // their shared metadata row. let mut reachable_metadata: HashSet = HashSet::new(); - for snap_key in &reachable { + for snap_key in &metadata_reachable { reachable_metadata.insert(snap_key.without_peer()); } @@ -141,15 +149,17 @@ fn retain_reachable(map: &mut ResolvedDependencyMap, reachable: &HashSet( importers: &HashMap, snapshots: Option<&HashMap>, - skipped: &SkippedSnapshots, -) -> HashSet { + should_skip: ShouldSkip, +) -> HashSet +where + ShouldSkip: Fn(&PackageKey) -> bool, +{ let Some(snapshots) = snapshots else { return HashSet::new() }; let mut reachable: HashSet = HashSet::new(); @@ -173,7 +183,7 @@ fn collect_reachable( { for (name, spec) in map { let Some(key) = spec.version.resolved_key(name) else { continue }; - if skipped.contains(&key) { + if should_skip(&key) { continue; } if snapshots.contains_key(&key) { @@ -192,7 +202,7 @@ fn collect_reachable( [snap.dependencies.as_ref(), snap.optional_dependencies.as_ref()].into_iter().flatten() { for (alias, dep_ref) in dep_map { - if let Some(child) = resolve_child(alias, dep_ref, snapshots, skipped) { + if let Some(child) = resolve_child(alias, dep_ref, snapshots, &should_skip) { queue.push_back(child); } } @@ -202,19 +212,22 @@ fn collect_reachable( reachable } -/// Resolve a snapshot child to its `PackageKey`, dropping it if -/// the target is in `skipped` or absent from the snapshot map. -fn resolve_child( +/// Resolve a snapshot child to its `PackageKey`, dropping it if the +/// target should be skipped or is absent from the snapshot map. +fn resolve_child( alias: &PkgName, dep_ref: &SnapshotDepRef, snapshots: &HashMap, - skipped: &SkippedSnapshots, -) -> Option { + should_skip: &ShouldSkip, +) -> Option +where + ShouldSkip: Fn(&PackageKey) -> bool, +{ // `link:` deps live outside the virtual store and have no // snapshot to reach — they aren't part of the reachable-snapshot // graph this helper computes. let resolved = dep_ref.resolve(alias)?; - if skipped.contains(&resolved) { + if should_skip(&resolved) { return None; } snapshots.contains_key(&resolved).then_some(resolved) diff --git a/pacquet/crates/package-manager/src/current_lockfile/tests.rs b/pacquet/crates/package-manager/src/current_lockfile/tests.rs index fb55f4abf7..2fc81b07f7 100644 --- a/pacquet/crates/package-manager/src/current_lockfile/tests.rs +++ b/pacquet/crates/package-manager/src/current_lockfile/tests.rs @@ -3,8 +3,9 @@ use std::collections::HashMap; use pacquet_lockfile::{ - ComVer, ImporterDepVersion, Lockfile, LockfileVersion, PackageKey, PkgName, PkgVerPeer, - ProjectSnapshot, ResolvedDependencyMap, ResolvedDependencySpec, SnapshotDepRef, SnapshotEntry, + ComVer, ImporterDepVersion, Lockfile, LockfileResolution, LockfileVersion, PackageKey, + PackageMetadata, PkgName, PkgVerPeer, ProjectSnapshot, ResolvedDependencyMap, + ResolvedDependencySpec, SnapshotDepRef, SnapshotEntry, TarballResolution, }; use pacquet_modules_yaml::IncludedDependencies; use pretty_assertions::assert_eq; @@ -40,6 +41,28 @@ fn snapshot_with_deps(deps: &[(&str, &str)]) -> SnapshotEntry { SnapshotEntry { dependencies: Some(map), ..Default::default() } } +fn package_metadata(name: &str) -> PackageMetadata { + PackageMetadata { + resolution: LockfileResolution::Tarball(TarballResolution { + integrity: None, + tarball: format!("https://example.test/{name}.tgz"), + git_hosted: None, + path: None, + }), + version: None, + engines: None, + cpu: None, + os: None, + libc: None, + deprecated: None, + has_bin: None, + prepare: None, + bundled_dependencies: None, + peer_dependencies: None, + peer_dependencies_meta: None, + } +} + fn empty_lockfile() -> Lockfile { Lockfile { lockfile_version: LockfileVersion::<9>::try_from(ComVer { major: 9, minor: 0 }).unwrap(), @@ -196,9 +219,7 @@ fn snapshot_reachable_via_kept_path_survives() { } #[test] -fn packages_filtered_to_surviving_metadata_keys() { - use pacquet_lockfile::{LockfileResolution, PackageMetadata, TarballResolution}; - +fn user_excluded_packages_filtered_to_surviving_metadata_keys() { let mut importers = HashMap::new(); importers.insert( ".".to_string(), @@ -213,29 +234,9 @@ fn packages_filtered_to_surviving_metadata_keys() { snapshots.insert(key("keep", "1.0.0"), SnapshotEntry::default()); snapshots.insert(key("drop", "1.0.0"), SnapshotEntry::default()); - let make_metadata = |name: &str| PackageMetadata { - resolution: LockfileResolution::Tarball(TarballResolution { - integrity: None, - tarball: format!("https://example.test/{name}.tgz"), - git_hosted: None, - path: None, - }), - version: None, - engines: None, - cpu: None, - os: None, - libc: None, - deprecated: None, - has_bin: None, - prepare: None, - bundled_dependencies: None, - peer_dependencies: None, - peer_dependencies_meta: None, - }; - let mut packages = HashMap::new(); - packages.insert(key("keep", "1.0.0"), make_metadata("keep")); - packages.insert(key("drop", "1.0.0"), make_metadata("drop")); + packages.insert(key("keep", "1.0.0"), package_metadata("keep")); + packages.insert(key("drop", "1.0.0"), package_metadata("drop")); let lockfile = Lockfile { importers, @@ -253,10 +254,55 @@ fn packages_filtered_to_surviving_metadata_keys() { assert!(pkgs.contains_key(&key("keep", "1.0.0"))); assert!( !pkgs.contains_key(&key("drop", "1.0.0")), - "metadata for a pruned snapshot must also be pruned", + "metadata for a user-excluded snapshot must also be pruned", ); } +#[test] +fn installability_skipped_metadata_is_preserved() { + let mut importers = HashMap::new(); + importers.insert( + ".".to_string(), + ProjectSnapshot { + dependencies: Some(importer_map(&[("keep", "1.0.0")])), + optional_dependencies: Some(importer_map(&[("drop", "1.0.0")])), + ..Default::default() + }, + ); + + let mut snapshots = HashMap::new(); + snapshots.insert(key("keep", "1.0.0"), SnapshotEntry::default()); + snapshots.insert(key("drop", "1.0.0"), snapshot_with_deps(&[("child", "1.0.0")])); + snapshots.insert(key("child", "1.0.0"), SnapshotEntry::default()); + + let mut packages = HashMap::new(); + packages.insert(key("keep", "1.0.0"), package_metadata("keep")); + packages.insert(key("drop", "1.0.0"), package_metadata("drop")); + packages.insert(key("child", "1.0.0"), package_metadata("child")); + + let lockfile = Lockfile { + importers, + snapshots: Some(snapshots), + packages: Some(packages), + ..empty_lockfile() + }; + + let mut skipped = SkippedSnapshots::new(); + skipped.insert_installability(key("drop", "1.0.0")); + + let filtered = super::filter_lockfile_for_current(&lockfile, include_all(), &skipped); + + let snaps = filtered.snapshots.as_ref().unwrap(); + assert!(snaps.contains_key(&key("keep", "1.0.0"))); + assert!(!snaps.contains_key(&key("drop", "1.0.0"))); + assert!(!snaps.contains_key(&key("child", "1.0.0"))); + + let pkgs = filtered.packages.as_ref().unwrap(); + assert!(pkgs.contains_key(&key("keep", "1.0.0"))); + assert!(pkgs.contains_key(&key("drop", "1.0.0"))); + assert!(pkgs.contains_key(&key("child", "1.0.0"))); +} + #[test] fn link_optional_entries_survive_post_filter() { let mut opt_map = ResolvedDependencyMap::new(); diff --git a/pacquet/crates/package-manager/src/install.rs b/pacquet/crates/package-manager/src/install.rs index ce9d530618..594b09ec79 100644 --- a/pacquet/crates/package-manager/src/install.rs +++ b/pacquet/crates/package-manager/src/install.rs @@ -1222,7 +1222,7 @@ where // gate at the tail. Kept out of the tuple below to avoid a // `clippy::type_complexity` annotation. let ignored_builds: Vec; - let (hoisted_dependencies, hoisted_locations, frozen_skipped, fresh_lockfile): ( + let (hoisted_dependencies, hoisted_locations, install_skipped, fresh_lockfile): ( HoistedDependencies, BTreeMap>, crate::SkippedSnapshots, @@ -1410,7 +1410,7 @@ where ( fresh_result.hoisted_dependencies, fresh_result.hoisted_locations, - crate::SkippedSnapshots::new(), + fresh_result.skipped, fresh_result.wanted_lockfile, ) }; @@ -1494,7 +1494,7 @@ where crate::prune_virtual_store::prune_virtual_store( &prune_dir, wanted.snapshots.iter().flat_map(|snapshots| snapshots.keys()), - &frozen_skipped, + &install_skipped, config.virtual_store_dir_max_length as usize, ) .is_some() @@ -1537,7 +1537,7 @@ where included, hoisted_dependencies, hoisted_locations, - &frozen_skipped, + &install_skipped, &ignored_builds, pruned_at, ), @@ -1546,7 +1546,7 @@ where let filtered_current_lockfile = if take_frozen_path { lockfile.map(|lockfile| { - crate::filter_lockfile_for_current(lockfile, included, &frozen_skipped) + crate::filter_lockfile_for_current(lockfile, included, &install_skipped) }) } else { None @@ -1581,13 +1581,12 @@ where } else if let Some(fresh_lockfile) = fresh_lockfile.as_ref() { // Fresh-install path: mirror the frozen behavior by // persisting `/lock.yaml` from the - // freshly-built wanted lockfile. No filtering needed — - // the resolver only walked the dep groups the install - // requested, so the wanted and materialized graphs match - // by construction. The save is gated on the same - // `config.lockfile` knob the wanted-side write honors - // (`fresh_lockfile` is `None` when the opt-out fired). - fresh_lockfile + // freshly-built wanted lockfile after removing snapshots + // the install skipped before materialization. The save is + // gated on the same `config.lockfile` knob the wanted-side + // write honors (`fresh_lockfile` is `None` when the + // opt-out fired). + crate::filter_lockfile_for_current(fresh_lockfile, included, &install_skipped) .save_current_to_virtual_store_dir(&config.virtual_store_dir) .map_err(InstallError::SaveCurrentLockfile)?; } diff --git a/pacquet/crates/package-manager/src/install/tests.rs b/pacquet/crates/package-manager/src/install/tests.rs index f7b85cc0ab..29ab9b6e7b 100644 --- a/pacquet/crates/package-manager/src/install/tests.rs +++ b/pacquet/crates/package-manager/src/install/tests.rs @@ -5321,6 +5321,124 @@ async fn fresh_install_marks_optional_snapshots_in_pnpm_lock_yaml() { drop((dir, mock_instance)); } +#[tokio::test] +async fn fresh_install_skips_platform_incompatible_optional_dependency() { + let mock_instance = TestRegistry::start(); + + 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"); + let mut manifest = PackageManifest::create_if_needed(manifest_path.clone()).unwrap(); + manifest + .add_dependency("@pnpm.e2e/hello-world-js-bin", "1.0.0", DependencyGroup::Prod) + .unwrap(); + manifest + .add_dependency("@pnpm.e2e/not-compatible-with-any-os", "1.0.0", DependencyGroup::Optional) + .unwrap(); + manifest.save().unwrap(); + + let mut config = Config::new(); + config.enable_global_virtual_store = false; + config.store_dir = store_dir.into(); + config.modules_dir = modules_dir.clone(); + config.virtual_store_dir = virtual_store_dir.clone(); + config.registry = mock_instance.url(); + let config = config.leak(); + + Install { + tarball_mem_cache: Default::default(), + http_client: &Default::default(), + http_client_arc: std::sync::Arc::new(Default::default()), + config, + manifest: &manifest, + lockfile: MaybeLazyLockfile::Loaded(None), + lockfile_path: None, + dependency_groups: [DependencyGroup::Prod, DependencyGroup::Dev, DependencyGroup::Optional], + frozen_lockfile: false, + prefer_frozen_lockfile: None, + ignore_manifest_check: false, + skip_runtimes: false, + trust_lockfile: false, + update_checksums: false, + is_full_install: true, + supported_architectures: None, + node_linker: pacquet_config::NodeLinker::default(), + lockfile_only: false, + dry_run: false, + resolved_packages: &Default::default(), + update_seed_policy: crate::UpdateSeedPolicy::KeepAll, + auth_override: None, + resolution_observer: None, + catalogs_override: None, + disable_optimistic_repeat_install: false, + } + .run::() + .await + .expect("install should succeed"); + + assert!( + is_symlink_or_junction(&project_root.join("node_modules/@pnpm.e2e/hello-world-js-bin")) + .unwrap(), + "compatible prod dependency should be linked", + ); + assert!( + !project_root.join("node_modules/@pnpm.e2e/not-compatible-with-any-os").exists(), + "platform-incompatible optional dependency must not be linked", + ); + assert!( + !virtual_store_dir.join("@pnpm.e2e+not-compatible-with-any-os@1.0.0").exists(), + "platform-incompatible optional dependency must not be extracted", + ); + + let lockfile_path = dir.path().join(Lockfile::FILE_NAME); + let content = std::fs::read_to_string(&lockfile_path).expect("read lockfile"); + let lockfile: Lockfile = serde_saphyr::from_str(&content).expect("parse lockfile"); + let snapshots = lockfile.snapshots.as_ref().expect("snapshots map"); + let skipped_key = snapshots + .iter() + .find(|(key, _)| { + key.name.scope.as_deref() == Some("pnpm.e2e") + && key.name.bare == "not-compatible-with-any-os" + }) + .map(|(key, snapshot)| { + assert!(snapshot.optional, "lockfile snapshot should stay marked optional"); + key.clone() + }) + .expect("optional dependency should stay in the lockfile"); + + let written = modules_dir + .pipe_as_ref(read_modules_manifest::) + .expect("read .modules.yaml") + .expect("modules manifest exists"); + assert_eq!(written.skipped, [skipped_key.to_string()]); + + let current_lockfile_path = virtual_store_dir.join(Lockfile::CURRENT_FILE_NAME); + let current_content = + std::fs::read_to_string(¤t_lockfile_path).expect("read current lockfile"); + let current_lockfile: Lockfile = + serde_saphyr::from_str(¤t_content).expect("parse current lockfile"); + assert!( + current_lockfile + .packages + .as_ref() + .is_some_and(|packages| packages.contains_key(&skipped_key.without_peer())), + "platform-incompatible optional dependency metadata must stay in current lockfile", + ); + assert!( + !current_lockfile + .snapshots + .as_ref() + .is_some_and(|snapshots| snapshots.contains_key(&skipped_key)), + "platform-incompatible optional dependency must not be saved in current lockfile", + ); + + drop((dir, mock_instance)); +} + /// `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 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 02a78950ad..99dbcc262d 100644 --- a/pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs +++ b/pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs @@ -294,6 +294,11 @@ pub enum InstallWithFreshLockfileError { #[diagnostic(transparent)] BuildPhase(#[error(source)] crate::install_frozen_lockfile::BuildPhaseError), + /// Surfaces any failure from the fresh-lockfile installability + /// pass before virtual-store materialization starts. + #[diagnostic(transparent)] + Installability(#[error(source)] Box), + /// Failed to resolve and hash `patchedDependencies` against the /// workspace directory. #[diagnostic(transparent)] @@ -430,6 +435,9 @@ pub struct InstallWithFreshLockfileResult { /// is on (the default). Empty on the `lockfile_only` path, which /// never materializes or builds. pub ignored_builds: Vec, + /// Installability-skipped optional snapshots. The outer install + /// writer persists these into `.modules.yaml.skipped`. + pub skipped: SkippedSnapshots, } impl InstallWithFreshLockfile<'_, DependencyGroupList> { @@ -788,6 +796,7 @@ impl InstallWithFreshLockfile<'_, DependencyGroupList> { verified_files_cache: &verified_files_cache, config, requester, + supported_architectures, progress_reported: &progress_reported, }, )) @@ -1287,6 +1296,7 @@ impl InstallWithFreshLockfile<'_, DependencyGroupList> { // `lockfile_only` never materializes node_modules, so no // build phase ran and nothing was ignored. ignored_builds: Vec::new(), + skipped: SkippedSnapshots::new(), }); } @@ -1385,26 +1395,45 @@ impl InstallWithFreshLockfile<'_, DependencyGroupList> { elapsed_ms = phase_start.elapsed().as_millis() as u64, "phase complete", ); - // Detect the host node once and reuse it for both the engine-name - // cache key and (under the hoisted linker) the installability - // check — mirroring the frozen path, which shares one - // `InstallabilityHost::detect` between the two instead of spawning - // `node --version` twice. Only needed when the hoisted walker runs - // the installability check; the isolated path has none yet. - let needs_installability_check = is_hoisted + let needs_optional_installability_check = + built_lockfile.packages.as_ref().is_some_and(|packages| { + built_lockfile.snapshots.as_ref().is_some_and(|snapshots| { + crate::any_optional_installability_constraint(snapshots, packages) + }) + }); + let needs_hoisted_installability_host = is_hoisted && built_lockfile.packages.as_ref().is_some_and(|packages| { built_lockfile.snapshots.as_ref().is_some_and(|snapshots| { crate::any_installability_constraint(snapshots, packages) }) }); - let host_node: Option<(bool, String)> = if needs_installability_check { - tokio::task::spawn_blocking(crate::InstallabilityHost::detect) + let needs_installability_host = + needs_optional_installability_check || needs_hoisted_installability_host; + let installability_host = if needs_installability_host { + let mut host = tokio::task::spawn_blocking(crate::InstallabilityHost::detect) .await .ok() - .map(|host| (host.node_detected, host.node_version)) + .unwrap_or_else(|| crate::InstallabilityHost { + node_version: "99999.0.0".to_string(), + node_detected: false, + os: pacquet_graph_hasher::host_platform(), + cpu: pacquet_graph_hasher::host_arch(), + libc: pacquet_graph_hasher::host_libc(), + supported_architectures: None, + engine_strict: false, + }); + if let Some(supp) = supported_architectures { + host.supported_architectures = Some(supp.clone()); + } + Some(host) } else { None }; + // Detect the host node once and reuse it for both the engine-name + // cache key and installability check, mirroring the frozen path. + let host_node: Option<(bool, String)> = installability_host + .as_ref() + .map(|host| (host.node_detected, host.node_version.clone())); // `engine_name` priority mirrors the frozen path: a `node@runtime:` // pin in the lockfile wins outright (so pinned and non-pinned @@ -1466,6 +1495,28 @@ impl InstallWithFreshLockfile<'_, DependencyGroupList> { ); } + let mut skipped = if needs_optional_installability_check { + match ( + built_lockfile.snapshots.as_ref(), + built_lockfile.packages.as_ref(), + installability_host.as_ref(), + ) { + (Some(snapshots), Some(packages), Some(host)) => { + crate::compute_skipped_snapshots::( + snapshots, + packages, + host, + requester, + SkippedSnapshots::new(), + ) + .map_err(InstallWithFreshLockfileError::Installability)? + } + _ => SkippedSnapshots::new(), + } + } else { + SkippedSnapshots::new() + }; + // Materialise the virtual store via the same phased // warm/cold-batch pipeline the frozen-lockfile path uses. The // phased pipeline in `CreateVirtualStore` runs a single @@ -1473,15 +1524,6 @@ impl InstallWithFreshLockfile<'_, DependencyGroupList> { // ~94% wall-time gap to pnpm on the full-resolution-warm scenario // without regressing the cold-cache or frozen-lockfile paths. // - // 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 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, @@ -1912,6 +1954,7 @@ impl InstallWithFreshLockfile<'_, DependencyGroupList> { wanted_lockfile, can_record_lockfile_verification, ignored_builds, + skipped, }) } } diff --git a/pacquet/crates/package-manager/src/installability.rs b/pacquet/crates/package-manager/src/installability.rs index c2db044a1a..6c3de4f95c 100644 --- a/pacquet/crates/package-manager/src/installability.rs +++ b/pacquet/crates/package-manager/src/installability.rs @@ -13,7 +13,10 @@ //! host, since the host arch may have changed since the previous //! install wrote `.modules.yaml`. -use std::collections::{HashMap, HashSet}; +use std::{ + borrow::Cow, + collections::{HashMap, HashSet}, +}; use pacquet_lockfile::{PackageKey, PackageMetadata, SnapshotEntry}; use pacquet_package_is_installable::{ @@ -24,6 +27,8 @@ use pacquet_reporter::{ LogEvent, LogLevel, Reporter, SkippedOptionalDependencyLog, SkippedOptionalPackage, SkippedOptionalReason, }; +use pacquet_resolving_resolver_base::ResolveResult; +use serde_json::Value; /// The set of snapshot keys skipped on this host. /// @@ -162,6 +167,19 @@ impl SkippedSnapshots { || self.optional_excluded.contains(key) } + #[must_use] + pub(crate) fn contains_optional_excluded(&self, key: &PackageKey) -> bool { + self.optional_excluded.contains(key) + } + + pub(crate) fn retain_installability_for_optional_snapshots( + &mut self, + snapshots: &HashMap, + ) { + self.installability + .retain(|key| snapshots.get(key).is_some_and(|snapshot| snapshot.optional)); + } + #[must_use] pub fn len(&self) -> usize { self.installability.len() + self.fetch_failed.len() + self.optional_excluded.len() @@ -252,6 +270,80 @@ impl InstallabilityHost { } } +pub(crate) fn check_installability( + package_id: &str, + manifest: &PackageInstallabilityManifest, + options: &InstallabilityOptions<'_>, +) -> Result, Box> { + let manifest = if options.optional { + manifest_with_inferred_platform(manifest) + } else { + Cow::Borrowed(manifest) + }; + check_package(package_id, manifest.as_ref(), options) + .map_err(|invalid| Box::new(InstallabilityError::InvalidNodeVersion(invalid))) +} + +pub(crate) fn manifest_with_inferred_platform( + manifest: &PackageInstallabilityManifest, +) -> Cow<'_, PackageInstallabilityManifest> { + let Some(platform) = inferred_platform( + &manifest.name, + WantedPlatformRef { + os: manifest.os.as_deref(), + cpu: manifest.cpu.as_deref(), + libc: manifest.libc.as_deref(), + }, + ) else { + return Cow::Borrowed(manifest); + }; + Cow::Owned(PackageInstallabilityManifest { + name: manifest.name.clone(), + engines: manifest.engines.clone(), + os: platform.os, + cpu: platform.cpu, + libc: platform.libc, + }) +} + +pub(crate) fn platform_manifest_from_resolve_result( + result: &ResolveResult, + fallback_alias: Option<&str>, +) -> PackageInstallabilityManifest { + let manifest = result.manifest.as_deref(); + PackageInstallabilityManifest { + name: result + .name_ver + .as_ref() + .map(|name_ver| name_ver.name.to_string()) + .or_else(|| { + manifest + .and_then(|manifest| manifest.get("name")) + .and_then(Value::as_str) + .map(ToString::to_string) + }) + .or_else(|| result.alias.clone()) + .or_else(|| fallback_alias.map(ToString::to_string)) + .unwrap_or_default(), + engines: None, + cpu: read_string_list(manifest, "cpu"), + os: read_string_list(manifest, "os"), + libc: read_string_list(manifest, "libc"), + } +} + +fn read_string_list(manifest: Option<&Value>, key: &str) -> Option> { + let value = manifest?.get(key)?; + let out: Vec = match value { + Value::String(value) => vec![value.clone()], + Value::Array(items) => { + items.iter().filter_map(Value::as_str).map(ToString::to_string).collect() + } + _ => Vec::new(), + }; + (!out.is_empty()).then_some(out) +} + /// Compute the [`SkippedSnapshots`] set for a frozen-lockfile install. /// /// For each `(snapshot_key, snapshot)`: @@ -279,8 +371,10 @@ pub fn compute_skipped_snapshots( packages: &HashMap, host: &InstallabilityHost, prefix: &str, - seed: SkippedSnapshots, + mut seed: SkippedSnapshots, ) -> Result> { + seed.retain_installability_for_optional_snapshots(snapshots); + // Fast path: if no package in the lockfile declares any // installability constraint, every snapshot is trivially // installable. Skip the per-snapshot @@ -291,9 +385,9 @@ pub fn compute_skipped_snapshots( // decomposing each metadata row only to find no constraints to // evaluate. // - // The `seed` is returned as-is on the fast path so previously - // skipped snapshots survive across reinstalls even when the - // lockfile's per-snapshot constraints have since been removed. + // The filtered `seed` is returned on the fast path so previously + // skipped optional snapshots survive across reinstalls even when + // the lockfile's per-snapshot constraints have since been removed. // // Concretely on the integrated benchmark (1352 packages with no // platform / engine constraints): drops ~1352 `String` and @@ -364,24 +458,10 @@ pub fn compute_skipped_snapshots( let warn = if let Some(cached) = check_cache.get(&cache_key) { cached.clone() } else { - let mut manifest = manifest_from_metadata(&metadata_key, metadata); - if snapshot.optional - && let Some(platform) = inferred_platform( - &manifest.name, - WantedPlatformRef { - os: manifest.os.as_deref(), - cpu: manifest.cpu.as_deref(), - libc: manifest.libc.as_deref(), - }, - ) - { - manifest.os = platform.os; - manifest.cpu = platform.cpu; - manifest.libc = platform.libc; - } + let manifest = manifest_from_metadata(&metadata_key, metadata); let pkg_id = metadata_key.to_string(); - let result = check_package(&pkg_id, &manifest, &base_options) - .map_err(|invalid| Box::new(InstallabilityError::InvalidNodeVersion(invalid)))?; + let options = InstallabilityOptions { optional: snapshot.optional, ..base_options }; + let result = check_installability(&pkg_id, &manifest, &options)?; check_cache.insert(cache_key, result.clone()); result }; @@ -443,7 +523,7 @@ pub fn any_installability_constraint( let metadata_key = snapshot_key.without_peer(); packages.get(&metadata_key).is_some_and(|metadata| { inferred_platform( - &metadata_key.name.to_string(), + metadata_key.name.bare.as_str(), WantedPlatformRef { os: metadata.os.as_deref(), cpu: metadata.cpu.as_deref(), @@ -456,6 +536,31 @@ pub fn any_installability_constraint( }) } +#[must_use] +pub fn any_optional_installability_constraint( + snapshots: &HashMap, + packages: &HashMap, +) -> bool { + snapshots.iter().any(|(snapshot_key, snapshot)| { + if !snapshot.optional { + return false; + } + let metadata_key = snapshot_key.without_peer(); + packages.get(&metadata_key).is_some_and(|metadata| { + metadata_has_meaningful_constraint(metadata) + || inferred_platform( + metadata_key.name.bare.as_str(), + WantedPlatformRef { + os: metadata.os.as_deref(), + cpu: metadata.cpu.as_deref(), + libc: metadata.libc.as_deref(), + }, + ) + .is_some() + }) + }) +} + /// True if a single metadata row carries a constraint pacquet would /// actually evaluate. fn metadata_has_meaningful_constraint(metadata: &PackageMetadata) -> bool { diff --git a/pacquet/crates/package-manager/src/installability/tests.rs b/pacquet/crates/package-manager/src/installability/tests.rs index 0745ff3e85..c5fa6f3edb 100644 --- a/pacquet/crates/package-manager/src/installability/tests.rs +++ b/pacquet/crates/package-manager/src/installability/tests.rs @@ -1,7 +1,8 @@ //! Unit tests for [`crate::installability::compute_skipped_snapshots`]. use crate::installability::{ - InstallabilityHost, SkippedSnapshots, any_installability_constraint, compute_skipped_snapshots, + InstallabilityHost, SkippedSnapshots, any_installability_constraint, + any_optional_installability_constraint, compute_skipped_snapshots, }; use pacquet_lockfile::{ LockfileResolution, PackageKey, PackageMetadata, PkgNameVerPeer, SnapshotEntry, @@ -292,6 +293,35 @@ fn meaningful_platform_value_triggers_slow_path() { ); } +#[test] +fn required_only_constraint_does_not_trigger_optional_gate() { + let key = snapshot_key("not-compatible-with-any-os@1.0.0"); + let mut snapshots = HashMap::new(); + snapshots.insert(key.clone(), SnapshotEntry { optional: false, ..Default::default() }); + let mut packages = HashMap::new(); + packages.insert(key, synthetic_metadata(None, None, Some(&["this-os-does-not-exist"]), None)); + + assert!(any_installability_constraint(&snapshots, &packages)); + assert!( + !any_optional_installability_constraint(&snapshots, &packages), + "fresh pre-pass should stay off when only required snapshots carry constraints", + ); +} + +#[test] +fn optional_constraint_triggers_optional_gate() { + let key = snapshot_key("not-compatible-with-any-os@1.0.0"); + let mut snapshots = HashMap::new(); + snapshots.insert(key.clone(), SnapshotEntry { optional: true, ..Default::default() }); + let mut packages = HashMap::new(); + packages.insert(key, synthetic_metadata(None, None, Some(&["this-os-does-not-exist"]), None)); + + assert!( + any_optional_installability_constraint(&snapshots, &packages), + "fresh pre-pass should run when an optional snapshot can be skipped", + ); +} + #[test] fn duplicate_metadata_dedupes_reporter_events() { reset_events(); @@ -421,6 +451,33 @@ fn seeded_snapshot_short_circuits_recheck() { ); } +#[test] +fn seeded_non_optional_snapshot_is_rechecked() { + reset_events(); + let key = snapshot_key("not-compatible-with-any-os@1.0.0"); + let mut snapshots = HashMap::new(); + snapshots.insert(key.clone(), SnapshotEntry { optional: false, ..Default::default() }); + let mut packages = HashMap::new(); + packages.insert(key.clone(), synthetic_metadata(None, None, Some(&["missing-os"]), None)); + + let seed = SkippedSnapshots::from_set(std::iter::once(key.clone()).collect()); + let skipped = compute_skipped_snapshots::( + &snapshots, + &packages, + &host("20.10.0", "darwin", "arm64"), + "/proj", + seed, + ) + .unwrap(); + + assert!(!skipped.contains(&key), "non-optional snapshots must not stay seeded as skipped"); + let events = take_events(); + assert!( + events.iter().all(|event| !matches!(event, LogEvent::SkippedOptionalDependency(_))), + "non-optional incompatibility must not emit skipped-optional events, got {events:?}", + ); +} + #[test] fn fast_path_preserves_seed() { reset_events(); @@ -444,6 +501,27 @@ fn fast_path_preserves_seed() { assert!(skipped.contains(&key)); } +#[test] +fn fast_path_drops_seed_for_non_optional_snapshot() { + let key = snapshot_key("previously-skipped@1.0.0"); + let mut snapshots = HashMap::new(); + snapshots.insert(key.clone(), SnapshotEntry { optional: false, ..Default::default() }); + let mut packages = HashMap::new(); + packages.insert(key.clone(), synthetic_metadata(None, None, None, None)); + + let seed = SkippedSnapshots::from_set(std::iter::once(key).collect()); + let skipped = compute_skipped_snapshots::( + &snapshots, + &packages, + &host("20.10.0", "darwin", "arm64"), + "/proj", + seed, + ) + .unwrap(); + + assert!(skipped.is_empty()); +} + #[test] fn from_strings_skips_unparsable_entries() { let set = SkippedSnapshots::from_strings([ @@ -591,6 +669,10 @@ fn name_inferable_optional_snapshot_triggers_slow_path() { any_installability_constraint(&snapshots, &packages), "a platform-named optional snapshot must trigger the slow path", ); + assert!( + any_optional_installability_constraint(&snapshots, &packages), + "a platform-named optional snapshot must trigger the fresh pre-pass", + ); } #[test] diff --git a/pacquet/crates/package-manager/src/prefetching_resolver.rs b/pacquet/crates/package-manager/src/prefetching_resolver.rs index 60e1f0e90c..6d56ea8b12 100644 --- a/pacquet/crates/package-manager/src/prefetching_resolver.rs +++ b/pacquet/crates/package-manager/src/prefetching_resolver.rs @@ -30,6 +30,9 @@ use dashmap::{DashMap, DashSet}; use pacquet_config::Config; use pacquet_lockfile::{LockfileResolution, is_git_hosted_tarball_url}; use pacquet_network::{AuthHeaders, ThrottledClient}; +use pacquet_package_is_installable::{ + SupportedArchitectures, WantedPlatformRef, platform_is_supported, +}; use pacquet_reporter::{Reporter, SilentReporter}; use pacquet_resolving_resolver_base::{ LatestQuery, ResolveError, ResolveFuture, ResolveLatestFuture, ResolveOptions, ResolveResult, @@ -61,6 +64,7 @@ pub struct PrefetchContext<'a> { pub verified_files_cache: &'a SharedVerifiedFilesCache, pub config: &'static Config, pub requester: &'a str, + pub supported_architectures: Option<&'a SupportedArchitectures>, /// Install-scoped set the background download records when it emits /// a package-status progress event. The later warm/cold install pass /// consults the set so prefetch progress is visible immediately @@ -85,6 +89,10 @@ struct OwnedFetchCtx { requester: Arc, offline: bool, verify_store_integrity: bool, + supported_architectures: Option, + current_os: &'static str, + current_cpu: &'static str, + current_libc: &'static str, progress_reported: SharedReportedProgressKeys, /// Set of URLs that already had a prefetch task spawned, used as /// an atomic check-and-claim gate so concurrent resolves for the @@ -139,6 +147,7 @@ impl PrefetchingResolver { verified_files_cache, config, requester, + supported_architectures, progress_reported, } = prefetch_ctx; let ctx = OwnedFetchCtx { @@ -153,6 +162,10 @@ impl PrefetchingResolver { requester: Arc::::from(requester), offline: config.offline, verify_store_integrity: config.verify_store_integrity, + supported_architectures: supported_architectures.cloned(), + current_os: pacquet_graph_hasher::host_platform(), + current_cpu: pacquet_graph_hasher::host_arch(), + current_libc: pacquet_graph_hasher::host_libc(), progress_reported: SharedReportedProgressKeys::clone(progress_reported), spawned_urls: Arc::new(DashSet::new()), integrity_cache: Arc::new(DashMap::new()), @@ -324,6 +337,32 @@ impl PrefetchingResolver { .await; }); } + + fn should_skip_prefetch( + &self, + wanted_dependency: &WantedDependency, + result: &ResolveResult, + ) -> bool { + if wanted_dependency.optional != Some(true) || !is_remote_tarball(&result.resolution) { + return false; + } + let manifest = crate::platform_manifest_from_resolve_result( + result, + wanted_dependency.alias.as_deref(), + ); + let manifest = crate::manifest_with_inferred_platform(&manifest); + !platform_is_supported( + WantedPlatformRef { + os: manifest.os.as_deref(), + cpu: manifest.cpu.as_deref(), + libc: manifest.libc.as_deref(), + }, + self.ctx.supported_architectures.as_ref(), + self.ctx.current_os, + self.ctx.current_cpu, + self.ctx.current_libc, + ) + } } impl Resolver for PrefetchingResolver { @@ -336,7 +375,9 @@ impl Resolver for PrefetchingResolver Resolver for PrefetchingResolver bool { + let LockfileResolution::Tarball(tarball) = resolution else { return false }; + !tarball.tarball.starts_with("file:") && !is_git_hosted_tarball_url(&tarball.tarball) +} + +#[cfg(test)] +mod tests; diff --git a/pacquet/crates/package-manager/src/prefetching_resolver/tests.rs b/pacquet/crates/package-manager/src/prefetching_resolver/tests.rs new file mode 100644 index 0000000000..6a0b4befd8 --- /dev/null +++ b/pacquet/crates/package-manager/src/prefetching_resolver/tests.rs @@ -0,0 +1,300 @@ +use super::PrefetchingResolver; +use crate::PrefetchContext; +use pacquet_config::Config; +use pacquet_lockfile::{DirectoryResolution, LockfileResolution, TarballResolution}; +use pacquet_network::ThrottledClient; +use pacquet_reporter::SilentReporter; +use pacquet_resolving_default_resolver::DefaultResolver; +use pacquet_resolving_resolver_base::{ + LatestQuery, ResolveFuture, ResolveLatestFuture, ResolveOptions, ResolveResult, Resolver, + WantedDependency, +}; +use pacquet_store_dir::{SharedVerifiedFilesCache, StoreIndexWriter}; +use pacquet_tarball::{MemCache, SharedReportedProgressKeys}; +use serde_json::json; +use std::{io::Write, path::Path, sync::Arc}; +use tempfile::tempdir; + +fn result_with_manifest(name: &str, manifest: serde_json::Value) -> ResolveResult { + let id = format!("{name}@1.0.0"); + ResolveResult { + id: id.clone().into(), + name_ver: Some(id.parse().unwrap()), + latest: None, + published_at: None, + manifest: Some(Arc::new(manifest)), + resolution: LockfileResolution::Tarball(TarballResolution { + integrity: None, + tarball: "https://registry.example/not-compatible.tgz".to_string(), + git_hosted: None, + path: None, + }), + resolved_via: "npm-registry".to_string(), + normalized_bare_specifier: None, + alias: None, + policy_violation: None, + } +} + +fn result_without_manifest(name: &str) -> ResolveResult { + let mut result = result_with_manifest(name, json!({})); + result.manifest = None; + result +} + +fn alias_tarball_result(alias: &str, manifest: serde_json::Value) -> ResolveResult { + ResolveResult { + id: "https://registry.example/not-compatible.tgz".into(), + name_ver: None, + latest: None, + published_at: None, + manifest: Some(Arc::new(manifest)), + resolution: LockfileResolution::Tarball(TarballResolution { + integrity: None, + tarball: "https://registry.example/not-compatible.tgz".to_string(), + git_hosted: None, + path: None, + }), + resolved_via: "tarball".to_string(), + normalized_bare_specifier: None, + alias: Some(alias.to_string()), + policy_violation: None, + } +} + +fn anonymous_tarball_result(manifest: serde_json::Value) -> ResolveResult { + ResolveResult { + id: "https://registry.example/not-compatible.tgz".into(), + name_ver: None, + latest: None, + published_at: None, + manifest: Some(Arc::new(manifest)), + resolution: LockfileResolution::Tarball(TarballResolution { + integrity: None, + tarball: "https://registry.example/not-compatible.tgz".to_string(), + git_hosted: None, + path: None, + }), + resolved_via: "tarball".to_string(), + normalized_bare_specifier: None, + alias: None, + policy_violation: None, + } +} + +#[derive(Clone)] +struct FixedResolver { + result: ResolveResult, +} + +impl Resolver for FixedResolver { + fn resolve<'a>( + &'a self, + _wanted_dependency: &'a WantedDependency, + _opts: &'a ResolveOptions, + ) -> ResolveFuture<'a> { + let result = self.result.clone(); + Box::pin(async move { Ok(Some(result)) }) + } + + fn resolve_latest<'a>( + &'a self, + _query: &'a LatestQuery, + _opts: &'a ResolveOptions, + ) -> ResolveLatestFuture<'a> { + Box::pin(async { Ok(None) }) + } +} + +fn resolver_with_inner( + dir: &Path, + inner: Box, +) -> PrefetchingResolver { + let mut config = Config::new(); + config.store_dir = dir.join("store").into(); + config.cache_dir = dir.join("cache"); + let config = Box::leak(Box::new(config)); + let http_client = Arc::new(ThrottledClient::default()); + let mem_cache = Arc::new(MemCache::default()); + let (store_index_writer, _writer_task) = StoreIndexWriter::spawn_disabled(); + PrefetchingResolver::new( + inner, + PrefetchContext { + http_client: &http_client, + mem_cache: &mem_cache, + store_index: None, + store_index_writer: Some(&store_index_writer), + verified_files_cache: &SharedVerifiedFilesCache::default(), + config, + requester: "/project", + supported_architectures: None, + progress_reported: &SharedReportedProgressKeys::default(), + }, + ) +} + +fn resolver() -> PrefetchingResolver { + let dir = tempdir().unwrap(); + resolver_with_inner(dir.path(), Box::new(DefaultResolver::new(Vec::new()))) +} + +fn minimal_tarball(name: &str, version: &str) -> Vec { + let manifest = serde_json::json!({ "name": name, "version": version }).to_string(); + let manifest = manifest.as_bytes(); + + let mut builder = tar::Builder::new(Vec::new()); + let mut header = tar::Header::new_gnu(); + header.set_path("package/package.json").expect("set tar entry path"); + header.set_size(manifest.len() as u64); + header.set_mode(0o644); + header.set_cksum(); + builder.append(&header, manifest).expect("append package.json to tar"); + let tar_bytes = builder.into_inner().expect("finish tar"); + + let mut encoder = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::default()); + encoder.write_all(&tar_bytes).expect("gzip tar"); + encoder.finish().expect("finish gzip") +} + +#[tokio::test] +async fn skips_prefetch_for_unsupported_optional_manifest() { + let resolver = resolver(); + let wanted = WantedDependency { optional: Some(true), ..WantedDependency::default() }; + let result = result_with_manifest( + "@pnpm.e2e/not-compatible-with-any-os", + json!({ + "name": "@pnpm.e2e/not-compatible-with-any-os", + "version": "1.0.0", + "os": ["this-os-does-not-exist"] + }), + ); + + assert!(resolver.should_skip_prefetch(&wanted, &result)); +} + +#[tokio::test] +async fn skips_prefetch_for_platform_inferred_from_optional_name() { + let resolver = resolver(); + let wanted = WantedDependency { optional: Some(true), ..WantedDependency::default() }; + let result = result_with_manifest( + "@esbuild/openharmony-arm64", + json!({ + "name": "@esbuild/openharmony-arm64", + "version": "1.0.0" + }), + ); + + assert!(resolver.should_skip_prefetch(&wanted, &result)); +} + +#[tokio::test] +async fn skips_prefetch_for_manifestless_platform_inferred_name() { + let resolver = resolver(); + let wanted = WantedDependency { optional: Some(true), ..WantedDependency::default() }; + let result = result_without_manifest("@esbuild/openharmony-arm64"); + + assert!(resolver.should_skip_prefetch(&wanted, &result)); +} + +#[tokio::test] +async fn skips_prefetch_using_alias_when_manifest_name_missing() { + let resolver = resolver(); + let wanted = WantedDependency { + alias: Some("@esbuild/openharmony-arm64".to_string()), + optional: Some(true), + ..WantedDependency::default() + }; + let result = alias_tarball_result("@esbuild/openharmony-arm64", json!({ "version": "1.0.0" })); + + assert!(resolver.should_skip_prefetch(&wanted, &result)); +} + +#[tokio::test] +async fn skips_prefetch_for_anonymous_manifest_with_explicit_platform_constraint() { + let resolver = resolver(); + let wanted = WantedDependency { optional: Some(true), ..WantedDependency::default() }; + let result = anonymous_tarball_result(json!({ + "version": "1.0.0", + "os": ["this-os-does-not-exist"] + })); + + assert!(resolver.should_skip_prefetch(&wanted, &result)); +} + +#[tokio::test] +async fn resolve_populates_integrity_before_skipping_optional_prefetch() { + let dir = tempdir().unwrap(); + let mut server = mockito::Server::new_async().await; + let tarball_path = "/not-compatible-1.0.0.tgz"; + let tarball_url = format!("{}{tarball_path}", server.url()); + let get_mock = server + .mock("GET", tarball_path) + .with_status(200) + .with_body(minimal_tarball("not-compatible", "1.0.0")) + .expect(1) + .create_async() + .await; + let mut result = result_with_manifest( + "not-compatible", + json!({ + "name": "not-compatible", + "version": "1.0.0", + "os": ["this-os-does-not-exist"] + }), + ); + result.resolution = LockfileResolution::Tarball(TarballResolution { + integrity: None, + tarball: tarball_url, + git_hosted: None, + path: None, + }); + let resolver = resolver_with_inner(dir.path(), Box::new(FixedResolver { result })); + let wanted = WantedDependency { optional: Some(true), ..WantedDependency::default() }; + + let resolved = resolver + .resolve(&wanted, &ResolveOptions::default()) + .await + .expect("resolve succeeds") + .expect("resolver returns a result"); + + let LockfileResolution::Tarball(tarball) = resolved.resolution else { + panic!("expected tarball resolution"); + }; + assert!(tarball.integrity.is_some(), "unsupported optional tarball still needs integrity"); + get_mock.assert_async().await; +} + +#[tokio::test] +async fn keeps_prefetch_check_off_non_tarball_resolutions() { + let resolver = resolver(); + let wanted = WantedDependency { optional: Some(true), ..WantedDependency::default() }; + let mut result = result_with_manifest( + "@pnpm.e2e/not-compatible-with-any-os", + json!({ + "name": "@pnpm.e2e/not-compatible-with-any-os", + "version": "1.0.0", + "os": ["this-os-does-not-exist"] + }), + ); + result.resolution = LockfileResolution::Directory(DirectoryResolution { + directory: "../not-compatible".to_string(), + }); + + assert!(!resolver.should_skip_prefetch(&wanted, &result)); +} + +#[tokio::test] +async fn keeps_prefetch_for_required_manifest() { + let resolver = resolver(); + let wanted = WantedDependency { optional: Some(false), ..WantedDependency::default() }; + let result = result_with_manifest( + "@pnpm.e2e/not-compatible-with-any-os", + json!({ + "name": "@pnpm.e2e/not-compatible-with-any-os", + "version": "1.0.0", + "os": ["this-os-does-not-exist"] + }), + ); + + assert!(!resolver.should_skip_prefetch(&wanted, &result)); +}