[2026.5] - framework rootfs-create: remove LEGACY_DEBOOTSTRAP, use only the upstream mmdebstrap#9512
[2026.5] - framework rootfs-create: remove LEGACY_DEBOOTSTRAP, use only the upstream mmdebstrap#9512tabrisnet wants to merge 1 commit intoarmbian:mainfrom
Conversation
tabrisnet
requested review from
ColorfulRhino and
ShravanSingh64
and removed request for
a team
📝 WalkthroughWalkthroughRemoves LEGACY_DEBOOTSTRAP branching and legacy hash input, consolidating rootfs creation into a unified mmdebstrap-only flow and computing rootfs cache id without the legacy debootstrap flag (only extension hooks and DEST_LANG are hashed). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
size/medium
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/functions/rootfs/rootfs-create.sh (1)
54-88:⚠️ Potential issue | 🟠 MajorPin the mmdebstrap ref or fold its revision into the cache key.
Fetching
branch:mainhere makes the rootfs depend on upstream HEAD, butcalculate_rootfs_cache_id()inlib/functions/rootfs/create-cache.shand the artifact naming inlib/functions/artifacts/artifact-rootfs.shdo not include that fetched revision. That means an upstream mmdebstrap change can produce different rootfs contents under the same cache/OCI tag within the same month. Prefer an immutable tag/commit here, or add the fetched revision to the cache hash before artifact lookup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/functions/rootfs/rootfs-create.sh` around lines 54 - 88, The mmdebstrap ref is set to a floating "branch:main" (mmdebstrap_branch) and is fetched via fetch_from_repo, but the fetched revision (debootstrap_version / contents of debootstrap_wanted_dir) is not folded into the rootfs cache key (calculate_rootfs_cache_id()) or artifact naming, allowing upstream changes to change rootfs under the same tag; fix by either pinning mmdebstrap_branch to an immutable tag/commit before fetch_from_repo or modify cache/ artifact logic to include the fetched revision (for example derive a commit/tag id from debootstrap_wanted_dir/CHANGELOG into debootstrap_version and add that value to calculate_rootfs_cache_id() and artifact-rootfs naming) so the cache id/OCI tag changes when mmdebstrap changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/functions/rootfs/rootfs-create.sh`:
- Around line 96-102: The debootstrap_arguments currently unconditionally
include "'--skip=check/empty'", which suppresses the empty-rootfs guard and
allows leftover files to be baked into the cache; update the logic so that
either (A) remove the "'--skip=check/empty'" element from debootstrap_arguments
and restore the default empty-dir check, or (B) keep the flag only when an
explicit emptiness assertion for SDCARD is performed immediately before calling
mmdebstrap (e.g., ensure create_new_rootfs_cache() asserts SDCARD is empty by
checking the directory contents and failing early if not). Reference:
debootstrap_arguments array, create_new_rootfs_cache(), SDCARD, and mmdebstrap.
- Around line 109-118: The debootstrap/mmdebstrap arguments contain spaces and
shell syntax and are being broken by run_host_command_logged reassembling
arguments; update the mmdebstrap invocation so the array is passed with
array-safe quoting through the runner (use the array expansion form that yields
a single properly quoted string) instead of plain "${debootstrap_arguments[@]}",
or skip the wrapper and invoke "${debootstrap_bin}" directly; modify the call
site that currently calls run_host_command_logged with debootstrap_arguments
(referenced as run_host_command_logged, debootstrap_arguments, debootstrap_bin
and mmdebstrap/mmdebstrap invocation) to use the array-safe expansion
(${debootstrap_arguments[*]@Q} style) or bypass run_host_command_logged for this
command so the hook strings are not re-parsed.
---
Outside diff comments:
In `@lib/functions/rootfs/rootfs-create.sh`:
- Around line 54-88: The mmdebstrap ref is set to a floating "branch:main"
(mmdebstrap_branch) and is fetched via fetch_from_repo, but the fetched revision
(debootstrap_version / contents of debootstrap_wanted_dir) is not folded into
the rootfs cache key (calculate_rootfs_cache_id()) or artifact naming, allowing
upstream changes to change rootfs under the same tag; fix by either pinning
mmdebstrap_branch to an immutable tag/commit before fetch_from_repo or modify
cache/ artifact logic to include the fetched revision (for example derive a
commit/tag id from debootstrap_wanted_dir/CHANGELOG into debootstrap_version and
add that value to calculate_rootfs_cache_id() and artifact-rootfs naming) so the
cache id/OCI tag changes when mmdebstrap changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 19ac3d9f-c2b0-431e-8741-6da4524b7f61
📒 Files selected for processing (2)
lib/functions/rootfs/create-cache.shlib/functions/rootfs/rootfs-create.sh
an interesting idea, but two thoughts... |
|
Tip For best results, initiate chat on the files or code changes.
(a) is a good historical precedent: since the old (b) is the crux of why it's non-trivial: folding the fetched commit hash into the cache key without also surfacing it prominently in the build log (and ideally in the OCI tag) would just produce silent, unexplained rebuilds — arguably worse than the current situation. The right fix is a small, self-contained change that (i) derives the HEAD hash from the checked-out Happy to defer this to a follow-up and let (ﾐ･ω･ﾐ)ﾉ ✏️ Learnings added
🧠 Learnings used |
Another reason it's non-trivial. One has to checkout |
|
Tip For best results, initiate chat on the files or code changes.
The ordering problem in a nutshell:
The mmdebstrap fetch moves from "only on cache miss" to "always, unconditionally" — which is a meaningful regression for any build that would have been a cache hit. Any implementation has to either accept that cost, or find a way to keep the fetch lazy (e.g., a lightweight Pinning Happy to open a follow-up issue to track all three options (pin tag, lazy ✏️ Learnings added
🧠 Learnings used |
I'm not sure I got the intent of @rpardini's griping [please take no offense] about #8785 [which iirc happened on Discord, not on GitHub] correct or not... |
|
Tip For best results, initiate chat on the files or code changes.
To put it bluntly: the simplification argument likely wins here. A few reasons:
The honest residual risk is: an mmdebstrap So I'd say: keep (ﾐ^ω^ﾐ)ﾉ 🧠 Learnings used |
tabrisnet
force-pushed
the
tabrisnet_mmdebstrap
branch
from
bb4339a to
36aec47
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/functions/rootfs/create-cache.sh (1)
31-31: Warn on staleLEGACY_DEBOOTSTRAPconfigs instead of silently ignoring them.
calculate_rootfs_cache_id()no longer varies onLEGACY_DEBOOTSTRAP, while the rootfs path now unconditionally uses mmdebstrap. An old board/user config can therefore keep setting the variable, reuse the same cache ID, and still get mmdebstrap behavior with no signal. A one-time warning/error whenLEGACY_DEBOOTSTRAPis present would make the migration explicit; I’d also drop it fromartifact_rootfs_config_dump()so the artifact metadata stops advertising a dead input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/functions/rootfs/create-cache.sh` at line 31, calculate_rootfs_cache_id() currently ignores LEGACY_DEBOOTSTRAP which causes silent migration to mmdebstrap; modify calculate_rootfs_cache_id() to detect if LEGACY_DEBOOTSTRAP is set and emit a one-time warning (e.g., via echo or logger) informing the user the legacy flag is ignored and mmdebstrap will be used, and remove any reference to LEGACY_DEBOOTSTRAP from artifact_rootfs_config_dump() so the artifact metadata no longer advertises the obsolete input; reference the LEGACY_DEBOOTSTRAP variable and the functions calculate_rootfs_cache_id() and artifact_rootfs_config_dump() when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/functions/rootfs/create-cache.sh`:
- Line 31: calculate_rootfs_cache_id() currently ignores LEGACY_DEBOOTSTRAP
which causes silent migration to mmdebstrap; modify calculate_rootfs_cache_id()
to detect if LEGACY_DEBOOTSTRAP is set and emit a one-time warning (e.g., via
echo or logger) informing the user the legacy flag is ignored and mmdebstrap
will be used, and remove any reference to LEGACY_DEBOOTSTRAP from
artifact_rootfs_config_dump() so the artifact metadata no longer advertises the
obsolete input; reference the LEGACY_DEBOOTSTRAP variable and the functions
calculate_rootfs_cache_id() and artifact_rootfs_config_dump() when making the
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f83cfb84-66af-44ab-9f0c-27417ea7e0fa
📒 Files selected for processing (2)
lib/functions/rootfs/create-cache.shlib/functions/rootfs/rootfs-create.sh
1 participant
Description
so we've done now two releases [
2025.11&2026.2] withmmdebstrapandLEGACY_DEBOOTSTRAPas an option.But... the whole point, at least per @rpardini, of
mmdebstrapwas to get rid of the "use a different tool/branch for debian vs ubuntu".So... let's prepare that and test it.
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.
trixie/forky/jammy/noble/questingandarmhf/arm64/riscv64/loong64. Or at least the arches I have the tooling to build for.loong64withsiddoesn't seem to build at the moment.time nice -n 19 ./compile.sh artifact WHAT=rootfs BOARD=odroidxu4 BRANCH=current BUILD_DESKTOP=no BUILD_MINIMAL=yes RELEASE=trixie KERNEL_CONFIGURE=no KERNEL_GIT=full MANAGE_ACNG='http://squid.tabris.net:3142/' COMPRESS_OUTPUTIMAGE=xz DEB_COMPRESS=xz EXPERT=yesbookworm,forkyworked toojammy,nobleandquestingworked.time nice -n 19 ./compile.sh artifact WHAT=rootfs BOARD=odroidm1 BRANCH=current BUILD_MINIMAL=no BUILD_DESKTOP=yes DESKTOP_ENVIRONMENT=gnome DESKTOP_ENVIRONMENT_CONFIG_NAME=config_base DESKTOP_APPGROUPS_SELECTED= KERNEL_CONFIGURE=no KERNEL_GIT=full MANAGE_ACNG='http://squid.tabris.net:3142/' COMPRESS_OUTPUTIMAGE=xz DEB_COMPRESS=xz EXPERT=yes RELEASE=pluckynoble,jammytime nice -n 19 ./compile.sh artifact WHAT=rootfs BOARD=uefi-riscv64 BRANCH=current BUILD_MINIMAL=no BUILD_DESKTOP=yes DESKTOP_ENVIRONMENT=gnome DESKTOP_ENVIRONMENT_CONFIG_NAME=config_base DESKTOP_APPGROUPS_SELECTED= KERNEL_CONFIGURE=no KERNEL_GIT=full MANAGE_ACNG='http://squid.tabris.net:3142/' COMPRESS_OUTPUTIMAGE=xz DEB_COMPRESS=xz EXPERT=yes RELEASE=plucky[🐳|💥] error! [ Artifact 'full_firmware' is in DONT_BUILD_ARTIFACTS. This usually means that the artifact cache is not being hit because it is outdated. ]Checklist:
Please delete options that are not relevant.
Summary by CodeRabbit