Catcrafts/Crafter.Build
Watch
2
0
Fork
You've already forked Crafter.Build
 0
Code Issues 5 Pull requests Projects Releases 7 Packages Wiki Activity Actions

fix(external): fetch and build each clone once per build pass

Browse source 
`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 <noreply@anthropic.com>
This commit is contained in:
Jorijn van der Graaf 2026-10-31 02:09:24 +01:00
commit f77d913171
Signed by: jorijnvdgraaf
GPG key ID: 2937E59CDCC1BCFB
6 changed files with 141 additions and 109 deletions
Download patch file Download diff file Expand all files Collapse all files

83
implementations/Crafter.Build-Clang.cpp
View file

@ -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<BuildResult> OncePerPass(std::unordered_map<fs::path, std::shared_future<BuildResult>>& depResults, std::mutex& depMutex, const fs::path& key, const std::function<BuildResult()>& work) {
std::promise<BuildResult> promise;
std::shared_future<BuildResult> 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<fs::path, std::shared_future<BuildResult>>& 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<fs::path, s
externalThreads.reserve(config.externalDependencies.size());
for (std::size_t i = 0; i < config.externalDependencies.size(); ++i) {
externalThreads.emplace_back([&, i]() {
Progress::Task task(std::format("Building external dep {}", config.externalDependencies[i].name));
const ExternalDependency& dep = config.externalDependencies[i];
Progress::Task task(std::format("Building external dep {}", dep.name));
if (buildCancelled.load(std::memory_order_relaxed)) return;
externalResults[i] = BuildExternal(config.externalDependencies[i], config.target, buildCancelled);
if (!externalResults[i].error.empty()) {
bool expected = false;
if (buildCancelled.compare_exchange_strong(expected, true)) {
buildError = externalResults[i].error;
}
std::string error;
try {
// Every configuration of the pass that declares this
// dependency shares one fetch and build of it. A project's
// tests often each copy its externalDependencies, and RunTests
// builds them side by side: each of them pulling the clone
// cost a network round trip per test, and before the clone had
// a lock, the simultaneous pulls broke its FETCH_HEAD and
// failed every test (issue #34).
const std::shared_future<BuildResult> 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<fs::path, s
try {
if (buildCancelled.load(std::memory_order_relaxed)) return;
std::shared_ptr<std::promise<BuildResult>> promise;
std::shared_future<BuildResult> 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<std::promise<BuildResult>>();
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<BuildResult> 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);

125
implementations/Crafter.Build-External.cpp
View file

@ -231,15 +231,16 @@ std::vector<std::string> Crafter::ExternalIncludeFlags(const ExternalDependency&
return flags;
}
ExternalBuildResult Crafter::BuildExternal(const ExternalDependency& dep, std::string_view target, std::atomic<bool>& 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<fs::path> 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<fs::path> 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;
}
}