feat: supportedArchitectures config + --cpu/--os/--libc CLI + real libc detection (#434 slice 2)#456
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (14)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
📝 WalkthroughWalkthroughReads workspace ChangesArchitecture Overrides: Config, CLI, and Pipeline
Sequence DiagramsequenceDiagram
participant User as CLI/User
participant Workspace as Workspace YAML / Config
participant CLIArgs as SupportedArchitecturesArgs
participant Add as Add::run
participant Install as Install::run
participant Frozen as InstallFrozenLockfile::run
participant Host as InstallabilityHost
User->>CLIArgs: provide --cpu/--os/--libc
Workspace->>Add: config.supported_architectures
CLIArgs->>Add: apply_to(config)
Add->>Install: merged supported_architectures
Install->>Frozen: supported_architectures.as_ref()
Frozen->>Host: host.supported_architectures = merged.clone()
Host->>Host: compute_skipped_snapshots()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
(Note: only strongly related PRs included.) Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #456 +/- ##
==========================================
- Coverage 87.35% 87.32% -0.04%
==========================================
Files 113 114 +1
Lines 9356 9435 +79
==========================================
+ Hits 8173 8239 +66
- Misses 1183 1196 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
650e4ca to
4869aad
Compare
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.5056810511800003,
"stddev": 0.06704103664828652,
"median": 2.49227248538,
"user": 2.62552852,
"system": 3.4133552599999994,
"min": 2.43075478138,
"max": 2.63020043138,
"times": [
2.46425531738,
2.5110531333800004,
2.63020043138,
2.5181211983800003,
2.4510501753800003,
2.43640916938,
2.57933636038,
2.43075478138,
2.47349183738,
2.56213810738
]
},
{
"command": "pacquet@main",
"mean": 2.4265209244800006,
"stddev": 0.0368948408760975,
"median": 2.42299495638,
"user": 2.62806242,
"system": 3.39463406,
"min": 2.3754646953800003,
"max": 2.49894611338,
"times": [
2.41399232438,
2.45376945138,
2.49894611338,
2.38739630838,
2.44587993238,
2.4319975883800002,
2.3754646953800003,
2.4497292613800004,
2.40151989938,
2.4065136703800003
]
},
{
"command": "pnpm",
"mean": 5.883836167779999,
"stddev": 0.05679152222733026,
"median": 5.886599115879999,
"user": 8.67837052,
"system": 4.3440667600000005,
"min": 5.78903452338,
"max": 5.9447208493799994,
"times": [
5.92969064538,
5.8887709373799995,
5.9447208493799994,
5.8205141103799996,
5.78903452338,
5.88442729438,
5.8776969573799995,
5.82263836738,
5.93860503438,
5.94226295838
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.7009244933600002,
"stddev": 0.028768796960718424,
"median": 0.6936816721600001,
"user": 0.3599598,
"system": 1.5085419999999998,
"min": 0.6708365791600001,
"max": 0.7571468181600001,
"times": [
0.7571468181600001,
0.68162578716,
0.71991462216,
0.7014290981600001,
0.72294015716,
0.6729365081600001,
0.6859342461600001,
0.7221373131600001,
0.6743438041600001,
0.6708365791600001
]
},
{
"command": "pacquet@main",
"mean": 0.7864283630600002,
"stddev": 0.06116748073806874,
"median": 0.77932895416,
"user": 0.35689479999999996,
"system": 1.5329089,
"min": 0.7052754871600001,
"max": 0.91717990116,
"times": [
0.91717990116,
0.72006694816,
0.7592199671600001,
0.7052754871600001,
0.8443794961600001,
0.7964348171600001,
0.79014373916,
0.7591049511600001,
0.8039641541600001,
0.76851416916
]
},
{
"command": "pnpm",
"mean": 2.4947091115599997,
"stddev": 0.17042456809704956,
"median": 2.4273988616599995,
"user": 2.8885353999999994,
"system": 2.2196869,
"min": 2.29208741716,
"max": 2.8136389661599996,
"times": [
2.8136389661599996,
2.77121429016,
2.4065055961599997,
2.46749658716,
2.4227922181599997,
2.43200550516,
2.29208741716,
2.3995086671599997,
2.55564059216,
2.38620127616
]
}
]
} |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/package-manager/src/install.rs (1)
274-287:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCLI architecture overrides are dropped on the no-lockfile install path.
On Line 286, the merged
supported_architecturesis forwarded only toInstallFrozenLockfile, but on Line 294 theInstallWithoutLockfilebranch receives no equivalent value. That makes--cpu/--os/--libcineffective for the non-frozen flow (the common path whenconfig.lockfileis false).💡 Suggested fix direction
} else { InstallWithoutLockfile { tarball_mem_cache, resolved_packages, http_client, config, manifest, dependency_groups, logged_methods: &logged_methods, requester: &prefix, + supported_architectures: supported_architectures.as_ref(), } .run::<R>() .await .map_err(InstallError::WithoutLockfile)?; }Also add the matching field to
InstallWithoutLockfileand thread it into its installability checks.Also applies to: 294-303
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/package-manager/src/install.rs` around lines 274 - 287, The non-frozen install branch drops CLI architecture overrides because supported_architectures is only passed into InstallFrozenLockfile; add a matching supported_architectures field to the InstallWithoutLockfile struct construction and to its definition, then propagate that value into the installability checks used by InstallWithoutLockfile (the same checks that use supported_architectures in InstallFrozenLockfile) so --cpu/--os/--libc are respected on the no-lockfile path; update any functions/methods called by InstallWithoutLockfile that compute installability to accept and use the supported_architectures parameter (or read it from the struct) to ensure parity with InstallFrozenLockfile.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cli/src/cli_args/supported_architectures.rs`:
- Line 34: The clap attribute `global = true` was incorrectly applied to fields
inside the flattened Args struct; remove the `global = true` from the fields in
SupportedArchitecturesArgs so the architecture flags are only scoped to the
subcommands they are flattened into (InstallArgs and AddArgs). Locate the struct
SupportedArchitecturesArgs and drop `global = true` on its flagged fields (the
ones annotated with #[clap(long, value_delimiter = ',', num_args = 1.., ...)]),
leaving the other clap attributes intact; do not change the top-level CliArgs
`--reporter` global usage.
---
Outside diff comments:
In `@crates/package-manager/src/install.rs`:
- Around line 274-287: The non-frozen install branch drops CLI architecture
overrides because supported_architectures is only passed into
InstallFrozenLockfile; add a matching supported_architectures field to the
InstallWithoutLockfile struct construction and to its definition, then propagate
that value into the installability checks used by InstallWithoutLockfile (the
same checks that use supported_architectures in InstallFrozenLockfile) so
--cpu/--os/--libc are respected on the no-lockfile path; update any
functions/methods called by InstallWithoutLockfile that compute installability
to accept and use the supported_architectures parameter (or read it from the
struct) to ensure parity with InstallFrozenLockfile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 872ec02a-8842-4d93-8a98-fef72a829b74
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
crates/cli/Cargo.tomlcrates/cli/src/cli_args.rscrates/cli/src/cli_args/add.rscrates/cli/src/cli_args/install.rscrates/cli/src/cli_args/supported_architectures.rscrates/config/Cargo.tomlcrates/config/src/lib.rscrates/config/src/workspace_yaml.rscrates/config/src/workspace_yaml/tests.rscrates/graph-hasher/src/engine_name.rscrates/package-manager/src/add.rscrates/package-manager/src/install.rscrates/package-manager/src/install/tests.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/install_package_from_registry/tests.rscrates/package-manager/src/installability/tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Preserve existing method chains andpipe-traitchains; do not break them into intermediateletbindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;for any glob whose target is a module you control. External-crate preludes (e.g.,use rayon::prelude::*;) and root-of-module re-exports (e.g.,pub use submodule::*;inlib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plainStringor&str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only viaTryFrom<String>and/orFromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_str_unchecked(or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...
Files:
crates/cli/src/cli_args.rscrates/cli/src/cli_args/supported_architectures.rscrates/config/src/lib.rscrates/package-manager/src/add.rscrates/cli/src/cli_args/install.rscrates/package-manager/src/install.rscrates/package-manager/src/install_frozen_lockfile.rscrates/config/src/workspace_yaml/tests.rscrates/config/src/workspace_yaml.rscrates/package-manager/src/install_package_from_registry/tests.rscrates/package-manager/src/install/tests.rscrates/graph-hasher/src/engine_name.rscrates/cli/src/cli_args/add.rscrates/package-manager/src/installability/tests.rs
🧠 Learnings (2)
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.
Applied to files:
crates/cli/src/cli_args.rscrates/cli/src/cli_args/supported_architectures.rscrates/config/src/lib.rscrates/package-manager/src/add.rscrates/cli/src/cli_args/install.rscrates/package-manager/src/install.rscrates/package-manager/src/install_frozen_lockfile.rscrates/config/src/workspace_yaml/tests.rscrates/config/src/workspace_yaml.rscrates/package-manager/src/install_package_from_registry/tests.rscrates/package-manager/src/install/tests.rscrates/graph-hasher/src/engine_name.rscrates/cli/src/cli_args/add.rscrates/package-manager/src/installability/tests.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.
Applied to files:
crates/config/src/workspace_yaml/tests.rscrates/package-manager/src/install_package_from_registry/tests.rscrates/package-manager/src/install/tests.rscrates/package-manager/src/installability/tests.rs
🔇 Additional comments (16)
crates/graph-hasher/src/engine_name.rs (1)
130-190: LGTM!crates/cli/Cargo.toml (1)
18-29: LGTM!crates/config/Cargo.toml (1)
14-16: LGTM!crates/config/src/lib.rs (1)
437-450: LGTM!crates/package-manager/src/install_package_from_registry/tests.rs (1)
55-55: LGTM!crates/cli/src/cli_args.rs (1)
5-5: LGTM!crates/package-manager/src/install/tests.rs (1)
56-56: LGTM!Also applies to: 109-109, 145-145, 210-210, 268-268, 343-343, 403-403, 483-483, 620-620, 713-713, 819-819, 903-903, 1001-1001, 1043-1043, 1122-1122, 1209-1209, 1259-1259
crates/package-manager/src/add.rs (1)
29-32: LGTM!Also applies to: 63-63, 91-91
crates/cli/src/cli_args/install.rs (1)
48-84: LGTM!crates/cli/src/cli_args/add.rs (1)
75-117: LGTM!crates/package-manager/src/installability/tests.rs (1)
349-419: LGTM!crates/config/src/workspace_yaml.rs (1)
141-151: LGTM!Also applies to: 296-298
crates/package-manager/src/install_frozen_lockfile.rs (1)
78-83: LGTM!Also applies to: 215-234
crates/config/src/workspace_yaml/tests.rs (1)
459-515: LGTM!crates/cli/src/cli_args/supported_architectures.rs (2)
59-78: LGTM!
86-131: LGTM!
|
On CodeRabbit's outside-diff note about Slice 1 (#439) explicitly scoped the installability hook to the frozen-lockfile pipeline; this slice mirrors that. Written by an agent (Claude Code, claude-opus-4-7). |
| /// [`InstallabilityHost`]: pacquet_package_is_installable::SupportedArchitectures | ||
| pub supported_architectures: Option<pacquet_package_is_installable::SupportedArchitectures>, |
There was a problem hiding this comment.
Good catch — pacquet-config doesn't depend on pacquet-package-manager (where InstallabilityHost actually lives), so rustdoc was happily collapsing the broken intra-doc reference onto the closest matching name in scope (SupportedArchitectures). Rewrote the docstring as prose that names pacquet-package-manager's InstallabilityHost without a link, since adding the crate as a dep just for a doc target would be a layering inversion. Fixed in b933e52.
Written by an agent (Claude Code, claude-opus-4-7).
…bc detection (#434 slice 2) Slice 1 (#439) wired `pacquet-package-is-installable` into the install pipeline but the `SupportedArchitectures` input was always `None`, so every install behaved as if the user had opted into "host triple only". This slice closes that gap by wiring the three upstream input sources pinned at pnpm/pnpm@94240bc046: - `Config.supported_architectures` deserialized from `pnpm-workspace.yaml`, mirroring `getOptionsFromRootManifest.ts`. - `--cpu` / `--os` / `--libc` flags on `install` / `add`, multi-valued, comma-separated. Per-axis CLI override replaces the yaml axis wholesale, matching `overrideSupportedArchitecturesWithCLI.ts`. - Real Linux libc detection — replaces the slice 1 `"unknown"` stub with a `/lib*/ld-musl-*` probe cached via `OnceLock`. Mirrors `detect-libc.familySync()` semantics: `"musl"` / `"glibc"` on Linux, `"unknown"` everywhere else. No new dep — open-coded `read_dir` is cheaper than spawning `ldd --version` and works in slim containers without `ldd` on PATH. Threaded through `Install` / `InstallFrozenLockfile` / `Add` as an explicit field instead of mutating `State.config`, because `State.config` is `&'static Config` — the CLI override merge happens at the CLI layer and the fully-resolved value lands on the install pipeline. Tests: - `workspace_yaml/tests.rs` — yaml round-trip across `os` / `cpu` / `libc`, omission, and partial-axis cases. - `installability/tests.rs` — `supportedArchitectures` widens the accept set so a darwin-only package stays on a linux host when the user lists `darwin`; conversely, listing `linux` keeps the darwin-only package skipped (no implicit host include). Out of scope (umbrella slice 3+): `.modules.yaml.skipped` persistence, current-lockfile diffing for `removeOptionalDependenciesThatAreNotUsed`, and `--no-optional` plumbing. Closes #453.
…elds `global = true` is for fields on the top-level `Parser` struct so they propagate to every subcommand. On an `Args` struct that is `#[clap(flatten)]`'d into specific subcommand argument types, the flatten itself already scopes the flags — `global` is redundant and risks surfacing the same flag in unrelated subcommands if clap's behavior changes. Verified `pacquet install --help` / `pacquet add --help` still list `--cpu` / `--os` / `--libc`, and `pacquet run --help` does not.
`pacquet-config` doesn't depend on `pacquet-package-manager`, so the `[InstallabilityHost]` reference was resolving to `SupportedArchitectures` in `pacquet-package-is-installable` rather than the type it actually named. Rewrite as prose pointing at the package-manager crate by name so the reader still knows where to look, without a misleading link.
… sites added by GVS PR
`Install { ... }` literals at lines 1349, 1418, 1493 were added by
the global-virtual-store activation PR (#449) that landed on `main`
while this branch was open. They were missing the
`supported_architectures` field this PR introduced.
`None` for all three since none exercise platform-tagged optional
dependencies.
b933e52 to
fb213e5
Compare
Pull in 28 commits from upstream main, including the `pacquet-npmrc` → `pacquet-config` rename (#420) plus features: - supportedArchitectures + --cpu/--os/--libc (#456) - frozen-lockfile (#442, #443, #447, #450) - git-fetcher (#436 / #446, #451, #454) - side-effects cache (#421 / #422, #423, #424) - real-hoist + global-virtual-store (#432 / #438 / #444, #449, #452) - patchedDependencies + allow-builds (#425, #427, #428) - engine/platform installability (#434 / #439) Conflicts resolved: - `crates/npmrc/` files migrated under the renamed `crates/config/` directory; `Npmrc` → `Config` everywhere except `NpmrcAuth` (which keeps the `.npmrc`-domain name). - `Config::current` reads the env-var DI generic `Api: EnvVar` for ${VAR}-substitution in `.npmrc`. Production turbofish in `cli_args.rs` is `Config::current::<RealApi, _, _, _, _>(...)`. - Two-phase `NpmrcAuth::apply_*` retained so default-registry creds key at the yaml-resolved registry URL. - New `Config::auth_headers` field plumbed through `install_package_by_snapshot`'s `DownloadTarballToStore`. - Tests under `crates/config/src/workspace_yaml/tests.rs` pick up the new ParseYaml unit test added on this branch.
… slice 3) (#467) ## Summary Slice 3 of the [#434 umbrella](#434) (`Proper optionalDependencies support`). Slice 1 (#439) computed a `SkippedSnapshots` set on every frozen-lockfile install but never serialized it; slice 2 (#456) closed the input side. This PR closes the loop by mirroring three upstream sites pinned at [pnpm/pnpm@94240bc046](https://github.com/pnpm/pnpm/tree/94240bc046): - **Write** — `Modules.skipped` was declared at [`crates/modules-yaml/src/lib.rs:197`](https://github.com/pnpm/pacquet/blob/b13b8180/crates/modules-yaml/src/lib.rs#L197) with sort-on-write but always serialized empty. `build_modules_manifest` now takes `&SkippedSnapshots` and produces the depPath string list pnpm writes at [`installing/deps-installer/src/install/index.ts:1625`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/index.ts#L1625). - **Seed** — `install_frozen_lockfile::run` reads `.modules.yaml.skipped` before computing the in-memory set and passes the parsed `PackageKey`s as the seed to `compute_skipped_snapshots`. Mirrors upstream's load at [`installing/read-projects-context/src/index.ts:79`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/read-projects-context/src/index.ts#L79) plus the early return at [`deps/graph-builder/src/lockfileToDepGraph.ts:194`](https://github.com/pnpm/pnpm/blob/94240bc046/deps/graph-builder/src/lockfileToDepGraph.ts#L194). - **Short-circuit** — `compute_skipped_snapshots` returns the seed verbatim on the fast path (no constraints in the lockfile) and, on the slow path, skips the per-snapshot re-check for keys already in the seed without emitting a duplicate `pnpm:skipped-optional-dependency` event. Non-seeded snapshots still re-run `package_is_installable`, covering the \"host arch changed since last install\" case upstream guards at [`:206-215`](https://github.com/pnpm/pnpm/blob/94240bc046/deps/graph-builder/src/lockfileToDepGraph.ts#L206-L215). `InstallFrozenLockfile::run` now returns a small `InstallFrozenLockfileOutput { hoisted_dependencies, skipped }` struct so `Install::run` can thread both into `.modules.yaml`. Read errors on `.modules.yaml` degrade to an empty seed — the file is a cache artifact, not authoritative.
…e payload (#434 slice 4) (#474) Slice 4 of the [#434 umbrella](#434) (`Proper optionalDependencies support`). Slices 1–3 (#439, #456, #467) close the installability-driven skip path; this slice adds the second skip pathway upstream handles in the frozen install: an `optional: true` snapshot whose **fetch / extract** step fails at install time must not abort the install. Local-materialization errors (`CreateVirtualDir`) and config-shape errors still abort even for optional snapshots — matching upstream's `linkPkg` path which sits outside the catch. Mirrors the silent catch sites at [`deps/graph-builder/src/lockfileToDepGraph.ts:294-298`](https://github.com/pnpm/pnpm/blob/94240bc046/deps/graph-builder/src/lockfileToDepGraph.ts#L294-L298) and [`installing/deps-restorer/src/index.ts:912-921`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/src/index.ts#L912-L921): ```ts try { ... fetch ... } catch (err) { if (pkgSnapshot.optional) return; throw err } ``` ## Changes - **Reporter**: `SkippedOptionalPackage` becomes a `#[serde(untagged)]` enum with `Installed { id, name, version }` and `ResolutionFailure { name?, version?, bareSpecifier }` variants. Models upstream's discriminated union at [`core-loggers/src/skippedOptionalDependencyLogger.ts:10-31`](https://github.com/pnpm/pnpm/blob/94240bc046/core/core-loggers/src/skippedOptionalDependencyLogger.ts#L10-L31). The new variant is wire-shape-only in slice 4 — pacquet has no resolver yet, but a future resolver port at the upstream emit site ([`resolveDependencies.ts:1376-1383`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-resolver/src/resolveDependencies.ts#L1376-L1383)) lands without re-touching this type. - **`SkippedSnapshots`**: gains a `fetch_failed` subset disjoint from the existing `installability` subset. `contains` / `iter` return the union (downstream consumers treat both as absent); `iter_installability` returns only the persistent subset that `.modules.yaml.skipped` records, matching upstream's behavior of never updating `opts.skipped` at the fetch-failure catch sites. - **`CreateVirtualStore`**: cold-batch dispatch swallows per-snapshot errors when `snapshot.optional` is true **and** the error is fetch-side. A new `is_fetch_side_failure` helper restricts the swallow to `InstallPackageBySnapshotError::DownloadTarball` and `::GitFetch` — the same surface upstream wraps in `storeController.fetchPackage`. Local-materialization (`CreateVirtualDir`) and config-shape errors (`MissingTarballIntegrity` / `UnsupportedResolution`) propagate even for optional snapshots, matching upstream's `linkPkg` path which sits outside the catch. Side benefit: the warm-batch path (which only surfaces `CreateVirtualDir` errors) stays consistently abort-on-error without a parallel swallow site. The per-install `fetch_failed: HashSet<PackageKey>` rides out on `CreateVirtualStoreOutput`. - **`InstallFrozenLockfile::run`**: folds the returned `fetch_failed` into its mutable `SkippedSnapshots` so downstream phases (`SymlinkDirectDependencies`, `LinkVirtualStoreBins`, `BuildModules`, hoist) treat the dropped snapshots as absent through the same gate they already use for installability skips. ## Test plan - [x] `reporter::tests::skipped_optional_resolution_failure_event_matches_pnpm_wire_shape` — full payload (`bareSpecifier` camelCase, no `id`). - [x] `reporter::tests::skipped_optional_resolution_failure_omits_absent_name_and_version` — optional-field shape. - [x] `install::tests::frozen_install_silently_swallows_unreachable_optional_tarball` — ports [`installing/deps-restorer/test/index.ts:340-360`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/test/index.ts#L340-L360): unreachable optional tarball (`http://127.0.0.1:1/...`), install succeeds, slot not created, `.modules.yaml.skipped` left empty. **Test-the-test verified** by inverting the `optional` guard in the cold-batch match arm. Runs with `enable_global_virtual_store = false` so the slot-path assertion targets the legacy layout. - [x] `install::tests::frozen_install_propagates_non_optional_fetch_failure` — pins the polarity: same fixture, non-optional snapshot, install must error. - [x] `just ready` (typos + fmt + check + nextest + clippy). - [x] `taplo format --check`. - [x] `just dylint` (perfectionist). - [x] `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace`. ## Out of scope - Resolver-side `resolution_failure` emit site — needs a resolver, tracked separately. - `--no-optional` / `--omit=optional` plumbing — umbrella slice 5. - Surfacing fetch failures to the reporter on the frozen path — upstream itself is silent here; only the resolver-side emit fires `pnpm:skipped-optional-dependency`. Closes #471.
…#485) Slice 5 of the [#434 umbrella](#434) (`Proper optionalDependencies support`). Slices 1–4 (#439, #456, #467, #474) handle installability + the fetch-failure swallow. This slice closes the user-facing `--no-optional` flag: the CLI flag already exists and threads through `Install::dependency_groups`, but only `SymlinkDirectDependencies` and the on-disk `included` field actually honored it. The rest of the install pipeline walked `optional_dependencies` unconditionally, so the optional subtree was still extracted, linked, and built. Mirrors upstream's depNode filter at [`installing/deps-installer/src/install/link.ts:109-111`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/link.ts#L109-L111): ```ts if (!opts.include.optionalDependencies) { depNodes = depNodes.filter(({ optional }) => !optional) } ``` ## Changes - **`SkippedSnapshots`**: gains a third disjoint subset `optional_excluded` alongside `installability` (slice 1) and `fetch_failed` (slice 4). The existing `contains` / `iter` already return the union, so every downstream consumer that filters by skip-set (`CreateVirtualStore`, `SymlinkDirectDependencies`, `BuildModules`, hoist, `link_bins`) now respects `--no-optional` through the same gate. `iter_installability` still returns only the persistent subset for `.modules.yaml.skipped` writes — `--no-optional` exclusions are transient (matching upstream's behavior of never updating `opts.skipped` at the filter site). - **`InstallFrozenLockfile::run`**: iterates the lockfile snapshots once and inserts every `snap.optional == true` entry into the new subset when the dispatch list doesn't contain `DependencyGroup::Optional`. The `SnapshotEntry::optional` flag is set by the resolver only when the snapshot is reachable **exclusively** through optional edges, so a snapshot reachable through any non-optional edge carries `optional: false` and survives the filter — covers [`optionalDependencies.ts:712`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/optionalDependencies.ts#L712) (`dependency that is both optional and non-optional is installed`).
Summary
Slice 2 of the #434 umbrella (
Proper optionalDependencies support). Slice 1 (#439) wiredpacquet-package-is-installableinto the install pipeline but theSupportedArchitecturesinput was alwaysNone, so every install behaved as if the user had opted into "host triple only". This PR closes that gap and replaces the slice 1 libc stub with a real Linux probe.Mirrors the three upstream input sources pinned at pnpm/pnpm@94240bc046:
Config.supported_architecturesfrompnpm-workspace.yaml— same shape as upstream'sgetOptionsFromRootManifest.ts(config/reader/src/getOptionsFromRootManifest.ts#L23).--cpu/--os/--libcflags oninstall/add— multi-valued, comma-separated. Per-axis CLI override replaces the yaml axis wholesale, matching upstream'soverrideSupportedArchitecturesWithCLI.ts.\"unknown\"stub with a/lib*/ld-musl-*probe cached viaOnceLock. Mirrorsdetect-libc.familySync()semantics:\"musl\"/\"glibc\"on Linux,\"unknown\"everywhere else. No new dep — open-codedread_diris cheaper than spawningldd --versionand works in slim containers withoutlddon PATH.Threaded through
Install/InstallFrozenLockfile/Addas an explicit field instead of mutatingState.config, becauseState.configis&'static Config— the CLI override merge happens at the CLI layer and the fully-resolved value lands on the install pipeline.Test plan
Unit tests added; full suite ran via
just ready+cargo doc --no-deps+taplo format --check+just dylint.cargo test -p pacquet-config workspace_yaml::tests::*supported_architectures*— yaml round-trip acrossos/cpu/libc, omission, partial-axis (3 tests).cargo test -p pacquet-package-manager --lib installability::tests::supported_architectures*—supportedArchitectureswidens the accept set so a darwin-only package stays on a linux host when the user listsdarwin; conversely, listinglinuxkeeps the darwin-only package skipped (no implicit host include).just ready— typos + fmt + check + nextest (792 tests pass) + clippy.taplo format --check.just dylint(perfectionist).RUSTDOCFLAGS=\"-D warnings\" cargo doc --no-deps --workspace.Out of scope
.modules.yaml.skippedpersistence (umbrella slice 3).removeOptionalDependenciesThatAreNotUsed(umbrella slice 6).--no-optionalplumbing (umbrella slice 5).Closes #453.
Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Improvements
Tests