From f519dfbec05f9a78a979906092fde5cc99a989da Mon Sep 17 00:00:00 2001 From: speak-agent Date: Sun, 30 Aug 2026 12:49:45 +0800 Subject: [PATCH 1/2] fix(build): a package's generated inputs come before its compiles; an empty link is refused (2026.8.30.1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects, one change of mind about evidence. Each of them was a place where something answered "is this ready?" from a proxy instead of from the artifact. ── mcpp#534: `role = "source"` did not order a generated header ───────────── The engine's own comment claimed ordering needed no special handling because "a Source action's outputs ARE the compile edge's inputs". That is true of a generated `.cpp` and false of a generated `.h`: a header is reached through `-I`, never appears as an edge input, and the depfile that would record it does not exist until a compile has already succeeded. So an action whose outputs were all headers had a node in build.ninja that nothing could reach — not `default` (Source outputs are excluded on purpose), not the goal phony (objects and link outputs only), and no consuming edge. It never ran. The issue was filed as an intermittent race; it is deterministic, and five consecutive builds reproduce it identically. What made it look like a race is that `prepare_actions` wrote a zero-byte placeholder for every declared Source output, headers included — so the file was on disk whether or not the generator had run. * `BuildAction` records the package that declared it, spelled the same way `CompileUnit::packageName` is (`qualified_package_name`, now exported so the two cannot drift). * Each package that declares a gating action gets one phony over its outputs, and every compile edge of that package takes it as an order-only prerequisite. Per package, not per build: `include_dir` colours only the declaring package's own TUs, and a build-wide edge would express a dependency that does not exist while landing on the critical path. * `check_action_ordering` scans the emitted manifest and fails the build if any such edge is missing one — including the denominator, because "every edge that should carry it does" is vacuously true when none does, which was exactly the previous state. Seven call sites append the string today; being careful at seven sites is not a mechanism. * `blocking` on a `check` now does what it has been documented to do since it was introduced. It was typed, emitted over the build-program protocol, parsed, documented in two languages and demonstrated in a shipped example — and read by nothing. * Placeholders are no longer written for outputs that are not translation units. The scan never reads a header, and the empty file only ever turned "the generator did not run" into "the header is empty". ── mcpp#533: an empty link unit, reported as a shell error ────────────────── A dependency whose `install()` was skipped over a package-identity collision left a version directory with no sources. mcpp planned its shared library anyway and the user was shown `/bin/sh: 1: -shared: not found`. * A link unit with no inputs is refused at plan time, naming the target. The static case is why this is an error rather than a better linker message: `ar rcs` with no members exits 0 and writes an 8-byte archive, so the build REPORTED SUCCESS and every consumer failed later with undefined symbols. * `cc` is emitted unconditionally, for the reason `c_ldflags` twenty-six lines below already carried (mcpp#426). The rule had been written down for one variable of `c_link`/`c_shared` and not for the other. * `check_rule_commands_name_a_program` scans the manifest for the class: a rule's command must begin with a program. Deliberately not the more obvious "no undefined variables" — ninja's empty expansion is a feature several rules rely on (`$soname_flag`, `$unit_ldflags`), and that check would have needed an allowlist of exceptions. * `.mcpp_ok` is no longer written from the installer's exit code plus the existence of a directory the installer creates before doing any work. It now requires one entry that neither mcpp nor xlings wrote. Withheld rather than fatal, because a package may legitimately install no payload. * The same predicate runs on the fast path, so a store already poisoned by this bug heals on the next build instead of requiring the user to know which directory to delete. * The lib-root warning asked `has_lib_target` — "does this produce a library" — when the property it wants is "is this a C++ module library". A source-built C package warned that `src/.cppm` was missing in every consumer's build. ── Tests ─────────────────────────────────────────────────────────────────── e2e 314 (a dependency generating its only header), 315 (blocking gates the compile, non-blocking does not), 316 (empty shared AND static targets refused; a populated one still builds). All three were run against the pre-fix binary and all three fail there, so they discriminate rather than describe. Unit: 14 new cases over the two emitter guards, the ordering denominator, and the install-marker evidence — including the poisoned-store heal. Analysis and cross-repo plan: .agents/docs/2026-08-30-*.md Refs #533, #534 --- ...6-08-30-cross-repo-fix-plan-532-533-534.md | 837 ++++++++++++++++++ .../2026-08-30-issues-532-533-534-analysis.md | 443 +++++++++ docs/07-build-mcpp.md | 23 +- docs/zh/07-build-mcpp.md | 17 +- mcpp.toml | 2 +- modules/buildmcpp/src/directives.cppm | 19 +- modules/manifest/src/types.cppm | 12 + modules/versioning/src/version.cppm | 2 +- src/build/ninja_backend.cppm | 410 ++++++++- src/build/plan.cppm | 25 +- src/build/prepare.cppm | 65 +- src/fallback/install_integrity.cppm | 66 ++ src/modgraph/validate.cppm | 18 +- src/pm/package_fetcher.cppm | 23 +- .../314_dependency_action_generated_header.sh | 158 ++++ .../315_blocking_check_gates_compilation.sh | 105 +++ ...316_link_unit_with_no_inputs_is_refused.sh | 96 ++ tests/unit/test_install_integrity.cpp | 84 ++ tests/unit/test_ninja_backend.cpp | 197 +++++ 19 files changed, 2550 insertions(+), 52 deletions(-) create mode 100644 .agents/docs/2026-08-30-cross-repo-fix-plan-532-533-534.md create mode 100644 .agents/docs/2026-08-30-issues-532-533-534-analysis.md create mode 100755 tests/e2e/314_dependency_action_generated_header.sh create mode 100755 tests/e2e/315_blocking_check_gates_compilation.sh create mode 100755 tests/e2e/316_link_unit_with_no_inputs_is_refused.sh diff --git a/.agents/docs/2026-08-30-cross-repo-fix-plan-532-533-534.md b/.agents/docs/2026-08-30-cross-repo-fix-plan-532-533-534.md new file mode 100644 index 00000000..1d52dde8 --- /dev/null +++ b/.agents/docs/2026-08-30-cross-repo-fix-plan-532-533-534.md @@ -0,0 +1,837 @@ +# Cross-repo fix plan: #532, #533, #534 + +> Status: plan. No code changed yet. +> Branch: `fix/action-ordering-and-package-identity`, cut from +> `mcpp-community/mcpp` @ `ab1da5d` (origin/main). +> Evidence: `.agents/docs/2026-08-30-issues-532-533-534-analysis.md` — every +> claim below rests on a reproduction recorded there, not on reading alone. +> Repos touched: **mcpp** (mcpp-community/mcpp), **xlings** +> (d2learn/xlings, released as openxlings/xlings), **mcpp-index** +> (mcpplibs/mcpp-index). + +--- + +## 0. What this plan is + +Seven changes in mcpp, one in xlings, and a **policy decision** in mcpp-index. +They are grouped into four tracks that can be reviewed, merged and released +independently, plus one ordering constraint between repos that cannot be +reordered. + +The one thing to get right before anything else is §1. Everything after it is +ordinary work. + +| track | repo | issue | depends on | +|---|---|---|---| +| **A** — the misleading error | mcpp | #533 L3 | nothing | +| **B** — action ordering | mcpp | #534 | nothing | +| **C** — install marker, lib root | mcpp | #533 L2/L4 | nothing | +| **X** — store identity | xlings | #533 L1 | nothing | +| **F** — floor bump | mcpp | — | X released | +| **I** — index policy | mcpp-index | #533 impact | F released *and adopted* | + +A, B, C and X are mutually independent and can land in parallel. F and I are +not, and §1 is why. + +--- + +## 1. The cross-repo ordering constraint + +**Fixing xlings does not make it safe to publish a colliding version.** + +This is the part that cannot be undone once it goes wrong, so it is stated +first. + +After X lands, a machine running the *fixed* xlings will install +`compat:libdrm@2.4.123` correctly even though `xim:libdrm@2.4.123` is in the +store. A machine running an *older* xlings will still silently skip `install()` +and produce the `-shared: not found` failure. The index is data consumed by +every installed client, not just the newest — so the moment mcpp-index publishes +a descriptor whose `@` collides with an xim package, **every +user below the fixed xlings breaks**, and breaks in the way #533 documents: +silently, with an error naming the linker. + +mcpp already has the mechanism for this. `src/xlings/xlings.cppm:65`: + +```cpp +inline constexpr std::string_view kXlingsVersion = "2026.8.27.5"; +``` + +The comment above it is explicit that this is a **floor, not a current pick**, +and `.github/tools/check_version_pins.sh` enforces that the seven copies under +`.github/` agree with it. `acquire_xlings_binary` +(`src/fallback/xlings_binary.cppm:53`) replaces a vendored xlings that is +strictly older than the floor. + +So the ordering is: + +``` +X merged in xlings + → xlings release V + → mcpp Track F: kXlingsVersion = V (one edit; CI enforces the 7 copies) + → mcpp release carrying that floor + → the installed base moves + → ONLY THEN may mcpp-index publish a colliding @ +``` + +The last arrow is not a build step and has no green checkmark. It is a judgement +about the installed base. §6 proposes not taking it at all. + +**Nothing in tracks A, B, C or X requires this.** Track X is worth merging on its +own merit — it removes a silent failure for anyone who hits the collision by +accident, which per §6.1 is already 20 short names wide. + +--- + +## 2. Track A — mcpp: stop reporting a package-identity bug as a shell error + +Three changes. **A2 is the load-bearing one** — see the measurement immediately +below. A1 is hygiene; A3 is what stops the class. + +### A0 — the measurement that sets the priority + +The self-review (§11) asked what `-shared: not found` would become if only A1 +landed, and whether the shared-library case is the whole story. Both were +measured: + +``` +$ gcc -shared @empty.rsp -o out.so +gcc: fatal error: no input files # exit 1 — bad message, but it FAILS + +$ ar rcs libempty.a +$ echo $?; stat -c %s libempty.a +0 # exit 0 +8 # an empty archive, silently +``` + +So the reported symptom has a **silent sibling**. A `kind = "lib"` package whose +`install()` was skipped produces an empty 8-byte `.a` through `cxx_archive` +(`ninja_backend.cppm:1915`, `$ar` from `dial.archiveCmd`) and the build **reports +success**. Consumers then fail at link time with undefined symbols, three layers +further away than #533's shell error. + +Two consequences for this plan: + +1. **A2 is the fix; A1 is not.** A1 alone converts a shell error into + `gcc: fatal error: no input files`, and does nothing at all for the static + case, which never reaches a compiler driver. +2. **A1 must not land without A2.** On its own it makes the shared case quieter + without making it correct, and quieter is the direction #533 was already + suffering from. + +### A1 — emit `cc` unconditionally + +`src/build/ninja_backend.cppm:594` + +```cpp +if (need_c_rule || need_asm_rule || need_ios_init_shim) { + append(std::format("cc = {}\n", escape_ninja_path(flags.ccBinary))); +} +``` + +`c_link` and `c_shared` reference `$cc`, and a link unit with zero compile units +selects `c_shared` while `need_c_rule` is false. `$cc` expands to nothing and the +shell executes `-shared`. + +Three emission sites across two dialect branches — `c_link` at +`ninja_backend.cppm:1171` (the `ldDriver` branch, which emits no `c_shared`), and +`c_link`/`c_shared` at `:1188` and `:1190` (the default branch). Reasoning about +which branch is reachable with which variables defined is exactly the work A3 +removes. + +**The file already states the rule that prevents this, 26 lines below**, for the +other variable of the same two rules (`ninja_backend.cppm:615-620`, mcpp#426): + +> *ALWAYS emitted, even when identical — `c_link` references `$c_ldflags`, and a +> conditional definition would make an empty link line the failure mode on +> exactly the toolchains where the two happen to agree.* + +Apply it to `$cc`. Leave `cflags` conditional: it is referenced only by +`c_object`, which is itself gated on `need_c_rule`. + +### A2 — refuse a link unit with no inputs, at plan time + +`LinkUnit::objects` (`src/build/plan.cppm:88`) is empty and nothing checks it. +There is no guard anywhere in `plan.cppm`, `prepare.cppm` or `ninja_backend.cppm`. + +Add the check where link units are finalised, after the `role = "object"` action +outputs are attached (`prepare.cppm:8370`) and after the PE `.def` attachment +(`prepare.cppm:8608-8610`) — those are the two places objects arrive from +somewhere other than the compile set, and checking before them would fire on a +legitimate unit. + +**Scope, measured.** mcpp gives every library target of a package the package's +whole inferred source set — a two-target project where one target "has no +sources of its own" still links the same objects, so it is not affected. The +only route to an empty unit is a package with **zero** compile units, which is +exactly the state a skipped `install()` leaves behind. The check is therefore +narrow, and it must cover **all three kinds** — `Binary`, `SharedLibrary` and +`StaticLibrary` — because per A0 only the static one is currently silent. + +Message must name the target and what matched nothing: + +``` +error: target 'libdrm' (kind = shared) has no inputs to link + inferred sources [src/**/*.{cppm,cpp,cc,c,S,s,asm}] matched 0 files under + + a library target must have at least one translation unit, one `role = + "object"` action output, or an explicit [lib].path +``` + +**Not** a `mcpp::build::refusal::Code`. That enum +(`src/build/refusal.cppm`) classifies *target* refusals — which triple, which +toolchain — and every one of its 14 codes answers "why can this platform not be +built". A link unit with no inputs is a project error, not a platform verdict. +Adding a code here would widen the enum's meaning to something its consumers +(`tests/matrix/scan.sh`) do not read it for. + +### A3 — the durable guard: no rule may reference an undefined variable + +A1 fixes one variable. A3 makes the class impossible. + +`ninja_backend.cppm` already has the pattern, and its comment says why +(`check_inline_command_lengths`): + +> *Scanning the emitted manifest rather than instrumenting each emit site is +> deliberate — a new edge kind is then covered the day it is added, which is +> precisely how the previous seven slipped through.* + +Add a sibling, `check_undefined_ninja_variables(manifest)`, run at the same +point: collect every `$name` referenced inside an emitted `rule` block, collect +every top-level `name = ` definition, and fail the build if a rule references a +name that is never defined. Same shape, same call site, same test file. + +This is the change that would have caught #533 L3 before it shipped, and it +costs one function. + +> **Review question.** A3 turns any future conditional-variable slip into a hard +> build failure at manifest-emit time. That is the right strength for a defect +> whose alternative failure mode is `/bin/sh: 1: -shared: not found` — but it +> means a rule referencing a genuinely optional variable (none today) would have +> to define it empty. Confirm that is acceptable before implementing A3. + +--- + +## 3. Track B — mcpp: action ordering (#534, all three defects) + +The issue asks for exactly the right fix. Four changes deliver it. + +### B1 — `BuildAction` learns which package declared it + +`modules/manifest/src/types.cppm:299` — add: + +```cpp +std::string packageName; // the package whose build.mcpp declared this +``` + +Set it in `collect()` at `src/build/prepare.cppm:8298-8305`, which already walks +`*m` (root) and `packages[i].manifest` (dependencies) and knows which is which. +`CompileUnit` already carries `packageName` (`plan.cppm:44`), so after B1 both +sides of the edge speak the same key. + +This field is *not* serialised across the build-program protocol — the protocol +carries what the program declared, and the package is known to the engine, not +to the program. + +### B2 — a per-package action phony, order-only on that package's compile edges + +Model it on `kStagedCachePhony`, which is the same shape already working in this +file: + +```cpp +// today, ninja_backend.cppm:1534 — one phony, one global string +stagedOrderOnly = " || " + std::string(kStagedCachePhony); +``` + +Replace the scalar with a per-unit lookup: + +```cpp +std::string order_only_for(const CompileUnit& cu); // staged phony + this package's action phony +``` + +Emit `build mcpp-actions- : phony ` for each +package that declares ≥1 `Source` action, or ≥1 `Check` action with +`blocking = true` (B3). + +**The seven call sites that must all be converted** — this is the enumeration, +not an example: + +| line | edge | +|---|---| +| `ninja_backend.cppm:1558` | `cxx_scan` (the .ddi edge) | +| `:1683` | dyndep-mode object edge | +| `:1754` | split-BMI object edge | +| `:1778` | object edge | +| `:1786` | object edge | +| `:1789` | object edge | +| `:1841` | object/asm edge | + +`Object` and `Artifact` roles stay out of the phony: an `Object` action's outputs +are link inputs (`prepare.cppm:8370`) and an `Artifact` action's inputs are link +outputs, so both are already sequenced by file dependency — which is what +`ninja_backend.cppm:2090` claims for all four roles and is true for these two. + +`Source` role also stops being excluded from `actionDefaults` +(`ninja_backend.cppm:2173`)? **No** — leave that exclusion. After B2 the phony +makes Source outputs reachable through the compile edges, which is the +architecturally correct route and keeps explicit-goal builds (#274) working +without a `default` entry. Reaching them two ways would resurrect the +soname-alias problem in mirror image. + +**Per-package, not one global phony.** The simpler design — a single phony over +every action output in the build, order-only on every compile edge — needs no +B1 and mirrors `stagedOrderOnly` exactly. It is rejected for two reasons that +point the same way. Semantically, `include_dir` *"colours only this package's own +TUs"* (`docs/07-build-mcpp.md`, line 67), so a generated header is visible to one +package by construction and cross-package ordering would express a dependency +that does not exist. Practically, a modular mcpp build is latency-bound — the +critical path is effectively 100% of wall clock — so a false edge from package +A's compile to package B's generator lands directly on that path. B1 costs one +string field; the global variant costs correctness of meaning and wall clock. + +### B5 — a structural self-check, not seven trusted edits + +B2 edits seven call sites. An eighth added later without the order-only string +reintroduces #534 for that edge kind, silently — and §10 originally left this as +a risk to be careful about. Being careful is not a mechanism. + +The emitter has everything needed to check itself: after the manifest is built, +it knows each package's action phony and each package's object paths +(`CompileUnit::packageName` + `CompileUnit::object`). Assert, over the emitted +text, that every `build` line producing an object of package P carries +`|| mcpp-actions-P` whenever P declares a qualifying action. + +Same shape and same call site as `check_inline_command_lengths`, whose comment +states the principle this reuses: + +> *Scanning the emitted manifest rather than instrumenting each emit site is +> deliberate — a new edge kind is then covered the day it is added, which is +> precisely how the previous seven slipped through.* + +Seven, again. B5 is what makes B2's enumeration hold. + +### B3 — wire `blocking` + +`action.blocking` is typed (`types.cppm:331`), emitted +(`src/build/hostprogram.cppm:126`), parsed +(`modules/buildmcpp/src/directives.cppm:721`), documented in both languages +(`docs/07-build-mcpp.md:315`, `docs/zh/07-build-mcpp.md:281`) and demonstrated +(`examples/08-build-rules/rules-tidy`) — and **read nowhere**. The only `||` in +the backend is `stagedOrderOnly`. + +B2 supplies the mechanism. A `Check` action with `blocking = true` puts its stamp +in the package's phony; a non-blocking one does not. One `if`, once the phony +exists. `blocking = true` stops being a documented no-op. + +### B4 — stop materialising placeholders for outputs that are not translation units + +`modules/buildmcpp/src/directives.cppm:788-800` writes a zero-byte file for every +`Source` output with no extension filter. The scanner only ever reads translation +units, and `adoptActionOutputs` already refuses to adopt anything else +(`prepare.cppm:4255`). + +The placeholder's only present effect on a generated header is to turn *"the +generator did not run"* into *"the header is empty"*. That substitution is what +cost #534 its diagnosis: the reporter saw the file on disk and concluded the +action had run. + +Gate the placeholder on `is_compilable_output` (`directives.cppm:770`), the same +predicate `adoptActionOutputs` uses. + +> **Review question.** B4 is behaviour-visible: a project that today compiles +> against an empty generated header (because the action never ran, and nothing +> in it was used yet) will start failing with *file not found* instead. That is +> the better error, but it is a change. B4 is separable from B1–B3 — it can be +> dropped without weakening the fix, at the cost of leaving the misleading +> artefact in place. + +--- + +## 4. Track C — mcpp: the marker, and the warning + +### C1 — do not stamp `.mcpp_ok` on the callee's word + +`src/pm/package_fetcher.cppm:1095` + +```cpp +if (inst->exitCode == 0 && std::filesystem::exists(verdir)) { + mcpp::fallback::mark_install_complete(verdir); +``` + +mcpp asks the right question — `verdir` is namespace-qualified at +`package_fetcher.cppm:986` — and accepts the wrong proof. Because step 1 of the +resolution chain (`is_install_complete`, `:1049`) short-circuits on this marker, +the wrong state survives every subsequent build. + +The reported directory contained `.mcpp_ok`, `.xpkg-install.json` and +`mcpp_generated/` — **nothing except what mcpp and xlings wrote themselves**. +That is a detectable signature that needs no knowledge of the descriptor: + +``` +payload_is_substantive(verdir) := + ∃ entry in verdir not in { .mcpp_ok, .xpkg-install.json, mcpp_generated/ } + OR verdir has any of bin/ include/ lib/ (the payload shape mcpp consumes) +``` + +Deliberately weak. It cannot verify a descriptor produced the *right* tree, and +it should not try. + +**Two call sites, not one — this is what makes the upgrade seamless.** + +1. **Before writing the marker** (`package_fetcher.cppm:1095`): if + `!payload_is_substantive(verdir)`, do **not** call + `mark_install_complete`, and warn naming the package and the directory + contents. Not a hard error — see the scoping note below. +2. **On the fast path** (`package_fetcher.cppm:1049`, + `is_install_complete`): re-check there too. `is_install_complete` is + marker-only by design (`src/fallback/install_integrity.cppm:135-150`), so + **a store already poisoned by this bug never heals** — the bad `.mcpp_ok` is + already on disk and short-circuits forever. Without (2) the fix helps only + users who have not hit the bug yet, which is the wrong half. + +**Scoping — why a warning and not a hard error.** xlings supports type-only +packages that legitimately install no payload (`installer.cppm`: *"type-only +packages like auto-config don't need a payload"*), and #531 now provisions +`[xlings] deps` on first build, so mcpp resolves packages it does not consume a +tree from. A hard error would fail those. Withholding the marker is the honest +action — the marker means *verified complete*, and mcpp could not verify it — and +the cost of being wrong is one cheap re-check per build (xlings short-circuits +its own install), not a failure. The build then fails, if it fails, at A2 with a +message that names the target. + +This is the third recurrence of "`.mcpp_ok` proves a process exited 0, not that +an artifact is correct", so the guard belongs beside `kInstallMarker` in +`src/fallback/install_integrity.cppm` — one predicate, both call sites — not +open-coded at either. + +### C2 — the lib-root warning asks the wrong question + +`src/modgraph/validate.cppm:157`, gated on `has_lib_target` +(`modules/manifest/src/types.cppm:1317`), true for any `Library` or +`SharedLibrary`. The convention candidate is `src/.cppm`; a pure-C library +has none and can never have one. Reproduced verbatim in a two-line `mcpp.toml`. + +The predicate is *"does this target produce a library"*; the property wanted is +*"is this a C++ module library"*. Narrow the gate: skip the lib-root check when +the target's source set contains no file classifying as a module interface under +the package's own extension table (`mcpp::extension_table_for`, the same table +`adoptActionOutputs` uses at `prepare.cppm:4253`). + +Per #533: this warning propagates to every consumer of a C package, so it is not +cosmetic. + +--- + +## 4b. Track D — mcpp: the documentation is part of the defect + +Not cosmetic. `docs/07-build-mcpp.md:315` and its Chinese counterpart carry the +table that says what each role guarantees, and two of its cells are the claims +#534 disproves: + +| cell | today | after B | +|---|---|---| +| `source` / Ordering | *"the compile edge consumes them"* | true only for a generated TU; a generated **header** is never an edge input. Must say the package's compile edges wait for the action. | +| `check` / Ordering | *"runs **alongside** compilation (set `blocking = true` to gate it)"* | the parenthesis describes a mechanism that does not exist until B3 | + +Also `ninja_backend.cppm:2088-2095`, the comment that states the false +assumption in the engine itself: + +> *Ordering needs no special handling: a Source action's outputs ARE the compile +> edge's inputs …* + +That comment is why the defect was not found earlier, and leaving it in place +after B would leave the next reader with the same wrong model. It must be +rewritten to say what is actually true: `Object` and `Artifact` are sequenced by +file dependency; `Source` and blocking `Check` are sequenced by the package's +action phony, because their outputs may be headers, which are reached through +`-I` and never appear as edge inputs. + +Both language versions of the docs change together — CI enforces the pair. The +`examples/08-build-rules/README.md` line about `blocking = true` becomes true for +the first time and needs no change. + +--- + +## 5. Track X — xlings: key the store lookup on the qualified identity + +`src/core/xim/installer.cppm:903-911`: + +```cpp +// Default: check xvm version database when no installed hook +else if (!payloadInstalled) { + auto db = Config::versions(); + auto resolved = xvm::match_version(db, node.name, node.version); // ← short name + if (!resolved.empty()) { + log::debug("{} already installed in xvm (version {})", node.name, resolved); + payloadInstalled = true; + } +} +``` + +Three facts: + +1. `node.name` is the bare short name. The same `PlanNode` carries + `namespaceName` and `canonicalName` + (`src/core/xim/libxpkg/types/type.cppm:34-44`). The qualified identity is at + the call site and discarded. +2. `xvm::match_version` does `db.find(target)` (`src/core/xvm/db.cppm:99`); the + VersionDB is keyed by **program name**. That is the correct key for xvm's own + question — *which version of the program named X is active* — and the wrong + key for *is package `:@`'s payload installed*. +3. The **primary** path is already correct. `src/core/xim/catalog.cppm:302`: + + ```cpp + auto installDir = match.storeRoot / package_store_name(match.namespaceName, match.name) / match.version; + match.installed = exists(installDir) && is_directory(installDir) && !is_empty(installDir); + ``` + +**Proposed fix.** The fallback should ask the store the way `catalog.cppm:302` +already does, not the xvm program table. Extract that expression into one +function and call it from both places — two answers to one question should be +one function. The xvm lookup then either goes away, or is kept strictly for what +it is about (shim activation) and no longer gates `install()`. + +**X2 — the diagnostic.** Whatever the key becomes, a lookup that matched a +*different* namespace must not be `log::debug`. Either refuse, or say at default +verbosity which store entry was matched: + +``` +compat:libdrm@2.4.123 — skipping install(): matched xim-x-libdrm/2.4.123 +``` + +#533's closing sentence is the requirement: *"现在的表现是把一个包管理器的身份 +问题伪装成了一条链接器命令行错误。"* The engine fix removes the substitution; +X2 is what makes the *next* identity question legible instead of silent. + +Related, not the same: xlings#381 is this defect on the **index** side (keying by +`(namespace, name)`). X is the **store** side. Fixing one does not fix the other. + +--- + +## 6. mcpp-index: what becomes possible, and the recommendation not to use it + +### 6.1 Current state, measured + +Nothing in the index is broken today. The four packages routed around the +collision by choosing versions that do not collide — `origin/main` @ `89cfee7`: + +| package | version on index main | #533 said it wanted | +|---|---|---| +| `compat.libdrm` | `2.4.134` | `2.4.123` | +| `compat.wayland` | `2026.08.30` | `1.23.1` | +| `compat.libffi` | `3.4.8` | `3.4.4` | +| `compat.expat` | `2.7.1` | `2.6.2` | + +The store on the development machine already holds **20 short names occupying +two or more namespaces** — `libpng`, `expat`, `zlib`, `cairo`, `fontconfig`, +`linux-headers`, `nasm`, `ncurses`, `mcpp`, `xlings` and more. The constraint +#533 identifies is already load-bearing across the installed base; it holds only +because the versions happen to differ. + +### 6.2 Recommendation: do not re-pin + +Track X removes the defect for anyone who hits the collision **by accident** — +which, given 6.1, is the common case and the one worth fixing. Deliberately +publishing a colliding version is a different act, and per §1 it breaks every +client below the floor, silently. + +Concretely: + +- **Do** merge and release X. It makes accidental collisions install correctly + and (via X2) legible. +- **Do not** change the four packages' versions. They work, and a bare version + pin is exact — every consumer would have to be re-pinned individually, and the + version a consumer pinned is not visible in the index descriptor. +- **Do** allow *new* packages to use the upstream version once the floor has + shipped and been adopted, as a per-package judgement rather than a blanket + policy change. + +An index-side `min_mcpp` on colliding descriptors is **not** proposed: a floor +in the index turns old clients into bricks, and the index is data while mcpp is a +program — published data must not invalidate the program. + +> **Review question.** §6.2 says the four packages keep their current versions +> permanently. The alternative is to schedule a re-pin after the floor ships, +> which buys "the consumer and Mesa name the same libdrm version" at the cost of +> an enumerated re-pin of every consumer and a window where older clients fail +> silently. This is a judgement call, not a technical one — flagging it rather +> than deciding it. + +--- + +## 7. #532 — the graphics-stack example + +Not part of any track above; listed so the three issues close together. + +The technical answer is sound and worth landing: four declarations resolve an +11-entry closure including three transitive libraries no manifest names, which +is what #527 §1.4 said was impossible. Three amendments before merge: + +1. **Reword the rationale.** The sentence *"mcpp 在构建期就明说了"* is true only + when the closure would be unsatisfiable. Measured: `-lcap` (host-only) is + refused with a precise message; `-lgbm` on a machine that also has Mesa in the + SubOS **builds silently, exit 0**, and the artifact takes its ABI from + `/usr/include` while its private loader resolves `libgbm.so.1` from + `registry/subos/default/lib`. On the exact class of machine this example + targets, #527's reporter gets silence, not a refusal. + + The correction strengthens the argument: the reason not to write + `-L/usr/lib` is not that mcpp refuses it — it is that mcpp **cannot** refuse + it when the soname resolves on both sides, and you are then shipping an + artifact compiled against one library and loaded against another. + +2. **Re-measure the closure in a sandbox.** The 11 entries were counted on the + development machine, where `registry/subos/default/lib` is a shared, + accumulating view — the same directory that silently supplied `libgbm.so.1` + above. That reading cannot distinguish *"satisfied by the four declared + packages"* from *"satisfied by whatever else this machine has installed"*. + +3. **Decide the CI posture and write it in the README.** Examples are covered + individually (`tests/e2e/312_build_rules_example.sh` builds example 08). + Example 09 opens `/dev/dri/renderD128`, which no runner has, and its four + dependencies are index packages needing network and a current index. Either + build-only in CI with the device work behind a runtime guard, or documented as + uncovered — an example with neither rots, and this one carries four external + version pins that will drift. + +**Separate follow-up to file:** the silent link-A/load-B case in (1) is a defect +in its own right, and it is the part of #527 §1 that is actually about mcpp +rather than about usage. + +--- + +## 8. Tests and criteria + +Criteria are stated so that a *false* reading and a *did-not-run* reading are +different, and so that every count carries a denominator. + +### New e2e (next free number is 314; all `# requires: gcc`, which both shards have) + +| id | asserts | why this shape | +|---|---|---| +| **314** | a **dependency package** whose `Source` action emits **only a header** builds, and the header is non-empty with expected content | #534 D1 in the reported shape. Assert **content**, not existence — existence passes today against a 0-byte placeholder | +| **315** | a companion header (`p.cc` + `p.h`) consumed by a **different TU** compiles, with a `sleep` in the generator and at `-j` > 1 | #534 D2. `188_build_actions.sh` §2c has this action but its `main.cpp` does not include `p.h`, so ordering is never exercised | +| **316** | `blocking = true` on a failing check prevents the object from being **produced**; `blocking = false` does not | #534 D3. Assert on the artifact, not on a log line | +| **317** | a `kind = "shared"` target matching zero sources fails with a message naming the target, and the message does **not** contain `-shared: not found` | Track A2. The negative half is the criterion that catches a regression to the shell error | +| **318** | after a reported-successful install, a version dir containing only mcpp/xlings-written entries is refused and named | Track C1 | + +### Structural criteria on the emitted manifest (unit tests on `emit_ninja_string`) + +- **Denominator, Track B.** For a plan with one package declaring ≥1 Source + action: `count(compile edges carrying "|| mcpp-actions-")` equals + `count(compile edges of that package)`, and **both counts are asserted + non-zero separately**. A single equality passes when both are zero — which is + exactly today's state. +- **Track A1.** A plan with zero C compile units and one `SharedLibrary` link + unit emits a `cc = ` line. Today it does not; the repro is in the analysis + document. +- **Track A3.** A manifest referencing an undefined `$var` inside a rule is + rejected. Add one deliberately-broken fixture so the checker itself is tested. + +### Amend, do not replace + +`tests/e2e/188_build_actions.sh` §2c stays as is — it tests that a companion +header is not adopted as a TU, which is a real invariant. 315 is a new case, not +a rewrite. Line 178's comment (`// companion: produced, NOT compiled`) should +gain a pointer to 315 so the ordering gap is not re-derived. + +### Existing coverage that must not regress + +`tests/e2e/313_check_stamp_on_every_platform.sh` covers the `check` role's stamp +wrapper. B3 changes when a blocking check's stamp is *demanded*, not how it is +written; 313 must stay green unmodified. + +--- + +## 9. PR sequence + +**Chosen shape: xlings first and released, then one mcpp PR carrying everything +including the floor pin.** The alternative — four small mcpp PRs — was rejected +because Track F (the floor bump) can only be written once V exists, and splitting +would either strand F in a fifth PR or pin a version that is not yet released. +One PR also means one CI matrix proves the combination, which matters here: A2 +and B2 both change what a *failing* build prints, and their interaction is only +observable together. + +| # | repo | contents | gate | +|---|---|---|---| +| 1 | xlings | X1 + X2 + a test that two namespaces × same `(name, version)` both install | — | +| 2 | xlings | merge, then release **V** | 1 green on xlings `main` | +| 3 | mcpp | **one PR**: A1+A2+A3, B1–B5, C1+C2, Track D docs (EN+ZH), e2e 314–318, unit tests with denominators, version bump, and `kXlingsVersion = V` | 2 released | +| 4 | mcpp | CI green → second self-review of the diff → merge → verify the run on `origin/main` HEAD SHA | 3 | +| 5 | mcpp | release; backfill GitCode assets with `gtc` | 4 | +| 6 | — | sandbox verification of the released artifacts (§8 "released-form criteria") | 5 | +| 7 | mcpp | #532 example with the three §7 amendments; file the link-A/load-B follow-up | independent | + +Within PR 3 the tracks are independent and can be implemented in parallel; only +the floor pin is gated on step 2. + +Per §1, step 5 does **not** authorise an index change. §6.2 stands. + +House rules: everything except pure documentation goes through a PR; a green PR +is not a green `main` — the criterion is the CI run on `origin/main`'s HEAD SHA +after merge; and for the release, the criterion is the index `latest` pointing at +it, not the tag existing. + +--- + +## 10. What could still go wrong + +- **B2's phony membership is a new enumeration.** Seven call sites today; an + eighth added later without the order-only string reintroduces #534 for that + edge kind, silently. *Resolved during self-review:* this is B5, not a risk to + be careful about. The denominator test in §8 remains necessary — a plain + `grep -c … || …` test reads zero as success — but B5 is what makes the + enumeration hold. +- **A2 could refuse a legitimate unit.** A link unit whose objects all arrive + from `role = "object"` actions is legitimate and non-empty at + `prepare.cppm:8370` — hence placing the check *after* that attachment and + after the PE `.def` attachment at `:8608`. If a third source of link inputs is + added later, it must land before the check. Worth a comment at the check site + naming both existing sources. +- **C1's exclusion list is a hand-maintained list**, which is the shape this + codebase has been burned by before. Keep it to the three names that exist, + put it next to `kInstallMarker` in `src/fallback/install_integrity.cppm`, and + state in the comment that a new mcpp-written entry must be added to it. +- **Track F's floor bump is measured on the released artifact, not the merge.** + The criterion is that a fresh install pulls xlings V and that `mcpp self env` + reports it — not that PR 7 is green. +- **X1 changes install-skip behaviour for every xlings user**, not just mcpp's. + A package that was being skipped and is now installed will run an `install()` + hook that may not have run in a long time. Worth checking on xlings' side + whether any descriptor depends on being skipped. + +--- + +## 11. Self-review log + +The plan above was reviewed against nine axes before any code was written. Six +axes produced changes; three confirmed the plan as drafted. Everything recorded +here is a change already folded into §1–§10 — this section exists so a reviewer +can see *what moved and why*, not to hold pending work. + +### 11.1 Architecture — one principle, found three times + +`.mcpp_ok`, the ninja graph, and the runtime closure validator all answer some +form of *"is this thing ready?"*, and this investigation found each of them +trusting a proxy instead of the artifact: + +| answerer | trusted | should trust | +|---|---|---| +| `.mcpp_ok` | the callee's exit code + directory existence | something the package installed (C1) | +| ninja graph | *"a Source action's outputs ARE the compile edge's inputs"* | an explicit order-only edge, because headers never are (B2) | +| link edge | that a rule's variables are defined because its inputs exist | a manifest-level check (A3) | + +Stated once: **a completeness marker must be derived from the artifact, not from +the producer's report.** Each of A3, B2 and C1 is that principle applied where it +was missing. This is why they belong in one PR — they are one change of mind +about evidence, in three places. + +Second architectural note: after B, `mcpp::action`'s documented contract +(*"`role` only decides where the edge's outputs attach"*) becomes true for all +four roles for the first time. Today it is true for two. + +### 11.2 Stability — the priority inside Track A was wrong + +Drafted as *"A1 is three lines; A2 is the one that says something true"*, which +read as though A1 were the safe first step. Measurement (§A0) inverted it: `ar +rcs` with no members exits 0, so the static-library form of this defect is +**silent today**, and A1 does not touch it. A1 alone would also make the shared +form quieter without making it correct. §2 now leads with A0 and states that A1 +must not land without A2. + +### 11.3 Elegance — the simpler B was considered and rejected on merit + +A single global action phony needs no `BuildAction::packageName` and mirrors +`stagedOrderOnly` exactly. Rejected: `include_dir` colours only the declaring +package's TUs, so cross-package ordering would encode a dependency that does not +exist, and mcpp's builds are latency-bound so the false edge lands on the +critical path. Recorded in §3 rather than left implicit, because "why not the +obvious simpler thing" is the question a reviewer will ask. + +Conversely, elegance *added* B5: seven hand-edited sites guarded by a +manifest-scanning check is a smaller thing to maintain than seven sites guarded +by care. + +### 11.4 User experience — the diagnostics are the deliverable + +Every one of these defects is, from a user's seat, a bad message. So the messages +are specified in the plan rather than left to implementation: A2 names the target +and what matched nothing; C1 names the package and the directory contents; X2 +names the store entry that was matched instead. All three follow mcpp's existing +shape — name the thing, name the reason, name the way out — which the runtime +closure validator already demonstrates and which §7 quotes in full as the +standard to meet. + +### 11.5 Compatibility — every behaviour change is now enumerated + +| change | today | after | who notices | +|---|---|---|---| +| A2 | empty `.a` built, build succeeds | error naming the target | a package with zero sources — broken already, silently | +| A2 | `/bin/sh: 1: -shared: not found` | error naming the target | same | +| B2 | header-only action never runs | runs, ordered | anyone who worked around it by running the generator eagerly — their build still works, one extra edge | +| B4 | empty generated header | *file not found* | a project relying on the placeholder; better error, still a change | +| C1 | `.mcpp_ok` written unconditionally | withheld when nothing was installed | type-only packages re-check each build (cheap); see §4 scoping | +| X1 | collision skips `install()` | installs correctly | **every xlings user**, not just mcpp's — see §10 | + +The one with the widest blast radius is X1, and it is in the other repo. §10's +last bullet stands: xlings should check whether any descriptor depends on being +skipped. + +### 11.6 Seamless upgrade — the original plan fixed only half the population + +`is_install_complete` is marker-only (`install_integrity.cppm:135-150`), so a +store already poisoned by #533 carries a `.mcpp_ok` that short-circuits forever. +The drafted C1 guarded only the *write* site, which helps users who have not yet +hit the bug and does nothing for those who have — the wrong half, since the +people who have hit it are the ones who filed the issue. C1 now specifies the +fast-path re-check as well. Nobody has to know to delete a directory. + +The xlings floor upgrade needs no user action: `acquire_xlings_binary` replaces a +vendored xlings strictly older than the pin, and looks before it leaps. + +### 11.7 Cross-platform — verified, one asymmetry found + +- A1/A2/B/C are platform-neutral; `||` is ninja syntax, not shell. +- On the MSVC dialect `separateLinker` is true, so `cxx_shared` is used and the + `$cc` gap cannot arise there — A1 is a no-op on Windows, A2 is not. +- The **asymmetry**: `need_ios_init_shim` (macOS, static libc++) defines `cc` + where Linux would not, so the same defective input can take different paths per + host. A2 removes the divergence by refusing before either path is chosen — + another reason to treat A2, not A1, as the fix. +- New e2e must be reachable: `# requires: gcc` is the token with 55 existing + tests and it runs on both Linux shards. `run_all.sh:153` already warns that + declaring a token is part of adding it. + +### 11.8 Consistency — one mechanism, not two + +`stagedOrderOnly` and the action phony are the same idea (aggregate into a phony, +attach order-only, one word per edge — mcpp#274's reason). They must share one +code path — a single `order_only_for(cu)` returning both — rather than two +parallel strings appended at the same seven sites. Folded into §3. + +Likewise C1's predicate lives beside `kInstallMarker`, used by both call sites, +rather than open-coded twice; and X1 extracts `catalog.cppm:302`'s expression so +the store is asked the same way in both places. In all three cases the defect +being fixed *was* a second derivation of a decision that already existed +elsewhere, so a fix that adds a third would be self-defeating. + +### 11.9 Test coverage — the plan was under-specified in one place + +§8's criteria were adequate for B and A, and thin for C. Added: e2e 318 must +assert the **fast-path** case too — a store seeded with a poisoned `.mcpp_ok` +must heal on the next build — because that is the half of C1 that §11.6 added +and an assertion on the write path alone would pass without it. + +Confirmed as drafted: the denominator requirement (a bare equality passes when +both sides are zero, which is today's state), asserting header **content** rather +than existence (existence passes against the 0-byte placeholder), and asserting +B3 on the produced artifact rather than a log line. + +### 11.10 What the review did not change + +- §1's ordering constraint and §6.2's recommendation not to re-pin the index. + Re-examined; the reasoning holds and is the most consequential judgement here. +- The decision to keep `Source` outputs out of `actionDefaults` (§3): reaching + them two ways is how the soname aliases went missing in 0.0.104. +- §7's three amendments to #532. diff --git a/.agents/docs/2026-08-30-issues-532-533-534-analysis.md b/.agents/docs/2026-08-30-issues-532-533-534-analysis.md new file mode 100644 index 00000000..090defcd --- /dev/null +++ b/.agents/docs/2026-08-30-issues-532-533-534-analysis.md @@ -0,0 +1,443 @@ +# Three issues, measured: #532, #533, #534 + +> Status: analysis. No code changed. +> Baseline: `origin/main` @ `ab1da5d` (the working tree at the time of writing +> was 31 commits behind and predates the `modules/` split, so every anchor +> below was read from a detached worktree at `origin/main`, not from `src/`). +> Measured with: mcpp `2026.8.28.2`, xlings `2026.8.30.1`, gcc 16.1.0, +> ninja 1.12.1, Linux x86_64. +> Reproductions: five, all in this document, all reduced to inputs that need +> nothing from the index. + +--- + +## 0. Summary + +The three issues are not equally well diagnosed, and two of them contain a +defect that is larger than the one they report. + +| | filed as | what it is | +|---|---|---| +| **#534** | an intermittent ordering race in dependency packages | a **deterministic** failure in *any* package, root included, plus a separate genuine race, plus a documented flag with no reader | +| **#533** | a namespace-blind store lookup | correct, and the lookup is in **xlings**; mcpp contributes two independent defects that make it sticky and unreadable | +| **#532** | an example answering #527 §1 | technically sound; one load-bearing sentence of its rationale is not what the implementation does, and the correction *strengthens* the argument | + +A shape common to all three: **a value is computed correctly and then not +consulted.** `action.blocking` is parsed and never read. `PlanNode::namespaceName` +sits in the same struct as the key that omits it. `mcpp` computes a +namespace-qualified store path and then accepts an unqualified proof that it +was filled. + +--- + +## 1. #534 — `build.mcpp` action ordering + +### 1.1 The filed diagnosis is wrong for the primary case + +The issue concludes *race, intermittent, parallelism-dependent*, from this +evidence: the build failed with undeclared identifiers, and afterwards the +generated header was on disk. The inference "`action` 也执行了" does not follow. + +`prepare_actions` materialises a **zero-byte placeholder** for every output of +every `Source` action, headers included, with no extension filter +(`modules/buildmcpp/src/directives.cppm:795`). The file exists because mcpp +created it empty — not because the generator ran. + +### 1.2 D1 — a Source action whose outputs are all headers never runs + +Three independent facts compose: + +1. `src/build/prepare.cppm:4255` — a Source output that is not a translation + unit is skipped when the action's outputs are adopted into the compile set. + The filter is `is_compilable_output` + (`modules/buildmcpp/src/directives.cppm:770`), false for + `SourceKind::Header` and `SourceKind::Other`. The comment names the intent + exactly: *"Companion outputs (protoc's .pb.h next to its .pb.cc) are produced + by the edge but are NOT translation units."* +2. `src/build/ninja_backend.cppm:2173` — `Source` and `Object` roles are + excluded from `actionDefaults`, so the node does not enter `default`. The + stated reason holds only under the assumption in (3). +3. `mcpp-requested-goals` aggregates objects and link outputs; it never names an + action node. + +So the edge exists in the manifest and **nothing can reach it**. The comment at +`ninja_backend.cppm:2090` states the assumption that fails: + +> *Ordering needs no special handling: a Source action's outputs ARE the compile +> edge's inputs …* + +True for a generated `.cpp`. False for a generated `.h`, which is never an edge +input — it is reached only through `-I`, and the depfile that would record it +does not exist until after a compile has already succeeded. + +**Reproduction** (root project, no dependencies, no index): + +``` +mcpp.toml : [package] name = "hdrgen" +src/main.cpp: #include "gen.h" → uses ANSWER +build.mcpp : action role="source", single output out/gen.h, include_dir(out) +``` + +``` +error: 'ANSWER' was not declared in this scope +``` + +``` +$ stat -c %s target/.build-mcpp/out/gen.h +0 # the placeholder, not the output + +$ grep -n 'gen\.h' build.ninja +101:build /…/out/gen.h : mcpp_action_0 # the node exists +$ grep -c ':.*gen\.h' build.ninja +0 # nothing consumes it +$ grep '^default' build.ninja +default bin/hdrgen # nothing defaults it + +$ ninja -n -d explain | grep -c 'mcpp_action_0' +0 # ninja never plans it +$ ninja -t targets all | grep gen.h +/…/out/gen.h: mcpp_action_0 # …though it is in the manifest + +$ ninja /…/out/gen.h # name it explicitly and it works +[1/1] GENERATE genhdr → 18 bytes, correct content +``` + +Five consecutive `mcpp build` runs, deleting the header before each: **0 bytes +every time.** This is not a race. It never runs. + +This also refutes the issue's §"为什么 protoc 那个例子没暴露": the root project is +not safe. It has the identical defect the moment an action's outputs are all +headers. + +### 1.3 D2 — the genuine race, which is the companion shape + +When the action emits `p.cc` **and** `p.h`, the `.cc` is adopted, so the edge is +reachable and the compile of `p.cc` is correctly ordered after it. A *different* +TU that includes `p.h` is not. + +**Reproduction** — same action as `tests/e2e/188_build_actions.sh` §2c, with a +`sleep 2` in the generator and `src/main.cpp` including `p.h`: + +``` +error: 'paired' was not declared in this scope # first build +Finished dev … in 0.04s → PAIRED=13 # second build, nothing changed +``` + +Fails then passes with no input change — the intermittent signature the issue +describes, now on demand. + +`188_build_actions.sh` contains this exact action but its `main.cpp` does not +include `p.h`, and its only assertions are that the build succeeds and that no +`object path collision` appears. The ordering is never exercised. Line 178's +comment — `// companion: produced, NOT compiled` — describes precisely the +output whose ordering is untested. + +### 1.4 D3 — `action.blocking` has no reader + +Found while tracing D1/D2. The flag is: + +| | | +|---|---| +| typed | `modules/manifest/src/types.cppm:331` — *"Check only: make compilation wait for this to pass."* | +| emitted by the helper | `src/build/hostprogram.cppm:126` | +| parsed | `modules/buildmcpp/src/directives.cppm:721` | +| documented, EN | `docs/07-build-mcpp.md:315` — *"set `blocking = true` to gate it"* | +| documented, ZH | `docs/zh/07-build-mcpp.md:281` | +| demonstrated | `examples/08-build-rules/rules-tidy/src/rules-tidy.cppm` | +| **read** | **nowhere** | + +The only order-only (`||`) emission in the entire ninja backend is +`stagedOrderOnly` at `ninja_backend.cppm:1534`, which is BMI staging. There is +no mechanism by which `blocking = true` can gate anything. It is a documented, +exemplified no-op — and it names exactly the mechanism #534 asks for. + +### 1.5 Fix shape + +One change covers D1, D2 and D3: **per-package, aggregate the package's action +outputs into a phony, and give every compile edge of that package `|| `.** +That is what the issue proposes, and it is right. Two additions: + +- A Source action whose outputs are *all* non-compilable, and which nothing else + makes reachable, should be **refused or warned at plan time**. Without it, D1's + silent form returns in a new shape the moment the phony's membership rule + changes. +- Reconsider materialising placeholders for non-compilable outputs + (`directives.cppm:795`). The scanner never reads a header. The placeholder's + only present effect is to convert "the generator did not run" into "the header + is empty", which is what cost this issue its diagnosis. + +### 1.6 Criteria the fix needs + +- `grep -cE '^build obj/.*\.o *:.*\|\|' build.ninja` **with a denominator**: it + must equal the number of compile edges in a package that declares ≥1 action, + and that count must be asserted separately — otherwise the assertion passes + when both are zero. +- The generated header is **non-empty and has the expected content** after a + cold build, at `-j1` *and* at high parallelism. Size alone is the criterion + that distinguishes the placeholder from the output. +- A header-only Source action builds. This is the case with no coverage today. +- `blocking = true` produces the `||`, and a failing blocking check prevents the + object from being produced. Assert on the artifact, not on a log line. + +--- + +## 2. #533 — the store name collision + +The issue is correct. The chain has three links in two components, and the fix +for each is different. + +### 2.1 L1 — root cause, in xlings: `installer.cppm:907` + +```cpp +// Default: check xvm version database when no installed hook +else if (!payloadInstalled) { + auto db = Config::versions(); + auto resolved = xvm::match_version(db, node.name, node.version); + if (!resolved.empty()) { + log::debug("{} already installed in xvm (version {})", node.name, resolved); + payloadInstalled = true; // ⇒ install() is skipped + } +} +``` + +- `node.name` is the **bare short name**. The same `PlanNode` carries + `namespaceName` and `canonicalName` + (`src/core/xim/libxpkg/types/type.cppm:34-44`). The qualified identity is + present at the call site and discarded. +- `xvm::match_version` does `db.find(target)` (`src/core/xvm/db.cppm:99`); the + VersionDB is keyed by program name. That is the right key for xvm's own + question — *which version of the program named X is active* — and the wrong + key for *is package `:@`'s payload installed*. Two namespaces, + one word. +- The **primary** path is already correct: `src/core/xim/catalog.cppm:302` + computes `match.installed` from + `storeRoot / package_store_name(ns, name) / version`. Only this **fallback** + is blind, and the fallback is what a source-build package with no `installed` + hook hits. +- The report is `log::debug` — invisible at default verbosity. That is the + silence the issue describes. + +The fix is to make the fallback ask the store the way `catalog.cppm:302` already +does. Two answers to one question should be one function. + +### 2.2 L2 — mcpp makes it permanent: `package_fetcher.cppm:1095` + +```cpp +if (inst->exitCode == 0 && std::filesystem::exists(verdir)) { + mcpp::fallback::mark_install_complete(verdir); // writes .mcpp_ok +``` + +mcpp asked the right question — `verdir` is namespace-qualified at +`package_fetcher.cppm:986` (`{indexName}-x-{packageName}/{version}`) — and then +accepted the wrong proof. The callee's exit code plus directory existence stands +in for "the descriptor's `install()` produced its tree". + +The consequence is worse than one bad build. From the second build onward, +step 1 of the resolution chain (`is_install_complete`, +`package_fetcher.cppm:1049`) short-circuits on the `.mcpp_ok` mcpp itself wrote, +so the wrong state survives every rebuild and every cache clear that does not +delete the store. + +This is the third recurrence of the same gap: **`.mcpp_ok` proves a process +exited 0, not that an artifact is correct.** + +A minimum viable check exists and is cheap. In the reported case the version +directory contained `.mcpp_ok`, `.xpkg-install.json` and `mcpp_generated/` — +that is, **nothing except what mcpp and xlings wrote themselves**. Requiring at +least one entry that neither party authored catches exactly this signature +without needing to know the descriptor's tree shape. + +### 2.3 L3 — mcpp's error names something unrelated: the zero-input link edge + +The reported `/bin/sh: 1: -shared: not found` reproduces from a **pure mcpp +input**, with no xlings and no index involved: + +```toml +[package] +name = "ghostlib" +version = "0.1.0" + +[targets.ghostlib] +kind = "shared" # and src/ is empty +``` + +``` +error: build failed +failed: bin/libghostlib.so + -shared @bin/libghostlib.so.rsp -o bin/libghostlib.so … +/bin/sh: 1: -shared: not found +``` + +``` +$ grep -nE '^build .*shared' build.ninja +86:build bin/libghostlib.so : c_shared # zero inputs +$ grep -nE '^(cc|cxx) +=' build.ninja +5:cxx = …/bin/g++ # cc is never defined +$ stat -c %s bin/libghostlib.so.rsp +0 +``` + +Mechanism: with zero compile units, `need_c_rule` is false, so `cc` is not +emitted (`ninja_backend.cppm:594`); `unit_needs_cxx_runtime` is false, so the +rule chosen is `c_shared` (`ninja_backend.cppm:1190`); `$cc -shared …` expands to +` -shared …` and the shell executes `-shared`. + +**The file already states the rule that prevents this, 26 lines below**, for the +other variable of the same rule (`ninja_backend.cppm:615-620`, mcpp#426): + +> *ALWAYS emitted, even when identical — `c_link` references `$c_ldflags`, and a +> conditional definition would make an empty link line the failure mode …* + +`c_shared` and `c_link` reference `$cc` **and** `$c_ldflags`. The rule was +written down and applied to one of the two. Two independent fixes, both cheap: + +1. **Refuse a link unit with zero inputs at plan time**, naming the target and + the patterns that matched nothing. There is no such guard anywhere today. +2. **Emit `cc` unconditionally**, exactly as `c_ldflags` is, for the reason + already recorded at line 617. + +Either one alone converts this failure from a shell error three layers from its +cause into a statement about the target. (1) is the one that says something +true; (2) is the one that stops the class. + +### 2.4 L4 — the secondary note is confirmed + +`warning: src/ghostlib.cppm: lib target without conventional lib root` reproduces +verbatim in the same project. `src/modgraph/validate.cppm:157`, gated on +`has_lib_target` (`modules/manifest/src/types.cppm:1317`), which is true for any +`Library` or `SharedLibrary` target. The convention candidate is +`src/.cppm`; a pure-C library has none and can never have one. + +The predicate is *"does this target produce a library"*; the property it wants is +*"is this a C++ module library"*. A library target whose sources contain no +module-extension file should not be asked for a lib root. + +### 2.5 Blast radius is not hypothetical + +The store on this machine already holds **20 short names occupying two or more +namespaces** — `libpng`, `expat`, `zlib`, `cairo`, `fontconfig`, +`linux-headers`, `nasm`, `ncurses`, `mcpp`, `xlings`, and more. The rule #533 +infers ("no compat package may share `@` with any xim +package") is already load-bearing across the installed base; it holds today only +because the versions happen to differ. + +--- + +## 3. #532 — the graphics-stack example + +Not a defect report; the question is whether the claims hold and what merging +costs. + +### 3.1 The technical answer is right + +Declaring four packages resolves an 11-entry runtime closure that includes three +libraries (`libexpat`, `libffi`, `libGLdispatch`) no manifest names. #527 §1.4 +asserted that this cascade cannot be resolved under a private loader. It can. +That is a real answer to a real question, and it is worth landing. + +### 3.2 One load-bearing sentence is not what the implementation does + +> *`-L/usr/lib -lgbm` 不是缺失的支持,而是错误用法,mcpp 在构建期就明说了。* + +The guard exists and its message is good — but it is **closure-based, not +host-path-based**. It fires when the resulting artifact would not start, and is +silent otherwise. Two measurements: + +**Host-only library** — `-L/usr/lib/x86_64-linux-gnu -lcap`: + +``` +error: runtime closure validation failed (proven Linux ELF defect) +runtime closure for …/bin/hostonly cannot be satisfied: libcap.so.2 not found +on the search path this artifact will actually use. + Its PT_INTERP is a private loader, so the host's /usr/lib is NOT + consulted — the program will fail to start with "cannot open shared + object file". + Fix: install the provider into the selected SubOS … +``` + +Refused, precisely, with the way out. Exactly as #532 claims. + +**The graphics case** — `-L/usr/lib/x86_64-linux-gnu -lgbm`, on a machine that +also has mesa in the SubOS: + +``` + Compiling hostlink v0.1.0 (.) + Finished dev [unoptimized + debuginfo] in 0.07s +``` + +Silent, exit 0. And the artifact: + +``` +$ readelf -d … | grep NEEDED +libgbm.so.1 ← ABI taken from /usr/include, /usr/lib +$ "$(private loader)" --list … +libgbm.so.1 => /home/…/.mcpp/registry/subos/default/lib/libgbm.so.1 +``` + +**Link-time provider and runtime provider are different builds, and nothing says +so.** The check cannot fire, because the soname exists on both sides. + +So on precisely the class of machine this example targets — one with Mesa +installed — #527's reporter gets silence, not a refusal. The correction +strengthens #532's case rather than weakening it: the reason not to write +`-L/usr/lib` is not that mcpp refuses it. It is that mcpp **cannot** refuse it +when the name resolves on both sides, and you are then shipping an artifact +compiled against one library and loaded against another. + +Recommended: reword that paragraph, and consider filing the silent link-A/load-B +case separately — it is the part of #527 §1 that is actually a defect. + +### 3.3 The zero-host measurement needs a different machine + +The 11-entry closure was measured on the development machine, where +`registry/subos/default/lib` is a shared, accumulating view — the same directory +that silently supplied `libgbm.so.1` in §3.2. A `--list` there cannot distinguish +*"the closure is satisfied by the four declared packages"* from *"the closure is +satisfied by whatever else this machine has installed"*. + +The claim is worth keeping; it needs re-measuring somewhere holding only the +declared packages. This is the standing house rule — a sandbox is the only thing +that verifies a published artifact — applied to a closure claim. + +### 3.4 CI coverage is the merge risk + +Examples are covered individually (`tests/e2e/312_build_rules_example.sh` builds +example 08). Example 09 opens `/dev/dri/renderD128`, which no CI runner has, and +its four dependencies are index packages, so it needs network and a current +index even to configure. + +Decide explicitly, and say which in the README: **build-only in CI with the +device work behind a runtime guard**, or **documented as uncovered**. An example +with neither rots silently, and this one has four external version pins that will +drift. + +The Vulkan gap is recorded honestly in the README. No objection. + +--- + +## 4. What the three share + +1. **A value is computed and then not consulted.** `action.blocking` — parsed, + emitted, documented in two languages, exemplified, never read. + `PlanNode::namespaceName` — sits in the struct whose key omits it. +2. **A criterion aimed at the wrong object.** `.mcpp_ok` written from the + callee's exit code. The lib-root check keyed on *produces a library* rather + than *is a module library*. +3. **A rule stated for one instance instead of enumerated.** `c_ldflags` is + emitted unconditionally for a documented reason; `$cc`, referenced by the + same two ninja rules, is not. +4. **The failure reports something unrelated.** #533's user saw a linker + command-line error for a package-identity bug. #534's user saw a compiler + error for an unreachable graph node, and reasonably concluded "race" from a + file that mcpp had created empty. + +## 5. Suggested order + +| | change | why first | +|---|---|---| +| 1 | mcpp: refuse a zero-input link unit at plan time; emit `cc` unconditionally | smallest, self-contained, and it is what turns #533's next occurrence into a readable message. Independent of xlings. | +| 2 | xlings `installer.cppm:907`: key the fallback on the qualified identity | the root cause of #533. Unblocks index packages that want the ecosystem's own upstream version. | +| 3 | mcpp: per-package action phony + `||` on that package's compile edges; wire `blocking` | #534 D1+D2+D3 in one change. Needs the criteria in §1.6, especially the denominator. | +| 4 | mcpp `package_fetcher.cppm:1095`: require one entry neither mcpp nor xlings wrote before `.mcpp_ok` | defence in depth for the class, not just this instance | +| 5 | mcpp: lib-root check only for module libraries | cosmetic, but it propagates to every consumer of a C package | +| 6 | #532: reword §"为什么是示例", re-measure the closure in a sandbox, decide CI posture | contribution, not a defect | diff --git a/docs/07-build-mcpp.md b/docs/07-build-mcpp.md index cf06e1ab..eca7eef4 100644 --- a/docs/07-build-mcpp.md +++ b/docs/07-build-mcpp.md @@ -311,14 +311,27 @@ attach: | `role` | Outputs | Ordering | Typical | |---|---|---|---| -| `source` | join the compile set | the compile edge consumes them | protoc, a transpiler | -| `check` | a stamp file, written by mcpp | runs **alongside** compilation (set `blocking = true` to gate it) | clang-tidy, a format or ABI check | +| `source` | compilable ones join the compile set; the rest are produced but not compiled | **every compile edge of the declaring package waits for them** | protoc, a transpiler, a protocol/IDL generator | +| `check` | a stamp file, written by mcpp | runs **alongside** compilation; `blocking = true` makes the package's compile edges wait for it | clang-tidy, a format or ABI check | | `object` | join the **link** set | the link edge consumes them | a resource compiler, `objcopy` embedding a blob, a generated `.def`, a pre-built `.o` | | `artifact` | a new file | its *inputs* are link outputs, so it runs after the link | codesign, packaging, size budgets | -No phase machinery is involved: ninja's own file dependencies do the -sequencing, which is also why an `artifact` action cannot double-apply itself -the way a naive "post-build hook" would. +No phase machinery is involved. `object` and `artifact` are sequenced by +ninja's own file dependencies — which is also why an `artifact` action cannot +double-apply itself the way a naive "post-build hook" would. `source` and a +blocking `check` are sequenced by an order-only edge from the declaring +package's compile edges to that package's action outputs. + +> **Why `source` needs the edge (mcpp 2026.8.30.2+).** A generated `.cpp` +> becomes an input of the edge that compiles it, so it was ordered for free. A +> generated **header** never does: it is reached through `-I`, and the depfile +> that would record it does not exist until a compile has already succeeded. +> Before this, an action whose outputs were all headers had a node in +> `build.ninja` that nothing could reach — not `default`, not the goal set, no +> consuming edge — so it never ran, and what the compiler read was the empty +> placeholder mcpp writes for a declared output. The ordering is **per +> package**, because `include_dir` colours only the declaring package's own +> translation units. **A check's command does not have to write its stamp** (mcpp 2026.8.29.1+). The verdict is the exit code; the stamp is bookkeeping the graph needs, and diff --git a/docs/zh/07-build-mcpp.md b/docs/zh/07-build-mcpp.md index cdf9db11..222644d9 100644 --- a/docs/zh/07-build-mcpp.md +++ b/docs/zh/07-build-mcpp.md @@ -277,13 +277,22 @@ int main() { | `role` | 输出 | 顺序 | 典型 | |---|---|---|---| -| `source` | 进编译集 | 编译边消费它们 | protoc、转译器 | -| `check` | 一个 stamp 文件,由 mcpp 写入 | **与编译并行**(`blocking = true` 才前置) | clang-tidy、格式/ABI 检查 | +| `source` | 可编译的进编译集,其余只产出、不编译 | **声明它的那个包的每条编译边都等它** | protoc、转译器、协议/IDL 生成器 | +| `check` | 一个 stamp 文件,由 mcpp 写入 | 与编译并行;`blocking = true` 让该包的编译边等它 | clang-tidy、格式/ABI 检查 | | `object` | 进**链接**集 | 链接边消费它们 | 资源编译器、`objcopy` 嵌 blob、生成的 `.def`、预编译 `.o` | | `artifact` | 一个新文件 | 它的**输入**是链接产物,所以在链接之后跑 | 签名、打包、size budget | -全程不涉及任何 phase 机制:顺序由 ninja 自己的文件依赖决定 —— 这也是为什么 -`artifact` 不会像朴素的「post 构建钩子」那样把自己重复施加一遍。 +全程不涉及任何 phase 机制。`object` 与 `artifact` 由 ninja 自己的文件依赖定序 —— +这也是为什么 `artifact` 不会像朴素的「post 构建钩子」那样把自己重复施加一遍。 +`source` 与 blocking 的 `check` 则由一条 order-only 边定序:从声明它的那个包的 +编译边,指向该包的 action 产物。 + +> **`source` 为什么需要这条边(mcpp 2026.8.30.2+)。** 生成的 `.cpp` 会成为编译它 +> 那条边的输入,所以顺序是白得的。生成的**头文件**永远不会:它是通过 `-I` 找到的, +> 而能记录它的 depfile 要等到某次编译成功之后才存在。在此之前,一个产物全是头文件的 +> action 在 `build.ninja` 里有节点却无人可达 —— 不在 `default`、不在 goal 集、没有 +> 任何边消费它 —— 于是它从不执行,而编译器读到的是 mcpp 为已声明产物写下的那个空占位 +> 文件。这条边**按包**划分,因为 `include_dir` 只染色声明它的那个包自己的 TU。 **check 的命令不必自己写 stamp**(mcpp 2026.8.29.1+)。判定是退出码,stamp 是**构建图** 需要的记账;命令成功时由 mcpp 创建它。在此之前每个 check 都需要一个包装脚本去 touch diff --git a/mcpp.toml b/mcpp.toml index ffa8ad25..411b6723 100644 --- a/mcpp.toml +++ b/mcpp.toml @@ -1,6 +1,6 @@ [package] name = "mcpp" -version = "2026.8.29.1" +version = "2026.8.30.1" description = "Modern C++ build & package management tool" license = "Apache-2.0" authors = ["mcpp-community"] diff --git a/modules/buildmcpp/src/directives.cppm b/modules/buildmcpp/src/directives.cppm index ed192bfb..e61825f8 100644 --- a/modules/buildmcpp/src/directives.cppm +++ b/modules/buildmcpp/src/directives.cppm @@ -387,8 +387,17 @@ std::string action_error(const Directives& d); // Never truncates an existing file: after the first build the real content is // there, and rewriting it would make ninja think the input changed on every // prepare. +// +// ⚠️ ONLY FOR OUTPUTS THAT ARE TRANSLATION UNITS, which is why this needs the +// table. A placeholder exists so the SCAN has something to read, and the scan +// never reads a header — but writing one anyway turned "the generator did not +// run" into "the header is empty", and mcpp#534 was diagnosed as a race for +// exactly that reason: the file was on disk, so the action looked like it had +// run. A missing file is the honest report, and after the ordering fix the +// generator runs before anything reads it either way. void prepare_actions(std::vector& actions, - const std::filesystem::path& pkgRoot); + const std::filesystem::path& pkgRoot, + const mcpp::ExtensionTable& extensions); // Does this action output belong in the COMPILE set? // @@ -773,7 +782,8 @@ bool is_compilable_output(const fs::path& p, const mcpp::ExtensionTable& t) { } void prepare_actions(std::vector& actions, - const fs::path& pkgRoot) { + const fs::path& pkgRoot, + const mcpp::ExtensionTable& extensions) { for (auto& a : actions) { auto absolutize = [&](std::vector& v) { for (auto& p : v) { @@ -788,6 +798,11 @@ void prepare_actions(std::vector& actions, if (a.role != mcpp::manifest::BuildAction::Role::Source) continue; for (auto const& o : a.outputs) { if (o.find("${mcpp.") != std::string::npos) continue; + // A placeholder exists so the scan has a translation unit to read. + // A header is not one — nothing scans it, and the empty file it + // used to leave behind is what made a generator that never ran + // look like one that had (mcpp#534). + if (!is_compilable_output(o, extensions)) continue; std::error_code ec; fs::path p(o); if (fs::exists(p, ec)) continue; // real content already there diff --git a/modules/manifest/src/types.cppm b/modules/manifest/src/types.cppm index 9d3ec6a0..551f5f16 100644 --- a/modules/manifest/src/types.cppm +++ b/modules/manifest/src/types.cppm @@ -300,6 +300,18 @@ struct BuildAction { enum class Role { Source, Check, Object, Artifact }; std::string id; // diagnostics + edge naming + // Which package's `build.mcpp` declared this. Filled by the engine when + // actions are collected into the plan, NOT by the build program — the + // program does not know, and the engine already does. + // + // Load-bearing, not bookkeeping: the ordering edge an action needs is + // scoped to the declaring package, because `include_dir` colours only that + // package's own translation units. A build-wide ordering would express a + // dependency that does not exist and put it on the critical path of a + // build whose wall clock is dominated by one. Spelled the same way + // `CompileUnit::packageName` is (`qualified_package_name`), because the + // two are matched against each other. + std::string packageName; Role role = Role::Source; std::vector inputs; // absolute or package-relative std::vector outputs; // ditto; declared, see INV-D diff --git a/modules/versioning/src/version.cppm b/modules/versioning/src/version.cppm index ede229c7..d8bd4e53 100644 --- a/modules/versioning/src/version.cppm +++ b/modules/versioning/src/version.cppm @@ -31,6 +31,6 @@ import std; export namespace mcpp { -inline constexpr std::string_view MCPP_VERSION = "2026.8.29.1"; +inline constexpr std::string_view MCPP_VERSION = "2026.8.30.1"; } // namespace mcpp diff --git a/src/build/ninja_backend.cppm b/src/build/ninja_backend.cppm index 1adce255..23a3f0a9 100644 --- a/src/build/ninja_backend.cppm +++ b/src/build/ninja_backend.cppm @@ -65,6 +65,18 @@ std::string emit_ninja_string(const BuildPlan& plan); std::string filter_ninja_output(std::string_view output, std::span commandPrefixes); +// Emitter self-check: every ninja rule's command must begin with a program. +// +// Exported so the invariant can be stated against hand-written manifests as +// well as generated ones — the interesting cases (a rule referencing a +// variable nothing defines) are ones `emit_ninja_string` is supposed never to +// produce, so a test that could only reach it through a BuildPlan could not +// assert the checker works. Returns the diagnostic, or nullopt when every +// rule is well-formed. See the definition for why this is not the more +// obvious "no undefined variables" check. +std::optional check_rule_commands_name_a_program( + const std::string& manifest); + // Advice appended to a failed build whose linker output names a replaceable // function nothing in the graph defines. Empty when there is nothing to add. // @@ -420,6 +432,32 @@ runtime_env_for_dirs(const std::vector& dirs) { return std::pair{std::move(key), std::move(value)}; } +// ── mcpp#534: which actions gate compilation, and what the gate is called ── +// +// Both the emitter and the post-emit self-check need these, and they are the +// same question asked twice — so they are functions, not two lambdas that +// happen to agree today. The whole defect being fixed here was a second +// derivation of an ordering decision drifting away from the first. + +// `Object` outputs are link inputs and `Artifact` inputs are link outputs, so +// ninja's file dependencies already order both. `Source` may produce a header, +// which is never an edge input, and a `Check` gates compilation exactly when it +// says it does — the `blocking` flag, which nothing read before mcpp#534. +bool action_precedes_compilation(const mcpp::manifest::BuildAction& a) { + return a.role == mcpp::manifest::BuildAction::Role::Source + || (a.role == mcpp::manifest::BuildAction::Role::Check && a.blocking); +} + +// A phony per package. Not a path — ninja resolves it in the build dir, and no +// rule produces a file by this name (same contract as `kGoalPhony`). +std::string action_phony_name(std::string_view pkg) { + std::string s = "mcpp-actions-"; + for (char c : pkg) + s += (std::isalnum(static_cast(c)) || c == '.' + || c == '_' || c == '-') ? c : '_'; + return s; +} + } // namespace std::string link_failure_advice(std::string_view output) { @@ -591,9 +629,28 @@ std::string emit_ninja_string(const BuildPlan& plan) { const bool need_ios_init_shim = flags.needsStreamInitShim; append(std::format("cxx = {}\n", escape_ninja_path(flags.cxxBinary))); append(std::format("cxxflags = {}\n", flags.cxx)); - if (need_c_rule || need_asm_rule || need_ios_init_shim) { // asm_object drives the C compiler too - append(std::format("cc = {}\n", escape_ninja_path(flags.ccBinary))); - } + // ALWAYS emitted, for the reason `c_ldflags` is, 26 lines below — and the + // two are referenced by the SAME rules. mcpp#426 recorded that reasoning + // for one variable of `c_link`/`c_shared` and left the other conditional + // on `need_c_rule || need_asm_rule || need_ios_init_shim` (asm_object + // drives the C compiler too), which is a property of the COMPILE set and + // says nothing about which LINK rules exist. + // + // mcpp#533 is what that cost. A package whose install was skipped has zero + // compile units, so `need_c_rule` is false; its shared-library unit still + // selects `c_shared`, `$cc` expands to nothing, and the shell is handed + // `-shared` as a program name: + // + // /bin/sh: 1: -shared: not found + // + // ⚠️ THIS IS NOT THE FIX FOR THAT, and must not be mistaken for one. It + // makes the message name the linker instead of the shell; the defect is + // that a link unit had no inputs, and `prepare.cppm` refuses that now. On + // its own this line would only make the shared case quieter — and the + // static case, which goes to `ar` and produces an empty archive with exit + // 0, it does not touch at all. `check_undefined_ninja_variables` below is + // what stops the class. + append(std::format("cc = {}\n", escape_ninja_path(flags.ccBinary))); if (need_c_rule || need_ios_init_shim) { append(std::format("cflags = {}\n", flags.cc)); } @@ -1535,6 +1592,51 @@ std::string emit_ninja_string(const BuildPlan& plan) { } } + // ── A package's generated inputs come before that package's compiles ──── + // + // mcpp#534. The comment on the action block below used to claim ordering + // needed no special handling, because "a Source action's outputs ARE the + // compile edge's inputs". That holds for a generated TRANSLATION UNIT and + // fails for a generated HEADER, which is never an edge input — it is + // reached through `-I`, and the depfile that would record it does not + // exist until a compile has already succeeded. Measured: an action whose + // only output was a header had a node in the manifest that nothing could + // reach, so ninja never ran it, and the compile read the zero-byte + // placeholder `prepare_actions` leaves behind. Five consecutive builds, + // same result — not a race, never run. + // + // ⭐ PER PACKAGE, not per build. `include_dir` colours only the declaring + // package's own TUs (docs/07-build-mcpp.md), so a generated header is + // visible to exactly one package and a build-wide phony would encode a + // dependency that does not exist. It would also land on the critical path + // of a build whose wall clock IS its critical path. + // + // Same mechanism as the staged-cache phony above, for the same reason + // (mcpp#274: one word per edge, not a list) — and deliberately the same + // code path, so a future edge kind cannot pick up one and miss the other. + // + // `action_precedes_compilation` and `action_phony_name` are file-scope + // functions rather than lambdas here, because `check_action_ordering` + // asks the identical questions after the manifest is written. + std::map> actionOutputsByPackage; + for (auto const& a : plan.actions) { + if (!action_precedes_compilation(a)) continue; + if (a.packageName.empty()) continue; // nothing to scope it to + auto& outs = actionOutputsByPackage[a.packageName]; + for (auto const& o : a.outputs) outs.push_back(escape_ninja_path(o)); + } + // The phony edges themselves are emitted after the action rules, below; + // ninja resolves the whole manifest before building, so a forward + // reference here is fine and keeps each block's emission local. + const auto order_only_for = [&](const CompileUnit& cu) { + auto it = actionOutputsByPackage.find(cu.packageName); + if (it == actionOutputsByPackage.end() || it->second.empty()) + return stagedOrderOnly; + auto phony = action_phony_name(cu.packageName); + return stagedOrderOnly.empty() ? " || " + phony + : stagedOrderOnly + " " + phony; + }; + if (dyndep) { // ── Phase 1: scan edges (one .ddi per TU). ────────────────────── // .ddi is placed beside the object so multi-version mangling can @@ -1555,7 +1657,7 @@ std::string emit_ninja_string(const BuildPlan& plan) { auto ddi = (cu.object.parent_path() / cu.source.filename()).string() + ".ddi"; ddi_paths.push_back(ddi); append(std::format("build {} : cxx_scan {}{}\n", escape_ninja_path(ddi), - escape_ninja_path(cu.source), stagedOrderOnly)); + escape_ninja_path(cu.source), order_only_for(cu))); // `-o` and `-fdeps-target` are DIFFERENT under the split shape and // must not share a variable. The scan writes a throwaway object to // `-o`, but the dyndep file has to bind the BMI edge — that is the @@ -1680,7 +1782,7 @@ std::string emit_ninja_string(const BuildPlan& plan) { std::string e = std::format("build {} : cxx_module_bmi {} | {}", bmi, escape_ninja_path(cu.source), it->second); - e += stagedOrderOnly; + e += order_only_for(cu); e += "\n dyndep = " + it->second + "\n"; e += " bmi_out = " + bmi + "\n"; e += " obj_out = " + obj + "\n"; @@ -1751,7 +1853,7 @@ std::string emit_ninja_string(const BuildPlan& plan) { out, rule, escape_ninja_path(cu.source), it->second); - e += stagedOrderOnly; + e += order_only_for(cu); e += "\n dyndep = " + it->second + "\n"; if (auto inc = local_include_flags(cu, dial); !inc.empty()) e += " local_includes =" + inc + "\n"; @@ -1775,7 +1877,7 @@ std::string emit_ninja_string(const BuildPlan& plan) { auto it = ddi_to_dd.find(ddi); if (it != ddi_to_dd.end()) { out_line += " | " + it->second; - out_line += stagedOrderOnly; + out_line += order_only_for(cu); out_line += "\n dyndep = " + it->second; // P2: set bmi_out for the copy_if_different logic in cxx_module. if (cu.providesModule) { @@ -1783,10 +1885,10 @@ std::string emit_ninja_string(const BuildPlan& plan) { } out_line += "\n"; } else { - out_line += stagedOrderOnly + "\n"; + out_line += order_only_for(cu) + "\n"; } } else { - out_line += stagedOrderOnly + "\n"; + out_line += order_only_for(cu) + "\n"; } if (auto includes = local_include_flags(cu, dial); !includes.empty()) out_line += " local_includes =" + includes + "\n"; @@ -1838,7 +1940,7 @@ std::string emit_ninja_string(const BuildPlan& plan) { out_line += std::format(" : {} {}", rule, escape_ninja_path(cu.source)); if (!implicit.empty()) out_line += " |" + implicit; - out_line += stagedOrderOnly; + out_line += order_only_for(cu); out_line += "\n"; if (auto includes = local_include_flags(cu, dial); !includes.empty()) out_line += " local_includes =" + includes + "\n"; @@ -2087,12 +2189,32 @@ std::string emit_ninja_string(const BuildPlan& plan) { // ── Declared build-graph nodes (`mcpp:action=`) ───────────────────────── // - // One rule + one edge per action. Ordering needs no special handling: a - // Source action's outputs ARE the compile edge's inputs, and an Artifact - // action's inputs ARE the link edge's outputs, so ninja's own file - // dependencies sequence everything. That is the entire reason this is a - // graph node rather than a phase — the alternative (a post hook) would - // have to re-derive the ordering by hand and would still lose incrementality. + // One rule + one edge per action. Ordering comes from two sources, and + // which one applies is a property of the ROLE: + // + // Object its outputs ARE the link edge's inputs + // Artifact its inputs ARE the link edge's outputs + // — for these two, ninja's own file dependencies sequence + // everything, and nothing else is needed. + // + // Source its outputs MAY be compile-edge inputs, and may equally be + // Check headers, which are never edge inputs at all + // — for these, the declaring package's compile edges take the + // action phony as an order-only prerequisite (see + // `order_only_for`, far above). + // + // ⚠️ THIS COMMENT USED TO CLAIM THE FIRST ROW FOR ALL FOUR. "A Source + // action's outputs ARE the compile edge's inputs" is true of a generated + // `.cpp` and false of a generated `.h`: `adoptActionOutputs` deliberately + // does not adopt a non-TU into the compile set, so nothing consumed the + // header, nothing defaulted it, and the node sat in the manifest + // unreachable. mcpp#534 was filed as an intermittent race; it was not one. + // The action never ran at all, and what the compiler read was the + // zero-byte placeholder `prepare_actions` writes. + // + // The reason this is a graph node rather than a phase is unchanged: a post + // hook would have to re-derive the ordering by hand and would still lose + // incrementality. std::string actionDefaults; for (std::size_t i = 0; i < plan.actions.size(); ++i) { auto const& a = plan.actions[i]; @@ -2164,18 +2286,41 @@ std::string emit_ninja_string(const BuildPlan& plan) { for (auto const& in : a.inputs) ins += " " + escape_ninja_path(in); append(std::format("build{} : mcpp_action_{}{}\n", outs, i, ins)); append("\n"); - // A Source action's outputs are already reachable through the compile - // edges that consume them, and an Object action's through the link edge - // that lists them. Check and Artifact outputs are terminal, so without - // this nothing would ever ask for them — and under explicit ninja goals - // (#274) an edge reachable only via `default` is skipped, which is - // exactly how the soname aliases went missing in 0.0.104. + // A Source action's outputs are reachable through the phony its + // package's compile edges name, and an Object action's through the + // link edge that lists them. Check and Artifact outputs are terminal, + // so without this nothing would ever ask for them — and under explicit + // ninja goals (#274) an edge reachable only via `default` is skipped, + // which is exactly how the soname aliases went missing in 0.0.104. + // + // A BLOCKING check is in the phony as well, and still belongs here: + // its verdict is wanted even in a build whose goal set names no object + // of that package. if (a.role != mcpp::manifest::BuildAction::Role::Source && a.role != mcpp::manifest::BuildAction::Role::Object) for (auto const& o : a.outputs) actionDefaults += " " + escape_ninja_path(o); } + // The phonies `order_only_for` promised, one per package that declares an + // action which must precede its compiles. Emitted here, after the edges + // that produce their members, purely for readability — ninja resolves the + // whole manifest before building anything. + // + // ⚠️ A Source action's outputs enter the graph ONLY through this edge. + // They are deliberately not in `default` (above), because being reachable + // two ways is how the 0.0.104 soname aliases went missing under explicit + // goals. So if this loop stops emitting, the header-only case of mcpp#534 + // comes straight back — `check_action_ordering` below is what says so. + if (!actionOutputsByPackage.empty()) append("\n"); + for (auto const& [pkg, outs] : actionOutputsByPackage) { + if (outs.empty()) continue; + append("build " + action_phony_name(pkg) + " : phony"); + for (auto const& o : outs) append(" " + o); + append("\n"); + } + if (!actionOutputsByPackage.empty()) append("\n"); + if (!plan.linkUnits.empty() || !actionDefaults.empty()) { std::string defaults; for (auto& lu : plan.linkUnits) { @@ -2235,6 +2380,216 @@ std::string append_goal_phony(std::string& manifest, // Scanning the emitted manifest rather than instrumenting each emit site is // deliberate — a new edge kind is then covered the day it is added, which is // precisely how the previous seven slipped through. +// Every compile edge of a package that generates its own inputs must wait for +// them (mcpp#534). +// +// ⭐ WHY A SCAN AND NOT CARE. The order-only string is appended at SEVEN call +// sites — the scan edge, three dyndep object edges, two static-mode object +// edges and the asm edge — and an eighth added later without it reintroduces +// the defect for that edge kind, silently, in exactly the shape that took a +// filed issue and a five-run experiment to characterise. `emit_ninja_string` +// knows both halves (which packages declare gating actions, and which objects +// belong to which package), so it can check its own output; being careful at +// seven sites is not a mechanism. +// +// Same argument, same call site and same shape as +// `check_inline_command_lengths`, whose comment says it outright: scanning the +// emitted manifest covers a new edge kind the day it is added. +// +// ⚠️ THE DENOMINATOR IS PART OF THE CHECK. "every edge that should carry it +// does" is vacuously true when no edge should, which is precisely today's +// state — so a package with gating actions and zero matched compile edges is +// itself the failure, not a pass. +std::optional check_action_ordering(const std::string& manifest, + const BuildPlan& plan) { + std::set gating; // packages with such actions + for (auto const& a : plan.actions) + if (action_precedes_compilation(a) && !a.packageName.empty() + && !a.outputs.empty()) + gating.insert(a.packageName); + if (gating.empty()) return std::nullopt; + + // object path -> owning package, for the packages that matter. + std::map ownerOf; + for (auto const& cu : plan.compileUnits) { + if (!gating.contains(cu.packageName)) continue; + if (cu.servedFromCache) continue; // staged, never compiled here + ownerOf.emplace(escape_ninja_path(cu.object), cu.packageName); + } + + std::map seen; // package -> edges checked + for (auto line : manifest | std::views::split('\n')) { + std::string_view l{line.begin(), line.end()}; + if (!l.starts_with("build ")) continue; + auto colon = l.find(" : "); + if (colon == std::string_view::npos) continue; + auto outs = l.substr(6, colon - 6); + // An edge may declare several outputs; any one of them being a + // tracked object makes the edge a compile edge for that package. + for (auto const& [obj, pkg] : ownerOf) { + if (outs != obj + && outs.find(obj) == std::string_view::npos) continue; + ++seen[pkg]; + auto phony = action_phony_name(pkg); + if (l.find(phony) == std::string_view::npos) + return std::format( + "internal: compile edge '{}' belongs to package '{}', " + "which generates its own inputs, but the edge does not " + "wait for '{}'.\n" + " A generated header is reached through -I and is " + "never an edge input, so without this ninja may compile " + "before the generator runs (mcpp#534).\n" + " This is a build-emitter defect; please report it.", + outs, pkg, phony); + break; + } + } + + for (auto const& pkg : gating) { + if (seen[pkg] > 0) continue; + // No compile edge of this package was found to check. That is either a + // package whose sources are all cache-staged — legitimate — or the + // emitter no longer producing what this check reads, which would make + // every assertion above pass by describing nothing. + bool anyLive = false; + for (auto const& cu : plan.compileUnits) + if (cu.packageName == pkg && !cu.servedFromCache) { anyLive = true; break; } + if (!anyLive) continue; + return std::format( + "internal: package '{}' generates its own inputs and has compile " + "units, but no compile edge for it was found in the manifest — " + "the ordering guard cannot see what it is supposed to check.\n" + " This is a build-emitter defect; please report it.", + pkg); + } + return std::nullopt; +} + +// A rule's command must begin with a program. +// +// ⭐ WHY THIS SHAPE, and not "no rule may reference an undefined variable". +// ninja expands an undefined variable to the empty string, which is a FEATURE +// several rules here depend on: `$soname_flag`, `$implib_flag`, `$def_flag` +// and `$unit_ldflags` are set per-edge and absent on purpose everywhere else, +// so a general undefined-variable check would need an allowlist of the ones +// that are optional — and a hand-maintained list of exceptions is the shape +// this file has been burned by twice already. +// +// The FIRST token has no such ambiguity. Whatever else a command line may +// legitimately omit, it cannot omit the program. So the invariant needs no +// exceptions, and it is exactly the one mcpp#533 violated: `c_shared` is +// `$cc -shared $in -o $out …`, `cc` was emitted only when the compile set +// contained a C or asm unit, and a package with no sources at all still +// reached the rule. `$cc` expanded to nothing, and `/bin/sh` was handed +// `-shared` as the program name. +// +// Scanning the emitted manifest rather than instrumenting each emit site is +// deliberate, for the reason `check_inline_command_lengths` states above it: a +// rule added later is covered the day it is added. Three `c_link`/`c_shared` +// emissions across two dialect branches already exist, and reasoning about +// which of them is reachable with which variables defined is precisely the +// work this removes. +// +// Rule-local variables count as defined — a rule may set its own — and so does +// a leading `$$`, which is a literal `$` and not a reference at all. +std::optional check_rule_commands_name_a_program( + const std::string& manifest) { + // Top-level `name = value`. Column 0 only: an indented assignment belongs + // to whatever rule or edge precedes it. + std::map topLevel; + for (auto line : manifest | std::views::split('\n')) { + std::string_view l{line.begin(), line.end()}; + if (l.empty() || l.front() == ' ' || l.front() == '\t') continue; + if (l.starts_with("rule ") || l.starts_with("build ") + || l.starts_with("default") || l.starts_with("#")) continue; + auto eq = l.find('='); + if (eq == std::string_view::npos) continue; + auto name = l.substr(0, eq); + while (!name.empty() && name.back() == ' ') name.remove_suffix(1); + if (name.empty() + || name.find(' ') != std::string_view::npos) continue; + auto value = l.substr(eq + 1); + while (!value.empty() && value.front() == ' ') value.remove_prefix(1); + topLevel.emplace(std::string(name), std::string(value)); + } + + std::string rule; // current rule, empty outside one + std::set ruleLocal; // variables that rule defines + std::string pendingCommand; // its command, checked at rule end + bool haveCommand = false; + + // Returns the diagnostic, or nullopt when the command is fine. + auto verdict = [&]() -> std::optional { + if (!haveCommand) return std::nullopt; + std::string_view cmd{pendingCommand}; + while (!cmd.empty() && (cmd.front() == ' ' || cmd.front() == '\t')) + cmd.remove_prefix(1); + if (cmd.empty()) + return std::format("ninja rule '{}' has an empty command", rule); + if (cmd.front() != '$') return std::nullopt; // a literal program + if (cmd.size() >= 2 && cmd[1] == '$') return std::nullopt; // literal $ + std::string_view name = cmd.substr(1); + if (!name.empty() && name.front() == '{') { + auto close = name.find('}'); + if (close == std::string_view::npos) return std::nullopt; + name = name.substr(1, close - 1); + } else { + std::size_t n = 0; + while (n < name.size() + && (std::isalnum(static_cast(name[n])) + || name[n] == '_' || name[n] == '.' || name[n] == '-')) + ++n; + name = name.substr(0, n); + } + if (name.empty()) return std::nullopt; + std::string key(name); + if (ruleLocal.contains(key)) return std::nullopt; + auto it = topLevel.find(key); + if (it == topLevel.end()) + return std::format( + "ninja rule '{}' begins its command with ${}, which no " + "variable defines — ninja expands it to nothing and the " + "shell would run the next argument as the program.\n" + " This is a build-emitter defect, not a project error; " + "please report it with the failing project.", + rule, key); + if (it->second.empty()) + return std::format( + "ninja rule '{}' begins its command with ${}, which is " + "defined but empty — the shell would run the next argument " + "as the program.\n" + " This is a build-emitter defect, not a project error; " + "please report it with the failing project.", + rule, key); + return std::nullopt; + }; + + for (auto line : manifest | std::views::split('\n')) { + std::string_view l{line.begin(), line.end()}; + const bool indented = !l.empty() && (l.front() == ' ' || l.front() == '\t'); + if (!indented) { + if (auto bad = verdict()) return bad; // close the previous + rule.clear(); ruleLocal.clear(); + pendingCommand.clear(); haveCommand = false; + if (l.starts_with("rule ")) rule = std::string(l.substr(5)); + continue; + } + if (rule.empty()) continue; // an edge's binding + auto body = l; + while (!body.empty() && (body.front() == ' ' || body.front() == '\t')) + body.remove_prefix(1); + auto eq = body.find('='); + if (eq == std::string_view::npos) continue; + auto name = body.substr(0, eq); + while (!name.empty() && name.back() == ' ') name.remove_suffix(1); + auto value = body.substr(eq + 1); + while (!value.empty() && value.front() == ' ') value.remove_prefix(1); + if (name == "command") { pendingCommand = std::string(value); haveCommand = true; } + else if (!name.empty()) ruleLocal.emplace(name); + } + return verdict(); +} + std::optional check_inline_command_lengths(const std::string& manifest) { std::set rspRules; std::string current; @@ -2319,6 +2674,17 @@ std::expected NinjaBackend::build(const BuildPlan& plan // neither the edge nor the reason. if (auto over = check_inline_command_lengths(manifest)) return std::unexpected(BuildError{*over, ninja_path}); + // Same backstop, same reason, one class over: a rule whose command begins + // with a variable that is not defined runs the REST of the command as a + // program. mcpp#533 spent four layers of a user's time on + // `/bin/sh: 1: -shared: not found`, which is that sentence. + if (auto bad = check_rule_commands_name_a_program(manifest)) + return std::unexpected(BuildError{*bad, ninja_path}); + // mcpp#534: a package that generates its own inputs must have every + // compile edge waiting on them. Checked against the plan, not just the + // text, because the interesting half is the denominator. + if (auto bad = check_action_ordering(manifest, plan)) + return std::unexpected(BuildError{*bad, ninja_path}); auto goalArg = append_goal_phony(manifest, opts.ninjaTargets); write_file(ninja_path, manifest); stage("write-ninja"); diff --git a/src/build/plan.cppm b/src/build/plan.cppm index 9558ec00..b38840d9 100644 --- a/src/build/plan.cppm +++ b/src/build/plan.cppm @@ -357,10 +357,26 @@ std::vector expand_manifest_include_entry(const std::filesystem::path& root, const std::filesystem::path& inc); +// How a package is named when one part of the plan has to be matched against +// another. `CompileUnit::packageName` and `BuildAction::packageName` are both +// this, and mcpp#534's ordering edge joins them — so it is exported rather +// than re-derived at the second call site, which is how the same decision +// ends up spelled two ways. +std::string qualified_package_name(const mcpp::manifest::Manifest& manifest); + } // namespace mcpp::build namespace mcpp::build { +std::string qualified_package_name(const mcpp::manifest::Manifest& manifest) { + if (!manifest.package.namespace_.empty() + && manifest.package.name.starts_with(manifest.package.namespace_ + ".")) { + return manifest.package.name; + } + if (manifest.package.namespace_.empty()) return manifest.package.name; + return manifest.package.namespace_ + "." + manifest.package.name; +} + namespace { std::string sanitize_for_path(std::string_view module_name) { @@ -378,15 +394,6 @@ std::string sanitize_for_path(std::string_view module_name) { // differently. This alias keeps the local spelling every call site below uses. using mcpp::object_filename_for; -std::string qualified_package_name(const mcpp::manifest::Manifest& manifest) { - if (!manifest.package.namespace_.empty() - && manifest.package.name.starts_with(manifest.package.namespace_ + ".")) { - return manifest.package.name; - } - if (manifest.package.namespace_.empty()) return manifest.package.name; - return manifest.package.namespace_ + "." + manifest.package.name; -} - std::vector dependency_name_candidates( const std::string& depName, const mcpp::manifest::DependencySpec& spec) diff --git a/src/build/prepare.cppm b/src/build/prepare.cppm index e55a1a6c..fdccc3ec 100644 --- a/src/build/prepare.cppm +++ b/src/build/prepare.cppm @@ -4237,15 +4237,17 @@ prepare_build(bool print_fingerprint, mm.buildConfig.actions.begin() + static_cast(firstNewAction), mm.buildConfig.actions.end()); - mcpp::build::directives::prepare_actions(fresh, pkgRoot); - std::copy(fresh.begin(), fresh.end(), - mm.buildConfig.actions.begin() - + static_cast(firstNewAction)); // The package that DECLARED the outputs classifies them: a dependency // generating a `.ixx` asks its own manifest, not the root project's. - // Built once per package, not once per output. + // Built once per package, not once per output — and BEFORE + // `prepare_actions`, which needs the same table to decide which + // outputs get a placeholder (a header does not; see mcpp#534). const auto pkgExtTable = mcpp::extension_table_for(mm.buildConfig.moduleExtensions); + mcpp::build::directives::prepare_actions(fresh, pkgRoot, pkgExtTable); + std::copy(fresh.begin(), fresh.end(), + mm.buildConfig.actions.begin() + + static_cast(firstNewAction)); for (auto const& a : fresh) { if (a.role != mcpp::manifest::BuildAction::Role::Source) continue; for (auto const& o : a.outputs) { @@ -8295,10 +8297,16 @@ prepare_build(bool print_fingerprint, return s; }; auto collect = [&](const mcpp::manifest::Manifest& mm) { + // The declaring package, recorded here because this is the only + // place that knows it: the build program emitted the action, and a + // program has no idea which package the engine loaded it for. + // mcpp#534's ordering edge is scoped to this name. + auto owner = mcpp::build::qualified_package_name(mm); for (auto a : mm.buildConfig.actions) { for (auto& x : a.inputs) x = substitute(x); for (auto& x : a.outputs) x = substitute(x); for (auto& x : a.command) x = substitute(x); + a.packageName = owner; ctx.plan.actions.push_back(std::move(a)); } }; @@ -9425,6 +9433,53 @@ prepare_build(bool print_fingerprint, } } + // ── A link unit with no inputs is not a build (mcpp#533) ──────────────── + // + // Checked HERE, last, because objects arrive from three places and each + // one is legitimate: the compile set, a `role = "object"` action + // (`lu.objects.emplace_back` above), and a Windows resource unit. A check + // placed before any of them would refuse a unit that was about to be + // filled. If a fourth source is ever added, it must land before this line. + // + // ⚠️ WHY THIS IS AN ERROR AND NOT A WARNING. The two library kinds fail + // differently and BOTH failures are worse than this message: + // + // shared — `$cc -shared` over an empty response file. Measured on + // gcc 16.1.0: `gcc: fatal error: no input files`, which names + // the driver and not the target. Before `cc` was emitted + // unconditionally it was `/bin/sh: 1: -shared: not found`, + // which names neither. + // static — `ar rcs libfoo.a` with no members. Measured: exit 0, an + // 8-byte archive, and a build that REPORTS SUCCESS. Every + // consumer then fails with undefined symbols, one repository + // further from the cause. + // + // The silent one is why this is not merely a nicer diagnostic. mcpp#533 + // reached here because a dependency's `install()` was skipped over a + // package-identity collision, leaving a version directory with no source + // tree; the shape is the same for any package whose sources fail to + // materialise, which is why the check is on the link unit rather than on + // the install path. + for (auto const& lu : ctx.plan.linkUnits) { + if (!lu.objects.empty()) continue; + const char* kindName = + lu.kind == mcpp::build::LinkUnit::SharedLibrary ? "shared library" + : lu.kind == mcpp::build::LinkUnit::StaticLibrary ? "static library" + : lu.kind == mcpp::build::LinkUnit::TestBinary ? "test binary" + : "binary"; + return std::unexpected(std::format( + "target '{}' ({}) has no inputs to link\n" + " no translation unit and no `role = \"object\"` action " + "output reached it, and an empty link is not a build: `ar` writes " + "an empty archive and reports success, so this would otherwise " + "surface as undefined symbols in whatever consumes '{}'\n" + " if '{}' is an installed dependency, its package directory " + "has no sources — reinstall it and check that its descriptor's " + "install step ran", + lu.targetName, kindName, lu.output.generic_string(), + lu.targetName)); + } + return ctx; } diff --git a/src/fallback/install_integrity.cppm b/src/fallback/install_integrity.cppm index cf910b2f..2f0b383f 100644 --- a/src/fallback/install_integrity.cppm +++ b/src/fallback/install_integrity.cppm @@ -33,8 +33,40 @@ export namespace mcpp::fallback { // Marker file name written into xpkg directories after successful install. inline constexpr std::string_view kInstallMarker = ".mcpp_ok"; +// Did the package put anything in its own directory? +// +// ⭐⭐ THE MARKER IS ONLY AS GOOD AS THE EVIDENCE IT IS WRITTEN FROM, and for +// three releases that evidence was `exitCode == 0 && exists(verdir)` — the +// installer's own report plus a directory the installer creates before it does +// any work. mcpp#533: a package-identity collision made xlings skip the +// descriptor's `install()`, the version directory was created anyway, mcpp +// stamped it complete, and the build failed four layers later in a linker +// command line. Because the stamp is what `is_install_complete` reads, the +// wrong state then survived every subsequent build. +// +// So: at least one entry that neither mcpp nor xlings wrote. That is the +// observed signature exactly — the directory held `.mcpp_ok`, +// `.xpkg-install.json` and `mcpp_generated/` and nothing else — and it needs +// no knowledge of what the descriptor was supposed to produce. +// +// ⚠️ DELIBERATELY WEAK, in both directions. It cannot tell a RIGHT tree from a +// wrong one, and it must not: a package that legitimately installs no payload +// exists (xlings has type-only packages, and #531 provisions `[xlings] deps` +// for packages mcpp consumes nothing from). So the caller WITHHOLDS THE MARKER +// rather than failing — the marker means "verified", we could not verify, and +// saying so costs one cheap re-check per build because xlings short-circuits +// its own install. If the package really was needed, the build now fails at +// the empty link unit, which names the target. +bool payload_is_substantive(const std::filesystem::path& xpkgDir); + // Check whether an xpkg directory has the .mcpp_ok marker. // STRICT marker-only — does not fall back to legacy heuristics. +// +// …with one exception, which is what makes the mcpp#533 fix reach a machine +// that already hit the bug: a marker over a directory holding nothing but +// mcpp's and xlings' own bookkeeping is not evidence, whoever wrote it. A +// store poisoned before this check existed heals on the next build instead of +// requiring the user to know which directory to delete. bool is_install_complete(const std::filesystem::path& xpkgDir); // Heuristic check for pre-.mcpp_ok packages (upgrade compat). @@ -132,8 +164,42 @@ bool has_marker(const std::filesystem::path& xpkgDir) { return std::filesystem::exists(xpkgDir / std::string(kInstallMarker)); } +bool payload_is_substantive(const std::filesystem::path& xpkgDir) { + // Everything mcpp or xlings writes into a version directory itself. Kept + // here, beside `kInstallMarker`, because that is the list's subject — and + // an entry added by either tool has to be added here or this predicate + // starts calling an empty install substantive again. + // + // .mcpp_ok mcpp's completeness marker (kInstallMarker) + // .xpkg-install.json xlings' install record + // mcpp_generated/ mcpp's `generated_files` synthesis + static constexpr std::string_view kSelfWritten[] = { + kInstallMarker, ".xpkg-install.json", "mcpp_generated", + }; + std::error_code ec; + if (!std::filesystem::is_directory(xpkgDir, ec)) return false; + for (auto const& e : std::filesystem::directory_iterator(xpkgDir, ec)) { + auto name = e.path().filename().string(); + bool self = false; + for (auto s : kSelfWritten) if (name == s) { self = true; break; } + if (!self) return true; + } + return false; +} + bool is_install_complete(const std::filesystem::path& xpkgDir) { if (!std::filesystem::exists(xpkgDir)) return false; + // The marker is necessary but not sufficient — see payload_is_substantive. + // This is the arm that heals a store poisoned by mcpp#533 before the fix + // existed: the stale `.mcpp_ok` stops being believed, the install is + // retried, and nobody has to be told which directory to remove. + if (std::filesystem::exists(xpkgDir / std::string(kInstallMarker)) + && !payload_is_substantive(xpkgDir)) { + mcpp::log::verbose("integrity", std::format( + "{}: marked complete but holds nothing the package installed — " + "treating as incomplete", xpkgDir.string())); + return false; + } // STRICT marker-only. // Used on the install/resolve path — half-extracted dirs with bin/ diff --git a/src/modgraph/validate.cppm b/src/modgraph/validate.cppm index b7af1193..47eeb12f 100644 --- a/src/modgraph/validate.cppm +++ b/src/modgraph/validate.cppm @@ -152,7 +152,23 @@ ValidateReport validate(const Graph& g, if (was_explicit) { r.errors.push_back({lib_root_rel, std::format( "[lib].path '{}' does not exist", lib_root_rel.string())}); - } else { + } else if (std::ranges::any_of(g.units, [](auto const& u) { + return u.provides.has_value(); + })) { + // ⚠️ THE PREDICATE ABOVE IS `has_lib_target` — "does this + // produce a library" — and the property this warning is + // about is "is this a C++ MODULE library". They came apart + // the moment the index grew source-built C packages + // (mcpp#533): `[targets.x] kind = "shared"` over a tree of + // `.c` files warned that `src/.cppm` was missing, and + // a C library has no lib root to miss. Worse, the warning + // is emitted while validating a DEPENDENCY, so one such + // package printed it in every consumer's build. + // + // A package with no module interface anywhere has no lib + // root by construction. One that has some, but not the + // conventional one, is the case the warning exists for and + // still gets it. r.warnings.push_back({lib_root_rel, std::format( "lib target without conventional lib root '{}' " "(create the file or set [lib].path)", diff --git a/src/pm/package_fetcher.cppm b/src/pm/package_fetcher.cppm index 0ea7a0cd..9fbf4b08 100644 --- a/src/pm/package_fetcher.cppm +++ b/src/pm/package_fetcher.cppm @@ -1093,8 +1093,27 @@ Fetcher::resolve_xpkg_path(std::string_view target, return std::unexpected(inst.error()); } if (inst->exitCode == 0 && std::filesystem::exists(verdir)) { - // Normal success path. - mcpp::fallback::mark_install_complete(verdir); + // Normal success path — but "the callee exited 0 and the directory + // it creates before doing any work exists" is not evidence that + // anything was installed. mcpp#533: a package-identity collision + // made xlings skip the descriptor's `install()` while still + // reporting success, and stamping that state cemented it, because + // `.mcpp_ok` is what the fast path above reads. + // + // Withheld rather than failed: a package can legitimately install + // no payload, and refusing here would break those. The cost of + // being wrong is one cheap re-check next build (xlings + // short-circuits its own install); the cost of the marker being + // wrong is permanent. + if (mcpp::fallback::payload_is_substantive(verdir)) { + mcpp::fallback::mark_install_complete(verdir); + } else { + mcpp::log::warn("fetcher", std::format( + "'{}@{}' reported a successful install but {} holds " + "nothing the package installed — not marking it complete", + parsed.packageName, parsed.version, + verdir.string())); + } stash.commit(); return make_payload(); } diff --git a/tests/e2e/314_dependency_action_generated_header.sh b/tests/e2e/314_dependency_action_generated_header.sh new file mode 100755 index 00000000..146eb743 --- /dev/null +++ b/tests/e2e/314_dependency_action_generated_header.sh @@ -0,0 +1,158 @@ +#!/usr/bin/env bash +# requires: gcc +# 314_dependency_action_generated_header.sh — a package that generates its own +# header compiles after the generator, not alongside it (mcpp#534). +# +# THE SHAPE THAT WAS BROKEN. `role = "source"` was documented as "outputs join +# the compile set; the compile edge consumes them", and that is true of a +# generated `.cpp` and false of a generated `.h`. A header is never an edge +# input — it is reached through `-I`, and the depfile that would record it does +# not exist until a compile has already succeeded. So the action's node sat in +# build.ninja with nothing able to reach it: not in `default` (Source outputs +# are deliberately excluded), not in the goal phony (objects and link outputs +# only), and consumed by no edge. +# +# ⚠️ THE ASSERTION IS ON CONTENT, NOT EXISTENCE. `prepare_actions` used to +# write a zero-byte placeholder for every Source output including headers, so +# the file was on disk whether or not the generator ran — which is exactly why +# mcpp#534 was filed as an intermittent race when it was a deterministic +# never-runs. A test asserting `[[ -f gen.h ]]` would have passed against the +# defect. +# +# The dependency is a PATH dependency because that is the reported shape: the +# generator, the header and the consumer of the header are all inside one +# dependency package, which is the case `tests/examples/protobuf-protoc` does +# not cover (there the generated header is used by the ROOT project). +set -e + +TMP=$(mktemp -d) +trap "rm -rf $TMP" EXIT + +# ── the dependency: generates a header, and compiles a source that uses it ── +mkdir -p "$TMP/proto/src" +cat > "$TMP/proto/mcpp.toml" <<'EOF' +[package] +name = "proto" +version = "0.1.0" + +[targets.proto] +kind = "lib" +EOF + +cat > "$TMP/proto/gen.sh" <<'EOF' +#!/usr/bin/env bash +# Slow on purpose: if the ordering edge is missing, the compile wins the race +# every time rather than occasionally, so a red run means the defect and not +# the weather. +sleep 2 +printf '#define PROTO_ANSWER 42\n' > "$1" +EOF +chmod +x "$TMP/proto/gen.sh" + +# The lib root is a module interface, so this package really is a module +# library — keeping the lib-root convention in play rather than side-stepping +# it. It includes the generated header from its own implementation. +cat > "$TMP/proto/src/proto.cppm" <<'EOF' +module; +#include "proto_generated.h" +export module proto; +export int proto_answer() { return PROTO_ANSWER; } +EOF + +cat > "$TMP/proto/build.mcpp" <<'EOF' +#include +import mcpp; +int main() { + const std::string root = mcpp::manifest_dir(); + const std::string out = mcpp::out_dir(); + mcpp::action a; + a.id = "proto:header"; + a.role = "source"; + a.arg((root + "/gen.sh").c_str()) + .arg((out + "/proto_generated.h").c_str()) + .output((out + "/proto_generated.h").c_str()) // HEADER, and the only output + .submit(); + mcpp::include_dir(out.c_str()); +} +EOF + +# ── the consumer ──────────────────────────────────────────────────────────── +mkdir -p "$TMP/app/src" +cat > "$TMP/app/mcpp.toml" <<'EOF' +[package] +name = "app" +version = "0.1.0" + +[dependencies] +proto = { path = "../proto" } +EOF +cat > "$TMP/app/src/main.cpp" <<'EOF' +#include +import proto; +int main() { std::printf("ANSWER=%d\n", proto_answer()); } +EOF + +cd "$TMP/app" +"$MCPP" build > b.log 2>&1 || { + cat b.log + echo "FAIL: a dependency whose action generates its only header did not build" + exit 1; } + +out="$("$MCPP" run 2>&1 | tail -1)" +[[ "$out" == "ANSWER=42" ]] || { + echo "FAIL: expected ANSWER=42 from the generated header, got '$out'"; exit 1; } + +# The generator actually ran. Size, not existence — see the header comment. +hdr=$(find "$TMP" -name proto_generated.h | head -1) +[[ -n "$hdr" ]] || { echo "FAIL: no proto_generated.h anywhere"; exit 1; } +bytes=$(wc -c < "$hdr") +[[ "$bytes" -gt 0 ]] || { + echo "FAIL: $hdr is $bytes bytes — the placeholder, not the generator's output" + exit 1; } +grep -q 'PROTO_ANSWER' "$hdr" || { + echo "FAIL: $hdr exists and is non-empty but is not what gen.sh writes:" + cat "$hdr"; exit 1; } + +# The ordering is declared, not merely observed. A green run on a fast machine +# proves nothing on its own; this is the edge that makes it a property. +nj=$(find "$TMP/app/target" -name build.ninja | head -1) +[[ -n "$nj" ]] || { echo "FAIL: no build.ninja"; exit 1; } + +# The phony is named for the QUALIFIED package (`mcpp-actions-mcpplibs.proto`), +# because that is how `CompileUnit::packageName` spells it and the two are +# matched against each other. Read the name out rather than hardcoding it: a +# test that spelled it by hand would be asserting the naming policy, which is +# not what this test is about. +phony=$(grep -oE '^build (mcpp-actions-[^ ]*proto[^ ]*) : phony' "$nj" \ + | awk '{print $2}' | head -1) +[[ -n "$phony" ]] || { + echo "FAIL: no action phony was emitted for the dependency" + grep -nE '^build mcpp-actions' "$nj" || echo " (none at all)" + exit 1; } +count=$(grep -cE "^build $phony : phony" "$nj" || true) +[[ "$count" -eq 1 ]] || { + echo "FAIL: expected exactly one '$phony' edge, found $count"; exit 1; } + +# DENOMINATOR. "every edge that should carry it does" is vacuously true when no +# edge does, and that vacuum is precisely the pre-fix state — so the count of +# the dependency's compile edges is asserted separately from the count that +# carries the ordering. +# Matched on the RULE and the SOURCE, not on the object path. An object edge +# can carry implicit outputs before the colon (`build a.o | gcm.cache/x.gcm :`), +# so a pattern anchored on the output is a pattern that quietly matches nothing +# — which, for a denominator, is the one failure mode that matters. +rules='cxx_object|c_object|cxx_module|cxx_module_obj|cxx_module_bmi|cxx_scan|asm_object' +src='proto/src/proto\.cppm' +edges=$(grep -cE "^build .*: *($rules) .*$src" "$nj" || true) +ordered=$(grep -cE "^build .*: *($rules) .*$src.*\|\| *$phony" "$nj" || true) +[[ "$edges" -gt 0 ]] || { + echo "FAIL: no compile edge for the dependency was found at all — the" + echo " assertion below would have passed by describing nothing" + grep -nE '^build ' "$nj" | head -20 + exit 1; } +[[ "$ordered" -eq "$edges" ]] || { + echo "FAIL: $ordered of $edges dependency compile edges wait for the action" + grep -nE "^build .*: *($rules) .*$src" "$nj" + exit 1; } + +echo "PASS: 314 (generator ran before the compile; $ordered/$edges edges ordered)" diff --git a/tests/e2e/315_blocking_check_gates_compilation.sh b/tests/e2e/315_blocking_check_gates_compilation.sh new file mode 100755 index 00000000..158145a8 --- /dev/null +++ b/tests/e2e/315_blocking_check_gates_compilation.sh @@ -0,0 +1,105 @@ +#!/usr/bin/env bash +# requires: gcc +# 315_blocking_check_gates_compilation.sh — `blocking = true` on a check does +# what it has always been documented to do (mcpp#534). +# +# WHAT THIS DEFENDS. `blocking` was typed (`BuildAction::blocking`), emitted +# over the build-program protocol (`hostprogram.cppm`), parsed +# (`directives.cppm`), documented in both languages (`docs/07-build-mcpp.md` +# and its Chinese counterpart) and demonstrated in a shipped example +# (`examples/08-build-rules/rules-tidy`) — and read by nothing. The only +# order-only edge the ninja backend emitted was the staged-BMI one. So a check +# marked blocking ran alongside compilation exactly like a non-blocking one, +# and the flag was a no-op with a paper trail. +# +# ⚠️ ASSERTED ON THE ARTIFACT, NOT ON A LOG LINE. "the check failed" appears in +# the output either way; what distinguishes blocking from non-blocking is +# whether the object was PRODUCED. A grep over stderr would pass against the +# defect. +set -e + +TMP=$(mktemp -d) +trap "rm -rf $TMP" EXIT +mkdir -p "$TMP/app/src" +cd "$TMP/app" + +cat > mcpp.toml <<'EOF' +[package] +name = "app" +version = "0.1.0" +EOF +printf '#include \nint main(){ std::printf("ok\\n"); }\n' > src/main.cpp + +# A check that fails, slowly. The sleep is what makes the difference +# observable: without the ordering edge the compile finishes long before the +# check does, so the object exists on disk when the build gives up. +cat > check.sh <<'EOF' +#!/usr/bin/env bash +sleep 2 +echo "the check says no" >&2 +exit 1 +EOF +chmod +x check.sh + +emit_build_mcpp() { # $1 = "true" | "false" + cat > build.mcpp < +import mcpp; +int main() { + const std::string root = mcpp::manifest_dir(); + mcpp::action a; + a.id = "gate"; + a.role = "check"; + a.blocking = $1; + a.arg((root + "/check.sh").c_str()) + .output("\${mcpp.out_dir}/gate.stamp") + .submit(); +} +EOF +} + +# ── blocking = true: the object must NOT be produced ──────────────────────── +emit_build_mcpp true +rm -rf target +if "$MCPP" build > b_blocking.log 2>&1; then + cat b_blocking.log + echo "FAIL: a failing blocking check let the build succeed"; exit 1 +fi +if find target -name 'main*.o' | grep -q .; then + echo "FAIL: a failing BLOCKING check did not stop the compile — the object exists:" + find target -name 'main*.o' + echo " (this is the pre-fix behaviour: 'blocking' was parsed and never read)" + exit 1 +fi + +# The edge that makes it a property rather than a race lost by the compiler. +nj=$(find target -name build.ninja | head -1) +[[ -n "$nj" ]] || { echo "FAIL: no build.ninja"; exit 1; } +grep -qE '^build mcpp-actions-app : phony.*gate\.stamp' "$nj" || { + echo "FAIL: the blocking check's stamp is not in the package's action phony" + grep -nE '^build mcpp-actions' "$nj" || echo " (no phony at all)" + exit 1; } +ordered=$(grep -cE '^build [^:]*main[^:]*\.o *:.*\|\|.*mcpp-actions-app' "$nj" || true) +[[ "$ordered" -ge 1 ]] || { + echo "FAIL: the compile edge does not wait for the blocking check"; exit 1; } + +# ── blocking = false: the CONTROL. Same check, same failure, compile runs ─── +# +# Without this half, a build that refused to compile for any unrelated reason +# would pass the assertion above. What must differ between the two runs is +# exactly one line of build.mcpp. +emit_build_mcpp false +rm -rf target +"$MCPP" build > b_parallel.log 2>&1 || true # still fails: the check still fails +if ! find target -name 'main*.o' | grep -q .; then + cat b_parallel.log + echo "FAIL: a NON-blocking check also stopped the compile — 'blocking' is" + echo " not distinguishing anything, it is just always on" + exit 1 +fi +nj2=$(find target -name build.ninja | head -1) +if grep -qE '^build mcpp-actions-app : phony.*gate\.stamp' "$nj2"; then + echo "FAIL: a NON-blocking check joined the ordering phony"; exit 1 +fi + +echo "PASS: 315 (blocking gates the compile; non-blocking does not)" diff --git a/tests/e2e/316_link_unit_with_no_inputs_is_refused.sh b/tests/e2e/316_link_unit_with_no_inputs_is_refused.sh new file mode 100755 index 00000000..6bb1f484 --- /dev/null +++ b/tests/e2e/316_link_unit_with_no_inputs_is_refused.sh @@ -0,0 +1,96 @@ +#!/usr/bin/env bash +# requires: gcc +# 316_link_unit_with_no_inputs_is_refused.sh — a target with nothing to link is +# an error that names the target (mcpp#533). +# +# WHERE THIS CAME FROM. A dependency whose `install()` was skipped over a +# package-identity collision left a version directory with no source tree. mcpp +# planned a shared-library target for it anyway, emitted a link edge with zero +# inputs, and the user was shown: +# +# /bin/sh: 1: -shared: not found +# +# — four layers from the cause and naming nothing that had anything to do with +# it. (`$cc` was emitted only when the compile set held a C or asm unit; a +# package with no sources has neither, so the variable expanded to nothing and +# the shell was handed `-shared` as a program name.) +# +# ⚠️ THE STATIC CASE IS THE ONE THAT MATTERED. `ar rcs libfoo.a` with no +# members exits 0 and writes an 8-byte archive, so before this the build +# REPORTED SUCCESS and every consumer failed later with undefined symbols. Both +# kinds are asserted here, and the static one is why the check is at plan time +# rather than a better linker diagnostic. +set -e + +TMP=$(mktemp -d) +trap "rm -rf $TMP" EXIT + +check_kind() { # $1 = mcpp kind, $2 = human name, $3 = artifact glob + local dir="$TMP/$1" + mkdir -p "$dir/src" + cat > "$dir/mcpp.toml" < b.log 2>&1; then + echo "FAIL($1): a target with no sources built successfully" + find . -name "$3" -printf ' produced %p (%s bytes)\n' 2>/dev/null + exit 1 + fi + + # The message names the target, and says which kind — a user reading it has + # to be able to find the thing in their manifest. + grep -q "target 'ghost'" b.log || { + echo "FAIL($1): the error does not name the target:"; cat b.log; exit 1; } + grep -q "$2" b.log || { + echo "FAIL($1): the error does not say it is a $2:"; cat b.log; exit 1; } + + # ⭐ THE NEGATIVE HALF. This is the assertion that catches a regression back + # to the reported symptom: if the refusal is ever removed, the build reaches + # the linker again and this is what comes out. + if grep -q -- '-shared: not found' b.log; then + echo "FAIL($1): the build reached the shell and reported the old symptom:" + cat b.log; exit 1 + fi + if grep -qi 'no input files' b.log; then + echo "FAIL($1): the build reached the compiler driver instead of being" + echo " refused at plan time:"; cat b.log; exit 1 + fi + + # Nothing was written. For the static kind this is the whole point. + if find . -name "$3" | grep -q .; then + echo "FAIL($1): an artifact was produced by a refused target:" + find . -name "$3" -printf ' %p (%s bytes)\n' + exit 1 + fi +} + +check_kind shared "shared library" 'libghost.so*' +check_kind lib "static library" 'libghost.a' + +# ── the control: a target WITH sources still builds ───────────────────────── +# +# Without this, a refusal that fired on every library target would pass +# everything above. +mkdir -p "$TMP/ok/src" +cat > "$TMP/ok/mcpp.toml" <<'EOF' +[package] +name = "ok" +version = "0.1.0" + +[targets.ok] +kind = "shared" +EOF +printf 'int ok_value(void) { return 7; }\n' > "$TMP/ok/src/ok.c" +cd "$TMP/ok" +"$MCPP" build > b.log 2>&1 || { + cat b.log; echo "FAIL: a library target WITH sources was refused"; exit 1; } +find . -name 'libok.so*' | grep -q . || { + echo "FAIL: the control target produced no library"; exit 1; } + +echo "PASS: 316 (empty shared and static targets refused by name; populated one builds)" diff --git a/tests/unit/test_install_integrity.cpp b/tests/unit/test_install_integrity.cpp index 1593c92e..f9180680 100644 --- a/tests/unit/test_install_integrity.cpp +++ b/tests/unit/test_install_integrity.cpp @@ -155,3 +155,87 @@ TEST(InstallIntegrityStash, NoopWhenAlreadyComplete) { fs::remove_all(root); } + +// ─── mcpp#533: the marker is only as good as the evidence behind it ───────── +// +// `.mcpp_ok` used to be written from `exitCode == 0 && exists(verdir)` — the +// installer's own report, plus a directory the installer creates before doing +// any work. A package-identity collision made xlings skip the descriptor's +// `install()` while still reporting success; mcpp stamped the result complete, +// and because the stamp is what `is_install_complete` reads, the wrong state +// then survived every later build. +// +// The observed directory held exactly three entries — `.mcpp_ok`, +// `.xpkg-install.json`, `mcpp_generated/` — and nothing else. That signature +// needs no knowledge of what the descriptor was supposed to build. + +namespace { + +// Everything mcpp and xlings write into a version directory themselves. +// Written out rather than taken from the code under test: this list IS the +// claim, and sharing it would let both sides drift together. +void make_self_written_only(const fs::path& verdir) { + write_file(verdir / ".mcpp_ok", "1\n"); + write_file(verdir / ".xpkg-install.json", "{}\n"); + write_file(verdir / "mcpp_generated" / "libdrm" / "lib" / "libdrm.so", ""); +} + +} // namespace + +TEST(InstallEvidence, ADirectoryHoldingOnlyOurOwnFilesIsNotAPayload) { + auto dir = make_tempdir("mcpp-evidence-self"); + auto verdir = dir / "compat-x-libdrm" / "2.4.123"; + make_self_written_only(verdir); + + EXPECT_FALSE(mcpp::fallback::payload_is_substantive(verdir)); + fs::remove_all(dir); +} + +TEST(InstallEvidence, OneEntryTheInstallWroteIsEnough) { + auto dir = make_tempdir("mcpp-evidence-real"); + auto verdir = dir / "compat-x-libdrm" / "2.4.123"; + make_self_written_only(verdir); + // …plus the tree the descriptor's install() was supposed to build. + write_file(verdir / "include" / "drm.h", "#pragma once\n"); + + EXPECT_TRUE(mcpp::fallback::payload_is_substantive(verdir)); + fs::remove_all(dir); +} + +// ⭐ THE HALF THAT MAKES THE UPGRADE SEAMLESS. `is_install_complete` is +// marker-only by design, so a store poisoned before this check existed carries +// a `.mcpp_ok` that short-circuits forever. Guarding only the WRITE site would +// help users who have not hit the bug and do nothing for the ones who filed +// it. Nobody should have to be told which directory to delete. +TEST(InstallEvidence, AStaleMarkerOverAnEmptyPayloadDoesNotCountAsComplete) { + auto dir = make_tempdir("mcpp-evidence-heal"); + auto verdir = dir / "compat-x-libdrm" / "2.4.123"; + make_self_written_only(verdir); + ASSERT_TRUE(has_ok_marker(verdir)); // the poisoned state, exactly + + EXPECT_FALSE(mcpp::fallback::is_install_complete(verdir)); + fs::remove_all(dir); +} + +// The other direction, which is the one that would make every build slower if +// it were wrong: a real install must still be believed on the strength of its +// marker, without re-running anything. +TEST(InstallEvidence, ARealInstallIsStillCompleteOnItsMarker) { + auto dir = make_tempdir("mcpp-evidence-good"); + auto verdir = dir / "xim-x-gcc" / "16.1.0"; + make_legacy_pkg(verdir, "payload"); + write_file(verdir / ".mcpp_ok", "1\n"); + + EXPECT_TRUE(mcpp::fallback::payload_is_substantive(verdir)); + EXPECT_TRUE(mcpp::fallback::is_install_complete(verdir)); + fs::remove_all(dir); +} + +// An absent directory is absent, not incomplete — the new arm must not change +// what the function says about a package that was never installed. +TEST(InstallEvidence, AnAbsentDirectoryIsStillJustAbsent) { + auto dir = make_tempdir("mcpp-evidence-absent"); + EXPECT_FALSE(mcpp::fallback::is_install_complete(dir / "nothing" / "here")); + EXPECT_FALSE(mcpp::fallback::payload_is_substantive(dir / "nothing" / "here")); + fs::remove_all(dir); +} diff --git a/tests/unit/test_ninja_backend.cpp b/tests/unit/test_ninja_backend.cpp index 46c8820b..53db9bb6 100644 --- a/tests/unit/test_ninja_backend.cpp +++ b/tests/unit/test_ninja_backend.cpp @@ -1632,3 +1632,200 @@ TEST(LinkFailureAdvice, CarriesNoVersionLiteral) { ASSERT_FALSE(advice.empty()); EXPECT_EQ(advice.find("0."), std::string::npos) << advice; } + +// ═══ mcpp#533 / mcpp#534: emitter self-checks ══════════════════════════════ + +// ── A rule's command must begin with a program (mcpp#533) ────────────────── +// +// `c_shared` is `$cc -shared $in -o $out …`, and `cc` was emitted only when +// the COMPILE set held a C or asm unit — a property that says nothing about +// which LINK rules exist. A package with no sources still selected `c_shared`, +// `$cc` expanded to nothing, and `/bin/sh` was handed `-shared` as the program +// name. Twenty-six lines below `cc`, this file already carried the rule that +// prevents it, written for `c_ldflags` and never applied to its sibling. +// +// ⭐ The checker is exported so it can be asserted against manifests +// `emit_ninja_string` is supposed never to produce. A test that could only +// reach it through a BuildPlan could not show the checker works at all. + +TEST(NinjaEmitterGuards, ARuleWhoseCommandStartsWithAnUndefinedVariableIsRefused) { + const std::string manifest = + "cxx = /usr/bin/g++\n" + "\n" + "rule c_shared\n" + " command = $cc -shared $in -o $out\n" + " description = SHARED $out\n"; + auto bad = check_rule_commands_name_a_program(manifest); + ASSERT_TRUE(bad.has_value()) << manifest; + EXPECT_NE(bad->find("c_shared"), std::string::npos) << *bad; + EXPECT_NE(bad->find("$cc"), std::string::npos) << *bad; +} + +TEST(NinjaEmitterGuards, ARuleWhoseLeadingVariableIsDefinedButEmptyIsRefused) { + const std::string manifest = + "cc =\n" + "\n" + "rule c_link\n" + " command = $cc $in -o $out\n"; + EXPECT_TRUE(check_rule_commands_name_a_program(manifest).has_value()); +} + +// The negatives, which are what stop this from being an always-fires check. +// Note especially `$soname_flag`: variables set per-EDGE are legitimately +// undefined at the top level, and expanding them to nothing is a feature +// several rules depend on. That is why this guard is about the first token and +// not about undefined variables in general — the general form would have +// needed an allowlist, and a hand-maintained exception list is the shape this +// file has been burned by twice. +TEST(NinjaEmitterGuards, WellFormedRulesAndOptionalPerEdgeVariablesPass) { + const std::string manifest = + "cc = /usr/bin/gcc\n" + "cxx = /usr/bin/g++\n" + "\n" + "rule c_shared\n" + " command = $cc -shared $in -o $out $soname_flag $unit_ldflags\n" + "\n" + "rule literal_tool\n" + " command = /usr/bin/strip $in\n" + "\n" + "rule rule_local\n" + " tool = /usr/bin/objcopy\n" + " command = $tool -O binary $in $out\n"; + auto bad = check_rule_commands_name_a_program(manifest); + EXPECT_FALSE(bad.has_value()) << (bad ? *bad : std::string{}); +} + +// The emitter's own output must satisfy its own guard — including the case +// that produced the defect: a plan whose compile set has no C or asm unit. +TEST(NinjaEmitterGuards, EmittedManifestsAlwaysNameAProgram) { + auto plan = minimal_plan(); + auto ninja = emit_ninja_string(plan); + auto bad = check_rule_commands_name_a_program(ninja); + EXPECT_FALSE(bad.has_value()) << (bad ? *bad : std::string{}); + + // `cc` unconditionally, not "when something C is being compiled". + EXPECT_NE(ninja.find("\ncc = "), std::string::npos) + << "cc is not defined in a plan with no C sources — this is mcpp#533\n" + << ninja; +} + +// ── A package's compiles wait for its generators (mcpp#534) ──────────────── + +namespace { + +// A plan with one package that generates a HEADER and compiles a source. The +// header is the whole point: a generated `.cpp` becomes an edge input and is +// ordered for free, a generated `.h` is reached through `-I` and never appears +// as an input at all. +BuildPlan plan_with_generated_header() { + auto plan = minimal_plan(); + plan.compileUnits.push_back({ + .source = "src/uses_header.cpp", + .kind = mcpp::SourceKind::Cxx, + .object = "obj/uses_header.o", + .packageName = "gen_pkg", + }); + mcpp::manifest::BuildAction a; + a.id = "gen:header"; + a.packageName = "gen_pkg"; + a.role = mcpp::manifest::BuildAction::Role::Source; + a.command = {"/bin/true"}; + a.outputs = {"out/generated.h"}; + plan.actions.push_back(std::move(a)); + return plan; +} + +} // namespace + +TEST(ActionOrdering, AGeneratedHeaderGetsAPhonyAndTheCompileEdgeWaitsForIt) { + auto ninja = emit_ninja_string(plan_with_generated_header()); + + EXPECT_NE(ninja.find("build mcpp-actions-gen_pkg : phony"), std::string::npos) + << ninja; + EXPECT_NE(ninja.find("|| mcpp-actions-gen_pkg"), std::string::npos) + << "the compile edge does not wait for the generator\n" << ninja; +} + +// ⚠️ THE DENOMINATOR. "every edge that should carry the ordering does" is +// vacuously true when no edge does — and that vacuum is exactly the state this +// change fixes, so the count of edges is asserted separately from the count +// that carries it. A bare equality would have passed against the defect. +TEST(ActionOrdering, EveryCompileEdgeOfThatPackageCarriesIt) { + auto ninja = emit_ninja_string(plan_with_generated_header()); + + const auto edges = count_occurrences(ninja, "build obj/uses_header.o :"); + const auto ordered = + count_occurrences(ninja, "|| mcpp-actions-gen_pkg"); + EXPECT_GT(edges, 0u) << "no compile edge was emitted at all — the " + "assertion below would describe nothing\n" << ninja; + EXPECT_GE(ordered, edges) << ninja; +} + +// A package with no gating action gets no phony, and its edges are untouched. +// Without this the change would read as "always add an order-only edge", which +// is not what it does and would serialise builds that have nothing to wait for. +TEST(ActionOrdering, APackageWithNoActionsIsUnchanged) { + auto plan = minimal_plan(); + plan.compileUnits.push_back({ + .source = "src/plain.cpp", + .kind = mcpp::SourceKind::Cxx, + .object = "obj/plain.o", + .packageName = "plain_pkg", + }); + auto ninja = emit_ninja_string(plan); + EXPECT_EQ(ninja.find("mcpp-actions-plain_pkg"), std::string::npos) << ninja; +} + +// `Object` and `Artifact` really are ordered by file dependency — their +// outputs are link inputs and their inputs are link outputs — so pulling them +// into the phony would add an edge that expresses nothing. This is the half of +// the original comment that was true, and it has to stay true. +TEST(ActionOrdering, ObjectRoleActionsDoNotJoinTheCompileGate) { + auto plan = minimal_plan(); + plan.compileUnits.push_back({ + .source = "src/x.cpp", + .kind = mcpp::SourceKind::Cxx, + .object = "obj/x.o", + .packageName = "obj_pkg", + }); + mcpp::manifest::BuildAction a; + a.id = "blob"; + a.packageName = "obj_pkg"; + a.role = mcpp::manifest::BuildAction::Role::Object; + a.command = {"/bin/true"}; + a.outputs = {"out/blob.o"}; + plan.actions.push_back(std::move(a)); + + auto ninja = emit_ninja_string(plan); + EXPECT_EQ(ninja.find("mcpp-actions-obj_pkg"), std::string::npos) << ninja; +} + +// A non-blocking check runs alongside compilation; a blocking one precedes it. +// `blocking` was typed, transported, parsed, documented in two languages and +// demonstrated in a shipped example — and read by nothing, which made it a +// no-op with a paper trail. +TEST(ActionOrdering, BlockingDecidesWhetherACheckGatesTheCompile) { + auto make = [](bool blocking) { + auto plan = minimal_plan(); + plan.compileUnits.push_back({ + .source = "src/c.cpp", + .kind = mcpp::SourceKind::Cxx, + .object = "obj/c.o", + .packageName = "chk_pkg", + }); + mcpp::manifest::BuildAction a; + a.id = "tidy"; + a.packageName = "chk_pkg"; + a.role = mcpp::manifest::BuildAction::Role::Check; + a.blocking = blocking; + a.command = {"/bin/true"}; + a.outputs = {"out/tidy.stamp"}; + plan.actions.push_back(std::move(a)); + return emit_ninja_string(plan); + }; + + EXPECT_NE(make(true).find("|| mcpp-actions-chk_pkg"), std::string::npos) + << "blocking = true did not gate the compile"; + EXPECT_EQ(make(false).find("mcpp-actions-chk_pkg"), std::string::npos) + << "blocking = false gated the compile anyway"; +} From ac2a84b4f5ac3ab3f3e688c091b94d0e72d37699 Mon Sep 17 00:00:00 2001 From: speak-agent Date: Sun, 30 Aug 2026 13:40:40 +0800 Subject: [PATCH 2/2] =?UTF-8?q?chore(xlings):=20raise=20the=20floor=20to?= =?UTF-8?q?=202026.8.30.2=20=E2=80=94=20the=20store-identity=20fix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit openxlings/xlings#576, released as v2026.8.30.2. Below this version xlings answers "is this package already installed" from the xvm version database keyed on the bare short name, so a package skips its own `install()` whenever any other namespace holds the same `@`. This is a FLOOR and not a preference, which is the whole reason the pin has the shape it does. The mcpp side of #533 makes the resulting failure legible on any client — the link unit is refused by name, and `.mcpp_ok` is withheld from a directory holding nothing the package installed — but only a client at or above this version INSTALLS correctly. ⚠️ This does NOT authorise an index change. Publishing a deliberately colliding `@` stays unsafe until the floor is adopted, not merely released, and .agents/docs/2026-08-30-cross-repo-fix-plan-532-533-534.md §6.2 recommends not doing it at all: the fix is for the collisions people hit by accident, which is already 20 short names wide on a real store. `src/xlings/xlings.cppm` is the source of truth; the seven copies under .github/ follow it and `check_version_pins.sh` enforces the agreement. Also records what landed against the plan (§12), including two things the plan had wrong: the xlings anchor was 167 commits stale and the call site had moved, and `⚠` is CI-governed in xlings while it is merely conventional here. --- ...6-08-30-cross-repo-fix-plan-532-533-534.md | 92 +++++++++++++++++++ .github/actions/bootstrap-mcpp/action.yml | 2 +- .github/actions/setup-macos-llvm/action.yml | 2 +- .github/workflows/bootstrap-macos.yml | 2 +- .github/workflows/ci-fresh-install.yml | 6 +- .github/workflows/ci-linux-e2e.yml | 2 +- .github/workflows/cross-build-test.yml | 4 +- .github/workflows/release.yml | 14 +-- src/xlings/xlings.cppm | 30 +++++- 9 files changed, 133 insertions(+), 21 deletions(-) diff --git a/.agents/docs/2026-08-30-cross-repo-fix-plan-532-533-534.md b/.agents/docs/2026-08-30-cross-repo-fix-plan-532-533-534.md index 1d52dde8..94ab5400 100644 --- a/.agents/docs/2026-08-30-cross-repo-fix-plan-532-533-534.md +++ b/.agents/docs/2026-08-30-cross-repo-fix-plan-532-533-534.md @@ -835,3 +835,95 @@ B3 on the produced artifact rather than a log line. - The decision to keep `Source` outputs out of `actionDefaults` (§3): reaching them two ways is how the soname aliases went missing in 0.0.104. - §7's three amendments to #532. + +--- + +## 12. What landed, and what the plan got wrong + +Written after implementation, against the code as merged. Everything here was +measured, not inferred. + +### 12.1 Two corrections to the plan itself + +**The xlings anchor was 167 commits stale.** §5 located Track X at +`installer.cpp:903-911` and described `xvm::match_version(db, node.name, …)` as +a fallback beside a namespace-aware primary path. That reading came from a local +checkout dated 8 July. Upstream `main` had since consolidated four separate +"is this installed" answerers into one `install_state` module whose predicate +takes a namespace — and the defective call site had moved to +`installer.cpp:2628`, survived the consolidation, and become a **fifth** +answerer to the question that module exists to answer alone. The fix is +unchanged in shape and better motivated than the plan knew. + +The general rule this is the second instance of: *what the implementation is* +can only be read from the tracked upstream branch. A worktree at the fetched +ref costs one command and is the only thing that makes an anchor trustworthy. + +**`⚠` is CI-enforced in xlings and merely conventional in mcpp.** +`tests/e2e/tui_output_contract_test.sh` §S6 greps all of `src/**/*.{cppm,cpp}` +for U+26A0 and U+24D8 and fails if either appears outside +`src/core/glyph.cppm` — they are label glyphs the renderer owns, and a second +spelling is how a dead icon table in `src/platform/` once drifted from the real +one. Two comment headers turned an otherwise-green xlings PR red (101 passed, 1 +failed) for comment decoration alone. `⭐` is not governed and is already used +in that tree. + +### 12.2 What shipped + +| | where | note | +|---|---|---| +| A1 | `ninja_backend.cppm` | `cc` unconditional | +| A2 | `prepare.cppm`, before `return ctx` | after all three object sources | +| A3 | `check_rule_commands_name_a_program` | exported, so hand-written manifests can test it | +| B1 | `BuildAction::packageName` | set in `collect()`; `qualified_package_name` exported so the two spellings cannot drift | +| B2 | per-package phony, 7 call sites | via `order_only_for(cu)` | +| B3 | `blocking` | reaches the graph for the first time | +| B4 | `prepare_actions` | placeholder gated on `is_compilable_output` | +| B5 | `check_action_ordering` | with the denominator | +| C1 | `payload_is_substantive` | both the write site and the fast path | +| C2 | `validate.cppm` | gated on the graph containing a module interface | +| D | `docs/07-build-mcpp.md` + zh | the role table and the engine comment | +| X | `installer.cpp:2628`, `owner.cppm` | `payload_path_names_another_package` | + +### 12.3 The measurement that changed the plan's priorities + +§A0 was added mid-review and is the most consequential single fact found: + +``` +$ ar rcs libempty.a ; echo $? ; stat -c %s libempty.a +0 +8 +``` + +A static library target with no sources built **successfully** — confirmed +against the released binary, which printed `Finished dev` and left an 8-byte +`.a`. The reported symptom (`/bin/sh: 1: -shared: not found`) was the loud half +of a defect whose quiet half reported success. That is why A2 is the fix and A1 +is hygiene, and why the check is at plan time rather than in the linker +diagnostic. + +### 12.4 Evidence that the tests discriminate + +Every new e2e was run against the pre-fix binary (`2026.8.28.2`) and every one +fails there: + +``` +314 'PROTO_ANSWER' was not declared in this scope +315 a failing BLOCKING check did not stop the compile — the object exists +316 /bin/sh: 1: -shared: not found +``` + +The xlings unit tests were checked the other way — by reverting +`payload_path_names_another_package` to its previous answer and re-running. +`PayloadOwnership.SameShortNameInAnotherNamespaceIsAnotherPackage` fails. + +Neither check is optional here. Three of these tests assert a *negative* +(nothing was produced, the marker was withheld, the phony is absent), and a +negative assertion against absent machinery passes by describing nothing. + +### 12.5 Unchanged + +§1's ordering constraint and §6.2's recommendation not to re-pin the index both +survived implementation. Track X removes the accidental collision, which is the +common case; publishing a deliberate one stays gated on the floor being adopted, +and that gate has no green checkmark. diff --git a/.github/actions/bootstrap-mcpp/action.yml b/.github/actions/bootstrap-mcpp/action.yml index ddd5a140..0dfcfb0e 100644 --- a/.github/actions/bootstrap-mcpp/action.yml +++ b/.github/actions/bootstrap-mcpp/action.yml @@ -25,7 +25,7 @@ inputs: # `package.name`, so one of the two was simply unreachable — and which one # depended on the machine, which is why CI failed on `compat:lua` on # Windows and `mcpplibs.capi:lua` on Linux. Never pin below that. - default: '2026.8.27.5' + default: '2026.8.30.2' cache-target: description: also restore/save target/ (build artifacts + BMIs) required: false diff --git a/.github/actions/setup-macos-llvm/action.yml b/.github/actions/setup-macos-llvm/action.yml index 14675983..a27f868a 100644 --- a/.github/actions/setup-macos-llvm/action.yml +++ b/.github/actions/setup-macos-llvm/action.yml @@ -15,7 +15,7 @@ inputs: # Floor imposed by the index, not a routine bump — see # .github/actions/bootstrap-mcpp/action.yml for why 0.4.69 is required # (two packages named `lua` in one repo need openxlings/xlings#381). - default: '2026.8.27.5' + default: '2026.8.30.2' runs: using: composite diff --git a/.github/workflows/bootstrap-macos.yml b/.github/workflows/bootstrap-macos.yml index d34cfe56..af574f32 100644 --- a/.github/workflows/bootstrap-macos.yml +++ b/.github/workflows/bootstrap-macos.yml @@ -17,7 +17,7 @@ jobs: # Dormant (workflow_dispatch only), but kept in step with the rest — # check_version_pins.sh holds it there. Floor: 0.4.69, below which the # index cannot resolve two packages that share a short name. - XLINGS_VERSION: '2026.8.27.5' + XLINGS_VERSION: '2026.8.30.2' steps: - uses: actions/checkout@v4 diff --git a/.github/workflows/ci-fresh-install.yml b/.github/workflows/ci-fresh-install.yml index 6ba2c894..a7aca72e 100644 --- a/.github/workflows/ci-fresh-install.yml +++ b/.github/workflows/ci-fresh-install.yml @@ -152,7 +152,7 @@ jobs: env: XLINGS_NON_INTERACTIVE: '1' run: | - curl -fsSL https://raw.githubusercontent.com/openxlings/xlings/main/tools/other/quick_install.sh | bash -s v2026.8.27.5 + curl -fsSL https://raw.githubusercontent.com/openxlings/xlings/main/tools/other/quick_install.sh | bash -s v2026.8.30.2 echo "$HOME/.xlings/subos/current/bin" >> "$GITHUB_PATH" - name: Install mcpp and config mirror @@ -293,7 +293,7 @@ jobs: - name: Install xlings + mcpp run: | - curl -fsSL https://raw.githubusercontent.com/openxlings/xlings/main/tools/other/quick_install.sh | bash -s v2026.8.27.5 + curl -fsSL https://raw.githubusercontent.com/openxlings/xlings/main/tools/other/quick_install.sh | bash -s v2026.8.30.2 # Deliberately NOT writing to $GITHUB_PATH here. On container # images that declare no PATH in their config (opensuse/ # tumbleweed), appending a single dir to GITHUB_PATH makes the @@ -364,7 +364,7 @@ jobs: # (older ones carry minos=15 and refuse to start). # v0.4.51+: in-process sha256 — this image has no sha256sum # binary, so pinned fetches failed before it. - curl -fsSL https://raw.githubusercontent.com/openxlings/xlings/main/tools/other/quick_install.sh | bash -s v2026.8.27.5 + curl -fsSL https://raw.githubusercontent.com/openxlings/xlings/main/tools/other/quick_install.sh | bash -s v2026.8.30.2 echo "$HOME/.xlings/subos/current/bin" >> "$GITHUB_PATH" - name: Install mcpp and config mirror diff --git a/.github/workflows/ci-linux-e2e.yml b/.github/workflows/ci-linux-e2e.yml index 2aa98cc6..c60fafc3 100644 --- a/.github/workflows/ci-linux-e2e.yml +++ b/.github/workflows/ci-linux-e2e.yml @@ -237,7 +237,7 @@ jobs: - name: Bootstrap xlings + released mcpp run: | - curl -fsSL https://raw.githubusercontent.com/openxlings/xlings/main/tools/other/quick_install.sh | bash -s v2026.8.27.5 + curl -fsSL https://raw.githubusercontent.com/openxlings/xlings/main/tools/other/quick_install.sh | bash -s v2026.8.30.2 export PATH="$HOME/.xlings/subos/current/bin:$PATH" xlings update xlings install mcpp -y -g diff --git a/.github/workflows/cross-build-test.yml b/.github/workflows/cross-build-test.yml index 5623dd9f..ac524491 100644 --- a/.github/workflows/cross-build-test.yml +++ b/.github/workflows/cross-build-test.yml @@ -122,7 +122,7 @@ jobs: # release assets were uploaded in a broken state (records present, # blobs missing → 404 on GET); re-uploaded clean. The stale-INDEX # half is handled by the marker-clear below. - XLINGS_VERSION: '2026.8.27.5' + XLINGS_VERSION: '2026.8.30.2' run: | tarball="xlings-${XLINGS_VERSION}-linux-x86_64.tar.gz" bash "$GITHUB_WORKSPACE/.github/tools/fetch_release.sh" \ @@ -263,7 +263,7 @@ jobs: - name: Bootstrap mcpp via xlings env: XLINGS_NON_INTERACTIVE: '1' - XLINGS_VERSION: '2026.8.27.5' + XLINGS_VERSION: '2026.8.30.2' run: | tarball="xlings-${XLINGS_VERSION}-linux-x86_64.tar.gz" bash "$GITHUB_WORKSPACE/.github/tools/fetch_release.sh" \ diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 87da0fc8..0f19082f 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -96,7 +96,7 @@ jobs: # Pin xlings to a known-good version. The upstream install # script always grabs `latest` (no version override), so we # download + self-install manually to avoid broken releases. - XLINGS_VERSION: '2026.8.27.5' + XLINGS_VERSION: '2026.8.30.2' run: | if [ ! -x "$HOME/.xlings/subos/default/bin/xlings" ]; then tarball="xlings-${XLINGS_VERSION}-linux-x86_64.tar.gz" @@ -289,7 +289,7 @@ jobs: - name: Bootstrap mcpp via xlings env: XLINGS_NON_INTERACTIVE: '1' - XLINGS_VERSION: '2026.8.27.5' + XLINGS_VERSION: '2026.8.30.2' run: | tarball="xlings-${XLINGS_VERSION}-linux-x86_64.tar.gz" bash "$GITHUB_WORKSPACE/.github/tools/fetch_release.sh" \ @@ -360,7 +360,7 @@ jobs: # below are pinned to the same version as XLINGS_VERSION; they are # NOT interpolated from it, so check_version_pins.sh scans for them # explicitly (they were absent from the old lock-step comment). - XLA="xlings-2026.8.27.5-linux-aarch64.tar.gz" + XLA="xlings-2026.8.30.2-linux-aarch64.tar.gz" # NOT fetch_release.sh: this asset is OPTIONAL and the `if` is the # point — an arch with no prebuilt xlings must fall through quietly, # while the helper retries a 404 five times before giving up. The one @@ -369,9 +369,9 @@ jobs: # cover it. if curl -fsSL --retry 3 --retry-delay 2 --retry-all-errors \ --connect-timeout 20 --max-time 600 -o "/tmp/$XLA" \ - "https://github.com/openxlings/xlings/releases/download/v2026.8.27.5/$XLA"; then + "https://github.com/openxlings/xlings/releases/download/v2026.8.30.2/$XLA"; then tar -xzf "/tmp/$XLA" -C /tmp - XLBIN=$(find /tmp/xlings-2026.8.27.5-linux-aarch64 -path '*/bin/xlings' -type f | head -1) + XLBIN=$(find /tmp/xlings-2026.8.30.2-linux-aarch64 -path '*/bin/xlings' -type f | head -1) if [ -n "$XLBIN" ]; then mkdir -p "$STAGING/$WRAPPER/registry/bin" cp "$XLBIN" "$STAGING/$WRAPPER/registry/bin/xlings" @@ -449,7 +449,7 @@ jobs: - name: Bootstrap mcpp via xlings env: XLINGS_NON_INTERACTIVE: '1' - XLINGS_VERSION: '2026.8.27.5' + XLINGS_VERSION: '2026.8.30.2' run: | if [ ! -x "$HOME/.xlings/subos/default/bin/xlings" ]; then WORK=$(mktemp -d) @@ -632,7 +632,7 @@ jobs: shell: bash env: XLINGS_NON_INTERACTIVE: '1' - XLINGS_VERSION: '2026.8.27.5' + XLINGS_VERSION: '2026.8.30.2' run: | # Captured before the `cd` below, in POSIX form: this step never # returns to the workspace, and GITHUB_WORKSPACE is a backslash diff --git a/src/xlings/xlings.cppm b/src/xlings/xlings.cppm index 40ba1983..f4ead099 100644 --- a/src/xlings/xlings.cppm +++ b/src/xlings/xlings.cppm @@ -46,10 +46,13 @@ namespace pinned { // actions, which is how CI's sandbox sat on 0.4.30 unnoticed while // everything else had moved on. Don't reintroduce a hand-maintained list. // - // ⚠️ 2026.8.27.5 is a FLOOR, not just the current pick. Below 2026.8.27.2 - // the bundled xlings takes a subos's runtime binding from a compiled-in - // constant, so a home can declare one glibc and install another the - // moment the index publishes a packaging revision -- and mcpp is the + // ⚠️ THIS IS A FLOOR, not just the current pick, and it has been raised + // twice for reasons that both still hold. + // + // First, at 2026.8.27.5. Below 2026.8.27.2 the bundled xlings takes a + // subos's runtime binding from a compiled-in constant, so a home can + // declare one glibc and install another the moment the index publishes a + // packaging revision -- and mcpp is the // party that notices, because select_glibc_payload_lib looks up the // payload directory by the binding's exact version and refuses to fall // back: @@ -62,7 +65,24 @@ namespace pinned { // 2026.8.27.4 and .5, which read the index, stayed consistent. .5 also // makes the declaration outrank the index during resolution, so it holds // even when `latest` is not the highest entry in the table. - inline constexpr std::string_view kXlingsVersion = "2026.8.27.5"; + // + // Second, at 2026.8.30.2 (mcpp#533). Below it, xlings answered "is this + // package already installed" from the xvm version database keyed on the + // BARE SHORT NAME, so a package skipped its own `install()` whenever any + // other namespace held the same `@` — + // `compat:libdrm@2.4.123` against the + // `xim:libdrm@2.4.123` that Mesa pulls in. The descriptor's tree was never + // built, and mcpp is again the party that notices, four layers later: + // + // /bin/sh: 1: -shared: not found + // + // mcpp now refuses that link unit by name and withholds `.mcpp_ok` from a + // directory holding nothing the package installed, so the failure is + // legible on any client. Only a client at or above this floor INSTALLS + // correctly — which is why this is a floor and not a preference, and why + // an index must still not publish a deliberately colliding + // `@` (see .agents/docs/2026-08-30-cross-repo-fix-plan §1). + inline constexpr std::string_view kXlingsVersion = "2026.8.30.2"; inline constexpr std::string_view kNasmVersion = "3.02"; }