Skip to content

Commit ff5eafc

Browse files
committed
refactor: 边图查询收敛成 mcpp.build.dep_graph,而不是再手写一遍
自审时发现的问题,而且正是本 PR 通篇在反对的那个模式:我给 tool store 的 upstream key **手写了一遍图遍历**,而 prepare.cppm 里**已经有**一个走同一张边图的递归遍历 (,构建缓存的 per-package key),带 memo 和环检测 —— 我那份是它的 一个更弱的副本。 核实后: 有 14 处读者,其中大多数问的是同两个问题 ——「X 直接依赖 谁」和「X 的传递闭包」。两个 build.mcpp 调用点(发 的那两处) 已经漂成了近乎逐字重复的两份,而 #355 正要再加第三个变体。 新增 : - / (按边类型模板化 —— 是 prepare_build 里的局部结构体,把它搬出来的代价远大于这次要还的债) - :一个依赖可被寻址的两种拼写(canonical 与去 namespace 的尾段)。 不是图查询,但放这里理由相同:三处各自展开过同一个 ,第四处正要出现。 **刻意不迁移**构建缓存那条递归 fold:它不是闭包查询 —— 它对每个节点算一个值(该包 完整的 cache key)、把环当**硬错误**而不是跳过、还顺带穿了一个 taint 标记。把它塞进 通用遍历要么丢掉这些性质,要么让抽象一路长到只描述一个调用者。**回答不同问题的两次 遍历不是重复。** 验证:单测 57/57;125/145(正是覆盖 MCPP_DEP_*_DIR 的两个)与 111/186/187/188/189 全通过。
1 parent 00847b8 commit ff5eafc

2 files changed

Lines changed: 138 additions & 56 deletions

File tree

