This repository was archived by the owner on May 14, 2026. It is now read-only.
Conversation
Pacquet port of upstream pnpm's global-virtual-store (GVS) machinery, landing the pieces the install pipeline needs but not the install-pipeline wiring itself. Splits the work in #432 along its Section A/B/C/D boundary — sections A and C ship here as pure additions; section B (the path split through CreateVirtualStore / SymlinkDirectDependencies / BuildModules) and section D (ported upstream tests) are tracked as a follow-up. Config: - New `enable_global_virtual_store: bool` (default `true`, matching upstream's [config/reader/src/index.ts:392-394][1]) and `global_virtual_store_dir: PathBuf` fields on `Config`. - `pnpm-workspace.yaml` deserialization recognises both new keys. - New `Config::apply_global_virtual_store_derivation` ports the derivation block at upstream's [extendInstallOptions.ts:343-355][2]. Deliberately not called from `Config::current` yet — the install layer will invoke it once it consumes the new fields, so this PR doesn't silently redirect existing installs to `<store_dir>/links`. Store dir helpers: - `StoreDir::links()`, `StoreDir::projects()`, and `StoreDir::root()`. - `pacquet_store_dir::project_registry::register_project` ports upstream's [registerProject][3] (idempotent symlink at `<store_dir>/projects/<short-hash>` → project dir, with the `isSubdir` guard). - `pacquet_store_dir::create_short_hash` ports upstream's [createShortHash][4] (sha256 hex, first 32 chars). Pinned test vector against `printf pacquet | shasum -a 256 | head -c 32`. Graph hasher: - `pacquet_graph_hasher::calc_graph_node_hash` and `format_global_virtual_store_path` port upstream's [calcGraphNodeHash][5] / [formatGlobalVirtualStorePath][6] — the helpers that compute the `<scope>/<name>/<version>/<hash>` shape packages take inside the global virtual store. Engine string threads through but is always passed verbatim — the engine-agnostic gating from upstream's `allowBuilds`-driven branch is out of scope here and tracked separately. Package manager: - `pacquet_package_manager::VirtualStoreLayout` precomputes each snapshot's slot directory on disk. Hides the GVS-vs-legacy path divergence behind one `slot_dir(&PackageKey) -> PathBuf` lookup so the future install-pipeline integration doesn't have to branch on `Config::enable_global_virtual_store` at every call site. No behavioural change at install time. Every new field, helper, and module is unreachable from the existing install path; the follow-up B PR wires them up and updates the install tests to assert the new layout. [1]: https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/index.ts#L392-L394 [2]: https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/extendInstallOptions.ts#L343-L355 [3]: https://github.com/pnpm/pnpm/blob/94240bc046/store/controller/src/storeController/projectRegistry.ts [4]: https://github.com/pnpm/pnpm/blob/94240bc046/crypto/hash/src/index.ts [5]: https://github.com/pnpm/pnpm/blob/94240bc046/deps/graph-hasher/src/index.ts#L122-L146 [6]: https://github.com/pnpm/pnpm/blob/94240bc046/deps/graph-hasher/src/index.ts#L155-L160
|
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 selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds pnpm-style global virtual store: new config fields and derivation, dependency-graph hashing + path formatting for GVS names, VirtualStoreLayout to compute package slot dirs for GVS vs legacy, and a project registry implemented as symlinks. ChangesGlobal Virtual Store Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds the “foundation” pieces for pnpm-style global virtual store (GVS) support without wiring it into the install pipeline yet, including config fields, store-dir helpers, project registry utilities, graph hashing/path formatting, and a VirtualStoreLayout helper.
Changes:
- Add config fields + workspace YAML parsing for
enableGlobalVirtualStore/globalVirtualStoreDirand a derivation helper. - Add store-dir helpers (
links/projects/root) and a new project registry module (create_short_hash,register_project). - Add graph-hasher helpers for GVS hashing/path formatting and introduce
VirtualStoreLayoutto abstract slot path computation.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/store-dir/src/store_dir.rs | Adds store root sibling helpers for GVS (links, projects, root). |
| crates/store-dir/src/project_registry.rs | Introduces project registry slug + registration symlink logic with tests. |
| crates/store-dir/src/lib.rs | Exposes the new project registry module publicly. |
| crates/package-manager/src/virtual_store_layout.rs | Adds VirtualStoreLayout for GVS vs legacy slot directory computation + tests. |
| crates/package-manager/src/lib.rs | Exports the new virtual_store_layout module. |
| crates/package-manager/src/install_package_from_registry/tests.rs | Updates test config construction for new config fields. |
| crates/graph-hasher/src/lib.rs | Exposes GVS hashing/path formatting module. |
| crates/graph-hasher/src/global_virtual_store_path.rs | Implements calc_graph_node_hash and format_global_virtual_store_path + tests. |
| crates/graph-hasher/src/dep_state.rs | Makes calc_dep_graph_hash pub(crate) for reuse by GVS hasher. |
| crates/config/src/workspace_yaml.rs | Adds workspace YAML fields and applies global_virtual_store_dir override. |
| crates/config/src/lib.rs | Adds config fields + derivation method + tests and documents intentional non-wiring in current. |
| crates/config/src/defaults.rs | Adds default for enable_global_virtual_store (true). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
220
to
224
| side_effects_cache, side_effects_cache_readonly, | ||
| fetch_retries, fetch_retry_factor, | ||
| fetch_retry_mintimeout, fetch_retry_maxtimeout, | ||
| enable_global_virtual_store, | ||
| } |
Member
Author
There was a problem hiding this comment.
False alarm — the enable_global_virtual_store field is applied via the apply! macro at line 223 of workspace_yaml.rs (in the same change). The macro expands to if let Some(v) = self.enable_global_virtual_store { config.enable_global_virtual_store = v; }, so enableGlobalVirtualStore: false in yaml does land on the config. Verified by the gvs_disabled_keeps_project_local_virtual_store test in config/src/lib.rs, which writes that yaml line and asserts config.enable_global_virtual_store is false.
Written by an agent (Claude Code, claude-opus-4-7).
| store_dir: &StoreDir, | ||
| project_dir: &Path, | ||
| ) -> Result<(), RegisterProjectError> { | ||
| if is_subdir(project_dir, store_dir.root()) { |
Member
Author
There was a problem hiding this comment.
The previous code matched upstream semantics (is_subdir(parent, child) returning true when child is at or under parent — same as the npm is-subdir package), but the bare is_subdir name read ambiguously at the call site. Renamed to path_contains(outer, inner) in c00d8f0 — the directional name makes path_contains(project_dir, store_dir.root()) read unambiguously as "does project_dir contain store_dir.root()", matching upstream behavior. Added an inline comment cross-referencing upstream's isSubdir(projectDir, storeDir).
Written by an agent (Claude Code, claude-opus-4-7).
Comment on lines
+166
to
+170
| fn is_subdir(parent: &Path, child: &Path) -> bool { | ||
| let parent_canonical = dunce::canonicalize(parent).unwrap_or_else(|_| parent.to_path_buf()); | ||
| let child_canonical = dunce::canonicalize(child).unwrap_or_else(|_| child.to_path_buf()); | ||
| child_canonical.starts_with(&parent_canonical) | ||
| } |
Member
Author
There was a problem hiding this comment.
Same response as the duplicate above — renamed to path_contains(outer, inner) in c00d8f0 to remove the ambiguity. Semantics unchanged.
Written by an agent (Claude Code, claude-opus-4-7).
Comment on lines
+108
to
+110
| let name = metadata_key.name.to_string(); | ||
| let version = metadata_key.suffix.version().to_string(); | ||
| let suffix = format_global_virtual_store_path(&name, &version, &hex_digest); |
Member
Author
There was a problem hiding this comment.
Noted. Kept the to_string() allocations for now — this path runs once per snapshot during install setup (not in the per-file hot path), and the existing name/version accessors on PkgNameVerPeer.suffix only expose Display, not &str (scoped names render as @scope/bare dynamically). Threading impl Display through format_global_virtual_store_path would change the public signature of a pacquet_graph_hasher helper that mirrors upstream's formatGlobalVirtualStorePath(name, version, hexDigest). Worth revisiting if profiling on alot7-scale lockfiles flags it, but at hundreds of snapshots the allocations don't register against the surrounding I/O.
Written by an agent (Claude Code, claude-opus-4-7).
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #444 +/- ##
==========================================
+ Coverage 86.98% 87.22% +0.23%
==========================================
Files 93 96 +3
Lines 6647 7114 +467
==========================================
+ Hits 5782 6205 +423
- Misses 865 909 +44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Drop the redundant explicit link target on three intra-doc links in `VirtualStoreLayout`; the labels already resolve to the same path via the `use` imports / fully-qualified label, and `rustdoc -- -D warnings` was failing on `redundant_explicit_links`. - Rename `is_subdir(parent, child)` to `path_contains(outer, inner)` at the project-registry subdir guard. The semantics are unchanged (it still returns `true` when the inner path lives at or under the outer path) but the bare `is_subdir` name reads ambiguously at the call site — `is_subdir(project_dir, store_dir.root())` could be read as either order. `path_contains(outer, inner)` is directional. Add an inline comment at the call site cross- referencing upstream's `isSubdir(projectDir, storeDir)` for the matching parameter order.
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.56851055912,
"stddev": 0.06849063231544342,
"median": 2.57639310062,
"user": 2.5711441,
"system": 3.56074202,
"min": 2.4543348916200003,
"max": 2.67717994662,
"times": [
2.55538887162,
2.51677134462,
2.60330288462,
2.53243910862,
2.59739732962,
2.67717994662,
2.4543348916200003,
2.64045454862,
2.6067224606200003,
2.5011142046200003
]
},
{
"command": "pacquet@main",
"mean": 2.53015015752,
"stddev": 0.054607735991976616,
"median": 2.5483489011200002,
"user": 2.482087999999999,
"system": 3.56066322,
"min": 2.45251125462,
"max": 2.61092719962,
"times": [
2.5627119076200002,
2.51295438962,
2.56165839262,
2.45899586862,
2.5570474986200002,
2.46800832562,
2.45251125462,
2.61092719962,
2.53965030362,
2.57703643462
]
},
{
"command": "pnpm",
"mean": 6.104170280220001,
"stddev": 0.08169096791240726,
"median": 6.094271474619999,
"user": 8.8692043,
"system": 4.53418512,
"min": 5.966954647620001,
"max": 6.24959058862,
"times": [
6.13467689162,
6.01631313862,
6.08110677662,
6.10409793762,
6.19279777962,
5.966954647620001,
6.08444501162,
6.14229105162,
6.06942897862,
6.24959058862
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.67304127282,
"stddev": 0.06234074779092658,
"median": 0.65906638072,
"user": 0.35606768000000005,
"system": 1.46400834,
"min": 0.62502687072,
"max": 0.83990293372,
"times": [
0.83990293372,
0.69190326572,
0.62502687072,
0.67558851272,
0.66993668472,
0.6529023377200001,
0.63734654672,
0.66523042372,
0.63356239872,
0.63901275372
]
},
{
"command": "pacquet@main",
"mean": 0.69768437312,
"stddev": 0.05476254358218724,
"median": 0.68311637422,
"user": 0.36739017999999996,
"system": 1.49676174,
"min": 0.65411296672,
"max": 0.8423274107200001,
"times": [
0.69596938572,
0.65928794572,
0.65411296672,
0.66979157272,
0.8423274107200001,
0.68830101872,
0.67011167572,
0.67793172972,
0.72359741772,
0.69541260772
]
},
{
"command": "pnpm",
"mean": 2.65602473832,
"stddev": 0.11347993644432851,
"median": 2.6230787607200003,
"user": 3.15517718,
"system": 2.28561214,
"min": 2.54936314072,
"max": 2.86092397772,
"times": [
2.58578861772,
2.55338221572,
2.65193729272,
2.82242832372,
2.54936314072,
2.6055690297200003,
2.55473928972,
2.6405884917200004,
2.86092397772,
2.73552700372
]
}
]
} |
This was referenced May 13, 2026
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Threads `VirtualStoreLayout` through the frozen-lockfile install pipeline so packages installed under `enableGlobalVirtualStore: true` (pacquet's default since #444) land at the upstream-shaped `<store_dir>/links/<scope>/<name>/<version>/<hash>/node_modules/<pkg>` path instead of the legacy per-project flat layout. Closes the rest of #432 (Section B + C activation + D entry tests). Pipeline changes (frozen-lockfile path): - `InstallFrozenLockfile::run` constructs the install-scoped `VirtualStoreLayout` from the lockfile + engine_name + config, then threads `&VirtualStoreLayout` through `CreateVirtualStore`, `CreateVirtualDirBySnapshot`, `create_symlink_layout`, `SymlinkDirectDependencies`, `InstallPackageBySnapshot`, `BuildModules`, and `LinkVirtualStoreBins`. Every site that used to compute `virtual_store_dir.join(key.to_virtual_store_name())` now goes through one `layout.slot_dir(key)` lookup. - `Install::run` calls `pacquet_store_dir::register_project` when `enable_global_virtual_store` is on, writing `<store_dir>/projects/<short-hash>` so a future `pacquet store prune` can find the projects that still reference `<store_dir>/links/...` slots. Best-effort: registry write failures degrade to `tracing::warn!` and the install continues. - `Config::current` now applies `apply_global_virtual_store_derivation` after yaml — pacquet's variant of upstream's [`extendInstallOptions.ts:343-355`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/extendInstallOptions.ts#L343-L355). Field-split deviation from upstream: Upstream pnpm *mutates* `virtualStoreDir` in place under GVS, so every consumer that reads `virtualStoreDir` sees `<storeDir>/links`. Pacquet keeps `virtual_store_dir` at its project-local value and writes the GVS path into the separate `global_virtual_store_dir` field. The frozen-lockfile path consumes `global_virtual_store_dir` through `VirtualStoreLayout`; the legacy non-frozen `InstallWithoutLockfile` keeps reading `virtual_store_dir` (now via `VirtualStoreLayout::legacy`). The split is a pacquet-only concern: upstream has no without-lockfile install path and so doesn't need the second field. See the doc on `Config::apply_global_virtual_store_derivation` for the reasoning. Tests: - `install::tests::frozen_lockfile_under_gvs_registers_project_and_runs_clean` pins the registry write under default GVS-on. - `install::tests::frozen_lockfile_with_gvs_off_skips_project_registry` pins that GVS-off opts out of the registry write. - All 653 workspace tests pass under the new layout-threaded path. - Existing per-snapshot legacy tests construct `VirtualStoreLayout::legacy(...)` explicitly so they keep asserting the flat-name layout regardless of any global default. End-to-end port of upstream's [`installing/deps-installer/test/install/globalVirtualStore.ts`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/globalVirtualStore.ts) is tracked as a follow-up — those tests need a small frozen- lockfile fixture against the registry-mock, which is out of scope here. Based on #441 (`feat/438`); the `import_indexed_dir` rename from that PR is already merged in. The two PRs only co-touch `create_virtual_dir_by_snapshot.rs` — both pieces land cleanly on top of each other.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Threads `VirtualStoreLayout` through the frozen-lockfile install pipeline so packages installed under `enableGlobalVirtualStore: true` (pacquet's default since #444) land at the upstream-shaped `<store_dir>/links/<scope>/<name>/<version>/<hash>/node_modules/<pkg>` path instead of the legacy per-project flat layout. Closes the rest of #432 (Section B + C activation + D entry tests). Pipeline changes (frozen-lockfile path): - `InstallFrozenLockfile::run` constructs the install-scoped `VirtualStoreLayout` from the lockfile + engine_name + config, then threads `&VirtualStoreLayout` through `CreateVirtualStore`, `CreateVirtualDirBySnapshot`, `create_symlink_layout`, `SymlinkDirectDependencies`, `InstallPackageBySnapshot`, `BuildModules`, and `LinkVirtualStoreBins`. Every site that used to compute `virtual_store_dir.join(key.to_virtual_store_name())` now goes through one `layout.slot_dir(key)` lookup. - `Install::run` calls `pacquet_store_dir::register_project` when `enable_global_virtual_store` is on, writing `<store_dir>/projects/<short-hash>` so a future `pacquet store prune` can find the projects that still reference `<store_dir>/links/...` slots. Best-effort: registry write failures degrade to `tracing::warn!` and the install continues. - `Config::current` now applies `apply_global_virtual_store_derivation` after yaml — pacquet's variant of upstream's [`extendInstallOptions.ts:343-355`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/extendInstallOptions.ts#L343-L355). Field-split deviation from upstream: Upstream pnpm *mutates* `virtualStoreDir` in place under GVS, so every consumer that reads `virtualStoreDir` sees `<storeDir>/links`. Pacquet keeps `virtual_store_dir` at its project-local value and writes the GVS path into the separate `global_virtual_store_dir` field. The frozen-lockfile path consumes `global_virtual_store_dir` through `VirtualStoreLayout`; the legacy non-frozen `InstallWithoutLockfile` keeps reading `virtual_store_dir` (now via `VirtualStoreLayout::legacy`). The split is a pacquet-only concern: upstream has no without-lockfile install path and so doesn't need the second field. See the doc on `Config::apply_global_virtual_store_derivation` for the reasoning. Tests: - `install::tests::frozen_lockfile_under_gvs_registers_project_and_runs_clean` pins the registry write under default GVS-on. - `install::tests::frozen_lockfile_with_gvs_off_skips_project_registry` pins that GVS-off opts out of the registry write. - All 653 workspace tests pass under the new layout-threaded path. - Existing per-snapshot legacy tests construct `VirtualStoreLayout::legacy(...)` explicitly so they keep asserting the flat-name layout regardless of any global default. End-to-end port of upstream's [`installing/deps-installer/test/install/globalVirtualStore.ts`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/globalVirtualStore.ts) is tracked as a follow-up — those tests need a small frozen- lockfile fixture against the registry-mock, which is out of scope here. Based on #441 (`feat/438`); the `import_indexed_dir` rename from that PR is already merged in. The two PRs only co-touch `create_virtual_dir_by_snapshot.rs` — both pieces land cleanly on top of each other.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Threads `VirtualStoreLayout` through the frozen-lockfile install pipeline so packages installed under `enableGlobalVirtualStore: true` (pacquet's default since #444) land at the upstream-shaped `<store_dir>/links/<scope>/<name>/<version>/<hash>/node_modules/<pkg>` path instead of the legacy per-project flat layout. Closes the rest of #432 (Section B + C activation + D entry tests). Pipeline changes (frozen-lockfile path): - `InstallFrozenLockfile::run` constructs the install-scoped `VirtualStoreLayout` from the lockfile + engine_name + config, then threads `&VirtualStoreLayout` through `CreateVirtualStore`, `CreateVirtualDirBySnapshot`, `create_symlink_layout`, `SymlinkDirectDependencies`, `InstallPackageBySnapshot`, `BuildModules`, and `LinkVirtualStoreBins`. Every site that used to compute `virtual_store_dir.join(key.to_virtual_store_name())` now goes through one `layout.slot_dir(key)` lookup. - `Install::run` calls `pacquet_store_dir::register_project` when `enable_global_virtual_store` is on, writing `<store_dir>/projects/<short-hash>` so a future `pacquet store prune` can find the projects that still reference `<store_dir>/links/...` slots. Best-effort: registry write failures degrade to `tracing::warn!` and the install continues. - `Config::current` now applies `apply_global_virtual_store_derivation` after yaml — pacquet's variant of upstream's [`extendInstallOptions.ts:343-355`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/extendInstallOptions.ts#L343-L355). Field-split deviation from upstream: Upstream pnpm *mutates* `virtualStoreDir` in place under GVS, so every consumer that reads `virtualStoreDir` sees `<storeDir>/links`. Pacquet keeps `virtual_store_dir` at its project-local value and writes the GVS path into the separate `global_virtual_store_dir` field. The frozen-lockfile path consumes `global_virtual_store_dir` through `VirtualStoreLayout`; the legacy non-frozen `InstallWithoutLockfile` keeps reading `virtual_store_dir` (now via `VirtualStoreLayout::legacy`). The split is a pacquet-only concern: upstream has no without-lockfile install path and so doesn't need the second field. See the doc on `Config::apply_global_virtual_store_derivation` for the reasoning. Tests: - `install::tests::frozen_lockfile_under_gvs_registers_project_and_runs_clean` pins the registry write under default GVS-on. - `install::tests::frozen_lockfile_with_gvs_off_skips_project_registry` pins that GVS-off opts out of the registry write. - All 653 workspace tests pass under the new layout-threaded path. - Existing per-snapshot legacy tests construct `VirtualStoreLayout::legacy(...)` explicitly so they keep asserting the flat-name layout regardless of any global default. End-to-end port of upstream's [`installing/deps-installer/test/install/globalVirtualStore.ts`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/globalVirtualStore.ts) is tracked as a follow-up — those tests need a small frozen- lockfile fixture against the registry-mock, which is out of scope here. Based on #441 (`feat/438`); the `import_indexed_dir` rename from that PR is already merged in. The two PRs only co-touch `create_virtual_dir_by_snapshot.rs` — both pieces land cleanly on top of each other.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Threads `VirtualStoreLayout` through the frozen-lockfile install pipeline so packages installed under `enableGlobalVirtualStore: true` (pacquet's default since #444) land at the upstream-shaped `<store_dir>/links/<scope>/<name>/<version>/<hash>/node_modules/<pkg>` path instead of the legacy per-project flat layout. Closes the rest of #432 (Section B + C activation + D entry tests). Pipeline changes (frozen-lockfile path): - `InstallFrozenLockfile::run` constructs the install-scoped `VirtualStoreLayout` from the lockfile + engine_name + config, then threads `&VirtualStoreLayout` through `CreateVirtualStore`, `CreateVirtualDirBySnapshot`, `create_symlink_layout`, `SymlinkDirectDependencies`, `InstallPackageBySnapshot`, `BuildModules`, and `LinkVirtualStoreBins`. Every site that used to compute `virtual_store_dir.join(key.to_virtual_store_name())` now goes through one `layout.slot_dir(key)` lookup. - `Install::run` calls `pacquet_store_dir::register_project` when `enable_global_virtual_store` is on, writing `<store_dir>/projects/<short-hash>` so a future `pacquet store prune` can find the projects that still reference `<store_dir>/links/...` slots. Best-effort: registry write failures degrade to `tracing::warn!` and the install continues. - `Config::current` now applies `apply_global_virtual_store_derivation` after yaml — pacquet's variant of upstream's [`extendInstallOptions.ts:343-355`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/extendInstallOptions.ts#L343-L355). Field-split deviation from upstream: Upstream pnpm *mutates* `virtualStoreDir` in place under GVS, so every consumer that reads `virtualStoreDir` sees `<storeDir>/links`. Pacquet keeps `virtual_store_dir` at its project-local value and writes the GVS path into the separate `global_virtual_store_dir` field. The frozen-lockfile path consumes `global_virtual_store_dir` through `VirtualStoreLayout`; the legacy non-frozen `InstallWithoutLockfile` keeps reading `virtual_store_dir` (now via `VirtualStoreLayout::legacy`). The split is a pacquet-only concern: upstream has no without-lockfile install path and so doesn't need the second field. See the doc on `Config::apply_global_virtual_store_derivation` for the reasoning. Tests: - `install::tests::frozen_lockfile_under_gvs_registers_project_and_runs_clean` pins the registry write under default GVS-on. - `install::tests::frozen_lockfile_with_gvs_off_skips_project_registry` pins that GVS-off opts out of the registry write. - All 653 workspace tests pass under the new layout-threaded path. - Existing per-snapshot legacy tests construct `VirtualStoreLayout::legacy(...)` explicitly so they keep asserting the flat-name layout regardless of any global default. End-to-end port of upstream's [`installing/deps-installer/test/install/globalVirtualStore.ts`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/globalVirtualStore.ts) is tracked as a follow-up — those tests need a small frozen- lockfile fixture against the registry-mock, which is out of scope here. Based on #441 (`feat/438`); the `import_indexed_dir` rename from that PR is already merged in. The two PRs only co-touch `create_virtual_dir_by_snapshot.rs` — both pieces land cleanly on top of each other.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
## Summary Threads `VirtualStoreLayout` (from #444) through the frozen-lockfile install pipeline so packages installed with `enableGlobalVirtualStore: true` (pacquet's default since #444) land at the upstream-shaped `<store_dir>/links/<scope>/<name>/<version>/<hash>/node_modules/<pkg>` path. Closes the rest of #432 (Section B + C activation + D entry tests). ## Pipeline changes (frozen-lockfile path) - `InstallFrozenLockfile::run` constructs the install-scoped `VirtualStoreLayout` from the lockfile + engine name + config, then threads `&VirtualStoreLayout` through `CreateVirtualStore`, `CreateVirtualDirBySnapshot`, `create_symlink_layout`, `SymlinkDirectDependencies`, `InstallPackageBySnapshot`, `BuildModules`, and `LinkVirtualStoreBins`. Every site that used to compute `virtual_store_dir.join(key.to_virtual_store_name())` now goes through one `layout.slot_dir(key)` lookup. - `Install::run` calls `pacquet_store_dir::register_project` **once per importer** when `enable_global_virtual_store` is on, writing `<store_dir>/projects/<short-hash>` per workspace package so a future `pacquet store prune` can find every reachable consumer of `<store_dir>/links/...`. Best-effort: registry write failures degrade to `tracing::warn!` and the install continues. The walk runs *after* `InstallFrozenLockfile::run` succeeds so importer keys have already been validated. - `Config::current` now applies `apply_global_virtual_store_derivation` after yaml — pacquet's variant of upstream's [`extendInstallOptions.ts:343-355`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/extendInstallOptions.ts#L343-L355). An explicit `globalVirtualStoreDir:` in yaml wins over the derivation; otherwise the field falls back to the user's pinned `virtualStoreDir` (under GVS-on) or to `<store_dir>/links`. ## Field-split deviation from upstream Upstream pnpm *mutates* `virtualStoreDir` in place under GVS, so every consumer that reads `virtualStoreDir` sees `<storeDir>/links`. Pacquet keeps `virtual_store_dir` at its project-local value and writes the GVS path into the separate `global_virtual_store_dir` field. The frozen-lockfile path consumes `global_virtual_store_dir` through `VirtualStoreLayout`; the legacy non-frozen `InstallWithoutLockfile` keeps reading `virtual_store_dir` (now via `VirtualStoreLayout::legacy`). The split is a pacquet-only concern: upstream has no without-lockfile install path and so doesn't need the second field. See the doc on `Config::apply_global_virtual_store_derivation` for the reasoning. ## Benchmarks The integrated-benchmark fixture pins `enableGlobalVirtualStore: false` so existing scenarios continue to compare apples-to-apples against the project-local `<modules>/.pnpm/<flat-name>` shape. Without the pin, every revision newer than this PR would silently switch to the GVS layout, while `pacquet@main` and the pnpm side of the comparison would still use the isolated layout — different shape, unfair comparison. GVS-specific benchmark runs are available by passing `--fixture-dir <dir>` at a copy of the fixture with the flag flipped. ## Tests - `install::tests::frozen_lockfile_under_gvs_registers_project_and_runs_clean` pins the registry write under default GVS-on (single-project case). - `install::tests::frozen_lockfile_under_gvs_registers_each_workspace_importer` pins per-importer registration: workspace root + `packages/web` each end up with their own symlink in `<store_dir>/projects/`. - `install::tests::frozen_lockfile_with_gvs_off_skips_project_registry` pins that GVS-off opts out of the registry write. - `config::tests::yaml_global_virtual_store_dir_wins_over_derivation` pins the yaml-override precedence. - All **829 workspace tests** pass under the new layout-threaded path. - Existing per-snapshot legacy tests construct `VirtualStoreLayout::legacy(...)` explicitly so they keep asserting the flat-name layout regardless of any global default. Partial-install tests from #442 pin `enable_global_virtual_store = false` so their seeded legacy-shape slots match the probe path. End-to-end port of upstream's [`installing/deps-installer/test/install/globalVirtualStore.ts`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/globalVirtualStore.ts) is tracked as a follow-up — those tests need a small frozen-lockfile fixture against the registry-mock that doesn't exist yet.
This was referenced May 13, 2026
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.
6 tasks
5 tasks
zkochan
added a commit
that referenced
this pull request
May 13, 2026
CodeRabbit caught a critical Windows bug: pacquet's registry entries
are NTFS junctions (created via `junction::create` in
`pacquet_fs::symlink_dir`), and `std::fs::read_link` only handles
`IO_REPARSE_TAG_SYMLINK` reparse points — it returns `EINVAL`
("not a symlink") for `IO_REPARSE_TAG_MOUNT_POINT` junctions. The
EINVAL silent-skip would then drop every live registry entry on
Windows, breaking `pacquet store prune` entirely (see
[rust-lang/rust#28528](rust-lang/rust#28528),
open since 2015).
Added `pacquet_fs::read_symlink_dir` — falls back to
`junction::get_target` on Windows EINVAL. Routed two `fs::read_link`
sites through it:
- `get_registered_projects` (the new code from #475)
- `register_project::InspectExisting` (pre-existing bug — heal
branch was also broken on Windows since the original #432 / #444
landed)
Same one-helper-for-both pattern as `remove_symlink_dir` in the
previous review fix. Tests still pass; the Windows behaviour
isn't reachable from macOS / Linux CI but the platform split is
contained in one well-commented helper.
Also: `cargo fmt --all` reflow of one `EntryInaccessible` struct
literal that `just ready`'s `cargo fmt` step ran *before* the
Copilot-fix edits — CI's `cargo fmt --all -- --check` caught it.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
…) (#475) Closes #458. Ports upstream's [`pruneGlobalVirtualStore`](https://github.com/pnpm/pnpm/blob/94240bc046/store/controller/src/storeController/pruneGlobalVirtualStore.ts) and the read half of [`projectRegistry`](https://github.com/pnpm/pnpm/blob/94240bc046/store/controller/src/storeController/projectRegistry.ts#L37-L100) (pacquet shipped only the write half in #432 / #444). The CLI wiring already existed — `StoreCommand::Prune` → `StoreDir::prune()` — but `StoreDir::prune` was a `todo!()`. With this PR `pacquet store prune` actually runs. ## What it does **Mark phase**: walk every registered project (`<store>/projects/<short-hash>` → project root). For each project, find every `node_modules/`, follow symlinks that land under `<store>/links/...`, record reachable `<scope>/<name>/<version>/<hash>` slots, and recurse into the slot's own `node_modules/` for transitive deps. **Sweep phase**: walk `<store>/links/<scope>/<name>/<version>/<hash>` and remove every `<hash>` not in the reachable set. Empty `<version>/`, `<name>/`, and `<scope>/` parents get cleaned up. **Self-healing registry**: `get_registered_projects` unlinks entries whose project directory has been deleted (`ENOENT` on stat). Permission / unrelated errors surface as `PROJECT_INACCESSIBLE` / `PROJECT_REGISTRY_ENTRY_INACCESSIBLE` — matches upstream's strict stance, since silently dropping an inaccessible registry entry could remove slots the project still references.
3 tasks
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Copilot caught a regression introduced by flipping the `enableGlobalVirtualStore` default to `false`. The `node --version` detection in `InstallFrozenLockfile::run` was hoisted *before* `CreateVirtualStore::run` in #444 because the GVS-aware layout needed `engine_name` upfront. With GVS now off by default, that hoist costs ~tens of ms of synchronous spawn on the install critical path — the value just gets passed into a `VirtualStoreLayout::new` that immediately returns without using it. `engine_name` is still used by `BuildModules` for the side-effects- cache key prefix (cache defaults to on), so dropping the spawn entirely isn't right. Instead, split the detection by GVS state: - **GVS on**: spawn synchronously, same as today — layout needs it. - **GVS off, no host yet**: spawn into `tokio::task::spawn_blocking` and keep the `JoinHandle`. The blocking pool runs the spawn concurrently with `CreateVirtualStore::run`'s I/O. Await right before `BuildModules`, so the `node` startup cost is hidden under the install rather than serialised before it. `VirtualStoreLayout::new` is built with `None` on the deferred path, which is fine because GVS is off and the layout ignores the field in that path. Pre-GVS pacquet had this same overlap; this restores it for the default config.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Pacquet's `default_enable_global_virtual_store()` returned `true` and cited upstream's [`config/reader/src/index.ts:392-394`](https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/index.ts#L392-L394) as the authority for that default. But that range lives entirely inside the [`if (cliOptions['global']) { ... }`](https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/index.ts#L348-L395) block — surrounded by `CONFIG_CONFLICT_*_WITH_GLOBAL` errors and closed by `} else if (!pnpmConfig.bin) { ... }`. So in pnpm v11: - `pnpm install --global foo`: `enableGlobalVirtualStore` defaults to `true`. - `pnpm install` (regular): stays `null`/unset → effectively `false`. Pacquet doesn't have a `--global` CLI flag at all (only `install --frozen-lockfile`), so the applicable upstream default is the `false` one. Pre-fix, every pacquet install silently wrote slots to `<store>/links/...` and registered projects, even when the user never asked for GVS — and a project alternating between `pnpm install` and `pacquet install --frozen-lockfile` would see two different layouts. The default introduced by #444 cited the same `L392-L394` range but read it as an unconditional default. Corrected here. Test changes: - `gvs_default_writes_links_into_global_virtual_store_dir` renamed and inverted — now asserts the default-off behaviour. The path derivation still populates both fields cleanly so downstream code can read either without first checking the toggle. - `gvs_user_pinned_virtual_store_routes_into_global_virtual_store_dir` and `yaml_global_virtual_store_dir_wins_over_derivation`: yaml now opts into GVS explicitly (`enableGlobalVirtualStore: true`) so the GVS-on derivation path is still exercised. - `install/tests.rs` comments updated to reflect the new default. Benchmark fixture's explicit `enableGlobalVirtualStore: false` pin is kept (it's now redundant but future-proofs the bench against a default flip), with an updated comment explaining the design intent. Same for `MinimalWorkspaceManifest.enable_global_virtual_store` doc.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Copilot caught a regression introduced by flipping the `enableGlobalVirtualStore` default to `false`. The `node --version` detection in `InstallFrozenLockfile::run` was hoisted *before* `CreateVirtualStore::run` in #444 because the GVS-aware layout needed `engine_name` upfront. With GVS now off by default, that hoist costs ~tens of ms of synchronous spawn on the install critical path — the value just gets passed into a `VirtualStoreLayout::new` that immediately returns without using it. `engine_name` is still used by `BuildModules` for the side-effects- cache key prefix (cache defaults to on), so dropping the spawn entirely isn't right. Instead, split the detection by GVS state: - **GVS on**: spawn synchronously, same as today — layout needs it. - **GVS off, no host yet**: spawn into `tokio::task::spawn_blocking` and keep the `JoinHandle`. The blocking pool runs the spawn concurrently with `CreateVirtualStore::run`'s I/O. Await right before `BuildModules`, so the `node` startup cost is hidden under the install rather than serialised before it. `VirtualStoreLayout::new` is built with `None` on the deferred path, which is fine because GVS is off and the layout ignores the field in that path. Pre-GVS pacquet had this same overlap; this restores it for the default config.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Pacquet's `default_enable_global_virtual_store()` returned `true` and cited upstream's [`config/reader/src/index.ts:392-394`](https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/index.ts#L392-L394) as the authority. But that range lives entirely inside the [`if (cliOptions['global']) { ... }`](https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/index.ts#L348-L395) block — surrounded by `CONFIG_CONFLICT_*_WITH_GLOBAL` errors and closed by `} else if (!pnpmConfig.bin) { ... }`. So in pnpm v11: - `pnpm install --global foo` → `enableGlobalVirtualStore` defaults to `true` - `pnpm install` (regular) → stays `null`/unset → effectively `false` Pacquet doesn't have a `--global` CLI flag (only `install --frozen-lockfile`), so the applicable upstream default is the `false` one. Pre-fix, every pacquet install silently wrote slots to `<store>/links/...` and registered projects, even when the user never asked for GVS — and a project alternating between `pnpm install` and `pacquet install --frozen-lockfile` would see two different on-disk layouts. The default introduced by #444 cited the same `L392-L394` range but read it as an unconditional default. Corrected here. ## Test changes - `gvs_default_writes_links_into_global_virtual_store_dir` renamed to `gvs_default_is_off_and_paths_derive_cleanly` and inverted — now asserts the default-off behaviour. The path derivation still populates both `virtual_store_dir` and `global_virtual_store_dir` cleanly so downstream code can read either field without first checking the toggle. - `gvs_user_pinned_virtual_store_routes_into_global_virtual_store_dir` and `yaml_global_virtual_store_dir_wins_over_derivation`: yaml now opts into GVS explicitly (`enableGlobalVirtualStore: true`) so the GVS-on derivation path is still exercised. - `install/tests.rs` doc + inline comments updated to reflect that GVS-on tests need to pin the flag explicitly now. ## Benchmark The bench fixture's explicit `enableGlobalVirtualStore: false` pin is kept (it's now redundant but future-proofs the bench against a default flip), with an updated comment explaining the design intent. Same for `MinimalWorkspaceManifest.enable_global_virtual_store` doc.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Pacquet port of upstream pnpm's global-virtual-store (GVS) machinery — lands the config fields, the store-dir helpers, the graph-hasher path math, and a
VirtualStoreLayouthelper, but not the install-pipeline wiring. Carves the work in #432 along its Section A/B/C/D boundary so the foundation can land cleanly before the larger install refactor follows.What's in this PR
Section A — Config
Config::enable_global_virtual_store(defaulttrue, matching upstream'sconfig/reader/src/index.ts:392-394)Config::global_virtual_store_dirpnpm-workspace.yamldeserialization forenableGlobalVirtualStore/globalVirtualStoreDirConfig::apply_global_virtual_store_derivationmirroring upstream'sextendInstallOptions.ts:343-355. Not called fromConfig::currentyet — the install layer in the follow-up will invoke it explicitly so this PR doesn't silently redirect installs to<store_dir>/linksbefore the rest of the layout math is wired.Section C (foundation only) — Project registry helpers
StoreDir::links(),StoreDir::projects(),StoreDir::root()pacquet_store_dir::register_project(port of upstream'sregisterProject)pacquet_store_dir::create_short_hash(port ofcreateShortHash) — pinned test vector againstprintf pacquet | shasum -a 256 | head -c 32Section B (foundation only) — Graph-hasher + layout helpers
pacquet_graph_hasher::calc_graph_node_hash(port ofcalcGraphNodeHash)pacquet_graph_hasher::format_global_virtual_store_path(port offormatGlobalVirtualStorePath)pacquet_package_manager::VirtualStoreLayoutprecomputes each snapshot's slot directory on disk and hides the GVS-vs-legacy path divergence behind oneslot_dir(&PackageKey) -> PathBuflookupWhat's deferred to the follow-up PR
VirtualStoreLayoutthroughCreateVirtualStore,CreateVirtualDirBySnapshot,SymlinkDirectDependencies,InstallPackageBySnapshot,BuildModules,link_bins, andcreate_symlink_layoutregister_projectfromInstall::runapply_global_virtual_store_derivationfromConfig::currentinstalling/deps-installer/test/install/globalVirtualStore.tsNo behavior change at install time
Every new field, helper, and module is unreachable from the existing install path.
just readyis green; new unit tests added for each helper:pacquet-config: GVS derivation truth table (default re-points / disabled / user-pinned)pacquet-store-dir: short-hash pinned vector + register idempotency + isSubdir guardpacquet-graph-hasher: GVS hash determinism + engine sensitivity + child sensitivity + unscoped@/prefixpacquet-package-manager:VirtualStoreLayoutslot_dir under GVS-on and GVS-offCloses part of #432; #432 stays open until the install-pipeline wiring + test ports land.
Test plan
just ready(full suite green)taplo format --check(clean)just dylint(clean)Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Tests