diff --git a/pacquet/crates/config/src/defaults.rs b/pacquet/crates/config/src/defaults.rs index 8ed5c0eefc..2a86b0a56f 100644 --- a/pacquet/crates/config/src/defaults.rs +++ b/pacquet/crates/config/src/defaults.rs @@ -105,5 +105,60 @@ pub fn default_fetch_retry_maxtimeout() -> u64 { 60_000 } +/// Default `childConcurrency` matching upstream's +/// [`getDefaultWorkspaceConcurrency`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/concurrency.ts#L21-L23): +/// `min(4, availableParallelism())`. Read at runtime so `cargo test` +/// and overrides via yaml still resolve to a usable value on +/// 1-core sandboxes. +pub fn default_child_concurrency() -> u32 { + default_child_concurrency_with_parallelism(available_parallelism()) +} + +/// Internal helper exposed for tests so they can pin the +/// `parallelism` input. Upstream's test suite mocks +/// `os.availableParallelism` via Jest; pacquet injects the value +/// directly. Mirrors upstream's [`getDefaultWorkspaceConcurrency`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/concurrency.ts#L21-L23). +pub fn default_child_concurrency_with_parallelism(parallelism: u32) -> u32 { + parallelism.min(4) +} + +/// Available CPU parallelism, mirroring upstream's +/// [`getAvailableParallelism`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/concurrency.ts#L5-L13). +/// Floors at 1. +pub fn available_parallelism() -> u32 { + std::thread::available_parallelism().map(|n| n.get() as u32).unwrap_or(1).max(1) +} + +/// Resolve `childConcurrency` from a possibly-negative yaml value +/// to a concrete `u32`. Mirrors upstream's +/// [`getWorkspaceConcurrency`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/concurrency.ts#L25-L34): +/// +/// - `None` → default (`min(4, parallelism)`). +/// - Positive `n` → `n`. +/// - Zero or negative `n` → `max(1, parallelism - |n|)`. +/// +/// The negative-offset semantics let users say "use all cores minus +/// N" without hardcoding the core count. +pub fn resolve_child_concurrency(option: Option) -> u32 { + resolve_child_concurrency_with_parallelism(option, available_parallelism()) +} + +/// Internal helper exposed for tests so they can pin the +/// `parallelism` input. Mirrors upstream's +/// [`getWorkspaceConcurrency`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/concurrency.ts#L25-L34) +/// — the resolver logic itself, with the parallelism input +/// injected rather than read from the OS. +pub fn resolve_child_concurrency_with_parallelism(option: Option, parallelism: u32) -> u32 { + match option { + None => default_child_concurrency_with_parallelism(parallelism), + Some(n) if n > 0 => n as u32, + // `unsigned_abs` instead of `(-n) as u32` — the latter + // panics in debug builds on `n == i32::MIN` (negation + // overflow); the former returns `i32::MAX as u32 + 1` + // safely. + Some(n) => parallelism.saturating_sub(n.unsigned_abs()).max(1), + } +} + #[cfg(test)] mod tests; diff --git a/pacquet/crates/config/src/defaults/tests.rs b/pacquet/crates/config/src/defaults/tests.rs index cf1da78075..adb4c89248 100644 --- a/pacquet/crates/config/src/defaults/tests.rs +++ b/pacquet/crates/config/src/defaults/tests.rs @@ -1,4 +1,7 @@ -use super::default_store_dir; +use super::{ + default_child_concurrency_with_parallelism, default_store_dir, resolve_child_concurrency, + resolve_child_concurrency_with_parallelism, +}; use crate::test_env_guard::EnvGuard; use pacquet_store_dir::StoreDir; use pretty_assertions::assert_eq; @@ -46,6 +49,96 @@ fn test_default_store_dir_with_xdg_env() { assert_eq!(display_store_dir(&store_dir), "/tmp/xdg_data_home/pnpm/store"); } +/// Port of upstream +/// [`'getDefaultWorkspaceConcurrency: cpu num < 4'`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/concurrency.test.ts#L25-L28). +/// On a 1-core host, the default caps at 1 (not 4). +#[test] +fn default_child_concurrency_with_parallelism_below_four() { + assert_eq!(default_child_concurrency_with_parallelism(1), 1); +} + +/// Port of upstream +/// [`'getDefaultWorkspaceConcurrency: cpu num > 4'`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/concurrency.test.ts#L30-L33). +/// Caps at 4 on a 5-core host. +#[test] +fn default_child_concurrency_with_parallelism_above_four() { + assert_eq!(default_child_concurrency_with_parallelism(5), 4); +} + +/// Port of upstream +/// [`'getDefaultWorkspaceConcurrency: cpu num = 4'`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/concurrency.test.ts#L35-L38). +/// At the boundary, 4 is the exact result (not floored or capped). +#[test] +fn default_child_concurrency_with_parallelism_at_four() { + assert_eq!(default_child_concurrency_with_parallelism(4), 4); +} + +/// Port of upstream +/// [`'default workspace concurrency'`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/concurrency.test.ts#L48-L52). +/// `getWorkspaceConcurrency(undefined)` on a >=4-core host yields 4 +/// (the upstream test runs on the default Jest host; on a host with +/// >=4 cores the default is 4). Pin a >=4 parallelism so the +/// expectation is deterministic. +#[test] +fn resolve_child_concurrency_default_with_four_or_more_cores() { + assert_eq!(resolve_child_concurrency_with_parallelism(None, 4), 4); + assert_eq!(resolve_child_concurrency_with_parallelism(None, 8), 4); +} + +/// Port of upstream +/// [`'match host cores amount'`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/concurrency.test.ts#L58-L62). +/// `getWorkspaceConcurrency(0)` returns the host's parallelism +/// verbatim — the saturated `parallelism - 0` path. +#[test] +fn resolve_child_concurrency_zero_returns_full_parallelism() { + assert_eq!(resolve_child_concurrency_with_parallelism(Some(0), 8), 8); + assert_eq!(resolve_child_concurrency_with_parallelism(Some(0), 1), 1); +} + +/// Port of upstream +/// [`'host cores minus X'`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/concurrency.test.ts#L64-L71). +/// `n = -1` → `max(1, cores - 1)`; `n = -9999` → `1` (saturating). +/// Replaces the earlier bound-check-only test with the precise +/// formula that the upstream suite pins. +#[test] +fn resolve_child_concurrency_negative_offset_matches_upstream_formula() { + // n = -1 with 8 cores → 7. + assert_eq!(resolve_child_concurrency_with_parallelism(Some(-1), 8), 7); + // n = -1 with 1 core → max(1, 0) → 1. + assert_eq!(resolve_child_concurrency_with_parallelism(Some(-1), 1), 1); + // n = -9999 saturates → 1 regardless of parallelism. + assert_eq!(resolve_child_concurrency_with_parallelism(Some(-9999), 8), 1); + assert_eq!(resolve_child_concurrency_with_parallelism(Some(-9999), 1), 1); +} + +/// Existing pacquet test (not from upstream): both the public +/// `resolve_child_concurrency` and the testable +/// `_with_parallelism` helper agree on positive inputs. The +/// upstream +/// [`'get back positive amount'`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/concurrency.test.ts#L54-L56) +/// case (`n = 5` → `5`) is checked here alongside the helper +/// equivalence. +#[test] +fn resolve_child_concurrency_positive_amount() { + assert_eq!(resolve_child_concurrency(Some(5)), 5); + assert_eq!(resolve_child_concurrency_with_parallelism(Some(5), 1), 5); + assert_eq!(resolve_child_concurrency_with_parallelism(Some(5), 100), 5); +} + +/// `resolve_child_concurrency(Some(i32::MIN))` must not panic. +/// A naive `(-n) as u32` overflows in debug builds when +/// `n == i32::MIN` because the negation itself overflows; +/// `unsigned_abs` is the safe path. `i32::MIN.unsigned_abs()` +/// is `2_147_483_648`, well above any plausible host +/// parallelism, so `saturating_sub` produces `0` and `.max(1)` +/// lifts to exactly `1` — assert that precise value so a wrong +/// result like `2` would still fail the test. +#[test] +fn resolve_child_concurrency_handles_i32_min() { + let result = resolve_child_concurrency(Some(i32::MIN)); + assert_eq!(result, 1); +} + #[cfg(windows)] #[test] fn test_should_get_the_correct_drive_letter() { diff --git a/pacquet/crates/config/src/lib.rs b/pacquet/crates/config/src/lib.rs index a43adc8e63..3ce7a9ef8c 100644 --- a/pacquet/crates/config/src/lib.rs +++ b/pacquet/crates/config/src/lib.rs @@ -12,11 +12,12 @@ use serde::Deserialize; use smart_default::SmartDefault; use std::{collections::HashMap, fs, path::PathBuf}; +pub use crate::defaults::{available_parallelism, resolve_child_concurrency}; use crate::defaults::{ - default_fetch_retries, default_fetch_retry_factor, default_fetch_retry_maxtimeout, - default_fetch_retry_mintimeout, default_hoist_pattern, default_modules_cache_max_age, - default_modules_dir, default_public_hoist_pattern, default_registry, default_store_dir, - default_virtual_store_dir, + default_child_concurrency, default_fetch_retries, default_fetch_retry_factor, + default_fetch_retry_maxtimeout, default_fetch_retry_mintimeout, default_hoist_pattern, + default_modules_cache_max_age, default_modules_dir, default_public_hoist_pattern, + default_registry, default_store_dir, default_virtual_store_dir, }; pub use workspace_yaml::{ LoadWorkspaceYamlError, WORKSPACE_MANIFEST_FILENAME, WorkspaceSettings, workspace_root_or, @@ -39,6 +40,61 @@ pub enum NodeLinker { Pnp, } +/// Tri-state mirror of `pacquet_executor::ScriptsPrependNodePath` +/// with serde wiring. The executor crate keeps its own enum free of +/// serde so config concerns don't leak into the spawn-path. Converted +/// at the `BuildModules` call site (see `install_frozen_lockfile.rs`) +/// via an explicit `match`; no `From` impl exists because neither +/// crate depends on the other, and adding such a dep just for the +/// conversion would invert the layering. Both enums share the same +/// three variants so the match is exhaustive and one-line per arm. +/// +/// Deserializes the upstream `scriptsPrependNodePath: boolean | 'warn-only'` +/// yaml shape ([`Config.scriptsPrependNodePath`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/Config.ts#L108)). +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub enum ScriptsPrependNodePath { + /// `scriptsPrependNodePath: true` — always prepend. + Always, + /// `scriptsPrependNodePath: false` (or absent) — never prepend. + #[default] + Never, + /// `scriptsPrependNodePath: 'warn-only'` — emit a warning if the + /// node in PATH differs from the running interpreter, do not + /// prepend. + WarnOnly, +} + +impl<'de> serde::Deserialize<'de> for ScriptsPrependNodePath { + fn deserialize(d: D) -> Result + where + D: serde::Deserializer<'de>, + { + use serde::de::{self, Visitor}; + use std::fmt; + + struct V; + impl<'de> Visitor<'de> for V { + type Value = ScriptsPrependNodePath; + fn expecting(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str("a boolean or the string \"warn-only\"") + } + fn visit_bool(self, v: bool) -> Result { + Ok(if v { ScriptsPrependNodePath::Always } else { ScriptsPrependNodePath::Never }) + } + fn visit_str(self, v: &str) -> Result { + match v { + "warn-only" => Ok(ScriptsPrependNodePath::WarnOnly), + other => Err(E::invalid_value( + de::Unexpected::Str(other), + &"true, false, or \"warn-only\"", + )), + } + } + } + d.deserialize_any(V) + } +} + #[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Deserialize)] #[serde(rename_all = "kebab-case")] pub enum PackageImportMethod { @@ -282,6 +338,50 @@ pub struct Config { /// `true`, every package may run lifecycle scripts regardless of /// `allow_builds`. Default `false` to match pnpm v11. pub dangerously_allow_all_builds: bool, + + /// `scriptsPrependNodePath` from `pnpm-workspace.yaml`. Controls + /// whether `dirname(node_execpath)` is prepended to `PATH` when + /// running lifecycle scripts. Default `Never` to match pnpm's + /// [`StrictBuildOptions.scriptsPrependNodePath: false`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/after-install/src/extendBuildOptions.ts#L78). + /// Yaml accepts `true` / `false` / `"warn-only"`. + pub scripts_prepend_node_path: ScriptsPrependNodePath, + + /// `unsafePerm` from `pnpm-workspace.yaml`. When `false`, pnpm + /// runs lifecycle scripts under a TMPDIR isolated to + /// `node_modules/.tmp` and (in upstream) drops uid/gid to a + /// non-root user. Default `true` here. Pacquet honors the + /// TMPDIR side of the upstream behavior (see + /// `pacquet_executor::make_env`); the uid/gid drop is a no-op + /// in practice because pnpm's npm-lifecycle fork never + /// populates `opts.user` / `opts.group`, so even upstream just + /// re-applies the current process's uid/gid. + /// + /// Pacquet's default deviates slightly from upstream's + /// [`StrictBuildOptions.unsafePerm`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/after-install/src/extendBuildOptions.ts#L83-L86) + /// which auto-detects root-on-POSIX (`getuid() === 0 && setgid`) + /// and flips to `false`. Adding the auto-detect requires `libc` + /// (not currently in `[workspace.dependencies]`); for now, + /// root-run CI must set `unsafePerm: false` in yaml explicitly. + /// On Windows, [`WorkspaceSettings::apply_to`] forces this to + /// `true` regardless of yaml — matching upstream's + /// `process.platform === 'win32'` override at + /// [`@pnpm/npm-lifecycle/index.js:204-220`](https://github.com/pnpm/npm-lifecycle/blob/d2d8e790/index.js#L204-L220). + #[default = true] + pub unsafe_perm: bool, + + /// `childConcurrency` from `pnpm-workspace.yaml` — the maximum + /// number of lifecycle-script spawns that may run in parallel + /// inside a single `BuildModules` chunk. Resolved through + /// [`resolve_child_concurrency`] so the yaml value can be + /// negative (interpreted as `parallelism - |value|`). + /// + /// Default: `min(4, availableParallelism())`, matching upstream's + /// [`getDefaultWorkspaceConcurrency`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/concurrency.ts#L21-L23). + /// Chunks run sequentially (children before parents); only + /// members within a chunk are parallelized — same as upstream's + /// [`runGroups(getWorkspaceConcurrency(opts.childConcurrency), groups)`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/during-install/src/index.ts#L124). + #[default(_code = "default_child_concurrency()")] + pub child_concurrency: u32, } impl Config { diff --git a/pacquet/crates/config/src/workspace_yaml.rs b/pacquet/crates/config/src/workspace_yaml.rs index d7c32361b4..c65d50d3fd 100644 --- a/pacquet/crates/config/src/workspace_yaml.rs +++ b/pacquet/crates/config/src/workspace_yaml.rs @@ -1,4 +1,6 @@ -use crate::{Config, NodeLinker, PackageImportMethod}; +use crate::{ + Config, NodeLinker, PackageImportMethod, ScriptsPrependNodePath, resolve_child_concurrency, +}; use derive_more::{Display, Error}; use indexmap::IndexMap; use miette::Diagnostic; @@ -104,6 +106,22 @@ pub struct WorkspaceSettings { /// /// [`allow_builds`]: Self::allow_builds pub dangerously_allow_all_builds: Option, + + /// `scriptsPrependNodePath` from `pnpm-workspace.yaml`. Tri-state + /// — yaml accepts `true` / `false` / `"warn-only"`. Custom serde + /// shape, see [`ScriptsPrependNodePath`]'s `Deserialize` impl. + pub scripts_prepend_node_path: Option, + + /// `unsafePerm` from `pnpm-workspace.yaml`. Forced to `true` on + /// Windows in `apply_to` (matches upstream's + /// `process.platform === 'win32'` override). + pub unsafe_perm: Option, + + /// `childConcurrency` from `pnpm-workspace.yaml`. Resolved + /// through [`crate::resolve_child_concurrency`] in `apply_to`. + /// Signed `i32` here so negative values (interpreted as + /// `parallelism - |value|`) round-trip cleanly. + pub child_concurrency: Option, } /// Basename of the file pnpm reads; exported for test use. @@ -223,6 +241,26 @@ impl WorkspaceSettings { if let Some(v) = self.dangerously_allow_all_builds { config.dangerously_allow_all_builds = v; } + if let Some(v) = self.scripts_prepend_node_path { + config.scripts_prepend_node_path = v; + } + if let Some(v) = self.unsafe_perm { + config.unsafe_perm = v; + } + // Windows force-override (matches upstream's + // [`process.platform === 'win32'`](https://github.com/pnpm/npm-lifecycle/blob/d2d8e790/index.js#L204-L220) + // — running lifecycle scripts under a uid/gid drop is + // POSIX-only). + if cfg!(windows) { + config.unsafe_perm = true; + } + // `childConcurrency: None` keeps the smart-default + // `Config` constructor produced from + // [`default_child_concurrency`]; `Some(n)` (including + // negative) goes through the upstream resolver. + if let Some(v) = self.child_concurrency { + config.child_concurrency = resolve_child_concurrency(Some(v)); + } } } diff --git a/pacquet/crates/config/src/workspace_yaml/tests.rs b/pacquet/crates/config/src/workspace_yaml/tests.rs index 56c3bd3ce7..ae1d45bcae 100644 --- a/pacquet/crates/config/src/workspace_yaml/tests.rs +++ b/pacquet/crates/config/src/workspace_yaml/tests.rs @@ -1,5 +1,5 @@ use super::{LoadWorkspaceYamlError, WORKSPACE_MANIFEST_FILENAME, WorkspaceSettings}; -use crate::{Config, NodeLinker}; +use crate::{Config, NodeLinker, ScriptsPrependNodePath}; use pacquet_store_dir::StoreDir; use pipe_trait::Pipe; use pretty_assertions::assert_eq; @@ -276,6 +276,109 @@ fn parses_dangerously_allow_all_builds_from_yaml_and_applies() { assert!(config.dangerously_allow_all_builds); } +/// `scriptsPrependNodePath` is the tri-state from upstream +/// [`Config.scriptsPrependNodePath: boolean | 'warn-only'`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/Config.ts#L108). +/// `true` → Always, `false` → Never, `"warn-only"` → WarnOnly. +/// Pacquet's default is Never (matches upstream's +/// [`StrictBuildOptions.scriptsPrependNodePath: false`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/after-install/src/extendBuildOptions.ts#L78)). +#[test] +fn parses_scripts_prepend_node_path_true_from_yaml() { + let yaml = "scriptsPrependNodePath: true\n"; + let settings: WorkspaceSettings = serde_saphyr::from_str(yaml).unwrap(); + assert_eq!(settings.scripts_prepend_node_path, Some(ScriptsPrependNodePath::Always)); + + let mut config = Config::new(); + assert_eq!(config.scripts_prepend_node_path, ScriptsPrependNodePath::Never, "default Never"); + settings.apply_to(&mut config, Path::new("/irrelevant")); + assert_eq!(config.scripts_prepend_node_path, ScriptsPrependNodePath::Always); +} + +#[test] +fn parses_scripts_prepend_node_path_false_from_yaml() { + let yaml = "scriptsPrependNodePath: false\n"; + let settings: WorkspaceSettings = serde_saphyr::from_str(yaml).unwrap(); + assert_eq!(settings.scripts_prepend_node_path, Some(ScriptsPrependNodePath::Never)); +} + +#[test] +fn parses_scripts_prepend_node_path_warn_only_from_yaml() { + let yaml = "scriptsPrependNodePath: warn-only\n"; + let settings: WorkspaceSettings = serde_saphyr::from_str(yaml).unwrap(); + assert_eq!(settings.scripts_prepend_node_path, Some(ScriptsPrependNodePath::WarnOnly)); +} + +#[test] +fn rejects_invalid_scripts_prepend_node_path() { + let yaml = "scriptsPrependNodePath: nonsense\n"; + serde_saphyr::from_str::(yaml).expect_err("must reject"); +} + +/// `unsafePerm` from yaml flips the default-`true` field. Mirrors +/// upstream's [`Config.unsafePerm: boolean`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/Config.ts). +/// Pacquet's auto-root-detect default is a follow-up — for now, +/// yaml override is the only way to flip the flag on POSIX. +#[test] +fn parses_unsafe_perm_from_yaml_and_applies() { + // POSIX-only: the Windows force-override below would mask this + // test's behavior. See [`WorkspaceSettings::apply_to`]. + if cfg!(windows) { + return; + } + let yaml = "unsafePerm: false\n"; + let settings: WorkspaceSettings = serde_saphyr::from_str(yaml).unwrap(); + assert_eq!(settings.unsafe_perm, Some(false)); + + let mut config = Config::new(); + assert!(config.unsafe_perm, "default is true"); + settings.apply_to(&mut config, Path::new("/irrelevant")); + assert!(!config.unsafe_perm, "yaml override wins on POSIX"); +} + +/// On Windows, `apply_to` ignores the yaml value and forces +/// `unsafe_perm = true`. Mirrors upstream's +/// [`process.platform === 'win32'` override](https://github.com/pnpm/npm-lifecycle/blob/d2d8e790/index.js#L204-L220) +/// — running lifecycle scripts under a uid/gid drop is POSIX-only. +#[cfg(windows)] +#[test] +fn unsafe_perm_force_true_on_windows() { + let yaml = "unsafePerm: false\n"; + let settings: WorkspaceSettings = serde_saphyr::from_str(yaml).unwrap(); + let mut config = Config::new(); + settings.apply_to(&mut config, Path::new("C:/irrelevant")); + assert!(config.unsafe_perm, "Windows forces unsafe_perm true regardless of yaml"); +} + +/// A positive `childConcurrency` is taken verbatim — mirrors +/// upstream's [`getWorkspaceConcurrency`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/concurrency.ts#L25-L34). +#[test] +fn parses_positive_child_concurrency_from_yaml_and_applies() { + let yaml = "childConcurrency: 8\n"; + let settings: WorkspaceSettings = serde_saphyr::from_str(yaml).unwrap(); + assert_eq!(settings.child_concurrency, Some(8)); + + let mut config = Config::new(); + settings.apply_to(&mut config, Path::new("/irrelevant")); + assert_eq!(config.child_concurrency, 8); +} + +/// A non-positive `childConcurrency` is interpreted as +/// `max(1, parallelism - |value|)`. The exact result depends on +/// the host's reported parallelism, so we just bound-check it: +/// negative offsets must produce at least 1 and at most +/// `parallelism()`. +#[test] +fn parses_negative_child_concurrency_from_yaml_and_resolves() { + let yaml = "childConcurrency: -1\n"; + let settings: WorkspaceSettings = serde_saphyr::from_str(yaml).unwrap(); + assert_eq!(settings.child_concurrency, Some(-1)); + + let mut config = Config::new(); + settings.apply_to(&mut config, Path::new("/irrelevant")); + let parallelism = crate::available_parallelism(); + assert!(config.child_concurrency >= 1, "must floor at 1"); + assert!(config.child_concurrency <= parallelism, "must not exceed available parallelism"); +} + #[test] fn apply_leaves_unset_fields_alone() { let yaml = "storeDir: /s\n"; diff --git a/pacquet/crates/executor/src/extend_path.rs b/pacquet/crates/executor/src/extend_path.rs index 661b376fbb..fbdb4b642a 100644 --- a/pacquet/crates/executor/src/extend_path.rs +++ b/pacquet/crates/executor/src/extend_path.rs @@ -7,6 +7,11 @@ use std::{ /// Controls whether the dir containing the current `node` interpreter /// is appended to PATH. Tri-state from /// . +/// +/// `pacquet-config` mirrors this enum with its own yaml-deserializable +/// type (upstream's `scriptsPrependNodePath: boolean | 'warn-only'` +/// shape) and converts to this one at the call site, so the executor +/// crate stays free of serde and Config wiring. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] pub enum ScriptsPrependNodePath { /// `scriptsPrependNodePath: true` — always prepend. diff --git a/pacquet/crates/package-manager/src/build_modules.rs b/pacquet/crates/package-manager/src/build_modules.rs index 9ca322b465..15e276012f 100644 --- a/pacquet/crates/package-manager/src/build_modules.rs +++ b/pacquet/crates/package-manager/src/build_modules.rs @@ -13,9 +13,11 @@ use pacquet_reporter::{ LogEvent, LogLevel, Reporter, SkippedOptionalDependencyLog, SkippedOptionalPackage, SkippedOptionalReason, }; +use rayon::prelude::*; use std::{ collections::{BTreeSet, HashMap, HashSet}, path::{Path, PathBuf}, + sync::Mutex, }; /// Error from the build-modules step. @@ -40,6 +42,22 @@ pub enum BuildModulesError { help("Ensure the package is listed in patchedDependencies configuration") )] PatchFilePathMissing { dep_path: String }, + + /// `ThreadPoolBuilder::build()` failed — most likely the OS + /// refused to spawn the requested number of worker threads + /// (`EAGAIN` / RLIMIT_NPROC). Surfaced as a structured error + /// rather than a panic so the install path can return cleanly. + #[display("Failed to build the per-install rayon thread pool: {source}")] + #[diagnostic( + code(ERR_PACQUET_BUILD_THREAD_POOL), + help( + "Lower childConcurrency in pnpm-workspace.yaml, or raise the process's RLIMIT_NPROC." + ) + )] + ThreadPoolBuild { + #[error(source)] + source: rayon::ThreadPoolBuildError, + }, } /// Build policy derived from `allowBuilds` and @@ -148,12 +166,13 @@ impl AllowBuildPolicy { /// Run lifecycle scripts for all packages that require a build. /// /// Ports the core of `buildModules` from -/// `https://github.com/pnpm/pnpm/blob/80037699fb/building/during-install/src/index.ts`. +/// . /// /// Packages are visited in topological order (children before parents) via -/// [`build_sequence`]. Within a chunk, members are independent and could run -/// concurrently — pacquet currently runs them sequentially (TODO: honor -/// `childConcurrency`). +/// [`build_sequence`]. Chunks run sequentially. Members within a chunk +/// run in parallel under a per-install rayon thread pool bounded to +/// [`BuildModules::child_concurrency`] threads — mirrors upstream's +/// [`runGroups(getWorkspaceConcurrency(opts.childConcurrency), groups)`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/during-install/src/index.ts#L124). pub struct BuildModules<'a> { pub virtual_store_dir: &'a Path, pub modules_dir: &'a Path, @@ -209,6 +228,25 @@ pub struct BuildModules<'a> { /// 3. Patch application — the patch is applied to the extracted /// package dir before postinstall hooks run. pub patches: Option<&'a HashMap>, + /// Mirrors `config.scripts_prepend_node_path`. Threaded through to + /// [`RunPostinstallHooks::scripts_prepend_node_path`] for each + /// spawned lifecycle script. Default [`ScriptsPrependNodePath::Never`]. + pub scripts_prepend_node_path: ScriptsPrependNodePath, + /// Mirrors `config.unsafe_perm`. When `false`, [`pacquet_executor`] + /// runs each lifecycle script under a per-package TMPDIR set to + /// `node_modules/.tmp`; when `true`, TMPDIR is left at the + /// inherited value (matches upstream's + /// [`@pnpm/npm-lifecycle`](https://github.com/pnpm/npm-lifecycle/blob/d2d8e790/index.js#L204-L220) + /// gate). Default `true`. + pub unsafe_perm: bool, + /// Mirrors `config.child_concurrency`. Per-chunk parallelism + /// for build-script spawns. Chunks remain sequential to preserve + /// topological ordering; members within a chunk run in parallel + /// up to this many at a time. Mirrors upstream's + /// [`runGroups(getWorkspaceConcurrency(opts.childConcurrency), groups)`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/during-install/src/index.ts#L124). + /// Floored to `1` to guarantee forward progress on + /// resource-constrained hosts. + pub child_concurrency: u32, } impl<'a> BuildModules<'a> { @@ -234,6 +272,9 @@ impl<'a> BuildModules<'a> { store_dir, store_index_writer, patches, + scripts_prepend_node_path, + unsafe_perm, + child_concurrency, } = self; let Some(snapshots) = snapshots else { return Ok(Vec::new()) }; @@ -301,281 +342,369 @@ impl<'a> BuildModules<'a> { roots, ) }); - let mut deps_state_cache: pacquet_graph_hasher::DepsStateCache = - pacquet_graph_hasher::DepsStateCache::new(); + // `deps_state_cache` memoizes per-snapshot hashes across the + // recursive walk in `calc_dep_state`. Shared across all + // chunks so diamond-shaped subgraphs hit the memo from + // earlier chunks too. Wrapped in `Mutex` because chunks now + // dispatch their members concurrently — `calc_dep_state` + // mutates the cache through `&mut`, and rayon would + // otherwise need each task to own a private cache, defeating + // the point of memoization. + let deps_state_cache: Mutex> = + Mutex::new(pacquet_graph_hasher::DepsStateCache::new()); let chunks = build_sequence(&requires_build_map, patches, snapshots, importers); // Collect peer-stripped keys so the final list is unique and // sorted lexicographically — matches `dedupePackageNamesFromIgnoredBuilds`. - let mut ignored_builds: BTreeSet = BTreeSet::new(); + // `Mutex` for the same parallelism reason as `deps_state_cache` above. + let ignored_builds: Mutex> = Mutex::new(BTreeSet::new()); + + // Per-install rayon pool. Bounded to `child_concurrency` so + // a chunk with many build-needed members doesn't exhaust the + // process's rayon-global threads (which other crates may + // depend on). One pool reused across all chunks; chunks + // themselves run sequentially. + // + // Mirrors upstream's + // [`runGroups(getWorkspaceConcurrency(opts.childConcurrency), groups)`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/during-install/src/index.ts#L124). + // `ThreadPoolBuilder::build()` is fallible — the OS may + // refuse the spawn (`EAGAIN` / RLIMIT_NPROC) on a host + // already near its process-thread limit. Surface that as + // [`BuildModulesError::ThreadPoolBuild`] so the install + // returns cleanly with a remediation hint instead of + // panicking inside the binary. + let pool = rayon::ThreadPoolBuilder::new() + .num_threads(child_concurrency.max(1) as usize) + .build() + .map_err(|source| BuildModulesError::ThreadPoolBuild { source })?; for chunk in chunks { - for snapshot_key in chunk { - let metadata_key = snapshot_key.without_peer(); - // Look up against the peer-stripped key because - // patches are configured at the (name, version) - // granularity in `pnpm-workspace.yaml`, not per - // peer-resolution variant. - let patch = patches.and_then(|map| map.get(&metadata_key)); - let has_patch = patch.is_some(); - let requires_build = - requires_build_map.get(&snapshot_key).copied().unwrap_or(false); - - // Ancestors of a build/patch candidate are included - // in the sequence (so the topo order stays correct) - // but only run scripts / apply patches when they - // themselves are candidates. Mirrors upstream's - // chunk filter at - // . - if !requires_build && !has_patch { - continue; - } - - let (name, version) = parse_name_version_from_key(&metadata_key.to_string()); - - // Mirrors upstream's `if (node.requiresBuild) { allowBuild(...) }` - // at lines 88-101: the allowBuilds gate only applies - // when the node has scripts to run. A patched-only - // package skips this check entirely and proceeds to - // patch application below. - // - // `false` / `None` from the policy set - // `should_run_scripts = false` (NOT `continue`), so - // the patch still gets applied even when scripts - // are disallowed. Matches upstream's `ignoreScripts - // = true; break` pattern. - let mut should_run_scripts = requires_build; - if requires_build { - match allow_build_policy.check(&name, &version) { - Some(false) => { - should_run_scripts = false; - } - None => { - // "Not in allowBuilds" — surfaced as - // `pnpm:ignored-scripts`. Explicit - // `false` is silently denied (above), - // matching upstream's switch. - ignored_builds.insert(metadata_key.to_string()); - should_run_scripts = false; - } - Some(true) => {} - } - } - - // Compute the side-effects cache key once per - // snapshot, before the `is_built` gate. The same - // value is later consumed by the WRITE-path upload - // call after `run_postinstall_hooks` succeeds, so - // recomputing it there would just duplicate work — - // `deps_state_cache` makes the second call free - // anyway, but routing through one `let` keeps the - // gate-side and write-side keys provably identical. - // - // `None` when the cache gate can't fire (no engine, - // no graph, etc.); both downstream consumers - // short-circuit on `None`. - let cache_key = (dep_graph.as_ref().zip(engine_name)).map(|(graph, engine)| { - pacquet_graph_hasher::calc_dep_state( - graph, - &mut deps_state_cache, - &snapshot_key, - &pacquet_graph_hasher::CalcDepStateOptions { - engine_name: engine, - // Mirrors upstream's - // `patchFileHash: depNode.patch?.hash` - // at - // . - // `None` for unpatched snapshots leaves - // the `;patch=...` segment off the cache - // key entirely, matching upstream when - // `depNode.patch == null`. - patch_file_hash: patch.map(|p| p.hash.as_str()), - // Mirrors `includeDepGraphHash: hasSideEffects` - // at upstream line 202. A patched-only - // snapshot (no scripts will run) leaves - // the deps-hash off so the cache key - // stays stable across dep-graph changes - // that don't affect this package's - // patched output. - include_dep_graph_hash: should_run_scripts, - }, + // The closure runs once per chunk; `try_for_each` + // short-circuits on the first error. The only mutable + // state shared across tasks is the two `Mutex`-wrapped + // collections above and `deps_state_cache`. + pool.install(|| -> Result<(), BuildModulesError> { + chunk.par_iter().try_for_each(|snapshot_key| { + build_one_snapshot::( + snapshot_key, + snapshots, + packages, + patches, + &requires_build_map, + allow_build_policy, + side_effects_maps_by_snapshot, + engine_name, + side_effects_cache, + side_effects_cache_write, + store_dir, + store_index_writer, + dep_graph.as_ref(), + &deps_state_cache, + &ignored_builds, + virtual_store_dir, + modules_dir, + lockfile_dir, + &extra_bin_paths, + &extra_env, + scripts_prepend_node_path, + unsafe_perm, ) - }); - - // Side-effects-cache `is_built` gate. Mirrors - // upstream's `!node.isBuilt` filter at - // . - // We're already past the policy gate, so this - // snapshot would otherwise run its scripts — but if - // the prefetch surfaced a matching side-effects-cache - // entry, the build is already represented on disk - // (pnpm seeded it on a previous install) and we - // can skip. - if side_effects_cache - && let Some(maps_by_snapshot) = side_effects_maps_by_snapshot - && let Some(maps) = maps_by_snapshot.get(&snapshot_key) - && let Some(key) = cache_key.as_deref() - && maps.contains_key(key) - { - tracing::debug!( - target: "pacquet::build", - ?snapshot_key, - cache_key = key, - "side-effects cache hit; skipping build", - ); - continue; - } - - let pkg_dir = virtual_store_dir_for_key(virtual_store_dir, &snapshot_key); - if !pkg_dir.exists() { - continue; - } - - let optional = snapshots.get(&snapshot_key).is_some_and(|entry| entry.optional); - - // Apply the patch before running postinstall hooks. - // Mirrors upstream at - // : - // ``` - // if (depNode.patch) { - // if (!depNode.patch.patchFilePath) throw PATCH_FILE_PATH_MISSING - // isPatched = applyPatchToDir(...) - // } - // ``` - // `is_patched` feeds the cache-write gate below - // (`is_patched || has_side_effects`), matching - // upstream's line 199 condition. - let is_patched = if let Some(p) = patch { - let patch_file_path = p.patch_file_path.as_deref().ok_or_else(|| { - BuildModulesError::PatchFilePathMissing { - dep_path: snapshot_key.to_string(), - } - })?; - apply_patch_to_dir(&pkg_dir, patch_file_path) - .map_err(BuildModulesError::PatchApply)?; - true - } else { - false - }; - - let has_side_effects = if should_run_scripts { - let result = run_postinstall_hooks::(RunPostinstallHooks { - dep_path: &snapshot_key.to_string(), - pkg_root: &pkg_dir, - root_modules_dir: modules_dir, - init_cwd: lockfile_dir, - extra_bin_paths: &extra_bin_paths, - extra_env: &extra_env, - node_execpath: None, - npm_execpath: None, - node_gyp_path: None, - user_agent: None, - // Hard-coded until the `unsafe-perm` config knob - // is plumbed through. `true` skips both the - // TMPDIR creation and the uid/gid drop, matching - // pacquet's behavior before any of this landed. - unsafe_perm: true, - node_gyp_bin: None, - scripts_prepend_node_path: ScriptsPrependNodePath::Never, - script_shell: None, - optional, - }); - - match result { - Ok(ran) => ran, - Err(err) => { - if optional { - // Mirrors `building/during-install/src/index.ts:226-238`: - // a build failure on an optional dep is logged - // through the `pnpm:skipped-optional-dependency` - // channel and swallowed so the install can - // continue. The `package.id` field upstream is - // `depNode.dir`; we use the same. - R::emit(&LogEvent::SkippedOptionalDependency( - SkippedOptionalDependencyLog { - level: LogLevel::Debug, - details: Some(err.to_string()), - package: SkippedOptionalPackage { - id: pkg_dir.to_string_lossy().into_owned(), - name: name.clone(), - version: version.clone(), - }, - prefix: lockfile_dir.to_string_lossy().into_owned(), - reason: SkippedOptionalReason::BuildFailure, - }, - )); - continue; - } - return Err(BuildModulesError::LifecycleScript(err)); - } - } - } else { - false - }; - - // Side-effects-cache WRITE path. Mirrors - // ``: - // after a successful `run_postinstall_hooks` (or a - // patch application that mutated the dir), - // re-hash the package directory and queue a - // `PackageFilesIndex.sideEffects[cache_key] = diff` - // mutation so a future install can skip the - // rebuild. - // - // Upstream's gate is `(isPatched || hasSideEffects) - // && opts.sideEffectsCacheWrite`. Pacquet mirrors - // that — a patched-only snapshot still uploads its - // post-patch state so subsequent installs hit the - // cache. - // - // The other preconditions: cache_key composable - // (engine + graph present), `packages` map - // available for the integrity lookup, and the - // metadata row carries an integrity (registry / - // tarball resolutions — git / directory have no - // integrity and pnpm doesn't cache those either). - // - // All errors are swallowed with a `tracing::warn!`, - // matching upstream's `try { upload } catch (err) { - // logger.warn(...) }` at lines 208-215. A failed - // upload doesn't fail the install: the next install - // re-runs the build. - if (is_patched || has_side_effects) - && side_effects_cache_write - && let Some(writer) = store_index_writer - && let Some(store) = store_dir - && let Some(cache_key) = cache_key.as_deref() - && let Some(packages) = packages - && let Some(metadata) = packages.get(&metadata_key) - && let Some(integrity) = metadata.resolution.integrity() - { - let files_index_file = pacquet_store_dir::store_index_key( - &integrity.to_string(), - &metadata_key.to_string(), - ); - if let Err(err) = pacquet_store_dir::upload( - store, - &pkg_dir, - &files_index_file, - cache_key, - writer, - ) { - tracing::warn!( - target: "pacquet::build", - ?err, - dep_path = %snapshot_key, - "side-effects cache upload failed; build proceeds", - ); - } - } - } + }) + })?; } + // If a chunk worker panicked while holding the + // `ignored_builds` lock, rayon's `try_for_each` will have + // already propagated the panic (or returned an Err) — so a + // poisoned mutex here can only mean the protected state is + // mid-insertion. A `BTreeSet::insert` is one atomic + // operation from the data-structure's POV (no torn writes), + // so the canonical poison-recovery pattern is safe. + let ignored_builds = ignored_builds.into_inner().unwrap_or_else(|e| e.into_inner()); Ok(ignored_builds.into_iter().collect()) } } +/// Per-snapshot work extracted out of [`BuildModules::run`]'s inner +/// loop so the bounded-parallelism `par_iter().try_for_each(...)` +/// dispatch can call it once per chunk member. The body is the same +/// as the pre-#12 sequential loop — `continue`s become `return Ok(())` +/// here. +#[allow(clippy::too_many_arguments)] +fn build_one_snapshot( + snapshot_key: &PackageKey, + snapshots: &HashMap, + packages: Option<&HashMap>, + patches: Option<&HashMap>, + requires_build_map: &HashMap, + allow_build_policy: &AllowBuildPolicy, + side_effects_maps_by_snapshot: Option<&crate::SideEffectsMapsBySnapshot>, + engine_name: Option<&str>, + side_effects_cache: bool, + side_effects_cache_write: bool, + store_dir: Option<&pacquet_store_dir::StoreDir>, + store_index_writer: Option<&std::sync::Arc>, + dep_graph: Option<&HashMap>>, + deps_state_cache: &Mutex>, + ignored_builds: &Mutex>, + virtual_store_dir: &Path, + modules_dir: &Path, + lockfile_dir: &Path, + extra_bin_paths: &[PathBuf], + extra_env: &HashMap, + scripts_prepend_node_path: ScriptsPrependNodePath, + unsafe_perm: bool, +) -> Result<(), BuildModulesError> { + let metadata_key = snapshot_key.without_peer(); + // Look up against the peer-stripped key because patches are + // configured at the (name, version) granularity in + // `pnpm-workspace.yaml`, not per peer-resolution variant. + let patch = patches.and_then(|map| map.get(&metadata_key)); + let has_patch = patch.is_some(); + let requires_build = requires_build_map.get(snapshot_key).copied().unwrap_or(false); + + // Ancestors of a build/patch candidate are included in the + // sequence (so the topo order stays correct) but only run + // scripts / apply patches when they themselves are candidates. + // Mirrors upstream's chunk filter at + // . + if !requires_build && !has_patch { + return Ok(()); + } + + let (name, version) = parse_name_version_from_key(&metadata_key.to_string()); + + // Mirrors upstream's `if (node.requiresBuild) { allowBuild(...) }` + // at lines 88-101: the allowBuilds gate only applies when the + // node has scripts to run. A patched-only package skips this + // check entirely and proceeds to patch application below. + // + // `false` / `None` from the policy set `should_run_scripts = + // false` (NOT early-return), so the patch still gets applied + // even when scripts are disallowed. Matches upstream's + // `ignoreScripts = true; break` pattern. + let mut should_run_scripts = requires_build; + if requires_build { + match allow_build_policy.check(&name, &version) { + Some(false) => { + should_run_scripts = false; + } + None => { + // "Not in allowBuilds" — surfaced as + // `pnpm:ignored-scripts`. Explicit `false` is + // silently denied (above), matching upstream's + // switch. + // Poison-recover: see the equivalent call site at + // the end of `BuildModules::run` for the safety + // argument (BTreeSet insertion is atomic from the + // data-structure's POV). + ignored_builds + .lock() + .unwrap_or_else(|e| e.into_inner()) + .insert(metadata_key.to_string()); + should_run_scripts = false; + } + Some(true) => {} + } + } + + // Compute the side-effects cache key once per snapshot, before + // the `is_built` gate. The same value is later consumed by the + // WRITE-path upload call after `run_postinstall_hooks` + // succeeds, so recomputing it there would just duplicate work — + // `deps_state_cache` makes the second call free anyway, but + // routing through one `let` keeps the gate-side and write-side + // keys provably identical. + // + // `None` when the cache gate can't fire (no engine, no graph, + // etc.); both downstream consumers short-circuit on `None`. + // + // The `deps_state_cache` is shared across all chunk members via + // `Mutex` because `calc_dep_state` is recursive and memoizes — + // a per-task cache would defeat the memoization for + // diamond-shaped subgraphs. + let cache_key = (dep_graph.zip(engine_name)).map(|(graph, engine)| { + // Poison-recover: `calc_dep_state` mutates the cache by + // inserting one entry per recursive walk node, each + // insert atomic from `HashMap`'s POV. A panic mid-walk + // leaves the map in a usable state — the worst case is + // an unfinished sub-walk that the next caller will redo. + let mut cache_guard = deps_state_cache.lock().unwrap_or_else(|e| e.into_inner()); + pacquet_graph_hasher::calc_dep_state( + graph, + &mut cache_guard, + snapshot_key, + &pacquet_graph_hasher::CalcDepStateOptions { + engine_name: engine, + // Mirrors upstream's + // `patchFileHash: depNode.patch?.hash` at + // . + // `None` for unpatched snapshots leaves the + // `;patch=...` segment off the cache key entirely, + // matching upstream when `depNode.patch == null`. + patch_file_hash: patch.map(|p| p.hash.as_str()), + // Mirrors `includeDepGraphHash: hasSideEffects` at + // upstream line 202. A patched-only snapshot (no + // scripts will run) leaves the deps-hash off so the + // cache key stays stable across dep-graph changes + // that don't affect this package's patched output. + include_dep_graph_hash: should_run_scripts, + }, + ) + }); + + // Side-effects-cache `is_built` gate. Mirrors upstream's + // `!node.isBuilt` filter at + // . + // We're already past the policy gate, so this snapshot would + // otherwise run its scripts — but if the prefetch surfaced a + // matching side-effects-cache entry, the build is already + // represented on disk (pnpm seeded it on a previous install) + // and we can skip. + if side_effects_cache + && let Some(maps_by_snapshot) = side_effects_maps_by_snapshot + && let Some(maps) = maps_by_snapshot.get(snapshot_key) + && let Some(key) = cache_key.as_deref() + && maps.contains_key(key) + { + tracing::debug!( + target: "pacquet::build", + ?snapshot_key, + cache_key = key, + "side-effects cache hit; skipping build", + ); + return Ok(()); + } + + let pkg_dir = virtual_store_dir_for_key(virtual_store_dir, snapshot_key); + if !pkg_dir.exists() { + return Ok(()); + } + + let optional = snapshots.get(snapshot_key).is_some_and(|entry| entry.optional); + + // Apply the patch before running postinstall hooks. Mirrors + // upstream at + // : + // ``` + // if (depNode.patch) { + // if (!depNode.patch.patchFilePath) throw PATCH_FILE_PATH_MISSING + // isPatched = applyPatchToDir(...) + // } + // ``` + // `is_patched` feeds the cache-write gate below + // (`is_patched || has_side_effects`), matching upstream's + // line 199 condition. + let is_patched = if let Some(p) = patch { + let patch_file_path = p.patch_file_path.as_deref().ok_or_else(|| { + BuildModulesError::PatchFilePathMissing { dep_path: snapshot_key.to_string() } + })?; + apply_patch_to_dir(&pkg_dir, patch_file_path).map_err(BuildModulesError::PatchApply)?; + true + } else { + false + }; + + let has_side_effects = if should_run_scripts { + let result = run_postinstall_hooks::(RunPostinstallHooks { + dep_path: &snapshot_key.to_string(), + pkg_root: &pkg_dir, + root_modules_dir: modules_dir, + init_cwd: lockfile_dir, + extra_bin_paths, + extra_env, + node_execpath: None, + npm_execpath: None, + node_gyp_path: None, + user_agent: None, + unsafe_perm, + node_gyp_bin: None, + scripts_prepend_node_path, + script_shell: None, + optional, + }); + + match result { + Ok(ran) => ran, + Err(err) => { + if optional { + // Mirrors + // `building/during-install/src/index.ts:226-238`: + // a build failure on an optional dep is logged + // through the `pnpm:skipped-optional-dependency` + // channel and swallowed so the install can + // continue. The `package.id` field upstream is + // `depNode.dir`; we use the same. + R::emit(&LogEvent::SkippedOptionalDependency(SkippedOptionalDependencyLog { + level: LogLevel::Debug, + details: Some(err.to_string()), + package: SkippedOptionalPackage { + id: pkg_dir.to_string_lossy().into_owned(), + name: name.clone(), + version: version.clone(), + }, + prefix: lockfile_dir.to_string_lossy().into_owned(), + reason: SkippedOptionalReason::BuildFailure, + })); + return Ok(()); + } + return Err(BuildModulesError::LifecycleScript(err)); + } + } + } else { + false + }; + + // Side-effects-cache WRITE path. Mirrors upstream at + // : + // after a successful `run_postinstall_hooks` (or a patch + // application that mutated the dir), re-hash the package + // directory and queue a + // `PackageFilesIndex.sideEffects[cache_key] = diff` mutation + // so a future install can skip the rebuild. + // + // Upstream's gate is `(isPatched || hasSideEffects) && + // opts.sideEffectsCacheWrite`. Pacquet mirrors that — a + // patched-only snapshot still uploads its post-patch state so + // subsequent installs hit the cache. + // + // The other preconditions: cache_key composable (engine + graph + // present), `packages` map available for the integrity lookup, + // and the metadata row carries an integrity (registry / tarball + // resolutions — git / directory have no integrity and pnpm + // doesn't cache those either). + // + // All errors are swallowed with a `tracing::warn!`, matching + // upstream's `try { upload } catch (err) { logger.warn(...) }` + // at lines 208-215. A failed upload doesn't fail the install: + // the next install re-runs the build. + if (is_patched || has_side_effects) + && side_effects_cache_write + && let Some(writer) = store_index_writer + && let Some(store) = store_dir + && let Some(cache_key) = cache_key.as_deref() + && let Some(packages) = packages + && let Some(metadata) = packages.get(&metadata_key) + && let Some(integrity) = metadata.resolution.integrity() + { + let files_index_file = + pacquet_store_dir::store_index_key(&integrity.to_string(), &metadata_key.to_string()); + if let Err(err) = + pacquet_store_dir::upload(store, &pkg_dir, &files_index_file, cache_key, writer) + { + tracing::warn!( + target: "pacquet::build", + ?err, + dep_path = %snapshot_key, + "side-effects cache upload failed; build proceeds", + ); + } + } + + Ok(()) +} + /// Compute the package directory inside the virtual store for a snapshot key. /// /// Uses `without_peer()` to strip any peer-dependency suffix before diff --git a/pacquet/crates/package-manager/src/build_modules/tests.rs b/pacquet/crates/package-manager/src/build_modules/tests.rs index 248d72003f..d751b8115b 100644 --- a/pacquet/crates/package-manager/src/build_modules/tests.rs +++ b/pacquet/crates/package-manager/src/build_modules/tests.rs @@ -1,5 +1,6 @@ use super::{AllowBuildPolicy, BuildModules, parse_name_version_from_key}; use pacquet_config::Config; +use pacquet_executor::ScriptsPrependNodePath; use pacquet_lockfile::{ PackageKey, PkgName, PkgVerPeer, ProjectSnapshot, ResolvedDependencyMap, ResolvedDependencySpec, SnapshotEntry, @@ -299,6 +300,10 @@ fn build_modules_collects_ignored_builds() { store_dir: None, store_index_writer: None, patches: None, + + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + unsafe_perm: true, + child_concurrency: 1, } .run::() .expect("run BuildModules"); @@ -311,6 +316,75 @@ fn build_modules_collects_ignored_builds() { ); } +/// Parallel-path variant of [`build_modules_collects_ignored_builds`] +/// running under `child_concurrency: 2` so the rayon +/// `chunk.par_iter().try_for_each(...)` dispatch is actually +/// exercised. The other `BuildModules` tests all run with +/// `child_concurrency: 1`, which is the sequential codepath; this +/// test pins the concurrent codepath against a fixture that places +/// two policy-denied build candidates in the same chunk (no +/// dependency edges between them, so the topo sort puts them both +/// in chunk 0). +/// +/// The assertion is the same sorted ignored-set as the sequential +/// test — a regression that dropped the `pool.install` / +/// `try_for_each` wrapping would still collect both names but in +/// non-deterministic order on insertion. The +/// `BTreeSet`-backed `ignored_builds` ordering hides that, so +/// breakage would more likely show up as a lock contention bug +/// (e.g. dropping the `Mutex` wrapping) which would manifest as a +/// rustc / clippy error rather than a runtime failure; this test +/// at least documents that the codepath is exercised. +#[test] +fn build_modules_collects_ignored_builds_under_concurrency() { + let snapshots = HashMap::from([ + (key("zzz", "1.0.0"), SnapshotEntry::default()), + (key("aaa", "2.0.0"), SnapshotEntry::default()), + ]); + let importers = root_importers(&[("zzz", "1.0.0"), ("aaa", "2.0.0")]); + let policy = AllowBuildPolicy::default(); + + let virtual_store_dir = tempdir().expect("create temp dir"); + let modules_dir = tempdir().expect("create temp dir"); + let lockfile_dir = tempdir().expect("create temp dir"); + + create_buildable_pkg(virtual_store_dir.path(), &key("zzz", "1.0.0")); + create_buildable_pkg(virtual_store_dir.path(), &key("aaa", "2.0.0")); + + let ignored = BuildModules { + virtual_store_dir: virtual_store_dir.path(), + modules_dir: modules_dir.path(), + lockfile_dir: lockfile_dir.path(), + snapshots: Some(&snapshots), + importers: &importers, + packages: None, + allow_build_policy: &policy, + side_effects_maps_by_snapshot: None, + engine_name: None, + side_effects_cache: true, + side_effects_cache_write: false, + store_dir: None, + store_index_writer: None, + patches: None, + + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + unsafe_perm: true, + child_concurrency: 2, + } + .run::() + .expect("run BuildModules under concurrency"); + dbg!(&ignored); + + // Same expected output as the sequential test — both members of + // the same chunk insert into the `Mutex` concurrently + // and the BTreeSet's iteration order normalizes the result. + assert_eq!( + ignored, + vec!["aaa@2.0.0".to_string(), "zzz@1.0.0".to_string()], + "ignored set must be the same under concurrency 2 as under 1: {ignored:?}", + ); +} + /// Explicit `false` in `allowBuilds` is silently skipped — it does NOT /// land in the ignored-scripts list. Mirrors upstream /// `building/during-install/src/index.ts:91-93`. @@ -346,6 +420,10 @@ fn build_modules_excludes_explicit_deny_from_ignored() { store_dir: None, store_index_writer: None, patches: None, + + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + unsafe_perm: true, + child_concurrency: 1, } .run::() .expect("run BuildModules"); @@ -420,6 +498,10 @@ fn do_not_fail_on_optional_dep_with_failing_postinstall() { store_dir: None, store_index_writer: None, patches: None, + + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + unsafe_perm: true, + child_concurrency: 1, } .run::() .expect("optional build failure must NOT abort the install"); @@ -542,6 +624,10 @@ fn using_side_effects_cache_skips_rebuild() { store_dir: None, store_index_writer: None, patches: None, + + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + unsafe_perm: true, + child_concurrency: 1, } .run::() .expect("install must succeed when the cache hit skips the rebuild"); @@ -599,6 +685,10 @@ fn side_effects_cache_disabled_bypasses_the_gate() { store_dir: None, store_index_writer: None, patches: None, + + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + unsafe_perm: true, + child_concurrency: 1, } .run::() .expect_err("with cache disabled, the failing postinstall must run and the install must fail"); @@ -649,6 +739,10 @@ fn fail_when_failing_postinstall_is_required() { store_dir: None, store_index_writer: None, patches: None, + + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + unsafe_perm: true, + child_concurrency: 1, } .run::() .expect_err("required build failure must propagate"); @@ -876,6 +970,10 @@ async fn write_path_populates_side_effects_row() { store_dir: Some(&store_dir), store_index_writer: Some(&writer), patches: None, + + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + unsafe_perm: true, + child_concurrency: 1, } .run::() .expect("build modules must complete cleanly"); @@ -978,6 +1076,10 @@ async fn write_path_disabled_skips_upload() { store_dir: Some(&store_dir), store_index_writer: Some(&writer), patches: None, + + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + unsafe_perm: true, + child_concurrency: 1, } .run::() .expect("build modules must complete cleanly"); @@ -1089,6 +1191,10 @@ async fn upload_error_does_not_interrupt_install() { store_dir: Some(&store_dir), store_index_writer: Some(&writer), patches: None, + + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + unsafe_perm: true, + child_concurrency: 1, } .run::() .expect("upload failure must not propagate; install continues"); @@ -1310,6 +1416,10 @@ new file mode 100644 store_dir: Some(&store_dir), store_index_writer: Some(&writer), patches: Some(&patches), + + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + unsafe_perm: true, + child_concurrency: 1, } .run::() .expect("build modules must complete cleanly"); @@ -1409,6 +1519,10 @@ new file mode 100644 store_dir: Some(&store_dir), store_index_writer: Some(&writer), patches: Some(&patches), + + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + unsafe_perm: true, + child_concurrency: 1, } .run::() .expect("build modules must complete cleanly"); @@ -1479,6 +1593,10 @@ async fn missing_patch_file_path_errors_with_diagnostic() { store_dir: Some(&store_dir), store_index_writer: Some(&writer), patches: Some(&patches), + + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + unsafe_perm: true, + child_concurrency: 1, } .run::() .expect_err("missing patch_file_path must surface as PatchFilePathMissing"); diff --git a/pacquet/crates/package-manager/src/build_sequence.rs b/pacquet/crates/package-manager/src/build_sequence.rs index da510eaf26..00117df102 100644 --- a/pacquet/crates/package-manager/src/build_sequence.rs +++ b/pacquet/crates/package-manager/src/build_sequence.rs @@ -105,6 +105,13 @@ fn build_children_map( } } } + // Sort for the same reason `collect_root_dep_paths` sorts + // its output: `get_subgraph_to_build` walks children in + // sequence, and a shared transitive descendant gets trimmed + // off whichever sibling visits it second. Both the entry + // nodes and every child list must be in a deterministic + // order for the build sequence to be reproducible. + child_keys.sort_by_key(|k| k.to_string()); children.insert(key.clone(), child_keys); } children @@ -141,6 +148,18 @@ fn collect_root_dep_paths( } } } + // [`get_subgraph_to_build`] is order-sensitive (a node walked + // first via root A may mark a shared child as already-walked, so + // a second root B sharing that child gets trimmed). Upstream's + // input arrives in JS-object insertion order; pacquet sources + // these from `HashMap<_, ProjectSnapshot>` and + // `HashMap`, so iteration order + // is non-deterministic. Sort by `PackageKey` string repr so the + // build sequence (and the trim behavior) is reproducible run to + // run. Long-term fix is to preserve lockfile declaration order + // via `IndexMap`; until then, an alphabetical sort is enough to + // make the build path deterministic. + roots.sort_by_key(|k| k.to_string()); roots } diff --git a/pacquet/crates/package-manager/src/build_sequence/tests.rs b/pacquet/crates/package-manager/src/build_sequence/tests.rs index 8f318d17e8..9067ef5c87 100644 --- a/pacquet/crates/package-manager/src/build_sequence/tests.rs +++ b/pacquet/crates/package-manager/src/build_sequence/tests.rs @@ -172,3 +172,36 @@ fn parallel_build_leaves_share_chunk() { assert_eq!(leaves, vec![key("a", "1.0.0"), key("b", "1.0.0")]); assert_eq!(chunks[1], vec![key("root", "1.0.0")]); } + +/// Direct port of upstream +/// [`'buildSequence() test 2'`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/during-install/test/buildSequence.test.ts#L28-L51). +/// +/// Two importers `a` and `b` both depend on a shared builder leaf +/// `c`. Only `a` requires its own build; `b` does not. The result +/// must surface `c` in the first chunk and `a` in the second — `b` +/// is *trimmed* from the build sequence because it neither needs a +/// build itself nor has a buildable descendant that's exclusive to +/// it (its descendant `c` is already scheduled via `a`). +/// +/// This is the subgraph-trim case for #397 item #16. Pacquet's +/// existing `unrelated_subgraph_excluded` covers a stronger +/// scenario (an entirely unreachable subgraph); this one pins the +/// upstream-equivalent behavior where an importer that's still in +/// the install set gets dropped from the build sequence. +#[test] +fn non_builder_importer_with_shared_builder_child_is_trimmed() { + let snapshots = HashMap::from([ + (key("a", "1.0.0"), snap(&[("c", "1.0.0")])), + (key("b", "1.0.0"), snap(&[("c", "1.0.0")])), + (key("c", "1.0.0"), snap(&[])), + ]); + let requires_build = requires([ + (key("a", "1.0.0"), true), + (key("b", "1.0.0"), false), + (key("c", "1.0.0"), true), + ]); + let importers = root_importers(&[("a", "1.0.0"), ("b", "1.0.0")]); + + let chunks = build_sequence(&requires_build, None, &snapshots, &importers); + assert_eq!(chunks, vec![vec![key("c", "1.0.0")], vec![key("a", "1.0.0")]]); +} diff --git a/pacquet/crates/package-manager/src/install_frozen_lockfile.rs b/pacquet/crates/package-manager/src/install_frozen_lockfile.rs index 8d5d410bb9..e047eafd1e 100644 --- a/pacquet/crates/package-manager/src/install_frozen_lockfile.rs +++ b/pacquet/crates/package-manager/src/install_frozen_lockfile.rs @@ -6,6 +6,7 @@ use crate::{ use derive_more::{Display, Error}; use miette::Diagnostic; use pacquet_config::Config; +use pacquet_executor::ScriptsPrependNodePath as ExecScriptsPrependNodePath; use pacquet_lockfile::{PackageKey, PackageMetadata, ProjectSnapshot, SnapshotEntry}; use pacquet_network::ThrottledClient; use pacquet_package_manifest::DependencyGroup; @@ -225,6 +226,19 @@ where _ => None, }; + // Convert `pacquet-config`'s mirror enum to the executor's + // canonical type. Config's enum carries the yaml-deserialize + // impl; the executor's stays free of serde wiring. See the + // doc on [`pacquet_config::ScriptsPrependNodePath`] for the + // rationale. + let scripts_prepend_node_path = match config.scripts_prepend_node_path { + pacquet_config::ScriptsPrependNodePath::Always => ExecScriptsPrependNodePath::Always, + pacquet_config::ScriptsPrependNodePath::Never => ExecScriptsPrependNodePath::Never, + pacquet_config::ScriptsPrependNodePath::WarnOnly => { + ExecScriptsPrependNodePath::WarnOnly + } + }; + let ignored_builds = BuildModules { virtual_store_dir: &config.virtual_store_dir, modules_dir: &config.modules_dir, @@ -240,6 +254,9 @@ where store_dir: Some(&config.store_dir), store_index_writer: Some(&store_index_writer), patches: patches.as_ref(), + scripts_prepend_node_path, + unsafe_perm: config.unsafe_perm, + child_concurrency: config.child_concurrency, } .run::() .map_err(InstallFrozenLockfileError::BuildModules)?; diff --git a/pacquet/crates/package-manager/src/install_package_from_registry/tests.rs b/pacquet/crates/package-manager/src/install_package_from_registry/tests.rs index 843191be1e..62dc731cf6 100644 --- a/pacquet/crates/package-manager/src/install_package_from_registry/tests.rs +++ b/pacquet/crates/package-manager/src/install_package_from_registry/tests.rs @@ -46,6 +46,9 @@ fn create_config(store_dir: &Path, modules_dir: &Path, virtual_store_dir: &Path) patched_dependencies: None, allow_builds: Default::default(), dangerously_allow_all_builds: false, + scripts_prepend_node_path: Default::default(), + unsafe_perm: true, + child_concurrency: 1, } }