This repository was archived by the owner on May 14, 2026. It is now read-only.
Conversation
Port pnpm's `transitivelyRequiresBuild` + `computeBuiltDepPaths` into `graph-hasher`, and extend `calc_graph_node_hash` with the `builtDepPaths`-driven engine-gating that mirrors upstream's [`calcGraphNodeHash`](https://github.com/pnpm/pnpm/blob/94240bc046/deps/graph-hasher/src/index.ts#L140-L142). When a snapshot — and its entire transitive subtree — has no entry in `allowBuilds` (and `dangerouslyAllowAllBuilds` is off), its GVS hash payload now sets `engine: null` instead of `engine: ENGINE_NAME`. Pure-JS packages share one slot directory across Node.js versions, OSes, and CPUs, matching upstream and unlocking the cross-host store sharing the GVS layout was designed for. `VirtualStoreLayout::new` grows an `Option<&AllowBuildPolicy>` parameter. `InstallFrozenLockfile::run` passes the install's `AllowBuildPolicy` through; callers that pass `None` retain the pre-#459 always-include-engine shape (matches upstream's `builtDepPaths === undefined` short-circuit). Tests: - six new `transitively_requires_build` unit tests covering self-hit, transitive ancestor, unrelated tree, missing node, cycle termination, and cycle-doesn't-mask-sibling-builder - four new `calc_graph_node_hash` tests covering the engine-agnostic vs engine-specific paths and the `built_dep_paths = None` shortcut - two end-to-end `VirtualStoreLayout::new` tests proving a pure-JS snapshot's slot directory is identical across two `engine` strings, and that a built snapshot's slot differs --- Written by an agent (Claude Code, claude-opus-4-7).
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 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). (7)
📝 WalkthroughWalkthroughAdds transitively_requires_build; uses it in calc_graph_node_hash to optionally null out engine when snapshots (and their transitive deps) don't require builds; threads allow_build_policy through VirtualStoreLayout and InstallFrozenLockfile and updates tests. ChangesEngine-agnostic GVS hash gating via built_dep_paths
Sequence DiagramsequenceDiagram
participant VSL as VirtualStoreLayout::new
participant Policy as AllowBuildPolicy
participant Hasher as calc_graph_node_hash
participant BuilderCheck as transitively_requires_build
VSL->>Policy: check snapshots (name, version)
Policy-->>VSL: per-snapshot build_allowed flags
VSL->>VSL: build built_dep_paths HashSet
VSL->>Hasher: pass built_dep_paths & build_required_cache
Hasher->>BuilderCheck: transitively_requires_build checks dep graph
BuilderCheck-->>Hasher: boolean: requires build?
Hasher-->>VSL: compute slot hash (engine or null)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Comment |
Micro-Benchmark ResultsLinux |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #469 +/- ##
==========================================
+ Coverage 88.64% 89.17% +0.53%
==========================================
Files 116 116
Lines 9952 10432 +480
==========================================
+ Hits 8822 9303 +481
+ Misses 1130 1129 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR ports pnpm’s builtDepPaths-driven “engine in hash payload” gating into pacquet’s global virtual store (GVS) slot hashing, so snapshots that don’t (transitively) require builds hash with engine = null and can share the same slot directory across differing engine strings.
Changes:
- Add
transitively_requires_buildand threadbuilt_dep_paths+ an install-scoped memoization cache intocalc_graph_node_hash. - Extend
VirtualStoreLayout::newto derivebuilt_dep_pathsfromAllowBuildPolicyand pass it into every node-hash computation. - Wire
AllowBuildPolicyfromInstallFrozenLockfile::runinto the GVS layout construction.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/package-manager/src/virtual_store_layout.rs | Derives built_dep_paths from AllowBuildPolicy and threads it into per-snapshot GVS hash computation; adds end-to-end gating tests. |
| crates/package-manager/src/install_frozen_lockfile.rs | Passes the install’s AllowBuildPolicy into VirtualStoreLayout::new to enable gating during frozen installs. |
| crates/graph-hasher/src/global_virtual_store_path.rs | Extends calc_graph_node_hash API to support engine gating based on built_dep_paths, and adds targeted unit tests. |
| crates/graph-hasher/src/dep_state.rs | Adds transitively_requires_build (ported from upstream) plus unit tests for cycle handling and caching behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…docs - Drop the cross-module intra-doc link `[transitively_requires_build]: crate::dep_state::transitively_requires_build` that resolved to a pub(crate) item. The link rendered correctly under pacquet's CI (which runs `cargo doc --document-private-items`), but downstream doc builds without that flag would trip `private_intra_doc_links`. Switched to a plain-backticks reference + an inline "private to this crate" note so the doc remains informative either way. - Reword the `build_required_cache` hint to avoid the ambiguous `&mut HashMap::new()` phrasing — temporaries-in-call-args is a valid pattern but a confusing one to surface at the API doc. Pointed callers at the explicit `let mut cache = HashMap::new();` + `&mut cache` shape instead.
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.67077937988,
"stddev": 0.15531575858451632,
"median": 2.62893989068,
"user": 2.5605913800000004,
"system": 3.6474907599999993,
"min": 2.55187367618,
"max": 3.06128971818,
"times": [
2.70228094318,
2.5672896921799997,
2.6913888291799997,
3.06128971818,
2.7526557211799996,
2.5623664371799997,
2.58266738818,
2.67521239318,
2.56076900018,
2.55187367618
]
},
{
"command": "pacquet@main",
"mean": 2.57679513068,
"stddev": 0.08396979782990949,
"median": 2.5608576366799998,
"user": 2.5504745800000004,
"system": 3.598221659999999,
"min": 2.45586457318,
"max": 2.75463357118,
"times": [
2.75463357118,
2.5859677831799996,
2.59863927918,
2.4908421121799997,
2.66126161918,
2.55003376718,
2.5688169141799997,
2.45586457318,
2.55289835918,
2.54899332818
]
},
{
"command": "pnpm",
"mean": 6.11298625168,
"stddev": 0.11154124427354146,
"median": 6.104276802679999,
"user": 8.98668828,
"system": 4.44794566,
"min": 5.9524308791800005,
"max": 6.34086370118,
"times": [
5.973705396180001,
5.9524308791800005,
6.10880134518,
6.084184635180001,
6.06080507318,
6.20074212818,
6.34086370118,
6.15207356218,
6.15650353618,
6.09975226018
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.7301575544000001,
"stddev": 0.026230773108162665,
"median": 0.7243671747,
"user": 0.37047996,
"system": 1.5814983399999998,
"min": 0.6935072797,
"max": 0.7764429567000001,
"times": [
0.7764429567000001,
0.7031568447000001,
0.7577220787000001,
0.7463103067,
0.6935072797,
0.7225611077,
0.7161582717,
0.7261732417000001,
0.7483093777000001,
0.7112340787
]
},
{
"command": "pacquet@main",
"mean": 0.8224829989000002,
"stddev": 0.0680289468897996,
"median": 0.8045878572,
"user": 0.39178456,
"system": 1.60442494,
"min": 0.7345946787000001,
"max": 0.9661496737,
"times": [
0.8772675727,
0.8061633217,
0.7919156187,
0.7605835507,
0.8793753327,
0.8030123927,
0.7345946787000001,
0.7820357727,
0.9661496737,
0.8237320747
]
},
{
"command": "pnpm",
"mean": 2.6104956914999997,
"stddev": 0.08500306212912238,
"median": 2.5842466507,
"user": 3.1227089599999998,
"system": 2.2300388399999997,
"min": 2.5074749647,
"max": 2.7334173637,
"times": [
2.6776218607,
2.7314060387,
2.5894623477,
2.6646528047,
2.5074749647,
2.5649535147,
2.5165307907,
2.5790309537000002,
2.7334173637,
2.5404062757
]
}
]
} |
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
Closes #459. Ports pnpm's
transitivelyRequiresBuild+computeBuiltDepPathsintopacquet-graph-hasherand wires thebuiltDepPaths-driven engine gating from upstream'scalcGraphNodeHashinto pacquet's GVS slot-path computation.When a snapshot — and its entire transitive subtree — has no entry in
allowBuilds(anddangerouslyAllowAllBuildsis off), its GVS hash payload now setsengine: nullinstead ofengine: ENGINE_NAME. Pure-JS packages share one slot directory across Node.js versions, OSes, and CPUs, which is what makes the shared global virtual store cross-host portable.What changed
graph-hasher::dep_state— newpub(crate) fn transitively_requires_build, mirroring upstream'stransitivelyRequiresBuildverbatim. Notable subtleties preserved: cycle guard usesdepPath(notfullPkgId), and a cycle hit returnsfalsewithout caching to avoid poisoning sibling walks.graph-hasher::calc_graph_node_hash— grows two parameters:built_dep_paths: Option<&HashSet<K>>andbuild_required_cache: &mut HashMap<K, bool>.Nonereproduces the always-include-engine shape (matches upstream'sbuiltDepPaths === undefinedshort-circuit), so callers can opt into the gating only when they have anAllowBuildPolicyavailable.VirtualStoreLayout::new— grows anOption<&AllowBuildPolicy>parameter. Internally walkssnapshotsonce to derivebuilt_dep_paths, then threads that set through everycalc_graph_node_hashcall.InstallFrozenLockfile::run— passes the install's existingAllowBuildPolicytoVirtualStoreLayout::new. The policy was already built upstream of this call site so this is a one-line wiring change.Why pacquet still needed this after #449
#449 introduced the GVS path computation but left every snapshot hashing with
engine = ENGINE_NAME. Adarwin-arm64-node20install and alinux-x64-node22install of the same pure-JS package would land in different GVS slots, defeating the whole point of a shared global store. #459 closes that gap: pure-JS subtrees hash engine-agnostically, native subtrees keep partitioning by engine.Test plan
transitively_requires_buildunit tests (self-hit, transitive ancestor, unrelated tree, missing node, cycle termination, cycle-doesn't-mask-sibling-builder)calc_graph_node_hashtests (engine-agnostic when gated, engine-specific for self-builder, engine-specific for ancestor-of-builder,Nonedisables gating)VirtualStoreLayout::newend-to-end tests (slot identical across engines for pure-JS with empty policy; slot differs across engines when the snapshot is inallowBuilds)just ready(926 tests pass)taplo format --checkjust dylintWritten by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit