A compiler warning fails the build, so a test's verdict depends on whether its TU was recompiled #32

New issue

Closed

opened 2026-08-26 00:33:13 +00:00 by jorijnvdgraaf · 0 comments

jorijnvdgraaf commented

2026-08-26 00:33:13 +00:00

Owner

Summary

A compiler warning fails the build, because success is inferred from whether
the compiler printed anything rather than from its exit code. Warnings are only
emitted when a translation unit is actually recompiled, so the same unchanged
source passes or fails depending on whether its object file happened to be
up to date.

$ crafter-build test          # fresh compile
❌ ShouldPassDespiteWarning (0ms) exit -1
    build failed: .../main.cpp:6:31: warning: trigraph ignored [-Wtrigraphs]
1 failed

$ crafter-build test          # same source, nothing to recompile
✅ ShouldPassDespiteWarning (4ms)
1 passed

Impact

It presents as flaky tests, and it is very convincing: a test "fails", you
re-run it, it passes, and you conclude the first result was a stale artifact or
a race. I misdiagnosed it three times in one session before reading the source —
twice writing it off as a stale build tree, once filing it in my own notes as an
intermittent crash.

The failure mode is worst exactly where it costs most:

Anything that forces a wide recompile — a dependency bump, a changed
module interface, a clean, a fresh CI checkout — surfaces every latent
warning in the project at once, as a wall of unrelated test failures.
CI always builds cold, so CI fails on warnings while local incremental
builds pass. The reverse of the usual "works on my machine".
The (0ms) and exit -1 in the output read like the binary failed to launch,
not like a compile diagnostic. The reason is printed, but on indented
continuation lines that are easy to filter out or lose in a parallel run's
interleaved output.

Reproducer

A project with one test whose source emits a warning and returns 0:

project.cpp

import std;
import Crafter.Build;
namespace fs = std::filesystem;
using namespace Crafter;

extern "C" Configuration CrafterBuildProject(std::span<const std::string_view> args) {
    Configuration cfg;
    cfg.path = "./";
    cfg.name = "warnrepro";
    cfg.outputName = "warnrepro";
    cfg.type = ConfigurationType::Executable;
    ApplyStandardArgs(cfg, args);
    {
        std::array<fs::path, 0> noIfaces{};
        std::array<fs::path, 0> noImpls{};
        cfg.GetInterfacesAndImplementations(noIfaces, noImpls);
    }
    cfg.AddTest("ShouldPassDespiteWarning").Timeout(std::chrono::seconds{60});
    return cfg;
}

tests/ShouldPassDespiteWarning/main.cpp

import std;

int main() {
    // Emits -Wtrigraphs: the ??' sequence below is a trigraph clang ignores.
    // The test itself passes -- it prints and returns 0.
    std::println("what is this??'");
    return 0;
}

$ crafter-build test   # ❌ build failed: ... warning: trigraph ignored
$ crafter-build test   # ✅ 1 passed
$ crafter-build test   # ✅ 1 passed
$ touch tests/ShouldPassDespiteWarning/main.cpp
$ crafter-build test   # ❌ again

-Wtrigraphs is convenient because it is on by default at the flags this build
uses; any enabled-by-default warning does the same thing.

Diagnosis

RunCommand merges stderr into stdout and returns the combined text, throwing
away pclose's status (implementations/Crafter.Build-Platform.cpp:764):

std::string Crafter::RunCommand(const std::string_view cmd) {
    ...
    std::string with = std::format("{} 2>&1", cmd);
    FILE* pipe = popen(with.c_str(), "r");
    ...
    pclose(pipe);      // status discarded
    return result;
}

Every caller then treats "printed something" as "failed". For a test's
main.cpp that is implementations/Crafter.Build-Implementation.cpp:55:

std::string result = RunCommand(std::format("{0} {1}.cpp -c -MD -MF {2}_impl.o.d -o {2}_impl.o", ...));

