Skip to content
This repository was archived by the owner on May 14, 2026. It is now read-only.

feat: supportedArchitectures config + --cpu/--os/--libc CLI + real libc detection (#434 slice 2)#456

Merged
zkochan merged 4 commits into
mainfrom
feat-supported-architectures
May 13, 2026
Merged

feat: supportedArchitectures config + --cpu/--os/--libc CLI + real libc detection (#434 slice 2)#456
zkochan merged 4 commits into
mainfrom
feat-supported-architectures

Conversation

@zkochan

@zkochan zkochan commented May 13, 2026

Copy link
Copy Markdown
Member

Summary

Slice 2 of the #434 umbrella (Proper optionalDependencies support). 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 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_architectures from pnpm-workspace.yaml — same shape as upstream's getOptionsFromRootManifest.ts (config/reader/src/getOptionsFromRootManifest.ts#L23).
  • --cpu / --os / --libc flags on install / add — multi-valued, comma-separated. Per-axis CLI override replaces the yaml axis wholesale, matching upstream's 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.

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 across os / cpu / libc, omission, partial-axis (3 tests).
  • cargo test -p pacquet-package-manager --lib installability::tests::supported_architectures*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).
  • 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.skipped persistence (umbrella slice 3).
  • Current-lockfile diffing for removeOptionalDependenciesThatAreNotUsed (umbrella slice 6).
  • --no-optional plumbing (umbrella slice 5).

Closes #453.


Written by an agent (Claude Code, claude-opus-4-7).

Summary by CodeRabbit

  • New Features

    • CLI flags (--cpu, --os, --libc) to override target platforms for add/install (repeating or comma-separated).
    • Workspace YAML can declare supportedArchitectures to preserve platform-tagged dependencies.
    • Improved libc detection on Linux (musl vs glibc).
  • Improvements

    • CLI and workspace platform constraints are merged and honored throughout add/install flows, including frozen-lockfile paths.
  • Tests

    • New unit tests covering CLI merging, workspace supportedArchitectures, and optional-dependency installability behavior.

Review Change Stack

Copilot AI review requested due to automatic review settings May 13, 2026 13:51
@coderabbitai

coderabbitai Bot commented May 13, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 548f38a0-de65-407f-97bc-39dffce44bd8

📥 Commits

Reviewing files that changed from the base of the PR and between b933e52 and fb213e5.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (16)
  • crates/cli/Cargo.toml
  • crates/cli/src/cli_args.rs
  • crates/cli/src/cli_args/add.rs
  • crates/cli/src/cli_args/install.rs
  • crates/cli/src/cli_args/supported_architectures.rs
  • crates/config/Cargo.toml
  • crates/config/src/lib.rs
  • crates/config/src/workspace_yaml.rs
  • crates/config/src/workspace_yaml/tests.rs
  • crates/graph-hasher/src/engine_name.rs
  • crates/package-manager/src/add.rs
  • crates/package-manager/src/install.rs
  • crates/package-manager/src/install/tests.rs
  • crates/package-manager/src/install_frozen_lockfile.rs
  • crates/package-manager/src/install_package_from_registry/tests.rs
  • crates/package-manager/src/installability/tests.rs
✅ Files skipped from review due to trivial changes (2)
  • crates/package-manager/src/install_package_from_registry/tests.rs
  • crates/package-manager/src/install/tests.rs
🚧 Files skipped from review as they are similar to previous changes (14)
  • crates/cli/src/cli_args.rs
  • crates/config/src/workspace_yaml/tests.rs
  • crates/cli/Cargo.toml
  • crates/config/src/lib.rs
  • crates/config/Cargo.toml
  • crates/graph-hasher/src/engine_name.rs
  • crates/cli/src/cli_args/add.rs
  • crates/cli/src/cli_args/install.rs
  • crates/package-manager/src/install_frozen_lockfile.rs
  • crates/package-manager/src/add.rs
  • crates/package-manager/src/installability/tests.rs
  • crates/package-manager/src/install.rs
  • crates/config/src/workspace_yaml.rs
  • crates/cli/src/cli_args/supported_architectures.rs
📜 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)
  • GitHub Check: Agent
  • GitHub Check: Code Coverage
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Lint and Test (macos-latest)

📝 Walkthrough

Walkthrough

Reads workspace supportedArchitectures and adds CLI flags --cpu/--os/--libc; merges CLI+YAML into an optional SupportedArchitectures, threads it through Add→Install→InstallFrozenLockfile, and applies it during frozen-lockfile installability checks. Adds Linux libc detection (musl vs glibc).

Changes

Architecture Overrides: Config, CLI, and Pipeline

