Add configurable cache bind for chroot

[benhoff]

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.

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• Any dependent changes have been merged and published in downstream modules

Summary by CodeRabbit

• Chores
  • Build environment now tracks mounts and unmounts in reverse order, with guarded per-path mounting and earlier creation of runtime directories.
  • Host cache bind-mounting added (driven by ARMBIAN_CACHE_DIR), skipping if absent and warning on failures.
• Bug Fixes
  • Improved error handling to reduce failed mounts and ensure reliable cleanup.
• Documentation
  • Added config docs explaining the config directory and ARMBIAN_CACHE_DIR behavior.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds conditional host-cache bind-mounting to chroot helpers: mount_chroot now creates and bind-mounts the host ARMBIAN_CACHE_DIR into target/armbian/cache with guarded mount checks and cleanup on failure; umount_chroot now unmounts that cache path before the existing unmount sequence. No public signatures changed.

Changes

Cohort / File(s)	Summary
Chroot helpers lib/functions/general/chroot-helpers.sh	Added per-path mount checks, ordered mount tracking and cleanup, creation of target/run/user/0, conditional host-cache bind-mount logic driven by ARMBIAN_CACHE_DIR (create target/armbian/cache, bind if host cache exists), and corresponding unmount handling for target/armbian/cache.
Documentation config/README.md	Documented the Configuration Directory and the ARMBIAN_CACHE_DIR environment variable, explained default cache path and host-cache bind into the chroot.

Sequence Diagram(s)

sequenceDiagram
  participant Host
  participant Script as mount_chroot / umount_chroot
  participant TargetFS as target (chroot)

  Host->>Script: optional ARMBIAN_CACHE_DIR env
  Script->>Host: check host cache exists
  alt host cache exists
    Script->>TargetFS: ensure target/armbian/cache directory exists
    Script->>TargetFS: mount --bind host_cache -> target/armbian/cache
    Note right of Script: record mount in stack for cleanup
  else host cache missing
    Script->>Host: log warning / skip bind
  end

  Script->>TargetFS: perform other guarded mounts (tmp,var,tmp,run/user/0,proc,sys,dev,dev/pts)
  Note right of Script: on error -> unmount recorded mounts in reverse order

  Script->>TargetFS: umount target/armbian/cache (if recorded/mounted)
  Script->>TargetFS: continue existing unmount sequence

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Focus review on mountpoint checks, mount-tracking stack and reverse-unmount ordering, error/log paths, and creation permissions for target/armbian/cache and target/run/user/0.

Possibly related PRs

• Extend chroot mount helper to include cache directories #8186 — modifies the same mount_chroot / umount_chroot logic to add host-cache bind-mount handling (direct code-level overlap).

Suggested reviewers

• igorpecovnik

Poem

🐇 I dug a tiny tunnel, neat and quick,
A cache to share, a gentle, handy trick,
I bind, I guard, then tidy up the lot,
Soft paws leave no mess — the build is taut. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Add configurable cache bind for chroot' directly and clearly summarizes the main changes: introducing configurable cache bind functionality for chroot operations.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between a51e9fd and 87db834.

📒 Files selected for processing (1)

• lib/functions/general/chroot-helpers.sh (2 hunks)

🧰 Additional context used

🧠 Learnings (3)

📚 Learning: 2025-10-24T04:46:22.901Z

Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-10-24T04:46:22.901Z
Learning: In lib/functions/rootfs/rootfs-create.sh, the FIXME comment about mmdebstrap usage with --aptopt is a future note related to PR #8785, which hasn't been merged yet.

Applied to files:

• lib/functions/general/chroot-helpers.sh

📚 Learning: 2025-06-12T21:03:39.686Z

Learnt from: djurny
Repo: armbian/build PR: 8287
File: config/bootscripts/boot-sunxi.cmd:148-161
Timestamp: 2025-06-12T21:03:39.686Z
Learning: In Armbian U-Boot bootscripts, the `${cpu}` environment variable is always pre-set by U-Boot (or defaults to a valid value), so additional guards against it being unset are unnecessary.

