From f8e20bba9f912e5cd36df7429d3540f0e92f4702 Mon Sep 17 00:00:00 2001 From: Santiago Date: Mon, 24 Aug 2026 17:32:28 -0300 Subject: [PATCH 1/3] fix(build): stamp the revision the binary is actually built from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cargo caches a build script's output and re-runs the script only when a path it asked to watch changes. `vergen-gitcl` watches the `HEAD` file of whichever worktree ran the script first, and a symbolic `HEAD` does not change when its branch advances — so with a `CARGO_TARGET_DIR` shared across git worktrees the recorded revision froze at the first value it ever saw, and `dolos --version` confidently named a commit the binary was not built from. Replace the vergen build script with a direct `git` call that asks to be re-run unconditionally, falling back to `unknown` when git cannot answer and suffixing `-dirty` when tracked files differ from `HEAD`. Report the revision through `dolos --version`, which previously carried only the package version, and add a test asserting the stamped revision names the tree it was built from. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016WeTSXxC7KPU4WSmgCW5oB --- Cargo.lock | 49 ----------------------- Cargo.toml | 3 -- build.rs | 87 ++++++++++++++++++++++++++++++++++++++--- src/bin/dolos/banner.rs | 8 +--- src/bin/dolos/main.rs | 2 +- tests/build_revision.rs | 65 ++++++++++++++++++++++++++++++ 6 files changed, 149 insertions(+), 65 deletions(-) create mode 100644 tests/build_revision.rs diff --git a/Cargo.lock b/Cargo.lock index 10704de00..7113d8848 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1466,7 +1466,6 @@ dependencies = [ "tracing-opentelemetry", "tracing-subscriber", "trait-variant", - "vergen-gitcl", "xxhash-rust", ] @@ -3449,15 +3448,6 @@ dependencies = [ "libc", ] -[[package]] -name = "num_threads" -version = "0.1.7" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5c7398b9c8b70908f6371f47ed36737907c87c52af34c268fed0bf0ceb92ead9" -dependencies = [ - "libc", -] - [[package]] name = "number_prefix" version = "0.4.0" @@ -5636,9 +5626,7 @@ checksum = "743bd48c283afc0388f9b8827b976905fb217ad9e647fae3a379a9283c4def2c" dependencies = [ "deranged", "itoa", - "libc", "num-conv", - "num_threads", "powerfmt", "serde_core", "time-core", @@ -6448,43 +6436,6 @@ version = "0.2.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "accd4ea62f7bb7a82fe23066fb0957d48ef677f6eeb8215f372f52e48bb32426" -[[package]] -name = "vergen" -version = "9.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b849a1f6d8639e8de261e81ee0fc881e3e3620db1af9f2e0da015d4382ceaf75" -dependencies = [ - "anyhow", - "derive_builder", - "rustversion", - "vergen-lib", -] - -[[package]] -name = "vergen-gitcl" -version = "9.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "77ff3b5300a085d6bcd8fc96a507f706a28ae3814693236c9b409db71a1d15b9" -dependencies = [ - "anyhow", - "derive_builder", - "rustversion", - "time", - "vergen", - "vergen-lib", -] - -[[package]] -name = "vergen-lib" -version = "9.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b34a29ba7e9c59e62f229ae1932fb1b8fb8a6fdcc99215a641913f5f5a59a569" -dependencies = [ - "anyhow", - "derive_builder", - "rustversion", -] - [[package]] name = "version_check" version = "0.9.5" diff --git a/Cargo.toml b/Cargo.toml index 38e2669e3..a48c397b6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -124,9 +124,6 @@ serial_test = "3" dolos-redb3 = { path = "crates/redb3" } stelae = { path = "crates/stelae" } -[build-dependencies] -vergen-gitcl = "9.1.0" - [[bench]] name = "archive_backends" harness = false diff --git a/build.rs b/build.rs index 6aca822e6..31c6c7801 100644 --- a/build.rs +++ b/build.rs @@ -1,9 +1,86 @@ -use vergen_gitcl::{Emitter, GitclBuilder}; +//! Stamps the revision the binary is built from into the compiled artifact. +//! +//! The interesting part is not reading the revision — it is making sure the +//! recorded one cannot outlive the code it names. Cargo caches a build +//! script's output and re-runs the script only when one of the paths it asked +//! to watch has changed. Any git path this script could watch belongs to the +//! worktree that happened to build first: with a `CARGO_TARGET_DIR` shared +//! across worktrees there is a single cached output for all of them, so a +//! second worktree at a different commit silently inherits the first +//! worktree's revision. Watching a `HEAD` that is a symbolic ref makes it +//! worse still — its contents do not change when the branch advances, so even +//! the original worktree keeps the first revision it ever recorded. +//! +//! So the script asks to be re-run unconditionally, by watching a path under +//! `OUT_DIR` that it never creates. The cost is one `git` invocation per +//! build plus a recompile of this package; the gain is that +//! `dolos --version` names the revision it was actually built from, or says +//! `unknown`, and never a confident wrong answer. -fn main() -> Result<(), Box> { - let gitcl = GitclBuilder::default().sha(true).build()?; +use std::path::Path; +use std::process::Command; - Emitter::default().add_instructions(&gitcl)?.emit()?; +/// Set this to record a revision without consulting git — for a build from a +/// source archive, or a pipeline that already knows the commit it checked out. +const REVISION_OVERRIDE: &str = "DOLOS_GIT_SHA"; - Ok(()) +/// Runs `git` in the package directory, returning its trimmed stdout on +/// success. Any failure — no git, no repository, no commit — is `None`, which +/// the caller turns into `unknown` rather than into a guess. +fn git(manifest_dir: &str, args: &[&str]) -> Option { + let output = Command::new("git") + .arg("--no-optional-locks") + .args(args) + .current_dir(manifest_dir) + .output() + .ok()?; + + if !output.status.success() { + return None; + } + + Some(String::from_utf8_lossy(&output.stdout).trim().to_owned()) +} + +/// The revision to stamp: the override if one is set, else `HEAD` abbreviated +/// to eight characters, suffixed `-dirty` when tracked files differ from it. +fn revision(manifest_dir: &str) -> String { + if let Ok(sha) = std::env::var(REVISION_OVERRIDE) { + if !sha.trim().is_empty() { + return sha.trim().to_owned(); + } + } + + let Some(sha) = + git(manifest_dir, &["rev-parse", "--short=8", "HEAD"]).filter(|s| !s.is_empty()) + else { + return "unknown".to_owned(); + }; + + match git( + manifest_dir, + &["status", "--porcelain", "--untracked-files=no"], + ) { + Some(status) if !status.is_empty() => format!("{sha}-dirty"), + _ => sha, + } +} + +fn main() { + // Cargo reads these from the environment of *this* process, so they + // describe the build actually running. The `env!` equivalents would be + // baked into the build script binary, which is itself cached across + // worktrees — the very staleness this script exists to avoid. + let manifest_dir = std::env::var("CARGO_MANIFEST_DIR").expect("CARGO_MANIFEST_DIR"); + let out_dir = std::env::var("OUT_DIR").expect("OUT_DIR"); + let package_version = std::env::var("CARGO_PKG_VERSION").expect("CARGO_PKG_VERSION"); + + let never_created = Path::new(&out_dir).join("always-rerun"); + println!("cargo:rerun-if-changed={}", never_created.display()); + println!("cargo:rerun-if-env-changed={REVISION_OVERRIDE}"); + + let revision = revision(&manifest_dir); + + println!("cargo:rustc-env=DOLOS_GIT_SHA={revision}"); + println!("cargo:rustc-env=DOLOS_VERSION={package_version} ({revision})"); } diff --git a/src/bin/dolos/banner.rs b/src/bin/dolos/banner.rs index e15356968..a6606de35 100644 --- a/src/bin/dolos/banner.rs +++ b/src/bin/dolos/banner.rs @@ -11,11 +11,5 @@ pub fn print_init_banner() { println!("\x1b[90moooooooooooooooooooooooooooooooooooooooo\x1b[0m"); - let git_sha = option_env!("VERGEN_GIT_SHA").unwrap_or("unknown"); - - println!( - "\x1b[1;95mv{} ({})\x1b[0m\n", - env!("CARGO_PKG_VERSION"), - git_sha - ); + println!("\x1b[1;95mv{}\x1b[0m\n", env!("DOLOS_VERSION")); } diff --git a/src/bin/dolos/main.rs b/src/bin/dolos/main.rs index 43a15f6d2..32fe53c58 100644 --- a/src/bin/dolos/main.rs +++ b/src/bin/dolos/main.rs @@ -70,7 +70,7 @@ enum Command { #[derive(Debug, Parser)] #[clap(name = "Dolos")] #[clap(bin_name = "dolos")] -#[clap(author, version, about, long_about = None)] +#[clap(author, version = env!("DOLOS_VERSION"), about, long_about = None)] struct Cli { #[command(subcommand)] command: Command, diff --git a/tests/build_revision.rs b/tests/build_revision.rs new file mode 100644 index 000000000..517f18c64 --- /dev/null +++ b/tests/build_revision.rs @@ -0,0 +1,65 @@ +//! Guards the revision the binary reports about itself. +//! +//! A build script's output is cached, and every git path it could watch +//! belongs to whichever worktree ran it first — so with a `CARGO_TARGET_DIR` +//! shared across worktrees a binary used to report a commit it was not built +//! from, confidently and with no way to tell. `build.rs` now re-runs on every +//! build; this test asserts the property that failed, so a future change that +//! reintroduces caching is caught here rather than in a report that +//! attributes results to the wrong commit. + +use std::process::Command; + +fn git(args: &[&str]) -> Option { + let manifest_dir = std::env::var("CARGO_MANIFEST_DIR").ok()?; + + let output = Command::new("git") + .arg("--no-optional-locks") + .args(args) + .current_dir(manifest_dir) + .output() + .ok()?; + + if !output.status.success() { + return None; + } + + Some(String::from_utf8_lossy(&output.stdout).trim().to_owned()) +} + +#[test] +fn stamped_revision_names_the_tree_it_was_built_from() { + if std::env::var("DOLOS_GIT_SHA").is_ok() { + eprintln!("skipped: the revision was overridden via DOLOS_GIT_SHA"); + return; + } + + let Some(head) = git(&["rev-parse", "--short=8", "HEAD"]).filter(|s| !s.is_empty()) else { + eprintln!("skipped: no git revision available for this tree"); + return; + }; + + let Some(status) = git(&["status", "--porcelain", "--untracked-files=no"]) else { + eprintln!("skipped: git could not report the state of the working tree"); + return; + }; + + let expected = if status.is_empty() { + head + } else { + format!("{head}-dirty") + }; + + assert_eq!( + env!("DOLOS_GIT_SHA"), + expected, + "the binary reports a revision it was not built from; the build script's \ + output was reused from an earlier build (a target directory shared across \ + git worktrees is the usual cause)", + ); + + assert_eq!( + env!("DOLOS_VERSION"), + format!("{} ({})", env!("CARGO_PKG_VERSION"), expected), + ); +} From 1372a913fea8a4a123b877d60f505a9b7b6f7f00 Mon Sep 17 00:00:00 2001 From: Santiago Date: Mon, 24 Aug 2026 17:59:42 -0300 Subject: [PATCH 2/3] fix(build): detect the revision override with its own compile-time marker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test guarded on `env::var("DOLOS_GIT_SHA")` being set, meaning to skip itself only when a build supplied the revision explicitly. But cargo puts every `rustc-env` variable into the environment of the executables it runs, and the build script always emits `DOLOS_GIT_SHA` — so the guard held on every build and the test skipped without asserting anything. Emit a separate `DOLOS_GIT_SHA_OVERRIDDEN` marker only when an override actually supplied the revision, and check it with `option_env!`. The test now asserts on ordinary builds and skips only for real overrides. Verified: with the stamp forced to a wrong value the test fails, naming both the stamped and the expected revision; with `DOLOS_GIT_SHA` set it still skips. Raised by CodeRabbit on PR #1262. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NrWE4c81Qt7N38HzVKykHZ --- build.rs | 30 ++++++++++++++++++++++-------- tests/build_revision.rs | 5 ++++- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/build.rs b/build.rs index 31c6c7801..d516fa96b 100644 --- a/build.rs +++ b/build.rs @@ -24,6 +24,13 @@ use std::process::Command; /// source archive, or a pipeline that already knows the commit it checked out. const REVISION_OVERRIDE: &str = "DOLOS_GIT_SHA"; +/// Emitted only when [`REVISION_OVERRIDE`] supplied the revision. The stamp +/// itself cannot carry that fact: cargo puts every `rustc-env` variable into +/// the environment of the executables it runs, so a test asking whether +/// `DOLOS_GIT_SHA` is set sees the stamp and concludes every build was +/// overridden. This marker is absent unless an override really happened. +const OVERRIDE_MARKER: &str = "DOLOS_GIT_SHA_OVERRIDDEN"; + /// Runs `git` in the package directory, returning its trimmed stdout on /// success. Any failure — no git, no repository, no commit — is `None`, which /// the caller turns into `unknown` rather than into a guess. @@ -42,28 +49,31 @@ fn git(manifest_dir: &str, args: &[&str]) -> Option { Some(String::from_utf8_lossy(&output.stdout).trim().to_owned()) } -/// The revision to stamp: the override if one is set, else `HEAD` abbreviated +/// The revision to stamp, and whether it came from [`REVISION_OVERRIDE`] +/// rather than from git: the override if one is set, else `HEAD` abbreviated /// to eight characters, suffixed `-dirty` when tracked files differ from it. -fn revision(manifest_dir: &str) -> String { +fn revision(manifest_dir: &str) -> (String, bool) { if let Ok(sha) = std::env::var(REVISION_OVERRIDE) { if !sha.trim().is_empty() { - return sha.trim().to_owned(); + return (sha.trim().to_owned(), true); } } let Some(sha) = git(manifest_dir, &["rev-parse", "--short=8", "HEAD"]).filter(|s| !s.is_empty()) else { - return "unknown".to_owned(); + return ("unknown".to_owned(), false); }; - match git( + let sha = match git( manifest_dir, &["status", "--porcelain", "--untracked-files=no"], ) { Some(status) if !status.is_empty() => format!("{sha}-dirty"), _ => sha, - } + }; + + (sha, false) } fn main() { @@ -79,8 +89,12 @@ fn main() { println!("cargo:rerun-if-changed={}", never_created.display()); println!("cargo:rerun-if-env-changed={REVISION_OVERRIDE}"); - let revision = revision(&manifest_dir); + let (revision, overridden) = revision(&manifest_dir); - println!("cargo:rustc-env=DOLOS_GIT_SHA={revision}"); + println!("cargo:rustc-env={REVISION_OVERRIDE}={revision}"); println!("cargo:rustc-env=DOLOS_VERSION={package_version} ({revision})"); + + if overridden { + println!("cargo:rustc-env={OVERRIDE_MARKER}=1"); + } } diff --git a/tests/build_revision.rs b/tests/build_revision.rs index 517f18c64..3b14fdc15 100644 --- a/tests/build_revision.rs +++ b/tests/build_revision.rs @@ -29,7 +29,10 @@ fn git(args: &[&str]) -> Option { #[test] fn stamped_revision_names_the_tree_it_was_built_from() { - if std::env::var("DOLOS_GIT_SHA").is_ok() { + // Not `env::var("DOLOS_GIT_SHA")`: cargo puts the stamp itself into this + // process's environment, so that guard holds for every build and skips the + // whole test. The marker exists only when an override really happened. + if option_env!("DOLOS_GIT_SHA_OVERRIDDEN").is_some() { eprintln!("skipped: the revision was overridden via DOLOS_GIT_SHA"); return; } From 55bb8da2e25fc925f7b6041c4bafbd1b3fb4278c Mon Sep 17 00:00:00 2001 From: Santiago Date: Mon, 24 Aug 2026 17:59:48 -0300 Subject: [PATCH 3/3] style(build): trim a build-script comment to the comment standard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comment-only sweep of this PR's diff against the TxPipe comment standard: one four-line inline comment condensed to two, its closing clause dropped as a restatement of the module docstring above it. Nothing else in the diff needed changing — 0 removed, 1 trimmed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NrWE4c81Qt7N38HzVKykHZ --- build.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/build.rs b/build.rs index d516fa96b..0f8b5c1be 100644 --- a/build.rs +++ b/build.rs @@ -77,10 +77,8 @@ fn revision(manifest_dir: &str) -> (String, bool) { } fn main() { - // Cargo reads these from the environment of *this* process, so they - // describe the build actually running. The `env!` equivalents would be - // baked into the build script binary, which is itself cached across - // worktrees — the very staleness this script exists to avoid. + // Read from this process's environment, not via `env!` — that would bake the + // values into the build-script binary, which is itself cached across worktrees. let manifest_dir = std::env::var("CARGO_MANIFEST_DIR").expect("CARGO_MANIFEST_DIR"); let out_dir = std::env::var("OUT_DIR").expect("OUT_DIR"); let package_version = std::env::var("CARGO_PKG_VERSION").expect("CARGO_PKG_VERSION");