Skip to content

Latest commit

 

History

History
443 lines (342 loc) · 19.7 KB

File metadata and controls

443 lines (342 loc) · 19.7 KB

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:2173Source 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 || <phony>. 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

// 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 <ns>:<name>@<ver>'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

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:

[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/<tail>.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 namespaceslibpng, expat, zlib, cairo, fontconfig, linux-headers, nasm, ncurses, mcpp, xlings, and more. The rule #533 infers ("no compat package may share <shortname>@<version> 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 + `
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