Applied to files:

• lib/functions/general/chroot-helpers.sh

📚 Learning: 2025-07-21T04:12:02.439Z

Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-07-21T04:12:02.439Z
Learning: In the Armbian build system, for recurring maintenance tasks like kernel version bumping, TODO comments should use generic version formats (e.g., "MAJOR.MINOR-rc1") rather than specific version numbers (e.g., "6.17-rc1") to avoid the need for frequent comment updates that would create unnecessary maintenance overhead.

Applied to files:

• lib/functions/general/chroot-helpers.sh

🔇 Additional comments (6)

lib/functions/general/chroot-helpers.sh (6)

19-26: Mount tracking and reverse-order cleanup is well-structured.

The mounted_points array and cleanup_mounted_points function correctly implement reverse-order unwinding for mounted filesystems on failure. This is the right approach for robust error handling.

---

27-30: Early directory creation for run/user/0 with explicit error handling is sound.

Creating the target directory before mounting and returning on failure prevents confusing "mount failed" messages when the root cause is a missing directory. This is good defensive coding.

---

33-88: Consistent mount guards with tracking: pattern is correct.

Each mount (tmp, var/tmp, run/user/0, proc, sys, dev, dev/pts) follows the same robust pattern: check if already mounted, attempt mount, add to array for cleanup, or fail early. The guards prevent remounting and make cleanup deterministic.

---

90-104: Cache mount logic handles errors gracefully but introduces a cleanup gap.

The nested if/elif/else structure correctly distinguishes three cases (mkdir failure, already mounted, mount attempt) and does not fail the function if cache binding fails—appropriate for optional functionality.

However, the cache mount is not tracked in the mounted_points array. If the cache bind succeeds but a subsequent required mount (e.g., /proc on line 57) fails, cleanup_mounted_points will not unmount the cache. The cache will only be unmounted later by umount_chroot (lines 116–118), creating a window where the cache remains mounted after an intermediate failure.

This may be intentional (cache is optional and best-effort), but it creates an implicit assumption that the caller always invokes umount_chroot after mount_chroot fails. Verify whether this is the intended design.

Does the cache mount belong in the mounted_points array so it is cleaned up alongside other failed mounts, or is the current best-effort behavior deliberate? Please clarify the intended cleanup semantics, especially if mount_chroot can fail and umount_chroot might not always be called.

---

116-118: Cache unmount is placed correctly and handles errors safely.

Unmounting the cache before the standard unmount sequence and using || true to tolerate transient failures is the right approach. However, this unmount is separate from the mounted_points cleanup, which is noted above.

---

1-133: Verify cleanup behavior on mount failure.

The refactoring significantly improves error handling for mount operations, but testing the cleanup path (especially after cache-mount success followed by a required-mount failure) is critical. Since the author indicates that testing has not yet been completed and they lack bare-metal hardware, please ensure this scenario is validated, either with simulation or by a reviewer with hardware access.

• Test a scenario where mount_chroot succeeds in binding the cache but fails on a required mount (e.g., /proc) afterward. Verify that cleanup_mounted_points correctly unmounts the already-mounted filesystems and that no orphaned mounts remain.
• Confirm that when mount_chroot fails and returns 1, the subsequent call to umount_chroot fully cleans up (including the cache, if mounted).

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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_DIR is 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 /dev bind mounts have no checks and will fail silently if permissions are denied or the target is read-only.

1. Add ARMBIAN_CACHE_DIR to documentation (config/README.md or similar) with the default value and use case.
2. 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.

📥 Commits

Reviewing files that changed from the base of the PR and between d5bceb9 and dc6f301.

📒 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_chroot function 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_chroot with umount_chroot
• Error handling with || true allows partial failures without stopping cleanup
• The implementation maintains proper mount/unmount semantics

[coderabbitai]

⚠️ Potential issue | 🟠 Major

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.

Suggested change

	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.

[leggewie]

ping @benhoff

[coderabbitai]

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!