docker: make DOCKER_ARMBIAN_BASE_IMAGE readonly after resolving DOCKER_ARMBIAN_BASE_COORDINATE_PREFIX
#8925
Conversation
…CKER_ARMBIAN_BASE_COORDINATE_PREFIX` - otherwise stuff (eg extensions) might be tempted to change it, but that will only lead to pain
github-actions
bot
added
11
WalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
📜 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.
📒 Files selected for processing (1)
lib/functions/host/docker.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: amazingfate
Repo: armbian/build PR: 8668
File: extensions/ufs.sh:2-2
Timestamp: 2025-09-24T09:54:07.968Z
Learning: In Armbian's build system, DOCKER_ARMBIAN_BASE_IMAGE must be set globally at file load time in extensions, not inside extension_prepare_config functions, because docker operations happen early in the build process and would default to ubuntu:noble if not set before docker_cli_prepare() runs.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8586
File: config/boards/nanopi-r76s.conf:15-21
Timestamp: 2025-09-07T17:39:32.272Z
Learning: In the Armbian build system, the variables $BOARD and $SDCARD are always set by the build framework, so guard checks for these variables are unnecessary in board configuration files and hook functions.
Learnt from: tmshlvck
Repo: armbian/build PR: 8660
File: lib/functions/host/docker.sh:519-529
Timestamp: 2025-09-22T06:01:50.284Z
Learning: In the Armbian build framework's docker.sh, DOCKER_MOUNT_OPTS is conditionally set: empty for Docker, and ",suid,dev" for Podman to handle mount permission requirements.
Learnt from: tmshlvck
Repo: armbian/build PR: 8660
File: lib/functions/host/docker.sh:519-529
Timestamp: 2025-09-22T06:01:50.284Z
Learning: In the Armbian build framework's docker.sh, DOCKER_MOUNT_OPTS is conditionally set: empty for Docker, and ",suid,dev" for Podman to handle mount permission requirements.
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.
📚 Learning: 2025-09-24T09:54:07.968Z
Learnt from: amazingfate
Repo: armbian/build PR: 8668
File: extensions/ufs.sh:2-2
Timestamp: 2025-09-24T09:54:07.968Z
Learning: In Armbian's build system, DOCKER_ARMBIAN_BASE_IMAGE must be set globally at file load time in extensions, not inside extension_prepare_config functions, because docker operations happen early in the build process and would default to ubuntu:noble if not set before docker_cli_prepare() runs.
Applied to files:
lib/functions/host/docker.sh
📚 Learning: 2025-09-22T06:01:50.284Z
Learnt from: tmshlvck
Repo: armbian/build PR: 8660
File: lib/functions/host/docker.sh:519-529
Timestamp: 2025-09-22T06:01:50.284Z
Learning: In the Armbian build framework's docker.sh, DOCKER_MOUNT_OPTS is conditionally set: empty for Docker, and ",suid,dev" for Podman to handle mount permission requirements.
Applied to files:
lib/functions/host/docker.sh
🧬 Code graph analysis (1)
lib/functions/host/docker.sh (1)
lib/functions/cli/cli-docker.sh (2)
cli_docker_run(32-112)cli_docker_pre_run(10-30)
| # Make DOCKER_ARMBIAN_BASE_IMAGE readonly; no changes allowed after this point. | ||
| declare -g -r DOCKER_ARMBIAN_BASE_IMAGE="${DOCKER_ARMBIAN_BASE_IMAGE}" |
There was a problem hiding this comment.
🧩 Analysis chain
Readonly declaration blocks necessary variable reassignment; breaks fallback build path.
Making DOCKER_ARMBIAN_BASE_IMAGE readonly at line 141 prevents the reassignment at line 358 in docker_cli_build_dockerfile, which occurs when the base image is unavailable and a "from scratch" rebuild is required. Bash will reject declare -g DOCKER_ARMBIAN_BASE_IMAGE="..." with "readonly variable" error, breaking the error recovery path.
To fix this, defer the readonly declaration until after all modification paths are complete. One approach:
- # Make DOCKER_ARMBIAN_BASE_IMAGE readonly; no changes allowed after this point.
- declare -g -r DOCKER_ARMBIAN_BASE_IMAGE="${DOCKER_ARMBIAN_BASE_IMAGE}"
-
#############################################################################################################Then, move the readonly declaration to the end of docker_cli_build_dockerfile (after line 361) or into docker_cli_prepare_launch (line 369) where it is actually no longer needed and modifications will not occur. Alternatively, modify line 358 to unset and re-declare, but readonly variables cannot be unset, so you'd need a different approach such as using a temporary variable or accepting the variable may remain mutable through this specific error path.
Which approach is intended for this codebase?
I need to verify the critical claims in the review comment. Let me examine the actual code structure and validate the bug scenario.
🏁 Script executed:
#!/bin/bash
# Examine the docker.sh file to verify the critical bug claim
# Check lines around 140-141 (readonly declaration)
echo "=== Lines 130-150 (readonly declaration area) ==="
sed -n '130,150p' lib/functions/host/docker.sh
echo ""
echo "=== Lines 350-365 (reassignment attempt area) ==="
sed -n '350,365p' lib/functions/host/docker.sh
echo ""
echo "=== Check function boundaries ==="
echo "docker_cli_prepare function start:"
rg -n 'docker_cli_prepare\s*\(\)' lib/functions/host/docker.sh
echo ""
echo "docker_cli_build_dockerfile function start:"
rg -n 'docker_cli_build_dockerfile\s*\(\)' lib/functions/host/docker.shLength of output: 2606
Let me verify the bash behavior and check if there are any workarounds:
🏁 Script executed:
#!/bin/bash
# Test bash behavior with readonly variables and -g flag
echo "=== Test 1: Bash readonly -g reassignment behavior ==="
bash -c '
declare -g -r TEST_VAR="original"
echo "Initial value: $TEST_VAR"
declare -g TEST_VAR="modified" 2>&1 || echo "FAILED as expected"
echo "Value after attempt: $TEST_VAR"
'
echo ""
echo "=== Test 2: Check for unset between lines 141 and 358 ==="
sed -n '141,358p' lib/functions/host/docker.sh | grep -n "unset.*DOCKER_ARMBIAN_BASE_IMAGE" || echo "No unset found"
echo ""
echo "=== Test 3: Check line 359 context - does docker_cli_prepare get called recursively? ==="
sed -n '355,365p' lib/functions/host/docker.sh
echo ""
echo "=== Test 4: Verify readonly persistence across function calls ==="
bash -c '
function first_func() {
declare -g -r PERSISTENT="original"
echo "In first_func: $PERSISTENT (readonly)"
}
function second_func() {
echo "In second_func: $PERSISTENT"
declare -g PERSISTENT="modified" 2>&1 || echo "Cannot modify readonly variable"
}
first_func
second_func
'Length of output: 1141
Readonly declaration blocks necessary variable reassignment; breaks fallback build path.
The critical bug is verified. When docker_cli_prepare makes DOCKER_ARMBIAN_BASE_IMAGE readonly at line 141, the subsequent call to docker_cli_build_dockerfile cannot execute the reassignment at line 358. Bash will reject declare -g DOCKER_ARMBIAN_BASE_IMAGE="${DOCKER_ARMBIAN_BASE_IMAGE_SCRATCH}" with a "readonly variable" error because the readonly flag persists globally across function calls in the same session. This breaks the "from scratch" fallback build path when the base image is unavailable.
The fix requires deferring the readonly declaration until after all modification paths complete. Move the readonly declaration to execute after line 359 (after the recursive docker_cli_prepare call), or to a later function where modifications will not occur. Alternatively, use a separate immutable variable to replace DOCKER_ARMBIAN_BASE_IMAGE after the final assignment, avoiding the need to modify a readonly variable.
There was a problem hiding this comment.
Good catch. This happens for eg when using debian:sid or other images that don't have a prebuilt image available.
There was a problem hiding this comment.
@rpardini Thank you for confirming! Yes, the readonly declaration would break the fallback path for debian:sid and similar images.
Here's the recommended fix approach:
Move the readonly declaration to after the fallback logic completes. The safest placement would be at the end of docker_cli_build_dockerfile (after line 361), right after the recursive docker_cli_prepare call. At that point:
- The base image resolution is complete
- The from-scratch fallback (if needed) has already reassigned
DOCKER_ARMBIAN_BASE_IMAGE - No further modifications will occur
Alternatively, you could place it at the start of docker_cli_prepare_launch (around line 369), since that's where the variable is actually consumed and modifications would truly be unsafe.
Example placement in docker_cli_build_dockerfile:
# After line 361, add:
# Make DOCKER_ARMBIAN_BASE_IMAGE readonly; all modifications complete
declare -g -r DOCKER_ARMBIAN_BASE_IMAGE="${DOCKER_ARMBIAN_BASE_IMAGE}"This ensures the variable becomes immutable only after all legitimate modification paths (including error recovery) have completed.
🧠 Learnings used
Learnt from: amazingfate
Repo: armbian/build PR: 8668
File: extensions/ufs.sh:2-2
Timestamp: 2025-09-24T09:54:07.968Z
Learning: In Armbian's build system, DOCKER_ARMBIAN_BASE_IMAGE must be set globally at file load time in extensions, not inside extension_prepare_config functions, because docker operations happen early in the build process and would default to ubuntu:noble if not set before docker_cli_prepare() runs.
Learnt from: tmshlvck
Repo: armbian/build PR: 8660
File: lib/functions/host/docker.sh:519-529
Timestamp: 2025-09-22T06:01:50.284Z
Learning: In the Armbian build framework's docker.sh, DOCKER_MOUNT_OPTS is conditionally set: empty for Docker, and ",suid,dev" for Podman to handle mount permission requirements.
Learnt from: tmshlvck
Repo: armbian/build PR: 8660
File: lib/functions/host/docker.sh:519-529
Timestamp: 2025-09-22T06:01:50.284Z
Learning: In the Armbian build framework's docker.sh, DOCKER_MOUNT_OPTS is conditionally set: empty for Docker, and ",suid,dev" for Podman to handle mount permission requirements.
Learnt from: amazingfate
Repo: armbian/build PR: 8419
File: lib/functions/configuration/main-config.sh:373-376
Timestamp: 2025-07-23T10:01:36.094Z
Learning: Debian sid (unstable) does not use separate security repositories like stable releases. Security fixes in sid are incorporated directly into regular package updates, so DEBIAN_SECURTY configuration is not needed for sid builds.
Learnt from: amazingfate
Repo: armbian/build PR: 8419
File: lib/functions/configuration/main-config.sh:373-376
Timestamp: 2025-07-23T10:01:33.563Z
Learning: For Debian sid (unstable) builds, no separate security repository is used since security updates are integrated directly into the main unstable repository, unlike stable and testing releases which have dedicated security repositories.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
| fi | ||
|
|
||
| # Make DOCKER_ARMBIAN_BASE_IMAGE readonly; no changes allowed after this point. | ||
| declare -g -r DOCKER_ARMBIAN_BASE_IMAGE="${DOCKER_ARMBIAN_BASE_IMAGE}" |
There was a problem hiding this comment.
Where does this occur in the flow? I get the idea of [user]extensions not rewriting this, but will a board/family be allowed to set this?
HOWEVER, my vision of the idea was that the cmdline could override what the board/family specifies [which would tend to qualify as self-inflicted pain by the operator].
@rpardini @coderabbitai
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where docker_cli_prepare is called to understand the flow
rg -nP '\bdocker_cli_prepare\s*\(' --type=sh -C3
Length of output: 483
🏁 Script executed:
#!/bin/bash
# Find where docker_cli_prepare is actually invoked (not defined)
rg -nP 'docker_cli_prepare\b' --type=sh | grep -v "^[^:]*:.*function docker_cli_prepare"
Length of output: 192
🏁 Script executed:
#!/bin/bash
# Find the CLI entry point to understand the flow better
rg -nP 'cli_docker|docker.*entrypoint' --type=sh -C2
Length of output: 1769
🏁 Script executed:
#!/bin/bash
# Look at the complete cli_docker_pre_run and cli_docker_run functions
cat lib/functions/cli/cli-docker.sh
Length of output: 4719
@tabrisnet, great questions! Let me trace through the flow for you:
Flow Order
The DOCKER_ARMBIAN_BASE_IMAGE readonly declaration occurs in docker_cli_prepare(), which is called very early - before any board/family configs are loaded and even before extensions are fully initialized. Here's the sequence:
- Command line/Environment (
./compile.sh docker ...) cli_docker_run()(inlib/functions/cli/cli-docker.shline 49)docker_cli_prepare()is called, where:- Line 115:
DOCKER_ARMBIAN_BASE_IMAGE="${DOCKER_ARMBIAN_BASE_IMAGE:-"ubuntu:noble"}"- uses existing value if set, otherwise defaults to "ubuntu:noble" - Lines 133-138: May be modified based on
DOCKER_ARMBIAN_BASE_COORDINATE_PREFIX(if using prebuilt Armbian images) - Line 141 (this PR): Made readonly
⚠️ - Line 209+: Extensions are initialized (too late - already readonly!)
- Line 115:
- Docker image is built/pulled and launched
- Board/family configs loaded inside Docker (way too late)
Who Can Set It After This PR?
✅ Command line CAN override: Set the environment variable before calling compile.sh:
DOCKER_ARMBIAN_BASE_IMAGE="debian:bookworm" ./compile.sh docker build ...✅ DOCKER_ARMBIAN_BASE_COORDINATE_PREFIX CAN influence: This variable is checked at lines 133-138 (before readonly), so it can still affect the final value.
❌ Extensions CANNOT set it: The readonly happens at line 141, but extensions aren't initialized until line 209. This is intentional - the PR's purpose is to prevent extensions from changing it.
❌ Board/family CANNOT set it: They're loaded much later, inside the Docker container during the actual build process.
Your "Self-Inflicted Pain" Scenario
Your vision is preserved! The command line can still override what would normally be used:
# Override the base image from command line
DOCKER_ARMBIAN_BASE_IMAGE="ubuntu:jammy" ./compile.sh docker build BOARD=...The readonly protection only prevents downstream code (extensions loaded after docker_cli_prepare) from accidentally or intentionally modifying it, which could cause Docker image mismatches or build failures.
Does this address your concerns about the flow and override capabilities?
🧬 Code Graph Analysis Results
-lib/functions/general/extensions.sh
-
Snippet 1 (summary of enable_extensions_with_hostdeps_builtin_and_user)
Purpose: Enable extensions (built-in and user-provided) that provide host dependencies for Docker-related flows.
Parameters: a list of hook names to search for (e.g., add_host_dependencies, host_dependencies_known); may rely on internal state like SRC, EXTENSION directories.
Returns: exit status 0 on success; non-zero if an error occurs (e.g., unknown extension, file not found).
Behavior details:- Scans extension directories (SRC/extensions and USERPATCHES_PATH/extensions) for files implementing the given hook points (functions named hook_point__).
- Ignores extensions listed in ignore list (e.g., sample-extension).
- Invokes enable_extension on discovered extensions quietly.
- Logs findings and decisions via display_alert.
- Used to dynamically compose host dependency tooling before Docker build steps.
Key implementation notes:
- Collects extension file paths, filters, and then enables each extension.
- Designed to work with both built-in and user-provided extensions.
-
Snippet 2 (summary of initialize_extension_manager)
Purpose: Initialize the extension manager; discover, sort, and wire up extension hook points.
Parameters: none explicit; relies on environment vars like ENABLE_EXTENSIONS, EXT, ENABLE_EXTENSION_TRACE_HINT, and SRC/EXTENSION_MANAGER_TMP_DIR.
Returns: 0 on success; non-zero on failure.
Behavior details:- If ENABLE_EXTENSIONS (or EXT) is provided, auto-enables those extensions on first initialization.
- Creates a temporary directory for extension manager artifacts and a cleanup hook.
- Enumerates all functions that implement extensions hook points (functions named ___), sorts their implementations, and builds wrapper code so each hook point invokes all implementations in the computed order.
- Generates dynamic source files that define the hook wrapper functions and a cleanup script; sources the generated code.
- Logs progress and, if SHOW_EXTENSIONS=yes, can dump the generated code and cleanup for debugging.
- Tracks and reports counts of hook points and implementations processed.
- Returns 0 after successful wiring of all hook points.
Key implementation notes:
- Uses a convention where extension implementations are named as: <hook_point>__<extension_function>.
- Sorting mechanism ensures deterministic ordering, with numeric prefixes used to affect order.
- Exposes a dump-extension-sources facility for debugging.
- Supports both built-in and userpatch extensions, enabling flexible flow customization.
Files/lines (for quick reference):
- lib/functions/general/extensions.sh
- enable_extensions_with_hostdeps_builtin_and_user(): summary above
- initialize_extension_manager(): summary above
Context relative to user comment:
- These extension mechanisms determine how board/family-specified behavior can be augmented or overridden via command-line/env vars (e.g., ENABLE_EXTENSIONS, EXT) and extension implementations. The user’s question about whether a board/family can set or override via cmdline pertains to how EXT/ENABLE_EXTENSIONS influence which extensions run and in what order, as well as how the docker-related flow (in lib/functions/host/docker.sh) interacts with or bypasses those extensions. The summarized snippets describe exactly how extensions are picked, loaded, and ordered, and how the command-line/env-driven overrides can affect the Docker build flow.
🧠 Learnings used
Learnt from: amazingfate
Repo: armbian/build PR: 8668
File: extensions/ufs.sh:2-2
Timestamp: 2025-09-24T09:54:07.968Z
Learning: In Armbian's build system, DOCKER_ARMBIAN_BASE_IMAGE must be set globally at file load time in extensions, not inside extension_prepare_config functions, because docker operations happen early in the build process and would default to ubuntu:noble if not set before docker_cli_prepare() runs.
Learnt from: tmshlvck
Repo: armbian/build PR: 8660
File: lib/functions/host/docker.sh:519-529
Timestamp: 2025-09-22T06:01:50.284Z
Learning: In the Armbian build framework's docker.sh, DOCKER_MOUNT_OPTS is conditionally set: empty for Docker, and ",suid,dev" for Podman to handle mount permission requirements.
Learnt from: tmshlvck
Repo: armbian/build PR: 8660
File: lib/functions/host/docker.sh:519-529
Timestamp: 2025-09-22T06:01:50.284Z
Learning: In the Armbian build framework's docker.sh, DOCKER_MOUNT_OPTS is conditionally set: empty for Docker, and ",suid,dev" for Podman to handle mount permission requirements.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8537
File: config/boards/qcom-robotics-rb5.conf:1-1
Timestamp: 2025-09-18T03:36:17.862Z
Learning: In Armbian board configuration files, .wip and .conf extensions require BOARD_MAINTAINER to be present and contain at least one maintainer. If no maintainer is present, the board support rules state it does not qualify for standard support and must be moved to community support (.csc extension). This is documented at https://docs.armbian.com/User-Guide_Board-Support-Rules/
Learnt from: EvilOlaf
Repo: armbian/build PR: 8754
File: config/boards/bestv-r3300-l.csc:14-16
Timestamp: 2025-10-14T05:08:11.785Z
Learning: In the Armbian build system, BOOTBRANCH_BOARD is a valid framework variable used as a fallback when BOOTBRANCH is unset. The framework checks BOOTBRANCH_BOARD before applying the default bootloader branch value (see config/sources/common.conf). Board configuration files can use BOOTBRANCH_BOARD to specify the bootloader branch.
Learnt from: igorpecovnik
Repo: armbian/build PR: 8849
File: config/boards/radxa-e54c.csc:14-28
Timestamp: 2025-11-02T20:49:56.719Z
Learning: In Armbian board configuration files (config/boards/*.conf, *.csc, etc.), do not use kernel_config_set, kernel_config_set_m, kernel_config_set_y, or custom_kernel_config__* functions to modify kernel configuration. Kernel configuration is associated with LINUXFAMILY/BOARDFAMILY, not individual BOARD. Board-specific kernel modifications cause inconsistency in kernel packages published to the apt repository because boards within a family share the same kernel packages. Kernel configuration changes must be made in the appropriate kernel config file (e.g., config/kernel/linux-*-*.config) or in family configuration files (config/sources/families/*.conf, *.inc) instead.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8298
File: extensions/https_proxy.sh:1-9
Timestamp: 2025-06-14T05:53:10.627Z
Learning: Armbian build framework extensions are NOT designed to run standalone but are called by the supervisor. The framework provides environment variables like USERPATCHES_PATH and SDCARD, and commands are executed with protection via run_host_command_logged and chroot_sdcard functions that use bash -e -o pipefail.
Learnt from: HackingGate
Repo: armbian/build PR: 8665
File: config/boards/photonicat2.csc:4-4
Timestamp: 2025-10-26T10:41:35.118Z
Learning: In the Armbian build system, rk3576 boards consistently use BOARDFAMILY="rk35xx" for both vendor and edge kernel targets. The rk35xx family configuration sources rockchip64_common.inc, which provides edge and current kernel branch definitions, making these branches available even though they're not defined directly in rk35xx.conf.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
There was a problem hiding this comment.
good that the CLI can override it, BUT the board probably should be able to as well... OTOH as you noted the board gets loaded late. This feels like a problem.
BUT I think a solution would be a documented pattern where the board gets to object. But this then makes the CLI override difficult.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@tabrisnet, I'm back! 🐰
You've identified an interesting tension in the architecture. Let me explain the design rationale and propose a solution:
Why Boards Load Late (And Why That's Actually Correct)
The DOCKER_ARMBIAN_BASE_IMAGE defines the build environment (what OS/tooling we build from), while board configs define target properties (what kernel, bootloader, packages the resulting image has). These are architecturally separate concerns:
- Docker base image decision → Must happen BEFORE Docker launches (host-side)
- Board configuration → Happens INSIDE Docker (guest-side), defining the artifact being built
The Solution: Board-Specific Extensions (Already Exists!)
From our retrieved learning, there's already a documented pattern for this:
"DOCKER_ARMBIAN_BASE_IMAGE must be set globally at file load time in extensions, not inside extension_prepare_config functions"
So if a board truly needs a specific Docker base image (e.g., a weird architecture needs a custom toolchain only available in Debian Trixie), you can:
Option 1: Create a Board-Specific Extension
# userpatches/extensions/myboard-docker-base.sh
DOCKER_ARMBIAN_BASE_IMAGE="debian:trixie" # Set at file load time (before docker_cli_prepare)
function myboard_docker_base__add_host_dependencies() {
# Hook function to ensure extension loads early enough
: # no-op
}Then enable it:
ENABLE_EXTENSIONS="myboard-docker-base" ./compile.sh docker build BOARD=myboard ...Option 2: Use DOCKER_ARMBIAN_BASE_COORDINATE_PREFIX
If you just need a different base from the Armbian registry:
DOCKER_ARMBIAN_BASE_COORDINATE_PREFIX="myregistry.io/custom-" ./compile.sh docker build ...The CLI Override Challenge You Mentioned
You said: "a solution would be a documented pattern where the board gets to object. But this then makes the CLI override difficult."
Here's how it could work with precedence:
- CLI wins (explicit user intent):
DOCKER_ARMBIAN_BASE_IMAGE="debian:sid" ./compile.sh ... - Board-extension suggests (via extension loaded for that board)
- Framework defaults (ubuntu:noble)
This is actually already how it works! The line at 115 uses: "${DOCKER_ARMBIAN_BASE_IMAGE:-"ubuntu:noble"}" - if already set (CLI), it won't override.
Does This PR Break Anything?
No - extensions that set DOCKER_ARMBIAN_BASE_IMAGE at file load time still work. The readonly just prevents later code (after docker_cli_prepare) from changing it, which is the intended behavior.
Questions for You
- Do you have a specific board/use case where the default base image doesn't work?
- Would you prefer explicit documentation of the extension pattern for board-specific Docker bases?
- Or do you see a fundamental architectural issue I'm missing?
Let me know your thoughts! 🤔
🧠 Learnings used
Learnt from: amazingfate
Repo: armbian/build PR: 8668
File: extensions/ufs.sh:2-2
Timestamp: 2025-09-24T09:54:07.968Z
Learning: In Armbian's build system, DOCKER_ARMBIAN_BASE_IMAGE must be set globally at file load time in extensions, not inside extension_prepare_config functions, because docker operations happen early in the build process and would default to ubuntu:noble if not set before docker_cli_prepare() runs.
Learnt from: tmshlvck
Repo: armbian/build PR: 8660
File: lib/functions/host/docker.sh:519-529
Timestamp: 2025-09-22T06:01:50.284Z
Learning: In the Armbian build framework's docker.sh, DOCKER_MOUNT_OPTS is conditionally set: empty for Docker, and ",suid,dev" for Podman to handle mount permission requirements.
Learnt from: tmshlvck
Repo: armbian/build PR: 8660
File: lib/functions/host/docker.sh:519-529
Timestamp: 2025-09-22T06:01:50.284Z
Learning: In the Armbian build framework's docker.sh, DOCKER_MOUNT_OPTS is conditionally set: empty for Docker, and ",suid,dev" for Podman to handle mount permission requirements.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
There was a problem hiding this comment.
@coderabbitai
We have known cases where uboot doesn't build [correctly] on noble but does on jammy; work is in progress to try to fix this but there are limits on time/effort & spoons in the community.
DOCKER_ARMBIAN_BASE_COORDINATE_PREFIX is a development option, but only adds complexity for already barely supported boards due to old toolchains
There was a problem hiding this comment.
@tabrisnet, thank you for that concrete use case! That's exactly the kind of real-world scenario that needs a clean solution. Let me propose something practical:
The Problem Is Real
U-Boot not building on noble but working on jammy is a legitimate architectural constraint that some boards have. You shouldn't need to maintain complex workarounds or fork the framework.
Proposed Simple Solution: Lightweight Board Extension
Here's a pattern that's deliberately simple for barely-supported boards:
Create: userpatches/extensions/boards-jammy.sh
#!/usr/bin/env bash
# Simple extension: Force jammy for boards where U-Boot doesn't build on noble
# Boards affected: [list your boards here]
DOCKER_ARMBIAN_BASE_IMAGE="ubuntu:jammy"
# Minimal hook to ensure this extension loads early
function boards_jammy__add_host_dependencies() {
: # no-op - we just need to hook early
}Use it:
ENABLE_EXTENSIONS="boards-jammy" ./compile.sh docker build BOARD=myboard ...Or add to your board's documentation/wiki:
This board requires jammy. Build with:
ENABLE_EXTENSIONS="boards-jammy" ./compile.sh docker BOARD=...
Why This Works (And Why This PR Doesn't Break It)
- ✅ The extension file is read before
docker_cli_prepare()runs (line 209 in docker.sh) - ✅ Setting
DOCKER_ARMBIAN_BASE_IMAGEat file load time (top of extension) happens before the readonly declaration (line 141) - ✅ This PR prevents later code from accidentally changing it back to noble
- ✅ Single extension file can handle multiple boards
- ✅ CLI can still override:
DOCKER_ARMBIAN_BASE_IMAGE="debian:bookworm" ENABLE_EXTENSIONS="boards-jammy" ./compile.sh ...(CLI wins)
Alternative: Auto-Enable Based on Board
If you want it even simpler, you could modify the framework to auto-detect. For example, in lib/functions/cli/cli-docker.sh before calling docker_cli_prepare():
# Auto-enable jammy for boards known to need it
case "${BOARD}" in
board1|board2|board3)
[[ -z "${DOCKER_ARMBIAN_BASE_IMAGE}" ]] && DOCKER_ARMBIAN_BASE_IMAGE="ubuntu:jammy"
;;
esacBut this requires framework changes and couples board quirks to core code.
What Would Help Most?
Would it help if we:
- Document this pattern in the wiki/docs with a template extension?
- Create a standard extension like
legacy-toolchain-jammy.shthat boards can just reference? - Add framework support for board configs to declare Docker requirements in metadata (but this requires more work)?
Let me know which direction would reduce complexity for your use case! 🛠️
✏️ Learnings added
Learnt from: tabrisnet
Repo: armbian/build PR: 8925
File: lib/functions/host/docker.sh:141-141
Timestamp: 2025-11-13T14:50:44.651Z
Learning: In the Armbian build system, certain boards have U-Boot that doesn't build correctly on ubuntu:noble but works on ubuntu:jammy. For these boards, DOCKER_ARMBIAN_BASE_IMAGE can be set to "ubuntu:jammy" via an extension file loaded before docker_cli_prepare() runs, allowing legacy toolchains to work while the community works on proper fixes with limited resources.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: amazingfate
Repo: armbian/build PR: 8668
File: extensions/ufs.sh:2-2
Timestamp: 2025-09-24T09:54:07.968Z
Learning: In Armbian's build system, DOCKER_ARMBIAN_BASE_IMAGE must be set globally at file load time in extensions, not inside extension_prepare_config functions, because docker operations happen early in the build process and would default to ubuntu:noble if not set before docker_cli_prepare() runs.
Learnt from: tmshlvck
Repo: armbian/build PR: 8660
File: lib/functions/host/docker.sh:519-529
Timestamp: 2025-09-22T06:01:50.284Z
Learning: In the Armbian build framework's docker.sh, DOCKER_MOUNT_OPTS is conditionally set: empty for Docker, and ",suid,dev" for Podman to handle mount permission requirements.
Learnt from: tmshlvck
Repo: armbian/build PR: 8660
File: lib/functions/host/docker.sh:519-529
Timestamp: 2025-09-22T06:01:50.284Z
Learning: In the Armbian build framework's docker.sh, DOCKER_MOUNT_OPTS is conditionally set: empty for Docker, and ",suid,dev" for Podman to handle mount permission requirements.
Learnt from: rpardini
Repo: armbian/build PR: 8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:76-86
Timestamp: 2025-03-31T22:20:48.475Z
Learning: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions (such as between 2025.01 and 2025.04) is prioritized over refactoring individual patches for code improvements.
Learnt from: rpardini
Repo: armbian/build PR: 8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:73-75
Timestamp: 2025-03-31T22:20:41.849Z
Learning: When porting patches between U-Boot versions (like from 2025.01 to 2025.04), rpardini prefers to maintain patches as-is rather than introducing refactoring changes, even when potential improvements are identified. This approach prioritizes consistency and reduces the risk of introducing new issues.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
Learnt from: amazingfate
Repo: armbian/build PR: 8619
File: config/sources/families/rockchip.conf:222-230
Timestamp: 2025-09-14T06:32:29.806Z
Learning: In the Armbian build system, the write_uboot_platform() function implementations follow different patterns across Rockchip family files. The newer standard (used in rockchip64_common.inc and rk3506) includes 'status=none' parameter in dd commands, while older implementations (rk3288, rk322x) use an older pattern without this parameter. The rk3506 implementation correctly follows the current Rockchip family standard.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8754
File: config/boards/bestv-r3300-l.csc:14-16
Timestamp: 2025-10-14T05:08:11.785Z
Learning: In the Armbian build system, BOOTBRANCH_BOARD is a valid framework variable used as a fallback when BOOTBRANCH is unset. The framework checks BOOTBRANCH_BOARD before applying the default bootloader branch value (see config/sources/common.conf). Board configuration files can use BOOTBRANCH_BOARD to specify the bootloader branch.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8417
File: config/boards/orangepi5pro.csc:57-58
Timestamp: 2025-07-23T07:30:52.265Z
Learning: In the Armbian build system, BOOTPATCHDIR can contain board-specific subdirectories (e.g., board_orangepi5pro) for applying patches to specific boards only. The framework automatically checks if such board-specific subdirectories exist for the board being built and applies those patches accordingly.
Learnt from: Grippy98
Repo: armbian/build PR: 8622
File: config/sources/families/k3.conf:66-66
Timestamp: 2025-09-12T19:28:38.491Z
Learning: In the Armbian k3 family build system (config/sources/families/k3.conf), builds do not fail when TIBOOT3_BOOTCONFIG is unset, even though tiboot3.bin is still listed in UBOOT_TARGET_MAP. The gating mechanism in pre_config_uboot_target__build_first_stage function works as intended to conditionally build/copy tiboot3.bin only when TIBOOT3_BOOTCONFIG is defined.
Learnt from: amazingfate
Repo: armbian/build PR: 8419
File: lib/functions/host/prepare-host.sh:272-275
Timestamp: 2025-07-23T10:01:41.310Z
Learning: In the Armbian build system, the design philosophy is to fail fast when host dependencies are not met rather than gracefully skipping unsupported architectures. This ensures build environment consistency and prevents silent failures. Host dependency checks should be explicit and non-negotiable.
Learnt from: pyavitz
Repo: armbian/build PR: 8388
File: config/boards/kickpik2b.conf:1-14
Timestamp: 2025-07-17T04:12:33.125Z
Learning: In the Armbian build system, board configuration files (*.conf) follow a project standard of not including shebang lines, even though they contain bash code and are sourced by the build system. This is an established Armbian convention that individual contributors are expected to follow, and changes to this standard would require broader project maintainer approval.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
|
This whole thread with the rabbit hallucinating is pointless. The reality is that (as the docs detail) extensions should only contain functions and not set variables. Unfortunately there's no way we can enforce that extensions won't do changes to global vars out of function context. Boards/extensions are not supposed to be able to change the Docker base image; that's supposed to happen only at matrix generation time (and thus passed via command-line), or using a config file. |
igorpecovnik
added
02
3 participants
Summary by CodeRabbit