Layer / File(s) Summary
CLI argument parsing and module export
crates/cli/src/cli_args.rs, crates/cli/src/cli_args/supported_architectures.rs
Adds SupportedArchitecturesArgs clap Args (multi-valued --cpu, --os, --libc), exposes it from cli_args, and implements apply_to() to merge CLI overrides into existing config.
CLI integration into add/install commands
crates/cli/src/cli_args/add.rs, crates/cli/src/cli_args/install.rs
Flattens SupportedArchitecturesArgs into AddArgs and InstallArgs; computes merged supported_architectures in run by applying CLI overrides onto config clone and passes the merged value into the pipeline.
Workspace YAML deserialization and Config storage
crates/config/Cargo.toml, crates/config/src/lib.rs, crates/config/src/workspace_yaml.rs, crates/config/src/workspace_yaml/tests.rs
Adds Config.supported_architectures; deserializes supportedArchitectures from pnpm-workspace.yaml into WorkspaceSettings and applies it to Config. Tests cover present, absent, and partial YAML cases.
Real libc detection: musl vs glibc
crates/graph-hasher/src/engine_name.rs
Reimplements host_libc() with a cached detector. On Linux probes /lib and /lib64 for ld-musl-* to detect musl; otherwise returns glibc; non-Linux returns unknown.
Package manager pipeline: Add → Install → InstallFrozenLockfile
crates/package-manager/src/add.rs, crates/package-manager/src/install.rs, crates/package-manager/src/install_frozen_lockfile.rs, crates/package-manager/src/install/tests.rs, crates/package-manager/src/installability/tests.rs, crates/package-manager/src/install_package_from_registry/tests.rs
Adds optional supported_architectures fields to Add, Install, and InstallFrozenLockfile. The merged CLI+YAML value is forwarded Add→Install→InstallFrozenLockfile; the frozen-lockfile path applies it to InstallabilityHost before computing skipped snapshots. Tests updated to initialize the new field and new unit tests validate widened and constrained optional-dependency semantics.
CLI manifest formatting
crates/cli/Cargo.toml
Reformats pacquet-* workspace dependency entries (no semantic change).

Sequence Diagram

sequenceDiagram
  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()
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • pnpm/pacquet#439: Introduced installability platform checks that this PR feeds architecture overrides into.
  • pnpm/pacquet#439: (duplicate listed by relevance) Same upstream slice for threading supportedArchitectures.
  • pnpm/pacquet#439: (included to preserve top matches)

(Note: only strongly related PRs included.)

Suggested reviewers

  • anthonyshew

"🐰 Flags in my paws, configs in my tote,
Musl or glibc I now gently note,
Axes merge and thread, pipelines hum slow,
Multi‑arch carrots sprout — hop, off we go!"

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: adding supportedArchitectures config from YAML, CLI flags (--cpu/--os/--libc), and real libc detection.
Description check ✅ Passed The description covers all required template sections: summary, linked issue (#453), upstream reference (pnpm/pnpm@94240bc046), and checklist items including pnpm behavior match, testing via 'just ready', and test additions.
Linked Issues check ✅ Passed The PR fully implements the coding requirements from issue #453: Config.supported_architectures from YAML [#453], --cpu/--os/--libc CLI flags on install/add [#453], real Linux libc detection replacing the stub [#453], and explicit threading through Install/InstallFrozenLockfile/Add pipelines [#453]. Unit tests added for YAML parsing, CLI merge logic, and installability behavior [#453].
Out of Scope Changes check ✅ Passed All changes directly support the linked issue #453 objectives: YAML config parsing, CLI flag handling, libc detection, and pipeline threading. The PR correctly respects scope boundaries by excluding .modules.yaml.skipped persistence, current-lockfile diffing, and --no-optional plumbing as noted in the description.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat-supported-architectures

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented May 13, 2026

Copy link
Copy Markdown

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.00     16.2±0.56ms   268.0 KB/sec    1.03     16.7±0.95ms   259.3 KB/sec

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@codecov

codecov Bot commented May 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.72289% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.32%. Comparing base (99d708a) to head (fb213e5).

Files with missing lines Patch % Lines
crates/graph-hasher/src/engine_name.rs 0.00% 12 Missing ⚠️
...tes/package-manager/src/install_frozen_lockfile.rs 20.00% 4 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zkochan zkochan force-pushed the feat-supported-architectures branch from 650e4ca to 4869aad Compare May 13, 2026 13:57
@github-actions

github-actions Bot commented May 13, 2026

Copy link
Copy Markdown

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.506 ± 0.067 2.431 2.630 1.03 ± 0.03
pacquet@main 2.427 ± 0.037 2.375 2.499 1.00
pnpm 5.884 ± 0.057 5.789 5.945 2.42 ± 0.04
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)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 700.9 ± 28.8 670.8 757.1 1.00
pacquet@main 786.4 ± 61.2 705.3 917.2 1.12 ± 0.10
pnpm 2494.7 ± 170.4 2292.1 2813.6 3.56 ± 0.28
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
      ]
    }
  ]
}

@zkochan

zkochan commented May 13, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 13, 2026

Copy link
Copy Markdown
✅ Actions performed

Full review triggered.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

