Catcrafts/Crafter.Build

Watch

Fork 0

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

Merged

jorijnvdgraaf merged 4 commits from claude/issue-34 into master

2026-10-31 02:20:57 +00:00

jorijnvdgraaf commented

2026-10-31 01:11:33 +00:00

Owner

Closes #34.

653dd0b already put a lock on each external clone, which stopped parallel pulls from breaking .git/FETCH_HEAD. But every test configuration still pulled the shared clone itself, one after another: 26 Crafter.Graphics tests cost 26 network round trips per unpinned dependency, and as many configure checks and builds for a CMake one. This PR does the issue's two remaining items: each clone is fetched once per build pass, and there is a regression test on the path the issue hit.

What changes

Externals go in depResults. The map already records each dependency configuration built in one pass, keyed by its PcmDir. External dependencies now go in the same map, keyed by the directory their build leaves (ExternalBuildDir: the clone, or its CMake build directory). The first configuration that needs one fetches and builds it, and the rest wait for that outcome. A fresh map is a fresh pass, so the next build still pulls. Every caller of Build() gets this unchanged: RunTests, a plain build, and the pragma dependencies LoadProject builds.
BuildExternal is split. BuildExternal(dep, target) is now the shared work (lock, fetch, configure, build) and returns the error. The new ExternalOutputs(dep, target) gives each declaration its own flags, because two declarations of one clone can differ in includeDirs, libDirs and libs. ExternalBuildResult loses its error field.
No cancellation partway through. Once started, the shared work runs to completion, as a dependency configuration's Build() already does. Other configurations may be waiting on it, and a run cut short would leave them a clone nobody fetched. The cost: a build that fails for another reason while an external is fetching now also waits for that external's CMake build.
One helper. OncePerPass replaces the inline promise and future code of the dependency threads, and the external threads use it too. An exception thrown in BuildExternal used to escape its thread into std::terminate; it is now a build error.

Choices worth a look

Reusing depResults puts two kinds of entries in one map, and an external's entry only uses BuildResult::result. The alternative was a new pass type in Build()'s signature, which 15 tests and every in-process caller spell out. I kept the signature; the interface comment on Build() now says what the map holds.
The key is ExternalBuildDir rather than the clone. Keyed on the clone, a CMake declaration could take the outcome of a header-only declaration of the same clone, which only fetched it, and then link against a build that never ran. That case is unlikely and has no test.
BuildExternal and ExternalBuildResult are exported, and both change. Nothing outside Build() and one test used them, in this repo or the sibling repos.

Tests

SharedExternalFetch (new). Eight tests that include a header from one tracked branch of a local repository run through RunTests at once, twice, with the branch moving in between. GIT_TRACE counts the fetches: each clone or pull runs one git upload-pack. On the commit before the fix, each run fetches 8 times. With it, each run fetches once, and the second run still pulls the new commit.
ConcurrentExternalFetch (extended). Its threads now also race BuildExternal, whose lock still keeps two crafter-builds apart. With that lock removed the test failed in 2 of 3 runs: it is a race test, not a deterministic one.
ExternalArtifactTime is adapted to the split API.

The README now says how often an unpinned external is pulled, and lists target in the clone's cache key. AGENTS.md's note on depResults mentions fetches.

Verification

./bin/crafter-build test: 34 passed, 2 skipped (ConcurrentCacheRace without CRAFTER_BUILD_HOME, WasmHelloRuns without wasmtime). That is master's result plus the new test. ConcurrentCacheRace passes with CRAFTER_BUILD_HOME set.
format --check is clean, and the self-host build succeeds.
lint exits 1 with 48 [naming] findings. Master has exactly the same set; this branch adds none.
Not done: running Crafter.Graphics's own suite against this branch.

🤖 Generated with Claude Code

