fix(build): a package's generated inputs come before its compiles; an empty link is refused (2026.8.30.1) - #536
Merged
Conversation
… empty link is refused (2026.8.30.1)
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/<name>.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
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 `<name>@<version>`. 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 `<name>@<version>` 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #534. Addresses the mcpp-side half of #533 (the xlings-side root cause is openxlings/xlings#576).
Analysis and the cross-repo plan are in
.agents/docs/2026-08-30-issues-532-533-534-analysis.mdand.agents/docs/2026-08-30-cross-repo-fix-plan-532-533-534.md. Every claim below has a reproduction recorded there.One change of mind, three places
Each defect here is somewhere that answered "is this ready?" from a proxy instead of from the artifact.
.mcpp_okThat is why they are one PR: they are one decision, applied where it was missing.
#534 — the filed diagnosis was wrong, and the defect is larger
Reported as an intermittent race in dependency packages. It is deterministic, and it reaches the root project too.
role = "source"is documented as "outputs join the compile set; the compile edge consumes them". True of a generated.cpp; false of a generated.h, which is reached through-I, never appears as an edge input, and whose depfile does not exist until a compile has already succeeded. So an action whose outputs were all headers had a node inbuild.ninjathat nothing could reach — notdefault(Source outputs are excluded on purpose), not the goal phony (objects and link outputs only), no consuming edge.Five consecutive builds, header deleted before each: 0 bytes every time. What made it look like a race is that
prepare_actionswrote a zero-byte placeholder for every declared Source output including headers, so the file was on disk whether or not the generator ran — and the reporter reasonably concluded from its presence that the action had run.Fix. A per-package phony over that package's gating action outputs; every compile edge of the package takes it order-only. Per package rather than build-wide because
include_dircolours only the declaring package's own TUs, and a build-wide edge would encode a dependency that does not exist while landing on the critical path of a build whose wall clock is its critical path.blocking = trueon achecknow works. It was typed, emitted over the build-program protocol, parsed, documented in two languages and demonstrated in a shipped example — and read by nothing. It needed the same mechanism.#533 — an empty link unit, reported as a shell error
A dependency whose
install()was skipped left a version directory with no sources. mcpp planned its shared library anyway:Reproduced from a pure mcpp input — a
kind = "shared"target matching zero sources — with no xlings involved.The static case is why this is refused at plan time rather than given a better linker message:
Consumers then fail with undefined symbols, one repository further from the cause. Confirmed against the released binary:
Finished devplus an 8-byte.a.Also here:
ccis emitted unconditionally, for the reasonc_ldflagstwenty-six lines below already carried (#426) — the rule had been written down for one variable ofc_link/c_sharedand not the other.Guards, not care
Both new checks scan the emitted manifest, the way
check_inline_command_lengthsalready does and for the reason its comment gives — "a new edge kind is covered the day it is added".check_rule_commands_name_a_program— a rule's command must begin with a program. Deliberately not "no undefined variables": ninja's empty expansion is a feature$soname_flagand$unit_ldflagsrely on, and that check would have needed an allowlist of exceptions.check_action_ordering— every compile edge of a package that generates its own inputs must wait for them, with the denominator. Seven call sites append the order-only string today; an eighth added later without it would reintroduce 依赖包 build.mcpp 的 mcpp::action 产物未排在该包自身编译之前(竞态,间歇性失败) #534 for that edge kind, silently.Seamless upgrade
.mcpp_okis checked on the fast path too, not only where it is written.is_install_completeis marker-only by design, so a store already poisoned by #533 carries a stale marker that short-circuits forever — guarding only the write site would help people who have not hit the bug and do nothing for the ones who filed it. Nobody has to be told which directory to delete.The marker is withheld, not fatal, when nothing was installed: a package may legitimately install no payload (xlings type-only packages, and #531 provisioning). The cost of being wrong is one cheap re-check per build; the cost of a wrong marker is permanent.
Behaviour changes
.a/bin/sh: 1: -shared: not foundsourceaction.mcpp_okover an empty installTests
e2e 314 (a dependency generating its only header), 315 (blocking gates the compile; non-blocking does not), 316 (empty shared and static refused; a populated one still builds). All three were run against the pre-fix binary and all three fail there — they discriminate rather than describe.
14 unit cases over the two emitter guards, the ordering denominator, and the marker evidence including the poisoned-store heal.
Local:
mcpp buildclean,mcpp test96 passed / 0 failed, all five.github/tools/check_*.shguards pass.Still to come on this branch
The
kXlingsVersionfloor bump to the xlings release carrying openxlings/xlings#576. Per §1 of the plan doc, that does not authorise an index change — publishing a colliding<name>@<version>stays unsafe until the floor is adopted, and §6.2 recommends not doing it at all.