CLI architecture overrides are dropped on the no-lockfile install path.

On Line 286, the merged supported_architectures is forwarded only to InstallFrozenLockfile, but on Line 294 the InstallWithoutLockfile branch receives no equivalent value. That makes --cpu/--os/--libc ineffective for the non-frozen flow (the common path when config.lockfile is 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 InstallWithoutLockfile and 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8b35794 and 4869aad.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (16)
  • crates/cli/Cargo.toml
  • crates/cli/src/cli_args.rs
  • crates/cli/src/cli_args/add.rs
  • crates/cli/src/cli_args/install.rs
  • crates/cli/src/cli_args/supported_architectures.rs
  • crates/config/Cargo.toml
  • crates/config/src/lib.rs
  • crates/config/src/workspace_yaml.rs
  • crates/config/src/workspace_yaml/tests.rs
  • crates/graph-hasher/src/engine_name.rs
  • crates/package-manager/src/add.rs
  • crates/package-manager/src/install.rs
  • crates/package-manager/src/install/tests.rs
  • crates/package-manager/src/install_frozen_lockfile.rs
  • crates/package-manager/src/install_package_from_registry/tests.rs
  • crates/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 and pipe-trait chains; do not break them into intermediate let bindings 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 (&Path over &PathBuf, &str over &String) when it does not force extra copies.
Prefer Arc::clone(&x) and Rc::clone(&x) over x.clone() for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Write use super::{Foo, bar} instead of use 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::*; in lib.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 plain String or &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 via TryFrom<String> and/or FromStr; 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 infallible From<String> and From<&str> constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bare as assertion), add a from_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.rs
  • crates/cli/src/cli_args/supported_architectures.rs
  • crates/config/src/lib.rs
  • crates/package-manager/src/add.rs
  • crates/cli/src/cli_args/install.rs
  • crates/package-manager/src/install.rs
  • crates/package-manager/src/install_frozen_lockfile.rs
  • crates/config/src/workspace_yaml/tests.rs
  • crates/config/src/workspace_yaml.rs
  • crates/package-manager/src/install_package_from_registry/tests.rs
  • crates/package-manager/src/install/tests.rs
  • crates/graph-hasher/src/engine_name.rs
  • crates/cli/src/cli_args/add.rs
  • crates/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.rs
  • crates/cli/src/cli_args/supported_architectures.rs
  • crates/config/src/lib.rs
  • crates/package-manager/src/add.rs
  • crates/cli/src/cli_args/install.rs
  • crates/package-manager/src/install.rs
  • crates/package-manager/src/install_frozen_lockfile.rs
  • crates/config/src/workspace_yaml/tests.rs
  • crates/config/src/workspace_yaml.rs
  • crates/package-manager/src/install_package_from_registry/tests.rs
  • crates/package-manager/src/install/tests.rs
  • crates/graph-hasher/src/engine_name.rs
  • crates/cli/src/cli_args/add.rs
  • crates/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.rs
  • crates/package-manager/src/install_package_from_registry/tests.rs
  • crates/package-manager/src/install/tests.rs
  • crates/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!

Comment thread crates/cli/src/cli_args/supported_architectures.rs Outdated
@zkochan

zkochan commented May 13, 2026

Copy link
Copy Markdown
Member Author

On CodeRabbit's outside-diff note about InstallWithoutLockfile dropping the overrides: that's intentional for slice 2. The no-lockfile path doesn't run an installability check today — there's no packages: metadata to evaluate os / cpu / libc / engines against without a lockfile, so the skip set is empty by construction (see install_without_lockfile.rs:209-218). Threading supported_architectures into it would be dead until the resolver gains optional-dep filtering, which is umbrella slice 4+.

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).

Copilot AI review requested due to automatic review settings May 13, 2026 14:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.

Comment thread crates/config/src/lib.rs Outdated
Comment on lines +449 to +450
/// [`InstallabilityHost`]: pacquet_package_is_installable::SupportedArchitectures
pub supported_architectures: Option<pacquet_package_is_installable::SupportedArchitectures>,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

zkochan added 4 commits May 13, 2026 17:10
…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.
Copilot AI review requested due to automatic review settings May 13, 2026 15:18
@zkochan zkochan force-pushed the feat-supported-architectures branch from b933e52 to fb213e5 Compare May 13, 2026 15:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.

@zkochan zkochan merged commit b13b818 into main May 13, 2026
18 of 20 checks passed
@zkochan zkochan deleted the feat-supported-architectures branch May 13, 2026 15:31
KSXGitHub pushed a commit that referenced this pull request May 13, 2026
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.
zkochan added a commit that referenced this pull request May 13, 2026
… 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.
zkochan added a commit that referenced this pull request May 13, 2026
…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.
zkochan added a commit that referenced this pull request May 13, 2026
…#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`).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

supportedArchitectures config + --cpu/--os/--libc CLI flags + real libc detection (#434 slice 2)

2 participants