Description

This reimplements this pull request, while fixing the edge case (hopefully). https://github.com/armbian/build/pull/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.

[x] My code follows the style guidelines of this project
[x] 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

Release Notes

Chores
- Build environment cache is now more safely mounted and unmounted during builds, with improved lifecycle handling and error warnings.
Bug Fixes
- Safer mount/unmount sequences reduce failed bind mounts and preserve runtime stability.
Documentation
- Added configuration docs describing the config directory and the ARMBIAN_CACHE_DIR host cache bind behavior and relocation guidance.

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

Nov 21 '25 19:11 benhoff

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

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

armbian/build#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 a51e9fdf2abd305963956f535ec0c15292fdfb57 and 87db834771f0b79f45a1d9205ace241ca3e27ed2.

📒 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

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

Nov 21 '25 19:11 coderabbitai[bot]

@coderabbitai

Dec 13 '25 16:12 benhoff

[!TIP] For best results, initiate chat on the files or code changes.

@benhoff Hello! How can I assist you with this PR?

I can help with:

Reviewing the code changes in detail
Suggesting improvements or identifying potential issues
Generating test cases
Answering questions about the implementation
Providing documentation suggestions
Or anything else you need

Just let me know what you'd like me to do!

Dec 13 '25 16:12 coderabbitai[bot]

@coderabbitai Review the code changes in detail

Dec 13 '25 16:12 benhoff

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the diff for this PR
gh pr diff 8973

echo "---"
echo "Examining the full file content for chroot-helpers.sh"
echo "---"

# Get the full current file content
gh pr view 8973 --json files --jq '.files[].path' | while read file; do
    echo "File: $file"
done

Length of output: 5402

🏁 Script executed:

#!/bin/bash
# Get the actual file content from the PR branch
gh api repos/armbian/build/contents/lib/functions/general/chroot-helpers.sh?ref=cache | jq -r '.content' | base64 -d

echo "---END OF chroot-helpers.sh---"

# Get the README content
gh api repos/armbian/build/contents/config/README.md?ref=cache | jq -r '.content' | base64 -d

Length of output: 135

@benhoff I'll provide a detailed review of the code changes.

Overall Assessment

The implementation adds useful mount tracking and error handling to the chroot mounting process, plus configurable cache binding. However, there are several issues that need to be addressed.

Critical Issues

1. Cache mount not tracked in cleanup logic

Location: Lines 90-98 in mount_chroot

The cache bind mount is performed after all other mounts, but it's not added to the mounted_points array. This means:

