fix(build): stamp-file dependency for version.o REVISION staleness - #1386
fix(build): stamp-file dependency for version.o REVISION staleness#1386nerdCopter wants to merge 7 commits into
Conversation
PR#1238 (ae9a736) reverted the FORCE dependency on version.o back to $(SRC) for BF-parity no-op incremental builds, reintroducing the exact bug PR#1125 (e8d439d) had fixed: a dirty-tree build embeds REVISION=uncommitted_<timestamp> into version.o, and a subsequent clean-tree build with no source-file changes leaves that stale object untouched, so the firmware keeps reporting "uncommitted" even though HEAD is clean and REVISION now resolves to the real commit hash. Add REVISION_STAMP, a per-target file whose content is the current REVISION value. Its mtime only advances when the value actually changes. version.o depends on both $(SRC) and REVISION_STAMP, so: - same REVISION, no source change -> stamp untouched -> no rebuild (preserves the no-op incremental-build behavior PR#1238 wanted) - REVISION changes (dirty->clean transition, or a new commit landing with no other source diff) -> stamp rewritten -> version.o rebuilds with the correct value Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
AI generated comment Not requesting a full review — asking for analysis on three specific points before this goes further:
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (13)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🪛 checkmake (0.3.2)Makefile[warning] 314-314: Target "$(OBJECT_DIR)/$(TARGET)/build/version.o" should be declared PHONY. (phonydeclared) 🔇 Additional comments (1)
📝 WalkthroughWalkthroughThe Makefile adds a revision stamp to the ChangesRevision stamp build tracking
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change keeps version.o synchronized with the current revision while preserving no-op incremental builds; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
makefile=$(fd -t f -a '^Makefile$' . | head -n 1)
if [ -z "$makefile" ]; then
echo "Makefile not found" >&2
exit 1
fi
echo "=== Makefile outline: revision and version-related lines ==="
rg -n -C 5 'REVISION|version\.o|FORCE|mkdir -p|git diff|shortstat|date|grep|awk|\$\(\s*shell' "$makefile"
echo
echo "=== Current target section (lines 260-340) ==="
sed -n '260,340p' "$makefile"
echo
echo "=== Commit e8d439d26 ==="
git show --format=fuller --find-renames --stat e8d439d26
git show --format= --find-renames e8d439d26 -- "$makefile"
echo
echo "=== Commit ae9a736d8 ==="
git show --format=fuller --find-renames --stat ae9a736d8
git show --format= --find-renames ae9a736d8 -- "$makefile"
echo
echo "=== Pull request metadata for `#1125` and `#1238` ==="
gh pr view 1125 --repo emuflight/EmuFlight --json number,title,state,mergedAt,body,commits,files
gh pr view 1238 --repo emuflight/EmuFlight --json number,title,state,mergedAt,body,commits,filesLength of output: 31506 🏁 Script executed: #!/bin/bash
set -euo pipefail
makefile=$(fd -t f -a '^Makefile$' . | head -n 1)
echo "=== Shell declarations and POSIX-shell recipe constructs ==="
rg -n -C 3 '^[[:space:]]*(SHELL|\.SHELLFLAGS)[[:space:]]*[:?+]?=|(^|[^$])\[\s|&&|;[[:space:]]*\\$|trap |mktemp |for [A-Za-z_].* in |if \[' "$makefile"
echo
echo "=== Build, dependency-include, and cleanup rules ==="
rg -n -C 5 'include .*TARGET_DEPS|TARGET_DEPS|CLEAN_ARTIFACTS|^clean:|^clean_all:|build/version\.o|/%.o:' "$makefile"
echo
echo "=== Revision stamp path relative to target directory prerequisites ==="
rg -n -C 4 'TARGET_DIRS|REVISION_STAMP|\.SECONDEXPANSION|dir \$\$@' "$makefile"
echo
echo "=== GNU Make equal-mtime prerequisite experiment (temporary directory only) ==="
tmpdir=$(mktemp -d)
trap 'rm -rf "$tmpdir"' EXIT
cat > "$tmpdir/Makefile" <<'EOF'
out: stamp
`@echo` rebuilt > out
EOF
printf 'old\n' > "$tmpdir/out"
printf 'new\n' > "$tmpdir/stamp"
touch -t 202601010101 "$tmpdir/out" "$tmpdir/stamp"
echo "mtimes:"
stat -c '%n %Y' "$tmpdir/out" "$tmpdir/stamp"
echo "make result:"
make -C "$tmpdir" --no-print-directory out
printf 'out content: '
cat "$tmpdir/out"Length of output: 15010 🏁 Script executed: #!/bin/bash
set -euo pipefail
tmpdir=$(mktemp -d)
trap 'rm -rf "$tmpdir"' EXIT
cat > "$tmpdir/Makefile" <<'EOF'
out: stamp
`@printf` 'rebuilt\n' > out
EOF
printf 'old\n' > "$tmpdir/out"
printf 'new\n' > "$tmpdir/stamp"
# Deliberately assign the same coarse (one-second) mtime to both files.
touch -t 202601010101 "$tmpdir/out" "$tmpdir/stamp"
echo "=== GNU Make version ==="
make --version | head -n 1
echo "=== Build result with equal target/prerequisite mtimes ==="
make -C "$tmpdir" --no-print-directory out
printf 'out content: '
cat "$tmpdir/out"Length of output: 929
The dependency model is correct for the reported staleness path.
This covers these value changes without a
The existing The implementation is minimal in the dependency-graph sense. It adds one content-addressed freshness input. It does not restore the unconditional There is one strict qualification to “every case.” GNU Make compares mtimes. On a filesystem with coarse timestamp resolution, a rewritten stamp can receive the same mtime as an existing I could not run the focused equal-mtime Make experiment because the sandbox does not provide
The approach fits the intent of both prior changes.
When When One minor architectural concern exists. The parse-time
The stamp logic does not introduce a new practical shell-environment requirement. The Makefile already assumes a POSIX-compatible shell and userland tools:
Therefore, native Windows GNU Make with Conclusion: The stamp is the right minimal fix for the stated regression and preserves You are interacting with an AI system. |
The parse-time $(shell mkdir -p ...) and stamp-write ran on every Make invocation, including non-build goals like 'make help', not just when version.o was actually being built. Move the mkdir/stamp-compare logic into a proper recipe rule (version.o's REVISION_STAMP prerequisite, via a phony check_revision_stamp trigger) so it only runs when REVISION_STAMP is actually pulled into the build, matching this Makefile's existing target-based rule architecture. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@coderabbitai, AI generated comment Addressed the architectural nit from the analysis above in 7da81fa: Moved the REVISION_STAMP mkdir/compare logic out of parse-time The coarse-mtime-filesystem caveat noted in the analysis is accepted as-is — not applicable on the |
|
Tip For best results, initiate chat on the files or code changes.
The A/B/C/D results cover the original stale- ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
AI Generated pull-request
Summary
version.o's build rule only depends on$(SRC), not on theREVISIONvalue itself. Adirty-tree build (
REVISION=uncommitted_<timestamp>) followed by a clean-tree build with nosource-file changes leaves that stale object untouched by Make's own dependency graph, so the
firmware keeps reporting
uncommittedin CLIversioneven thoughHEADis clean andREVISIONnow resolves to the real 7-char commit hash.This is a regression: #1125 (
e8d439d26) originally fixed this exact bug by makingversion.odepend on a
FORCEphony target. #1238 (ae9a736d8) reverted that back to$(SRC), explicitlyto restore no-op incremental-build behavior for parallel builds — a legitimate goal, but it
reintroduced the staleness bug because
$(SRC)cannot express "REVISION's value changed."Fix: add
REVISION_STAMP, a per-target file containing the currentREVISIONstring.version.onow depends on both
$(SRC)andREVISION_STAMP, so:REVISION, no source change → stamp untouched → no rebuild (preserves fix(make): eliminate mkdir races in parallel builds (-jN) #1238's no-opincremental-build behavior)
REVISIONchanges (dirty→clean transition, or a new commit landing with no other source diff)→ stamp rewritten →
version.orebuilds with the correct valueREVISION_STAMP's mkdir/compare logic is an explicit recipe rule (check_revision_stampphonytrigger), not a parse-time
$(shell ...)call, so it only runs when actually pulled into a buildgoal — non-build goals like
make helpnever touch it.Test plan
$(SRC)file → rebuild →
uncommitted_<timestamp>embedded; revert to clean, zero further changes →rebuild →
version.ocorrectly recompiles with the real SHA (this transition was previouslysilently skipped)
version.omtimeunchanged, no unnecessary recompile
make helpleaves.revision_stampabsent(7da81fa)— thereal commit hash, not
uncommitted— in CLIversionSummary by CodeRabbit