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