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

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.
This commit is contained in:
catbot 2026-08-26 00:51:15 +00:00
commit 8e2d21a2bc
7 changed files with 77 additions and 64 deletions

View file

@ -788,7 +788,7 @@ BuildResult Crafter::Build(Configuration& config, std::unordered_map<fs::path, s
Progress::Task task(std::format("Compiling {}.c", cFile.filename().string()));
if (buildCancelled.load(std::memory_order_relaxed)) return;
std::string result = RunCommand(std::format("clang {0}.c --target={1}{2} -O3{3} -c{4}{5}{6} -MD -MF {7}_source.o.d -o {7}_source.o", cFile.string(), config.target, cArchFlags, ltoCompileFlags, includeFlags, defineFlags, userFlags, (buildDir / cFile.filename()).string()));
std::string result = RunBuildCommand(std::format("clang {0}.c --target={1}{2} -O3{3} -c{4}{5}{6} -MD -MF {7}_source.o.d -o {7}_source.o", cFile.string(), config.target, cArchFlags, ltoCompileFlags, includeFlags, defineFlags, userFlags, (buildDir / cFile.filename()).string()));
if (result.empty()) return;
bool expected = false;
@ -812,7 +812,7 @@ BuildResult Crafter::Build(Configuration& config, std::unordered_map<fs::path, s
Progress::Task task(std::format("Compiling {}.cu", cFile.filename().string()));
if (buildCancelled.load(std::memory_order_relaxed)) return;
std::string result = RunCommand(std::format("nvcc {}.cu -c -o {}_source.o -O3 -arch=sm_89", cFile.string(), (buildDir / cFile.filename()).string()));
std::string result = RunBuildCommand(std::format("nvcc {}.cu -c -o {}_source.o -O3 -arch=sm_89", cFile.string(), (buildDir / cFile.filename()).string()));
if (result.empty()) return;
bool expected = false;
@ -1165,9 +1165,9 @@ BuildResult Crafter::Build(Configuration& config, std::unordered_map<fs::path, s
}
}
if (config.target.starts_with("wasm32")) {
buildResult.result = RunCommand(std::format("{}{} -o {}.wasm -fuse-ld=lld{}", command, files, (outputDir/config.outputName).string(), linkExtras));
buildResult.result = RunBuildCommand(std::format("{}{} -o {}.wasm -fuse-ld=lld{}", command, files, (outputDir/config.outputName).string(), linkExtras));
} else {
buildResult.result = RunCommand(std::format("{}{} -o {} -fuse-ld=lld{}", command, files, (outputDir/config.outputName).string(), linkExtras));
buildResult.result = RunBuildCommand(std::format("{}{} -o {} -fuse-ld=lld{}", command, files, (outputDir/config.outputName).string(), linkExtras));
}
#endif
@ -1197,10 +1197,10 @@ BuildResult Crafter::Build(Configuration& config, std::unordered_map<fs::path, s
};
for (Configuration* dep : config.dependencies) copyDepDlls(dep);
buildResult.result = RunCommand(std::format("{}{} -o {} -fuse-ld=lld{}", command, files, (outputDir/config.outputName).string(), linkExtras));
buildResult.result = RunBuildCommand(std::format("{}{} -o {} -fuse-ld=lld{}", command, files, (outputDir/config.outputName).string(), linkExtras));
} else {
std::system(std::format("copy \"%LIBCXX_DIR%\\lib\\c++.dll\" \"{}\\c++.dll\"", outputDir.string()).c_str());
buildResult.result = RunCommand(std::format("{}{} -o {}.exe -fuse-ld=lld -L %LIBCXX_DIR%\\lib -lc++ -nostdinc++ -nostdlib++{}", command, files, (outputDir/config.outputName).string(), linkExtras));
buildResult.result = RunBuildCommand(std::format("{}{} -o {}.exe -fuse-ld=lld -L %LIBCXX_DIR%\\lib -lc++ -nostdinc++ -nostdlib++{}", command, files, (outputDir/config.outputName).string(), linkExtras));
}
#endif
} else if(config.type == ConfigurationType::LibraryStatic) {
@ -1208,11 +1208,11 @@ BuildResult Crafter::Build(Configuration& config, std::unordered_map<fs::path, s
// ThinLTO emits LLVM bitcode objects; plain `ar` writes an archive
// index that omits their symbols, so a consumer's lld link can't
// pull the needed members. llvm-ar writes a bitcode-aware index.
buildResult.result = RunCommand(std::format("{} rcs {}.a {}", useLto ? "llvm-ar" : "ar", (outputDir/fs::path(std::string("lib")+config.outputName)).string(), files));
buildResult.result = RunBuildCommand(std::format("{} rcs {}.a {}", useLto ? "llvm-ar" : "ar", (outputDir/fs::path(std::string("lib")+config.outputName)).string(), files));
#endif
#if defined(CRAFTER_BUILD_CONFIGURATION_TARGET_x86_64_pc_windows_msvc) || defined(CRAFTER_BUILD_CONFIGURATION_TARGET_x86_64_w64_mingw32)
buildResult.result = RunCommand(std::format("llvm-lib.exe {} /OUT:{}.lib", files, (outputDir/fs::path(config.outputName)).string()));
buildResult.result = RunBuildCommand(std::format("llvm-lib.exe {} /OUT:{}.lib", files, (outputDir/fs::path(config.outputName)).string()));
#endif
} else {
// LibraryDynamic. Output names follow each target's convention so
@ -1223,13 +1223,13 @@ BuildResult Crafter::Build(Configuration& config, std::unordered_map<fs::path, s
if (config.target == "x86_64-w64-mingw32") {
fs::path dll = outputDir / std::format("{}.dll", config.outputName);
fs::path implib = outputDir / std::format("lib{}.dll.a", config.outputName);
buildResult.result = RunCommand(std::format("{}{} -shared -o {} -Wl,--out-implib,{} -fuse-ld=lld{}", command, files, dll.string(), implib.string(), linkExtras));
buildResult.result = RunBuildCommand(std::format("{}{} -shared -o {} -Wl,--out-implib,{} -fuse-ld=lld{}", command, files, dll.string(), implib.string(), linkExtras));
} else if (config.target == "x86_64-pc-windows-msvc") {
fs::path dll = outputDir / std::format("{}.dll", config.outputName);
fs::path implib = outputDir / std::format("{}.lib", config.outputName);
buildResult.result = RunCommand(std::format("{}{} -shared -o {} -Wl,/IMPLIB:{} -fuse-ld=lld{}", command, files, dll.string(), implib.string(), linkExtras));
buildResult.result = RunBuildCommand(std::format("{}{} -shared -o {} -Wl,/IMPLIB:{} -fuse-ld=lld{}", command, files, dll.string(), implib.string(), linkExtras));
} else {
buildResult.result = RunCommand(std::format("{}{} -shared -o {}.so -Wl,-rpath,'$ORIGIN' -fuse-ld=lld{}", command, files, (outputDir/(std::string("lib")+config.outputName)).string(), linkExtras));
buildResult.result = RunBuildCommand(std::format("{}{} -shared -o {}.so -Wl,-rpath,'$ORIGIN' -fuse-ld=lld{}", command, files, (outputDir/(std::string("lib")+config.outputName)).string(), linkExtras));
}
}
}

View file

@ -52,7 +52,7 @@ namespace Crafter {
}
// -MD leaves <name>_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)) {

View file

@ -79,7 +79,7 @@ namespace Crafter {
// -MD records every header the preamble pulled in, next to the BMI as
// <name>.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
// <name>.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;

View file

@ -259,28 +259,34 @@ bool Crafter::MatchAny(std::span<const std::string> 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<char, 128> 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<std::int32_t>(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<char, 128> 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<const
cacheDir.string(),
absProject.string(), crafterBuildLib.string(), dllPath.string());
std::string result = RunCommand(compileCmd);
std::string result = RunBuildCommand(compileCmd);
if (!result.empty()) {
throw std::runtime_error(std::format("Failed to compile project {}: {}", absProject.string(), result));
}
@ -597,7 +603,7 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) {
if (fs::exists(stdPcm) && fs::last_write_time(stdPcm) >= 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<const
absProject.string(), dllPath.string(),
hostExe.parent_path().string());
std::string result = RunCommand(compileCmd);
std::string result = RunBuildCommand(compileCmd);
if (!result.empty()) {
throw std::runtime_error(std::format("Failed to compile project {}: {}", absProject.string(), result));
}
@ -761,26 +767,6 @@ Configuration Crafter::LoadProject(const fs::path& projectFile, std::span<const
#ifdef CRAFTER_BUILD_CONFIGURATION_TARGET_x86_64_pc_linux_gnu
std::string Crafter::RunCommand(const std::string_view cmd) {
Progress::EchoCommand(cmd);
std::array<char, 128> 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<char, 128> 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<const
cacheDir.string(),
absProject.string(), soPath.string());
std::string result = RunCommand(compileCmd);
std::string result = RunBuildCommand(compileCmd);
if (!result.empty()) {
throw std::runtime_error(std::format("Failed to compile project {}: {}", absProject.string(), result));
}

View file

@ -127,6 +127,18 @@ void EchoCommand(std::string_view command) {
}
}
void Diagnostic(std::string_view text) {
std::lock_guard lock(StateMutex);
if (ActiveVerbosity == Verbosity::Quiet) return;
ClearLineLocked();
// Written whole under StateMutex: parallel compiles hand us one warning
// block each, and interleaving them line-by-line would make every
// "note: ..." continuation follow the wrong diagnostic.
std::fwrite(text.data(), 1, text.size(), stderr);
if (!text.ends_with('\n')) std::fputc('\n', stderr);
std::fflush(stderr);
}
void Clear() {
std::lock_guard lock(StateMutex);
ClearLineLocked();

View file

@ -18,8 +18,18 @@ namespace Crafter {
};
std::string BuildStdPcm(const Configuration& config, fs::path stdPcm);
fs::path GetCacheDir();
std::string RunCommand(const std::string_view command);
CommandResult RunCommandChecked(std::string_view command);
// A compile / link / archive step. Success is the command's exit status,
// never whether it printed anything: a warning does not stop the compiler
// from writing a valid object, and warnings are only emitted when a
// translation unit is actually recompiled — so keying failure off the
// output made the same unchanged source pass or fail depending on whether
// its object happened to be up to date.
//
// Returns an empty string on success (any warnings go to the user via
// Progress::Diagnostic), otherwise the compiler's diagnostics. Callers
// therefore keep the `if (!result.empty())` shape they already had.
std::string RunBuildCommand(std::string_view command);
export CRAFTER_API CommandResult RunCommandWithTimeout(std::string_view command, std::chrono::seconds timeout);
std::string GetBaseCommand(const Configuration& config);
export CRAFTER_API Configuration LoadProject(const fs::path& projectFile, std::span<const std::string_view> args);

View file

@ -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();