From 8e2d21a2bc4f0ca7d59b208ea0ae05b7dc82e56d Mon Sep 17 00:00:00 2001 From: catbot Date: Wed, 26 Aug 2026 00:51:15 +0000 Subject: [PATCH] fix: decide a build step by its exit code, not by whether it printed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- implementations/Crafter.Build-Clang.cpp | 22 ++--- .../Crafter.Build-Implementation.cpp | 2 +- implementations/Crafter.Build-Interface.cpp | 8 +- implementations/Crafter.Build-Platform.cpp | 80 ++++++++----------- implementations/Crafter.Build-Progress.cpp | 12 +++ interfaces/Crafter.Build-Platform.cppm | 12 ++- interfaces/Crafter.Build-Progress.cppm | 5 ++ 7 files changed, 77 insertions(+), 64 deletions(-) diff --git a/implementations/Crafter.Build-Clang.cpp b/implementations/Crafter.Build-Clang.cpp index 9262475..cd929bc 100644 --- a/implementations/Crafter.Build-Clang.cpp +++ b/implementations/Crafter.Build-Clang.cpp @@ -788,7 +788,7 @@ BuildResult Crafter::Build(Configuration& config, std::unordered_map_impl.o.d beside the object for Check to read. - std::string result = RunCommand(std::format("{0} {1}.cpp -c -MD -MF {2}_impl.o.d -o {2}_impl.o", clang, path.string(), (buildDir/path.filename()).string())); + std::string result = RunBuildCommand(std::format("{0} {1}.cpp -c -MD -MF {2}_impl.o.d -o {2}_impl.o", clang, path.string(), (buildDir/path.filename()).string())); bool expected = false; if(!result.empty() && buildCancelled.compare_exchange_strong(expected, true)) { diff --git a/implementations/Crafter.Build-Interface.cpp b/implementations/Crafter.Build-Interface.cpp index 526cf3b..2fb7b37 100644 --- a/implementations/Crafter.Build-Interface.cpp +++ b/implementations/Crafter.Build-Interface.cpp @@ -79,7 +79,7 @@ namespace Crafter { // -MD records every header the preamble pulled in, next to the BMI as // .pcm.d, so the next Check can see an edit to one of them. - std::string result = RunCommand(std::format("{0} {1}.cppm --precompile -MD -MF {2}.pcm.d -o {2}.pcm", clang, path.string(), (pcmDir/path.filename()).string())); + std::string result = RunBuildCommand(std::format("{0} {1}.cppm --precompile -MD -MF {2}.pcm.d -o {2}.pcm", clang, path.string(), (pcmDir/path.filename()).string())); if (!result.empty()) { bool expected = false; @@ -94,7 +94,7 @@ namespace Crafter { compiled.store(true); compiled.notify_all(); - result = RunCommand(std::format("{} -Wno-unused-command-line-argument {}.pcm -c -o {}.o", clang, (pcmDir/path.filename()).string(), (buildDir/path.filename()).string())); + result = RunBuildCommand(std::format("{} -Wno-unused-command-line-argument {}.pcm -c -o {}.o", clang, (pcmDir/path.filename()).string(), (buildDir/path.filename()).string())); if (!result.empty()) { bool expected = false; @@ -203,7 +203,7 @@ namespace Crafter { // -MD records every header the preamble pulled in, next to the BMI as // .pcm.d, so the next Check can see an edit to one of them. - std::string result = RunCommand(std::format("{0} {1}.cppm --precompile -MD -MF {2}.pcm.d -o {2}.pcm", clang, path.string(), (pcmDir/path.filename()).string())); + std::string result = RunBuildCommand(std::format("{0} {1}.cppm --precompile -MD -MF {2}.pcm.d -o {2}.pcm", clang, path.string(), (pcmDir/path.filename()).string())); if (!result.empty()) { bool expected = false; @@ -218,7 +218,7 @@ namespace Crafter { compiled.store(true); compiled.notify_all(); - result = RunCommand(std::format("{} -Wno-unused-command-line-argument {}.pcm -c -o {}.o", clang, (pcmDir/path.filename()).string(), (buildDir/path.filename()).string())); + result = RunBuildCommand(std::format("{} -Wno-unused-command-line-argument {}.pcm -c -o {}.o", clang, (pcmDir/path.filename()).string(), (buildDir/path.filename()).string())); if (!result.empty()) { bool expected = false; diff --git a/implementations/Crafter.Build-Platform.cpp b/implementations/Crafter.Build-Platform.cpp index 051dcae..4e4196b 100644 --- a/implementations/Crafter.Build-Platform.cpp +++ b/implementations/Crafter.Build-Platform.cpp @@ -259,28 +259,34 @@ bool Crafter::MatchAny(std::span globs, std::string_view name return false; } -#if defined(CRAFTER_BUILD_CONFIGURATION_TARGET_x86_64_pc_windows_msvc) || defined(CRAFTER_BUILD_CONFIGURATION_TARGET_x86_64_w64_mingw32) -std::string Crafter::RunCommand(const std::string_view cmd) { +// See the declaration in Crafter.Build-Platform.cppm for why this branches on +// the exit code rather than on whether the command said anything. +std::string Crafter::RunBuildCommand(std::string_view cmd) { Progress::EchoCommand(cmd); - std::array buffer; - std::string result; + CommandResult r = RunCommandChecked(cmd); - // Use cmd.exe to interpret redirection - std::string with = std::format("cmd /C \"{} 2>&1\"", std::string(cmd)); - - FILE* pipe = _popen(with.c_str(), "r"); - if (!pipe) { - throw std::runtime_error("_popen() failed!"); + if (r.exitCode == 0) { + // Warnings from a step that succeeded. Show them — until now they were + // invisible on an incremental build and fatal on a cold one. + if (!r.output.empty()) Progress::Diagnostic(r.output); + return ""; } - while (fgets(buffer.data(), static_cast(buffer.size()), pipe) != nullptr) { - result += buffer.data(); + // A compiler killed outright (the OOM killer is the common one) prints + // nothing, so the old "output means failure" rule read it as success and + // let the build carry on with a missing object. Say what happened instead. + if (r.output.empty()) { + return r.crashed + ? std::format("terminated by signal {}: {}", r.signal, cmd) + : std::format("exited with code {}: {}", r.exitCode, cmd); } - - _pclose(pipe); - return result; + if (r.crashed) { + return std::format("{}\nterminated by signal {}", r.output, r.signal); + } + return r.output; } +#if defined(CRAFTER_BUILD_CONFIGURATION_TARGET_x86_64_pc_windows_msvc) || defined(CRAFTER_BUILD_CONFIGURATION_TARGET_x86_64_w64_mingw32) CommandResult Crafter::RunCommandChecked(std::string_view cmd) { std::array buffer; CommandResult result{}; @@ -429,7 +435,7 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) { CacheLock lock(stdPcm.parent_path()); if(!fs::exists(stdPcm) || fs::last_write_time(stdPcm) < fs::last_write_time(stdcppm)) { - return RunCommand(std::format("clang++ --target={} -march={} -mtune={} -isystem %LIBCXX_DIR%\\include\\c++\\v1 -nostdinc++ -nostdlib++ -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile %LIBCXX_DIR%\\modules\\c++\\v1\\std.cppm -o {}", config.target, config.march, config.mtune, stdPcm.string())); + return RunBuildCommand(std::format("clang++ --target={} -march={} -mtune={} -isystem %LIBCXX_DIR%\\include\\c++\\v1 -nostdinc++ -nostdlib++ -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile %LIBCXX_DIR%\\modules\\c++\\v1\\std.cppm -o {}", config.target, config.march, config.mtune, stdPcm.string())); } return ""; } @@ -521,7 +527,7 @@ Configuration Crafter::LoadProject(const fs::path& projectFile, std::span= fs::last_write_time(stdcppm)) { return ""; } - return RunCommand(std::format( + return RunBuildCommand(std::format( "clang++ --target={} -march={} -mtune={} -isystem %LIBCXX_DIR%\\include\\c++\\v1 " "-nostdinc++ -nostdlib++ -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier " "--precompile %LIBCXX_DIR%\\modules\\c++\\v1\\std.cppm -o {}", @@ -613,17 +619,17 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) { if (fs::exists(stdPcm) && fs::last_write_time(stdPcm) >= fs::last_write_time(stdCc)) { return ""; } - // Copy std.cc → std.cppm in C++ rather than via cmd's `copy /Y` because - // `copy` always prints "1 file(s) copied." to stdout and RunCommand - // treats any output as an error. Held under the cache lock, so a plain - // filename is safe — no other builder writes this path concurrently. + // Copy std.cc → std.cppm in C++ rather than via cmd's `copy /Y`: keeping + // it in-process means one less shell round-trip and a std::error_code we + // can report directly. Held under the cache lock, so a plain filename is + // safe — no other builder writes this path concurrently. fs::path stdCppm = stdPcm.parent_path() / "std.cppm"; std::error_code ec; fs::copy_file(stdCc, stdCppm, fs::copy_options::overwrite_existing, ec); if (ec) { return std::format("copy {} -> {}: {}", stdCc.string(), stdCppm.string(), ec.message()); } - return RunCommand(std::format( + return RunBuildCommand(std::format( "clang++ --target={} -march={} -mtune={} " "--sysroot=\"{}\" -femulated-tls " "-O3 -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier " @@ -730,7 +736,7 @@ Configuration Crafter::LoadProject(const fs::path& projectFile, std::span buffer; - std::string result; - - std::string with = std::format("{} 2>&1", cmd); - // Open pipe to file - FILE* pipe = popen(with.c_str(), "r"); - if (!pipe) throw std::runtime_error("popen() failed!"); - - // Read till end of process: - while (fgets(buffer.data(), buffer.size(), pipe) != nullptr) { - result += buffer.data(); - } - - // Close pipe - pclose(pipe); - return result; -} - CommandResult Crafter::RunCommandChecked(std::string_view cmd) { std::array buffer; CommandResult result{}; @@ -874,7 +860,7 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) { if (copyEc) { return std::format("copy {} -> {}: {}", stdCc.string(), stdCppm.string(), copyEc.message()); } - return RunCommand(std::format("clang++ --target={} -march={} -mtune={} -femulated-tls -O3 -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile {} -o {}", config.target, config.march, config.mtune, stdCppm.string(), stdPcm.string())); + return RunBuildCommand(std::format("clang++ --target={} -march={} -mtune={} -femulated-tls -O3 -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile {} -o {}", config.target, config.march, config.mtune, stdCppm.string(), stdPcm.string())); } else { return ""; } @@ -922,7 +908,7 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) { archFlags += std::format(" -nostdinc++ -isystem {}/usr/include/c++/v1", config.sysroot); } if(!fs::exists(stdPcm) || fs::last_write_time(stdPcm) < fs::last_write_time(stdCppm)) { - return RunCommand(std::format("clang++ --target={} -std=c++26 -stdlib=libc++{}{} -O3 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile {} -o {}", config.target, sysrootFlag, archFlags, stdCppm, stdPcm.string())); + return RunBuildCommand(std::format("clang++ --target={} -std=c++26 -stdlib=libc++{}{} -O3 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile {} -o {}", config.target, sysrootFlag, archFlags, stdCppm, stdPcm.string())); } else { return ""; } @@ -1038,7 +1024,7 @@ Configuration Crafter::LoadProject(const fs::path& projectFile, std::span args); diff --git a/interfaces/Crafter.Build-Progress.cppm b/interfaces/Crafter.Build-Progress.cppm index fe563c6..1ed4bc7 100644 --- a/interfaces/Crafter.Build-Progress.cppm +++ b/interfaces/Crafter.Build-Progress.cppm @@ -31,6 +31,11 @@ export namespace Crafter::Progress { // Verbose-mode command echo. No-op outside Verbose. CRAFTER_API void EchoCommand(std::string_view command); + // Non-fatal output from a build step that succeeded — compiler and linker + // warnings. Erases the status line first so the text doesn't land on top of + // it, then writes to stderr. No-op on Quiet. + CRAFTER_API void Diagnostic(std::string_view text); + // Erase the in-place status line so subsequent stderr writes (errors, // banners) don't collide with it. No-op when not in TTY-redraw mode. CRAFTER_API void Clear();