mirror of
https://github.com/pnpm/pnpm.git
synced 2026-06-02 13:13:17 -04:00
fix(package-manager): port pnpm's lockfile-pruner BFS to re-derive transitive optional (#11919)
The resolver's per-node AND-fold updates only the directly-revisited package, so descendants walked first via an `optionalDependencies` edge stay stuck at `optional: true` even when a later non-optional path reaches them transitively. Upstream hides this from users by re-deriving the flag in `copyDependencySubGraph`; pacquet had no equivalent pass, so a fetch or build failure on a transitively-required package was silently tolerated as if it were optional. Port the BFS into `dependencies_graph_to_lockfile`: walk from the importer's direct deps (classified by manifest dep-group), recurse through each node's children with parent-inherited optional for regular edges and forced `optional: true` for `optionalDependencies` edges, then override `SnapshotEntry.optional` to `false` for any package reached by an all-non-optional path. Refs https://github.com/pnpm/pnpm/issues/11916.
This commit is contained in:
Zoltan Kochan
@@ -73,7 +73,9 @@ pub fn dependencies_graph_to_lockfile(opts: GraphToLockfileOptions<'_>) -> Lockf
|
||||
|
||||
let importer = build_root_importer(manifest, resolved);
|
||||
|
||||
let (packages, snapshots) = build_packages_and_snapshots(&resolved.peers_result.graph);
|
||||
let optional_overrides = compute_corrected_optional(manifest, resolved);
|
||||
let (packages, snapshots) =
|
||||
build_packages_and_snapshots(&resolved.peers_result.graph, &optional_overrides);
|
||||
|
||||
let mut importers: HashMap<String, ProjectSnapshot> = HashMap::with_capacity(1);
|
||||
importers.insert(Lockfile::ROOT_IMPORTER_KEY.to_string(), importer);
|
||||
@@ -243,8 +245,13 @@ fn real_name(result: &ResolveResult) -> Option<String> {
|
||||
///
|
||||
/// Multiple snapshot entries (peer variants) share one packages entry,
|
||||
/// so the loop dedupes by peer-stripped key.
|
||||
///
|
||||
/// `optional_overrides` carries the corrected `optional` flag per
|
||||
/// depPath produced by [`compute_corrected_optional`]; a missing
|
||||
/// entry falls back to [`DependenciesGraphNode::optional`].
|
||||
fn build_packages_and_snapshots(
|
||||
graph: &DependenciesGraph,
|
||||
optional_overrides: &HashMap<DepPath, bool>,
|
||||
) -> (HashMap<PackageKey, PackageMetadata>, HashMap<PackageKey, SnapshotEntry>) {
|
||||
let mut packages: HashMap<PackageKey, PackageMetadata> = HashMap::new();
|
||||
let mut snapshots: HashMap<PackageKey, SnapshotEntry> = HashMap::new();
|
||||
@@ -253,7 +260,7 @@ fn build_packages_and_snapshots(
|
||||
let Ok(snapshot_key) = node.dep_path.as_str().parse::<PackageKey>() else { continue };
|
||||
let metadata_key = snapshot_key.without_peer();
|
||||
|
||||
let snapshot = build_snapshot_entry(node, graph);
|
||||
let snapshot = build_snapshot_entry(node, graph, optional_overrides);
|
||||
snapshots.insert(snapshot_key, snapshot);
|
||||
|
||||
packages.entry(metadata_key).or_insert_with(|| build_package_metadata(node));
|
||||
@@ -372,14 +379,19 @@ fn build_peer_dep_blocks(node: &DependenciesGraphNode) -> PeerDepBlocks {
|
||||
/// [`toLockfileDependency`](https://github.com/pnpm/pnpm/blob/097983fbca/installing/deps-resolver/src/updateLockfile.ts#L49-L156):
|
||||
/// the `dependencies` / `optionalDependencies` partition follows the
|
||||
/// node's own `optionalDependencies` set and peer-optional flag;
|
||||
/// `transitivePeerDependencies` is sorted; `optional` is copied from
|
||||
/// the resolver's [`DependenciesGraphNode::optional`] (AND-folded
|
||||
/// across every visit so a snapshot is marked `optional: true` only
|
||||
/// when every path from any importer to it goes through an
|
||||
/// `optionalDependencies` edge). `BuildModules` consults this flag to
|
||||
/// decide whether a build failure is fatal or should be reported via
|
||||
/// `transitivePeerDependencies` is sorted; `optional` is sourced from
|
||||
/// [`compute_corrected_optional`] (the lockfile-pruner BFS port,
|
||||
/// which re-derives the flag from the importer graph because the
|
||||
/// resolver's per-node fold misses transitive descendants on
|
||||
/// revisits — see <https://github.com/pnpm/pnpm/issues/11916>).
|
||||
/// `BuildModules` consults this flag to decide whether a build
|
||||
/// failure is fatal or should be reported via
|
||||
/// `pnpm:skipped-optional-dependency`.
|
||||
fn build_snapshot_entry(node: &DependenciesGraphNode, graph: &DependenciesGraph) -> SnapshotEntry {
|
||||
fn build_snapshot_entry(
|
||||
node: &DependenciesGraphNode,
|
||||
graph: &DependenciesGraph,
|
||||
optional_overrides: &HashMap<DepPath, bool>,
|
||||
) -> SnapshotEntry {
|
||||
let optional_children = optional_children_of(node);
|
||||
|
||||
let mut dependencies: HashMap<PkgName, SnapshotDepRef> = HashMap::new();
|
||||
@@ -400,13 +412,106 @@ fn build_snapshot_entry(node: &DependenciesGraphNode, graph: &DependenciesGraph)
|
||||
list
|
||||
};
|
||||
|
||||
let optional = optional_overrides.get(&node.dep_path).copied().unwrap_or(node.optional);
|
||||
|
||||
SnapshotEntry {
|
||||
id: None,
|
||||
dependencies: (!dependencies.is_empty()).then_some(dependencies),
|
||||
optional_dependencies: (!optional_dependencies.is_empty()).then_some(optional_dependencies),
|
||||
transitive_peer_dependencies: (!transitive.is_empty()).then_some(transitive),
|
||||
patched: None,
|
||||
optional: node.optional,
|
||||
optional,
|
||||
}
|
||||
}
|
||||
|
||||
/// Re-derive each snapshot's `optional` flag by walking the graph
|
||||
/// from the importer's direct deps, classifying each starting edge by
|
||||
/// the dep-group it lives in on the manifest. A package ends up
|
||||
/// `optional: false` iff every visit didn't land on it through an
|
||||
/// `optionalDependencies` edge — i.e. there exists at least one path
|
||||
/// from any importer to it whose edges are all non-optional.
|
||||
///
|
||||
/// Ports upstream pnpm's
|
||||
/// [`copyDependencySubGraph`](https://github.com/pnpm/pnpm/blob/b9de85dcb6/lockfile/pruner/src/index.ts#L160-L205)
|
||||
/// BFS, which exists for exactly this reason: the resolver's
|
||||
/// per-node AND-fold at
|
||||
/// [`resolveDependencies.ts:1630`](https://github.com/pnpm/pnpm/blob/b9de85dcb6/installing/deps-resolver/src/resolveDependencies.ts#L1627-L1648)
|
||||
/// updates only the directly-revisited package, so the
|
||||
/// already-walked descendants stay stuck at whatever `optional` they
|
||||
/// were tagged with on the first visit. See
|
||||
/// <https://github.com/pnpm/pnpm/issues/11916> for the scenario.
|
||||
///
|
||||
/// A missing entry in the returned map means the node was never
|
||||
/// reachable from any importer dep — [`build_snapshot_entry`] falls
|
||||
/// back to [`DependenciesGraphNode::optional`] for those, matching
|
||||
/// upstream's "untouched depLockfile keeps its existing flag" arm.
|
||||
fn compute_corrected_optional(
|
||||
manifest: &PackageManifest,
|
||||
resolved: &ResolveImporterResult,
|
||||
) -> HashMap<DepPath, bool> {
|
||||
let graph = &resolved.peers_result.graph;
|
||||
let direct = &resolved.peers_result.direct_dependencies_by_alias;
|
||||
let alias_to_group = manifest_alias_to_group(manifest);
|
||||
|
||||
// Partition importer deps by group, mirroring the
|
||||
// `(devDepPaths, optionalDepPaths, prodDepPaths)` split upstream
|
||||
// hands to `copyDependencySubGraph`.
|
||||
let mut dev_seeds: Vec<&DepPath> = Vec::new();
|
||||
let mut optional_seeds: Vec<&DepPath> = Vec::new();
|
||||
let mut prod_seeds: Vec<&DepPath> = Vec::new();
|
||||
for (alias, dep_path) in direct {
|
||||
let group = alias_to_group.get(alias).copied().unwrap_or(DependencyGroup::Prod);
|
||||
match group {
|
||||
DependencyGroup::Dev => dev_seeds.push(dep_path),
|
||||
DependencyGroup::Optional => optional_seeds.push(dep_path),
|
||||
DependencyGroup::Prod | DependencyGroup::Peer => prod_seeds.push(dep_path),
|
||||
}
|
||||
}
|
||||
|
||||
let mut walked: HashSet<(&DepPath, bool)> = HashSet::new();
|
||||
let mut visited: HashSet<&DepPath> = HashSet::new();
|
||||
let mut non_optional: HashSet<&DepPath> = HashSet::new();
|
||||
|
||||
walk_subgraph(graph, &mut walked, &mut visited, &mut non_optional, dev_seeds, false);
|
||||
walk_subgraph(graph, &mut walked, &mut visited, &mut non_optional, optional_seeds, true);
|
||||
walk_subgraph(graph, &mut walked, &mut visited, &mut non_optional, prod_seeds, false);
|
||||
|
||||
let mut out: HashMap<DepPath, bool> = HashMap::with_capacity(visited.len());
|
||||
for dep_path in visited {
|
||||
out.insert(dep_path.clone(), !non_optional.contains(dep_path));
|
||||
}
|
||||
out
|
||||
}
|
||||
|
||||
/// Iterative half of [`compute_corrected_optional`]. Pushes
|
||||
/// `(dep_path, optional)` pairs onto an explicit stack rather than
|
||||
/// recursing so deep graphs can't blow the call stack. Children
|
||||
/// declared by the parent's `optionalDependencies` (or by a
|
||||
/// peer-deps-meta `optional: true`) always recurse with
|
||||
/// `optional: true`; the rest inherit the parent's `optional`.
|
||||
fn walk_subgraph<'g>(
|
||||
graph: &'g DependenciesGraph,
|
||||
walked: &mut HashSet<(&'g DepPath, bool)>,
|
||||
visited: &mut HashSet<&'g DepPath>,
|
||||
non_optional: &mut HashSet<&'g DepPath>,
|
||||
seeds: Vec<&'g DepPath>,
|
||||
optional: bool,
|
||||
) {
|
||||
let mut stack: Vec<(&'g DepPath, bool)> = seeds.into_iter().map(|dp| (dp, optional)).collect();
|
||||
while let Some((dep_path, optional)) = stack.pop() {
|
||||
if !walked.insert((dep_path, optional)) {
|
||||
continue;
|
||||
}
|
||||
let Some(node) = graph.get(dep_path) else { continue };
|
||||
visited.insert(dep_path);
|
||||
if !optional {
|
||||
non_optional.insert(dep_path);
|
||||
}
|
||||
let opt_children = optional_children_of(node);
|
||||
for (alias, child_dep_path) in &node.children {
|
||||
let child_optional = optional || opt_children.contains(alias.as_str());
|
||||
stack.push((child_dep_path, child_optional));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -523,3 +523,203 @@ fn snapshot_optional_flag_round_trips_from_dependencies_graph_node() {
|
||||
"snapshot marked optional in the graph propagates to the lockfile",
|
||||
);
|
||||
}
|
||||
|
||||
/// Scenario from
|
||||
/// [pnpm/pnpm#11916](https://github.com/pnpm/pnpm/issues/11916): root
|
||||
/// declares `optionalDependencies.a` and `dependencies.b`; `a.dependencies = {c}`
|
||||
/// and `b.dependencies = {a}`. The resolver's [`DependenciesGraphNode::optional`]
|
||||
/// field is stale on `c` — the tree walker marks it `optional: true`
|
||||
/// on the `optional → a → c` descent and then misses the AND-fold on
|
||||
/// the `prod → b → a` revisit because the lazy-children path doesn't
|
||||
/// re-traverse `a`'s subtree. The lockfile-pruner BFS re-derives the
|
||||
/// flag from the importer-rooted graph, so `c` ends up `optional:
|
||||
/// false` (reachable through the all-non-optional path `prod → b →
|
||||
/// a → c`).
|
||||
#[test]
|
||||
fn transitive_optional_is_recomputed_for_packages_reachable_via_a_non_optional_path() {
|
||||
let (_tmp, manifest) = write_manifest(json!({
|
||||
"name": "fixture",
|
||||
"version": "1.0.0",
|
||||
"dependencies": { "b": "^1.0.0" },
|
||||
"optionalDependencies": { "a": "^1.0.0" },
|
||||
}));
|
||||
|
||||
// `c` was first reached via the optional path, so the resolver
|
||||
// left `node.optional = true`. The pruner should flip it back to
|
||||
// false.
|
||||
let c = make_node_with_optional(
|
||||
"c",
|
||||
"1.0.0",
|
||||
json!({ "name": "c", "version": "1.0.0" }),
|
||||
BTreeMap::new(),
|
||||
BTreeMap::new(),
|
||||
HashSet::new(),
|
||||
true,
|
||||
);
|
||||
|
||||
// `a` was revisited from `b` so the resolver AND-folded its flag
|
||||
// to false — that part already works. The bug is purely in the
|
||||
// descendants.
|
||||
let mut a_children = BTreeMap::new();
|
||||
a_children.insert("c".to_string(), DepPath::from("c@1.0.0".to_string()));
|
||||
let a = make_node_with_optional(
|
||||
"a",
|
||||
"1.0.0",
|
||||
json!({ "name": "a", "version": "1.0.0", "dependencies": { "c": "^1.0.0" } }),
|
||||
a_children,
|
||||
BTreeMap::new(),
|
||||
HashSet::new(),
|
||||
false,
|
||||
);
|
||||
|
||||
let mut b_children = BTreeMap::new();
|
||||
b_children.insert("a".to_string(), DepPath::from("a@1.0.0".to_string()));
|
||||
let b = make_node_with_optional(
|
||||
"b",
|
||||
"1.0.0",
|
||||
json!({ "name": "b", "version": "1.0.0", "dependencies": { "a": "^1.0.0" } }),
|
||||
b_children,
|
||||
BTreeMap::new(),
|
||||
HashSet::new(),
|
||||
false,
|
||||
);
|
||||
|
||||
let mut graph = DependenciesGraph::new();
|
||||
graph.insert(a.dep_path.clone(), a);
|
||||
graph.insert(b.dep_path.clone(), b);
|
||||
graph.insert(c.dep_path.clone(), c);
|
||||
|
||||
let mut direct = BTreeMap::new();
|
||||
direct.insert("a".to_string(), DepPath::from("a@1.0.0".to_string()));
|
||||
direct.insert("b".to_string(), DepPath::from("b@1.0.0".to_string()));
|
||||
|
||||
let resolved = ResolveImporterResult {
|
||||
resolved_tree: ResolvedTree::default(),
|
||||
peers_result: ResolvePeersResult {
|
||||
graph,
|
||||
direct_dependencies_by_alias: direct,
|
||||
peer_dependency_issues: PeerDependencyIssues::default(),
|
||||
},
|
||||
};
|
||||
|
||||
let lockfile = dependencies_graph_to_lockfile(GraphToLockfileOptions {
|
||||
manifest: &manifest,
|
||||
resolved: &resolved,
|
||||
auto_install_peers: false,
|
||||
exclude_links_from_lockfile: false,
|
||||
overrides: None,
|
||||
ignored_optional_dependencies: None,
|
||||
});
|
||||
|
||||
let snapshots = lockfile.snapshots.as_ref().expect("snapshots map");
|
||||
let a_key: PackageKey = "a@1.0.0".parse().unwrap();
|
||||
let b_key: PackageKey = "b@1.0.0".parse().unwrap();
|
||||
let c_key: PackageKey = "c@1.0.0".parse().unwrap();
|
||||
assert!(!snapshots[&b_key].optional, "b is a direct prod dep");
|
||||
assert!(!snapshots[&a_key].optional, "a is reachable via prod → b → a");
|
||||
assert!(!snapshots[&c_key].optional, "c is reachable via prod → b → a → c");
|
||||
}
|
||||
|
||||
/// Ported from upstream pnpm's
|
||||
/// [`'subdependency is both optional and dev'`](https://github.com/pnpm/pnpm/blob/b9de85dcb6/lockfile/pruner/test/index.ts#L378-L449)
|
||||
/// pruner test: when one shared subdep is reached via a dev parent's
|
||||
/// `optionalDependencies` and a prod parent's `dependencies`, only
|
||||
/// the strictly-optional subdep ends up `optional: true`.
|
||||
#[test]
|
||||
fn shared_subdep_reached_through_dev_optional_and_prod_paths_is_marked_non_optional() {
|
||||
let (_tmp, manifest) = write_manifest(json!({
|
||||
"name": "fixture",
|
||||
"version": "1.0.0",
|
||||
"dependencies": { "prod-parent": "^1.0.0" },
|
||||
"devDependencies": { "parent": "^1.0.0" },
|
||||
}));
|
||||
|
||||
let subdep = make_node_with_optional(
|
||||
"subdep",
|
||||
"1.0.0",
|
||||
json!({ "name": "subdep", "version": "1.0.0" }),
|
||||
BTreeMap::new(),
|
||||
BTreeMap::new(),
|
||||
HashSet::new(),
|
||||
true,
|
||||
);
|
||||
let subdep2 = make_node_with_optional(
|
||||
"subdep2",
|
||||
"1.0.0",
|
||||
json!({ "name": "subdep2", "version": "1.0.0" }),
|
||||
BTreeMap::new(),
|
||||
BTreeMap::new(),
|
||||
HashSet::new(),
|
||||
true,
|
||||
);
|
||||
|
||||
let mut parent_children = BTreeMap::new();
|
||||
parent_children.insert("subdep".to_string(), DepPath::from("subdep@1.0.0".to_string()));
|
||||
parent_children.insert("subdep2".to_string(), DepPath::from("subdep2@1.0.0".to_string()));
|
||||
let parent = make_node_with_optional(
|
||||
"parent",
|
||||
"1.0.0",
|
||||
json!({
|
||||
"name": "parent",
|
||||
"version": "1.0.0",
|
||||
"optionalDependencies": { "subdep": "^1.0.0", "subdep2": "^1.0.0" },
|
||||
}),
|
||||
parent_children,
|
||||
BTreeMap::new(),
|
||||
HashSet::new(),
|
||||
false,
|
||||
);
|
||||
|
||||
let mut prod_children = BTreeMap::new();
|
||||
prod_children.insert("subdep2".to_string(), DepPath::from("subdep2@1.0.0".to_string()));
|
||||
let prod_parent = make_node_with_optional(
|
||||
"prod-parent",
|
||||
"1.0.0",
|
||||
json!({
|
||||
"name": "prod-parent",
|
||||
"version": "1.0.0",
|
||||
"dependencies": { "subdep2": "^1.0.0" },
|
||||
}),
|
||||
prod_children,
|
||||
BTreeMap::new(),
|
||||
HashSet::new(),
|
||||
false,
|
||||
);
|
||||
|
||||
let mut graph = DependenciesGraph::new();
|
||||
graph.insert(parent.dep_path.clone(), parent);
|
||||
graph.insert(prod_parent.dep_path.clone(), prod_parent);
|
||||
graph.insert(subdep.dep_path.clone(), subdep);
|
||||
graph.insert(subdep2.dep_path.clone(), subdep2);
|
||||
|
||||
let mut direct = BTreeMap::new();
|
||||
direct.insert("parent".to_string(), DepPath::from("parent@1.0.0".to_string()));
|
||||
direct.insert("prod-parent".to_string(), DepPath::from("prod-parent@1.0.0".to_string()));
|
||||
|
||||
let resolved = ResolveImporterResult {
|
||||
resolved_tree: ResolvedTree::default(),
|
||||
peers_result: ResolvePeersResult {
|
||||
graph,
|
||||
direct_dependencies_by_alias: direct,
|
||||
peer_dependency_issues: PeerDependencyIssues::default(),
|
||||
},
|
||||
};
|
||||
|
||||
let lockfile = dependencies_graph_to_lockfile(GraphToLockfileOptions {
|
||||
manifest: &manifest,
|
||||
resolved: &resolved,
|
||||
auto_install_peers: false,
|
||||
exclude_links_from_lockfile: false,
|
||||
overrides: None,
|
||||
ignored_optional_dependencies: None,
|
||||
});
|
||||
|
||||
let snapshots = lockfile.snapshots.as_ref().unwrap();
|
||||
let subdep_key: PackageKey = "subdep@1.0.0".parse().unwrap();
|
||||
let subdep2_key: PackageKey = "subdep2@1.0.0".parse().unwrap();
|
||||
assert!(snapshots[&subdep_key].optional, "subdep only reachable via dev → optional path");
|
||||
assert!(
|
||||
!snapshots[&subdep2_key].optional,
|
||||
"subdep2 is reachable via prod-parent → subdep2 (all non-optional)",
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user