artifact-rootfs: surface git-log failure when computing configng hash#9849
artifact-rootfs: surface git-log failure when computing configng hash#9849igorpecovnik wants to merge 2 commits into
Conversation
igorpecovnik
requested review from
ArkadiuszRaj and
martinayotte
and removed request for
a team
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe pull request improves error handling and scope documentation for desktop-specific cache invalidation. It refines the CONFIGNG_DESKTOPS_HASH computation to emit warnings on git failures instead of silently suppressing errors, and explicitly clarifies in comments that this fingerprinting applies only to desktop builds. ChangesDesktop hash error handling and scope clarity
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/functions/artifacts/artifact-rootfs.sh`:
- Around line 30-32: The code masks failures from "git -C \"${configng_dir}\"
log -1 --format=%H" by appending "|| echo \"unknown\"", which hides real errors;
change the logic around configng_hash so that failures from git log are not
suppressed—either remove the "|| echo \"unknown\"" so the command fails fast
under set -o errexit, or explicitly capture the git output and check the exit
status and call error/exit if git returns non-zero. Update the block that sets
configng_hash (referencing configng_dir and configng_hash) to propagate git
errors instead of silencing them, and add a clear error log if you choose the
explicit-check approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9678335b-6eed-4b0a-a752-ebf649bdb73b
📒 Files selected for processing (3)
lib/functions/artifacts/artifact-rootfs.shlib/functions/configuration/config-desktop.shlib/functions/rootfs/create-cache.sh
| if [[ -d "${configng_dir}/.git" ]]; then | ||
| configng_hash="$(git -C "${configng_dir}" log -1 --format=%H 2>/dev/null || echo "unknown")" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file and surrounding context
head -50 lib/functions/artifacts/artifact-rootfs.sh | cat -nRepository: armbian/build
Length of output: 2681
🏁 Script executed:
# Check if the file exists and see full context around lines 30-32
wc -l lib/functions/artifacts/artifact-rootfs.shRepository: armbian/build
Length of output: 104
🏁 Script executed:
# Read the specific section with more context
sed -n '1,50p' lib/functions/artifacts/artifact-rootfs.sh | cat -nRepository: armbian/build
Length of output: 2681
🏁 Script executed:
# Search for how configng_hash is used in the file
rg "configng_hash" lib/functions/artifacts/artifact-rootfs.sh -A 3 -B 1Repository: armbian/build
Length of output: 506
🏁 Script executed:
# Check if the script uses set -e
rg "set -e" lib/functions/artifacts/artifact-rootfs.shRepository: armbian/build
Length of output: 39
🏁 Script executed:
# Check if exit_with_error function exists
rg "exit_with_error" . --type sh | head -20Repository: armbian/build
Length of output: 2817
🏁 Script executed:
# Search for where artifact-rootfs.sh is sourced
rg "artifact-rootfs" . --type shRepository: armbian/build
Length of output: 615
🏁 Script executed:
# Check main build script or entrypoint for set -e
rg "set -e" . --type sh | head -20Repository: armbian/build
Length of output: 2073
🏁 Script executed:
# Search for how CONFIGNG_HASH is used in cache calculations
rg "CONFIGNG_HASH" . --type sh -B 2 -A 2Repository: armbian/build
Length of output: 2370
🏁 Script executed:
# Check git log failure scenarios
rg "git.*log" lib/functions/artifacts/ --type sh -B 2 -A 2Repository: armbian/build
Length of output: 987
🏁 Script executed:
# Check how CONFIGNG_HASH is used in the cache filename/calculation
rg -A 10 "_configng_fp=" lib/functions/rootfs/create-cache.shRepository: armbian/build
Length of output: 508
🏁 Script executed:
# Check if there's any special handling for "unknown" values
rg "unknown" lib/functions/rootfs/create-cache.shRepository: armbian/build
Length of output: 103
🏁 Script executed:
# Understand the full cache ID calculation
rg "cache.*id\|rootfs_cache_id" lib/functions/rootfs/create-cache.sh -B 2 -A 2 | head -50Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Find exit_with_error definition
rg "^function exit_with_error|^exit_with_error\(\)" lib/ -A 5Repository: armbian/build
Length of output: 461
🏁 Script executed:
# Check if exit_with_error is sourced before artifact-rootfs.sh
head -100 lib/library-functions.sh | grep -E "exit_with_error|artifact-rootfs"Repository: armbian/build
Length of output: 226
Fail fast when git log fails in an existing armbian-configng clone.
Line 31 masks git log failure with || echo "unknown". This violates fail-fast expectations given set -o errexit is enabled in the sourcing context (lib/library-functions.sh). If git log fails—due to repo corruption, permission issues, or other problems—the build continues silently instead of surfacing the error. While the cache layer explicitly excludes "unknown" from the fingerprint calculation, masking the failure hides a genuine error condition that should halt the build.
Suggested fix
if [[ -d "${configng_dir}/.git" ]]; then
- configng_hash="$(git -C "${configng_dir}" log -1 --format=%H 2>/dev/null || echo "unknown")"
+ configng_hash="$(git -C "${configng_dir}" log -1 --format=%H 2>/dev/null)" \
+ || exit_with_error "Failed to resolve CONFIGNG_HASH from ${configng_dir}"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ -d "${configng_dir}/.git" ]]; then | |
| configng_hash="$(git -C "${configng_dir}" log -1 --format=%H 2>/dev/null || echo "unknown")" | |
| fi | |
| if [[ -d "${configng_dir}/.git" ]]; then | |
| configng_hash="$(git -C "${configng_dir}" log -1 --format=%H 2>/dev/null)" \ | |
| || exit_with_error "Failed to resolve CONFIGNG_HASH from ${configng_dir}" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/functions/artifacts/artifact-rootfs.sh` around lines 30 - 32, The code
masks failures from "git -C \"${configng_dir}\" log -1 --format=%H" by appending
"|| echo \"unknown\"", which hides real errors; change the logic around
configng_hash so that failures from git log are not suppressed—either remove the
"|| echo \"unknown\"" so the command fails fast under set -o errexit, or
explicitly capture the git output and check the exit status and call error/exit
if git returns non-zero. Update the block that sets configng_hash (referencing
configng_dir and configng_hash) to propagate git errors instead of silencing
them, and add a clear error log if you choose the explicit-check approach.
a3bc971 to
019c0f4
Compare
github-actions
Bot
added
size/small
igorpecovnik
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/functions/artifacts/artifact-rootfs.sh`:
- Line 27: The variable name still uses
CONFIGNG_DESKTOPS_HASH/configng_desktops_hash but should be renamed to
CONFIGNG_HASH/configng_hash everywhere; update the declaration (declare
configng_hash="undetermined"), all subsequent references and assignments (e.g.,
places currently referencing configng_desktops_hash or CONFIGNG_DESKTOPS_HASH),
and any export or string interpolation to use CONFIGNG_HASH/configng_hash so the
hash applies to all build types rather than only "desktops".
- Around line 26-50: The BUILD_DESKTOP conditional is still preventing configng
cache-busting for non-desktop builds; remove the surrounding if [[
"${BUILD_DESKTOP}" == "yes" ]] check so the configng_desktops_hash logic always
runs: always declare and compute configng_desktops_hash (using configng_dir and
the git -C ... log command with git_out/git_rc), call display_alert on failure,
set artifact_input_variables[CONFIGNG_DESKTOPS_HASH] to the resulting hash (or
"unknown"/"undetermined" as the existing logic does), and ensure no
BUILD_DESKTOP gating remains around those statements.
- Line 40: The git invocation in artifact-rootfs.sh still uses a path filter
("tools/modules/desktops/") so the commit hash in git_out only reflects changes
to that path; remove the path filter so git computes the last commit for the
whole repo by changing the git command that sets git_out (the git -C
"${configng_dir}" log -1 --format=%H ... call) to not include the path argument,
leaving only git -C "${configng_dir}" log -1 --format=%H 2>&1; keep the same
variable (git_out) and error handling around that command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 313a13e2-e0ea-46a5-8eda-0eaec64cfa52
📒 Files selected for processing (2)
lib/functions/artifacts/artifact-rootfs.shlib/functions/rootfs/create-cache.sh
✅ Files skipped from review due to trivial changes (1)
- lib/functions/rootfs/create-cache.sh
| # build still produces a valid image, it just doesn't get | ||
| # the configng-aware cache bust. | ||
| declare git_out git_rc | ||
| git_out="$(git -C "${configng_dir}" log -1 --format=%H -- tools/modules/desktops/ 2>&1)" |
There was a problem hiding this comment.
Path filter -- tools/modules/desktops/ still present in git log.
The PR objectives state the fix should "remove the path filter from git log -1" so that "any commit" in armbian-configng invalidates the cache, but line 40 still restricts the hash to commits touching tools/modules/desktops/. This means commits to CLI helpers, system modules, postinst scripts, or the parser will not invalidate the cache—the exact bug the PR claims to fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/functions/artifacts/artifact-rootfs.sh` at line 40, The git invocation in
artifact-rootfs.sh still uses a path filter ("tools/modules/desktops/") so the
commit hash in git_out only reflects changes to that path; remove the path
filter so git computes the last commit for the whole repo by changing the git
command that sets git_out (the git -C "${configng_dir}" log -1 --format=%H ...
call) to not include the path argument, leaving only git -C "${configng_dir}"
log -1 --format=%H 2>&1; keep the same variable (git_out) and error handling
around that command.
|
Reviewed the current revision. Two issues with the new explicit-capture block; the intent of the PR is right but the implementation regresses the failure path it is meant to fix. 1.
|
Original code masked git failures with `|| echo "unknown"` - the build silently fell through to a degraded cache key without telling the operator anything. A torn cache/sources/armbian-configng clone, broken HEAD, or permissions glitch was indistinguishable from a healthy "couldn't compute" state. Capture stdout+stderr from git log, branch on exit status, and display_alert the git error text when rc != 0. Still falls through to "unknown" so the build completes - this is a best-effort caching knob, not a build-blocker, and the downstream check in create-cache.sh already skips the fingerprint fold on "unknown". Scope kept exactly as before: BUILD_DESKTOP=yes only, path-filtered to tools/modules/desktops/. CLI builds don't invoke armbian-config at build time and configng content outside the desktop subtree doesn't reach the rootfs from a build-time path, so broadening the filter would only churn cache for no benefit. Comment touch-up in create-cache.sh documents that scoping for future readers.
igorpecovnik
force-pushed
the
rootfs-cache-invalidate-on-configng-change
branch
from
019c0f4 to
60cde77
Compare
Address review on #9849: 1. Under compile.sh's `set -e`, a bare `git_out=$(git log ...)` aborts the build on the assignment itself when git fails — `git_rc=$?` and the warn/"unknown" fallback were never reached, so a torn checkout / broken HEAD / permission error hard-aborted with an opaque "Error 1" instead of degrading the cache key. Run git inside an `if cmd; then` condition, which errexit does not trip. 2. `git log` can exit 0 yet still emit stderr (hint:/advice:/fsmonitor/locale), and `2>&1` concatenated that onto the SHA — create-cache.sh folds in the first 8 chars, so polluted keys could collide or carry junk. Send stderr to a temp file instead; the SHA stays pure, and the failure path still surfaces git's message in the warn. mktemp is guarded (falls back to /dev/null) so it can't reintroduce an errexit abort, and the temp file is cleaned up. Signed-off-by: Igor Pecovnik <igor@armbian.com>
2 participants
Scope kept exactly as before — path filter on `tools/modules/desktops/` and `BUILD_DESKTOP=yes` gate both stay. Only behaviour change is visibility on git failure.
Before
```bash
configng_desktops_hash="$(git -C "${configng_dir}" log -1 --format=%H -- tools/modules/desktops/ 2>/dev/null || echo "unknown")"
```
Failure modes (torn checkout, broken HEAD, permissions, stale clone) silently swallowed; operator gets the degraded cache key with no log line explaining why.
After
```bash
git_out="$(git -C "${configng_dir}" log -1 --format=%H -- tools/modules/desktops/ 2>&1)"
git_rc=$?
if (( git_rc == 0 )); then
configng_desktops_hash="${git_out}"
else
display_alert "configng_desktops hash: git log failed (rc=${git_rc}) in ${configng_dir}" "${git_out}" "warn"
configng_desktops_hash="unknown"
fi
```
Falls through to the same "unknown" sentinel, so the build still completes — this is a best-effort caching input, not a build-blocker, and the downstream check in `create-cache.sh` already skips the fingerprint fold on "unknown" / "undetermined". An earlier rev of this PR broadened the scope to all builds + the entire repo; reverted after discussion — CLI builds don't invoke armbian-config at build time so configng content outside the desktop subtree doesn't reach the rootfs from a build-time path.
Also picked up a comment touch-up in `create-cache.sh` documenting the BUILD_DESKTOP scoping for future readers.
Test plan
Summary by CodeRabbit
Bug Fixes
gitdata can’t be determined and falling back to an “unknown” value rather than failing silently.Documentation