Skip to content

fix(version): show published Cargo sources without the dev marker #5899

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.

Sign up for GitHub

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

Merged
Hmbown merged 2 commits into from
Sep 6, 2026
+46 −12
Merged

fix(version): show published Cargo sources without the dev marker #5899

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7309eb3
fix(version): distinguish packaged Cargo sources from dev checkouts
Sep 5, 2026
de1a531
refactor(version): reuse the shared SHA formatter
Sep 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 46 additions & 12 deletions crates/build-support/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
//! Two different shas live here and they are not interchangeable.
//! `CODEWHALE_BUILD_VERSION`/`CODEWHALE_BUILD_COMMIT` describe *the build the
//! environment asked for* (`CODEWHALE_BUILD_SHA`/`DEEPSEEK_BUILD_SHA`/`GITHUB_SHA`); an unstamped
//! local build renders a `(dev)` marker instead.
//! local checkout renders `(dev)`; an unstamped Cargo source package displays
//! its package version without claiming a release-binary SHA.
//! `CODEWHALE_RELEASE_BUILD_SHA` describes a *published* binary and has no
//! fallback at all, because it leaves the machine.
//!
Expand All @@ -23,7 +24,8 @@
//! So the contract is: a sha appears in the version string only when the
//! build environment supplied one (`CODEWHALE_BUILD_SHA` wins over
//! `GITHUB_SHA`), the build script reruns only when those variables change,
//! and a build nobody stamped says `(dev)`. CI and release builds are
//! and an unstamped checkout says `(dev)`. Cargo source packages use the plain
//! package version. CI and release builds are
//! byte-identical to the old behavior; dogfood builds pass the sha
//! explicitly (the install script prints the exact command).

Expand All @@ -44,19 +46,21 @@ pub fn declare_rerun_conditions(_manifest_dir: &Path) {

/// Emit `cargo:rustc-env=CODEWHALE_BUILD_VERSION=...` — the package version,
/// suffixed with the short build SHA when the environment supplied one
/// (`CODEWHALE_BUILD_SHA`, then `DEEPSEEK_BUILD_SHA`, then `GITHUB_SHA`), or with the literal `dev`
/// marker when it did not. `CODEWHALE_BUILD_COMMIT` is emitted only in the
/// stamped case.
/// (`CODEWHALE_BUILD_SHA`, then `DEEPSEEK_BUILD_SHA`, then `GITHUB_SHA`).
/// Unstamped Cargo source packages show the plain package version; unpackaged
/// checkouts retain `(dev)`. `CODEWHALE_BUILD_COMMIT` is emitted only when stamped.
///
/// `package_version` is the calling build script's `CARGO_PKG_VERSION`;
/// `manifest_dir` is accepted for call-shape stability.
pub fn emit_build_version(_manifest_dir: &Path, package_version: &str) {
/// Cargo writes `Cargo.toml.orig` when normalizing a distributable package.
/// Its presence classifies the source layout, not release provenance: no VCS
/// metadata is read and no additional commit value is emitted.
pub fn emit_build_version(manifest_dir: &Path, package_version: &str) {
let commit = build_commit();

@codewhale-agent codewhale-agent Bot Sep 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declare the marker file as an input so Cargo reruns the build script when the packaged/unpackaged source layout changes.

Suggested change
let commit = build_commit();
println!("cargo:rerun-if-changed=Cargo.toml.orig");
let commit = build_commit();

let build_version = commit
.as_ref()
.and_then(|sha| short_sha(sha.clone()))
.map(|sha| format!("{package_version} ({sha})"))
.unwrap_or_else(|| format!("{package_version} (dev)"));
let build_version = format_build_version(
package_version,
commit.as_deref(),
manifest_dir.join("Cargo.toml.orig").is_file(),

@codewhale-agent codewhale-agent Bot Sep 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WARNING] Build script does not rerun when Cargo.toml.orig changes

emit_build_version now changes its output based on manifest_dir.join("Cargo.toml.orig").is_file(), but declare_rerun_conditions(_manifest_dir) still ignores manifest_dir and only emits environment-based rerun conditions. In incremental builds where the packaged/unpackaged layout changes without an environment variable change, CODEWHALE_BUILD_VERSION can remain stale. Add a cargo:rerun-if-changed directive for the marker file or update declare_rerun_conditions to use manifest_dir.

@codewhale-agent codewhale-agent Bot Sep 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[INFO] Packaged-source filesystem detection is not covered by automated tests

The new tests exercise format_build_version directly, but the crucial Cargo.toml.orig presence check in emit_build_version is only validated by a manual fixture. Consider extracting the path check into a small helper and unit-testing both true and false cases, or adding a tempdir integration test, so the crates.io/checkout distinction cannot regress.

@codewhale-agent codewhale-agent Bot Sep 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WARNING] Build script rerun conditions do not account for Cargo.toml.orig

format_build_version now depends on whether manifest_dir/Cargo.toml.orig exists, but the build script's declared rerun conditions only cover the stamping environment variables. In a reused target directory where a source tree gains or loses Cargo.toml.orig, Cargo may not rerun the build script and can keep a stale CODEWHALE_BUILD_VERSION. Fresh Cargo package builds are unaffected, which limits impact, but the new file dependency should be declared.

@codewhale-agent codewhale-agent Bot Sep 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[INFO] Cargo.toml.orig detection lacks automated coverage

The new tests exercise format_build_version directly, but no unit test creates a temporary Cargo.toml.orig next to a manifest to verify emit_build_version packaged/unpackaged classification. The PR mentions an offline fixture, but that is not part of the automated test suite and could let the filesystem branch regress.

);

println!("cargo:rustc-env=CODEWHALE_BUILD_VERSION={build_version}");
// Keep the pre-rebrand compile-time name through the 0.9.x compatibility
Expand All @@ -67,6 +71,18 @@ pub fn emit_build_version(_manifest_dir: &Path, package_version: &str) {
}
}

