Extend chroot mount helper to include cache directories #8186
Conversation
WalkthroughThe Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
size/small
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
lib/functions/general/chroot-helpers.sh (4)
21-22: Ensure cache mountpoint exists and guard against missing host directory.The new
mkdir -p "${target}/armbian/cache"correctly creates the mount point inside the chroot. However, if the host’s/armbian/cachedirectory doesn’t exist, the subsequent bind mount will fail. Consider adding a check or a warning before creating and mounting:+ if [[ ! -d /armbian/cache ]]; then + display_alert "Host cache directory /armbian/cache not found; skipping cache mount" "$target" "warn" + else + mkdir -p "${target}/armbian/cache" + fi
32-32: Bind-mount the host cache directory.Binding
/armbian/cacheinto the chroot is vital for sharing build and download caches. To tighten mount propagation and avoid leaking host mounts into the chroot (or vice versa), you could add a private propagation flag:- mount --bind /armbian/cache "${target}/armbian/cache" + mount --bind /armbian/cache "${target}/armbian/cache" + # Make this bind-mount private to the chroot + mount --make-rprivate "${target}/armbian/cache"
43-43: Updated unmount loop to include cache.Great catch on extending the
grep -Eqpattern to coverarmbian/cache. This ensures the cache bind-mount is recognized in the unmount loop. As a follow-up, you may want to update the downstream debug logging on line 54 to reflect all mount points:-run_host_command_logged grep -E "'${target}/(dev|proc|sys|tmp)'" /proc/mounts "||" true +run_host_command_logged grep -E "'${target}/(dev|proc|sys|tmp|var/tmp|run/user/0|armbian/cache)'" /proc/mounts "||" true
52-52: Unmount the cache directory.Including
umount "${target}/armbian/cache" || trueguarantees cleanup of the cache bind mount. You might also consider a recursive unmount to catch nested mounts and removing the now-unused directory:- umount "${target}/armbian/cache" || true + umount --recursive "${target}/armbian/cache" || true + rm -rf "${target}/armbian/cache"
benhoff
changed the title
🥲 When running without Docker:
[🌱] Cleaning up after debootstrap [ debootstrap cleanup ] |
Oh no! I didn't think about running without docker. @igorpecovnik want to revert and I'll find a fix and then resubmit? |
|
If you can check / fix this within a week, we can wait. Its corner case. |
This is from the official aembizn doc. I am using the "native" build method and it's broken currently. If that's acceptable, would you mind reverting until there is fix for "native builds"? |
|
OK, reverted until fixed. |
|
@igorpecovnik Thank you. Is there a plan to phase out the "native build" at some point in favor of "docker based" build ? |
No such plan. |
Description
This change enhances the mount_chroot() and umount_chroot() helper functions to:
Motivation & Context:
Current chroot helper does not bind the Armbian cache directory. When customizing the image, each build starts “from cold,” losing all previously downloaded sources and compiled artifacts. By adding this mounts, we:
No downstream dependencies introduced.
How Has This Been Tested?
Tested working both cache and check the image to make sure that the cache was not included in the image.
Checklist