src/build/dep_graph.cppm

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
// mcpp.build.dep_graph — queries over the resolved consumer→dependency edge
2+
// graph.
3+
//
4+
// WHY THIS EXISTS
5+
//
6+
// prepare.cppm records one authoritative edge graph during resolution, and by
7+
// now fourteen places read it. Most ask one of exactly two questions —
8+
// "what does package X depend on directly?" and "what is X's transitive
9+
// closure?" — and each had hand-written the loop. Two of them (the two
10+
// build.mcpp call sites emitting MCPP_DEP_<NAME>_DIR) were near-identical
11+
// copies, and #355 added a third variant plus a hand-rolled BFS.
12+
//
13+
// That is the shape this codebase keeps paying for (#233/#240/#242/#344): the
14+
// same decision derived in N places does not fail when you add the N+1th, it
15+
// fails later, somewhere else. Feature activation already learned it the hard
16+
// way — activation and resolution each walked their own idea of "the edges"
17+
// and silently disagreed about transitive requests (#242/#243).
18+
//
19+
// Templated on the edge type rather than owning it: `DependencyEdge` is a
20+
// local struct inside prepare_build, and moving it out would be a much larger
21+
// change than the one this pays for. An edge only has to expose
22+
// `consumerPackageIndex` and `dependencyPackageIndex`.
23+
//
24+
// WHAT IS DELIBERATELY *NOT* HERE
25+
//
26+
// The build cache's per-package key walk (prepare.cppm, `self(self, …)`) is
27+
// NOT a closure query and is not migrated. It is a memoized fold that computes
28+
// a value per node (that package's full cache key), treats a cycle as a hard
29+
// ERROR rather than something to skip, and threads a taint flag alongside.
30+
// Folding it into a generic traversal would either lose those properties or
31+
// force the abstraction to grow until it described exactly one caller. Two
32+
// walks that answer genuinely different questions are not duplication.
33+
34+
export module mcpp.build.dep_graph;
35+
36+
import std;
37+
38+
export namespace mcpp::build::dep_graph {
39+
40+
// Package indices this consumer depends on DIRECTLY, in edge-record order,
41+
// deduplicated. Order is preserved because several callers surface it to the
42+
// user (dependency dirs, diagnostics) and a stable order keeps output
43+
// diffable.
44+
template <class Edge>
45+
std::vector<std::size_t>
46+
direct_dependencies(const std::vector<Edge>& edges, std::size_t consumer) {
47+
std::vector<std::size_t> out;
48+
for (auto const& e : edges) {
49+
if (e.consumerPackageIndex != consumer) continue;
50+
if (std::find(out.begin(), out.end(), e.dependencyPackageIndex) == out.end())
51+
out.push_back(e.dependencyPackageIndex);
52+
}
53+
return out;
54+
}
55+
56+
// Every package reachable from `from`, excluding `from` itself. Sorted and
57+
// deduplicated, so a caller folding it into a cache key gets a stable answer
58+
// without re-sorting.
59+
//
60+
// A cycle is TOLERATED here (the visited set terminates it) rather than
61+
// reported. This is a reachability question, and the callers that must reject
62+
// a cycle — the build-cache key walk — detect it where they can say which
63+
// package the cycle runs through, which is the only form of that message worth
64+
// printing.
65+
template <class Edge>
66+
std::vector<std::size_t>
67+
transitive_dependencies(const std::vector<Edge>& edges, std::size_t from) {
68+
std::set<std::size_t> seen;
69+
std::vector<std::size_t> stack{from};
70+
while (!stack.empty()) {
71+
auto cur = stack.back();
72+
stack.pop_back();
73+
for (auto const& e : edges) {
74+
if (e.consumerPackageIndex != cur) continue;
75+
if (!seen.insert(e.dependencyPackageIndex).second) continue;
76+
stack.push_back(e.dependencyPackageIndex);
77+
}
78+
}
79+
seen.erase(from);
80+
return {seen.begin(), seen.end()};
81+
}
82+
83+
// The two spellings a dependency is addressable by: its canonical package name
84+
// and, when it is namespaced, the namespace-stripped tail.
85+
//
86+
// Not a graph query, but it lives here for the same reason: three call sites
87+
// had each open-coded the `rfind('.')` split, and a fourth was about to. A
88+
// consumer may write `compat.zlib` or `zlib`, and every place that surfaces a
89+
// dependency by name has to accept both.
90+
inline std::vector<std::string> name_spellings(const std::string& canonical) {
91+
std::vector<std::string> out{canonical};
92+
if (auto dot = canonical.rfind('.');
93+
dot != std::string::npos && dot + 1 < canonical.size())
94+
out.push_back(canonical.substr(dot + 1));
95+
return out;
96+
}
97+
98+
} // namespace mcpp::build::dep_graph

src/build/prepare.cppm

Lines changed: 40 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import mcpp.build.cache_key;
3535
import mcpp.build.build_program;
3636
import mcpp.build.directives; // directive table: mark / fold_private_tail
3737
import mcpp.build.tool_store; // #355 host tools: store layout + key + overrides
38+
import mcpp.build.dep_graph; // queries over the resolved edge graph
3839
import mcpp.build.backend; // BuildOptions for the tool sub-build
3940
import mcpp.build.ninja; // make_ninja_backend — driving that sub-build
4041
import mcpp.lockfile;
@@ -2613,6 +2614,7 @@ prepare_build(bool print_fingerprint,
26132614
std::vector<std::string> requestedTools;
26142615
};
26152616
std::vector<DependencyEdge> dependencyEdges;
2617+
namespace dg = mcpp::build::dep_graph;
26162618
// #355: consumer package index → (env var, absolute path) for each host
26172619
// tool that consumer requested. Filled by the provisioning pass below;
26182620
// read by BOTH build.mcpp call sites (the dependency loop and the root),
@@ -2681,6 +2683,22 @@ prepare_build(bool print_fingerprint,
26812683
mcpp::build::directives::fold_private_tail(pkg.privateBuild, ran, t);
26822684
};
26832685

