[WIP] - build framework - mmdebstrap minimal conversion #8785
Conversation
github-actions
bot
added
size/medium
WalkthroughAdds Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
lib/functions/rootfs/rootfs-create.sh (1)
129-129: Update error and informational messages to reference mmdebstrap.Lines throughout the function still reference "debootstrap" in alerts and error messages (e.g., line 94: "Debootstrap version", line 129: "Debootstrap first stage failed") even though the tool has been switched to mmdebstrap. While the function name
create_new_rootfs_cache_via_debootstrap()is unchanged, updating messages would improve clarity for operators reading build logs.Also applies to: 94-94
📜 Review details
Configuration used: CodeRabbit UI
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/rootfs/rootfs-create.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/functions/rootfs/rootfs-create.sh (2)
lib/functions/general/git.sh (1)
fetch_from_repo(80-300)lib/functions/logging/runners.sh (1)
run_host_command_logged(204-206)
🔇 Additional comments (1)
lib/functions/rootfs/rootfs-create.sh (1)
77-83: Debian mmdebstrap branch is appropriately set tomain; concern does not apply.Unlike Ubuntu's release-specific branching strategy (Ubuntu uses
branch:ubuntu/noble), the mmdebstrap main branch is designed to support all Debian suites internally (unstable, stable, oldstable, stretch, buster, bullseye, bookworm, trixie, etc.). Themainbranch is the universal branch for Debian support, not a problematic hardcoding. While the FIXME comment about making the branch a variable has merit as technical debt, it does not represent a functional issue like the Ubuntu case might suggest.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
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/rootfs/rootfs-create.sh(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/functions/rootfs/rootfs-create.sh (2)
lib/functions/general/git.sh (1)
fetch_from_repo(80-300)lib/functions/logging/runners.sh (1)
run_host_command_logged(204-206)
🔇 Additional comments (6)
lib/functions/rootfs/rootfs-create.sh (6)
64-72: Follow up on Ubuntu branch hardcoding—verify multi-release compatibility.The
ubuntu/noblebranch is hardcoded with a FIXME comment (line 68). Per the previous review discussion, mmdebstrap should theoretically bootstrap any Ubuntu release (jammy, focal, noble, etc.) as long as the host has up-to-date debootstrap scripts and distro-info metadata. Before merging, verify whether theubuntu/noblebranch successfully builds rootfs for other Ubuntu releases, or if a dynamicbranch:ubuntu/${RELEASE}approach is needed.Also consider whether using Debian's
branch:main(like line 76) would be simpler for Ubuntu as well, since mmdebstrap is fundamentally a Debian tool that works for both distributions.Test whether the
ubuntu/noblebranch of mmdebstrap successfully bootstraps at least one other Ubuntu release (e.g., jammy or focal). If successful, the FIXME can be resolved; if not, update the branch dynamically.
73-80: Debian version extraction looks correct.The markdown header pattern
^## \[([^]]*)\]correctly extracts versions from markdown changelog headers. Verify that the first non-comment header line in mmdebstrap's CHANGELOG.md is always a version header (not "## Unreleased" or other headers that would break extraction).
86-88: mmdebstrap binary setup looks good.The binary path construction and permissions are correct. Version extraction is displayed for audit/logging purposes.
110-127: mmdebstrap invocation and post-bootstrap logic look correct.The binary is invoked directly without DEBOOTSTRAP_DIR (which mmdebstrap doesn't respect), and the second-stage bootstrap is correctly omitted. Error messages reference mmdebstrap appropriately. The /bin/bash check is a reasonable bootstrap completion marker.
267-267: Final status messaging appropriately updated for mmdebstrap.The "Ending mmdebstrap process and preparing cache" message is clear and reflects the tool being used.
64-72: Follow up on hardcoded Ubuntu branch—verify multi-release support.The
ubuntu/noblebranch is hardcoded on line 68 with a FIXME comment. Per the previous review discussion, mmdebstrap should theoretically bootstrap any Ubuntu release (jammy, focal, noble, etc.) as long as the host has up-to-date debootstrap scripts and distro-info. Before merging, verify whether this singleubuntu/noblebranch successfully builds rootfs for other Ubuntu releases (e.g., jammy, focal), or whetherbranch:ubuntu/${RELEASE}is required.Alternatively, consider using Debian's
branch:main(line 76) for Ubuntu as well, since mmdebstrap is fundamentally a Debian tool that works for both distributions.Test the
ubuntu/noblebranch to confirm it successfully bootstraps at least one other Ubuntu release (e.g., jammy). If successful, the FIXME can remain deferred; if not, update the branch to be dynamic based on${RELEASE}.
tabrisnet
force-pushed
the
tabrisnet_mmdebstrap
branch
from
80d0aff to
ba919eb
Compare
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @tabrisnet. * #8785 (comment) The following files were modified: * `lib/functions/rootfs/rootfs-create.sh`
tabrisnet
force-pushed
the
tabrisnet_mmdebstrap
branch
from
ba919eb to
caaa7e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/functions/rootfs/rootfs-create.sh (1)
121-121: Clarify purpose of secondlocal_apt_deb_cache_preparecall.Line 121 calls
local_apt_deb_cache_preparewithskip_target_check="yes"after mmdebstrap completes. The comment says "just for size reference in logs," but this is not intuitive. Confirm this is only for logging and not required for functional correctness, or add a more descriptive comment explaining the operational purpose (e.g., cache statistics, cleanup scheduling).If this is purely informational, consider adding a brief inline comment:
- skip_target_check="yes" local_apt_deb_cache_prepare "for mmdebstrap" # just for size reference in logs + skip_target_check="yes" local_apt_deb_cache_prepare "for mmdebstrap" # log cache stats after bootstrap
📜 Review details
Configuration used: CodeRabbit UI
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/rootfs/rootfs-create.sh(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/functions/rootfs/rootfs-create.sh (4)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)lib/functions/general/git.sh (1)
fetch_from_repo(80-300)lib/functions/logging/runners.sh (1)
run_host_command_logged(204-206)lib/functions/rootfs/create-cache.sh (1)
create_new_rootfs_cache(53-73)
🔇 Additional comments (11)
lib/functions/rootfs/rootfs-create.sh (11)
48-53: ✓ Function docstring accurately reflects mmdebstrap usage.Updated to describe the mmdebstrap-driven flow and the stages it now handles.
65-89: Address distribution-specific mmdebstrap branch handling.Lines 72-73 (Ubuntu) and 80-81 (Debian) hardcode mmdebstrap branches with FIXME comments. The prior review noted this limits Ubuntu support to
noble, but the discussion resolved that mmdebstrap—like debootstrap—reads from/usr/share/debootstrap/scripts/and distro-info to determine which releases it can bootstrap, making a single branch viable for multiple releases.However, this warrants verification: test that the
ubuntu/noblebranch successfully bootstraps other Ubuntu releases (jammy, focal, etc.) to confirm the FIXME comment can be safely removed. If the ubuntu/noble branch is truly universal across Ubuntu releases, document that assumption and remove the FIXME comments.To verify whether the hardcoded ubuntu/noble branch works across Ubuntu releases:
- Inspect the mmdebstrap git history: does the
ubuntu/noblebranch contain version-agnostic tooling that supports older/newer Ubuntu codenames?- Test a build targeting ubuntu/jammy using the
ubuntu/noblemmdebstrap branch and confirm it succeeds.Alternatively, if the intent is to always fetch the latest mmdebstrap from Ubuntu's noble branch (which is reasonable), clarify the FIXME and document that design choice.
95-95: ✓ Stage messaging correctly updated to single-stage model."Stage 1/1" reflects mmdebstrap's single-stage bootstrap (unlike debootstrap's two-stage process). Correct.
98-119: Verify mmdebstrap argument construction and invocation.Lines 98–119 construct a
debootstrap_argumentsarray and invoke mmdebstrap. Key points:
- Line 106: Conditional keyring argument for loong64 architecture—valid.
- Lines 110–112: Apt cache hooks (setup-hook, sync-in, customize-hook) — prior review clarified that creating both host and chroot directories in the mkdir setup-hook is correct per mmdebstrap manpage.
- Line 115: Positional arguments (RELEASE, SDCARD, mirror) appended last—correct for mmdebstrap.
- Line 117: Invocation uses
"${debootstrap_arguments[@]}"array expansion—correct.The implementation matches the mmdebstrap manpage examples. No changes needed; this looks correct.
110-112: ✓ Apt cache hooks are syntactically correct for mmdebstrap.
- Line 110: mkdir setup-hook creates both host and chroot directories (per mmdebstrap manpage example).
- Line 111: sync-in as setup-hook copies cached packages into chroot before bootstrap.
- Line 112: sync-out as customize-hook copies new packages back to host after bootstrap.
All three hooks follow mmdebstrap conventions correctly.
117-119: ✓ mmdebstrap invocation correctly uses array arguments.Line 117 invokes
${debootstrap_bin}(which is set to mmdebstrap at line 91) with"${debootstrap_arguments[@]}"array expansion. This is the correct approach for passing variadic arguments in bash.Error handling at line 118 references
mmdebstrapin the message (matching the tool selected).
125-125: ✓ Error check for /bin/bash is correct.Verifies that mmdebstrap successfully created the base system. Message now references mmdebstrap (corrected from prior debootstrap-specific error messages).
127-131: ✓ mmdebstrap cleanup and cache logging are correct.
- Line 127–128: Cleanup message and apt cache purge correctly reference mmdebstrap.
- Line 129: Removes apt cache directories post-bootstrap.
- Line 131: Final cache statistics logged for reference.
Naming and logic align with mmdebstrap lifecycle.
256-262: ✓ machine-id handling is appropriate for mmdebstrap context.The logic to clear/remove machine-id (lines 256–262) is distribution-agnostic and works correctly with mmdebstrap. Comment (lines 256–260) explains the rationale: prevent systemd v2.50+ from auto-starting services on first boot. This is sound.
268-272: ✓ Cleanup and final messaging are correct.
- Line 269: Undeploy qemu binary after rootfs is complete.
- Line 272: "Ending mmdebstrap process" message correctly reflects the tool used and lifecycle stage.
Messaging is clear and consistent with mmdebstrap terminology.
70-89: Based on web search results, I can now verify the regex patterns against the documented changelog formats. Let me fetch the actual files to test:Based on my verification through web search and documentation analysis, I can now provide the final response:
Both version extraction regex patterns are correctly matched to their respective changelog file formats.
The Ubuntu debian/changelog format follows the standard "package (version) distributions; metadata" pattern where version is delimited by parentheses. The Ubuntu pattern on line 76 (
sed 's/.*(\(.*\)).*/\1/; q') correctly extracts content within parentheses.The Debian mmdebstrap CHANGELOG.md uses the standard markdown "Keep a Changelog" format with headers like
## [VERSION]. The Debian pattern on line 84 (sed 's/^## \[\([^]]*\)\].*/\1/; q') correctly matches this markdown header format and extracts the version within brackets.Both patterns are appropriate for their respective file formats and follow correct sed syntax for capturing and extracting the version strings.
|
@coderabbitai docstring added here: https://github.com/armbian/build/pull/8785/files#diff-0abead2985d704c0354a64c2249a0ad228967ea672873e544d1330735116e436R48-R52 via commit |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 17037 @tabrisnet Thank you for adding the docstring! I've reviewed the addition at lines 48-52, and it looks great. The docstring effectively describes the function's purpose and key operations: ✅ Well-structured: Clearly explains that the function populates a root FS using mmdebstrap The docstring provides excellent context for anyone reading or maintaining this code. Well done! 🎉 |
|
the rootfs tarballs are about 10MB bigger, attached is a diff of the file list from the tarballs. |
tabrisnet
force-pushed
the
tabrisnet_mmdebstrap
branch
from
caaa7e0 to
894709b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/functions/rootfs/rootfs-create.sh (2)
93-93: Minor terminology inconsistency: message says "Debootstrap version" but tool is mmdebstrap. Consider updating the display_alert at line 93 to say "mmdebstrap version" or similar for clarity.- display_alert "Debootstrap version" "'${debootstrap_version}' for ${debootstrap_bin}" "info" + display_alert "mmdebstrap version" "'${debootstrap_version}' for ${debootstrap_bin}" "info"
53-53: Function name still references debootstrap despite now using mmdebstrap. The functioncreate_new_rootfs_cache_via_debootstrap()uses mmdebstrap internally. Consider renaming tocreate_new_rootfs_cache_via_mmdebstrap()or similar if backward-compatibility with calling code allows.This is a low-priority suggestion—renaming would improve clarity but requires checking all callers.
📜 Review details
Configuration used: CodeRabbit UI
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/rootfs/rootfs-create.sh(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/functions/rootfs/rootfs-create.sh (2)
lib/functions/general/git.sh (1)
fetch_from_repo(80-300)lib/functions/logging/runners.sh (1)
run_host_command_logged(204-206)
🔇 Additional comments (4)
lib/functions/rootfs/rootfs-create.sh (4)
48-52: Docstring addition is clear and comprehensive. The comment accurately describes the mmdebstrap workflow including locale, apt, packages, policy diverts, qemu-binfmt, resolvconf, and machine-id handling.
110-112: Apt cache hook configuration is correct. The mkdir setup-hook creates both host and chroot directories (per mmdebstrap manpage), sync-in copies cache into chroot pre-bootstrap, and sync-out runs post-bootstrap to save new packages—phases and syntax match mmdebstrap expectations.
117-119: Bootstrap invocation and error handling look correct. The mmdebstrap binary is called with the full argument array and error messages reference the tool appropriately.
123-132: Post-bootstrap operations and cleanup are correct. Validation for /bin/bash, apt cache cleanup, and messaging all appropriately reference mmdebstrap and follow the expected sequence.
|
@tabrisnet I picked this to test, but https://paste.next.armbian.com/ubiridaduh - there might be something about the keyrings. |
I've done all my testing with I took the board/branch/release specified in your pastebin, so if you have an idea of what needs fixing in the script for the signing keys, please let me know. But I don't have any hosts with |
|
Isn't |
tabrisnet
force-pushed
the
tabrisnet_mmdebstrap
branch
from
15236ed to
1a10f8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/functions/rootfs/distro-specific.sh (1)
95-115: Hardcoded keyring versions should use dynamic retrieval (document FIXME scope).Lines 97 and 109 reference hardcoded debian-archive-keyring and debian-ports-archive-keyring
.debversions (2025.1and2025.04.05). Lines 96 and 108 include FIXME comments acknowledging this ("write something to retrieve newest"). Additionally, line 124 hardcodesubuntu-keyring_2023.11.28.1_all.deb, which is now over a year old.While hardcoding versions can ensure reproducibility, outdated keyrings may miss security updates or new distro releases. Consider either:
- Implementing dynamic version retrieval (referenced in the FIXMEs), or
- Adding a mechanism to update these versions periodically (e.g., a config file or environment variable override).
This is preparatory work, but should be tracked to avoid stale keyrings becoming a compliance issue.
lib/functions/rootfs/rootfs-create.sh (1)
112-112: Consider removing FIXME about Ubuntu branch—verify scope and priority.Line 112 includes a FIXME comment suggesting the ubuntu/noble branch "should be a variable eventually." However, per prior discussion with @tabrisnet, the ubuntu/noble branch of mmdebstrap can successfully bootstrap other Ubuntu releases (jammy, focal, etc.) because mmdebstrap reads supported suites from
/usr/share/debootstrap/scriptsanddistro-info.If this has been validated in testing with multiple Ubuntu releases, the FIXME may be unnecessary and can be removed or updated to clarify that a single branch supports multiple releases. If this remains uncertain, leave it as a tracker for future investigation.
📜 Review details
Configuration used: CodeRabbit UI
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 (3)
lib/functions/artifacts/artifact-rootfs.sh(1 hunks)lib/functions/rootfs/distro-specific.sh(1 hunks)lib/functions/rootfs/rootfs-create.sh(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.858Z
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.
📚 Learning: 2025-10-24T04:46:22.858Z
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.858Z
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/rootfs/distro-specific.shlib/functions/rootfs/rootfs-create.sh
📚 Learning: 2025-10-24T04:46:22.858Z
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.858Z
Learning: The `debootstrap` tool doesn't use `apt`, which is important context when reviewing apt-related configuration in rootfs creation code.
Applied to files:
lib/functions/rootfs/rootfs-create.sh
🧬 Code graph analysis (2)
lib/functions/rootfs/distro-specific.sh (1)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)
lib/functions/rootfs/rootfs-create.sh (4)
lib/functions/general/git.sh (1)
fetch_from_repo(80-300)lib/functions/logging/runners.sh (2)
run_host_command_logged(204-206)chroot_sdcard(133-136)lib/functions/rootfs/distro-specific.sh (1)
fetch_distro_keyring(73-139)lib/functions/rootfs/post-tweaks.sh (1)
post_debootstrap_tweaks(10-34)
🔇 Additional comments (10)
lib/functions/artifacts/artifact-rootfs.sh (1)
18-18: LGTM — LEGACY_DEBOOTSTRAP integration looks correct.The variable is properly initialized with sensible defaults and correctly included in the hash calculation for artifact cache invalidation. This ensures builds using different bootstrap methods (legacy debootstrap vs. mmdebstrap) produce different cache keys and don't collide.
Also applies to: 21-21
lib/functions/rootfs/distro-specific.sh (2)
131-131: Clarify Ubuntu-specific --keyring argument—Debian path lacks equivalent.Line 131 adds
--keyring=/usr/share/keyrings/ubuntu-archive-keyring.gpgfor Ubuntu releases, but the Debian case (lines 91–116) does not add a corresponding--keyringargument. Both paths add the same--setup-hook(line 138) for copying keyring files into the chroot.Is the Ubuntu --keyring argument necessary for mmdebstrap to recognize and use the keyring, while Debian keyrings are sufficient through the setup-hook alone? If so, consider documenting this distinction with an inline comment. If both distros should have --keyring, apply the same pattern to the Debian case.
69-87: Release-to-distro mapping is comprehensive and well-organized.The case statement correctly maps all currently supported releases to their respective distributions and rejects unknown releases with a clear error. This ensures early failure for typos or unsupported releases.
lib/functions/rootfs/rootfs-create.sh (7)
55-70: LGTM — LOG_NAME validation and defaulting is correct and defensive.The switch statement properly validates
LEGACY_DEBOOTSTRAP, defaults empty values to mmdebstrap ("no"), and rejects invalid values with a clear error. This prevents silent failures from typos or misconfiguration. Dynamic tool naming in messaging is good practice for maintainability.
96-139: LGTM — Distribution-specific bootstrap tool selection is well-structured.The code correctly branches between legacy debootstrap and mmdebstrap per distribution, fetches from appropriate upstream repositories, and extracts versions from the correct changelog format (debian/changelog for Ubuntu repositories, CHANGELOG.md for Debian's upstream). The symlink logic for missing release scripts (lines 146–149, legacy path only) is appropriate and debootstrap-specific.
156-180: LGTM — Debootstrap arguments construction is properly conditional and well-organized.The core arguments (variant, arch, include, components) are stable across both paths, while mmdebstrap-specific arguments (skip/empty check, fetch_distro_keyring invocation) and legacy-specific arguments (cache-dir, foreign mode) are cleanly separated. The order (core args, then conditional args, then positional args at line 182) is correct and matches debootstrap/mmdebstrap conventions.
168-168: Verify loong64 keyring handling does not conflict with mmdebstrap's fetch_distro_keyring.Line 168 unconditionally adds
--keyring=/usr/share/keyrings/debian-ports-archive-keyring.gpgfor loong64 architecture, regardless of bootstrap method. For the legacy debootstrap path, this is correct. However, for mmdebstrap (line 162–165),fetch_distro_keyring()will be invoked and also download/setup debian-ports-archive-keyring. This results in:
- The --keyring argument being added twice (line 168 + line 138's setup-hook already includes it)
- Potential redundancy or argument conflicts
Verify that mmdebstrap handles duplicate
--keyringarguments gracefully, or make this conditional onLEGACY_DEBOOTSTRAPto avoid redundancy.
184-196: LGTM — Bootstrap execution paths have proper error handling and tool-specific logic.Mmdebstrap (lines 184–187) and legacy debootstrap (lines 188–196) branches are cleanly separated. Error messages properly reference the active tool via
${LOG_NAME}. Marker file check (line 195) is appropriately placed only in the legacy path, since mmdebstrap is single-stage. The DEBOOTSTRAP_DIR environment variable is correctly scoped only to the legacy path (lines 190, 194).
202-207: LGTM — Second-stage bootstrap is correctly limited to legacy debootstrap path.The second-stage execution (line 206:
/debootstrap/debootstrap --second-stage) is appropriately guarded byLEGACY_DEBOOTSTRAP == "yes", since mmdebstrap completes in one stage. Stage labeling (lines 150–153) correctly reflects this (Stage 1/2 for legacy, Stage 1/1 for mmdebstrap).
48-53: Docstring is accurate and describes the dual-mode nature clearly.The updated docstring correctly reflects that the function now supports both mmdebstrap and legacy debootstrap, and appropriately summarizes the key operations (locale, apt sources, packages, chroot setup, etc.). Good documentation for future maintainers.
github-actions
bot
added
size/large
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
lib/functions/rootfs/distro-specific.sh (4)
69-86: Add validation and documentation for fetch_distro_keyring function.The function header documentation is minimal and doesn't describe the function's side effects (modifying
debootstrap_arguments, creating cache directories, downloading packages). While the function is defensive (exits on unrecognized release), it would benefit from:
- Explicit parameter validation
- Documentation of global state modifications
- Description of network calls and caching behavior
This aids maintainability and helps future contributors understand the function's full contract.
Consider enhancing the function documentation:
#fetch_distro_keyring <release> # # <release>: debian or ubuntu release name (required) # # Downloads and caches distro-specific archive keyrings, then: # - Appends keyrings to debootstrap_arguments array (modifies global) # - Copies keyring files to host /usr and /etc for chroot access # - Adds --setup-hook to debootstrap_arguments for setup phase # # Requires: MANAGE_ACNG, CACHEDIR setup; curl, dpkg-deb available # Exits on: unrecognized release, download/extraction failures # function fetch_distro_keyring() {
110-126: Refactor repeated Debian keyring fetching logic into a loop.Lines 110-126 (debian-archive-keyring) and 128-143 (debian-ports-archive-keyring) share nearly identical structure: fetch package URL, validate, download, extract, handle .pgp/.gpg naming. This duplication increases maintenance burden and risk of divergent handling.
Extract into a helper function or loop to reduce duplication and ensure consistent error handling:
# Helper function to fetch and cache a single Debian keyring _fetch_debian_keyring_file() { local pkg_name="${1}" local search_url="${2}" local keyring_in_deb="${3}" local keyring_output="${4}" if [[ -e "${CACHEDIR}/${keyring_output}" ]]; then return 0 fi PKG_URL=$(curl --compressed -Ls "${search_url}" | grep -oP 'http://(deb|ftp)\.debian\.org/debian/pool/main/[^/]*/'"${pkg_name}"'/'"${pkg_name}"'_[0-9.]*_[a-zA-Z0-9]*\.deb') [[ -z "${PKG_URL}" ]] && exit_with_error "fetch_distro_keyring failed" "unable to find newest ${pkg_name} package" run_host_command_logged curl "${PROXY[@]}" -fLOJ --output-dir "${CACHEDIR}" "${PKG_URL}" || \ exit_with_error "fetch_distro_keyring failed" "unable to download ${PKG_URL}" KEYRING_DEB=$(basename "${PKG_URL}") dpkg-deb -x "${CACHEDIR}/${KEYRING_DEB}" "${CACHEDIR}" || \ exit_with_error "fetch_distro_keyring" "dpkg-deb -x failed for ${KEYRING_DEB}" if [[ -e "${CACHEDIR}${keyring_in_deb}.pgp" ]]; then cp -l "${CACHEDIR}${keyring_in_deb}.pgp" "${CACHEDIR}/${keyring_output}" elif [[ -e "${CACHEDIR}${keyring_in_deb}.gpg" ]]; then cp -l "${CACHEDIR}${keyring_in_deb}.gpg" "${CACHEDIR}/${keyring_output}" else exit_with_error "fetch_distro_keyring" "unable to find ${keyring_in_deb}" fi } # In debian branch: _fetch_debian_keyring_file \ "debian-archive-keyring" \ 'https://packages.debian.org/sid/all/debian-archive-keyring/download' \ "/usr/share/keyrings/debian-archive-keyring" \ "debian-archive-keyring.gpg" _fetch_debian_keyring_file \ "debian-ports-archive-keyring" \ 'https://packages.debian.org/sid/all/debian-ports-archive-keyring/download' \ "/usr/share/keyrings/debian-ports-archive-keyring" \ "debian-ports-archive-keyring.gpg"This reduces the function from ~75 lines to ~40 while improving maintainability.
Also applies to: 128-143
104-166: Consider unifying keyring injection approach between Debian and Ubuntu.Debian uses a
--setup-hook=copy-into inject keyrings during debootstrap, while Ubuntu appends--keyring=/usr/share/keyrings/ubuntu-archive-keyring.gpgas a debootstrap argument (line 165). Both approaches work, but the inconsistency makes the code harder to follow and maintain. Additionally, the comment on line 168-169 flags a known cross-filesystem risk withcp -lhardlinks.Align both branches on a single approach. The
--keyringargument approach (used for Ubuntu) is cleaner and more direct for debootstrap/mmdebstrap. Consider using--keyringfor Debian as well:# Debian branch (lines 145-146): debootstrap_arguments+=("--keyring=${CACHEDIR}/debian-archive-keyring.gpg") debootstrap_arguments+=("--keyring=${CACHEDIR}/debian-ports-archive-keyring.gpg") # Ubuntu branch (line 165): debootstrap_arguments+=("--keyring=${CACHEDIR}/ubuntu-archive-keyring.gpg") # Then remove the setup-hook and cp -r lines (168-172) as keyrings are now injected via --keyringThis eliminates the setup-hook complexity and the cross-filesystem risk of hardlinks.
110-111: Web scraping approach for keyring packages is functional but fragile.The function parses HTML from package.debian.org and packages.ubuntu.com to find .deb package URLs. This approach is vulnerable to page structure changes and involves multiple network round-trips (2-4 curl calls per function invocation).
While the implementation is defensive (validates URLs found) and appropriate for an infrequent build system operation, consider as future improvement using more stable sources like:
- Debian metadata files (Release, Packages) from mirrors
- Ubuntu launchpad API or package metadata
- Mirror.example.com/by-hash or similar stable endpoints
This is acceptable for now but document as a potential maintenance point for future Armbian contributors.
Also applies to: 128-129, 151-156
📜 Review details
Configuration used: CodeRabbit UI
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 (3)
lib/functions/artifacts/artifact-rootfs.sh(1 hunks)lib/functions/rootfs/create-cache.sh(1 hunks)lib/functions/rootfs/distro-specific.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/functions/artifacts/artifact-rootfs.sh
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.858Z
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.
📚 Learning: 2025-10-24T04:46:22.858Z
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.858Z
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/rootfs/distro-specific.sh
🧬 Code graph analysis (1)
lib/functions/rootfs/distro-specific.sh (2)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)lib/functions/logging/runners.sh (1)
run_host_command_logged(204-206)
🔇 Additional comments (3)
lib/functions/rootfs/create-cache.sh (1)
31-32: Cache hash correctly includes bootstrap method flag.The LEGACY_DEBOOTSTRAP flag is properly integrated into the cache hash, ensuring that switching between bootstrap methods invalidates the cache and forces a fresh rootfs build. Good practice.
lib/functions/rootfs/distro-specific.sh (2)
88-100: Proxy configuration is well-structured.The proxy handling correctly accommodates three scenarios (MANAGE_ACNG=yes/no/custom) and properly uses bash fallthrough. This is solid work.
170-172: ****The setup-hook quoting at line 172 is correct and follows the established pattern in the codebase. mmdebstrap documentation indicates that hook arguments should be wrapped in single quotes to protect commands from outer shell expansion, with the hook command then parsed by mmdebstrap.
The identical quoting pattern appears in rootfs-create.sh (lines 173–174) and is already functional:
debootstrap_arguments+=("--setup-hook='mkdir -p ${LOCAL_APT_CACHE_INFO[HOST_DEBOOTSTRAP_CACHE_DIR]} \"\$1\"/var/cache/apt/archives/'")When the array expands via
"${debootstrap_arguments[@]}", mmdebstrap receives and correctly parses the quotes as part of the hook syntax—it does not treat them as literal characters in the executed command. If this were an issue, the existing code in rootfs-create.sh would already fail.Likely an incorrect or invalid review comment.
tabrisnet
force-pushed
the
tabrisnet_mmdebstrap
branch
from
059ec20 to
da1575d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/functions/rootfs/distro-specific.sh (3)
88-100: Ambiguous PROXY array construction when proxy is disabled.Lines 88–100 add an empty string to the
PROXYarray whenMANAGE_ACNGis "no" or empty (line 95:PROXY+=('')). This results in an empty array element that expands to nothing in the curl command, which is harmless but confusing. A clearer pattern would avoid polluting the array when no proxy is needed.Consider restructuring to only populate
PROXYwhen a proxy is actually configured:declare -a PROXY case "${MANAGE_ACNG}" in yes) PROXY+=('-x' 'http://localhost:3142/') ;; - no) ;& #fallthrough - "") - PROXY+=('') # don't use a proxy - ;; # stop falling *) + if [[ -n "${MANAGE_ACNG}" && "${MANAGE_ACNG}" != "no" ]]; then - PROXY+=('-x' "${MANAGE_ACNG}") + PROXY+=('-x' "${MANAGE_ACNG}") + fi ;; esacAlternatively, if an empty string is intentional as a placeholder, add an inline comment explaining why.
110-112: Fragile HTML scraping for package discovery is unmaintained and error-prone.The function relies on parsing HTML from packages.debian.org and packages.ubuntu.com to locate the newest keyring packages (lines 110–112, 128–130, 152–154). This approach is brittle:
- Depends on exact HTML structure; upstream layout changes will break discovery.
- No timeout, retry, or circuit-breaker logic; network issues cause silent failures.
- No validation that the extracted URL is syntactically valid or points to the correct package.
- For Ubuntu, parsing changelogs.ubuntu.com adds another external dependency (line 151).
Consider a more maintainable approach:
- Hardcode known package URLs if the URLs are relatively stable (e.g.,
https://deb.debian.org/debian/pool/main/d/debian-archive-keyring/).- Fetch package lists via APT if bootstrapping inside a container with pre-configured apt sources.
- Use distro-provided keyrings bundled with the Docker base image (already available in many cases).
For immediate stability, add explicit error handling and retry logic:
fetch_package_url() { local max_retries=3 attempt=1 while (( attempt <= max_retries )); do # existing curl + grep logic if [[ -n "${PKG_URL}" ]]; then echo "${PKG_URL}" return 0 fi (( attempt++ )) sleep $((2 ** (attempt - 1))) # exponential backoff done return 1 }Do you want me to propose a more robust implementation using one of the above approaches?
Also applies to: 128-130, 152-154
109-143: High code duplication: Debian keyring extraction logic repeated for two packages.Lines 109–126 and 128–143 duplicate the same pattern: extract package URL, download, extract .deb, check for .pgp vs .gpg variants, and link to .gpg. This can be extracted into a helper function to reduce maintenance burden and inconsistencies.
Consider:
fetch_and_cache_keyring_deb() { local package_name="$1" keyring_url="$2" cache_dir="$3" local keyring_deb keyring_file run_host_command_logged curl "${PROXY[@]}" -fLOJ --output-dir "${cache_dir}" "${keyring_url}" || \ exit_with_error "fetch_distro_keyring" "unable to download ${keyring_url}" keyring_deb=$(basename "${keyring_url}") dpkg-deb -x "${cache_dir}/${keyring_deb}" "${cache_dir}" || \ exit_with_error "fetch_distro_keyring" "dpkg-deb -x ${cache_dir}/${keyring_deb} failed" # Find .pgp or .gpg variant for variant in .pgp .gpg; do if [[ -e "${cache_dir}/usr/share/keyrings/${package_name}${variant}" ]]; then cp -l "${cache_dir}/usr/share/keyrings/${package_name}${variant}" "${cache_dir}/${package_name}.gpg" return 0 fi done exit_with_error "fetch_distro_keyring" "unable to find ${package_name} keyring file" }Then invoke it twice for Debian's two keyrings.
Would you like me to generate the complete refactored implementation?
📜 Review details
Configuration used: CodeRabbit UI
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 (3)
lib/functions/artifacts/artifact-rootfs.sh(1 hunks)lib/functions/rootfs/create-cache.sh(1 hunks)lib/functions/rootfs/distro-specific.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/functions/artifacts/artifact-rootfs.sh
- lib/functions/rootfs/create-cache.sh
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.858Z
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.
📚 Learning: 2025-10-24T04:46:22.858Z
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.858Z
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/rootfs/distro-specific.sh
🧬 Code graph analysis (1)
lib/functions/rootfs/distro-specific.sh (3)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)lib/functions/logging/runners.sh (1)
run_host_command_logged(204-206)lib/functions/rootfs/rootfs-create.sh (1)
create_new_rootfs_cache_via_debootstrap(48-298)
tabrisnet
force-pushed
the
tabrisnet_mmdebstrap
branch
from
da1575d to
6e187c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/functions/rootfs/rootfs-create.sh (1)
156-182: Bootstrap arguments correctly branch on legacy vs non-legacy with proper hook syntax.Base arguments (variant, arch, include, components) are correctly assembled. Non-legacy path adds
--skip=check/emptyand invokes keyring preparation. Local apt cache handling properly branches: legacy uses debootstrap's--cache-dirand--foreign; non-legacy uses mmdebstrap's setup/sync-hooks for the same purpose. Positional arguments (release, path, mirror) are correctly placed last.Minor note on line 168: Uses
$ARCHwhile most of the file uses${ARCH}. For consistency, consider[[ ${ARCH} == loong64 ]]. This is stylistic; no functional issue.
📜 Review details
Configuration used: CodeRabbit UI
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 (4)
lib/functions/artifacts/artifact-rootfs.sh(1 hunks)lib/functions/rootfs/create-cache.sh(1 hunks)lib/functions/rootfs/distro-specific.sh(1 hunks)lib/functions/rootfs/rootfs-create.sh(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/functions/rootfs/create-cache.sh
- lib/functions/artifacts/artifact-rootfs.sh
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.858Z
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.
📚 Learning: 2025-10-24T04:46:22.858Z
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.858Z
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/rootfs/distro-specific.shlib/functions/rootfs/rootfs-create.sh
📚 Learning: 2025-10-24T04:46:22.858Z
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.858Z
Learning: The `debootstrap` tool doesn't use `apt`, which is important context when reviewing apt-related configuration in rootfs creation code.
Applied to files:
lib/functions/rootfs/rootfs-create.sh
🧬 Code graph analysis (2)
lib/functions/rootfs/distro-specific.sh (2)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)lib/functions/logging/runners.sh (1)
run_host_command_logged(204-206)
lib/functions/rootfs/rootfs-create.sh (5)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)lib/functions/general/git.sh (1)
fetch_from_repo(80-300)lib/functions/logging/runners.sh (2)
run_host_command_logged(204-206)chroot_sdcard(133-136)lib/functions/rootfs/distro-specific.sh (1)
fetch_distro_keyring(73-172)lib/functions/rootfs/post-tweaks.sh (1)
post_debootstrap_tweaks(10-34)
🔇 Additional comments (5)
lib/functions/rootfs/distro-specific.sh (1)
69-172: Well-structured keyring preparation with comprehensive error handling.The
fetch_distro_keyringfunction correctly handles distro detection, proxy configuration, dual-keyring caching for Debian (archive + ports), and single keyring for Ubuntu, with proper error handling on all external operations (curl, dpkg-deb, cp). The caching logic prevents redundant downloads on subsequent builds.One verification needed: Line 170 copies keyrings to the host filesystem (
/), and line 171 adds a setup-hook to copy them into the chroot during bootstrap. Confirm whether both operations are necessary or if the setup-hook copy-in (line 171) alone is sufficient to provide keyrings to mmdebstrap. If the host-side copy (line 170) is redundant, consider simplifying to reduce complexity.lib/functions/rootfs/rootfs-create.sh (4)
48-70: Docstring and LOG_NAME initialization are clear and properly structured.The enhanced docstring (lines 48–53) accurately describes the function's dual-path behavior. The LOG_NAME initialization (lines 55–70) correctly handles all three states (LEGACY_DEBOOTSTRAP = yes/no/""), defaults to mmdebstrap when unset, and validates against invalid input with an appropriate error exit.
99-139: Distribution-specific tool selection correctly handles both legacy and non-legacy paths.Per-distribution branching (Ubuntu vs Debian) properly selects bootstrap tool source and version. Version extraction correctly differentiates between
debian/changelog(Ubuntu and legacy paths) and markdownCHANGELOG.md(Debian non-legacy upstream). Error handling for unrecognized distributions is in place.
184-207: Bootstrap execution correctly branches on legacy vs non-legacy with proper environment handling.Non-legacy path invokes mmdebstrap directly without DEBOOTSTRAP_DIR (which mmdebstrap ignores). Legacy path sets DEBOOTSTRAP_DIR before invocation and unsets after, with marker file validation only in the legacy block. Second-stage invocation (chroot_sdcard ... --second-stage) is correctly restricted to legacy only; mmdebstrap completes in a single stage. Error handling present for both paths.
208-214: Cleanup and messaging correctly use dynamic tool reference.Post-bootstrap validation (
/bin/bashcheck) and cleanup happen unconditionally after both paths complete. Messaging references${LOG_NAME}throughout, ensuring clarity regardless of which tool was active. Apt cache cleanup and logging are appropriate for both legacy and non-legacy flows.
tabrisnet
force-pushed
the
tabrisnet_mmdebstrap
branch
from
6e187c8 to
51a16e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/functions/rootfs/rootfs-create.sh (2)
155-155: Replace informal error message with professional language.Line 155 uses "cray-cray" which is informal and unclear in a production error message. Replace with a professional description of the issue.
-cd "${SDCARD}" || exit_with_error "cray-cray about SDCARD" "${SDCARD}" +cd "${SDCARD}" || exit_with_error "Failed to change directory" "SDCARD=${SDCARD} does not exist or is inaccessible"
341-343: Clarify systemd version reference in comment.The comment at line 341-343 mentions "systemd version 2.50+" which appears to be a typo. Systemd version numbering uses versions like 250, 251, etc., not 2.50. Clarify the version or remove the specific version reference.
-# Note: As we don't use systemd-firstboot.service functionality, we make it empty to prevent services -# from starting up automatically on first boot on systemd version 2.50+. If someone is using the same, +# Note: As we don't use systemd-firstboot.service functionality, we make it empty to prevent services +# from starting up automatically on first boot on systemd version 250+. If someone is using the same,
📜 Review details
Configuration used: CodeRabbit UI
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 (4)
lib/functions/artifacts/artifact-rootfs.sh(1 hunks)lib/functions/rootfs/create-cache.sh(1 hunks)lib/functions/rootfs/distro-specific.sh(1 hunks)lib/functions/rootfs/rootfs-create.sh(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/functions/artifacts/artifact-rootfs.sh
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.858Z
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.
📚 Learning: 2025-10-24T04:46:22.858Z
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.858Z
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/rootfs/rootfs-create.shlib/functions/rootfs/distro-specific.sh
📚 Learning: 2025-10-24T04:46:22.858Z
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.858Z
Learning: The `debootstrap` tool doesn't use `apt`, which is important context when reviewing apt-related configuration in rootfs creation code.
Applied to files:
lib/functions/rootfs/rootfs-create.sh
🧬 Code graph analysis (2)
lib/functions/rootfs/rootfs-create.sh (5)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)lib/functions/general/git.sh (1)
fetch_from_repo(80-300)lib/functions/logging/runners.sh (2)
run_host_command_logged(204-206)chroot_sdcard(133-136)lib/functions/rootfs/distro-specific.sh (1)
fetch_distro_keyring(73-172)lib/functions/rootfs/post-tweaks.sh (1)
post_debootstrap_tweaks(10-34)
lib/functions/rootfs/distro-specific.sh (2)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)lib/functions/logging/runners.sh (1)
run_host_command_logged(204-206)
🔇 Additional comments (5)
lib/functions/rootfs/create-cache.sh (1)
31-32: Cache ID correctly incorporates bootstrap method choice.Including
LDB=${legacy_debootstrap}in the hash ensures that mmdebstrap and legacy debootstrap builds generate separate cache entries, preventing cross-contamination. This approach properly reflects that the two bootstrap paths produce different artifacts.lib/functions/rootfs/rootfs-create.sh (3)
48-70: Docstring and LOG_NAME initialization provide clear control flow.The docstring accurately describes the function's responsibilities, and the LOG_NAME normalization at lines 55–70 correctly handles all three valid states (legacy "yes", mmdebstrap "no", and empty string defaulting to mmdebstrap) with explicit validation. This makes tool selection transparent throughout the function's logging and error messages.
99-142: Distribution-specific bootstrap tool selection correctly branches on distro and legacy flag.Per-distribution branching ensures debootstrap or mmdebstrap is fetched from the appropriate repository with correct version extraction logic for each source (Debian changelog format for legacy/Ubuntu, markdown for upstream Debian). The FIXME comments at lines 111 and 128 acknowledge known future refinements for dynamic branch selection.
156-196: Bootstrap argument construction and conditional execution are logically sound.Arguments are correctly assembled, conditionally extended (setup-hooks for mmdebstrap, cache-dir for legacy debootstrap), and executed with proper environment setup and validation. DEBOOTSTRAP_DIR is appropriately scoped to the legacy path only.
lib/functions/rootfs/distro-specific.sh (1)
69-172: Keyring preparation function correctly isolates per-distribution logic.The
fetch_distro_keyring()function cleanly separates Debian and Ubuntu keyring handling, with proper error checking at each step (download, extraction, file operations). Verification confirms thatdebootstrap_argumentsis declared as a global array on line 156 ofrootfs-create.sh, before the function is invoked at line 164, ensuring safe array manipulation.
tabrisnet
force-pushed
the
tabrisnet_mmdebstrap
branch
from
51a16e1 to
336de1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/functions/rootfs/rootfs-create.sh (1)
100-139: Minor: Hardcoded Ubuntu branch limits future flexibility but is flagged with FIXME.Ubuntu packages mmdebstrap across multiple releases (jammy, noble, oracular, plucky), but line 112 hardcodes
branch:ubuntu/noble. This matches the FIXME comment acknowledging the limitation. Past PR discussion noted this was acceptable as a first pass, with eventual dynamic branching deferred.Recommendation: The implementation is sound for now. If future Ubuntu releases fail to bootstrap, verify whether the ubuntu/noble branch can bootstrap all target Ubuntu releases (focal, jammy, noble, oracular, plucky) or if release-specific branches are needed.
The version extraction logic correctly distinguishes Ubuntu (debian/changelog format, parentheses regex) from Debian (CHANGELOG.md format, bracket regex), which is a solid implementation per past review clarifications.
📜 Review details
Configuration used: CodeRabbit UI
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 (4)
lib/functions/artifacts/artifact-rootfs.sh(1 hunks)lib/functions/rootfs/create-cache.sh(1 hunks)lib/functions/rootfs/distro-specific.sh(1 hunks)lib/functions/rootfs/rootfs-create.sh(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/functions/rootfs/create-cache.sh
- lib/functions/artifacts/artifact-rootfs.sh
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.858Z
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.
📚 Learning: 2025-10-24T04:46:22.858Z
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.858Z
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/rootfs/distro-specific.shlib/functions/rootfs/rootfs-create.sh
📚 Learning: 2025-10-24T04:46:22.858Z
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.858Z
Learning: The `debootstrap` tool doesn't use `apt`, which is important context when reviewing apt-related configuration in rootfs creation code.
Applied to files:
lib/functions/rootfs/rootfs-create.sh
🧬 Code graph analysis (2)
lib/functions/rootfs/distro-specific.sh (2)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)lib/functions/logging/runners.sh (1)
run_host_command_logged(204-206)
lib/functions/rootfs/rootfs-create.sh (4)
lib/functions/general/git.sh (1)
fetch_from_repo(80-300)lib/functions/logging/runners.sh (2)
run_host_command_logged(204-206)chroot_sdcard(133-136)lib/functions/rootfs/distro-specific.sh (1)
fetch_distro_keyring(73-172)lib/functions/rootfs/post-tweaks.sh (1)
post_debootstrap_tweaks(10-34)
🔇 Additional comments (3)
lib/functions/rootfs/distro-specific.sh (1)
69-145: LGTM — fetch_distro_keyring() function logic and error handling are sound.The function correctly:
- Classifies releases into Debian/Ubuntu distributions.
- Handles proxy configuration with proper fallthrough logic.
- Downloads and extracts Debian (dual) and Ubuntu (single) keyrings with error handling.
- Manages .pgp vs .gpg naming variations introduced in Debian 2025.1.
- Caches keyrings to avoid repeated downloads.
Past critical issues (variable expansion, error handling on dpkg-deb, continue statements) are resolved. The FIXME comment on line 109 about potential parameterization is reasonable and need not block this PR.
lib/functions/rootfs/rootfs-create.sh (2)
184-207: LGTM — Bootstrap execution and second-stage handling correctly branch by tool.
- mmdebstrap path (lines 184-187): Executes directly with error message using dynamic LOG_NAME.
- Legacy debootstrap path (lines 188-196): Exports DEBOOTSTRAP_DIR, runs first stage, validates marker file, and limits second-stage to this path only.
- Second-stage (lines 202-207): Correctly exclusive to legacy debootstrap, which uses a two-stage process. mmdebstrap has no second-stage concept.
Hardcoded "Debootstrap" strings in legacy-only error messages and comments are appropriate, as noted in past review feedback.
210-214: LGTM — Cleanup messaging and final state use dynamic tool names.Cleanup correctly references LOG_NAME for tool-agnostic messaging, and the post-bootstrap rootfs setup (policy-rc.d, divert, locales, apt sources, etc.) is agnostic to bootstrap tool choice.
|
Hey nice work. |
|
@igorpecovnik asked to allow both, see #8785 (comment)
as to how
See armbian/documentation#834
When? Don't know, other than if there is a problem I can't test for with `riscv` or `loong64`
But this does come to mind: #8785 (comment)
|
Oh I missed that. Sorry for the noise. |
What percentage in minimal image we are talking about? |
|
iirc ~10% compressed.106M vs 116M
|
tabrisnet
force-pushed
the
tabrisnet_mmdebstrap
branch
from
336de1b to
deed345
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/functions/rootfs/distro-specific.sh (1)
109-109: Minor: Consider parameterizing repeated keyring fetch logic.Lines 109–143 fetch debian-archive-keyring and debian-ports-archive-keyring using nearly identical patterns. The FIXME comment on line 109 suggests this awareness. A future refactor could extract a helper function to reduce duplication, but this is not a blocker.
lib/functions/rootfs/rootfs-create.sh (1)
55-70: Minor: LEGACY_DEBOOTSTRAP default behavior could be more explicit in comments.The case statement (lines 56–70) handles empty string by defaulting to mmdebstrap. While correct, a clarifying comment on line 63 (e.g.,
# empty string defaults to mmdebstrap) would improve readability for future maintainers unfamiliar with this design choice.
📜 Review details
Configuration used: CodeRabbit UI
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 (4)
lib/functions/artifacts/artifact-rootfs.sh(1 hunks)lib/functions/rootfs/create-cache.sh(1 hunks)lib/functions/rootfs/distro-specific.sh(1 hunks)lib/functions/rootfs/rootfs-create.sh(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/functions/artifacts/artifact-rootfs.sh
- lib/functions/rootfs/create-cache.sh
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: tabrisnet
PR: armbian/build#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.
📚 Learning: 2025-10-24T04:46:22.901Z
Learnt from: tabrisnet
PR: armbian/build#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/rootfs/rootfs-create.shlib/functions/rootfs/distro-specific.sh
📚 Learning: 2025-10-24T04:46:22.901Z
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.901Z
Learning: The `debootstrap` tool doesn't use `apt`, which is important context when reviewing apt-related configuration in rootfs creation code.
Applied to files:
lib/functions/rootfs/rootfs-create.sh
🧬 Code graph analysis (2)
lib/functions/rootfs/rootfs-create.sh (5)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)lib/functions/general/git.sh (1)
fetch_from_repo(80-300)lib/functions/logging/runners.sh (2)
run_host_command_logged(204-206)chroot_sdcard(133-136)lib/functions/rootfs/distro-specific.sh (1)
fetch_distro_keyring(73-172)lib/functions/rootfs/post-tweaks.sh (1)
post_debootstrap_tweaks(10-34)
lib/functions/rootfs/distro-specific.sh (2)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)lib/functions/logging/runners.sh (1)
run_host_command_logged(204-206)
🔇 Additional comments (5)
lib/functions/rootfs/distro-specific.sh (1)
69-172: Well-structured keyring fetching with proper error handling and cache management.The
fetch_distro_keyring()function correctly:
- Detects distro type (Debian/Ubuntu) from release name
- Manages proxy settings per
MANAGE_ACNGconfiguration- Caches keyrings per-distro to avoid repeated downloads
- Extracts keyring packages with error handling on dpkg-deb operations
- Handles both
.pgpand.gpgkeyring naming conventions- Copies keyrings to host and injects setup-hooks for mmdebstrap
The dual-stage approach (--keyring argument for Ubuntu + setup-hook copy-in for both) correctly supports both immediate tool usage and in-chroot availability.
lib/functions/rootfs/rootfs-create.sh (4)
48-70: Comprehensive docstring and clear LEGACY_DEBOOTSTRAP validation.The new docstring accurately describes the function's responsibilities across both debootstrap and mmdebstrap paths. The LEGACY_DEBOOTSTRAP validation logic (lines 56–70) correctly:
- Recognizes "yes", "no", and empty string (defaulting to mmdebstrap)
- Derives LOG_NAME for consistent messaging
- Exits on invalid input
Clear branching improves maintainability.
102-134: Proper per-distribution bootstrap source selection; version extraction correctly differs by distro.Bootstrap tool fetching is properly conditioned:
- Ubuntu: Legacy uses debootstrap from launchpad.net (tag-based), non-legacy uses mmdebstrap (branch-based)
- Debian: Legacy uses salsa.debian.org debootstrap (branch-based), non-legacy uses gitlab.mister-muffin.de mmdebstrap (branch-based)
Version extraction regex differs appropriately:
- Ubuntu packages (both tools):
debian/changelogformat (lines 107, 114)- Debian upstream mmdebstrap (line 132): Markdown CHANGELOG.md format
This aligns with how each upstream maintains their releases.
Note: Lines 112 and 128 hardcode branches (
ubuntu/nobleandmainrespectively) with FIXME comments suggesting future parameterization. Past discussions confirmed the current approach works across releases; this is acknowledged technical debt, not a blocker.Please confirm that the
ubuntu/noblebranch of mmdebstrap can successfully bootstrap other Ubuntu releases (jammy, focal) or if release-specific branching becomes necessary in the future.
156-180: APT cache configuration correctly branched between mmdebstrap and legacy debootstrap.Conditional logic properly applies:
- mmdebstrap (lines 162–175): Uses setup-hooks and customize-hooks for cache management (mkdir creates host+chroot paths, sync-in pre-populates, sync-out saves)
- Legacy debootstrap (lines 177–178): Uses native
--cache-dirand--foreignflagsThe mkdir setup-hook on line 173 correctly creates both the host directory (for sync-in to populate from) and the chroot target directory (for sync-in to write into), aligning with mmdebstrap's documented behavior.
fetch_distro_keyring() is called only for mmdebstrap (line 164, within the non-legacy block), which is correct.
184-207: Clean branching for first-stage execution and second-stage (legacy-only) bootstrap.First-stage execution (lines 184–196):
- mmdebstrap (non-legacy): Direct invocation with accumulated arguments; dynamic error messaging uses LOG_NAME
- debootstrap (legacy): Sets DEBOOTSTRAP_DIR environment variable (for script discovery), invokes, validates marker file, cleans environment after
Second-stage (lines 202–207):
- Correctly limited to legacy debootstrap (mmdebstrap completes in one pass)
- Uses chroot_sdcard to execute
--second-stagein-chrootSeparation of concerns is clear and maintainable.
empty is considered equivalent to "no", which makes sense given it is conceptually NULL, so I think that it shouldn't need to be called out. |
|
Tip For best results, initiate chat on the files or code changes. @tabrisnet That's a fair point. The case statement logic is clear enough as-is, with empty string falling through to the same behavior as "no". Since it's conceptually null/unset, the existing code structure is self-explanatory. I'll respect your decision to keep it as written. 👍 🧠 Learnings used |
allow old debootstrap to be used instead.
tabrisnet
force-pushed
the
tabrisnet_mmdebstrap
branch
from
deed345 to
1bf9cfd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
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 (4)
lib/functions/artifacts/artifact-rootfs.sh(1 hunks)lib/functions/rootfs/create-cache.sh(1 hunks)lib/functions/rootfs/distro-specific.sh(1 hunks)lib/functions/rootfs/rootfs-create.sh(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: tabrisnet
PR: armbian/build#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.
📚 Learning: 2025-10-24T04:46:22.901Z
Learnt from: tabrisnet
PR: armbian/build#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/rootfs/distro-specific.shlib/functions/rootfs/rootfs-create.sh
📚 Learning: 2025-10-24T04:46:22.901Z
Learnt from: tabrisnet
PR: armbian/build#0
File: :0-0
Timestamp: 2025-10-24T04:46:22.901Z
Learning: The `debootstrap` tool doesn't use `apt`, which is important context when reviewing apt-related configuration in rootfs creation code.
Applied to files:
lib/functions/rootfs/rootfs-create.sh
🧬 Code graph analysis (3)
lib/functions/artifacts/artifact-rootfs.sh (3)
lib/functions/artifacts/artifact-armbian-base-files.sh (1)
artifact_armbian-base-files_config_dump(13-16)lib/functions/artifacts/artifact-uboot.sh (1)
artifact_uboot_config_dump(10-17)lib/functions/artifacts/artifact-armbian-desktop.sh (1)
artifact_armbian-desktop_config_dump(10-18)
lib/functions/rootfs/distro-specific.sh (2)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)lib/functions/logging/runners.sh (1)
run_host_command_logged(204-206)
lib/functions/rootfs/rootfs-create.sh (5)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)lib/functions/general/git.sh (1)
fetch_from_repo(80-300)lib/functions/logging/runners.sh (2)
run_host_command_logged(204-206)chroot_sdcard(133-136)lib/functions/rootfs/distro-specific.sh (1)
fetch_distro_keyring(73-172)lib/functions/rootfs/post-tweaks.sh (1)
post_debootstrap_tweaks(10-34)
🔇 Additional comments (12)
lib/functions/artifacts/artifact-rootfs.sh (1)
18-18: Correct variable exposure and caching strategy.The LEGACY_DEBOOTSTRAP variable is properly exposed as a public input with default "no" and correctly excluded from pkgs_hash (which is handled separately in create-cache.sh). This follows the established pattern for artifact inputs.
lib/functions/rootfs/create-cache.sh (1)
31-32: Correct cache key incorporation of LEGACY_DEBOOTSTRAP.The variable is properly lowercased and integrated into the hash_hooks computation, ensuring cache variations are created when the bootstrap mode changes. This aligns with the artifact-rootfs.sh design.
lib/functions/rootfs/distro-specific.sh (5)
69-86: Release classification is comprehensive and includes future Ubuntu releases.The release classification at lines 77–86 correctly categorizes current and upcoming Ubuntu releases (including plucky and raccoon). The Debian classification similarly covers current and upcoming releases.
88-100: Proxy configuration correctly handles all MANAGE_ACNG modes.The proxy setup properly handles three cases: yes (localhost:3142), no/empty (no proxy), and custom URL. Fallthrough pattern is appropriate and well-commented.
102-145: Debian keyring handling robustly manages .pgp/.gpg naming inconsistency.The code correctly handles both debian-archive-keyring.pgp (newer 2025.1+ format) and debian-archive-keyring.gpg (older format) with fallback logic. Cache-hit optimization and dual keyring download (debian-archive and debian-ports) are appropriate.
147-166: Ubuntu keyring extraction and setup-hook registration are correct.The Ubuntu path properly extracts ubuntu-keyring, appends the keyring to debootstrap_arguments for consistency, and manages regional mirror normalization to ensure portability.
170-172: Keyrings are correctly staged for bootstrap injection.Line 170 copies keyrings to the host, and line 171 registers the setup-hook for mmdebstrap to copy them into the target. This two-stage approach is correct and has been verified in build logs.
lib/functions/rootfs/rootfs-create.sh (5)
48-70: Excellent docstring and validation logic.The function docstring clearly documents all major operations, and LEGACY_DEBOOTSTRAP validation properly handles edge cases with sensible defaults. The case-insensitive switch and LOG_NAME derivation work well.
100-139: Per-distribution bootstrap source selection looks solid.The branching properly separates Ubuntu vs Debian and legacy vs non-legacy paths. Version extraction uses appropriate formats for each distribution (parentheses for debian/changelog, markdown for CHANGELOG.md). The hardcoded
ubuntu/noblebranch with FIXME is noted in past comments as acceptable since mmdebstrap reads from debootstrap scripts.
184-196: First-stage execution properly branches by tool.Non-legacy (mmdebstrap) path is direct; legacy path correctly exports
DEBOOTSTRAP_DIRand validates the marker file afterward. Error messages appropriately reference${LOG_NAME}for mmdebstrap and "Debootstrap" in the legacy-only block.
202-209: Second-stage bootstrap correctly scoped to legacy debootstrap only.The conditional guard and chroot_sdcard invocation are appropriate. Final validation of
/bin/bashcorrectly uses${LOG_NAME}for tool-agnostic messaging.
210-355: Post-bootstrap setup and finalization are tool-agnostic.Cleanup, policy diverts, locale configuration, and package installation steps work for both mmdebstrap and debootstrap. Consistent use of
${LOG_NAME}in messaging (lines 211, 214, 355) maintains tool agnosticity throughout.
3 participants
Description
mmdebstrap, related, in part, to #8779Documentation
armbian/documentation#834
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.
tritium-h5bookworm&noble;lepotatotrixietritium-h5imageuefi-loong64on my machine, nor do I own any such machines to test the imagesriscv