Repository navigation
build: isolate the target dir when runtime/ diverges #801
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
269ec87
daea05e
672a6c1
fa2d907
67ea5ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -97,6 +97,13 @@ leaves=":(exclude)crates/nub-cli :(exclude)crates/nub-native :(exclude)crates/nu | |||||
| # Both checks are deliberately broad (any path under a depended-on crate, not just | ||||||
| # *.rs): over-isolating on an irrelevant file costs one cold build; under-isolating | ||||||
| # risks the clobber. Depended-on = every workspace/vendored crate except nub-cli. | ||||||
| # `runtime/` is in the set for a different reason than the crates: the binary | ||||||
| # resolves `runtime/*.cjs` at run time from the tree that compiled nub-core (its | ||||||
| # baked CARGO_MANIFEST_DIR), so a shared-bucket binary loads whichever SHARER | ||||||
| # compiled last — a worktree with edited runtime files would test a sibling's | ||||||
| # copy, silently. Isolating on runtime divergence keeps every shared-bucket | ||||||
| # binary pointed at base-identical runtime content. (Observed: three worktrees' | ||||||
| # red/green verdicts flipped as siblings rebuilt the common bucket.) | ||||||
| # `-C "$root"` on the git queries so the pathspecs resolve from the repo root | ||||||
| # regardless of the CWD the wrapper was invoked from (a subdir would otherwise | ||||||
| # misread them). The final `exec cargo` still runs in the original CWD. | ||||||
|
|
@@ -105,17 +112,17 @@ diverged="" | |||||
| if [ -n "$base" ]; then | ||||||
| # shellcheck disable=SC2086 # $leaves must word-split into separate pathspecs | ||||||
| diverged=$(git -C "$root" diff --name-only "$base" -- \ | ||||||
| vendor/aube crates $leaves 2>/dev/null || true) | ||||||
| vendor/aube crates runtime $leaves 2>/dev/null || true) | ||||||
| fi | ||||||
| # shellcheck disable=SC2086 | ||||||
| untracked=$(git -C "$root" ls-files --others --exclude-standard -- \ | ||||||
| vendor/aube crates $leaves 2>/dev/null || true) | ||||||
| vendor/aube crates runtime $leaves 2>/dev/null || true) | ||||||
|
|
||||||
| # The content key names the bucket AND, when isolating, names the seed to clone | ||||||
| # from — so it is computed unconditionally. `ls-files -s` emits the staged blob | ||||||
| # OIDs, so this is a pure content hash of the depended-on crates. ~0.2s. | ||||||
| # shellcheck disable=SC2086 | ||||||
| key=$(git -C "$root" ls-files -s -- vendor/aube crates $leaves 2>/dev/null \ | ||||||
| key=$(git -C "$root" ls-files -s -- vendor/aube crates runtime $leaves 2>/dev/null \ | ||||||
| | shasum 2>/dev/null | cut -c1-12 || true) | ||||||
| if [ "$keyed" = 1 ]; then | ||||||
| bucket="$shared${key:+-$key}" | ||||||
|
|
@@ -159,19 +166,60 @@ seed_from() { | |||||
| } | ||||||
|
|
||||||
| # The newest COMPLETE bucket on disk, or nothing. Buckets are only ever created | ||||||
| # by non-diverged worktrees, so every bucket name is a base-content key. The | ||||||
| # `.seeding` filter is load-bearing: a claim dir shares the `$shared-` prefix and | ||||||
| # `-t` sorts newest-first, so an actively-filling claim would be the MOST likely | ||||||
| # pick — seeding from a half-copied tree pairs a fresh-looking fingerprint with a | ||||||
| # truncated rlib, the exact failure this design exists to prevent. | ||||||
| # by non-diverged worktrees, so every bucket name is a base-content key. Two | ||||||
| # load-bearing filters: the `.seeding` skip (a claim dir shares the `$shared-` | ||||||
| # prefix and `-t` sorts newest-first, so an actively-filling claim would be the | ||||||
| # MOST likely pick — seeding from a half-copied tree pairs a fresh-looking | ||||||
| # fingerprint with a truncated rlib), and the emptiness skip (a just-created | ||||||
| # bucket is published by mkdir+touch below BEFORE cargo writes anything, so | ||||||
| # during its first cold build it is the newest by mtime while holding nothing | ||||||
| # worth cloning). | ||||||
| newest_bucket() { | ||||||
| # shellcheck disable=SC2012 # names are ours and contain no newlines | ||||||
| ls -dt "$shared"-* 2>/dev/null | grep -v '\.seeding$' | head -1 | ||||||
| for _b in $(ls -dt "$shared"-* 2>/dev/null | grep -v '\.seeding$'); do | ||||||
| if [ -n "$(ls -A "$_b" 2>/dev/null)" ]; then | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Technical details# The emptiness filter is not a warmth check
## Affected sites
- `scripts/rust-build.sh:180` — `[ -n "$(ls -A "$_b" 2>/dev/null)" ]` is satisfied by `.rustc_info.json` alone.
- `scripts/rust-build.sh:173-176` — "during its first cold build it is the newest by mtime while holding nothing worth cloning" describes a window that ends ~50 ms in.
- `scripts/rust-build.sh:203-205` — "an isolated worktree seeding during that window falls back to the newest non-empty bucket" states the cost accounting is now covered; it is not.
- `scripts/rust-build.sh:231-235` — an isolated worktree therefore still CoW-clones an artifact-less bucket and prints `(seeded from shared-target-...)` while paying a full cold build.
## Measurement
```sh
cargo new --lib /tmp/cp2 && cd /tmp/cp2 && rm -rf target && mkdir target
(cargo build --offline &) ; poll ls -A target every 50ms
# -> non-empty after 57ms, first entry: .rustc_info.json
```
## Required outcome
- Either the check distinguishes a bucket with compiled artifacts from one cargo has merely touched, or the two comments stop claiming it covers the cold build.
## Suggested approach (optional)
A warmth test wants the artifact directories rather than the root — e.g. requiring a non-empty `"$_b"/*/deps` or `"$_b"/*/.fingerprint`. If you would rather not add that, narrowing both comments to what `ls -A` actually buys is a legitimate resolution; the cost is a cold build, not a wrong build.
## Open questions for the human
- The prior round asked whether bounding this is worth a warmth check at all. If the answer is still "the window is short enough to accept", then the `ls -A` condition is not earning its complexity and the comments are the only thing that needs to change. |
||||||
| printf '%s\n' "$_b" | ||||||
| return 0 | ||||||
| fi | ||||||
| done | ||||||
| return 1 | ||||||
| } | ||||||
|
|
||||||
| if [ -n "$diverged" ] || [ -n "$untracked" ]; then | ||||||
| target="$root/target" # private, worktree-local; removed with the worktree | ||||||
| why="isolated — this worktree diverges a depended-on crate from origin/main" | ||||||
| isolated=1 | ||||||
| else | ||||||
| target="$bucket" # shared fast path, content-keyed | ||||||
| why="shared bucket ${key:-none} — depended-on crate content" | ||||||
| isolated="" | ||||||
| # A SHARED bucket is NEVER seeded from another bucket. Any other bucket's | ||||||
| # hashed content differs by construction (a matching key would BE this bucket), | ||||||
| # CoW clones preserve mtimes, and cargo's path-dep freshness check | ||||||
| # (LocalFingerprint::CheckDepInfo) compares mtimes rather than content — so a | ||||||
| # sharer whose checkout is older than the seeded dep-info would accept a | ||||||
| # foreign rlib as fresh: the phantom E0063 this keying exists to prevent, | ||||||
| # persisting inside a bucket other worktrees join. A key move therefore costs | ||||||
| # the first SHARER one cold build, later sharers join warm, and an isolated | ||||||
| # worktree seeding during that window falls back to the newest non-empty | ||||||
| # bucket. (The isolated branch may seed foreign content because its target is | ||||||
| # PRIVATE and its just-edited divergent files carry fresh mtimes.) | ||||||
| fi | ||||||
|
|
||||||
| # `--print-target` resolves the target dir the SAME way a real build would and | ||||||
| # prints it, so callers that must locate the artifact afterwards (make | ||||||
| # install-dev symlinking nub-dev) follow the shared/isolated decision instead of | ||||||
| # hardcoding a path that is only right in one of the two cases. It exits HERE, | ||||||
| # before the seeding and the mkdir/GC below, so it is a pure read on BOTH | ||||||
| # branches — tooling that compares against it (the disk-reduction bucket sweep's | ||||||
| # positive control) must not materialize a bucket, a private target, or a GC | ||||||
| # pass as a side effect of asking. | ||||||
| if [ "${1:-}" = "--print-target" ]; then | ||||||
| printf '%s\n' "$target" | ||||||
| exit 0 | ||||||
| fi | ||||||
|
|
||||||
| if [ -n "$isolated" ]; then | ||||||
| # ANY bucket will do, and the fallback is what makes seeding fire at all for | ||||||
| # the ordinary case: $bucket is keyed by this worktree's INDEX, but buckets are | ||||||
| # only created by non-diverged worktrees, so a committed or staged divergence | ||||||
|
|
@@ -180,22 +228,11 @@ if [ -n "$diverged" ] || [ -n "$untracked" ]; then | |||||
| # clobber a sibling, cargo just rebuilds it, and the crates.io rlibs we're | ||||||
| # after are identical across buckets. | ||||||
| _seed="$bucket" | ||||||
| [ -d "$_seed" ] || _seed=$(newest_bucket) | ||||||
| [ -d "$_seed" ] && [ -n "$(ls -A "$_seed" 2>/dev/null)" ] || _seed=$(newest_bucket) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Under Technical details# `newest_bucket`'s failure exit must not propagate through `set -e`
## Affected sites
- `scripts/rust-build.sh:231` — `_seed=$(newest_bucket)` terminates the `&& ... ||` list, so its status is the list's status and `set -e` fires.
- `scripts/rust-build.sh:177-186` — `newest_bucket()` gained `return 1`; the pre-`fa2d907` pipeline form could not fail.
- `scripts/rust-build.sh:232` — `[ -n "$_seed" ] && [ -d "$_seed" ] || _seed="$shared"` already normalizes an empty `$_seed`, so the callers downstream need no change.
- `Makefile:117-136` (`make verify`), `Makefile:68` (`make install-dev`) — the two gates that go down with it.
## Reproduction (run in this repo, both shapes confirmed on sh/dash/bash)
```sh
touch runtime/__probe.txt # force divergence
rm -rf /tmp/nb-a
NUB_SHARED_TARGET=/tmp/nb-a/target sh scripts/rust-build.sh --version # rc=1, no output
mkdir -p /tmp/nb-c/target # second shape: present but EMPTY
NUB_SHARED_TARGET=/tmp/nb-c/target sh scripts/rust-build.sh --version # rc=1, no output
```
Restoring only the pre-`fa2d907` `newest_bucket` body and the old `_seed` line into a copy of the script makes both cases print the banner and run cargo (rc=0), which isolates the cause to this change rather than to the surrounding restructure.
## Required outcome
- A diverged worktree with no usable seed source falls through to `$shared` and builds cold, instead of aborting.
- The `present-but-empty` seed dir behaves the same way.
Suggested change
|
||||||
| [ -n "$_seed" ] && [ -d "$_seed" ] || _seed="$shared" | ||||||
| if seed_from "$_seed" "$target"; then | ||||||
| why="$why (seeded from $(basename "$_seed"))" | ||||||
| fi | ||||||
| else | ||||||
| target="$bucket" # shared fast path, content-keyed | ||||||
| why="shared bucket ${key:-none} — depended-on crate content" | ||||||
| # MIGRATION: content-keying renames the bucket, so on the first run after this | ||||||
| # change lands the warm legacy dir would be orphaned and every worktree would | ||||||
| # eat one cold build. Seed the new bucket from it (same CoW clone, ~0 cost). | ||||||
| # Self-retiring: the legacy dir is covered by the GC below, so once every live | ||||||
| # worktree has moved to a keyed bucket it ages out on its own. | ||||||
| if [ "$target" != "$shared" ] && seed_from "$shared" "$target"; then | ||||||
| why="$why (migrated from the legacy shared dir)" | ||||||
| fi | ||||||
| fi | ||||||
|
|
||||||
| mkdir -p "$target" | ||||||
|
|
@@ -243,15 +280,6 @@ if [ "${NUB_BUILD_FG:-}" != "1" ] && [ "$(uname)" = "Darwin" ] \ | |||||
| qos="taskpolicy -c utility" | ||||||
| fi | ||||||
|
|
||||||
| # `--print-target` resolves the target dir the SAME way a real build would and | ||||||
| # prints it, so callers that must locate the artifact afterwards (make | ||||||
| # install-dev symlinking nub-dev) follow the shared/isolated decision instead of | ||||||
| # hardcoding a path that is only right in one of the two cases. | ||||||
| if [ "${1:-}" = "--print-target" ]; then | ||||||
| printf '%s\n' "$target" | ||||||
| exit 0 | ||||||
| fi | ||||||
|
|
||||||
| # The machine-global rustc wrapper (make qos-global) now carries the GLOBAL | ||||||
| # CONCURRENCY SEMAPHORE, not just a QoS clamp, so this invocation must let it | ||||||
| # run. It previously blanked RUSTC_WRAPPER — sound when the wrapper only | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.