From 13697cd0260773bba1024f531ba9e1ea8af37b6f Mon Sep 17 00:00:00 2001 From: catbot Date: Thu, 30 Jul 2026 17:12:24 +0000 Subject: [PATCH 1/3] fix: re-resolve module imports before checking staleness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding a data member to a class in a module interface did not rebuild every object compiled against the old layout. The build succeeded with no error or warning and the resulting binary mixed both layouts, surfacing later as a SIGSEGV in a destructor. GetInterfacesAndImplementations scans a TU's `import X;` statements when the source is declared. An import that matches neither a module in the Configuration nor one reachable through `dependencies` was dropped on the floor, leaving that TU with no staleness edge to the interface it consumes. `dependencies` is frequently assigned *after* the scan — AddTest does exactly that, resolving tests//main.cpp and only then returning a builder whose .Dependencies() supplies the library — so consumers of a dependency's modules routinely carried no edge at all. A layout change then rebuilt the library, relinked the consumer, and kept the consumer's object as it was. Unresolved names are now remembered on the partition/implementation as pendingImports, and Configuration::ResolvePendingImports retries them against the dependency DAG as it stands. Build() calls it immediately before comparing mtimes, which closes the window for every caller rather than only the ones that declare in the right order; TestBuilder::Dependencies also calls it so the Configuration is coherent for anyone inspecting it before the build. Resolves #27 --- implementations/Crafter.Build-Clang.cpp | 59 +++++- implementations/Crafter.Build-Test.cpp | 8 + interfaces/Crafter.Build-Clang.cppm | 9 + interfaces/Crafter.Build-Implementation.cppm | 3 + interfaces/Crafter.Build-Interface.cppm | 7 + project.cpp | 1 + .../fixture/lib/Widget-grown.cppm.in | 19 ++ .../fixture/lib/Widget.cpp | 7 + .../fixture/lib/Widget.cppm | 19 ++ .../fixture/main.cpp | 17 ++ tests/IncrementalInterfaceChange/main.cpp | 199 ++++++++++++++++++ 11 files changed, 341 insertions(+), 7 deletions(-) create mode 100644 tests/IncrementalInterfaceChange/fixture/lib/Widget-grown.cppm.in create mode 100644 tests/IncrementalInterfaceChange/fixture/lib/Widget.cpp create mode 100644 tests/IncrementalInterfaceChange/fixture/lib/Widget.cppm create mode 100644 tests/IncrementalInterfaceChange/fixture/main.cpp create mode 100644 tests/IncrementalInterfaceChange/main.cpp diff --git a/implementations/Crafter.Build-Clang.cpp b/implementations/Crafter.Build-Clang.cpp index 95a691d..4ae6660 100644 --- a/implementations/Crafter.Build-Clang.cpp +++ b/implementations/Crafter.Build-Clang.cpp @@ -25,9 +25,14 @@ namespace fs = std::filesystem; using namespace Crafter; -void Configuration::GetInterfacesAndImplementations(std::span interfaces, std::span implementations) { - auto resolveImport = [this](const std::string& importName, std::vector& localDeps, std::vector>& externalDeps) -> bool { - for(const std::unique_ptr& interface : this->interfaces) { +namespace { + // Map one `import X;` name onto either a module this Configuration owns or + // one exported by a Configuration reachable through its dependency DAG, + // appending the matching staleness edge. False means nothing in reach + // provides X — either it's supplied from outside the graph (`std`) or the + // dependency isn't wired up *yet*, which is why callers remember the name. + bool ResolveImportName(Configuration& cfg, const std::string& importName, std::vector& localDeps, std::vector>& externalDeps) { + for(const std::unique_ptr& interface : cfg.interfaces) { if(interface->name == importName) { localDeps.push_back(interface.get()); return true; @@ -50,10 +55,38 @@ void Configuration::GetInterfacesAndImplementations(std::span interfac return false; }; - for(Configuration* depCfg : this->dependencies) { + for(Configuration* depCfg : cfg.dependencies) { if (walk(depCfg)) return true; } return false; + } +} + +void Configuration::ResolvePendingImports() { + // Discard the local-module hits: a name recorded as pending already failed + // to match this Configuration's own interfaces, and nothing adds interfaces + // between the scan and here. Only the external edge can newly appear. + std::vector ignored; + auto sweep = [this, &ignored](std::vector& pending, std::vector>& externalDeps) { + std::erase_if(pending, [&](const std::string& name) { + return ResolveImportName(*this, name, ignored, externalDeps); + }); + }; + for(const std::unique_ptr& interface : interfaces) { + for(const std::unique_ptr& partition : interface->partitions) { + sweep(partition->pendingImports, partition->externalModuleDependencies); + } + } + for(Implementation& implementation : implementations) { + sweep(implementation.pendingImports, implementation.externalModuleDependencies); + } +} + +void Configuration::GetInterfacesAndImplementations(std::span interfaces, std::span implementations) { + auto resolveImport = [this](const std::string& importName, std::vector& localDeps, std::vector>& externalDeps, std::vector& pending) { + if (!ResolveImportName(*this, importName, localDeps, externalDeps)) { + pending.push_back(importName); + } }; std::vector> tempModulePaths = std::vector>(interfaces.size()); @@ -128,7 +161,7 @@ void Configuration::GetInterfacesAndImplementations(std::span interfac std::sregex_iterator modCurrent(fileContent.begin(), fileContent.end(), modulePattern); while (modCurrent != lastMatch) { std::smatch match = *modCurrent; - resolveImport(match[1].str(), partition->moduleDependencies, partition->externalModuleDependencies); + resolveImport(match[1].str(), partition->moduleDependencies, partition->externalModuleDependencies, partition->pendingImports); ++modCurrent; } } @@ -173,7 +206,7 @@ void Configuration::GetInterfacesAndImplementations(std::span interfac while (modCurrent != lastMatch) { std::smatch match2 = *modCurrent; if (match2[1] != match[1]) { - resolveImport(match2[1].str(), implementation.moduleDependencies, implementation.externalModuleDependencies); + resolveImport(match2[1].str(), implementation.moduleDependencies, implementation.externalModuleDependencies, implementation.pendingImports); } ++modCurrent; } @@ -188,7 +221,7 @@ void Configuration::GetInterfacesAndImplementations(std::span interfac std::sregex_iterator lastMatch; while (currentMatch != lastMatch) { std::smatch match2 = *currentMatch; - resolveImport(match2[1].str(), implementation.moduleDependencies, implementation.externalModuleDependencies); + resolveImport(match2[1].str(), implementation.moduleDependencies, implementation.externalModuleDependencies, implementation.pendingImports); ++currentMatch; } } @@ -224,6 +257,18 @@ BuildResult Crafter::Build(Configuration& config, std::unordered_map/main.cpp and + // only then hands back a builder whose .Dependencies() supplies the library. + // Any `import ;` in such a TU resolved to nothing and so carried + // no staleness edge, which meant a member added to a dependency's interface + // rebuilt the library, relinked the consumer, and silently kept the + // consumer's object compiled against the *old* class layout (issue #27). + // Re-resolving here — the last point before mtimes are compared, with the + // DAG fully wired — closes that window for every caller rather than for the + // ones that remember to declare in the right order. + config.ResolvePendingImports(); + // Auto-detect the WASI sysroot before any compile step runs so BuildStdPcm // and the main compile command see the same value. Linux-only — Windows // users supply cfg.sysroot pointing at their wasi-sdk install. Covers all diff --git a/implementations/Crafter.Build-Test.cpp b/implementations/Crafter.Build-Test.cpp index f686d26..3bac4ca 100644 --- a/implementations/Crafter.Build-Test.cpp +++ b/implementations/Crafter.Build-Test.cpp @@ -483,6 +483,14 @@ TestBuilder& TestBuilder::Args(std::vector a) { Ref().args = std:: TestBuilder& TestBuilder::Requires(std::string r) { Ref().requires_.push_back(std::move(r)); return *this; } TestBuilder& TestBuilder::Dependencies(std::vector d) { Ref().config.dependencies = std::move(d); + // AddTest already scanned tests//main.cpp, at which point this test + // had no dependencies, so every `import ;` in it came back + // unresolved. Place them now that the libraries are known — without this + // the test's object carries no staleness edge to the interfaces it consumes + // and survives a layout change to them (issue #27). Build() re-runs the + // same sweep as a backstop; doing it here keeps the Configuration coherent + // for anyone inspecting it before the build. + Ref().config.ResolvePendingImports(); return *this; } TestBuilder& TestBuilder::LinkFlag(std::string f) { Ref().config.linkFlags.push_back(std::move(f)); return *this; } diff --git a/interfaces/Crafter.Build-Clang.cppm b/interfaces/Crafter.Build-Clang.cppm index 64c22f7..03b870a 100644 --- a/interfaces/Crafter.Build-Clang.cppm +++ b/interfaces/Crafter.Build-Clang.cppm @@ -254,6 +254,15 @@ export namespace Crafter { // Lint rules for `crafter-build lint`. Populate via AddLintRule. std::vector lintRules; CRAFTER_API void GetInterfacesAndImplementations(std::span interfaces, std::span implementations); + // Retry the `import X;` names GetInterfacesAndImplementations could not + // place, against the dependency DAG as it stands now. Sources are + // scanned when they're declared, but `dependencies` is often assigned + // afterwards (AddTest does exactly this), and an import that resolved to + // nothing leaves no staleness edge — so a dependency's interface could + // change and this Configuration's objects would be silently reused + // against the new layout. Build() calls this before checking mtimes; + // calling it again is harmless. + CRAFTER_API void ResolvePendingImports(); // Declare a test. Sources default to `tests//main.cpp` resolved // against this Configuration's path; target/march/mtune/sysroot/debug // are inherited from this Configuration so cross-arch projects don't diff --git a/interfaces/Crafter.Build-Implementation.cppm b/interfaces/Crafter.Build-Implementation.cppm index fb9842a..95f816b 100644 --- a/interfaces/Crafter.Build-Implementation.cppm +++ b/interfaces/Crafter.Build-Implementation.cppm @@ -15,6 +15,9 @@ namespace Crafter { std::vector moduleDependencies; std::vector partitionDependencies; std::vector> externalModuleDependencies; + // See ModulePartition::pendingImports — imports this TU declared that + // no reachable Configuration provided when the source was scanned. + std::vector pendingImports; fs::path path; CRAFTER_API Implementation(fs::path&& path); CRAFTER_API bool Check(const fs::path& buildDir, const fs::path& pcmDir, fs::file_time_type sourceFloor = fs::file_time_type::min()) const; diff --git a/interfaces/Crafter.Build-Interface.cppm b/interfaces/Crafter.Build-Interface.cppm index e7274cb..b1c2d0c 100644 --- a/interfaces/Crafter.Build-Interface.cppm +++ b/interfaces/Crafter.Build-Interface.cppm @@ -14,6 +14,13 @@ namespace Crafter { std::vector moduleDependencies; std::vector partitionDependencies; std::vector> externalModuleDependencies; + // Names from `import X;` that matched neither a module in this + // Configuration nor one reachable through its dependencies at the time + // the source was scanned. Retried by ResolvePendingImports (see + // Crafter.Build:Clang) so a dependency wired up after + // GetInterfacesAndImplementations still produces a staleness edge. + // Names that never resolve (`std`, module-mapped externals) stay here. + std::vector pendingImports; std::atomic compiled; bool needsRecompiling; bool checked = false; diff --git a/project.cpp b/project.cpp index 251a8af..ed9486d 100644 --- a/project.cpp +++ b/project.cpp @@ -107,6 +107,7 @@ extern "C" Configuration CrafterBuildProject(std::span a cfg.AddTest("StaticLib").Dependencies({ CrafterBuildLib.get() }); cfg.AddTest("ModuleInterface").Dependencies({ CrafterBuildLib.get() }); cfg.AddTest("DependencyLink").Dependencies({ CrafterBuildLib.get() }); + cfg.AddTest("IncrementalInterfaceChange").Dependencies({ CrafterBuildLib.get() }); cfg.AddTest("ShaderCompile").Dependencies({ CrafterBuildLib.get() }); cfg.AddTest("StandardArgs").Dependencies({ CrafterBuildLib.get() }); cfg.AddTest("TestRunnerSpec").Dependencies({ CrafterBuildLib.get() }); diff --git a/tests/IncrementalInterfaceChange/fixture/lib/Widget-grown.cppm.in b/tests/IncrementalInterfaceChange/fixture/lib/Widget-grown.cppm.in new file mode 100644 index 0000000..e1616a2 --- /dev/null +++ b/tests/IncrementalInterfaceChange/fixture/lib/Widget-grown.cppm.in @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: LGPL-3.0-only +// SPDX-FileCopyrightText: Copyright (C) 2026 Catcrafts® + +// Copied over lib/Widget.cppm mid-test to stand in for the interface edit from +// issue #27: one extra data member, every signature and mangled name unchanged. +// Kept as a .cppm.in so the module scanner never treats it as a source of its +// own — and so the text does not have to live in a string literal inside the +// test, where `export module Widget;` would make the test itself look like an +// implementation unit of Widget. + +export module Widget; +import std; + +export struct Widget { + std::string a; + std::string b; +}; + +export std::size_t WidgetSizeInLibrary(); diff --git a/tests/IncrementalInterfaceChange/fixture/lib/Widget.cpp b/tests/IncrementalInterfaceChange/fixture/lib/Widget.cpp new file mode 100644 index 0000000..7228586 --- /dev/null +++ b/tests/IncrementalInterfaceChange/fixture/lib/Widget.cpp @@ -0,0 +1,7 @@ +// SPDX-License-Identifier: LGPL-3.0-only +// SPDX-FileCopyrightText: Copyright (C) 2026 Catcrafts® + +module Widget; +import std; + +std::size_t WidgetSizeInLibrary() { return sizeof(Widget); } diff --git a/tests/IncrementalInterfaceChange/fixture/lib/Widget.cppm b/tests/IncrementalInterfaceChange/fixture/lib/Widget.cppm new file mode 100644 index 0000000..aefc61e --- /dev/null +++ b/tests/IncrementalInterfaceChange/fixture/lib/Widget.cppm @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: LGPL-3.0-only +// SPDX-FileCopyrightText: Copyright (C) 2026 Catcrafts® + +export module Widget; +import std; + +// The test rewrites this struct to add a member. Adding one changes the class +// layout while leaving every signature and mangled name untouched, so nothing +// downstream fails to link — a consumer object left over from before the change +// keeps the old sizeof and quietly disagrees with the library. +export struct Widget { + std::string a; +}; + +// Deliberately out-of-line, in the library's own implementation unit, so the +// value reflects the layout the *library* was compiled against rather than the +// caller's. An inline body would be instantiated from the (rebuilt) BMI in the +// consumer and would agree with it by construction. +export std::size_t WidgetSizeInLibrary(); diff --git a/tests/IncrementalInterfaceChange/fixture/main.cpp b/tests/IncrementalInterfaceChange/fixture/main.cpp new file mode 100644 index 0000000..667809b --- /dev/null +++ b/tests/IncrementalInterfaceChange/fixture/main.cpp @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: LGPL-3.0-only +// SPDX-FileCopyrightText: Copyright (C) 2026 Catcrafts® + +import std; +import Widget; + +// Prints " " and exits 1 when they disagree. A mismatch is the +// observable form of the mixed-layout binary a stale consumer object produces — +// this reports it instead of waiting for the SIGSEGV that the real-world case +// (issue #27) produced in a destructor. +int main() { + std::size_t here = sizeof(Widget); + std::size_t inLibrary = WidgetSizeInLibrary(); + std::print("{} {}", here, inLibrary); + return here == inLibrary ? 0 : 1; +} diff --git a/tests/IncrementalInterfaceChange/main.cpp b/tests/IncrementalInterfaceChange/main.cpp new file mode 100644 index 0000000..a570060 --- /dev/null +++ b/tests/IncrementalInterfaceChange/main.cpp @@ -0,0 +1,199 @@ +// 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; + +// Adding a data member to a class in a module interface must rebuild every +// object compiled against the old layout. The dangerous shape (issue #27) is a +// consumer whose sources were scanned *before* its `dependencies` were assigned: +// its `import ;` matched nothing, so its object carried no staleness +// edge to the interface, and a layout change rebuilt the library, relinked the +// consumer, and produced a binary mixing both layouts with no error or warning. +// +// `AddTest` is exactly that shape — it scans tests//main.cpp and only then +// returns a builder whose .Dependencies() supplies the library — which is why +// the original report saw the corruption in test executables specifically. + +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, so work on a copy outside the repo. + fs::path StageFixture() { + fs::path source = fs::current_path() / "tests" / "IncrementalInterfaceChange" / "fixture"; + fs::path staged = fs::temp_directory_path() / "crafter-build-incremental-interface-change"; + fs::remove_all(staged); + fs::copy(source, staged, fs::copy_options::recursive); + return staged; + } + + // Swap in the variant carrying the extra member. Comes from a file rather + // than a string literal here: the module scanner reads raw source, so an + // `export module Widget;` spelled inside this test would make the test look + // like an implementation unit of Widget. + void GrowWidget(const fs::path& staged) { + fs::copy_file(staged / "lib" / "Widget-grown.cppm.in", staged / "lib" / "Widget.cppm", fs::copy_options::overwrite_existing); + // copy_file carries the source's mtime across, which would leave the + // rewritten interface looking older than the BMI built from it. + fs::last_write_time(staged / "lib" / "Widget.cppm", fs::file_time_type::clock::now()); + } + + std::unique_ptr MakeLib(const fs::path& staged) { + auto lib = std::make_unique(); + lib->path = staged / "lib"; + lib->name = "widget"; + lib->outputName = "widget"; + lib->target = HostTarget(); + lib->type = ConfigurationType::LibraryStatic; + std::array ifaces = { "Widget" }; + std::array impls = { "Widget" }; + lib->GetInterfacesAndImplementations(ifaces, impls); + return lib; + } + + // Scan first, wire the dependency up afterwards — the ordering that used to + // silently drop the staleness edge. + Configuration MakeConsumerScannedBeforeDependencies(const fs::path& staged, Configuration* lib) { + Configuration app; + app.path = staged; + app.name = "widget-app"; + app.outputName = "widget-app"; + app.target = HostTarget(); + app.type = ConfigurationType::Executable; + std::array ifaces = {}; + std::array impls = { "main" }; + app.GetInterfacesAndImplementations(ifaces, impls); + app.dependencies = { lib }; + return app; + } + + bool BuildOk(Configuration& app, std::string_view label) { + // 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 second build entirely. + std::unordered_map> depResults; + std::mutex depMutex; + BuildResult r = Build(app, depResults, depMutex); + if (!r.result.empty()) { + std::println(std::cerr, "FAIL: {} build failed: {}", label, r.result); + ++Failures; + return false; + } + return true; + } +} + +int main() { + { + // The scan leaves the unmatched import recorded rather than forgotten, + // and ResolvePendingImports places it once the library is reachable. + fs::path staged = StageFixture(); + std::unique_ptr lib = MakeLib(staged); + Configuration app; + app.path = staged; + app.name = "widget-app"; + app.outputName = "widget-app"; + app.target = HostTarget(); + app.type = ConfigurationType::Executable; + std::array ifaces = {}; + std::array impls = { "main" }; + app.GetInterfacesAndImplementations(ifaces, impls); + + Check(app.implementations.size() == 1, "consumer has one implementation"); + if (app.implementations.size() == 1) { + const Implementation& impl = app.implementations[0]; + Check(impl.externalModuleDependencies.empty(), "no external dep resolvable before dependencies are assigned"); + Check(std::ranges::find(impl.pendingImports, "Widget") != impl.pendingImports.end(), "unresolved 'import Widget;' is recorded as pending"); + + app.dependencies = { lib.get() }; + app.ResolvePendingImports(); + + Check(impl.externalModuleDependencies.size() == 1, "ResolvePendingImports adds the external module dep"); + if (impl.externalModuleDependencies.size() == 1) { + Check(impl.externalModuleDependencies[0].first->name == "Widget", "external dep is the Widget module"); + Check(impl.externalModuleDependencies[0].second == lib->PcmDir() / "Widget.pcm", "external dep points at the library's BMI"); + } + Check(std::ranges::find(impl.pendingImports, "Widget") == impl.pendingImports.end(), "resolved import is no longer pending"); + + // Idempotent: a second sweep must not duplicate the edge (Build + // runs one unconditionally, on top of whatever callers already did). + app.ResolvePendingImports(); + Check(impl.externalModuleDependencies.size() == 1, "ResolvePendingImports is idempotent"); + } + } + + { + // AddTest is the reported path: it scans the test source, then hands + // back a builder whose .Dependencies() names the library. + fs::path staged = StageFixture(); + fs::create_directories(staged / "tests" / "Consumer"); + fs::copy_file(staged / "main.cpp", staged / "tests" / "Consumer" / "main.cpp"); + std::unique_ptr lib = MakeLib(staged); + + Configuration app; + app.path = staged; + app.name = "host"; + app.outputName = "host"; + app.target = HostTarget(); + app.type = ConfigurationType::Executable; + // AddTest resolves tests//main against the cwd, which for a real + // run is the directory holding project.cpp. Stand in the staged project + // for the declaration so the fixture's test source is the one scanned. + fs::path restore = fs::current_path(); + fs::current_path(staged); + app.AddTest("Consumer").Dependencies({ lib.get() }); + fs::current_path(restore); + + Check(app.tests.size() == 1, "one test declared"); + if (app.tests.size() == 1 && app.tests[0].config.implementations.size() == 1) { + const Implementation& impl = app.tests[0].config.implementations[0]; + Check(impl.externalModuleDependencies.size() == 1, "AddTest(...).Dependencies() resolves the test's import of the library module"); + Check(impl.pendingImports.empty() || std::ranges::find(impl.pendingImports, "Widget") == impl.pendingImports.end(), "test's 'import Widget;' is no longer pending"); + } + } + + { + // End to end, and deliberately without calling ResolvePendingImports: + // the guarantee under test is that Build() closes the window on its own, + // for consumers that never knew they had to ask. + fs::path staged = StageFixture(); + std::unique_ptr lib = MakeLib(staged); + Configuration app = MakeConsumerScannedBeforeDependencies(staged, lib.get()); + fs::path binary = app.BinDir() / "widget-app"; + fs::path consumerObject = app.BuildDir() / "main_impl.o"; + + if (BuildOk(app, "first pass")) { + auto first = RunCommandWithTimeout(binary.string(), std::chrono::seconds(30)); + Check(first.exitCode == 0 && !first.crashed && !first.timedOut, std::format("first pass agrees on the layout (exit={} output='{}')", first.exitCode, first.output)); + + fs::file_time_type objectBefore = fs::last_write_time(consumerObject); + + // Same edit as the original report: one more member on a class in a + // module interface. Every signature and mangled name is unchanged, + // so a missed rebuild produces no diagnostic of any kind. + GrowWidget(staged); + + if (BuildOk(app, "second pass")) { + Check(fs::last_write_time(consumerObject) > objectBefore, "consumer object is recompiled after the interface gains a member"); + + auto second = RunCommandWithTimeout(binary.string(), std::chrono::seconds(30)); + Check(second.exitCode == 0 && !second.crashed && !second.timedOut, std::format("second pass agrees on the layout (exit={} output='{}')", second.exitCode, second.output)); + } + } + } + + if (Failures > 0) { + std::println(std::cerr, "{} assertions failed", Failures); + return 1; + } + return 0; +} -- 2.47.3 From e8fde57582de6626c26ca268cd10b1fa1340d295 Mon Sep 17 00:00:00 2001 From: catbot Date: Thu, 30 Jul 2026 17:43:09 +0000 Subject: [PATCH 2/3] fix: key the host PCM cache on source content, add clean, hash project args MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-ons to the stale-build report, all cases of an identity not capturing something that changes the output. The host PCM cache under /crafter.build/-/ is shared by every crafter-build on the machine, and freshness was a per-file mtime comparison. That cannot tell "this PCM is newer than my source" from "this PCM was built from different sources that happen to be newer", so a package install and a working checkout — or two checkouts of different versions — silently compiled their project.cpp against each other's declarations. Invalidation now keys on a stamp over the bytes of every module source, which also covers the case one file's mtime never could: the cached PCMs import each other, so a change to :Interface invalidates :Clang's PCM with Crafter.Build-Clang.cppm untouched. Project args ApplyStandardArgs does not itself interpret are now folded into VariantId. Such a flag typically decides what gets compiled or bundled — the report's example is --no-webgpu dropping entries from cfg.files — and without it both settings shared one bin dir and interleaved their outputs there, leaving a bundle matching neither. Sorted and deduplicated so flag order doesn't split the cache, and inherited by test Configurations. `crafter-build clean` removes the project's bin/ and build/ trees. It deliberately does not load project.cpp: cleaning is most often reached when something is already wrong, and a clean that first needs the project to compile is useless exactly then. --- implementations/Crafter.Build-Clang.cpp | 69 ++++++++-- implementations/Crafter.Build-Platform.cpp | 78 ++++++++++- implementations/Crafter.Build-Test.cpp | 5 + interfaces/Crafter.Build-Clang.cppm | 25 ++++ project.cpp | 4 + tests/CleanProject/main.cpp | 79 +++++++++++ tests/HostCacheSourceStamp/main.cpp | 150 +++++++++++++++++++++ tests/StandardArgs/main.cpp | 25 ++++ tests/VariantId/main.cpp | 19 +++ 9 files changed, 439 insertions(+), 15 deletions(-) create mode 100644 tests/CleanProject/main.cpp create mode 100644 tests/HostCacheSourceStamp/main.cpp diff --git a/implementations/Crafter.Build-Clang.cpp b/implementations/Crafter.Build-Clang.cpp index 4ae6660..6dc681c 100644 --- a/implementations/Crafter.Build-Clang.cpp +++ b/implementations/Crafter.Build-Clang.cpp @@ -63,22 +63,22 @@ namespace { } void Configuration::ResolvePendingImports() { - // Discard the local-module hits: a name recorded as pending already failed - // to match this Configuration's own interfaces, and nothing adds interfaces - // between the scan and here. Only the external edge can newly appear. - std::vector ignored; - auto sweep = [this, &ignored](std::vector& pending, std::vector>& externalDeps) { + // Same resolution the scan used, and against both dependency kinds — a + // second GetInterfacesAndImplementations call can add interfaces that an + // earlier batch's import was looking for, so a pending name may land on a + // local module and not just an external one. + auto sweep = [this](std::vector& pending, std::vector& localDeps, std::vector>& externalDeps) { std::erase_if(pending, [&](const std::string& name) { - return ResolveImportName(*this, name, ignored, externalDeps); + return ResolveImportName(*this, name, localDeps, externalDeps); }); }; for(const std::unique_ptr& interface : interfaces) { for(const std::unique_ptr& partition : interface->partitions) { - sweep(partition->pendingImports, partition->externalModuleDependencies); + sweep(partition->pendingImports, partition->moduleDependencies, partition->externalModuleDependencies); } } for(Implementation& implementation : implementations) { - sweep(implementation.pendingImports, implementation.externalModuleDependencies); + sweep(implementation.pendingImports, implementation.moduleDependencies, implementation.externalModuleDependencies); } } @@ -1446,7 +1446,15 @@ ArgQuery Crafter::ApplyStandardArgs(Configuration& cfg, std::span(-1) || ec) { + throw std::runtime_error(std::format("could not remove {}: {}", dir.string(), ec.message())); + } + removed.push_back(std::move(dir)); + } + return removed; +} + static void PrintHelp(std::string_view argv0) { std::println( R"(Usage: @@ -1468,6 +1491,7 @@ R"(Usage: {0} test [test-options] [globs...] Build and run the project's tests {0} lint [lint-options] [globs...] Run the project's lint rules over its sources {0} format [format-options] [globs...] Apply the project's transform rules (rewrites files) + {0} clean Delete the project's bin/ and build/ trees {0} help | -h | --help Show this help Loads ./project.cpp (override with --project=), compiles it to a shared @@ -1510,10 +1534,17 @@ Format options (after the `format` subcommand): transform. `format` rewrites changed files in place; `lint` reports the same transforms as would-reformat findings without writing. +Clean (after the `clean` subcommand): + Removes bin/ and build/ next to the project file. Does not load project.cpp, + so it works when the project itself no longer compiles. Every target, variant + and dependency artifact under those trees goes with it. + Project args: Any flag not consumed above is forwarded verbatim to CrafterBuildProject as part of its `args` span. Project-specific flags (e.g. --target=, custom - feature toggles) live there. + feature toggles) live there. Flags ApplyStandardArgs does not itself + interpret are folded into the variant hash, so switching one lands in its own + bin/ and build/ directory instead of overwriting the other setting's. Environment: CRAFTER_BUILD_MARCH Override -march (default: native). @@ -1541,6 +1572,7 @@ int Crafter::Run(int argc, char** argv) { bool runTests = false; bool runLint = false; bool runFormat = false; + bool runClean = false; bool runAfterBuild = false; RunTestsOptions testOpts; RunLintOptions lintOpts; @@ -1551,11 +1583,13 @@ int Crafter::Run(int argc, char** argv) { if (arg == "-h" || arg == "--help" || (!runTests && !runLint && !runFormat && arg == "help")) { PrintHelp(argv0); return 0; - } else if (!runLint && !runFormat && arg == "test") { + } else if (!runLint && !runFormat && !runClean && arg == "test") { runTests = true; - } else if (!runTests && !runFormat && arg == "lint") { + } else if (!runTests && !runFormat && !runClean && arg == "lint") { runLint = true; - } else if (!runTests && !runLint && arg == "format") { + } else if (!runTests && !runLint && !runClean && arg == "clean") { + runClean = true; + } else if (!runTests && !runLint && !runClean && arg == "format") { runFormat = true; lintOpts.mode = LintMode::Apply; } else if (runFormat && arg == "--check") { @@ -1603,6 +1637,17 @@ int Crafter::Run(int argc, char** argv) { return 1; } + // Ahead of LoadProject on purpose — see CleanProject. + if (runClean) { + std::vector removed = CleanProject(projectFile); + if (removed.empty()) { + std::println("Nothing to clean"); + } else { + for (const fs::path& dir : removed) std::println("Removed {}", dir.string()); + } + return 0; + } + Configuration config = LoadProject(projectFile, projectArgs); SetParentProject(&config); diff --git a/implementations/Crafter.Build-Platform.cpp b/implementations/Crafter.Build-Platform.cpp index c0b35a8..55e035b 100644 --- a/implementations/Crafter.Build-Platform.cpp +++ b/implementations/Crafter.Build-Platform.cpp @@ -74,6 +74,54 @@ namespace { CacheLock(const CacheLock&) = delete; CacheLock& operator=(const CacheLock&) = delete; }; + + // The cached Crafter.Build PCMs are keyed by `-` alone, so + // every crafter-build on the machine writes to the same files regardless of + // which share/crafter-build its module sources came from. Freshness used to + // be a per-file mtime comparison, which cannot distinguish "this PCM is + // newer than my source" from "this PCM was built from *different* sources + // that happen to be newer" — so a second install or checkout silently + // compiled its project against the other's declarations. Same failure shape + // as issue #27: no error, a binary built against a layout nobody linked. + // + // A stamp over the bytes of every module source answers what the mtime + // can't. It covers the whole set rather than one file at a time because the + // PCMs import each other: a change to :Interface invalidates :Clang's PCM + // even though Crafter.Build-Clang.cppm is untouched. + std::string CrafterBuildSourceStamp(const fs::path& sourceDir, std::span moduleNames) { + std::string all; + auto append = [&all, &sourceDir](const fs::path& relative) { + std::ifstream in(sourceDir / relative, std::ios::binary); + std::ostringstream buffer; + buffer << in.rdbuf(); + all += relative.string(); + all += '\0'; + all += buffer.str(); + all += '\0'; + }; + for (std::string_view name : moduleNames) { + append(fs::path(std::format("{}.cppm", name))); + } + // Exported through `module;` preambles, so its contents land in the PCMs + // too. + append("Crafter.Build-Api.h"); + return std::format("{:016x}", std::hash{}(all)); + } + + fs::path CacheStampPath(const fs::path& cacheDir) { + return cacheDir / "crafter-build-sources.stamp"; + } + + std::string ReadCacheStamp(const fs::path& cacheDir) { + std::ifstream in(CacheStampPath(cacheDir), std::ios::binary); + std::string stamp; + std::getline(in, stamp); + return stamp; + } + + void WriteCacheStamp(const fs::path& cacheDir, const std::string& stamp) { + std::ofstream(CacheStampPath(cacheDir), std::ios::binary | std::ios::trunc) << stamp << '\n'; + } } fs::path Crafter::GetCrafterBuildHome() { @@ -328,13 +376,17 @@ std::string Crafter::GetBaseCommand(const Configuration& config) { namespace { void EnsureCrafterBuildPcms(const fs::path& sourceDir, const fs::path& cacheDir) { CacheLock lock(cacheDir); + // Re-derived under the lock: another builder may have refreshed the + // cache from its own sources while we waited. + std::string stamp = CrafterBuildSourceStamp(sourceDir, CrafterBuildModules); + bool upToDate = ReadCacheStamp(cacheDir) == stamp; for (std::string_view name : CrafterBuildModules) { fs::path cppmPath = sourceDir / std::format("{}.cppm", name); fs::path pcmPath = cacheDir / std::format("{}.pcm", name); if (!fs::exists(cppmPath)) { throw std::runtime_error(std::format("module source {} not found in {} (set CRAFTER_BUILD_HOME)", name, sourceDir.string())); } - if (fs::exists(pcmPath) && fs::last_write_time(cppmPath) < fs::last_write_time(pcmPath)) { + if (upToDate && fs::exists(pcmPath)) { continue; } std::string cmd = std::format( @@ -350,6 +402,10 @@ namespace { throw std::runtime_error(std::format("Failed to precompile {} (exit {}): {}", name, r.exitCode, r.output)); } } + // Last, so an interrupted rebuild leaves the stamp disagreeing with the + // sources and the next run starts over rather than trusting a half-built + // set of PCMs. + if (!upToDate) WriteCacheStamp(cacheDir, stamp); } } @@ -523,6 +579,10 @@ std::string Crafter::GetBaseCommand(const Configuration& config) { namespace { void EnsureCrafterBuildPcms(const fs::path& sourceDir, const fs::path& cacheDir) { CacheLock lock(cacheDir); + // Re-derived under the lock: another builder may have refreshed the + // cache from its own sources while we waited. + std::string stamp = CrafterBuildSourceStamp(sourceDir, CrafterBuildModules); + bool upToDate = ReadCacheStamp(cacheDir) == stamp; fs::path prefix = MingwPrefix(); for (std::string_view name : CrafterBuildModules) { fs::path cppmPath = sourceDir / std::format("{}.cppm", name); @@ -530,7 +590,7 @@ namespace { if (!fs::exists(cppmPath)) { throw std::runtime_error(std::format("module source {} not found in {} (set CRAFTER_BUILD_HOME)", name, sourceDir.string())); } - if (fs::exists(pcmPath) && fs::last_write_time(cppmPath) < fs::last_write_time(pcmPath)) { + if (upToDate && fs::exists(pcmPath)) { continue; } std::string cmd = std::format( @@ -546,6 +606,10 @@ namespace { throw std::runtime_error(std::format("Failed to precompile {} (exit {}): {}", name, r.exitCode, r.output)); } } + // Last, so an interrupted rebuild leaves the stamp disagreeing with the + // sources and the next run starts over rather than trusting a half-built + // set of PCMs. + if (!upToDate) WriteCacheStamp(cacheDir, stamp); } } @@ -839,13 +903,17 @@ namespace { void EnsureCrafterBuildPcms(const fs::path& sourceDir, const fs::path& cacheDir) { CacheLock lock(cacheDir); + // Re-derived under the lock: another builder may have refreshed the + // cache from its own sources while we waited. + std::string stamp = CrafterBuildSourceStamp(sourceDir, CrafterBuildModules); + bool upToDate = ReadCacheStamp(cacheDir) == stamp; for (std::string_view name : CrafterBuildModules) { fs::path cppmPath = sourceDir / std::format("{}.cppm", name); fs::path pcmPath = cacheDir / std::format("{}.pcm", name); if (!fs::exists(cppmPath)) { throw std::runtime_error(std::format("module source {} not found in {} (set CRAFTER_BUILD_HOME)", name, sourceDir.string())); } - if (fs::exists(pcmPath) && fs::last_write_time(cppmPath) < fs::last_write_time(pcmPath)) { + if (upToDate && fs::exists(pcmPath)) { continue; } std::string cmd = std::format( @@ -860,6 +928,10 @@ namespace { throw std::runtime_error(std::format("Failed to precompile {} (exit {}): {}", name, r.exitCode, r.output)); } } + // Last, so an interrupted rebuild leaves the stamp disagreeing with the + // sources and the next run starts over rather than trusting a half-built + // set of PCMs. + if (!upToDate) WriteCacheStamp(cacheDir, stamp); } } diff --git a/implementations/Crafter.Build-Test.cpp b/implementations/Crafter.Build-Test.cpp index 3bac4ca..a6e3ec8 100644 --- a/implementations/Crafter.Build-Test.cpp +++ b/implementations/Crafter.Build-Test.cpp @@ -429,6 +429,10 @@ TestBuilder Configuration::AddTest(std::string_view name, std::span in t.config.mtune = this->mtune; t.config.sysroot = this->sysroot; t.config.debug = this->debug; + // Inherited so a project flag that changes what the library contains also + // moves the test's own outputs, rather than letting two flag settings share + // one test bin dir. + t.config.projectArgs = this->projectArgs; t.config.type = ConfigurationType::Executable; // Default source layout: tests//main.cpp resolved against the @@ -453,6 +457,7 @@ void Configuration::AddMarchVariants(std::string_view name, std::span t.config.mtune = tier.mtune; t.config.sysroot = this->sysroot; t.config.debug = this->debug; + t.config.projectArgs = this->projectArgs; t.config.type = ConfigurationType::Executable; fs::path mainSource = fs::path("tests") / std::string(name) / "main"; diff --git a/interfaces/Crafter.Build-Clang.cppm b/interfaces/Crafter.Build-Clang.cppm index 03b870a..637e174 100644 --- a/interfaces/Crafter.Build-Clang.cppm +++ b/interfaces/Crafter.Build-Clang.cppm @@ -250,6 +250,15 @@ export namespace Crafter { // clobber the baseline's. Not for direct project use — declare // wasmVariants instead. std::vector wasmVariantFlags; + // The project args ApplyStandardArgs did not interpret — i.e. the + // project's own flags, whose effect on the build the framework cannot + // see. Hashed into VariantId because such a flag typically decides what + // gets compiled or bundled (`--no-webgpu` dropping entries from + // cfg.files, say), and without it both flag settings share one bin dir + // and interleave their outputs there. Populated by ApplyStandardArgs; + // the args it does recognise are excluded, since their effect already + // shows up in target/march/mtune/debug/sysroot/type. + std::vector projectArgs; std::vector tests; // Lint rules for `crafter-build lint`. Populate via AddLintRule. std::vector lintRules; @@ -315,6 +324,12 @@ export namespace Crafter { compileKey += "|W:"; compileKey += f; } + // Sorted by ApplyStandardArgs, so flag order on the command line + // doesn't split the cache. + for (const std::string& a : projectArgs) { + compileKey += "|A:"; + compileKey += a; + } std::size_t configHash = std::hash{}(compileKey); return std::format("{}-{}-{}-{}-{:08x}", name, target, march, mtune, configHash); } @@ -345,6 +360,16 @@ export namespace Crafter { CRAFTER_API int Run(int argc, char** argv); + // Delete the bin/ and build/ trees beside `projectFile`, returning the paths + // that existed and were removed. Backs `crafter-build clean`. + // + // Deliberately does not load the project: cleaning is most often reached + // when something is already wrong, and a clean that first needs project.cpp + // to compile (and its git dependencies to be present) is useless exactly + // then. That means variant directories are not enumerated — the whole tree + // goes, which is what the by-hand `rm -rf bin build` did anyway. + CRAFTER_API std::vector CleanProject(const fs::path& projectFile); + // Add a small index.html + runtime.js pair next to the .wasm output so the // build can be loaded directly in a browser (just `serve` the bin dir and // open it). Opt-in: WASI builds destined for wasmtime/wasmer don't need diff --git a/project.cpp b/project.cpp index ed9486d..0def7cd 100644 --- a/project.cpp +++ b/project.cpp @@ -108,6 +108,7 @@ extern "C" Configuration CrafterBuildProject(std::span a cfg.AddTest("ModuleInterface").Dependencies({ CrafterBuildLib.get() }); cfg.AddTest("DependencyLink").Dependencies({ CrafterBuildLib.get() }); cfg.AddTest("IncrementalInterfaceChange").Dependencies({ CrafterBuildLib.get() }); + cfg.AddTest("CleanProject").Dependencies({ CrafterBuildLib.get() }); cfg.AddTest("ShaderCompile").Dependencies({ CrafterBuildLib.get() }); cfg.AddTest("StandardArgs").Dependencies({ CrafterBuildLib.get() }); cfg.AddTest("TestRunnerSpec").Dependencies({ CrafterBuildLib.get() }); @@ -121,6 +122,9 @@ extern "C" Configuration CrafterBuildProject(std::span a // for project.so. cfg.AddTest("ConcurrentCacheRace").Dependencies({ CrafterBuildLib.get() }) .LinkFlag("-Wl,--export-dynamic").LinkFlag("-ldl"); + // Same LoadProject wiring as ConcurrentCacheRace above. + cfg.AddTest("HostCacheSourceStamp").Dependencies({ CrafterBuildLib.get() }) + .LinkFlag("-Wl,--export-dynamic").LinkFlag("-ldl"); cfg.AddTest("ConcurrentDependencyReset").Dependencies({ CrafterBuildLib.get() }); cfg.AddTest("Lint").Dependencies({ CrafterBuildLib.get() }); cfg.AddTest("HouseRules").Dependencies({ CrafterBuildLib.get() }); diff --git a/tests/CleanProject/main.cpp b/tests/CleanProject/main.cpp new file mode 100644 index 0000000..4a9c8f0 --- /dev/null +++ b/tests/CleanProject/main.cpp @@ -0,0 +1,79 @@ +// 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; + +// `crafter-build clean` exists so the fix for a stale build is discoverable +// rather than folklore. It must not need the project to load: the state you most +// want to clear is one where the build is already broken. +namespace { + std::int32_t Failures = 0; + + void Check(bool cond, std::string_view msg) { + if (!cond) { + std::println(std::cerr, "FAIL: {}", msg); + ++Failures; + } + } + + fs::path StageProject(std::string_view leaf) { + fs::path root = fs::temp_directory_path() / std::format("crafter-build-clean-{}", leaf); + fs::remove_all(root); + fs::create_directories(root / "bin" / "app-x86_64-pc-linux-gnu-native-native-0badf00d"); + fs::create_directories(root / "build" / "app-x86_64-pc-linux-gnu-native-native-0badf00d"); + std::ofstream(root / "bin" / "app-x86_64-pc-linux-gnu-native-native-0badf00d" / "app") << "binary"; + std::ofstream(root / "build" / "app-x86_64-pc-linux-gnu-native-native-0badf00d" / "main_impl.o") << "object"; + std::ofstream(root / "keep-me.txt") << "untouched"; + return root; + } +} + +int main() { + { + // A project file that could never compile: clean has to work anyway. + fs::path root = StageProject("broken"); + fs::path projectFile = root / "project.cpp"; + std::ofstream(projectFile) << "this is not valid C++ at all\n"; + + std::vector removed = CleanProject(projectFile); + Check(removed.size() == 2, std::format("both trees reported removed, got {}", removed.size())); + Check(!fs::exists(root / "bin"), "bin/ is gone"); + Check(!fs::exists(root / "build"), "build/ is gone"); + Check(fs::exists(root / "keep-me.txt"), "unrelated files in the project root are left alone"); + Check(fs::exists(projectFile), "the project file itself is left alone"); + } + + { + // Idempotent, and quiet about it — nothing to remove is not an error. + fs::path root = StageProject("twice"); + fs::path projectFile = root / "project.cpp"; + std::ofstream(projectFile) << "\n"; + + CleanProject(projectFile); + std::vector second = CleanProject(projectFile); + Check(second.empty(), "a second clean reports nothing removed"); + } + + { + // Resolved relative to the project file, not the cwd, so --project= + // cleans the project it names. + fs::path root = StageProject("elsewhere"); + fs::path projectFile = root / "project.cpp"; + std::ofstream(projectFile) << "\n"; + fs::path cwdBin = fs::current_path() / "bin"; + bool cwdBinExisted = fs::exists(cwdBin); + + CleanProject(projectFile); + Check(!fs::exists(root / "bin"), "the named project's bin/ is gone"); + Check(fs::exists(cwdBin) == cwdBinExisted, "the cwd's bin/ is untouched"); + } + + if (Failures > 0) { + std::println(std::cerr, "{} assertions failed", Failures); + return 1; + } + return 0; +} diff --git a/tests/HostCacheSourceStamp/main.cpp b/tests/HostCacheSourceStamp/main.cpp new file mode 100644 index 0000000..b6904e8 --- /dev/null +++ b/tests/HostCacheSourceStamp/main.cpp @@ -0,0 +1,150 @@ +// SPDX-License-Identifier: LGPL-3.0-only +// SPDX-FileCopyrightText: Copyright (C) 2026 Catcrafts® + +#include +import std; +import Crafter.Build; +namespace fs = std::filesystem; +using namespace Crafter; + +// The host PCM cache is keyed by `-` alone, so every +// crafter-build on a machine — a package install and a working checkout, two +// checkouts of different versions — writes the same `Crafter.Build-*.pcm` +// files. Freshness was a per-file mtime comparison, which says "the PCM is +// newer than my source, reuse it" whether or not it was built from *my* +// source. The loser then compiled its project.cpp against the other install's +// declarations: no error, and a project.so built against a layout that doesn't +// match the library it's loaded into — the same silent shape as issue #27. +// +// So the invalidation has to key on the source bytes, and this test asserts it +// notices a change the mtime rule provably cannot see: different content with a +// deliberately *older* timestamp. +namespace { + std::int32_t Failures = 0; + + void Check(bool cond, std::string_view msg) { + if (!cond) { + std::println(std::cerr, "FAIL: {}", msg); + ++Failures; + } + } + + // The cache subdirectory is named for the host target and march; glob for + // it rather than reconstructing the naming rule here. + fs::path FindModuleCacheDir(const fs::path& cacheRoot) { + for (const fs::directory_entry& entry : fs::directory_iterator(cacheRoot / "crafter.build")) { + if (entry.is_directory()) return entry.path(); + } + return {}; + } + + std::string ReadStamp(const fs::path& moduleCacheDir) { + std::ifstream in(moduleCacheDir / "crafter-build-sources.stamp"); + std::string stamp; + std::getline(in, stamp); + return stamp; + } +} + +int main() { + fs::path installedHome = GetCrafterBuildHome(); + if (!fs::exists(installedHome / "Crafter.Build.cppm")) { + std::println(std::cerr, "SKIP: no module sources at {}", installedHome.string()); + return 77; + } + + fs::path scratch = fs::temp_directory_path() / "crafter-build-host-cache-source-stamp"; + std::error_code ec; + fs::remove_all(scratch, ec); + fs::create_directories(scratch); + + // A private copy of the module sources, so the mutation below can't touch + // the real install, and a cold private cache so the first LoadProject has + // to populate it. + fs::path home = scratch / "share" / "crafter-build"; + fs::create_directories(home.parent_path()); + fs::copy(installedHome, home, fs::copy_options::recursive); + setenv("CRAFTER_BUILD_HOME", home.string().c_str(), 1); + + fs::path cacheRoot = scratch / "cache"; + fs::create_directories(cacheRoot); + setenv("XDG_CACHE_HOME", cacheRoot.string().c_str(), 1); + + fs::path projectDir = scratch / "project"; + fs::create_directories(projectDir); + fs::path projectFile = projectDir / "project.cpp"; + std::ofstream(projectFile) << + "import std;\n" + "import Crafter.Build;\n" + "using namespace Crafter;\n" + "extern \"C\" Configuration CrafterBuildProject(std::span) {\n" + " Configuration cfg;\n" + " cfg.path = \"./\";\n" + " cfg.name = \"stamped\";\n" + " cfg.outputName = \"stamped\";\n" + " cfg.target = HostTarget();\n" + " cfg.type = ConfigurationType::Executable;\n" + " return cfg;\n" + "}\n"; + + std::array noArgs = {}; + + try { + (void)LoadProject(projectFile, noArgs); + } catch (const std::exception& e) { + std::println(std::cerr, "FAIL: first LoadProject threw: {}", e.what()); + return 1; + } + + fs::path moduleCacheDir = FindModuleCacheDir(cacheRoot); + Check(!moduleCacheDir.empty(), "a module cache directory was created"); + if (moduleCacheDir.empty()) return 1; + + fs::path probePcm = moduleCacheDir / "Crafter.Build-Clang.pcm"; + Check(fs::exists(probePcm), "the cold run populated the cache"); + if (!fs::exists(probePcm)) return 1; + + std::string firstStamp = ReadStamp(moduleCacheDir); + Check(!firstStamp.empty(), "the cold run recorded a source stamp"); + fs::file_time_type afterCold = fs::last_write_time(probePcm); + + // Unchanged sources must be a no-op — the stamp is an invalidation key, not + // an excuse to precompile eleven modules on every invocation. + try { + (void)LoadProject(projectFile, noArgs); + } catch (const std::exception& e) { + std::println(std::cerr, "FAIL: warm LoadProject threw: {}", e.what()); + return 1; + } + Check(fs::last_write_time(probePcm) == afterCold, "unchanged sources do not rebuild the cache"); + Check(ReadStamp(moduleCacheDir) == firstStamp, "unchanged sources keep the same stamp"); + + // Now the case the mtime rule cannot see. The edit lands in a module the + // probe does not name, because the cached PCMs import each other: a change + // anywhere in the set invalidates all of them. + fs::path edited = home / "Crafter.Build-Progress.cppm"; + { + std::ofstream append(edited, std::ios::app); + append << "\n// Content change with a backdated timestamp.\n"; + } + fs::last_write_time(edited, afterCold - std::chrono::hours(24)); + Check(fs::last_write_time(edited) < fs::last_write_time(probePcm), "the edited source really is older than the cached PCM"); + + try { + (void)LoadProject(projectFile, noArgs); + } catch (const std::exception& e) { + std::println(std::cerr, "FAIL: LoadProject after the edit threw: {}", e.what()); + return 1; + } + + Check(fs::last_write_time(probePcm) > afterCold, "a content change rebuilds the cache even when the source's mtime is older"); + Check(ReadStamp(moduleCacheDir) != firstStamp, "the recorded stamp follows the sources"); + + fs::remove_all(scratch, ec); + + if (Failures > 0) { + std::println(std::cerr, "{} assertions failed", Failures); + return 1; + } + return 0; +} diff --git a/tests/StandardArgs/main.cpp b/tests/StandardArgs/main.cpp index b30ca46..5024908 100644 --- a/tests/StandardArgs/main.cpp +++ b/tests/StandardArgs/main.cpp @@ -83,6 +83,31 @@ int main() { Check(!q.Get("--other=").has_value(), "ArgQuery::Get returns nullopt for absent prefix"); } + // Flags ApplyStandardArgs doesn't itself interpret are collected into + // cfg.projectArgs, which feeds VariantId — a project flag that changes what + // gets compiled has to move the output directory with it. + { + Configuration cfg; + std::array raw = { + "--debug", "--no-webgpu", "--target=aarch64-linux-gnu", "--feature=fancy", "bare", + }; + ApplyStandardArgs(cfg, raw); + std::vector expected = { "--feature=fancy", "--no-webgpu", "bare" }; + Check(cfg.projectArgs == expected, std::format("only unrecognised args are collected, sorted; got [{}]", std::format("{}", std::views::join_with(cfg.projectArgs, std::string(", ")) | std::ranges::to()))); + } + + // Order and repetition must not perturb the set — otherwise the same build + // spelled two ways would land in two directories. + { + Configuration a; + Configuration b; + std::array forward = { "--alpha", "--beta" }; + std::array reversed = { "--beta", "--alpha", "--beta" }; + ApplyStandardArgs(a, forward); + ApplyStandardArgs(b, reversed); + Check(a.projectArgs == b.projectArgs, "projectArgs is order- and duplicate-insensitive"); + } + if (Failures > 0) { std::println(std::cerr, "{} assertions failed", Failures); return 1; diff --git a/tests/VariantId/main.cpp b/tests/VariantId/main.cpp index 0f1b2e4..022b0d6 100644 --- a/tests/VariantId/main.cpp +++ b/tests/VariantId/main.cpp @@ -96,6 +96,25 @@ int main() { b.march = "x86-64-v3"; Check(a.VariantId() != b.VariantId(), "march change perturbs VariantId"); } + { + // A project flag the framework can't interpret still decides what gets + // compiled and bundled, so it has to key into VariantId. Otherwise both + // settings share one bin dir and interleave their outputs there — an + // index.html from one alongside a script from the other. + Configuration a = MakeBase(); + Configuration b = MakeBase(); + b.projectArgs.push_back("--no-webgpu"); + Check(a.VariantId() != b.VariantId(), "project arg perturbs VariantId"); + Check(a.BinDir() != b.BinDir(), "project arg yields a distinct BinDir"); + } + { + // Distinct project args must not collide with each other either. + Configuration a = MakeBase(); + a.projectArgs.push_back("--no-webgpu"); + Configuration b = MakeBase(); + b.projectArgs.push_back("--no-audio"); + Check(a.VariantId() != b.VariantId(), "different project args yield different VariantId"); + } { // PcmDir() differs between Executable (in BuildDir) and Library // (in BinDir) — Library PCMs land in the installable bin dir so -- 2.47.3 From 2fbcb6fbf36966faa942dc64daaaa9c55041a370 Mon Sep 17 00:00:00 2001 From: catbot Date: Thu, 30 Jul 2026 17:43:46 +0000 Subject: [PATCH 3/3] docs: incrementality, variant identity and clean in the README --- README.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/README.md b/README.md index 09bea8b..4b8b6de 100644 --- a/README.md +++ b/README.md @@ -125,6 +125,12 @@ Per-import precise tracking for both within-project and cross-project module dep Diamond deps (`A → {B, C}; B → X; C → X`) build `X` exactly once via a `std::shared_future` cache. +Tracking is derived from each translation unit's `import` statements, which are scanned when the sources are declared. `cfg.dependencies` is often assigned *afterwards* — `AddTest` works that way — so `Build()` re-resolves any import that matched nothing at scan time before it compares mtimes. Without that, a consumer of a dependency's module carried no edge to it at all: adding a data member to that dependency's interface rebuilt the library, relinked the consumer, and left the consumer's object compiled against the old class layout. Nothing fails to link when a member is added, so the result was a working build and a crash later. + +Everything that changes what gets built belongs in the variant id, since it names the `bin/` and `build/` directory. That includes project args crafter-build itself doesn't interpret: `crafter-build` and `crafter-build -- --no-webgpu` get separate directories rather than interleaving their outputs in one. The cached host PCMs under `/crafter.build/-/` are shared by every crafter-build on the machine, so they are invalidated by a hash of the module sources rather than by mtime — an mtime can't tell a newer PCM from one built by a different install. + +`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. + ## Tests Tests live under `tests//`. The simplest case is a single C++ file: -- 2.47.3