If a failure occurs later (though unlikely since it's the last mount), the cache won't be cleaned up
More importantly, it's inconsistent with the rest of the mount tracking pattern

Recommendation: Add the cache mount to the tracking array:

if ! mount --bind "${cache_src}" "${target}/armbian/cache"; then
    display_alert "Cache bind failed" "${cache_src} -> ${target}/armbian/cache" "warn"
else
    mounted_points+=("${target}/armbian/cache")
fi

2. Nested cleanup function scope issue

Location: Lines 20-26

The cleanup_mounted_points() function is defined inside mount_chroot, which is fine for error handling within the function. However, this creates a potential maintenance issue:

The cleanup logic is only available during mount failures
umount_chroot has separate unmount logic that could get out of sync

Recommendation: Consider whether cleanup should be more symmetrical with unmount, or document why they differ.

3. No validation of ARMBIAN_CACHE_DIR

Location: Line 90

The code uses ARMBIAN_CACHE_DIR without validating:

Whether it's an absolute path
Whether it contains special characters that could cause issues
Whether it's writable

Recommendation: Add validation:

cache_src="${ARMBIAN_CACHE_DIR:-/armbian/cache}"
# Validate cache_src is an absolute path
if [[ -n "${cache_src}" && "${cache_src}" != /* ]]; then
    display_alert "ARMBIAN_CACHE_DIR must be an absolute path" "${cache_src}" "err"
    cache_src=""  # Disable cache binding
fi

Moderate Issues

4. Race condition with mountpoint checks

Location: Lines 33, 40, 47, 54, 61, 68, 75

The pattern if ! mountpoint -q ... then mount ... has a potential TOCTOU (time-of-check-time-of-use) race condition, though unlikely in practice. More importantly, if something is already mounted and it's not what you expect, the function proceeds silently.

Recommendation: Consider logging when skipping an already-mounted path:

if ! mountpoint -q "${target}/tmp"; then
    if ! mount -t tmpfs -o "size=99%" tmpfs "${target}/tmp"; then
        display_alert "Failed to mount tmpfs inside chroot" "${target}/tmp" "err"
        cleanup_mounted_points
        return 1
    fi
    mounted_points+=("${target}/tmp")
else
    display_alert "Already mounted — skipping" "${target}/tmp" "debug"
fi

5. Inconsistent error handling for cache bind

Location: Lines 91-98

Cache directory creation and bind mount failures are logged as "warn" but don't cause function failure, while all other mount failures cause immediate return. This inconsistency should be intentional and documented.

Recommendation: Add a comment explaining why cache bind failures are non-fatal:

# Bind host cache into chroot if present (configurable via ARMBIAN_CACHE_DIR)
# Note: cache bind is optional — failures here are non-fatal

6. umount_chroot doesn't respect mount tracking

Location: Lines 115-116

The unmount of the cache is hardcoded, while the original mounts use a while loop with grep. This could lead to:

Leftover mounts if paths change
Inconsistent cleanup behavior

Recommendation: Consider whether umount_chroot should track what was actually mounted or continue with the current "force unmount everything" approach.

Minor Issues

7. mkdir -p error handling

Location: Lines 27-30, 93-94

The error message "Failed to prepare chroot runtime directory" could be more specific about whether it's a permissions issue, disk full, etc.

Recommendation: Consider capturing and displaying the actual error:

if ! mkdir -p "${target}/run/user/0" 2>&1 | display_alert "Failed to prepare chroot runtime directory" "${target}/run/user/0" "err"; then
    return 1
fi

8. Default cache path assumption

Location: Line 90, config/README.md line 5

The default /armbian/cache is a root-level directory that may not exist on many systems and requires root to create. This should be clearly documented as requiring setup.

Current documentation is good but could add a note: "Ensure the directory exists and is writable before building."

Positive Aspects

✅ Excellent error handling: Each mount operation now has proper error checking
✅ Mount tracking: The mounted_points array is a good pattern for cleanup
✅ Configurable cache: Makes the build system more flexible for different environments
✅ Non-fatal cache binding: Sensible choice to make cache optional
✅ Documentation: The README addition clearly explains the new feature

Testing Recommendations

Given you mentioned not having bare-metal for testing:

Test with cache directory present and absent
Test with ARMBIAN_CACHE_DIR set to various paths (relative, absolute, non-existent)
Test behavior when mounts already exist (e.g., calling mount_chroot twice)
Test cleanup on failure (artificially fail a late mount to verify cleanup works)
Verify no mount leaks after umount_chroot

Security Considerations

Bind mounts can expose host filesystem to chroot — ensure ARMBIAN_CACHE_DIR is trusted
tmpfs size=99% could cause memory exhaustion on systems with limited RAM during large builds
Consider whether cache directory should have specific permissions enforced

Would you like me to elaborate on any of these points or help with generating test cases?

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Dec 13 '25 16:12 coderabbitai[bot]

Add configurable cache bind for chroot

Description

How Has This Been Tested?

Checklist:

Summary by CodeRabbit

Release Notes

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Overall Assessment

Critical Issues

1. Cache mount not tracked in cleanup logic

2. Nested cleanup function scope issue

3. No validation of ARMBIAN_CACHE_DIR

Moderate Issues

4. Race condition with mountpoint checks

5. Inconsistent error handling for cache bind

6. umount_chroot doesn't respect mount tracking

Minor Issues

7. mkdir -p error handling

8. Default cache path assumption

Positive Aspects

Testing Recommendations

Security Considerations