bool expected = false;
if(!result.empty() && buildCancelled.compare_exchange_strong(expected, true)) {
    buildError = std::move(result);
}

The same shape is in Crafter.Build-Interface.cpp:82,97,206,221 (precompile and
object steps), Crafter.Build-Clang.cpp:791 (C), :815 (CUDA) and :1168-1232
(every link / archive / shared-library step), and it surfaces to tests at
Crafter.Build-Test.cpp:657:

if (!br.result.empty()) {
    r.outcome = TestOutcome::Fail;
    r.output = std::format("build failed: {}", br.result);
    r.exitCode = -1;

Note that the link steps are covered too, so a linker warning fails a build the
same way.

Suggested fix

RunCommandChecked sits directly below RunCommand in the same file
(Crafter.Build-Platform.cpp:784) and already captures the exit code, the
signal and the crash flag:

std::int32_t status = pclose(pipe);
if (WIFEXITED(status)) {
    result.exitCode = WEXITSTATUS(status);
} else if (WIFSIGNALED(status)) { ... }

So the compile and link sites can switch to it and branch on exitCode != 0,
keeping output for display. That also fixes a second, quieter problem: today a
compiler that is killed (OOM, for instance) but printed nothing is treated
as success, and the build proceeds with a missing object.

Whether warnings should still be shown on a successful build is a separate
question — printing them without failing would be a strict improvement, since
right now a warning on an incremental build is invisible until the next cold
one.

A note on intent

If failing on warnings is deliberate, the current behaviour still isn't it: a
policy that only applies when a file happens to be recompiled is not a policy.
Making it explicit and consistent — -Werror in the compile flags, failing on
the exit code — would fail the same way every time, which is the part that
matters. The current arrangement is the worst of both: warnings are fatal, but
only sometimes, and the resulting failure looks like something else entirely.

Environment

Crafter.Build at 046b5ee, clang 22.1.8, x86_64-pc-linux-gnu, libc++.
Found while building a QUIC ingress client whose test suite kept "flaking"
after dependency bumps.

## Summary A compiler **warning** fails the build, because success is inferred from whether the compiler printed anything rather than from its exit code. Warnings are only emitted when a translation unit is actually recompiled, so the same unchanged source passes or fails depending on whether its object file happened to be up to date. ``` $ crafter-build test # fresh compile ❌ ShouldPassDespiteWarning (0ms) exit -1 build failed: .../main.cpp:6:31: warning: trigraph ignored [-Wtrigraphs] 1 failed $ crafter-build test # same source, nothing to recompile ✅ ShouldPassDespiteWarning (4ms) 1 passed ``` ## Impact It presents as flaky tests, and it is very convincing: a test "fails", you re-run it, it passes, and you conclude the first result was a stale artifact or a race. I misdiagnosed it three times in one session before reading the source — twice writing it off as a stale build tree, once filing it in my own notes as an intermittent crash. The failure mode is worst exactly where it costs most: - **Anything that forces a wide recompile** — a dependency bump, a changed module interface, a `clean`, a fresh CI checkout — surfaces every latent warning in the project at once, as a wall of unrelated test failures. - **CI always builds cold**, so CI fails on warnings while local incremental builds pass. The reverse of the usual "works on my machine". - The `(0ms)` and `exit -1` in the output read like the binary failed to launch, not like a compile diagnostic. The reason *is* printed, but on indented continuation lines that are easy to filter out or lose in a parallel run's interleaved output. ## Reproducer A project with one test whose source emits a warning and returns 0: `project.cpp` ```cpp import std; import Crafter.Build; namespace fs = std::filesystem; using namespace Crafter; extern "C" Configuration CrafterBuildProject(std::span<const std::string_view> args) { Configuration cfg; cfg.path = "./"; cfg.name = "warnrepro"; cfg.outputName = "warnrepro"; cfg.type = ConfigurationType::Executable; ApplyStandardArgs(cfg, args); { std::array<fs::path, 0> noIfaces{}; std::array<fs::path, 0> noImpls{}; cfg.GetInterfacesAndImplementations(noIfaces, noImpls); } cfg.AddTest("ShouldPassDespiteWarning").Timeout(std::chrono::seconds{60}); return cfg; } ``` `tests/ShouldPassDespiteWarning/main.cpp` ```cpp import std; int main() { // Emits -Wtrigraphs: the ??' sequence below is a trigraph clang ignores. // The test itself passes -- it prints and returns 0. std::println("what is this??'"); return 0; } ``` ``` $ crafter-build test # ❌ build failed: ... warning: trigraph ignored $ crafter-build test # ✅ 1 passed $ crafter-build test # ✅ 1 passed $ touch tests/ShouldPassDespiteWarning/main.cpp $ crafter-build test # ❌ again ``` `-Wtrigraphs` is convenient because it is on by default at the flags this build uses; any enabled-by-default warning does the same thing. ## Diagnosis `RunCommand` merges stderr into stdout and returns the combined text, throwing away `pclose`'s status (`implementations/Crafter.Build-Platform.cpp:764`): ```cpp std::string Crafter::RunCommand(const std::string_view cmd) { ... std::string with = std::format("{} 2>&1", cmd); FILE* pipe = popen(with.c_str(), "r"); ... pclose(pipe); // status discarded return result; } ``` Every caller then treats "printed something" as "failed". For a test's `main.cpp` that is `implementations/Crafter.Build-Implementation.cpp:55`: ```cpp std::string result = RunCommand(std::format("{0} {1}.cpp -c -MD -MF {2}_impl.o.d -o {2}_impl.o", ...)); bool expected = false; if(!result.empty() && buildCancelled.compare_exchange_strong(expected, true)) { buildError = std::move(result); } ``` The same shape is in `Crafter.Build-Interface.cpp:82,97,206,221` (precompile and object steps), `Crafter.Build-Clang.cpp:791` (C), `:815` (CUDA) and `:1168-1232` (every link / archive / shared-library step), and it surfaces to tests at `Crafter.Build-Test.cpp:657`: ```cpp if (!br.result.empty()) { r.outcome = TestOutcome::Fail; r.output = std::format("build failed: {}", br.result); r.exitCode = -1; ``` Note that the link steps are covered too, so a linker warning fails a build the same way. ## Suggested fix `RunCommandChecked` sits directly below `RunCommand` in the same file (`Crafter.Build-Platform.cpp:784`) and already captures the exit code, the signal and the crash flag: ```cpp std::int32_t status = pclose(pipe); if (WIFEXITED(status)) { result.exitCode = WEXITSTATUS(status); } else if (WIFSIGNALED(status)) { ... } ``` So the compile and link sites can switch to it and branch on `exitCode != 0`, keeping `output` for display. That also fixes a second, quieter problem: today a compiler that is **killed** (OOM, for instance) but printed nothing is treated as success, and the build proceeds with a missing object. Whether warnings should still be *shown* on a successful build is a separate question — printing them without failing would be a strict improvement, since right now a warning on an incremental build is invisible until the next cold one. ## A note on intent If failing on warnings is deliberate, the current behaviour still isn't it: a policy that only applies when a file happens to be recompiled is not a policy. Making it explicit and consistent — `-Werror` in the compile flags, failing on the exit code — would fail the same way every time, which is the part that matters. The current arrangement is the worst of both: warnings are fatal, but only sometimes, and the resulting failure looks like something else entirely. ## Environment - `Crafter.Build` at `046b5ee`, clang 22.1.8, x86_64-pc-linux-gnu, libc++. - Found while building a QUIC ingress client whose test suite kept "flaking" after dependency bumps.

jorijnvdgraaf added the

claude:ready

label

2026-08-26 00:38:33 +00:00