2686+
// mcpp#241: the (name → dir) pairs a package's build.mcpp receives as
2687+
// MCPP_DEP_<NAME>_DIR. ONE owner: the dependency loop and the root call
2688+
// site had drifted into two near-identical copies of this, and #355 was
2689+
// about to add a third. Each dependency is emitted under BOTH its
2690+
// canonical name and its namespace-stripped tail, so
2691+
// `mcpp::dep_dir("compat.zlib")` and `mcpp::dep_dir("zlib")` both resolve
2692+
// regardless of which spelling the author used in `deps`.
2693+
auto fillDepDirs = [&](mcpp::build::BuildProgramEnv& e, std::size_t consumer) {
2694+
for (auto d : dg::direct_dependencies(dependencyEdges, consumer)) {
2695+
auto const& depPkg = packages[d];
2696+
for (auto const& spelling :
2697+
dg::name_spellings(depPkg.manifest.package.name))
2698+
e.depDirs.emplace_back(spelling, depPkg.root);
2699+
}
2700+
};
2701+
26842702
// A declared build-graph node's Source outputs must be visible to the
26852703
// scan, so they are materialized as placeholders and joined to the source
26862704
// set here — the same two lists `generated=` feeds, for the same reason
@@ -3931,12 +3949,16 @@ prepare_build(bool print_fingerprint,
39313949
// construction rather than by luck.
39323950
for (auto const& [depName, spec] : m->dependencies) {
39333951
if (!spec.hostModule) continue;
3934-
for (auto const& edge : dependencyEdges) {
3935-
if (edge.consumerPackageIndex != 0) continue;
3936-
auto const& depPkg = packages[edge.dependencyPackageIndex];
3952+
for (auto d : dg::direct_dependencies(dependencyEdges, 0)) {
3953+
auto const& depPkg = packages[d];
39373954
auto const& canon = depPkg.manifest.package.name;
3938-
if (canon != depName && !depName.ends_with(canon)
3939-
&& !canon.ends_with(depName)) continue;
3955+
// Match on either spelling, the same way `deps` keys and
3956+
// MCPP_DEP_<NAME>_DIR do — a consumer may have written
3957+
// `compat.zlib` or `zlib`.
3958+
bool hit = false;
3959+
for (auto const& s : dg::name_spellings(canon))
3960+
if (depName == s || depName.ends_with("." + s)) hit = true;
3961+
if (!hit) continue;
39403962
auto rel = mcpp::manifest::resolve_lib_root_path(depPkg.manifest);
39413963
hostModulesByConsumer[0].emplace_back(canon, depPkg.root / rel);
39423964
break;
@@ -4043,26 +4065,11 @@ prepare_build(bool print_fingerprint,
40434065
// packages (a frozen version cannot change its own deps),
40444066
// but a path dependency can: bump something two levels down
40454067
// and the tool's direct list is unchanged, so a stale binary
4046-
// stays in the store. That is a silently wrong artifact —
4047-
// the failure mode this project has paid for more than once
4048-
// — and the closure walk costs nothing.
4049-
{
4050-
std::set<std::size_t> seen{depIdx};
4051-
std::vector<std::size_t> queue{depIdx};
4052-
while (!queue.empty()) {
4053-
auto cur = queue.back();
4054-
queue.pop_back();
4055-
for (auto const& edge : dependencyEdges) {
4056-
if (edge.consumerPackageIndex != cur) continue;
4057-
auto up = edge.dependencyPackageIndex;
4058-
if (!seen.insert(up).second) continue;
4059-
queue.push_back(up);
4060-
key.upstreamKeys.push_back(std::format("{}@{}",
4061-
packages[up].manifest.package.name,
4062-
packages[up].manifest.package.version));
4063-
}
4064-
}
4065-
}
4068+
// stays in the store — a silently wrong artifact.
4069+
for (auto up : dg::transitive_dependencies(dependencyEdges, depIdx))
4070+
key.upstreamKeys.push_back(std::format("{}@{}",
4071+
packages[up].manifest.package.name,
4072+
packages[up].manifest.package.version));
40664073
std::ranges::sort(key.upstreamKeys);
40674074

40684075
const auto cacheRoot = mcpp::home::cache_root();
@@ -4211,25 +4218,12 @@ prepare_build(bool print_fingerprint,
42114218
bpEnv.artifactsDir = workRoot / "target" / ".build-mcpp" / "deps"
42124219
/ (dirSafe(pkg.manifest.package.name) + "@" + pkg.manifest.package.version);
42134220
bpEnv.genBase = bpEnv.artifactsDir / "out";
4214-
// mcpp#241: expose this package's resolved dependencies (verdir /
4215-
// payload root) as MCPP_DEP_<NAME>_DIR. Uses the authoritative
4216-
// consumer→dep edge graph (no name-guessing); covers feature-
4217-
// activated deps too (mergeActiveFeatureDeps folded them into
4218-
// `dependencies` before the edges were recorded). A dep is emitted
4219-
// under BOTH its canonical package name AND its namespace-stripped
4220-
// short name, so `mcpp::dep_dir("compat.zlib")` and
4221-
// `mcpp::dep_dir("zlib")` both resolve regardless of which spelling
4222-
// the author used in `deps`. (The ROOT project's build.mcpp gets
4223-
// the same treatment at its own call site right after this loop.)
4224-
for (auto const& edge : dependencyEdges) {
4225-
if (edge.consumerPackageIndex != i) continue;
4226-
auto const& depPkg = packages[edge.dependencyPackageIndex];
4227-
const auto& canon = depPkg.manifest.package.name;
4228-
bpEnv.depDirs.emplace_back(canon, depPkg.root);
4229-
if (auto dot = canon.rfind('.'); dot != std::string::npos
4230-
&& dot + 1 < canon.size())
4231-
bpEnv.depDirs.emplace_back(canon.substr(dot + 1), depPkg.root);
4232-
}
4221+
// mcpp#241: this package's resolved dependencies as
4222+
// MCPP_DEP_<NAME>_DIR, from the authoritative edge graph (no
4223+
// name-guessing); covers feature-activated deps too
4224+
// (mergeActiveFeatureDeps folded them in before the edges were
4225+
// recorded). Shared owner — see fillDepDirs.
4226+
fillDepDirs(bpEnv, i);
42334227
// #355: the host tools THIS package requested (resolved above).
42344228
if (auto tit = toolEnvByConsumer.find(i); tit != toolEnvByConsumer.end())
42354229
bpEnv.toolPaths = tit->second;
@@ -4361,18 +4355,8 @@ prepare_build(bool print_fingerprint,
43614355
// contract hash — and therefore the build.mcpp cache — is unchanged
43624356
// across the move for feature-identical builds.
43634357
bpEnv.features = feature_closure(*m, parse_feature_request(overrides.features));
4364-
// mcpp#241 (root): the root's resolved direct deps, from the same
4365-
// authoritative edge graph as the dep loop (consumer index 0 = root),
4366-
// emitted under canonical AND namespace-stripped names.
4367-
for (auto const& edge : dependencyEdges) {
4368-
if (edge.consumerPackageIndex != 0) continue;
4369-
auto const& depPkg = packages[edge.dependencyPackageIndex];
4370-
const auto& canon = depPkg.manifest.package.name;
4371-
bpEnv.depDirs.emplace_back(canon, depPkg.root);
4372-
if (auto dot = canon.rfind('.'); dot != std::string::npos
4373-
&& dot + 1 < canon.size())
4374-
bpEnv.depDirs.emplace_back(canon.substr(dot + 1), depPkg.root);
4375-
}
4358+
// mcpp#241 (root): consumer index 0, same owner as the dep loop.
4359+
fillDepDirs(bpEnv, 0);
43764360
// #355: the host tools the ROOT package requested (consumer index 0).
43774361
if (auto tit = toolEnvByConsumer.find(0u); tit != toolEnvByConsumer.end())
43784362
bpEnv.toolPaths = tit->second;

0 commit comments

Comments
 (0)