-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
docker: make DOCKER_ARMBIAN_BASE_IMAGE readonly after resolving DOCKER_ARMBIAN_BASE_COORDINATE_PREFIX
#8925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -137,6 +137,9 @@ function docker_cli_prepare() { | |
| display_alert "Using prebuilt Armbian image as base for '${wanted_os_tag}-${DOCKER_WANTED_RELEASE}'" "DOCKER_ARMBIAN_BASE_IMAGE: ${DOCKER_ARMBIAN_BASE_IMAGE}" "info" | ||
| fi | ||
|
|
||
| # Make DOCKER_ARMBIAN_BASE_IMAGE readonly; no changes allowed after this point. | ||
| declare -g -r DOCKER_ARMBIAN_BASE_IMAGE="${DOCKER_ARMBIAN_BASE_IMAGE}" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: Length of output: 483 🏁 Script executed: Length of output: 192 🏁 Script executed: Length of output: 1769 🏁 Script executed: Length of output: 4719 @tabrisnet, great questions! Let me trace through the flow for you: Flow OrderThe
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" ScenarioYour 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
Files/lines (for quick reference):
Context relative to user comment:
🧠 Learnings used
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. get back here @coderabbitai.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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
The Solution: Board-Specific Extensions (Already Exists!)From our retrieved learning, there's already a documented pattern for this:
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_PREFIXIf 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 MentionedYou 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:
This is actually already how it works! The line at 115 uses: Does This PR Break Anything?No - extensions that set Questions for You
Let me know your thoughts! 🤔 🧠 Learnings used
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 RealU-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 ExtensionHere's a pattern that's deliberately simple for barely-supported boards: Create:
|
||
|
|
||
| ############################################################################################################# | ||
| # Stop here if Docker can't be used at all. | ||
| if ! is_docker_ready_to_go; then | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Readonly declaration blocks necessary variable reassignment; breaks fallback build path.
Making
DOCKER_ARMBIAN_BASE_IMAGEreadonly at line 141 prevents the reassignment at line 358 indocker_cli_build_dockerfile, which occurs when the base image is unavailable and a "from scratch" rebuild is required. Bash will rejectdeclare -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:
Then, move the readonly declaration to the end of
docker_cli_build_dockerfile(after line 361) or intodocker_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:
Length of output: 2606
Let me verify the bash behavior and check if there are any workarounds:
🏁 Script executed:
Length of output: 1141
Readonly declaration blocks necessary variable reassignment; breaks fallback build path.
The critical bug is verified. When
docker_cli_preparemakesDOCKER_ARMBIAN_BASE_IMAGEreadonly at line 141, the subsequent call todocker_cli_build_dockerfilecannot execute the reassignment at line 358. Bash will rejectdeclare -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_preparecall), or to a later function where modifications will not occur. Alternatively, use a separate immutable variable to replaceDOCKER_ARMBIAN_BASE_IMAGEafter the final assignment, avoiding the need to modify a readonly variable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 recursivedocker_cli_preparecall. At that point:DOCKER_ARMBIAN_BASE_IMAGEAlternatively, 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:This ensures the variable becomes immutable only after all legitimate modification paths (including error recovery) have completed.
🧠 Learnings used