From d1d02cb4b6e886df53d38e74710d73f495afbc17 Mon Sep 17 00:00:00 2001 From: sunrisepeak Date: Mon, 3 Aug 2026 16:51:36 +0800 Subject: [PATCH 1/2] fix(cache): a package's object layout must not depend on the consumer (#344) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A dependency's objects live in the global build cache under a key that deliberately excludes the consuming project — that exclusion is what makes an entry shareable across projects. Their LAYOUT inside the entry did not exclude it: the address was the consumer's build-dir path with `obj/` stripped, and build-dir object paths are decided by #233's basename disambiguation, whose census spans the WHOLE build directory. So `compat.zlib`'s compress.o was obj/compress.o alone obj/compat_zlib/zlib-1.3.2/compress.o beside compat.bzip2 under one key. Whichever project ran second asked the entry for a path the first had never written, and ninja rejected the graph before running any command: ninja: error: '/…/obj/compress.o', needed by 'obj/compress.o', missing and no known rule to make it one line after the CLI printed `Cached compat.zlib v1.3.2 (15 units)`. 32 of 47 mcpp-index workspace members across three platforms. #233 (compile edges collided), #240 (link inputs didn't follow the rename) and this are three products of one machine: a layout decided by a global census. The fix is not a fourth place to keep in sync — it is to take dependencies out of the census. * plan.cppm now emits TWO addresses from ONE derivation: `object` (build dir) and `packageObjectRel` (inside a cache entry, a pure function of the owning package). Dependency objects go to `obj///.o` unconditionally — cross-package collisions are structurally impossible, so dependencies need no census at all. The root project, never cached, keeps its flat `obj/.o` and now censuses only its own sources. * prepare.cppm's `object_cache_path` — the second derivation — is deleted, along with its `filename()` fallback, which silently mapped distinct sources onto one entry address. * `is_cached(key)` validated the entry's OWN file list while the caller went on to read addresses it computed itself; the two answers were never compared. It is now `probe_cached(key, requested)`. Any divergence is a MISS, never a build failure, and is reported — a systematic recurrence would otherwise show up only as "the cache never hits", with no signal at all. * Cacheability is judged by where the payload is on disk, not by the `sourceKind` label. Multi-version mangling re-anchors a consumer package at `target/.mangled/…` and rewrites its sources while keeping that label; it stays out of the cache today only because axis F happens to differ. Containment is judged LEXICALLY (`path_is_under_any`): stores built out of symlinks are ordinary, and canonicalizing drops every such package out of the cache silently. * All-or-nothing per package: one unaddressable unit takes the whole package out, rather than leaving it half staged. * kCacheEpoch 1 → 2 (the artifact layout changed) and `cache verify` now reports obj addresses that escape their entry. Tests: unit ObjectAddress pins "a dependency's addresses are immune to the rest of the graph"; e2e 184 runs both orderings and asserts ZERO compile edges for the reused dependency — asserting only "it builds" would be satisfied by a build that quietly degraded every hit to a miss. e2e 123's assertion is generalized from "the consumer's main must be renamed" (the shape of #240's first fix) to "every link input is produced by some edge" (the property). --- ...03-issue344-cache-object-address-design.md | 448 ++++++++++++++++++ CHANGELOG.md | 25 + mcpp.toml | 2 +- src/bmi_cache.cppm | 139 +++++- src/bmi_cache/maintenance.cppm | 19 +- src/build/cache_key.cppm | 8 +- src/build/plan.cppm | 245 ++++++++-- src/build/prepare.cppm | 128 +++-- src/toolchain/fingerprint.cppm | 2 +- tests/e2e/123_same_named_main_across_dep.sh | 38 +- tests/e2e/184_build_cache_object_layout.sh | 193 ++++++++ tests/unit/test_bmi_cache.cpp | 108 ++++- tests/unit/test_object_address.cpp | 252 ++++++++++ 13 files changed, 1487 insertions(+), 120 deletions(-) create mode 100644 .agents/docs/2026-08-03-issue344-cache-object-address-design.md create mode 100755 tests/e2e/184_build_cache_object_layout.sh create mode 100644 tests/unit/test_object_address.cpp diff --git a/.agents/docs/2026-08-03-issue344-cache-object-address-design.md b/.agents/docs/2026-08-03-issue344-cache-object-address-design.md new file mode 100644 index 00000000..3ad2ba2f --- /dev/null +++ b/.agents/docs/2026-08-03-issue344-cache-object-address-design.md @@ -0,0 +1,448 @@ +# issue #344：全局 build cache 的对象地址必须与消费方无关 + +> 状态：**已实施**（2026.8.3.4）。实施中与设计的两处偏差记录在 §8。 +> 关联：#233（编译边撞名）、#240（链接输入未跟改名）、#344（cache 布局跟改名而 key 未跟改） +> 涉及：`src/build/plan.cppm`、`src/build/prepare.cppm`、`src/bmi_cache.cppm`、 +> `src/bmi_cache/maintenance.cppm`、`src/build/cache_key.cppm`、`src/build/ninja_backend.cppm` + +--- + +## 0. 结论摘要 + +issue 的归因是对的，但只描述到第一层。代码核实后，这一条实际叠了**四层**问题， +其中两层是 #344 本身，两层是同一根因的其它实例（其中一个是**静默产出错误产物**， +比 #344 的崩溃更危险）： + +| 层 | 问题 | 失败模态 | +|---|---|---| +| L0 | cache 条目内部的 `.o` 地址直接复用了消费方 build dir 的相对路径 | ninja graph 阶段崩（#344） | +| L1 | `obj/` 是**全局**命名空间，消歧普查跨越所有包；而 cache 条目是**per-package** 的 | 结构性地生成 L0 这类 bug（#233/#240/#344 同族） | +| L2 | 「命中判据」与「取用地址」是**两处独立推导** —— `is_cached` 校验条目自述的文件表，消费方却按自己算的地址去取 | 任何布局分歧都从「降级为 miss」变成「硬崩」 | +| L3 | 「可缓存性」用**标签**（`sourceKind == "version"`）判定，而不是用磁盘**出处** | `target/.mangled/` 重锚的包被判为可缓存；今天仅靠轴 F 侥幸不出错 | + +修复方向：**把「cache 条目内的产物地址」定义为包自身的纯函数，并让它成为唯一真源； +命中判据改为校验「本次实际要取的那批文件」；可缓存性改判出处。** +`--cache local` 是 workaround，不进方案。 + +--- + +## 1. 精确机制（代码级） + +### 1.1 消歧普查的作用域是全局的 + +`src/build/plan.cppm:587-607`： + +```cpp +std::map basenameCount; +for (auto idx : topoOrder) { // ← 全图所有包的所有 TU + basenameCount[object_filename_for(graph.units[idx].path, objExt)]++; + ... +} +for (auto& t : manifest.targets) { ... } // ← 再叠上 root 的 entry main（#240） +``` + +`src/build/plan.cppm:651-660`： + +```cpp +auto object_for = [&](src, pkg, relPath) { + const auto fname = object_filename_for(src, objExt); + if (basenameCount[fname] > 1) // ← 判据取自全局普查 + return "obj" / safe_object_prefix(pkg, relPath.parent_path()) / fname; + return "obj" / fname; // ← 否则平铺 +}; +``` + +于是：一个**依赖包**的 `.o` 路径，取决于**别的包有没有同名 basename**。 +`compat.zlib` 的 `compress.c`： + +- 消费方同时拉了 `compat.bzip2`（也有 `compress.c`）→ `obj/compat_zlib/zlib-1.3.2/compress.o` +- 消费方只拉 `compat.zlib` → `obj/compress.o` + +### 1.2 这个消费方相关的路径被直接当成条目内部地址 + +`src/build/prepare.cppm:4317-4328`： + +```cpp +auto object_cache_path = [&](const std::filesystem::path& objectPath) { + ... objectPath.lexically_relative("obj") // ← 只是把 "obj/" 前缀剥掉 + return objectPath.filename().generic_string(); // ← 兜底：退化成裸 basename +}; +``` + +`prepare.cppm:4516`（写入侧）与 `prepare.cppm:4530-4531`（读取侧）都用它： + +```cpp +arts.objFiles.push_back(object_cache_path(cu.object)); // populate 用 +cu.cachedObject = cached_obj_path(key, object_cache_path(cu.object)); // stage 用 +``` + +而 `cache_key.cppm` 的七轴（A 工具链 / B 语言 / C profile / D 身份 / E 自身配置 / +F 上游 Merkle / G epoch）**刻意不含 root 的身份与 flags** —— 这个前提对 +**产物内容**成立（root 的 cflags 确实不下发到依赖 TU），对**产物文件名**不成立。 +同一个 key `58d45813f79658a1`，两种布局。 + +### 1.3 命中判据看的是错的那张表 + +`src/bmi_cache.cppm:202-217`： + +```cpp +bool is_cached(const CacheKey& key) { + ... + auto arts = artifacts_from(*j); // ← 条目自述的文件表（第一个消费者写的） + for (auto& o : arts.objFiles) + if (!exists(cached_obj_path(key, o))) return false; + return true; +} +``` + +注意 `prepare.cppm` 在调用 `is_cached` **之前**（4499-4518 行）就已经算好了 +**本次真正要取的那批 `arts`**，然后在 4520 行调用 `is_cached(key)` —— 把它扔了。 +所以： + +- 方向 A（archive 先跑）：条目记录 14 平 + 1 嵌套，全部存在 ⇒ `is_cached` 返回 true； + libpng 却按自己的 15 个平地址去 stage ⇒ `obj/compress.o` 不存在。 +- 方向 B 完全对称。 + +**谁第二个跑谁挂**，且必然挂在 ninja **graph 加载**阶段：stage 边形如 + +```ninja +build obj/compress.o : stage_file /…/obj/compress.o +``` + +输入是绝对路径、图内无规则生成、磁盘上又不存在 ⇒ +`missing and no known rule to make it`。 +`stage.cppm:185` 那条 `staging source does not exist` 的运行期守卫**永远到不了** +（ninja 在跑任何命令之前就已经拒绝了图）。这解释了为什么错误信息如此难懂， +也决定了**修复只能落在 plan 阶段，不能落在 stage 阶段**。 + +### 1.4 与既有契约自相矛盾 + +`src/bmi_cache/maintenance.cppm:531`（`mcpp cache verify` 的输出文案）写着： + +> "They are treated as misses and rebuilt" + +这正是应有的契约：**不完整的条目降级为 miss**。 +但 `is_cached` 只对「条目缺自己记录的文件」执行这个契约， +对「条目的布局 ≠ 本次要的布局」不执行 —— 契约声明了，但没有被完整实现。 + +--- + +## 2. 根因分层与判据 + +### L1 —— 作用域不匹配（这才是生成 bug 的那台机器） + +一句话：**`.o` 的名字活在「整个 build dir」这个命名空间里，而 cache 条目活在「单个包」这个命名空间里。** +用一个作用域的标识符去做另一个作用域的地址，两者的等价关系不同 —— 必然出事。 + +#233 / #240 / #344 是同一台机器吐出的三个产物： + +- #233：全局普查产生的前缀本身不够唯一 → ninja「multiple rules generate」 +- #240：普查漏了 entry main → 链接输入与编译边分叉 +- #344：普查结果随消费方变化 → cache 条目布局随消费方变化 + +**只要「布局由一次全局普查决定」这个机制还在，这一族就会有第四个。** +所以正确的修法不是再补一处同步，而是**拆掉普查对依赖包的管辖权**。 + +### L2 —— 同一决策两处推导 + +「这个条目里 `compress.o` 叫什么」这件事，今天有两个独立推导： + +1. 条目自己的 `entry.json:obj[]`（第一个 populate 者写的） +2. 本次构建的 `object_cache_path(cu.object)` + +`is_cached` 只查 (1)，stage 边只用 (2)，两者从不比对。 +这是本仓库反复出现的那个隐性架构债形态（见 #239/#240、#315、#336 的复盘）： +**同一决策两处推导，加新语义时会变成构建失败。** + +### L3 —— 可缓存性用标签而非出处 + +`prepare.cppm:4469-4474` 的注释把规则写得很清楚： + +> anything it cannot prove came from the immutable xpkgs store stays out + +但代码实现的是一个**更弱的代理**：`depIdent->sourceKind != "version"`。 +反例就在同一个文件里 —— 多版本共存（mangling）路径，`prepare.cppm:3120-3171`： + +```cpp +auto stageBase = *root / "target" / ".mangled" / consumerManifest.package.name; +... +packages[item.consumerDepIndex + 1].root = consumerStage; // ← root 重锚到 target/ 下 +... +dep_cache_identities.push_back({ ..., .sourceKind = "version" }); // ← 标签仍是 version +``` + +消费方包的 `root` 被重锚到 `/target/.mangled//__self__`， +**源码被 `rewrite_module_decls` 改写过**（import 名被重命名）， +但它的 `sourceKind` 仍是 `"version"`，`localTaint` 也不会点亮它 +（被 mangle 的次要包同样是 `"version"`）。 +即：一个源码已被改写、根目录位于可变的 `target/` 下的包，**被判定为可缓存**。 + +今天没出错，是因为轴 F 恰好救了场 —— 次要包的 D 轴 `packageName` 变成了 mangled 名， +消费方的 `upstreamKeys` 因此不同，key 分裂。 +**这是一个「侥幸成立、且只由一条轴支撑」的不变量**，而它守护的失败模态是 +「静默取到用错模块名编出来的 `.o`」—— 比 #344 的崩溃更难诊断。 +`cache_key.cppm:41` 自己写过这个不对称性： + +> too narrow a key on the object axis is a silently wrong `.o` — objects carry no such self-check. + +--- + +## 3. 方案 + +七项，R1–R4 是必须的一组（缺任何一项都留下漏洞），R5–R7 是配套。 + +### R1（必须）· 依赖包的 obj 命名空间按包切分，且**无条件镜像** + +**改 `plan.cppm` 的 `object_for`：依赖包的对象一律落在自己的子树下，且不看普查。** + +``` +root 包（永不入 cache，保持现状）： + obj/.o // 无冲突 + obj///.o // 冲突时（现有行为） + +依赖包（可入 cache）： + obj///.o // 无条件，不做普查 +``` + +- `` = `sanitize(qualified package name)`（现有 `sanitize`，`.`/`/` → `_`） +- `` = 现有 `safe_object_prefix` 的 relDir 部分（去掉 pkg 前缀） + +三条性质，逐条对应上面三层： + +1. **跨包撞名结构性消失**（不同目录），普查对依赖包不再有管辖权 → L1 关闭。 +2. 包内撞名仍由包内普查处理 —— 但普查范围是包自身的 TU 集合， + 而「哪些 TU 属于这个包」已经完全被 key 的 E 轴（`sources` + `features` + + `generatedFiles` + `sourceGlobs`）覆盖，所以**是包的纯函数**。 + 本方案取消这层普查（无条件镜像），普查只留给 root。 +3. 条目内部地址 = build 路径剥掉 `obj//` = `/.o`， + **是包的纯函数**，与消费方无关 → L0 关闭。 + +**为什么无条件、不保留「无冲突则平铺」的优化**：那个条件判断就是生成 #233/#240/#344 +的那台机器。保留它就是保留状态；而它省下的只是路径长度。 + +**已知代价与缓解**：依赖对象路径变长（`obj/adler32.o` → `obj/compat_zlib/zlib-1.3.2/adler32.o`， +约 +25 字符）。Windows `MAX_PATH`=260 是唯一实际风险，且 build dir 本身已是 +`target///obj/...`。实施时**必须**用 mcpp-index 里路径最深的成员 +（ffmpeg / opencv-module）在 Windows 上实测。若触顶，退路是把 `` 换成 +``（仍是包的纯函数，性质不变），**不要退回条件化**。 + +### R2（必须）· 条目内地址收敛为唯一真源 + +`CompileUnit` 新增一个字段，由 `plan.cppm` 与 `object_for` **同一处**算出： + +```cpp +struct CompileUnit { + std::filesystem::path object; // build dir 相对路径（既有） + std::filesystem::path packageObjectRel; // cache 条目内相对地址；空 = 本单元不可缓存 + ... +}; +``` + +随之： + +- **删除** `prepare.cppm:4317-4328` 的 `object_cache_path` lambda —— 它是第二处推导。 +- populate 侧（`prepare.cppm:4516`）与 stage 侧（`prepare.cppm:4530`）都改用 + `cu.packageObjectRel`。 +- `bmi_cache::populate_from` 的 `projectObj / o` 需要从 `obj//` 起算， + 因此 `CacheKey` 增加 `objSubdir`（或直接传绝对源路径）， + 不再假设「条目内地址 == build dir 内 `obj/` 下的地址」—— + **这个假设正是 #344**，必须显式打破而不是巧合地维持。 + +`packageObjectRel` 的推导规则（含逃逸情形）： + +1. `relPath` 在包根内 → `safe_object_prefix(relDir) / object_filename_for(src)` +2. `relPath` 逃出包根（build.mcpp 的 `OUT_DIR` 生成源，注释见 `plan.cppm:613-624`） + → 先按 **xpkgs store root** 相对化，沿用 `fill_package_config` 已有的 + `/…` 手法，映射成 `__store//.o` +3. 两者都不成立 → `packageObjectRel` 置空 ⇒ 整个包**不可缓存**（见 R5 的全有全无） + +这条明确删掉了现有的 `return objectPath.filename()` 兜底 —— +那个兜底会把两个不同源文件静默映射到同一条目地址，是一个未被触发的 +「静默错误 `.o`」通道。 + +### R3（必须）· 命中判据校验「本次要取的那批」，不匹配一律降级为 miss + +```cpp +// bmi_cache.cppm +bool is_cached(const CacheKey& key, const DepArtifacts& requested); +``` + +语义： + +- `entry.json` 存在、schema 匹配、`key` 匹配、`inputs` 逐字段匹配（既有） +- **且** `requested` 的每一项都在条目的记录表中，**且**在磁盘上存在 +- 任一不成立 ⇒ **miss**（走 populate），**永不**成为构建失败 + +这一条是**稳定性护栏**，不是 #344 的修法本身：它把「条目布局与本次期望分歧」 +这一整类未知问题，从「ninja graph 阶段的天书错误」永久降级成「多编一次」。 +R1+R2 之后这个分歧不应再出现，正因为如此，**一旦出现就必须有个诊断出口**： + +``` +mcpp 侧（verbose 或 warn，一行）： + warning: build cache entry for compat.zlib@1.3.2 [58d4…] does not contain the + artifacts this build needs (2 of 15 missing, e.g. `compress.o`); + treating as a miss. Run `mcpp cache verify` for details. +``` + +没有这行，一个系统性的分歧会表现为「cache 永远 100% 不命中」而无任何信号 —— +这正是 v2026.7.30.2 之前那个假 `Cached` 骗了三个月的镜像失败模态。 + +同时把 `mcpp cache verify` 的检查扩展一档：**报告条目内地址是否符合规范形态** +（R1 定义的 `/.o`），使 L2 的一致性可离线审计。 + +### R4（必须）· `kCacheEpoch` 1 → 2 + +`cache_key.cppm:67` 的注释已经把判据写死了： + +> Bump ONLY when a change makes previously written entries unusable +> (the serialized input shape, **the artifact layout**, or the staging contract). + +R1 改的正是 artifact layout。不 bump 的话，旧条目会被新代码当成候选， +虽有 R3 兜底降级为 miss，但会在同一目录里叠加两套布局的文件， +让 `gc` 的体积统计与 `verify` 的输出都失真。bump epoch 是这里唯一干净的做法。 + +### R5（必须）· 可缓存性改判**出处**，并且全有全无 + +两处收紧，落在 `prepare.cppm:4452-4476`： + +**(a) 出处判据**。把 + +```cpp +if (!depIdent || depIdent->sourceKind != "version") continue; +``` + +改成「标签 **且** 磁盘出处」： + +```cpp +if (!depIdent || depIdent->sourceKind != "version") continue; +if (!is_under(packages[i].root, storeRoot)) continue; // 新增 +``` + +这直接实现了 4469-4473 行注释里已经写下的规则，并结构性地排除 +`target/.mangled/**` 重锚包（L3）。今天靠轴 F 侥幸成立的那个不变量， +从此有第二道、且是**按定义**成立的防线。 + +**(b) 全有全无**。若该包任一 `CompileUnit::packageObjectRel` 为空， +整个包退出缓存（既不读也不写）。理由：部分 stage 会让一个包的产物一半来自 +cache、一半来自本次编译 —— 这是 `.o`/BMI 混龄，恰是 GCC 把 BMI 的 CRC +烙进导入者时最难诊断的那种失败。 + +### R6（应做）· 不变量测试：地址对图的其余部分免疫 + +单元测试（`tests/unit/`），直接钉住 L1： + +> 构造两个 plan：图 X 只含包 P；图 Y 含包 P + 包 Q，且 Q 有与 P 同名的 basename。 +> 断言 P 的每个 `CompileUnit::packageObjectRel` 在 X 与 Y 中**逐字节相同**。 + +这条断言就是「条目地址是包的纯函数」的机器化表述。任何未来往 `object_for` +里加入图级状态的改动都会立刻挂在这里 —— 这正是 #233/#240/#344 三次都缺的那道闸。 + +### R7（应做）· e2e 复现 A/B 双向 + +新增 `tests/e2e/1xx_build_cache_object_layout.sh`，形状照抄 +`172_build_cache_cross_project.sh`（离线 path index + `fresh-sandbox`）： + +- 本地索引提供 `lib-a` 与 `lib-b`，**两者各有一个同名源文件** `compress.c` +- 工程 `both` 依赖 a+b；工程 `only-a` 只依赖 a +- **方向 A**：先 `both` 后 `only-a`；**方向 B**：清 cache，先 `only-a` 后 `both` +- 两个方向都必须构建成功，且第二个工程的 `build.ninja` 里 + **`lib-a` 的源文件零 compile 边**（沿用 172 的判据：从 `build.ninja` 读，不从状态行读） + +「零 compile 边」这条不能省 —— 只断言「构建成功」会被 R3 的 miss 降级悄悄满足， +测试就变成了假绿。 + +--- + +## 4. 不采纳的方案 + +| 方案 | 不采纳的理由 | +|---|---| +| 把「本次消歧结果」并入 cache key | issue 自己已经指出：按消费方分裂条目，与跨工程共享的设计目标直接冲突。26GB/1198 目录那次的教训就是键里混进了消费方。 | +| 只在 stage 阶段回退（源不存在则改用 compile 边） | ninja 在 **graph 加载**阶段就已失败，任何运行期回退都到不了（§1.3）。必须落在 plan 阶段 = R3。 | +| 条目里同时存两套布局 | 把 L2 的「两处推导」升级成「两处存储」。体积翻倍，且第三种布局出现时同样失效。 | +| 只做 R3（把崩溃降级为 miss）不做 R1 | 症状消失，但 zlib/ffmpeg 这类高扇入包在混合工程里将**永远不命中**，cache 收益归零而无任何信号 —— 与 `--cache local` 等价，只是更隐蔽。 | +| 只做 R1 不做 R3 | 修掉了今天这一个实例，留着「布局分歧 ⇒ graph 崩」这条通道给下一个实例。#233→#240→#344 已经证明会有下一个。 | + +--- + +## 5. 影响面与迁移 + +- **旧条目**：epoch bump 后自然失效，被 `mcpp cache gc` 回收。用户侧表现为一次全量重建， + 无需任何手工步骤，也不需要提示用户清 cache。 +- **build dir 布局变化**：依赖对象路径变化 ⇒ 首次构建全量重编（`target/` 内）， + 与 epoch bump 的影响重合，不额外增加成本。 +- **受影响的下游读者**：`compile_commands.json`、`distribution.cppm`、 + `.ddi` 放置（`ninja_backend.cppm:1026` 明确「跟随对象路径」）、链接输入 + —— 全部派生自 `cu.object` 这一真源，随之自动跟随；**不得**新增任何一处独立推导。 +- **CI**：mcpp-index 全量 workspace（linux/macos/windows 三 leg，47 成员）是本条的 + 最终验收面 —— 修复前 13/11/8 失败，修复后须回到 `all 47 member(s) passed`， + 且**必须核对第二个成员的 `build.ninja` 确有 stage 边**（否则可能是「全都变 miss」的假绿）。 + +--- + +## 6. 实施顺序 + +1. R4（epoch bump）—— 先行，使中间态不会读到旧布局条目 +2. R1 + R2 —— 一起改，`plan.cppm` 与 `prepare.cppm` 同一次提交（真源迁移不可拆） +3. R3 —— `is_cached` 签名变更 + 诊断 + `cache verify` 扩展 +4. R5 —— 出处判据 + 全有全无 +5. R6 + R7 —— 测试；R6 必须在 R1 之前写好并**看到它在 main 上失败** +6. mcpp-index 侧：pin 到预发布版本，跑全量 workspace 三 leg + +第 5 步的「先看到它在 main 上失败」不是形式主义：本仓库有过多次 +「测试写完就是绿的、其实根本没覆盖到」的记录（#230 的真凶早已被修好、 +#332 的「扫缓存比对字节等价」是假验证）。R6 必须先红后绿。 + +--- + +## 8. 实施记录（2026.8.3.4） + +落点：`plan.cppm`（R1/R2）、`prepare.cppm`（R2/R3/R5）、`bmi_cache.cppm`（R2/R3）、 +`bmi_cache/maintenance.cppm`（R3 审计）、`cache_key.cppm`（R4）、 +`tests/unit/test_object_address.cpp`（R6）、`tests/e2e/184_build_cache_object_layout.sh`（R7）。 + +与设计稿的三处偏差，都是实施时被现实证伪的假设： + +**① 「store root」不是一个目录，是一组。** 设计稿假设可缓存性判据可以写成 +「包根在 `/data/xpkgs` 之下」。实际上自定义 git 索引会把 payload 装进 +**项目本地**的数据根（`config::project_xlings_data_roots`: +`/.mcpp/data` 与 `/.mcpp/.xlings/data`）—— `tests/e2e/172` 正是 +这个形态。按单一 storeRoot 判定会把 172 里的依赖判成不可缓存， +表现为「cold build did not populate a cache entry」。 +所以 R5(a) 与 plan 的 `__store` 锚点都接受一个 `storeRoots` 列表。 + +> 这也是一条方法论证据:**先写测试再看它红，能同时验证「测试有效」和「判据正确」**。 +> 172 的红是判据过窄的第一手证据，如果只跑新增的 184，这个收窄会一路带到 CI。 + +**②「包根在 store 之下」必须按**字面路径**判定，不能用 `std::filesystem::relative`。** +`relative()` 对两侧都跑 `weakly_canonical`，**会解析符号链接**。而「store 里的条目是指向另一个 +store 的符号链接」是常态 —— `tests/e2e/_inherit_toolchain.sh` 就是这么把开发机的工具链 +借给隔离 MCPP_HOME 的，CI 缓存复用热 payload 树也是同一手法。一旦 canonical 化， +`/registry/data/xpkgs/` 就不再「位于 store 之下」，**该包静默退出缓存**。 + +这条被 e2e `40_llvm_bmi_cache` 抓到：第二次构建打印 `Compiling` 而不是 `Cached`。 +**症状是「cache 永远不命中」而非报错** —— 正是 R3 那条 warning 想要暴露、 +而这里恰好覆盖不到的形态(判据过窄时根本进不了 cache 那段代码，无从报告)。 +收敛为 `plan.cppm::path_is_under_any`，字面判定为主 + canonical 重试兜住路径拼写差异 +(Windows 上 `HOME` 与 `USERPROFILE`、盘符大小写)，两者任一为真即可 —— +**拼写不同最坏退化成慢，不会退化成错**。可缓存性门与 `__store` 锚点共用它。 + +**③ R6 的「先红」不能靠 checkout main 得到**，因为断言引用的 `packageObjectRel` +字段在 main 上不存在，测试根本编译不过 —— 那是编译失败，不是断言失败，证明不了任何事。 +实际做法是在新代码上**临时把 `object_for` 的依赖分支换回全局普查**， +确认两条断言精确地红（`RootObjectsStayFlatAndUncacheable` 保持绿），再还原。 +R7 则可以用真正的 pre-fix 二进制跑，并复现出与 issue 完全一致的报错文本。 + +验证状态：unit 54/54 通过；e2e 172 / 174 / 184 通过；184 在 pre-fix 二进制上 +按双方向各自复现 `missing and no known rule to make it`。 + +## 7. 遗留 / 后续 + +- **多版本共存 × 对象子树**：两个版本的同一包若同时在图中，`` 相同。 + 今天 mangling 会把次要包的 `package.name` 改成 mangled 名，因而 slug 天然分开； + 但这依赖 mangling 路径，不是结构保证。若将来出现「不 mangle 的多版本共存」， + slug 需要带版本。R6 的不变量测试无法覆盖此情形，**单独记一条**。 +- **L3 的彻底收敛**：`.mangled` 重锚让「包的 root 是不是不可变 payload」这件事 + 在 `prepare.cppm` 里有两处认知（`sourceKind` 标签与实际路径）。R5(a) 加的是防线， + 真正的收敛是让重锚同时更新 `dep_cache_identities` 的 sourceKind。 + 可与后续的 mangling 递归化改造一并做。 diff --git a/CHANGELOG.md b/CHANGELOG.md index 4095222f..2dea4208 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,31 @@ > 本文件追踪 `mcpp-community/mcpp` 公开仓的版本演进。 > 格式参考 [Keep a Changelog](https://keepachangelog.com/zh-CN/1.1.0/)。 +## [2026.8.3.4] — 2026-08-03 + +### 修复 + +- **全局 build cache:同一个 key 下第二个消费者必挂 `missing and no known rule to make it`(#344)。** 一个依赖的 `.o` 存在 cache 条目里的**路径**,此前取自消费方 build dir 的相对路径。而 build dir 里的对象布局由 #233 的 basename 消歧决定,消歧的判据是一次**跨越整个 build dir**的普查 —— 也就是说,它取决于消费方还拉了哪些别的包: + + - 同时拉了 `compat.zlib` 与 `compat.bzip2`(两者上游各有一个 `compress.c`)→ `obj/compat_zlib/zlib-1.3.2/compress.o` + - 只拉了 `compat.zlib` → `obj/compress.o` + + cache key **刻意不含消费方**(这正是跨工程共享成立的前提),于是两种布局落进同一个条目,后跑的那个工程按自己的布局去取,必然缺一个文件。失败发生在 ninja 的 **graph 加载**阶段 —— 一条命令都还没跑,上一行却刚打印过 `Cached … (15 units)`。mcpp-index 全量 workspace 在三个平台上共 32 个成员因此失败。 + + 条目内的对象地址现在是**包自身的纯函数**:镜像源文件相对它**自己**包根的路径,不含任何消费方信息。构建目录里,依赖包的对象一律落在 `obj//…` 下 —— 跨包撞名结构性地不可能发生,依赖包因此完全退出消歧普查。根工程(永不入 cache)保持沿用至今的扁平 `obj/.o`。 + + #233(编译边撞名)、#240(链接输入未跟改名)与本条是同一台机器的三个产物:**布局由一次全局普查决定**。所以修的不是再补一处同步,而是把依赖包从普查里彻底拿掉。 + +### 改进 + +- **cache 条目与本次构建的布局分歧,现在降级为 miss 并明确报告,而不是让 ninja 崩在图加载阶段。** `is_cached` 此前校验的是条目**自述的**文件表,而消费方随后按**自己算的**地址去取 —— 两处独立推导,从不比对。命中判据现在校验「本次实际要读的那批产物」,任何不匹配都只是一次重编。同时新增一行 warning:一个系统性的分歧否则会表现为「cache 永远不命中」而毫无信号,这正是 v2026.7.30.2 之前那个假 `Cached` 骗了三个月的失败模态。 + +- **可缓存性改判磁盘出处,不再只认标签。** 规则一直写着「无法证明来自不可变的 xpkgs store 就不准入」,但代码实现的是更弱的代理(`sourceKind == "version"`)。多版本共存(mangling)会把消费方包的根重锚到 `/target/.mangled/…` 并**改写其源码**,而标签仍是 `"version"` —— 它今天不出错只靠轴 F 侥幸。现在按包根的实际位置判定。同一批还加了「全有全无」:任何一个单元拿不到与机器无关的条目地址,整个包退出缓存,不留下半 staged 的包(那会表现为三条边之后的 BMI CRC mismatch)。 + +- **`mcpp cache verify` 现在会报告逃出条目的对象地址。** 让「条目地址必须是包内相对路径」这条不变量可以离线审计,而不是只能通过复现一次双工程构建才看得见。 + +- **`kCacheEpoch` 1 → 2。** 产物布局变了,旧条目描述的是本版本不会去要的布局。它们本来就会被判为 miss,但让两套布局共用一个目录会让 `cache gc` 的体积统计和 `cache verify` 的输出失去意义。用户侧表现为一次全量重建,无需任何手工步骤。 + ## [2026.8.3.3] — 2026-08-03 ### 修复 diff --git a/mcpp.toml b/mcpp.toml index 0c3be15c..fbd2b5d5 100644 --- a/mcpp.toml +++ b/mcpp.toml @@ -1,6 +1,6 @@ [package] name = "mcpp" -version = "2026.8.3.3" +version = "2026.8.3.4" description = "Modern C++ build & package management tool" license = "Apache-2.0" authors = ["mcpp-community"] diff --git a/src/bmi_cache.cppm b/src/bmi_cache.cppm index 24e7f63b..20101bbb 100644 --- a/src/bmi_cache.cppm +++ b/src/bmi_cache.cppm @@ -5,7 +5,16 @@ // (the cache root is $MCPP_HOME/build-cache/v1 — see mcpp.home::cache_root) // entry.json sentinel + self-description + file list // bmi/.{gcm,pcm} -// obj/.o +// obj/.o +// +// The obj address is PACKAGE-INTERNAL: it mirrors the source's path relative to +// its own package root and contains nothing about the consuming project +// (mcpp#344). It used to be the consumer's build-dir path with `obj/` stripped, +// which made the layout depend on which OTHER packages the consumer happened to +// pull in — #233's basename disambiguation is triggered by a census over the +// whole build dir — while the key deliberately excludes the consumer. Two +// consumers then wrote and read incompatible layouts under one key and the +// second one died in ninja's graph phase. // // comes from mcpp.build.cache_key: a per-package Merkle key over the // toolchain, the language/dialect settings, the resolved profile, the package's @@ -75,16 +84,63 @@ struct CacheKey { std::filesystem::path objDir() const { return dir() / "obj"; } }; -// File names (basenames for BMIs, output-relative paths for objects) belonging -// to one package's cache entry. +// One cached object file. The two addresses are deliberately separate +// (mcpp#344): +// +// cacheRel — where it lives INSIDE the entry (`/obj/`). +// Must be a pure function of the package, because the key +// deliberately excludes the consumer. plan.cppm derives it. +// buildRel — where THIS build produces it (`/`). +// Consumer-side and therefore not recordable: two consumers of +// one entry may legitimately place the same object at different +// build-dir paths. +// +// Collapsing the two — recording the consumer's path as the entry's address — +// is exactly what made the second consumer of an entry fail with ninja's +// "missing and no known rule to make it". Only `cacheRel` ever reaches +// entry.json; `buildRel` is populate-time input and is empty on read-back. +struct ObjArtifact { + std::string cacheRel; + std::filesystem::path buildRel; +}; + +// The artifacts belonging to one package's cache entry: BMI basenames plus the +// objects above. struct DepArtifacts { - std::vector bmiFiles; - std::vector objFiles; + std::vector bmiFiles; + std::vector objFiles; }; -// True when entry.json exists, its schema matches, its recorded inputs equal -// `key.inputs` field for field, and every listed file is present on disk. -bool is_cached(const CacheKey& key); +// Why an entry could not serve this build. +struct CacheProbe { + bool ok = false; + // Non-empty ONLY when the entry itself validated (schema, key, inputs) but + // does not carry the artifacts THIS build asked for. After mcpp#344 that + // shape should be unreachable, which is precisely why it must be reported + // rather than silently folded into "miss": a systematic recurrence would + // otherwise present as "the cache simply never hits", with no signal at + // all — the same failure mode as the fake `Cached` that went unnoticed for + // three months. + std::vector layoutMismatch; +}; + +// Validate an entry AGAINST WHAT THIS BUILD WILL ACTUALLY READ. +// +// A hit requires all of: entry.json exists, its schema matches, its recorded +// key matches, its recorded inputs equal `key.inputs` field for field, and +// every artifact in `requested` is both listed by the entry and present on +// disk. Checking only the entry's OWN file list — which is what this used to +// do — validates a different question than the one the caller goes on to ask, +// and the two answers diverged the moment the object layout stopped being a +// function of the package alone. +// +// Anything short of a full match is a MISS. This function must never be the +// reason a build fails: an unusable entry costs a recompile, and the staging +// edges that would read it are never emitted. +CacheProbe probe_cached(const CacheKey& key, const DepArtifacts& requested); + +// probe_cached(...).ok +bool is_cached(const CacheKey& key, const DepArtifacts& requested); // The artifact list of a validated entry. Does NOT copy anything: the ninja // backend stages cached files through its own `stage_file` edges, so that a @@ -137,12 +193,15 @@ std::optional read_entry(const std::filesystem::path& p) { return j; } +// Read-back fills `cacheRel` only: `buildRel` is consumer-side and is not — and +// must not be — recorded in the entry. DepArtifacts artifacts_from(const nlohmann::json& j) { DepArtifacts a; if (auto it = j.find("bmi"); it != j.end() && it->is_array()) for (auto& v : *it) if (v.is_string()) a.bmiFiles.push_back(v.get()); if (auto it = j.find("obj"); it != j.end() && it->is_array()) - for (auto& v : *it) if (v.is_string()) a.objFiles.push_back(v.get()); + for (auto& v : *it) + if (v.is_string()) a.objFiles.push_back({v.get(), {}}); return a; } @@ -199,21 +258,39 @@ std::filesystem::path cached_obj_path(const CacheKey& key, std::string_view rel) return key.objDir() / std::filesystem::path(std::string(rel)); } -bool is_cached(const CacheKey& key) { +CacheProbe probe_cached(const CacheKey& key, const DepArtifacts& requested) { + CacheProbe probe; auto j = read_entry(key.entryFile()); - if (!j) return false; - if (j->value("schema", 0) != kEntrySchema) return false; - if (j->value("key", std::string{}) != key.keyHex) return false; + if (!j) return probe; + if (j->value("schema", 0) != kEntrySchema) return probe; + if (j->value("key", std::string{}) != key.keyHex) return probe; auto it = j->find("inputs"); - if (it == j->end() || !inputs_match(*it, key.inputs)) return false; + if (it == j->end() || !inputs_match(*it, key.inputs)) return probe; + + // The entry itself is valid. From here on, every remaining check is about + // whether it holds what THIS build is going to read. + auto recorded = artifacts_from(*j); + std::set haveBmi(recorded.bmiFiles.begin(), recorded.bmiFiles.end()); + std::set haveObj; + for (auto& o : recorded.objFiles) haveObj.insert(o.cacheRel); - auto arts = artifacts_from(*j); std::error_code ec; - for (auto& g : arts.bmiFiles) - if (!std::filesystem::exists(cached_bmi_path(key, g), ec)) return false; - for (auto& o : arts.objFiles) - if (!std::filesystem::exists(cached_obj_path(key, o), ec)) return false; - return true; + for (auto& g : requested.bmiFiles) { + if (!haveBmi.contains(g) + || !std::filesystem::exists(cached_bmi_path(key, g), ec)) + probe.layoutMismatch.push_back(g); + } + for (auto& o : requested.objFiles) { + if (!haveObj.contains(o.cacheRel) + || !std::filesystem::exists(cached_obj_path(key, o.cacheRel), ec)) + probe.layoutMismatch.push_back(o.cacheRel); + } + probe.ok = probe.layoutMismatch.empty(); + return probe; +} + +bool is_cached(const CacheKey& key, const DepArtifacts& requested) { + return probe_cached(key, requested).ok; } std::expected resolve_cached(const CacheKey& key) { @@ -250,7 +327,6 @@ populate_from(const CacheKey& key, std::filesystem::create_directories(cacheObj, ec); auto projectBmi = projectTargetDir / key.bmiDirName; - auto projectObj = projectTargetDir / "obj"; for (auto& g : arts.bmiFiles) { auto from = projectBmi / g; @@ -263,15 +339,20 @@ populate_from(const CacheKey& key, "populate bmi '{}': {}", g, ec.message())); } } + // Read from `buildRel`, write at `cacheRel`. These are NOT the same path in + // general (mcpp#344): the build-dir layout partitions objects by package, + // the entry's layout is package-internal, and a source that sits outside its + // package root is re-anchored for the entry. Deriving one from the other + // here is what this split exists to prevent. for (auto& o : arts.objFiles) { - auto from = projectObj / o; - if (!std::filesystem::exists(from)) { + auto from = projectTargetDir / o.buildRel; + if (o.buildRel.empty() || !std::filesystem::exists(from)) { return std::unexpected(std::format( "expected build output missing: {}", from.string())); } - if (!copy_one(from, cached_obj_path(key, o), ec)) { + if (!copy_one(from, cached_obj_path(key, o.cacheRel), ec)) { return std::unexpected(std::format( - "populate obj '{}': {}", o, ec.message())); + "populate obj '{}': {}", o.cacheRel, ec.message())); } } @@ -289,7 +370,13 @@ populate_from(const CacheKey& key, j["tag"] = key.manifestTag; j["inputs"] = key.inputs; j["bmi"] = arts.bmiFiles; - j["obj"] = arts.objFiles; + // Only the entry-internal addresses. Recording the consumer's build path + // here is mcpp#344 in one line. + { + auto objs = nlohmann::json::array(); + for (auto& o : arts.objFiles) objs.push_back(o.cacheRel); + j["obj"] = std::move(objs); + } j["accessed"] = now_iso8601(); return write_entry(key.entryFile(), j); } diff --git a/src/bmi_cache/maintenance.cppm b/src/bmi_cache/maintenance.cppm index 482f02e7..7cd91593 100644 --- a/src/bmi_cache/maintenance.cppm +++ b/src/bmi_cache/maintenance.cppm @@ -177,9 +177,24 @@ void check_pkg_files(Entry& e, const nlohmann::json& j) { if (auto it = j.find("obj"); it != j.end() && it->is_array()) { for (auto& v : *it) { if (!v.is_string()) continue; - if (missing(e.dir / "obj" / v.get())) { + auto rel = v.get(); + // mcpp#344: an entry's obj addresses must be package-internal — + // downward, relative, no drive letter. An address that escapes the + // entry is one that was derived from some consumer's build tree, + // which is precisely the defect that made two consumers of one key + // disagree about the layout. Report it here so the invariant is + // auditable offline rather than only observable as a build that + // dies in ninja's graph phase. + if (rel.empty() || rel.starts_with("/") || rel.starts_with("..") + || rel.find(':') != std::string::npos) { e.complete = false; - e.problem = std::format("missing obj/{}", v.get()); + e.problem = std::format( + "obj address is not package-internal: '{}'", rel); + return; + } + if (missing(e.dir / "obj" / rel)) { + e.complete = false; + e.problem = std::format("missing obj/{}", rel); return; } } diff --git a/src/build/cache_key.cppm b/src/build/cache_key.cppm index f461a7e4..56691f81 100644 --- a/src/build/cache_key.cppm +++ b/src/build/cache_key.cppm @@ -64,7 +64,13 @@ export namespace mcpp::build::cache_key { // Deliberately NOT the mcpp release number: folding the whole version in // orphaned every entry on every release, including plain C object files whose // validity has nothing to do with mcpp's version. -inline constexpr int kCacheEpoch = 1; +// 2 (mcpp#344): the artifact layout changed. An entry's obj addresses are now +// package-internal instead of "the first consumer's build-dir path minus +// `obj/`", so entries written by an older mcpp describe a layout this one does +// not ask for. They would all miss anyway (probe_cached compares the REQUESTED +// artifacts), but sharing a directory between two layouts makes `cache gc`'s +// size accounting and `cache verify`'s output meaningless. +inline constexpr int kCacheEpoch = 2; // Axes A/B/C — identical for every package in one build, computed once. struct BuildAxes { diff --git a/src/build/plan.cppm b/src/build/plan.cppm index 318f1981..374ec108 100644 --- a/src/build/plan.cppm +++ b/src/build/plan.cppm @@ -42,6 +42,19 @@ struct CompileUnit { bool servedFromCache = false; std::filesystem::path cachedObject; // absolute, inside the cache std::filesystem::path cachedBmi; // absolute; empty if no module + // mcpp#344: this object's address INSIDE a global-cache entry — relative to + // `/obj/`, and a pure function of the owning package (its source's + // path relative to its own package root). Distinct from `object`, which is + // a build-dir path and therefore depends on which other packages this + // particular build contains. + // + // Empty means "no admissible cache address": the root package (never + // cached), or a dependency source that could not be anchored to anything + // machine-independent. prepare.cppm drops the WHOLE package out of the + // cache when any of its units has an empty address — a half-staged package + // mixes cached and freshly built BMIs, which is the mismatch GCC reports as + // a CRC error three edges later. + std::filesystem::path packageObjectRel; }; struct LinkUnit { @@ -116,6 +129,20 @@ struct BuildPlan { std::vector runtimeProviders; }; +// Is `p` inside one of `roots`, judged LEXICALLY? +// +// Lexical is the whole point (mcpp#344). std::filesystem::relative() runs +// weakly_canonical on both sides and therefore RESOLVES SYMLINKS, and a payload +// store whose entries are symlinks into another store is ordinary — e2e's +// _inherit_toolchain.sh builds one, and so does any CI cache that links a warm +// payload tree into a fresh MCPP_HOME. Under canonicalization those packages +// stop looking like store packages and silently drop out of the build cache. +// Both the cacheability gate and the cache-address anchor ask "where was this +// installed", which is a question about the path, not about the inode — and +// they must answer it the same way, so there is one function. +bool path_is_under_any(const std::filesystem::path& p, + const std::vector& roots); + // True if a source file defines a top-level `int main(`/`auto main(` entry, // ignoring comments and string/raw-string literals. Drives the archive-vs-inline // choice for kind="lib" dependencies (see plan.cppm). @@ -135,7 +162,14 @@ make_plan(const mcpp::manifest::Manifest& manifest, const std::filesystem::path& projectRoot, const std::filesystem::path& outputDir, const std::filesystem::path& stdBmiPath, - const std::filesystem::path& stdObjectPath); + const std::filesystem::path& stdObjectPath, + // Roots of the immutable xpkgs payload stores (there is more than + // one: the global registry, plus the project-local `.mcpp/**/data` + // roots a custom git index installs into). Used ONLY to anchor the + // cache address of a dependency source that lives outside its own + // package root (a build.mcpp OUT_DIR product). Empty is legal and + // simply makes those units uncacheable. + const std::vector& storeRoots = {}); } // namespace mcpp::build @@ -345,6 +379,37 @@ void append_unique_path(std::vector& out, // false-positive (that misfire chose archive linking for a no-main test → // gtest_main.o not pulled by MSVC lld-link → LNK1561). Heuristic but robust; // worst case is a sub-optimal archive-vs-inline choice, never a miscompile. +bool path_is_under_any(const std::filesystem::path& p, + const std::vector& roots) +{ + // Empty = unrelated (different roots/drives). ".." or a "../" prefix = + // outside. Everything else — including "." for the root itself — is in. + auto inside = [](const std::filesystem::path& a, + const std::filesystem::path& b) { + auto s = a.lexically_normal() + .lexically_relative(b.lexically_normal()) + .generic_string(); + return !s.empty() && s != ".." && !s.starts_with("../"); + }; + for (auto const& root : roots) { + if (root.empty()) continue; + if (inside(p, root)) return true; + // Retry on canonicalized paths. Lexical is the PRIMARY answer (it is + // the only one that survives a symlinked store), but it also requires + // the two paths to be spelled the same way, and mcpp's home is reached + // through more than one spelling on Windows (HOME vs USERPROFILE, drive + // letter case, 8.3 names). Both comparisons answer the same question + // under different equivalence relations, and either "yes" is sufficient + // evidence that the payload was installed into a store — so a spelling + // difference degrades to a slower build, never to a wrong one. + std::error_code e1, e2; + auto cp = std::filesystem::weakly_canonical(p, e1); + auto cr = std::filesystem::weakly_canonical(root, e2); + if (!e1 && !e2 && inside(cp, cr)) return true; + } + return false; +} + bool source_defines_main(const std::filesystem::path& src) { std::ifstream is(src); if (!is) return false; @@ -413,7 +478,8 @@ make_plan(const mcpp::manifest::Manifest& manifest, const std::filesystem::path& projectRoot, const std::filesystem::path& outputDir, const std::filesystem::path& stdBmiPath, - const std::filesystem::path& stdObjectPath) + const std::filesystem::path& stdObjectPath, + const std::vector& storeRoots) { BuildPlan plan; plan.manifest = manifest; @@ -567,43 +633,97 @@ make_plan(const mcpp::manifest::Manifest& manifest, append_unique_path(plan.runtimeLibraryDirs, tc.payloadPaths->glibcLib); } - // 1a. Detect basename collisions (both cross-package AND intra-package: - // ftxui ships dom/color.cpp + screen/color.cpp, for instance). - // For colliding files the object path gets a per-unit prefix. + // 1a. Object addressing. + // + // TWO addresses come out of one derivation here, and keeping it ONE + // derivation is the point (mcpp#344): + // + // cu.object where this build writes the object + // cu.packageObjectRel where the global cache stores it, if cacheable + // + // The rule that makes them safe: + // + // **A package's object layout may depend on that package and nothing + // else.** + // + // It used to depend on the whole build. Basename disambiguation + // (mcpp#233) was driven by a census over EVERY unit in the graph, so + // `compat.zlib`'s compress.o was `obj/compress.o` in a project that + // pulled zlib alone and `obj/compat_zlib/zlib-1.3.2/compress.o` in one + // that also pulled `compat.bzip2` (which ships its own compress.c). + // The global cache key deliberately excludes the consumer — that is + // what makes cross-project sharing sound — so both layouts landed under + // one key and whichever project ran SECOND asked the entry for a file + // the first had never written. ninja rejects that at graph load with + // "missing and no known rule to make it", before any command runs. + // + // #233 (compile edges collided), #240 (link inputs didn't follow the + // rename) and #344 are three products of the same machine: a layout + // decided by a global census. So the fix is not another place to keep + // in sync — it is to take dependencies out of the census entirely. + // + // root package (never cached): + // obj/.o (historical) + // obj///.o when the ROOT's + // own sources + // collide + // dependency package: + // obj///.o unconditionally + // + // Dependencies get no census at all. A conditional layout is exactly + // the state that generated this bug family, and all it buys is shorter + // paths; the mirrored relDir is unique by construction (two distinct + // files under one package root cannot share both relPath and basename), + // which is what the L1b assertion below still backstops. // - // mcpp#233: the prefix used to be derived from just the file's - // IMMEDIATE parent directory name (`_`), which - // itself collides whenever two files share a parent dir NAME at - // different depths — e.g. a/src/util.cpp and b/src/util.cpp both - // fold to `_src/util.o`, and ninja rejects the plan with - // "multiple rules generate obj/...". The prefix now mirrors the - // unit's FULL relative directory instead (SourceUnit::relPath, set - // by the scanner against the unit's own package root), which is - // unique by construction: two distinct files under one package - // root can never share both relPath and basename. Non-colliding - // files keep the pre-existing flat `obj/` layout untouched - // (back-compat for the overwhelmingly common single-file-per- - // basename project). + // The root keeps its flat layout because it is never cached and because + // `obj/main.o` is what every project has looked like since 0.0.1. + // Its census now spans only root-owned units, which is both correct + // (a dependency can no longer force the root to disambiguate) and + // sufficient (dependencies live in their own subtrees). + + // Owning package of a source. Longest matching root wins: package roots + // nest (a workspace member lives under the workspace root) and the first + // match would file the member's sources under the outer package. Index 0 is + // the root project; `packages.size()` means "outside every known root", + // which is treated as root-owned and never cached. + auto owner_of = [&](const std::filesystem::path& src) -> std::size_t { + std::size_t best = 0; + std::size_t bestLen = 0; + bool found = false; + for (std::size_t p = 0; p < packages.size(); ++p) { + std::error_code ec; + auto rel = std::filesystem::relative(src, packages[p].root, ec); + if (ec || rel.empty()) continue; + if (rel.generic_string().starts_with("..")) continue; + auto len = packages[p].root.generic_string().size(); + if (!found || len > bestLen) { best = p; bestLen = len; found = true; } + } + return found ? best : 0; + }; + std::set scannedSources; - std::map basenameCount; + std::map rootBasenameCount; + std::vector unitOwner(graph.units.size(), 0); for (auto idx : topoOrder) { - basenameCount[object_filename_for(graph.units[idx].path, objExt)]++; + unitOwner[idx] = owner_of(graph.units[idx].path); scannedSources.insert(graph.units[idx].path); + if (unitOwner[idx] == 0) + rootBasenameCount[object_filename_for(graph.units[idx].path, objExt)]++; } // mcpp#240: entry `main` sources are synthesized into compile units later // (during link assembly), NOT part of topoOrder — but they still occupy an // object path and must share ONE disambiguation census with everything - // else. Count each root target's entry that isn't already scanned (a globbed - // main IS scanned, so counting it again would falsely disambiguate the - // common single-binary project). This makes "consumer main not globbed + - // dependency ships a same-named main" disambiguate correctly too. + // else root-owned. Count each root target's entry that isn't already + // scanned (a globbed main IS scanned, so counting it again would falsely + // disambiguate the common single-binary project). for (auto& t : manifest.targets) { if (t.main.empty()) continue; if (t.kind != mcpp::manifest::Target::Binary && t.kind != mcpp::manifest::Target::TestBinary) continue; auto entry = projectRoot / t.main; if (scannedSources.contains(entry)) continue; - basenameCount[object_filename_for(entry, objExt)]++; + rootBasenameCount[object_filename_for(entry, objExt)]++; } auto sanitize = [](const std::string& s) { std::string out; out.reserve(s.size()); @@ -645,18 +765,64 @@ make_plan(const mcpp::manifest::Manifest& manifest, return pkg.empty() ? safe : std::filesystem::path(sanitize(pkg)) / safe; }; - // mcpp#233/#240: the single source of truth for a compile unit's object - // path — scanned units AND the synthesized entry main go through here, so - // the link input can never diverge from the compile edge. + // mcpp#233/#240/#344: the single source of truth for a compile unit's + // object addresses — scanned units AND the synthesized entry main go + // through here, so neither the link input nor the cache address can + // diverge from the compile edge. + struct ObjectAddress { + std::filesystem::path object; // relative to outputDir + std::filesystem::path packageRel; // inside a cache entry; empty = uncacheable + }; auto object_for = [&](const std::filesystem::path& src, const std::string& pkg, - const std::filesystem::path& relPath) - -> std::filesystem::path { + const std::filesystem::path& relPath, + std::size_t owner) -> ObjectAddress + { const auto fname = object_filename_for(src, objExt); - if (basenameCount[fname] > 1) - return std::filesystem::path("obj") - / safe_object_prefix(pkg, relPath.parent_path()) / fname; - return std::filesystem::path("obj") / fname; + if (owner == 0) { + // Root project: never cached, historical layout preserved. + if (rootBasenameCount[fname] > 1) + return { std::filesystem::path("obj") + / safe_object_prefix(pkg, relPath.parent_path()) / fname, + {} }; + return { std::filesystem::path("obj") / fname, {} }; + } + + auto slug = sanitize(pkg.empty() + ? qualified_package_name(packages[owner].manifest) + : pkg); + auto mirrored = safe_object_prefix({}, relPath.parent_path()) / fname; + + ObjectAddress addr; + addr.object = std::filesystem::path("obj") / slug / mirrored; + + // The cache address additionally has to be MACHINE-independent: another + // machine computes the same key and reads the same entry. A relPath + // that stays inside the package root already is. One that escapes (a + // build.mcpp OUT_DIR product living beside the payload) is re-anchored + // at the xpkgs store root — the same ``-relative trick + // cache_key.cppm uses for include dirs — and when even that fails the + // unit gets no address at all rather than one carrying this machine's + // absolute paths. + auto rels = relPath.generic_string(); + if (!relPath.empty() && !relPath.is_absolute() && !rels.starts_with("..")) { + addr.packageRel = mirrored; + } else { + // Lexically — see path_is_under_any: a store built out of symlinks + // is ordinary, and canonicalizing here would answer a different + // question than the cacheability gate does. + auto norm = src.lexically_normal(); + for (auto const& storeRoot : storeRoots) { + if (storeRoot.empty()) continue; + auto sr = norm.lexically_relative(storeRoot.lexically_normal()); + auto srs = sr.generic_string(); + if (srs.empty() || srs == ".." || srs.starts_with("../")) continue; + addr.packageRel = std::filesystem::path("__store") + / safe_object_prefix({}, sr.parent_path()) / fname; + break; + } + } + return addr; }; // 1. Compile units in topological order @@ -670,7 +836,11 @@ make_plan(const mcpp::manifest::Manifest& manifest, cu.packageCflags = u.packageCflags; cu.packageCxxflags = u.packageCxxflags; cu.packageAsmflags = u.packageAsmflags; - cu.object = object_for(u.path, u.packageName, u.relPath); + { + auto addr = object_for(u.path, u.packageName, u.relPath, unitOwner[idx]); + cu.object = std::move(addr.object); + cu.packageObjectRel = std::move(addr.packageRel); + } if (u.provides) { cu.providesModule = u.provides->logicalName; } @@ -1005,9 +1175,12 @@ make_plan(const mcpp::manifest::Manifest& manifest, } } if (!already) { + // Entry mains belong to the root project (`t.main` is resolved + // against projectRoot), which is owner 0 and never cached. main_cu.object = object_for( main_cu.source, main_cu.packageName, - std::filesystem::relative(main_cu.source, projectRoot)); + std::filesystem::relative(main_cu.source, projectRoot), + /*owner=*/0).object; plan.compileUnits.push_back(main_cu); entryObject = main_cu.object; } diff --git a/src/build/prepare.cppm b/src/build/prepare.cppm index 0471637e..ea67c6ba 100644 --- a/src/build/prepare.cppm +++ b/src/build/prepare.cppm @@ -4204,9 +4204,25 @@ prepare_build(bool print_fingerprint, ctx.outputDir = target_dir(*tc, fp, *root); ctx.stdBmi = stdBmiPath; ctx.stdObject = stdObjectPath; + // Every directory a package payload may legitimately have been INSTALLED + // into. There is more than one: the global registry, plus the two + // project-local data roots a custom git index installs into + // (`config::project_xlings_data_roots`). make_plan uses these to anchor the + // cache address of a dependency source that lives outside its own package + // root, and the cacheability gate below uses the same list to decide + // whether a package's sources really came from a store. ONE definition, + // two uses — deriving the same fact twice is how the object layout and the + // cache key drifted apart in the first place (#344). + const auto storeRoots = [&]() -> std::vector { + std::vector roots; + if (auto c = get_cfg()) roots.push_back((*c)->xlingsHome() / "data" / "xpkgs"); + for (auto& d : mcpp::config::project_xlings_data_roots(*root)) + roots.push_back(d / "xpkgs"); + return roots; + }(); auto planResult = mcpp::build::make_plan(*m, *tc, fp, scan.graph, report.topoOrder, packages, *root, ctx.outputDir, - stdBmiPath, stdObjectPath); + stdBmiPath, stdObjectPath, storeRoots); if (!planResult) return std::unexpected(planResult.error()); ctx.plan = std::move(*planResult); ctx.plan.stdCompatBmiPath = stdCompatBmiPath; @@ -4305,27 +4321,15 @@ prepare_build(bool print_fingerprint, if (cfg2 && ctx.cacheMode == CacheMode::Global) { std::error_code mkEc; std::filesystem::create_directories(ctx.outputDir, mkEc); - auto usable_object_rel = [](const std::filesystem::path& rel) - -> std::optional - { - auto s = rel.generic_string(); - if (s.empty() || s == "." || s == ".." || s.starts_with("../")) { - return std::nullopt; - } - return s; - }; - auto object_cache_path = [&](const std::filesystem::path& objectPath) { - if (objectPath.is_absolute()) { - if (auto s = usable_object_rel( - objectPath.lexically_relative(ctx.outputDir / "obj"))) { - return *s; - } - } - if (auto s = usable_object_rel(objectPath.lexically_relative("obj"))) { - return *s; - } - return objectPath.filename().generic_string(); - }; + + // NOTE (mcpp#344): there is deliberately no local "derive the entry + // address from the object path" helper here any more. There used to be + // one, and it was the SECOND derivation of a fact plan.cppm already + // owns — it stripped `obj/` off the consumer's build path, so the entry + // layout followed the consumer's package mix while the key did not. + // `CompileUnit::packageObjectRel` is now the only answer to "where does + // this object live inside a cache entry", and it is computed in exactly + // one place. Do not reintroduce a second one. // ── Per-package keys, bottom-up ────────────────────────────────── // Axes A/B/C are whole-graph, so they are computed once. Axes D/E are @@ -4335,7 +4339,6 @@ prepare_build(bool print_fingerprint, // explicit in-progress guard so a cycle that slipped past validation // fails loudly instead of recursing until the stack dies. namespace ck = mcpp::build::cache_key; - const auto storeRoot = (*cfg2)->xlingsHome() / "data" / "xpkgs"; auto axes = ck::build_axes( *tc, *m, stdFlagAndDialect, mcpp::toolchain::cppfly::effective_dialect_flags( @@ -4419,7 +4422,13 @@ prepare_build(bool print_fingerprint, packages[idx].manifest.package.name); } if (pa.version.empty()) pa.version = packages[idx].manifest.package.version; - ck::fill_package_config(pa, packages[idx], storeRoot); + // The GLOBAL registry root — index 0 by construction above. Include + // dirs are relativized against it so a key survives a different + // MCPP_HOME; a project-local payload falls back to the `` + // prefix inside fill_package_config and is equally stable. + ck::fill_package_config(pa, packages[idx], + storeRoots.empty() ? std::filesystem::path{} + : storeRoots.front()); pa.sources = pkgSources[idx]; const bool selfIsIndex = idx > 0 && idx - 1 < dep_cache_identities.size() @@ -4474,6 +4483,36 @@ prepare_build(bool print_fingerprint, if (!depIdent || depIdent->sourceKind != "version") continue; // ...and neither may anything it was built against be local. if (localTaint[i]) continue; + // ...and the package's sources must ACTUALLY be in the immutable + // store, not merely labelled as coming from it. + // + // The rule stated three paragraphs up is about provenance on disk; + // `sourceKind` is a label recorded at resolution time, which is a + // weaker proxy — and there is already a case where the two + // disagree. Multi-version mangling re-anchors a consumer package's + // root at `/target/.mangled//__self__` and REWRITES + // its sources (module/import declarations renamed) while leaving + // `sourceKind == "version"` and `localTaint` clear. Nothing about + // that copy is immutable or shareable. It stays out of the cache + // today only because axis F happens to fold in the mangled + // secondary's differing key — one axis away from serving objects + // compiled against renamed modules, which is the silent-wrong-`.o` + // failure this gate exists to prevent. + // + // Judge the location, not the label. + // + // LEXICALLY, not via std::filesystem::relative. `relative()` runs + // weakly_canonical on both sides, which RESOLVES SYMLINKS — and a + // store whose entries are symlinks into another store is ordinary + // (tests/e2e/_inherit_toolchain.sh builds exactly that, and so do + // CI caches that link a warm payload tree into a fresh + // MCPP_HOME). Canonicalizing turns + // `/registry/data/xpkgs/` into wherever the link points + // and the package stops looking like a store package at all. The + // question here is where the payload was INSTALLED, which is a + // statement about the path, not about the inode. + if (!mcpp::build::path_is_under_any(pkgRoot.root, storeRoots)) + continue; const auto& depName = depIdent->packageName; const auto& depVer = depIdent->version.empty() @@ -4498,6 +4537,7 @@ prepare_build(bool print_fingerprint, // must stop being compile edges. mcpp::bmi_cache::DepArtifacts arts; std::vector unitIdx; + bool addressable = true; for (std::size_t u = 0; u < ctx.plan.compileUnits.size(); ++u) { auto& cu = ctx.plan.compileUnits[u]; std::error_code ec; @@ -4506,6 +4546,15 @@ prepare_build(bool print_fingerprint, auto rels = rel.string(); if (rels.starts_with("..")) continue; // not under depRoot + // ALL OR NOTHING. A unit plan.cppm could not give a + // machine-independent entry address to takes its whole package + // out of the cache, rather than leaving the package half + // staged. Mixing cached and freshly built artifacts within one + // package is the case GCC reports as a BMI CRC mismatch in a + // consumer three edges away, which is far harder to read than + // one extra compile. + if (cu.packageObjectRel.empty()) { addressable = false; break; } + if (cu.providesModule) { std::string bmi; for (char c : *cu.providesModule) @@ -4513,11 +4562,18 @@ prepare_build(bool print_fingerprint, bmi += std::string(bmiT.bmiExt); arts.bmiFiles.push_back(std::move(bmi)); } - arts.objFiles.push_back(object_cache_path(cu.object)); + arts.objFiles.push_back({cu.packageObjectRel.generic_string(), + cu.object}); unitIdx.push_back(u); } - - if (mcpp::bmi_cache::is_cached(key)) { + if (!addressable) continue; + + // Validate the entry against THIS build's artifact list, not + // against the entry's own (mcpp#344). Anything short of a full + // match is a miss — never a failure: the stage edges below are + // simply not emitted and the units compile normally. + auto probe = mcpp::bmi_cache::probe_cached(key, arts); + if (probe.ok) { // Mark the units. The backend turns each into a stage_file // edge; nothing is copied here. Copying behind ninja's back is // exactly what made the old cache a no-op: the staged file was @@ -4528,7 +4584,7 @@ prepare_build(bool print_fingerprint, auto& cu = ctx.plan.compileUnits[u]; cu.servedFromCache = true; cu.cachedObject = mcpp::bmi_cache::cached_obj_path( - key, object_cache_path(cu.object)); + key, cu.packageObjectRel.generic_string()); if (cu.providesModule) { std::string bmi; for (char c : *cu.providesModule) @@ -4541,6 +4597,22 @@ prepare_build(bool print_fingerprint, ctx.cachedDeps.push_back({depName, depVer, unitIdx.size()}); continue; // no populate task; it is already cached } + // A valid entry that does not hold what we asked for means the + // entry and this build disagree about the layout under one key. + // After #344 that is unreachable; say so out loud if it ever + // happens again, because the alternative presentation is "the + // cache silently never hits", and a cache that lies about its own + // effectiveness went unnoticed for three months once already. + if (!probe.layoutMismatch.empty()) { + mcpp::ui::warning(std::format( + "build cache entry for {}@{} [{}] does not contain the " + "artifacts this build needs ({} of {} missing, e.g. `{}`); " + "treating it as a miss. Run `mcpp cache verify` for details.", + depName, depVer, key.keyHex, + probe.layoutMismatch.size(), + arts.bmiFiles.size() + arts.objFiles.size(), + probe.layoutMismatch.front())); + } ctx.depsToPopulate.push_back({ std::move(key), std::move(arts) }); } } diff --git a/src/toolchain/fingerprint.cppm b/src/toolchain/fingerprint.cppm index 91222c64..47f42d15 100644 --- a/src/toolchain/fingerprint.cppm +++ b/src/toolchain/fingerprint.cppm @@ -18,7 +18,7 @@ import mcpp.toolchain.detect; export namespace mcpp::toolchain { -inline constexpr std::string_view MCPP_VERSION = "2026.8.3.3"; +inline constexpr std::string_view MCPP_VERSION = "2026.8.3.4"; struct FingerprintInputs { Toolchain toolchain; diff --git a/tests/e2e/123_same_named_main_across_dep.sh b/tests/e2e/123_same_named_main_across_dep.sh index fa2d2de8..977a3905 100755 --- a/tests/e2e/123_same_named_main_across_dep.sh +++ b/tests/e2e/123_same_named_main_across_dep.sh @@ -65,15 +65,37 @@ cd consumer ninja_file="$(find target -name build.ninja | head -1)" [[ -n "$ninja_file" ]] || { echo "no build.ninja generated"; exit 1; } -# The link edge must NOT reference a bare obj/main.o (the stale, unproduced -# flat path). It must reference the consumer main's real, disambiguated object. -link_line="$(grep -E 'bin/consumer *:' "$ninja_file")" -if echo "$link_line" | grep -qE '(^| )obj/main\.o( |$)'; then - echo "FAIL: link still references stale flat obj/main.o" - echo "$link_line" - cat "$ninja_file" +# THE invariant: every object the link edge names must be PRODUCED by an edge in +# the same graph. #240's bug was a link input that no edge produced. +# +# This deliberately does not assert *which* path the consumer's main lands on. +# The original assertion ("must not be the flat obj/main.o") encoded the shape of +# the first fix rather than the property: back then the consumer's main WAS +# renamed, because disambiguation was decided by a census over the whole build +# dir, so a dependency's same-named file dragged the consumer along with it. +# Since mcpp#344 a dependency's objects live under obj//, cross-package +# collisions cannot happen, and the consumer's own main correctly stays flat. +# Both layouts satisfy #240; only "produced by some edge" distinguishes a fixed +# tree from a broken one. +link_line="$(grep -E '^build bin/consumer *:' "$ninja_file")" +[[ -n "$link_line" ]] || { echo "FAIL: no link edge for bin/consumer"; cat "$ninja_file"; exit 1; } + +for obj in $(echo "$link_line" | sed -E 's/^build bin\/consumer *: *cxx_link //' | tr ' ' '\n' | grep -E '\.o$'); do + grep -qE "^build ${obj//\//\\/} *:" "$ninja_file" || { + echo "FAIL: link input '$obj' is not produced by any edge" + echo "$link_line" + cat "$ninja_file" + exit 1 + } +done + +# And the dependency's same-named source must have gotten its own object rather +# than silently overwriting the consumer's. +grep -qE '^build obj/mydep/.*main\.o *:' "$ninja_file" || { + echo "FAIL: the dependency's src/main.cpp has no object of its own" + grep -n 'main\.o' "$ninja_file" exit 1 -fi +} out="$("$MCPP" run 2>&1 | tail -1)" [[ "$out" == "val=41" ]] || { echo "unexpected output: $out"; exit 1; } diff --git a/tests/e2e/184_build_cache_object_layout.sh b/tests/e2e/184_build_cache_object_layout.sh new file mode 100755 index 00000000..dabdbb92 --- /dev/null +++ b/tests/e2e/184_build_cache_object_layout.sh @@ -0,0 +1,193 @@ +#!/usr/bin/env bash +# requires: gcc fresh-sandbox +# 184_build_cache_object_layout.sh — mcpp#344. +# +# A dependency's objects live in the global cache under a key that deliberately +# excludes the consumer. So the LAYOUT of those objects inside the entry must +# also exclude the consumer — otherwise two projects that share a key disagree +# about where the files are, and whichever runs SECOND asks the entry for a path +# the first never wrote. +# +# That is what happened. Basename disambiguation (#233) was decided by a census +# over every unit in the build directory, so `lib-a`'s `compress.c` compiled to +# obj/compress.o in a project that pulls lib-a alone +# obj/lib_a/src/compress.o in one that also pulls lib-b +# (lib-b ships its own compress.c) +# with the SAME cache key. The second project's build died in ninja's GRAPH +# phase — before a single command ran — with +# ninja: error: '/…/obj/compress.o', needed by 'obj/compress.o', +# missing and no known rule to make it +# one line after the CLI printed "Cached lib-a (N units)". +# +# The test runs both orderings, because the defect is symmetric: whichever +# project is second is the one that breaks. +# +# TWO assertions per direction, and the second one is not optional: +# 1. the build succeeds +# 2. the reused dependency has ZERO compile edges +# Without (2) this test would pass on a build that merely degraded every hit to +# a miss — which is a real regression (the cache silently stops paying for +# itself) that "it built fine" cannot see. +set -e + +TMP=$(mktemp -d) +trap "rm -rf $TMP" EXIT + +export MCPP_HOME="$TMP/mcpp-home" +source "$(dirname "$0")/_inherit_toolchain.sh" + +INDEX_DIR="$TMP/local-index" +mkdir -p "$INDEX_DIR/pkgs/l" + +# ── two library packages that COLLIDE on a source basename ─────────────────── +# Neither package can avoid this: upstream zlib and upstream bzip2 both ship a +# file called compress.c, and a descriptor has no field that controls object +# paths. The collision has to be handled by mcpp or not at all. +make_descriptor() { # $1 = package name + cat > "$INDEX_DIR/pkgs/l/$1.lua" < ` +make_project() { + local dir="$1" name="$2" withb="$3" + local proj="$TMP/$dir" + mkdir -p "$proj/src" + + local payload="$proj/.mcpp/.xlings/data/xpkgs" + mkdir -p "$payload/local-dev.lib-a/1.0.0/src" + cat > "$payload/local-dev.lib-a/1.0.0/src/compress.c" <<'EOF' +int a_value(void) { return 40; } +EOF + local deps='"local-dev.lib-a" = "1.0.0"' + local extern='extern "C" int a_value(void);' + local expr='a_value() + 2' + if [[ "$withb" == "yes" ]]; then + mkdir -p "$payload/local-dev.lib-b/1.0.0/src" + cat > "$payload/local-dev.lib-b/1.0.0/src/compress.c" <<'EOF' +int b_value(void) { return 1; } +EOF + deps="$deps"$'\n' + deps="$deps"'"local-dev.lib-b" = "1.0.0"' + extern="$extern"$'\n''extern "C" int b_value(void);' + expr='a_value() + b_value() + 1' + fi + + cat > "$proj/src/main.cpp" < "$proj/mcpp.toml" < build.log 2>&1 || { + echo "FAIL: $dir failed to build ($mode)" + cat build.log + exit 1 + } + local nj; nj="$(find_ninja "$TMP/$dir")" + [[ -n "$nj" ]] || { echo "FAIL: $dir has no build.ninja"; exit 1; } + + if [[ "$mode" == "reuses" ]]; then + local edges; edges="$(a_compile_edges "$nj")" + if [[ "$edges" != "0" ]]; then + echo "FAIL: $dir recompiled lib-a ($edges compile edges) instead of" + echo " reusing the entry the other project populated." + echo " A hit that silently became a miss is still a regression:" + echo " the cache stops paying for itself with no signal." + grep -nE ': (cxx_module|cxx_object|c_object|cxx_scan) .*lib-a' "$nj" | head + exit 1 + fi + local staged; staged="$(a_stage_edges "$nj")" + [[ "$staged" -gt 0 ]] || { + echo "FAIL: $dir has neither compile nor stage edges for lib-a" + grep -n 'lib-a' "$nj" | head + exit 1 + } + fi + + ./target/*/*/bin/"$name" > run.log 2>&1 || { cat run.log; exit 1; } + grep -q '^42$' run.log || { + echo "FAIL: $dir produced the wrong answer — staged the wrong object?" + cat run.log + exit 1 + } +} + +make_project both both yes +make_project onlya onlya no + +# ── direction A: the project with BOTH libraries runs first ────────────────── +# It disambiguates (two compress.c in one build dir), so the entry is written +# with a nested layout; `onlya` then asks for the flat one. +rm -rf "$MCPP_HOME/build-cache" +check_project both both cold +check_project onlya onlya reuses + +# ── direction B: the single-library project runs first (fully symmetric) ───── +# Now the entry is written flat and `both` asks for the nested one. Before the +# fix this direction failed just as hard, with the paths swapped. +rm -rf "$MCPP_HOME/build-cache" +check_project onlya onlya cold +check_project both both reuses + +# ── the entry's own addresses must be package-internal ─────────────────────── +# `cache verify` reports an obj address that escaped its entry — the offline +# form of the same invariant, so a recurrence is auditable without reproducing +# a two-project build. +cd "$TMP/both" +"$MCPP" cache verify > verify.log 2>&1 || { + echo "FAIL: cache verify reported incomplete entries" + cat verify.log + exit 1 +} + +echo "OK" diff --git a/tests/unit/test_bmi_cache.cpp b/tests/unit/test_bmi_cache.cpp index 6fe2c517..c4716e30 100644 --- a/tests/unit/test_bmi_cache.cpp +++ b/tests/unit/test_bmi_cache.cpp @@ -68,8 +68,12 @@ nlohmann::json readJson(const std::filesystem::path& p) { return j; } +// `cacheRel` is the entry-internal address, `buildRel` is where this "build" +// produced the file (relative to the project target dir). They are separate on +// purpose — see mcpp#344. DepArtifacts oneOfEach() { - return DepArtifacts{ .bmiFiles = {"lib.gcm"}, .objFiles = {"lib.m.o"} }; + return DepArtifacts{ .bmiFiles = {"lib.gcm"}, + .objFiles = {{"lib.m.o", "obj/lib.m.o"}} }; } void seedProject(const std::filesystem::path& project) { @@ -101,7 +105,7 @@ TEST(BmiCache, EntryDirCarriesTheLayoutVersion) { TEST(BmiCache, IsCachedFalseWhenEntryMissing) { Tmp t; - EXPECT_FALSE(is_cached(makeKey(t.path))); + EXPECT_FALSE(is_cached(makeKey(t.path), oneOfEach())); } TEST(BmiCache, PopulateWritesSelfDescribingEntry) { @@ -117,7 +121,7 @@ TEST(BmiCache, PopulateWritesSelfDescribingEntry) { ASSERT_TRUE(std::filesystem::exists(k.entryFile())); EXPECT_TRUE(std::filesystem::exists(k.bmiDir() / "lib.gcm")); EXPECT_TRUE(std::filesystem::exists(k.objDir() / "lib.m.o")); - EXPECT_TRUE(is_cached(k)); + EXPECT_TRUE(is_cached(k, oneOfEach())); // The entry has to carry the key AND the full inputs: a hit is validated // field by field, so a cache whose entries only listed files could never be @@ -141,10 +145,10 @@ TEST(BmiCache, IsCachedFalseWhenRecordedInputsDiffer) { auto written = makeKey(home, "samekey00000000", "base"); ASSERT_TRUE(populate_from(written, project, oneOfEach())); - EXPECT_TRUE(is_cached(written)); + EXPECT_TRUE(is_cached(written, oneOfEach())); auto probed = makeKey(home, "samekey00000000", "different-flag"); - EXPECT_FALSE(is_cached(probed)); + EXPECT_FALSE(is_cached(probed, oneOfEach())); } // An entry written by an older mcpp may carry extra keys, but every field the @@ -162,7 +166,7 @@ TEST(BmiCache, IsCachedFalseWhenARequiredInputFieldIsAbsent) { j["inputs"].erase("profile"); std::ofstream(k.entryFile()) << j.dump(2); - EXPECT_FALSE(is_cached(k)); + EXPECT_FALSE(is_cached(k, oneOfEach())); } TEST(BmiCache, IsCachedFalseWhenSchemaDiffers) { @@ -178,7 +182,7 @@ TEST(BmiCache, IsCachedFalseWhenSchemaDiffers) { j["schema"] = kEntrySchema + 1; std::ofstream(k.entryFile()) << j.dump(2); - EXPECT_FALSE(is_cached(k)); + EXPECT_FALSE(is_cached(k, oneOfEach())); } TEST(BmiCache, IsCachedFalseWhenSentinelExistsButFileMissing) { @@ -189,10 +193,10 @@ TEST(BmiCache, IsCachedFalseWhenSentinelExistsButFileMissing) { auto k = makeKey(home); ASSERT_TRUE(populate_from(k, project, oneOfEach())); - ASSERT_TRUE(is_cached(k)); + ASSERT_TRUE(is_cached(k, oneOfEach())); std::filesystem::remove(k.objDir() / "lib.m.o"); - EXPECT_FALSE(is_cached(k)); + EXPECT_FALSE(is_cached(k, oneOfEach())); } TEST(BmiCache, ResolveCachedReportsTheArtifactsWithoutCopying) { @@ -207,7 +211,11 @@ TEST(BmiCache, ResolveCachedReportsTheArtifactsWithoutCopying) { auto arts = resolve_cached(k); ASSERT_TRUE(arts) << arts.error(); EXPECT_EQ(arts->bmiFiles, std::vector{"lib.gcm"}); - EXPECT_EQ(arts->objFiles, std::vector{"lib.m.o"}); + ASSERT_EQ(arts->objFiles.size(), 1u); + EXPECT_EQ(arts->objFiles[0].cacheRel, "lib.m.o"); + // Read-back never yields a build path: the entry does not record one, which + // is the whole point of mcpp#344. + EXPECT_TRUE(arts->objFiles[0].buildRel.empty()); // resolve_cached must not write into a project dir. Staging is a ninja edge // now: copying artifacts in from outside the graph is exactly what made the @@ -232,10 +240,76 @@ TEST(BmiCache, PopulateHandlesNestedObjectPaths) { writeFile(project / "obj" / "pkg_zlib" / "zlib-1.3" / "compress.o", "NESTED"); auto k = makeKey(home); - DepArtifacts arts { .objFiles = {"pkg_zlib/zlib-1.3/compress.o"} }; + DepArtifacts arts { + .objFiles = {{"zlib-1.3/compress.o", "obj/pkg_zlib/zlib-1.3/compress.o"}} }; ASSERT_TRUE(populate_from(k, project, arts)); - EXPECT_EQ(readFile(k.objDir() / "pkg_zlib" / "zlib-1.3" / "compress.o"), "NESTED"); - EXPECT_TRUE(is_cached(k)); + // Stored at the ENTRY address, read from the BUILD path. The entry knows + // nothing about the `pkg_zlib/` partition, which is consumer-side. + EXPECT_EQ(readFile(k.objDir() / "zlib-1.3" / "compress.o"), "NESTED"); + EXPECT_TRUE(is_cached(k, arts)); + auto j = readJson(k.entryFile()); + EXPECT_EQ(j["obj"][0].get(), "zlib-1.3/compress.o"); +} + +// mcpp#344, the load-bearing case. Two consumers of ONE key may place the same +// object at different build-dir paths (#233's basename disambiguation fires on +// a census over the whole build dir, which varies with the consumer's package +// mix). The entry is written by whoever runs first; whoever runs second must +// get a clean MISS, with the divergence named — never a hit that goes on to +// stage a file the entry never had, which is what ninja reported as +// "missing and no known rule to make it" at graph load. +TEST(BmiCache, ProbeMissesWhenTheEntryDoesNotHoldTheRequestedArtifacts) { + Tmp t; + auto home = t.path / "home"; + auto project = t.path / "proj" / "target"; + writeFile(project / "obj" / "compress.o", "FLAT"); + + auto k = makeKey(home); + // The first consumer populated the entry under a DIFFERENT internal + // address than the one this build is going to ask for. + DepArtifacts written { .objFiles = {{"other/compress.o", "obj/compress.o"}} }; + ASSERT_TRUE(populate_from(k, project, written)); + + DepArtifacts wanted { .objFiles = {{"zlib-1.3/compress.o", "obj/compress.o"}} }; + auto probe = probe_cached(k, wanted); + EXPECT_FALSE(probe.ok); + ASSERT_EQ(probe.layoutMismatch.size(), 1u); + EXPECT_EQ(probe.layoutMismatch[0], "zlib-1.3/compress.o"); +} + +// The same asymmetry on the read side: an entry that lists a file it does not +// have on disk must not be reported as a layout divergence OR as a hit. +TEST(BmiCache, ProbeMissesWhenARequestedFileIsListedButAbsent) { + Tmp t; + auto home = t.path / "home"; + auto project = t.path / "proj" / "target"; + seedProject(project); + + auto k = makeKey(home); + ASSERT_TRUE(populate_from(k, project, oneOfEach())); + std::filesystem::remove(k.objDir() / "lib.m.o"); + + auto probe = probe_cached(k, oneOfEach()); + EXPECT_FALSE(probe.ok); + ASSERT_EQ(probe.layoutMismatch.size(), 1u); + EXPECT_EQ(probe.layoutMismatch[0], "lib.m.o"); +} + +// An entry whose own validation fails (wrong inputs) is an ordinary miss and +// carries NO layout complaint — the caller warns on layout divergence only, and +// warning on every routine miss would make the signal worthless. +TEST(BmiCache, ProbeReportsNoLayoutMismatchWhenTheEntryItselfIsInvalid) { + Tmp t; + auto home = t.path / "home"; + auto project = t.path / "proj" / "target"; + seedProject(project); + + ASSERT_TRUE(populate_from(makeKey(home, "samekey00000000", "base"), + project, oneOfEach())); + auto probe = probe_cached(makeKey(home, "samekey00000000", "other"), + oneOfEach()); + EXPECT_FALSE(probe.ok); + EXPECT_TRUE(probe.layoutMismatch.empty()); } // touch_accessed is what makes `cache gc` an LRU rather than "drop what was @@ -266,7 +340,7 @@ TEST(BmiCache, TouchAccessedMovesTheStampAndNotTheArtifacts) { << "touch must not reset the creation stamp"; EXPECT_EQ(std::filesystem::last_write_time(k.objDir() / "lib.m.o"), objTime0); EXPECT_EQ(std::filesystem::last_write_time(k.bmiDir() / "lib.gcm"), bmiTime0); - EXPECT_TRUE(is_cached(k)) << "touching must not invalidate the entry"; + EXPECT_TRUE(is_cached(k, oneOfEach())) << "touching must not invalidate the entry"; } TEST(BmiCache, RepopulatePreservesCreatedStamp) { @@ -290,7 +364,7 @@ TEST(BmiCache, PopulateFailsIfBuildOutputMissing) { auto home = t.path / "home"; auto project = t.path / "proj" / "target"; std::filesystem::create_directories(project / "gcm.cache"); - DepArtifacts arts { .bmiFiles = {"missing.gcm"}, .objFiles = {} }; + DepArtifacts arts { .bmiFiles = {"missing.gcm"} }; auto k = makeKey(home); auto pop = populate_from(k, project, arts); EXPECT_FALSE(pop); @@ -311,8 +385,8 @@ TEST(BmiCache, DifferentKeysAreIndependentEntries) { ASSERT_TRUE(populate_from(a, project, oneOfEach())); ASSERT_TRUE(populate_from(b, project, oneOfEach())); EXPECT_NE(a.dir(), b.dir()); - EXPECT_TRUE(is_cached(a)); - EXPECT_TRUE(is_cached(b)); + EXPECT_TRUE(is_cached(a, oneOfEach())); + EXPECT_TRUE(is_cached(b, oneOfEach())); } #if !defined(_WIN32) diff --git a/tests/unit/test_object_address.cpp b/tests/unit/test_object_address.cpp new file mode 100644 index 00000000..7dab730e --- /dev/null +++ b/tests/unit/test_object_address.cpp @@ -0,0 +1,252 @@ +// mcpp#344 — a package's object addresses must be a function of that package +// alone. +// +// This is the machine-checkable form of the invariant the global build cache +// depends on. The cache key deliberately excludes the consuming project (that +// is what makes an entry shareable across projects), so if a dependency's +// object layout can shift when the consumer pulls in some UNRELATED package, +// two consumers write and read incompatible layouts under one key. #344 was +// exactly that: `compat.zlib`'s compress.o was `obj/compress.o` alone and +// `obj/compat_zlib/zlib-1.3.2/compress.o` alongside `compat.bzip2` (which ships +// its own compress.c), because basename disambiguation (#233) was driven by a +// census over every unit in the build. +// +// The test therefore builds the SAME dependency twice — once alone, once beside +// a package engineered to collide with it — and demands byte-identical +// addresses. It fails on the pre-#344 tree. + +#include + +import std; +import mcpp.build.plan; +import mcpp.manifest; +import mcpp.modgraph.graph; +import mcpp.modgraph.scanner; +import mcpp.toolchain.model; + +using namespace mcpp::build; + +namespace { + +struct Tmp { + std::filesystem::path path; + Tmp() { + path = std::filesystem::temp_directory_path() + / std::format("mcpp_obj_addr_{}", std::random_device{}()); + std::filesystem::create_directories(path); + } + ~Tmp() { + std::error_code ec; + std::filesystem::remove_all(path, ec); + } +}; + +void touchFile(const std::filesystem::path& p) { + std::filesystem::create_directories(p.parent_path()); + std::ofstream(p) << "/* test */\n"; +} + +mcpp::toolchain::Toolchain gccLike() { + mcpp::toolchain::Toolchain tc; + tc.compiler = mcpp::toolchain::CompilerId::GCC; + tc.version = "16.1.0"; + tc.binaryPath = "/usr/bin/g++"; + tc.targetTriple = "x86_64-linux-gnu"; + return tc; +} + +mcpp::modgraph::PackageRoot makePackage(const std::filesystem::path& root, + std::string_view name) { + mcpp::modgraph::PackageRoot p; + p.root = root; + p.manifest.package.name = std::string(name); + p.manifest.package.version = "1.0.0"; + p.manifest.package.standard = "c++23"; + return p; +} + +// One C source per package, all sharing the basename `compress.c` — the shape +// that used to trigger the global census. +mcpp::modgraph::SourceUnit unitFor(const std::filesystem::path& pkgRoot, + const std::filesystem::path& rel, + std::string_view pkgName) { + mcpp::modgraph::SourceUnit u; + u.path = pkgRoot / rel; + u.relPath = rel; + u.packageName = std::string(pkgName); + touchFile(u.path); + return u; +} + +struct Built { + std::filesystem::path object; + std::filesystem::path packageObjectRel; +}; + +// Plan a graph containing the root project plus `deps`, and return the +// addresses computed for the FIRST dependency's single unit. +std::optional planZlib(const Tmp& t, bool withBzip2, std::string* err) { + auto projectRoot = t.path / "proj"; + auto storeRoot = t.path / "store"; + + mcpp::manifest::Manifest rootManifest; + rootManifest.package.name = "app"; + rootManifest.package.version = "0.1.0"; + rootManifest.package.standard = "c++23"; + mcpp::manifest::Target lib; + lib.name = "app"; + lib.kind = mcpp::manifest::Target::Library; + rootManifest.targets.push_back(lib); + + std::vector packages; + auto rootPkg = makePackage(projectRoot, "app"); + rootPkg.manifest = rootManifest; + packages.push_back(rootPkg); + packages.push_back(makePackage(storeRoot / "compat.zlib@1.3.2", "compat.zlib")); + if (withBzip2) + packages.push_back( + makePackage(storeRoot / "compat.bzip2@1.0.8", "compat.bzip2")); + + mcpp::modgraph::Graph graph; + graph.units.push_back(unitFor(projectRoot, "src/app.cpp", "app")); + graph.units.push_back( + unitFor(packages[1].root, "zlib-1.3.2/compress.c", "compat.zlib")); + if (withBzip2) + graph.units.push_back( + unitFor(packages[2].root, "bzip2-1.0.8/compress.c", "compat.bzip2")); + + std::vector topo; + for (std::size_t i = 0; i < graph.units.size(); ++i) topo.push_back(i); + + auto plan = make_plan(rootManifest, gccLike(), {}, graph, topo, packages, + projectRoot, projectRoot / "target" / "t", + {}, {}, {storeRoot}); + if (!plan) { if (err) *err = plan.error(); return std::nullopt; } + + auto want = packages[1].root / "zlib-1.3.2" / "compress.c"; + for (auto& cu : plan->compileUnits) { + if (cu.source != want) continue; + return Built{cu.object, cu.packageObjectRel}; + } + if (err) *err = "zlib compile unit not found in the plan"; + return std::nullopt; +} + +} // namespace + +// The load-bearing assertion. Adding an unrelated, deliberately colliding +// package to the graph must not move zlib's object by one byte — not its build +// path, and above all not its cache address. +TEST(ObjectAddress, DependencyAddressesAreImmuneToTheRestOfTheGraph) { + Tmp t; + std::string errAlone, errBeside; + auto alone = planZlib(t, /*withBzip2=*/false, &errAlone); + auto beside = planZlib(t, /*withBzip2=*/true, &errBeside); + ASSERT_TRUE(alone) << errAlone; + ASSERT_TRUE(beside) << errBeside; + + EXPECT_EQ(alone->packageObjectRel, beside->packageObjectRel) + << "the cache-entry address moved when an unrelated package joined the " + "graph — that is mcpp#344"; + EXPECT_EQ(alone->object, beside->object); +} + +// The address must also be package-internal: nothing about the consumer, and +// nothing that only exists on this machine. It is read back by another machine +// that computed the same key. +TEST(ObjectAddress, DependencyCacheAddressIsPackageInternal) { + Tmp t; + std::string err; + auto built = planZlib(t, /*withBzip2=*/true, &err); + ASSERT_TRUE(built) << err; + + auto rel = built->packageObjectRel.generic_string(); + EXPECT_FALSE(rel.empty()); + EXPECT_FALSE(built->packageObjectRel.is_absolute()) << rel; + EXPECT_FALSE(rel.starts_with("..")) << rel; + // Mirrors the source's path relative to its OWN package root, and carries + // no package-partition component (that lives in the build path only). + EXPECT_EQ(rel, "zlib-1.3.2/compress.o") << rel; + + // The build path does partition by package — that is what makes the + // cross-package census unnecessary. + auto obj = built->object.generic_string(); + EXPECT_EQ(obj, "obj/compat_zlib/zlib-1.3.2/compress.o") << obj; +} + +// `path_is_under_any` decides both whether a package may be cached and where +// its objects are anchored, and it must answer LEXICALLY. +// +// A payload store whose entries are symlinks into another store is ordinary: +// tests/e2e/_inherit_toolchain.sh builds one so an isolated MCPP_HOME can reuse +// the developer's toolchains, and CI caches do the same to avoid re-downloading. +// std::filesystem::relative() runs weakly_canonical and resolves those links, at +// which point `/registry/data/xpkgs/` no longer looks like it is in +// the store — every such package silently drops out of the build cache. That +// regression was caught by e2e 40 and is pinned here where it is cheap. +TEST(ObjectAddress, PathContainmentIsLexicalSoSymlinkedStoresStillCount) { + Tmp t; + auto real = t.path / "real-store"; + auto store = t.path / "home" / "registry" / "data" / "xpkgs"; + std::filesystem::create_directories(real / "compat.zlib@1.3.2" / "src"); + std::filesystem::create_directories(store); + + std::error_code ec; + std::filesystem::create_directory_symlink( + real / "compat.zlib@1.3.2", store / "compat.zlib@1.3.2", ec); + if (ec) GTEST_SKIP() << "symlinks unavailable: " << ec.message(); + + auto pkgRoot = store / "compat.zlib@1.3.2"; + EXPECT_TRUE(path_is_under_any(pkgRoot, {store})); + EXPECT_TRUE(path_is_under_any(pkgRoot / "src" / "compress.c", {store})); + + // And it still says no to something genuinely outside — the gate has to + // keep rejecting `target/.mangled/**`, which is the case it exists for. + EXPECT_FALSE(path_is_under_any(t.path / "proj" / "target" / ".mangled" / "x", + {store})); + EXPECT_FALSE(path_is_under_any(pkgRoot, {})); +} + +// The root project is never cached, so it keeps the flat layout every project +// has had since 0.0.1 — and a dependency shipping a same-named file can no +// longer force it to disambiguate. +TEST(ObjectAddress, RootObjectsStayFlatAndUncacheable) { + Tmp t; + auto projectRoot = t.path / "proj"; + auto storeRoot = t.path / "store"; + + mcpp::manifest::Manifest rootManifest; + rootManifest.package.name = "app"; + rootManifest.package.version = "0.1.0"; + rootManifest.package.standard = "c++23"; + mcpp::manifest::Target lib; + lib.name = "app"; + lib.kind = mcpp::manifest::Target::Library; + rootManifest.targets.push_back(lib); + + std::vector packages; + auto rootPkg = makePackage(projectRoot, "app"); + rootPkg.manifest = rootManifest; + packages.push_back(rootPkg); + packages.push_back(makePackage(storeRoot / "compat.zlib@1.3.2", "compat.zlib")); + + mcpp::modgraph::Graph graph; + // Root ships its own compress.c, colliding with the dependency's. + graph.units.push_back(unitFor(projectRoot, "src/compress.c", "app")); + graph.units.push_back( + unitFor(packages[1].root, "zlib-1.3.2/compress.c", "compat.zlib")); + + std::vector topo{0, 1}; + auto plan = make_plan(rootManifest, gccLike(), {}, graph, topo, packages, + projectRoot, projectRoot / "target" / "t", + {}, {}, {storeRoot}); + ASSERT_TRUE(plan) << plan.error(); + + for (auto& cu : plan->compileUnits) { + if (cu.source == projectRoot / "src" / "compress.c") { + EXPECT_EQ(cu.object.generic_string(), "obj/compress.o"); + EXPECT_TRUE(cu.packageObjectRel.empty()) + << "the root project must never get a cache address"; + } + } +} From b79eb436baab304378447e8ae55e52db6de90cff Mon Sep 17 00:00:00 2001 From: sunrisepeak Date: Mon, 3 Aug 2026 17:38:11 +0800 Subject: [PATCH 2/2] fix(build): link through a response file on every platform, not just Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Discovered by running the real mcpp-index workspace against the #344 branch: `opencv-module`, `opencv-module-dnn` and `opencv-module-unifont` all died with ninja: fatal: posix_spawn: Argument list too long naming no edge, no file and no cause. #344 lengthened dependency object paths (they now carry a per-package directory) and that was enough to cross a ceiling nothing was watching. The rule emitter said rsp was needed only where commands spawn through CreateProcess, and that "POSIX driver-style keeps the inline form: ARG_MAX is ample". Both halves are wrong. ninja runs `sh -c ""` on POSIX, so the command is a SINGLE argv entry and the limit is MAX_ARG_STRLEN — 32 pages, 128 KiB — not the 2 MiB ARG_MAX anyone would think to check. And it was never ample: measured on opencv-module, the inline link line was already 56 840 bytes before this branch, 43% of the ceiling. With #344's paths it reached 161 687. So: rsp always, every platform, every dialect. clang/gcc drivers, link.exe, GNU ar and llvm-ar all accept @rspfile, so this is one rule shape instead of two, and the response file next to the output reads better than a 160 KB command line anyway. A build system may not have a maximum project size that it discovers by crashing, and "how long is this command" must not be something anyone carries in their head when choosing an object path. The unit test asserted the falsified premise for POSIX ("must NOT use rspfile"); it now asserts the invariant on every platform, plus that `$in` appears exactly once per rule — as rspfile_content, never inlined into the command. Note this was invisible to all 18 CI jobs: none of them builds an opencv-class package. CI has no large-link coverage at all. --- CHANGELOG.md | 4 ++++ src/build/ninja_backend.cppm | 34 +++++++++++++++++++++------- tests/unit/test_ninja_backend.cpp | 37 +++++++++++++++++++------------ 3 files changed, 53 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2dea4208..cf6240cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,10 @@ #233(编译边撞名)、#240(链接输入未跟改名)与本条是同一台机器的三个产物:**布局由一次全局普查决定**。所以修的不是再补一处同步,而是把依赖包从普查里彻底拿掉。 +- **链接/归档命令一律走 response file,不再有「项目大到一定程度就崩」的隐形上限。** 此前只有 Windows(CreateProcess 32 KiB)与 msvc 方言用 rspfile,POSIX 走内联 `$in`,理由写的是「ARG_MAX 很宽裕」。两半都错:ninja 在 POSIX 上是 `sh -c "<整条命令>"`,整条命令是**一个 argv 项**,撞的是 `MAX_ARG_STRLEN`(32 页 = 128 KiB)而不是 2 MiB 的 `ARG_MAX`;而且它从来就不宽裕 —— 实测 mcpp-index 的 `opencv-module`,内联链接行**本来就已经 56840 字节**,占那条无人看守的上限的 43%。 + + 上面 #344 让依赖对象路径变长(多一层包目录),同一条边到了 161687 字节,于是 ninja 直接 `ninja: fatal: posix_spawn: Argument list too long` —— **不报是哪条边、哪个文件、什么原因**。构建系统不能有一个「靠崩溃才被发现的项目规模上限」,「这条命令有多长」也不应该是选对象路径时需要有人记在脑子里的事。clang/gcc driver、link.exe、GNU ar、llvm-ar 全都认 `@rspfile`,现在全平台一个规则形态。 + ### 改进 - **cache 条目与本次构建的布局分歧,现在降级为 miss 并明确报告,而不是让 ninja 崩在图加载阶段。** `is_cached` 此前校验的是条目**自述的**文件表,而消费方随后按**自己算的**地址去取 —— 两处独立推导,从不比对。命中判据现在校验「本次实际要读的那批产物」,任何不匹配都只是一次重编。同时新增一行 warning:一个系统性的分歧否则会表现为「cache 永远不命中」而毫无信号,这正是 v2026.7.30.2 之前那个假 `Cached` 骗了三个月的失败模态。 diff --git a/src/build/ninja_backend.cppm b/src/build/ninja_backend.cppm index 9e3a72df..e8de94af 100644 --- a/src/build/ninja_backend.cppm +++ b/src/build/ninja_backend.cppm @@ -749,15 +749,33 @@ std::string emit_ninja_string(const BuildPlan& plan) { // link/archive command; revisit the first-match replace if a dialect // ever grows another). // - // rsp is used when the command spawns through CreateProcess (32 KiB - // command-line ceiling): always for the separate-linker msvc dialect, - // and on Windows for driver-style too (#247 — ffmpeg/opencv-class - // packages link thousands of objects; clang/gcc drivers and GNU/llvm ar - // all accept @rspfile). POSIX driver-style keeps the inline form - // byte-identical: ARG_MAX is ample and the plain command is easier to - // reproduce by hand. + // rsp is used ALWAYS, on every platform and every dialect. The objects of + // one link edge are unbounded — an ecosystem package like opencv or ffmpeg + // contributes thousands — and every way of spawning a command has a ceiling: + // + // Windows CreateProcess, 32 KiB command line (#247) + // POSIX ninja spawns `sh -c ""`, so the command is a + // SINGLE argv entry and hits MAX_ARG_STRLEN — 32 pages, 128 KiB + // — long before the 2 MiB ARG_MAX anyone would think to check. + // + // This used to read "POSIX keeps the inline form; ARG_MAX is ample and the + // plain command is easier to reproduce by hand". Both halves were wrong. + // ARG_MAX is the wrong limit, and it was never ample: measured on + // mcpp-index's opencv-module, the inline link line was already 56 840 bytes + // — 43% of a ceiling nothing was watching. mcpp#344 lengthened dependency + // object paths (they now carry a per-package directory) and the same edge + // reached 161 687 bytes, at which point ninja dies with + // + // ninja: fatal: posix_spawn: Argument list too long + // + // naming no edge, no file and no cause. A build system may not have a + // maximum project size that it discovers by crashing, and "how long is this + // command" must not be a thing anyone has to keep in their head when + // choosing an object path. clang/gcc drivers, link.exe, GNU ar and llvm-ar + // all accept @rspfile, so there is one rule shape everywhere; the response + // file sits next to the output and `cat`ing it beats reading a 160 KB line. { - const bool useRsp = separateLinker || mcpp::platform::is_windows; + constexpr bool useRsp = true; auto link_rule = [&](std::string_view name, std::string cmd, std::string_view desc) { append(std::format("rule {}\n", name)); diff --git a/tests/unit/test_ninja_backend.cpp b/tests/unit/test_ninja_backend.cpp index 7d0dac34..be803b8f 100644 --- a/tests/unit/test_ninja_backend.cpp +++ b/tests/unit/test_ninja_backend.cpp @@ -570,12 +570,23 @@ TEST(NinjaBackend, RawMultiTokenFlagIsNotQuoted) { std::string::npos) << ninja; } -// mcpp#247: driver-style (gnu dialect) link/archive/shared rules must route -// the object list through a response file on Windows — every command spawns -// via CreateProcess (32 KiB command-line ceiling), and ffmpeg/opencv-class -// source packages link thousands of objects, so an inlined $in overflows it. -// POSIX keeps the inline form byte-identical (ARG_MAX is ample). -TEST(NinjaBackend, DriverStyleLinkRulesUseRspfileOnWindowsOnly) { +// mcpp#247 + #344: link/archive/shared route the object list through a response +// file on EVERY platform. The number of objects on one link edge is unbounded — +// an ffmpeg/opencv-class source package contributes thousands — and every way of +// spawning a command has a ceiling: +// +// Windows CreateProcess, 32 KiB command line +// POSIX ninja runs `sh -c ""`, so the command is a SINGLE +// argv entry and hits MAX_ARG_STRLEN (32 pages, 128 KiB) long before +// the 2 MiB ARG_MAX anyone would think to check +// +// This test used to assert the OPPOSITE for POSIX ("ARG_MAX is ample"), pinning +// an assumption that was both about the wrong limit and false: mcpp-index's +// opencv-module link line was already 56 840 bytes inline, and #344's per-package +// object directories took it to 161 687 — at which point ninja dies with +// `posix_spawn: Argument list too long`, naming no edge and no cause. A ceiling +// nothing watches is not a ceiling anyone can stay under, so there isn't one now. +TEST(NinjaBackend, DriverStyleLinkRulesAlwaysUseRspfile) { auto plan = minimal_plan(); // GCC → gnu dialect → driver-style branch auto ninja = emit_ninja_string(plan); @@ -587,14 +598,12 @@ TEST(NinjaBackend, DriverStyleLinkRulesUseRspfileOnWindowsOnly) { auto end = ninja.find("\n\n", start); ASSERT_NE(end, std::string::npos) << ninja; auto body = ninja.substr(start, end - start); - if constexpr (mcpp::platform::is_windows) { - EXPECT_NE(body.find("@$out.rsp"), std::string::npos) << body; - EXPECT_NE(body.find("rspfile = $out.rsp"), std::string::npos) << body; - EXPECT_NE(body.find("rspfile_content = $in"), std::string::npos) << body; - } else { - EXPECT_EQ(body.find("rspfile"), std::string::npos) << body; - EXPECT_NE(body.find("$in"), std::string::npos) << body; - } + EXPECT_NE(body.find("@$out.rsp"), std::string::npos) << body; + EXPECT_NE(body.find("rspfile = $out.rsp"), std::string::npos) << body; + EXPECT_NE(body.find("rspfile_content = $in"), std::string::npos) << body; + // And the object list must no longer be inlined into the command: that + // is the whole point, so `$in` may appear only as rspfile_content. + EXPECT_EQ(count_occurrences(body, "$in"), 1u) << body; } }