Report the build commit in avocado --version - #186
Conversation
e566baa to
730d38b
Compare
|
Companion PR: avocado-linux/avocado-desktop#73 fixes the other consumer of this line ( Suggested order: merge the desktop one first. It accepts both the old bare |
jetm
left a comment
jetm
left a comment
There was a problem hiding this comment.
Cold second read on my own PR. Six findings inline, two of them things I would want fixed before this ships.
The first is a stale-version bug rather than a style point: the rerun-if-changed set is .git/HEAD plus the loose branch ref, so once git gc or git pack-refs packs the ref, the only watched file is one that never changes on commit. Cargo stops re-running the build script and the binary reports an old commit permanently. Reproduced in a scratch crate. The comment already names the trigger and stops at the caveat; the reflog (--git-path logs/HEAD) is the path that actually changes on every commit.
The second is that parse_reported_version reads a fixed field index out of what is actually the whole stdout blob from a login shell. A remote whose profile echoes anything shifts the index, and is_version_compatible fails open, so the older-remote guard silently disappears. For a bare-version remote the old .last() got that case right, which makes it a regression rather than an inherited weakness.
One finding is out of the diff and so not anchorable: tests/runs_on_integration.rs:475 still carries its own copy of the old .last() logic, then asserts the result contains('.'). Against the new line the token is 2026-03-05), which has no dot, so the assertion fails. It is #[ignore]d on SSH, so CI never runs it and this will surface as a confusing environment failure to whoever next runs --ignored on a box with a git-built avocado. It is also the last split_whitespace version parser left in the tree, so fixing it closes the set.
Sequencing note, since this cuts across repos: this PR is what introduces the (<sha> <date>) format, and avocado-desktop#73 is the fix for the desktop side reading it. Per the CHANGELOG finding, the exposure is wider than that one app - every already-released CLI parses with .last() and fails open on the result, so the rollout order is a real decision rather than a detail. Worth settling before either side merges. Related: #186 and desktop#73 now contain two independent parsers for the same wire format in two repos, and this one has the better edge-case coverage of the two.
Checked and deliberately not reported: empty and whitespace-only output cannot reach the parser (the is_empty() / "not-installed" bail at remote.rs:236 guards it); a v prefix fails open exactly as before and no avocado build emits one; #[command(version = env!(...))] disturbs no other consumer (every other site reads CARGO_PKG_VERSION directly); shallow and detached CI clones both produce a correct sha and date; and a dirty tree embeds HEAD's sha with no -dirty marker, which matches rustc.
A bug report citing "avocado 1.0.0-rc.1" cannot be tied to a build, and release candidates move faster than the version number does. Report the commit and its date alongside, in rustc's shape: `avocado 1.0.0-rc.1 (abc1234 2026-03-05)`. The date is the commit date, not the build date, so the string stays reproducible across rebuilds of the same commit. Watch the reflog, not just the branch ref, so the embedded sha cannot go permanently stale. `.git/HEAD` is a symref whose contents do not change on commit, and the loose branch ref that does change is deleted by `git gc`, `git pack-refs` and `git maintenance` - after which nothing watched would move again, cargo would never re-run this script, and the binary would report an older commit indefinitely without self-healing. `packed-refs` is no substitute; its mtime does not change on commit either. `logs/HEAD` is appended to on every commit and is never packed away. Reproduced in a scratch crate: with refs packed, the old watch set reported the same sha across three commits; the new one tracks each. Omit the build detail when git answers from a repository that is not this crate's. `git` runs with the build script's cwd and will answer from whatever repository encloses it, so a vendored copy inside another repo, or a source tree under a version-controlled Yocto WORKDIR, would embed that repository's commit - and the string looks authoritative enough that nobody questions it. Executed rather than assumed: a crate unpacked inside an unrelated repo embedded that repo's sha, and now reports the bare version instead. Parse the remote's reported version by anchoring on the line the CLI prints rather than by field index. The parsed input is the whole stdout blob, and the remote command sources ~/.profile and ~/.bashrc first, so a login banner shifts every index: for "Welcome to the board!\navocado 1.0.0-rc.1 (...)", the second field is `to`. That is a regression rather than a carried-forward weakness, because the `.last()` this replaces handled a banner in front of a bare version correctly. Falling back to the first version-shaped token restores that case. Correct the detached-HEAD comment, which had it backwards. `--symbolic-full-name HEAD` prints `HEAD` on a detached checkout and exits 0, so the ref is not absent - it resolves straight back to `.git/HEAD` and was pushed onto the watch list twice. The duplicate was harmless; the comment was not, since a detached checkout actually works correctly (`.git/HEAD` holds the sha directly) and anyone debugging the staleness above would have ruled out that path for the wrong reason. Pin the format in the integration test instead of merely reading it. `version_field_is_the_bare_crate_version` passed for the older `<version> <sha>` shape too, so nothing caught a revert to the one shape where a `.last()` parser reads a sha as the version; it now requires any build detail to be parenthesized. And the build-detail test returned silently when git was unavailable, turning the whole format assertion into a no-op that reported success on a runner without git - the one environment where a silent pass is least likely to be noticed. It now asserts the bare form on that branch instead of skipping. Record the rollout exposure in the changelog rather than leaving it implicit. Already-released binaries parse with `.last()` and cannot be fixed retroactively, so during the window where an older local CLI talks to a newer remote the check fails open. Moving the build detail to a second line would not have avoided it: the shipped parser splits the entire blob, not the first line. avocado-desktop is unaffected - its parser already reads the first line's second field. 1119 lib tests plus the integration suites pass, fmt and clippy clean.
730d38b to
3e9f3a0
Compare
1 participant
Problem
avocado --versionreports onlyavocado 1.0.0-rc.1, which does not identify which build a user is running. Release candidates move faster than the version number, so one string covers many different binaries and a bug report cannot be tied to a commit.build.rsalready computed a short SHA intoAVOCADO_CLI_VERSION, but nothing consumed it: the CLI declared a bare#[command(version)], which clap fills fromCARGO_PKG_VERSION.Solution
Report the commit in rustc's shape:
Keeping the version in its own field leaves it a bare semver, so version comparisons need no special handling for an appended commit. The date is the commit date, not the build date, so rebuilding the same commit yields the same string.
This does move the version out of the last field, which two consumers were reading:
utils::remote::check_cli_version(this repo) — fixed here.parse_cli_version— fixed in avocado-linux/avocado-desktop#73. That one accepts both the old and new shapes, so it is safe to merge first and is correct against today's released CLI too.Key changes
main.rs:#[command(version = env!("AVOCADO_CLI_VERSION"))], so the valuebuild.rscomputes actually reaches--version.build.rsemits<version> (<short-sha> <commit-date>), and is hardened: it unwrapped the git invocation, so building outside a checkout (source tarball, vendored crate) aborted the build instead of falling back to the bare version; it never trimmed the trailing newline offrev-parse, which would have put a line break inside the version string; and it declared norerun-if-changed, so cargo cached the script output and kept reporting a stale commit. The rerun hint follows the branch ref, notHEADalone, becauseHEADis a symref whose contents do not change on commit.check_cli_versionreads the version field via aparse_reported_versionhelper instead ofsplit_whitespace().last(), which now yields the commit date. This matters beyond cosmetics: an unparseable version makes that check fail open and silently accept a remote running an older CLI. The helper still tolerates a bare version from an older remote.tests/version.rs: the version field is a bare semver equal to the crate version, and the line carries the current commit and date (skips outside a git checkout).parse_reported_versionacross the new shape, the old bare shape, and a version with no prefix.Reviewer notes
avocado 1.0.0-rc.1 (e566baa 2026-07-31); committing without touchingbuild.rsrebuilds and reports the new commit, which is the check that the stale-commit fix actually works.cargo fmt,cargo clippy --all-targets --all-features -- -D warnings, and the fullcargo testsuite are clean.actions/checkout@v5leaves.gitpresent, so release and PR builds resolve a commit;crossmusl builds and the Windowscargo checkfall back to the bare version if git is unavailable rather than failing.env!("TARGET")from the same build script is unchanged (still used byavocado upgrade)..dirtymarker for uncommitted builds was deliberately left out to keep this focused; easy follow-up if wanted.