Closes #34. `653dd0b` already put a lock on each external clone, which stopped parallel pulls from breaking `.git/FETCH_HEAD`. But every test configuration still pulled the shared clone itself, one after another: 26 Crafter.Graphics tests cost 26 network round trips per unpinned dependency, and as many configure checks and builds for a CMake one. This PR does the issue's two remaining items: each clone is fetched once per build pass, and there is a regression test on the path the issue hit. ## What changes - **Externals go in `depResults`.** The map already records each dependency configuration built in one pass, keyed by its `PcmDir`. External dependencies now go in the same map, keyed by the directory their build leaves (`ExternalBuildDir`: the clone, or its CMake build directory). The first configuration that needs one fetches and builds it, and the rest wait for that outcome. A fresh map is a fresh pass, so the next build still pulls. Every caller of `Build()` gets this unchanged: `RunTests`, a plain build, and the pragma dependencies `LoadProject` builds. - **`BuildExternal` is split.** `BuildExternal(dep, target)` is now the shared work (lock, fetch, configure, build) and returns the error. The new `ExternalOutputs(dep, target)` gives each declaration its own flags, because two declarations of one clone can differ in `includeDirs`, `libDirs` and `libs`. `ExternalBuildResult` loses its `error` field. - **No cancellation partway through.** Once started, the shared work runs to completion, as a dependency configuration's `Build()` already does. Other configurations may be waiting on it, and a run cut short would leave them a clone nobody fetched. The cost: a build that fails for another reason while an external is fetching now also waits for that external's CMake build. - **One helper.** `OncePerPass` replaces the inline promise and future code of the dependency threads, and the external threads use it too. An exception thrown in `BuildExternal` used to escape its thread into `std::terminate`; it is now a build error. ## Choices worth a look - Reusing `depResults` puts two kinds of entries in one map, and an external's entry only uses `BuildResult::result`. The alternative was a new pass type in `Build()`'s signature, which 15 tests and every in-process caller spell out. I kept the signature; the interface comment on `Build()` now says what the map holds. - The key is `ExternalBuildDir` rather than the clone. Keyed on the clone, a CMake declaration could take the outcome of a header-only declaration of the same clone, which only fetched it, and then link against a build that never ran. That case is unlikely and has no test. - `BuildExternal` and `ExternalBuildResult` are exported, and both change. Nothing outside `Build()` and one test used them, in this repo or the sibling repos. ## Tests - **`SharedExternalFetch` (new).** Eight tests that include a header from one tracked branch of a local repository run through `RunTests` at once, twice, with the branch moving in between. `GIT_TRACE` counts the fetches: each clone or pull runs one `git upload-pack`. On the commit before the fix, each run fetches 8 times. With it, each run fetches once, and the second run still pulls the new commit. - **`ConcurrentExternalFetch` (extended).** Its threads now also race `BuildExternal`, whose lock still keeps two crafter-builds apart. With that lock removed the test failed in 2 of 3 runs: it is a race test, not a deterministic one. - **`ExternalArtifactTime`** is adapted to the split API. The README now says how often an unpinned external is pulled, and lists `target` in the clone's cache key. AGENTS.md's note on `depResults` mentions fetches. ## Verification - `./bin/crafter-build test`: 34 passed, 2 skipped (`ConcurrentCacheRace` without `CRAFTER_BUILD_HOME`, `WasmHelloRuns` without wasmtime). That is master's result plus the new test. `ConcurrentCacheRace` passes with `CRAFTER_BUILD_HOME` set. - `format --check` is clean, and the self-host build succeeds. - `lint` exits 1 with 48 `[naming]` findings. Master has exactly the same set; this branch adds none. - Not done: running Crafter.Graphics's own suite against this branch. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

jorijnvdgraaf added 4 commits

2026-10-31 01:11:33 +00:00

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

`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>

test(external): tests that share a dependency fetch it once a run 0a06faec6e

Eight tests that include a header from the same tracked branch of a
local repository go through RunTests at once, and again after the
branch has moved. GIT_TRACE logs every git process, and each fetch of
the repository, clone or pull, runs one `git upload-pack` in it, so the
trace counts the fetches: one per run rather than one per test, and the
second run still pulls and brings the clone to the new commit.

On the commit before the fix both runs fetch eight times. The tests
themselves pass there, because the lock from 653dd0b already keeps the
pulls apart. Going through RunTests, rather than Build() on threads of
the test's own, also holds RunTests to building its tests with one
depResults.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

test(external): race BuildExternal as well as GitFetch 7b548b9ce4

ConcurrentExternalFetch raced eight threads through GitFetch only.
BuildExternal takes the same lock on a line of its own, so that line
could go without any test noticing. Now that the configurations of one
pass share a single BuildExternal, its lock is what keeps two
crafter-builds, or two passes, out of each other's pulls.

The same threads now fetch through both, each into a clone of its own,
and both clones must end at the last round. With the lock in
BuildExternal removed, the test failed in two of three runs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

docs: an unpinned external is pulled once per build

Flatpak / flatpak (pull_request) Successful in 7m22s

Details

Flatpak / flatpak (push) Successful in 11m2s

Details

6e2d835aa6

Say how often an unpinned ExternalDependency is pulled now that the
configurations of a build share it, and list the target in the clone's
cache key, which it is part of.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

jorijnvdgraaf merged commit 6e2d835aa6 into master

2026-10-31 02:20:57 +00:00

jorijnvdgraaf deleted branch claude/issue-34

2026-10-31 02:20:57 +00:00

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

Catcrafts/Crafter.Build!39

No description provided.

Rows
Columns