diff --git a/README.md b/README.md index fd508f9..8a777ce 100644 --- a/README.md +++ b/README.md @@ -134,6 +134,8 @@ Everything that changes what gets built belongs in the variant id, since it name `crafter-build clean` removes the project's `bin/` and `build/` trees. It doesn't load `project.cpp`, so it still works when the project no longer compiles. +A build step fails when its command exits non-zero — never because it printed something. Warnings are printed to stderr and the build carries on; nothing here implies `-Werror`, so add it to `cfg.compileFlags` if you want warnings to be fatal, and they will then be fatal every time. Note that a warning only appears when the unit it belongs to is actually recompiled, so a build with nothing to do is silent. + ## Tests Tests live under `tests//`. The simplest case is a single C++ file: 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(); diff --git a/project.cpp b/project.cpp index f3e5af1..19e4f59 100644 --- a/project.cpp +++ b/project.cpp @@ -110,6 +110,7 @@ extern "C" Configuration CrafterBuildProject(std::span a cfg.AddTest("IncrementalInterfaceChange").Dependencies({ CrafterBuildLib.get() }); cfg.AddTest("TransitiveInterfaceChange").Dependencies({ CrafterBuildLib.get() }); cfg.AddTest("IncrementalHeaderChange").Dependencies({ CrafterBuildLib.get() }); + cfg.AddTest("CompilerWarning").Dependencies({ CrafterBuildLib.get() }); cfg.AddTest("CleanProject").Dependencies({ CrafterBuildLib.get() }); cfg.AddTest("ShaderCompile").Dependencies({ CrafterBuildLib.get() }); cfg.AddTest("StandardArgs").Dependencies({ CrafterBuildLib.get() }); diff --git a/tests/CompilerWarning/fixture/lib/Widget.cpp b/tests/CompilerWarning/fixture/lib/Widget.cpp new file mode 100644 index 0000000..692432b --- /dev/null +++ b/tests/CompilerWarning/fixture/lib/Widget.cpp @@ -0,0 +1,9 @@ +// SPDX-License-Identifier: LGPL-3.0-only +// SPDX-FileCopyrightText: Copyright (C) 2026 Catcrafts® + +module; +#warning "implementation unit is noisy" +module Widget; +import std; + +std::int32_t WidgetValue() { return 42; } diff --git a/tests/CompilerWarning/fixture/lib/Widget.cppm b/tests/CompilerWarning/fixture/lib/Widget.cppm new file mode 100644 index 0000000..4a1bcac --- /dev/null +++ b/tests/CompilerWarning/fixture/lib/Widget.cppm @@ -0,0 +1,13 @@ +// SPDX-License-Identifier: LGPL-3.0-only +// SPDX-FileCopyrightText: Copyright (C) 2026 Catcrafts® + +module; +// Every source in this fixture carries a #warning: -W#warnings is on by +// default, so each unit produces diagnostics on stderr and still exits 0. +// That is the whole point — the build has to read the exit status, not the +// chatter. +#warning "interface unit is noisy" +export module Widget; +import std; + +export std::int32_t WidgetValue(); diff --git a/tests/CompilerWarning/fixture/lib/counter.c b/tests/CompilerWarning/fixture/lib/counter.c new file mode 100644 index 0000000..2986d15 --- /dev/null +++ b/tests/CompilerWarning/fixture/lib/counter.c @@ -0,0 +1,8 @@ +/* SPDX-License-Identifier: LGPL-3.0-only + SPDX-FileCopyrightText: Copyright (C) 2026 Catcrafts® */ + +#warning "C source is noisy" + +long CounterValue(void) { + return 7; +} diff --git a/tests/CompilerWarning/fixture/main.cpp b/tests/CompilerWarning/fixture/main.cpp new file mode 100644 index 0000000..ef58cf6 --- /dev/null +++ b/tests/CompilerWarning/fixture/main.cpp @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: LGPL-3.0-only +// SPDX-FileCopyrightText: Copyright (C) 2026 Catcrafts® + +#warning "consumer is noisy" + +import std; +import Widget; + +extern "C" long CounterValue(); + +// Prints " ". The test asserts on it so that "the build +// succeeded" also means the objects behind those two numbers were really +// produced, not merely left over from an earlier pass. +int main() { + std::print("{} {}", WidgetValue(), CounterValue()); + return 0; +} diff --git a/tests/CompilerWarning/main.cpp b/tests/CompilerWarning/main.cpp new file mode 100644 index 0000000..91bd2aa --- /dev/null +++ b/tests/CompilerWarning/main.cpp @@ -0,0 +1,163 @@ +// SPDX-License-Identifier: LGPL-3.0-only +// SPDX-FileCopyrightText: Copyright (C) 2026 Catcrafts® + +import std; +import Crafter.Build; +namespace fs = std::filesystem; +using namespace Crafter; + +// A compiler warning must not fail the build. Success used to be inferred from +// whether the command printed anything, and a warning is printing something — +// so a translation unit that warns failed the build the moment it was compiled +// and passed on every run after that, because warnings are only emitted when +// something actually recompiles. Same source, opposite verdicts, decided by +// whether an object file happened to be up to date: it read as a flaky test, +// and a cold CI checkout turned every latent warning in a project into a wall +// of unrelated failures. +// +// The fixture puts a #warning in each kind of source the build knows how to +// compile — module interface, module implementation, C, consumer — and a +// linker warning on top, then asserts the build succeeds cold, succeeds when +// there is nothing to do, and succeeds again after each source is touched back +// into staleness. The last pass introduces a real error, because "never fails" +// would satisfy everything above just as well. + +namespace { + std::int32_t Failures = 0; + + void Check(bool cond, std::string_view msg) { + if (!cond) { + std::println(std::cerr, "FAIL: {}", msg); + ++Failures; + } + } + + // The fixture is mutated during the run, and every assertion here depends + // on units compiling for the first time, so work on a fresh copy outside + // the repo rather than against whatever an earlier run left behind. + fs::path StageFixture() { + fs::path source = fs::current_path() / "tests" / "CompilerWarning" / "fixture"; + fs::path staged = fs::temp_directory_path() / "crafter-build-compiler-warning"; + fs::remove_all(staged); + fs::copy(source, staged, fs::copy_options::recursive); + return staged; + } + + std::unique_ptr MakeLib(const fs::path& staged) { + auto lib = std::make_unique(); + lib->path = staged / "lib"; + lib->name = "noisy-widget"; + lib->outputName = "noisy-widget"; + lib->target = HostTarget(); + lib->type = ConfigurationType::LibraryStatic; + std::array ifaces = { "Widget" }; + std::array impls = { "Widget" }; + lib->GetInterfacesAndImplementations(ifaces, impls); + // cFiles are resolved against the cwd at build time, so spell it out. + lib->cFiles = { staged / "lib" / "counter" }; + return lib; + } + + Configuration MakeApp(const fs::path& staged, Configuration* lib) { + Configuration app; + app.path = staged; + app.name = "noisy-app"; + app.outputName = "noisy-app"; + app.target = HostTarget(); + app.type = ConfigurationType::Executable; + std::array ifaces = {}; + std::array impls = { "main" }; + app.GetInterfacesAndImplementations(ifaces, impls); + app.dependencies = { lib }; + // The link step reads a command's output the same way a compile does, + // so cover it too: an unknown -z value makes ld.lld warn and exit 0. + // GNU-style only — lld-link spells its options differently — so the + // link case rides along on hosts that use it and is skipped elsewhere. + if (!app.target.contains("windows")) { + app.linkFlags = { "-Wl,-z,crafter-build-nonexistent-z-value" }; + } + return app; + } + + BuildResult BuildOnce(Configuration& app) { + // A fresh depResults per pass: the map memoizes each Configuration's + // build for the duration of one pass, so reusing it would skip the + // library's rebuild entirely. + std::unordered_map> depResults; + std::mutex depMutex; + return Build(app, depResults, depMutex); + } + + bool BuildOk(Configuration& app, std::string_view label) { + BuildResult r = BuildOnce(app); + if (!r.result.empty()) { + std::println(std::cerr, "FAIL: {} build failed: {}", label, r.result); + ++Failures; + return false; + } + return true; + } + + void CheckRun(const fs::path& binary, std::string_view label) { + auto r = RunCommandWithTimeout(binary.string(), std::chrono::seconds(30)); + Check(r.exitCode == 0 && !r.crashed && !r.timedOut && r.output == "42 7", std::format("{}: expected '42 7', got '{}' (exit={})", label, r.output, r.exitCode)); + } + + void Touch(const fs::path& source) { + fs::last_write_time(source, fs::file_time_type::clock::now()); + } +} + +int main() { + fs::path staged = StageFixture(); + std::unique_ptr lib = MakeLib(staged); + Configuration app = MakeApp(staged, lib.get()); + fs::path binary = app.BinDir() / "noisy-app"; + + // Cold: every unit compiles, so every #warning in the fixture is emitted + // and the linker warns as well. This is the pass that used to fail. + if (!BuildOk(app, "cold")) { + std::println(std::cerr, "{} assertions failed", Failures); + return 1; + } + CheckRun(binary, "cold"); + + // Nothing to recompile, so nothing warns. Same sources as the pass above, + // and the verdict has to match it. + if (BuildOk(app, "idle")) { + CheckRun(binary, "idle"); + } + + // Back into staleness one source at a time: whichever compile path a + // warning comes out of, it stays non-fatal. + for (auto [source, label] : std::initializer_list>{ + { staged / "lib" / "Widget.cppm", "interface unit" }, + { staged / "lib" / "Widget.cpp", "implementation unit" }, + { staged / "lib" / "counter.c", "C source" }, + { staged / "main.cpp", "consumer" }, + }) { + Touch(source); + if (BuildOk(app, label)) { + CheckRun(binary, label); + } + } + + // The other direction, last because it leaves the fixture broken: a real + // error still has to fail, and still has to say what went wrong. + { + std::ofstream broken(staged / "main.cpp", std::ios::binary | std::ios::trunc); + broken << "int main() { return NotDeclaredAnywhere(); }\n"; + broken.close(); + Touch(staged / "main.cpp"); + + BuildResult r = BuildOnce(app); + Check(!r.result.empty(), "a source that fails to compile fails the build"); + Check(r.result.contains("error:"), std::format("the failure reports the compiler's diagnostic, got '{}'", r.result)); + } + + if (Failures > 0) { + std::println(std::cerr, "{} assertions failed", Failures); + return 1; + } + return 0; +}