Add configurable cache bind for chroot #8973
Conversation
WalkthroughThe PR adds host cache bind-mount handling to chroot helper functions: Changes
Sequence Diagram(s)sequenceDiagram
participant Host
participant Script as mount_chroot/umount_chroot
participant TargetFS as target (chroot)
rect rgba(173,216,230,0.15)
Host->>Script: provide ARMBIAN_CACHE_DIR (optional)
Script->>Host: check & create host cache dir (if needed)
end
rect rgba(144,238,144,0.12)
Script->>TargetFS: ensure target/armbian/cache exists
Script->>TargetFS: mount --bind host_cache -> target/armbian/cache (if not already mountpoint)
Note right of TargetFS: Warnings logged on create/bind failures
end
rect rgba(255,228,196,0.12)
Script->>TargetFS: umount target/armbian/cache (if mounted)
Script->>TargetFS: continue existing umount sequence
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (10)📚 Learning: 2025-07-17T04:12:33.125ZApplied to files:
📚 Learning: 2025-09-27T21:50:04.845ZApplied to files:
📚 Learning: 2025-09-27T21:49:55.796ZApplied to files:
📚 Learning: 2025-09-14T11:37:35.089ZApplied to files:
📚 Learning: 2025-07-23T07:30:52.265ZApplied to files:
📚 Learning: 2025-09-27T21:47:58.020ZApplied to files:
📚 Learning: 2025-08-02T05:46:10.664ZApplied to files:
📚 Learning: 2025-08-03T15:21:20.148ZApplied to files:
📚 Learning: 2025-05-16T15:34:34.672ZApplied to files:
📚 Learning: 2025-06-14T05:53:10.627ZApplied to files:
🔇 Additional comments (1)
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
11
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/functions/general/chroot-helpers.sh (1)
1-41: Document ARMBIAN_CACHE_DIR and add error handling for filesystem mounts.The variable
ARMBIAN_CACHE_DIRis not documented anywhere in the codebase—it only appears in the implementation. The cache mount itself is well-implemented (checks directory existence, validates already-bound mount points, logs warnings), but the early mounts lack error handling: tmpfs, proc, sysfs, and/devbind mounts have no checks and will fail silently if permissions are denied or the target is read-only.
- Add ARMBIAN_CACHE_DIR to documentation (config/README.md or similar) with the default value and use case.
- Add error handling to the early mounts in
mount_chroot(lines 21–27), since they're prerequisite operations that determine success or failure of the entire chroot setup.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/functions/general/chroot-helpers.sh(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/functions/general/chroot-helpers.sh (2)
lib/functions/image/initrd.sh (1)
update_initramfs(23-133)lib/functions/rootfs/rootfs-create.sh (1)
create_new_rootfs_cache_via_debootstrap(54-365)
🔇 Additional comments (1)
lib/functions/general/chroot-helpers.sh (1)
49-56: Cache unmount ordering is correct; no issues identified.Verification of call sites and the
umount_chrootfunction confirms:
- Cache unmount (lines 54–56) correctly executes before the proc/sys/dev/tmp/run cleanup loop (lines 58–69)
- All call sites properly pair
mount_chrootwithumount_chroot- Error handling with
|| trueallows partial failures without stopping cleanup- The implementation maintains proper mount/unmount semantics
| cache_src="${ARMBIAN_CACHE_DIR:-/armbian/cache}" | ||
| if mountpoint -q "${target}/armbian/cache"; then | ||
| umount "${target}/armbian/cache" || true | ||
| fi |
There was a problem hiding this comment.
Remove unused variable assignment.
Line 53 declares and assigns cache_src, but it is never referenced. The unmount logic on lines 54–56 works correctly without it.
Apply this diff to remove the unused variable:
- cache_src="${ARMBIAN_CACHE_DIR:-/armbian/cache}"
- if mountpoint -q "${target}/armbian/cache"; then
+ if mountpoint -q "${target}/armbian/cache"; then📝 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.
| cache_src="${ARMBIAN_CACHE_DIR:-/armbian/cache}" | |
| if mountpoint -q "${target}/armbian/cache"; then | |
| umount "${target}/armbian/cache" || true | |
| fi | |
| if mountpoint -q "${target}/armbian/cache"; then | |
| umount "${target}/armbian/cache" || true | |
| fi |
🤖 Prompt for AI Agents
lib/functions/general/chroot-helpers.sh around lines 53 to 56: remove the unused
variable assignment "cache_src=\"${ARMBIAN_CACHE_DIR:-/armbian/cache}\"" on line
53 since it is never referenced; leave the subsequent mountpoint check and
umount logic intact and ensure no other code in the file expects that variable
before committing the change.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
igorpecovnik
added
02
benhoff
requested review from
EvilOlaf,
TRSx80,
igorpecovnik and
littlecxm
as code owners
github-actions
bot
added
size/medium
3 participants
Description
This reimplements this pull request, while fixing the edge case (hopefully). #8186
I don't have a bare metal device to test.
How Has This Been Tested?
I haven't checked this yet, I'll run it with a Qt buildout that I have
Checklist:
Please delete options that are not relevant.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.