Decide a build step by its exit code, not by whether it printed

catbot commented

2026-08-26 00:52:50 +00:00

Member

A compiler warning failed the build, because success was inferred from whether the command printed anything rather than from its exit code. Warnings are only emitted when a translation unit is actually recompiled, so the same unchanged source passed or failed depending on whether its object happened to be up to date — flaky-looking tests locally, and a cold CI checkout surfacing every latent warning in a project at once.

The fix

RunCommand merged stderr into stdout, discarded pclose's status and returned the text; every compile, link and archive site read "printed something" as "failed". It's gone, replaced by RunBuildCommand, which goes through the existing RunCommandChecked:

exit 0 → returns "", so every caller's if (!result.empty()) error path is unchanged, and any warnings go to the new Progress::Diagnostic (status line cleared, written whole under the progress mutex so parallel compiles don't interleave their note: continuations).
exit non-zero → returns the diagnostics as before.
killed without printing → reports the signal. Previously that read as success and the build carried on with a missing object; an OOM-killed compiler is the common way in.

Every RunCommand call site was a build step, so the function is removed outright rather than left around to be reached for again. Covers the paths the issue lists: precompile and BMI→object (Interface), implementation units (Implementation), C and CUDA, every link/archive/shared-library step, BuildStdPcm, and LoadProject's project.cpp compile.

Warnings are now shown, which they weren't in either direction before — invisible on an incremental build, fatal on a cold one. Failing on them stays the project's choice via -Werror in compileFlags, which then fails the same way every time. README updated to say so.

Tests

New tests/CompilerWarning. The fixture puts a #warning in each kind of source the build compiles — module interface, module implementation, C, consumer — plus an unknown -z value so ld.lld warns on the link step too. It asserts the build succeeds cold (every unit compiling, every warning emitted), succeeds with nothing to do, and succeeds again after each source is touched back into staleness. The staged copy lives outside the repo because the whole test depends on units compiling for the first time.

The last pass breaks a source on purpose — "never fails" would satisfy everything above just as well — and asserts both that it fails and that it still reports the compiler's diagnostic.

Verified the test catches the old behaviour: with RunBuildCommand temporarily reverted to returning the output regardless, it fails with cold build failed: .../counter.c:4:2: warning: "C source is noisy" [-W#warnings].

crafter-build test: 21 passed, 0 failed (from a crafter-build clean). crafter-build lint: clean, 44 files, 17 rules. The wasi example still builds for wasm32-wasip1.

Resolves #32

A compiler warning failed the build, because success was inferred from whether the command printed anything rather than from its exit code. Warnings are only emitted when a translation unit is actually recompiled, so the same unchanged source passed or failed depending on whether its object happened to be up to date — flaky-looking tests locally, and a cold CI checkout surfacing every latent warning in a project at once. ## The fix `RunCommand` merged stderr into stdout, discarded `pclose`'s status and returned the text; every compile, link and archive site read "printed something" as "failed". It's gone, replaced by `RunBuildCommand`, which goes through the existing `RunCommandChecked`: - exit 0 → returns `""`, so every caller's `if (!result.empty())` error path is unchanged, and any warnings go to the new `Progress::Diagnostic` (status line cleared, written whole under the progress mutex so parallel compiles don't interleave their `note:` continuations). - exit non-zero → returns the diagnostics as before. - killed without printing → reports the signal. Previously that read as success and the build carried on with a missing object; an OOM-killed compiler is the common way in. Every `RunCommand` call site was a build step, so the function is removed outright rather than left around to be reached for again. Covers the paths the issue lists: precompile and BMI→object (`Interface`), implementation units (`Implementation`), C and CUDA, every link/archive/shared-library step, `BuildStdPcm`, and `LoadProject`'s `project.cpp` compile. Warnings are now *shown*, which they weren't in either direction before — invisible on an incremental build, fatal on a cold one. Failing on them stays the project's choice via `-Werror` in `compileFlags`, which then fails the same way every time. README updated to say so. ## Tests New `tests/CompilerWarning`. The fixture puts a `#warning` in each kind of source the build compiles — module interface, module implementation, C, consumer — plus an unknown `-z` value so `ld.lld` warns on the link step too. It asserts the build succeeds cold (every unit compiling, every warning emitted), succeeds with nothing to do, and succeeds again after each source is touched back into staleness. The staged copy lives outside the repo because the whole test depends on units compiling for the first time. The last pass breaks a source on purpose — "never fails" would satisfy everything above just as well — and asserts both that it fails and that it still reports the compiler's diagnostic. Verified the test catches the old behaviour: with `RunBuildCommand` temporarily reverted to returning the output regardless, it fails with `cold build failed: .../counter.c:4:2: warning: "C source is noisy" [-W#warnings]`. `crafter-build test`: 21 passed, 0 failed (from a `crafter-build clean`). `crafter-build lint`: clean, 44 files, 17 rules. The wasi example still builds for `wasm32-wasip1`. Resolves #32

catbot added 3 commits

2026-08-26 00:52:50 +00:00

fix: decide a build step by its exit code, not by whether it printed 8e2d21a2bc

RunCommand merged stderr into stdout, dropped pclose's status and returned
the text; every compile, link and archive site then read "printed something"
as "failed". A warning is printing something, so a translation unit that
warned failed the build — but only on the run that actually recompiled it,
since warnings aren't re-emitted for an object that's already up to date.
The same unchanged source therefore passed or failed depending on the state
of the build tree: flaky-looking tests locally, and a cold CI checkout
surfacing every latent warning in a project at once as unrelated failures.

RunCommand is gone, replaced by RunBuildCommand: it goes through
RunCommandChecked, returns "" when the command exited 0 (so every caller's
`if (!result.empty())` error path is unchanged) and hands any warnings to
the new Progress::Diagnostic instead of to the error path. That also closes
the quiet half of the bug — a compiler killed by the OOM killer prints
nothing, so it used to read as success and leave the build carrying on with
a missing object; it now reports the signal that killed it.

Warnings are now shown rather than swallowed, which they weren't in either
direction before: invisible on an incremental build, fatal on a cold one.
Failing on them stays a project's choice, via -Werror in compileFlags.

test: a warning on any compile path must not fail the build 67d31689bc

The fixture puts a #warning in each kind of source the build compiles —
module interface, module implementation, C, consumer — plus an unknown -z
value so ld.lld warns on the link step too, and asserts the build succeeds
cold (every unit compiling, every warning emitted), succeeds with nothing to
do, and succeeds again after each source is touched back into staleness.
The staged copy lives outside the repo because the whole test depends on
units compiling for the first time.

The last pass breaks a source on purpose: "never fails" would satisfy
everything above just as well, so the error path is asserted too — both that
it fails and that it still reports the compiler's diagnostic.

docs: state that build steps fail on exit status, not on output

CI / build-test-release (pull_request) Successful in 5m31s

Details

cec20717d0

catbot merged commit 802caac1c2 into master

2026-08-26 00:52:58 +00:00

catbot deleted branch claude/issue-32

2026-08-26 00:52:58 +00:00

catbot referenced this pull request from a commit

2026-08-26 00:52:59 +00:00

Merge pull request 'Decide a build step by its exit code, not by whether it printed' (#33) from claude/issue-32 into master

Rows
Columns

Decide a build step by its exit code, not by whether it printed #33

The fix

Tests