From f77d91317117d3aae7a35859250cf9ef83312a23 Mon Sep 17 00:00:00 2001 From: Jorijn van der Graaf Date: Sat, 31 Oct 2026 02:09:24 +0100 Subject: [PATCH] fix(external): fetch and build each clone once per build pass `crafter-build test` builds a project's tests side by side, and a project whose tests each copy its externalDependencies hands every one of them the same unpinned dependency. Each test's Build() pulled the shared clone. Before the clone had a lock, those pulls ran at the same time and broke its .git/FETCH_HEAD, which failed every test of Crafter.Graphics (issue #34). Since 653dd0b they wait for each other, but every one is still a network round trip: 26 tests cost 26 pulls of each dependency, one after another, and for a CMake dependency as many configure checks and builds. depResults already records one build pass, keyed by the PcmDir of every dependency configuration built in it. An external dependency now goes in the same map, keyed by the directory its build leaves (ExternalBuildDir: the clone, or for CMake its build directory). The first configuration that needs it fetches and builds it; the others wait for that and take its outcome. Keying on the build directory rather than the clone keeps a CMake declaration from taking the outcome of a header-only declaration of the same clone, which only fetched it. A fresh depResults is a fresh pass, so the next build still pulls, and every caller of Build() gets this: RunTests, a plain build, and the pragma dependencies LoadProject builds. To make that possible BuildExternal is split. BuildExternal is now the shared work, lock, fetch, configure and build, and returns the error. ExternalOutputs computes what one declaration compiles and links with, because two declarations of one clone can differ in includeDirs, libDirs and libs; ExternalBuildResult loses its error field. The work no longer stops between steps when the configuration that started it is cancelled: others may be waiting on it, and a run cut short would leave them a clone nobody fetched. A dependency configuration's Build() already runs to completion for the same reason. The promise and future the dependency threads used to build or wait for a configuration are now one helper, OncePerPass, which both kinds use. An exception thrown in BuildExternal used to escape its thread into std::terminate; it is now a build error, as for a dependency. With eight tests that share a local dependency, a test run fetched it eight times before this and fetches it once now (SharedExternalFetch, next commit). Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- implementations/Crafter.Build-Clang.cpp | 83 ++++++++------ implementations/Crafter.Build-External.cpp | 125 ++++++++++----------- interfaces/Crafter.Build-Clang.cppm | 7 ++ interfaces/Crafter.Build-External.cppm | 21 +++- tests/ExternalArtifactTime/main.cpp | 10 +- 6 files changed, 140 insertions(+), 108 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a97508b..6bdde3b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -86,7 +86,7 @@ Conventions the existing tests follow: - Collect failures with a `Check(cond, msg)` helper that prints `FAIL: ...` to stderr and counts, then return nonzero at the end. Do not stop at the first failure unless later steps are meaningless without it. - Drive the engine in-process: `Build(cfg, depResults, depMutex)`, `RunTests`, the lint driver. Do not shell out to `bin/crafter-build` and grep its stdout. - Fixtures under `tests//fixture/` are inputs. A test that edits or touches files first stages a copy under `fs::temp_directory_path() / "crafter-build-"` and removes it on the next run. Variant sources are checked in as `*.in` files and copied over the original inside the staged copy. -- Use a fresh `depResults` map per build pass: the map memoises each configuration's result for one pass, so reusing it skips rebuilds you meant to observe. `Build()` itself is safe to call repeatedly on the same `Configuration`. +- Use a fresh `depResults` map per build pass: the map memoises each configuration's result, and each external dependency's fetch and build, for one pass, so reusing it skips rebuilds and pulls you meant to observe. `Build()` itself is safe to call repeatedly on the same `Configuration`. - Set `cfg.path` before calling `GetInterfacesAndImplementations`, and spell `cFiles` as absolute paths: both resolve against the working directory, not the configuration. A bug fix ships with a test that fails before the fix and passes after it, in its own `test:` commit when it documents the behaviour on its own. diff --git a/implementations/Crafter.Build-Clang.cpp b/implementations/Crafter.Build-Clang.cpp index 416856e..550023e 100644 --- a/implementations/Crafter.Build-Clang.cpp +++ b/implementations/Crafter.Build-Clang.cpp @@ -423,6 +423,28 @@ CompileCommand Crafter::GetCompileCommand(const Configuration& config) { return out; } +namespace { + // The outcome of `work` for `key` in this build pass. The first caller + // runs it; every other caller, whether at the same time or later, waits + // for that run and gets its result or its exception. The keys are the + // directories the work writes, a configuration's PcmDir and an external + // dependency's ExternalBuildDir, so the two kinds never collide. + std::shared_future OncePerPass(std::unordered_map>& depResults, std::mutex& depMutex, const fs::path& key, const std::function& work) { + std::promise promise; + std::shared_future outcome = promise.get_future().share(); + { + std::lock_guard lock(depMutex); + if (auto [it, inserted] = depResults.try_emplace(key, outcome); !inserted) return it->second; + } + try { + promise.set_value(work()); + } catch (...) { + promise.set_exception(std::current_exception()); + } + return outcome; + } +} + BuildResult Crafter::Build(Configuration& config, std::unordered_map>& depResults, std::mutex& depMutex) { // Before anything is spawned: one message naming the missing compiler // beats a clang++-not-found per translation unit. @@ -678,14 +700,30 @@ BuildResult Crafter::Build(Configuration& config, std::unordered_map built = OncePerPass(depResults, depMutex, ExternalBuildDir(dep, config.target), [&] { + return BuildResult{BuildExternal(dep, config.target), false, {}}; + }); + error = built.get().result; + if (error.empty()) externalResults[i] = ExternalOutputs(dep, config.target); + } catch (const std::exception& e) { + error = std::format("external dep {} threw: {}", dep.name, e.what()); + } + if (error.empty()) return; + bool expected = false; + if (buildCancelled.compare_exchange_strong(expected, true)) { + buildError = std::move(error); } }); } @@ -746,35 +784,8 @@ BuildResult Crafter::Build(Configuration& config, std::unordered_map> promise; - std::shared_future resultFuture; - bool isBuilder = false; - - depMutex.lock(); - fs::path cacheKey = dep->PcmDir(); - auto it = depResults.find(cacheKey); - if (it == depResults.end()) { - isBuilder = true; - promise = std::make_shared>(); - resultFuture = promise->get_future().share(); - depResults.emplace(cacheKey, resultFuture); - } else { - resultFuture = it->second; - } - depMutex.unlock(); - - if (isBuilder) { - BuildResult built; - try { - built = Build(*dep, depResults, depMutex); - } catch (...) { - promise->set_exception(std::current_exception()); - throw; - } - promise->set_value(std::move(built)); - } - - const BuildResult& result = resultFuture.get(); + const std::shared_future built = OncePerPass(depResults, depMutex, dep->PcmDir(), [&] { return Build(*dep, depResults, depMutex); }); + const BuildResult& result = built.get(); fileMutex.lock(); for (const std::string& lib : result.libs) libSet.insert(lib); for (const std::string& f : result.publicCompileFlags) publicFlagSet.insert(f); diff --git a/implementations/Crafter.Build-External.cpp b/implementations/Crafter.Build-External.cpp index a0528ca..6e12988 100644 --- a/implementations/Crafter.Build-External.cpp +++ b/implementations/Crafter.Build-External.cpp @@ -231,15 +231,16 @@ std::vector Crafter::ExternalIncludeFlags(const ExternalDependency& return flags; } -ExternalBuildResult Crafter::BuildExternal(const ExternalDependency& dep, std::string_view target, std::atomic& cancelled) { - ExternalBuildResult result; - - if (cancelled.load(std::memory_order_relaxed)) return result; +fs::path Crafter::ExternalBuildDir(const ExternalDependency& dep, std::string_view target) { + fs::path cloneDir = ExternalCloneDir(dep, target); + if (cloneDir.empty() || dep.builder != ExternalBuilder::CMake) return cloneDir; + return cloneDir / "build"; +} +std::string Crafter::BuildExternal(const ExternalDependency& dep, std::string_view target) { std::string name = dep.name.empty() ? DeriveName(dep.source) : dep.name; if (name.empty()) { - result.error = std::format("Could not derive name for external dependency from URL '{}'", dep.source.url); - return result; + return std::format("Could not derive name for external dependency from URL '{}'", dep.source.url); } fs::path externalRoot = GetCacheDir() / "external"; @@ -247,77 +248,73 @@ ExternalBuildResult Crafter::BuildExternal(const ExternalDependency& dep, std::s std::error_code ec; fs::create_directories(externalRoot, ec); if (ec) { - result.error = std::format("Failed to create {}: {}", externalRoot.string(), ec.message()); - return result; + return std::format("Failed to create {}: {}", externalRoot.string(), ec.message()); } } fs::path cloneDir = ExternalCloneDir(dep, target); - // Builds running side by side (tests built in parallel, two - // crafter-builds) share the clone: one fetches and builds it at a time. + // Build() makes one call for all the configurations of a pass, but two + // crafter-builds, or two passes, can still reach the same clone at once, + // and two pulls in one clone race on its .git/FETCH_HEAD. FileLock lock(fs::path(std::format("{}.lock", cloneDir.string()))); - std::string fetchErr = FetchGit(dep.source, cloneDir); - if (!fetchErr.empty()) { - result.error = std::format("git fetch for '{}': {}", name, fetchErr); - return result; + if (std::string err = FetchGit(dep.source, cloneDir); !err.empty()) { + return std::format("git fetch for '{}': {}", name, err); } + if (dep.builder != ExternalBuilder::CMake) return ""; - if (cancelled.load(std::memory_order_relaxed)) return result; - - fs::path cmakeBuildDir; - if (dep.builder == ExternalBuilder::CMake) { - cmakeBuildDir = cloneDir / "build"; - if (std::string err = ConfigureCMake(cloneDir, cmakeBuildDir, dep.options, target); !err.empty()) { - result.error = std::format("cmake configure for '{}': {}", name, err); - return result; - } - if (cancelled.load(std::memory_order_relaxed)) return result; - if (std::string err = BuildCMake(cmakeBuildDir); !err.empty()) { - result.error = std::format("cmake build for '{}': {}", name, err); - return result; - } + const fs::path cmakeBuildDir = ExternalBuildDir(dep, target); + if (std::string err = ConfigureCMake(cloneDir, cmakeBuildDir, dep.options, target); !err.empty()) { + return std::format("cmake configure for '{}': {}", name, err); } + if (std::string err = BuildCMake(cmakeBuildDir); !err.empty()) { + return std::format("cmake build for '{}': {}", name, err); + } + return ""; +} +ExternalBuildResult Crafter::ExternalOutputs(const ExternalDependency& dep, std::string_view target) { + ExternalBuildResult result; result.compileFlags = ExternalIncludeFlags(dep, target); - if (dep.builder == ExternalBuilder::CMake) { - // Each search path gets both a -L (link-time) and a -Wl,-rpath - // (runtime-loader). The rpath stays embedded in the produced - // binary so it picks up shared deps from the cache without any - // LD_LIBRARY_PATH gymnastics. Static deps (.a) ignore the rpath - // harmlessly. The PE/COFF target (mingw) resolves DLLs via PATH or - // adjacency rather than rpath, so lld warns if we pass it. - const bool isPe = target == "x86_64-w64-mingw32"; - std::string buildDirAbs = fs::absolute(cmakeBuildDir).string(); - result.linkFlags.push_back(std::format("-L{}", buildDirAbs)); - if (!isPe) result.linkFlags.push_back(std::format("-Wl,-rpath,{}", buildDirAbs)); - for (const fs::path& libDir : dep.libDirs) { - fs::path full = libDir.is_absolute() ? libDir : cmakeBuildDir / libDir; - std::string fullAbs = fs::absolute(full).string(); - result.linkFlags.push_back(std::format("-L{}", fullAbs)); - if (!isPe) result.linkFlags.push_back(std::format("-Wl,-rpath,{}", fullAbs)); - } - for (const std::string& lib : dep.libs) { - result.linkFlags.push_back(std::format("-l{}", lib)); - } + const fs::path cmakeBuildDir = ExternalBuildDir(dep, target); + if (dep.builder != ExternalBuilder::CMake || cmakeBuildDir.empty()) return result; - // The newest library in every directory the link searches: the build - // directory and the libDirs, where projects like msquic put theirs, - // versioned names (libmsquic.so.2) included. - auto isLibrary = [](const std::string& name) { - return name.ends_with(".a") || name.ends_with(".so") || name.contains(".so.") || name.ends_with(".dll"); - }; - std::vector searched = { cmakeBuildDir }; - for (const fs::path& libDir : dep.libDirs) searched.push_back(libDir.is_absolute() ? libDir : cmakeBuildDir / libDir); - for (const fs::path& dir : searched) { - std::error_code listEc; - for (fs::directory_iterator it(dir, listEc), end; !listEc && it != end; it.increment(listEc)) { - std::error_code ec; - if (!it->is_regular_file(ec) || !isLibrary(it->path().filename().string())) continue; - fs::file_time_type t = it->last_write_time(ec); - if (!ec && t > result.latestArtifact) result.latestArtifact = t; - } + // Each search path gets both a -L (link-time) and a -Wl,-rpath + // (runtime-loader). The rpath stays embedded in the produced + // binary so it picks up shared deps from the cache without any + // LD_LIBRARY_PATH gymnastics. Static deps (.a) ignore the rpath + // harmlessly. The PE/COFF target (mingw) resolves DLLs via PATH or + // adjacency rather than rpath, so lld warns if we pass it. + const bool isPe = target == "x86_64-w64-mingw32"; + std::string buildDirAbs = fs::absolute(cmakeBuildDir).string(); + result.linkFlags.push_back(std::format("-L{}", buildDirAbs)); + if (!isPe) result.linkFlags.push_back(std::format("-Wl,-rpath,{}", buildDirAbs)); + for (const fs::path& libDir : dep.libDirs) { + fs::path full = libDir.is_absolute() ? libDir : cmakeBuildDir / libDir; + std::string fullAbs = fs::absolute(full).string(); + result.linkFlags.push_back(std::format("-L{}", fullAbs)); + if (!isPe) result.linkFlags.push_back(std::format("-Wl,-rpath,{}", fullAbs)); + } + for (const std::string& lib : dep.libs) { + result.linkFlags.push_back(std::format("-l{}", lib)); + } + + // The newest library in every directory the link searches: the build + // directory and the libDirs, where projects like msquic put theirs, + // versioned names (libmsquic.so.2) included. + auto isLibrary = [](const std::string& name) { + return name.ends_with(".a") || name.ends_with(".so") || name.contains(".so.") || name.ends_with(".dll"); + }; + std::vector searched = { cmakeBuildDir }; + for (const fs::path& libDir : dep.libDirs) searched.push_back(libDir.is_absolute() ? libDir : cmakeBuildDir / libDir); + for (const fs::path& dir : searched) { + std::error_code listEc; + for (fs::directory_iterator it(dir, listEc), end; !listEc && it != end; it.increment(listEc)) { + std::error_code ec; + if (!it->is_regular_file(ec) || !isLibrary(it->path().filename().string())) continue; + fs::file_time_type t = it->last_write_time(ec); + if (!ec && t > result.latestArtifact) result.latestArtifact = t; } } diff --git a/interfaces/Crafter.Build-Clang.cppm b/interfaces/Crafter.Build-Clang.cppm index 36cf1a9..12be520 100644 --- a/interfaces/Crafter.Build-Clang.cppm +++ b/interfaces/Crafter.Build-Clang.cppm @@ -656,6 +656,13 @@ export namespace Crafter { }; CompileCommand GetCompileCommand(const Configuration& config); + // Builds `config` and the configurations it depends on. depResults is the + // record of one build pass, guarded by depMutex and keyed by the directory + // each piece of work writes: a dependency's PcmDir, an external + // dependency's ExternalBuildDir. Configurations built with the same map, + // as RunTests builds its tests, build a shared dependency and fetch a + // shared external once between them. A fresh map starts a new pass, which + // checks everything again and pulls unpinned externals again. BuildResult Build(Configuration& config, std::unordered_map>& depResults, std::mutex& depMutex); // Takes main's argv verbatim so the CLI entry point is a one-liner; diff --git a/interfaces/Crafter.Build-External.cppm b/interfaces/Crafter.Build-External.cppm index fe8e229..d415587 100644 --- a/interfaces/Crafter.Build-External.cppm +++ b/interfaces/Crafter.Build-External.cppm @@ -36,7 +36,6 @@ export namespace Crafter { }; struct ExternalBuildResult { - std::string error; std::vector compileFlags; std::vector linkFlags; fs::file_time_type latestArtifact = fs::file_time_type::min(); @@ -47,12 +46,30 @@ export namespace Crafter { // through this, so a caller that only needs to know where the headers ended // up cannot drift from where they actually are. fs::path ExternalCloneDir(const ExternalDependency& dep, std::string_view target); + // Where BuildExternal leaves this dependency: the CMake build directory, + // or the clone itself when it is only fetched. Pure, like ExternalCloneDir. + // Build() keys a pass's record of the work on it rather than on the clone, + // so a declaration that shares the clone but builds it with CMake is not + // handed the outcome of a fetch alone. + fs::path ExternalBuildDir(const ExternalDependency& dep, std::string_view target); // The -I flags this dependency contributes, from its declared includeDirs. // Also pure, which is what lets the linter's AST layer parse a source that // includes an external's headers without running a build to find out where // they are. The paths only resolve once something has fetched the clone. std::vector ExternalIncludeFlags(const ExternalDependency& dep, std::string_view target); - ExternalBuildResult BuildExternal(const ExternalDependency& dep, std::string_view target, std::atomic& cancelled); + // Fetches the clone and, for ExternalBuilder::CMake, configures and builds + // it. Empty on success, otherwise what failed. A lock on the clone is held + // throughout, so another crafter-build, or another build pass in this one, + // works on it before or after this call and never during it. Not + // cancellable once started: Build() hands the outcome to every + // configuration of the pass that declares the dependency, and a run cut + // short would leave them a clone nobody fetched. + std::string BuildExternal(const ExternalDependency& dep, std::string_view target); + // What a configuration that declares `dep` compiles and links with once + // BuildExternal has run: the -I flags and, for CMake, the -L, -rpath and -l + // flags and the newest library. Per declaration, because two that share a + // clone can still differ in includeDirs, libDirs and libs. + ExternalBuildResult ExternalOutputs(const ExternalDependency& dep, std::string_view target); // Specification for a sibling crafter-build project to fetch and depend on. // GitSource picks the revision: leave branch + commit empty for the diff --git a/tests/ExternalArtifactTime/main.cpp b/tests/ExternalArtifactTime/main.cpp index 923ce0c..a2b4c5f 100644 --- a/tests/ExternalArtifactTime/main.cpp +++ b/tests/ExternalArtifactTime/main.cpp @@ -7,8 +7,8 @@ namespace fs = std::filesystem; using namespace Crafter; // A CMake dependency's newest library decides when its consumers are stale -// (BuildExternal's latestArtifact). Only files directly in the CMake build -// directory named *.a or *.so were looked at, so msquic's +// (the latestArtifact of ExternalOutputs). Only files directly in the CMake +// build directory named *.a or *.so were looked at, so msquic's // bin/Release/libmsquic.so.2, in one of the dependency's libDirs and with a // version after the .so, never counted: a rebuilt msquic left everything // built against the old one in place. @@ -69,9 +69,9 @@ int main() { dep.builder = ExternalBuilder::CMake; dep.libDirs = { "bin/Release" }; dep.libs = { "versioned" }; - std::atomic cancelled = false; - const ExternalBuildResult result = BuildExternal(dep, HostTarget(), cancelled); - Check(result.error.empty(), std::format("the dependency builds: {}", result.error)); + const std::string error = BuildExternal(dep, HostTarget()); + Check(error.empty(), std::format("the dependency builds: {}", error)); + const ExternalBuildResult result = ExternalOutputs(dep, HostTarget()); const fs::path library = ExternalCloneDir(dep, HostTarget()) / "build" / "bin" / "Release" / "libversioned.so.2.0.0"; Check(fs::exists(library), std::format("the library is where its CMake project puts it: {}", library.string()));