fn format_build_version(
package_version: &str,
commit: Option<&str>,
packaged_source: bool,
) -> String {
match commit.and_then(|sha| short_sha(sha.to_string())) {
Some(sha) => format!("{package_version} ({sha})"),
None if packaged_source => package_version.to_string(),

@codewhale-agent codewhale-agent Bot Sep 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WARNING] Invalid stamping SHA now yields plain version for packaged sources

format_build_version treats any short_sha failure as an unstamped build. If CODEWHALE_BUILD_SHA/GITHUB_SHA is set but is not a valid 40-hex value, packaged sources now emit the plain package version, whereas the previous fallback was (dev). That can hide a misconfigured stamped build. Consider preserving the dev marker for invalid values or adding a test to lock in the desired behavior.

None => format!("{package_version} (dev)"),
}
}

/// Declare the rerun conditions for [`emit_release_build_sha`] alone: the two
/// release-CI SHA variables, and nothing about the local checkout.
///
Expand Down Expand Up @@ -154,6 +170,24 @@ fn short_sha(value: String) -> Option<String> {
mod tests {
use super::{full_sha, release_build_sha, short_sha};

#[test]

@codewhale-agent codewhale-agent Bot Sep 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[INFO] Packaged-source detection is not covered by an emit_build_version test

The new behavior depends on manifest_dir.join("Cargo.toml.orig").is_file(), but the added test only calls format_build_version directly. The path construction and emitted cargo:rustc-env lines for packaged versus unpackaged layouts are exactly the PR's behavioral change and remain unprotected by an automated unit test.

fn packaged_sources_do_not_claim_to_be_unreleased_or_stamped() {
assert_eq!(super::format_build_version("0.9.13", None, true), "0.9.13");
assert_eq!(
super::format_build_version("0.9.13", None, false),
"0.9.13 (dev)"
);
let sha = "abcdef0123456789abcdef0123456789abcdef01";
for packaged in [true, false] {
assert_eq!(
super::format_build_version("0.9.13", Some(sha), packaged),
"0.9.13 (abcdef012345)"
);
}
// Source packaging must not create a telemetry/release provenance SHA.
assert_eq!(release_build_sha(|_| None), None);
}

#[test]
fn full_commit_requires_exact_forty_hex_characters() {
assert_eq!(
Expand Down
Loading