Conversation
WalkthroughAdds a new generic U-Boot bootscript template and its documentation; implements template-aware rendering and verification for bootscripts in install and compile flows; introduces helper functions for bootscript rendering; adds gettext to host dependencies; and makes numerous unrelated config, kernel, workflow, and packaging edits across board, family, kernel, extension, and packaging files. Changes
Sequence Diagram(s)sequenceDiagram
participant Installer as install_distribution_agnostic
participant Compiler as compile_armbian-bsp-cli
participant Renderer as render_bootscript_template
participant Verifier as proof_rendered_bootscript_template
participant FS as Target FS
Note over Installer,Compiler: bootscript deployment path
Installer->>Renderer: template path provided (.template)
Renderer-->>Installer: rendered content (envsubst)
Installer->>FS: write rendered bootscript
Installer->>Verifier: verify rendered file
alt verified
Verifier-->>Installer: OK
else failed
Verifier-->>Installer: FAIL (exit)
end
Note over Compiler: compile flow uses same helpers
Compiler->>Renderer: render when source ends with `.template`
Renderer-->>Compiler: rendered content
Compiler->>FS: write rendered bootscript
Compiler->>Verifier: verify rendered file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
08
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/functions/rootfs/distro-agnostic.sh (1)
54-68: Template rendering function: consider shellcheck annotations.
render_bootscript_templatecorrectly exportsBOOTSCRIPT_TEMPLATE__CREATE_DATEand console vars, then invokesenvsubst. To silence SC2034 (unused var) and SC2046 (word splitting onexport $(…)), add above the function:# shellcheck disable=SC2034,SC2046
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/bootscripts/boot-generic.cmd.template(1 hunks)lib/functions/bsp/armbian-bsp-cli-deb.sh(1 hunks)lib/functions/host/prepare-host.sh(1 hunks)lib/functions/rootfs/distro-agnostic.sh(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
lib/functions/rootfs/distro-agnostic.sh
[warning] 58-58: BOOTSCRIPT_TEMPLATE__CREATE_DATE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 66-66: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Shell script analysis
- GitHub Check: Check kernel security options
🔇 Additional comments (11)
lib/functions/host/prepare-host.sh (1)
189-189: Add gettext to host dependencies: Looks correct.
Includinggettextensures thatenvsubstis available for template rendering. Verify that the package name matches the target distributions (e.g., Debian’sgettext-basevs.gettext).lib/functions/bsp/armbian-bsp-cli-deb.sh (2)
145-153: Implement template rendering for.templatebootscripts.
Good use ofrender_bootscript_templateand validation viaproof_rendered_bootscript_templateto fail fast on unrendered placeholders.
155-159: Retain direct copy path for non-template bootscripts.
The fallback tocp -pvpreserves existing behavior for plain bootscripts.lib/functions/rootfs/distro-agnostic.sh (4)
10-30: New console export helpers look solid.
bootscript_export_display_consoleandbootscript_export_serial_consolecorrectly assemble console arguments and export them underBOOTSCRIPT_TEMPLATE__DISPLAY_CONSOLEandBOOTSCRIPT_TEMPLATE__SERIAL_CONSOLE.
70-72: Proof-rendering verification is concise and accurate.
The grep against${BOOTSCRIPT_TEMPLATE__ensures no placeholders remain.
182-191: Resolve bootscript source path with patch override support.
Thebootscript_src_pathselection betweenUSERPATCHES_PATHand defaultSRC/config/bootscriptsis correct.
226-235: Enable templating in rootfs installation.
Differentiates.templatesuffix, renders viarender_bootscript_template, and enforces completeness withproof_rendered_bootscript_template.config/bootscripts/boot-generic.cmd.template (4)
1-16: Template header and documentation are clear.
The introductory comments outline templating expectations and references to implementing scripts.
24-29: Default environment variable placeholders.
ALIGN_TO,DISPLAY_CONSOLE, andSERIAL_CONSOLEplaceholders are correctly positioned for substitution.
33-46: Board, rootfs, load address, and vendor placeholders.
Use of${BOOTSCRIPT_TEMPLATE__BOARD_FAMILY},${BOOTSCRIPT_TEMPLATE__ROOTFS_TYPE},${BOOTSCRIPT_TEMPLATE__LOAD_ADDR}, and${BOOTSCRIPT_TEMPLATE__BOARD_VENDOR}is appropriate and aligns with export functions.
49-52: Templating termination marker.
The# NOTEblock correctly signals that no furtherBOOTSCRIPT_TEMPLATE__variables should appear beyond this point.
|
Hi @The-going, |
djurny
force-pushed
the
feat/bootscript-templating
branch
from
5f93a68 to
7d42abc
Compare
github-actions
bot
removed
the
Ready to merge
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/functions/rootfs/distro-agnostic.sh (1)
32-52: Duplicate logic: see previous commentThe
bootscript_export_serial_consolefunction mirrors the display-console one. Apply the same refactoring and quoting recommendations above to consolidate and secure the code.
🧹 Nitpick comments (4)
lib/functions/rootfs/distro-agnostic.sh (4)
10-30: Refactor duplicated console-export logic and harden variable handlingThe two functions for display- and serial-console exports share nearly identical loops and use unquoted expansions (
${DISPLAYCON//,/ },$SRC_CMDLINE) that can lead to word‐splitting or globbing. Consider extracting a generic helper (e.g.bootscript_export_console VAR_NAME INPUT_LIST FILTER_REGEX) to DRY up both functions. Also quote parameter expansions to prevent unintended splitting.
70-72: Suppress grep output and use exit codes
proof_rendered_bootscript_templateshould usegrep -qto avoid printing matches and make its intent clearer. For example:-function proof_rendered_bootscript_template() { - ! egrep '\$\{?BOOTSCRIPT_TEMPLATE__' "${1:?}" -} +function proof_rendered_bootscript_template() { + if egrep -q '\$\{?BOOTSCRIPT_TEMPLATE__' "${1:?}"; then + return 1 + fi + return 0 +}
182-190: Combine local declarations for clarityGroup related locals into a single declaration to improve readability and reduce boilerplate. Example:
- local bootscript_src_path - local bootscript_src=${BOOTSCRIPT%%:*} - local bootscript_dst=${BOOTSCRIPT##*:} + local bootscript_src_path bootscript_src bootscript_dst + bootscript_src=${BOOTSCRIPT%%:*} + bootscript_dst=${BOOTSCRIPT##*:}
227-240: Avoid useless use ofcatYou can pipe the template directly into
render_bootscript_templatewithoutcat:- run_host_command_logged cat "${bootscript_src_path}/${bootscript_src}" \ - | render_bootscript_template > "${SDCARD}/boot/${bootscript_dst}" + run_host_command_logged render_bootscript_template \ + < "${bootscript_src_path}/${bootscript_src}" \ + > "${SDCARD}/boot/${bootscript_dst}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/bootscripts/boot-generic.cmd.template(1 hunks)lib/functions/bsp/armbian-bsp-cli-deb.sh(1 hunks)lib/functions/host/prepare-host.sh(1 hunks)lib/functions/rootfs/distro-agnostic.sh(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/functions/host/prepare-host.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/functions/bsp/armbian-bsp-cli-deb.sh
- config/bootscripts/boot-generic.cmd.template
🧰 Additional context used
🪛 Shellcheck (0.10.0)
lib/functions/rootfs/distro-agnostic.sh
[warning] 58-58: BOOTSCRIPT_TEMPLATE__CREATE_DATE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 66-66: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Shell script analysis
Enable bootscript templating using `envsubst`. Add generic bootscript.
djurny
force-pushed
the
feat/bootscript-templating
branch
from
7d42abc to
4b06fe4
Compare
github-actions
bot
removed
the
Ready to merge
coderabbitai
bot
added
05
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/bootscripts/boot-generic.cmd.template(1 hunks)lib/functions/bsp/armbian-bsp-cli-deb.sh(1 hunks)lib/functions/host/prepare-host.sh(1 hunks)lib/functions/rootfs/distro-agnostic.sh(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/functions/host/prepare-host.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/functions/bsp/armbian-bsp-cli-deb.sh
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: djurny
PR: armbian/build#8315
File: lib/functions/rootfs/distro-agnostic.sh:54-68
Timestamp: 2025-06-22T00:45:26.182Z
Learning: In Armbian build system, external command dependencies like envsubst are handled by adding the required packages (e.g., gettext for envsubst) to host_dependencies in adaptative_prepare_host_dependencies function in lib/functions/host/prepare-host.sh, rather than checking command availability at runtime.
Learnt from: djurny
PR: armbian/build#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.
Learnt from: EvilOlaf
PR: armbian/build#8328
File: lib/functions/compilation/patch/drivers_network.sh:542-545
Timestamp: 2025-06-24T10:08:40.313Z
Learning: In the Armbian build system, when a PR removes build support for a specific kernel version, version check issues for that removed version become practically irrelevant even if they appear incorrect in isolation. Context about which kernel versions are being deprecated/removed is important for understanding the impact of version-related code changes.
Learnt from: rpardini
PR: armbian/build#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: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:182-186
Timestamp: 2025-06-04T23:45:38.860Z
Learning: In config/bootscripts/boot-mvebu.cmd, the `fdtfile` variable is mandatory for booting and is pre-set by U-Boot, but can be overridden via armbianEnv.txt. If `fdtfile` is empty, the subsequent device tree file search logic will eventually fail and trigger the critical error "Cannot find DT!" with proper error handling.
Learnt from: djurny
PR: armbian/build#8203
File: config/bootscripts/boot-sunxi.cmd:111-114
Timestamp: 2025-05-17T23:08:31.746Z
Learning: In U-Boot scripts, the `setexpr sub` command is valid for string operations when CONFIG_REGEX=y is enabled. This allows for pattern substitution in strings, as documented in https://docs.u-boot.org/en/latest/usage/cmd/setexpr.html. The syntax is `setexpr <variable> sub <pattern> <replacement> <string>`.
Learnt from: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:182-186
Timestamp: 2025-06-04T23:45:38.860Z
Learning: In U-Boot bootscripts, the `setexpr` command will not fail if a variable is empty - it simply won't perform any substitution operations and leaves the target variable unchanged.
Learnt from: djurny
PR: armbian/build#8287
File: config/bootscripts/boot-sunxi.cmd:38-44
Timestamp: 2025-06-12T21:14:36.024Z
Learning: In config/bootscripts/boot-sunxi.cmd the unconditional "+1" increment in func_align_addr_next (when align_overlap_oboe_avoidance="on") is intentional. It compensates for a known off-by-one error in U-Boot where the end address is calculated as start+size instead of start+size-1, so the extra page prevents overlap. This behavior should not be “optimized away”.
Learnt from: rpardini
PR: armbian/build#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: djurny
PR: armbian/build#8166
File: config/bootscripts/boot-mvebu.cmd:38-50
Timestamp: 2025-05-07T20:49:40.969Z
Learning: In U-Boot scripts, `itest.s` should be used for explicit string comparisons rather than plain `itest` with string operators like `==`.
Learnt from: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:61-63
Timestamp: 2025-06-04T23:52:39.493Z
Learning: In U-Boot shell scripting, the `else ;` construct followed by commands on subsequent lines is valid syntax, as demonstrated by empirical testing. Both `else command` and `else ; command` forms are acceptable in U-Boot's shell implementation.
config/bootscripts/boot-generic.cmd.template (13)
Learnt from: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:182-186
Timestamp: 2025-06-04T23:45:38.860Z
Learning: In config/bootscripts/boot-mvebu.cmd, the `fdtfile` variable is mandatory for booting and is pre-set by U-Boot, but can be overridden via armbianEnv.txt. If `fdtfile` is empty, the subsequent device tree file search logic will eventually fail and trigger the critical error "Cannot find DT!" with proper error handling.
Learnt from: djurny
PR: armbian/build#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.
Learnt from: djurny
PR: armbian/build#8287
File: config/bootscripts/boot-sunxi.cmd:38-44
Timestamp: 2025-06-12T21:14:36.024Z
Learning: In config/bootscripts/boot-sunxi.cmd the unconditional "+1" increment in func_align_addr_next (when align_overlap_oboe_avoidance="on") is intentional. It compensates for a known off-by-one error in U-Boot where the end address is calculated as start+size instead of start+size-1, so the extra page prevents overlap. This behavior should not be “optimized away”.
Learnt from: rpardini
PR: armbian/build#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: djurny
PR: armbian/build#8287
File: config/bootscripts/boot-sunxi.cmd:13-23
Timestamp: 2025-06-12T21:08:11.318Z
Learning: In config/bootscripts/boot-sunxi.cmd the variable name `align_overlap_oboe_avoidance` intentionally uses the acronym “OBOE” (Off-By-One Error); it is not a typo and should not be renamed.
Learnt from: EvilOlaf
PR: armbian/build#0
File: :0-0
Timestamp: 2025-06-16T03:24:00.458Z
Learning: In Armbian board configuration files (.csc), the standard pattern is to have only one line as a comment describing the board hardware specifications. This single-line description typically includes the SoC model, core count, RAM options, and key features like connectivity options, storage interfaces, and special features.
Learnt from: djurny
PR: armbian/build#8166
File: config/bootscripts/boot-mvebu.cmd:38-50
Timestamp: 2025-05-07T20:49:40.969Z
Learning: The 4KB (0x1000) boundary used for device tree size calculations in U-Boot scripts is specific to how the `fdt resize` command works internally and should remain hardcoded rather than being tied to adjustable alignment variables.
Learnt from: Grippy98
PR: armbian/build#8202
File: config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh:2-3
Timestamp: 2025-05-16T15:34:34.672Z
Learning: The shell scripts in the Armbian build system's desktop configuration files, such as config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh, rely on environment variables like $SRC and $destination that are expected to be set externally before the script is executed.
Learnt from: rpardini
PR: armbian/build#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: djurny
PR: armbian/build#8315
File: lib/functions/rootfs/distro-agnostic.sh:54-68
Timestamp: 2025-06-22T00:45:26.182Z
Learning: In Armbian build system, external command dependencies like envsubst are handled by adding the required packages (e.g., gettext for envsubst) to host_dependencies in adaptative_prepare_host_dependencies function in lib/functions/host/prepare-host.sh, rather than checking command availability at runtime.
Learnt from: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:182-186
Timestamp: 2025-06-04T23:45:38.860Z
Learning: In U-Boot bootscripts, the `setexpr` command will not fail if a variable is empty - it simply won't perform any substitution operations and leaves the target variable unchanged.
Learnt from: EvilOlaf
PR: armbian/build#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: EvilOlaf
PR: armbian/build#8298
File: extensions/https_proxy.sh:1-9
Timestamp: 2025-06-14T05:53:10.627Z
Learning: In Armbian build framework, extensions are loaded and executed via lib/functions/general/extensions.sh using call_extension_method. The framework sets set -o errexit and set -o errtrace globally, and individual commands run with pipefail protection through the command execution functions.
lib/functions/rootfs/distro-agnostic.sh (16)
Learnt from: rpardini
PR: armbian/build#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: leggewie
PR: armbian/build#8133
File: extensions/apa.sh:1-2
Timestamp: 2025-04-28T08:27:26.890Z
Learning: In the Armbian build system, extension scripts in the `extensions/` directory contain hook functions and are meant to be sourced, not executed directly. These scripts don't require a shebang or `set -euo pipefail`.
Learnt from: djurny
PR: armbian/build#8315
File: lib/functions/rootfs/distro-agnostic.sh:54-68
Timestamp: 2025-06-22T00:53:10.331Z
Learning: The bash construct `export $(set | sed -En '/^BOOTSCRIPT_TEMPLATE__/s/=.*$//p')` is safe from word splitting because it extracts only variable names (by removing everything after `=`), and bash variable names cannot contain whitespace or special characters.
Learnt from: Grippy98
PR: armbian/build#8202
File: config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh:2-3
Timestamp: 2025-05-16T15:34:34.672Z
Learning: The shell scripts in the Armbian build system's desktop configuration files, such as config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh, rely on environment variables like $SRC and $destination that are expected to be set externally before the script is executed.
Learnt from: rpardini
PR: armbian/build#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: djurny
PR: armbian/build#8315
File: lib/functions/rootfs/distro-agnostic.sh:54-68
Timestamp: 2025-06-22T00:45:26.182Z
Learning: In Armbian build system, external command dependencies like envsubst are handled by adding the required packages (e.g., gettext for envsubst) to host_dependencies in adaptative_prepare_host_dependencies function in lib/functions/host/prepare-host.sh, rather than checking command availability at runtime.
Learnt from: djurny
PR: armbian/build#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.
Learnt from: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:182-186
Timestamp: 2025-06-04T23:45:38.860Z
Learning: In U-Boot bootscripts, the `setexpr` command will not fail if a variable is empty - it simply won't perform any substitution operations and leaves the target variable unchanged.
Learnt from: EvilOlaf
PR: armbian/build#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: djurny
PR: armbian/build#8287
File: config/bootscripts/boot-sunxi.cmd:13-23
Timestamp: 2025-06-12T21:08:11.318Z
Learning: In config/bootscripts/boot-sunxi.cmd the variable name `align_overlap_oboe_avoidance` intentionally uses the acronym “OBOE” (Off-By-One Error); it is not a typo and should not be renamed.
Learnt from: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:182-186
Timestamp: 2025-06-04T23:45:38.860Z
Learning: In config/bootscripts/boot-mvebu.cmd, the `fdtfile` variable is mandatory for booting and is pre-set by U-Boot, but can be overridden via armbianEnv.txt. If `fdtfile` is empty, the subsequent device tree file search logic will eventually fail and trigger the critical error "Cannot find DT!" with proper error handling.
Learnt from: EvilOlaf
PR: armbian/build#8298
File: extensions/https_proxy.sh:1-9
Timestamp: 2025-06-14T05:53:10.627Z
Learning: In Armbian build framework, extensions are loaded and executed via lib/functions/general/extensions.sh using call_extension_method. The framework sets set -o errexit and set -o errtrace globally, and individual commands run with pipefail protection through the command execution functions.
Learnt from: Grippy98
PR: armbian/build#8152
File: lib/functions/configuration/interactive.sh:209-266
Timestamp: 2025-05-05T12:35:07.143Z
Learning: For the interactive kernel selection in Armbian, KERNEL_MAJOR_MINOR and KERNEL_DESCRIPTION are parsed from family.conf but deliberately not set as environment variables to avoid potential interference with other parts of the build system.
Learnt from: EvilOlaf
PR: armbian/build#8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:42:09.086Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, KERNELSOURCE is explicitly declared when using unofficial or 3rd party kernel repositories (like the "dev" branch using https://github.com/apritzel/linux), but can be omitted when using the standard mainline kernel (like the "edge" branch) since it will fall back to the default mainline source.
Learnt from: EvilOlaf
PR: armbian/build#8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:40:52.109Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, when KERNELSOURCE is not explicitly declared in a case branch, the system falls back to a default KERNELSOURCE value, so missing KERNELSOURCE declarations are not critical issues.
Learnt from: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:61-63
Timestamp: 2025-06-04T23:52:39.493Z
Learning: In U-Boot shell scripting, the `else ;` construct followed by commands on subsequent lines is valid syntax, as demonstrated by empirical testing. Both `else command` and `else ; command` forms are acceptable in U-Boot's shell implementation.
🧬 Code Graph Analysis (1)
lib/functions/rootfs/distro-agnostic.sh (2)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)lib/functions/logging/runners.sh (1)
run_host_command_logged(204-206)
🪛 Shellcheck (0.10.0)
lib/functions/rootfs/distro-agnostic.sh
[warning] 58-58: BOOTSCRIPT_TEMPLATE__CREATE_DATE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 66-66: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (1)
lib/functions/rootfs/distro-agnostic.sh (1)
54-68: Template rendering pipeline looks solidThe new
render_bootscript_template/proof_rendered_bootscript_templatehelpers integrate cleanly, re-using existing logging and subshell isolation. Dependency handling forenvsubstis already covered upstream, and the variable-export construct is confirmed safe (per earlier discussion).
No further action needed.
djurny
force-pushed
the
feat/bootscript-templating
branch
from
4b06fe4 to
63365b5
Compare
github-actions
bot
removed
size/medium
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/functions/rootfs/distro-agnostic.sh (3)
70-72: Usegrep -Einstead of deprecatedegrep
egrepis officially deprecated in GNU grep 3.8+. Switch togrep -Eto future-proof the script:- ! egrep '\$\{?BOOTSCRIPT_TEMPLATE__' "${1:?}" + ! grep -E '\$\{?BOOTSCRIPT_TEMPLATE__' "${1:?}"
55-59: Suppress ShellCheck false-positive forBOOTSCRIPT_TEMPLATE__CREATE_DATE
BOOTSCRIPT_TEMPLATE__CREATE_DATEis intentionally exported for template
substitution, so the SC2034 “unused variable” warning is a false alarm.
Consider adding a directive to keep CI quiet:+# shellcheck disable=SC2034 # variable consumed indirectly by envsubst typeset BOOTSCRIPT_TEMPLATE__CREATE_DATE
63-68: Optional: silence SC2046 without altering behaviourShellCheck warns about possible word-splitting in
export $( … ), but we've
confirmed only variable names are produced. To keep the linter quiet, append a
disable comment:- export $(set | sed -En '/^BOOTSCRIPT_TEMPLATE__/s/=.*$//p') + # shellcheck disable=SC2046 # safe: list contains only variable names + export $(set | sed -En '/^BOOTSCRIPT_TEMPLATE__/s/=.*$//p')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/bootscripts/boot-generic.cmd.template(1 hunks)lib/functions/bsp/armbian-bsp-cli-deb.sh(1 hunks)lib/functions/host/prepare-host.sh(1 hunks)lib/functions/rootfs/distro-agnostic.sh(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/functions/host/prepare-host.sh
- lib/functions/bsp/armbian-bsp-cli-deb.sh
- config/bootscripts/boot-generic.cmd.template
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: djurny
PR: armbian/build#8315
File: lib/functions/rootfs/distro-agnostic.sh:54-68
Timestamp: 2025-06-22T00:45:26.182Z
Learning: In Armbian build system, external command dependencies like envsubst are handled by adding the required packages (e.g., gettext for envsubst) to host_dependencies in adaptative_prepare_host_dependencies function in lib/functions/host/prepare-host.sh, rather than checking command availability at runtime.
Learnt from: djurny
PR: armbian/build#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.
Learnt from: EvilOlaf
PR: armbian/build#8328
File: lib/functions/compilation/patch/drivers_network.sh:542-545
Timestamp: 2025-06-24T10:08:40.313Z
Learning: In the Armbian build system, when a PR removes build support for a specific kernel version, version check issues for that removed version become practically irrelevant even if they appear incorrect in isolation. Context about which kernel versions are being deprecated/removed is important for understanding the impact of version-related code changes.
Learnt from: rpardini
PR: armbian/build#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: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:182-186
Timestamp: 2025-06-04T23:45:38.860Z
Learning: In config/bootscripts/boot-mvebu.cmd, the `fdtfile` variable is mandatory for booting and is pre-set by U-Boot, but can be overridden via armbianEnv.txt. If `fdtfile` is empty, the subsequent device tree file search logic will eventually fail and trigger the critical error "Cannot find DT!" with proper error handling.
Learnt from: djurny
PR: armbian/build#8203
File: config/bootscripts/boot-sunxi.cmd:111-114
Timestamp: 2025-05-17T23:08:31.746Z
Learning: In U-Boot scripts, the `setexpr sub` command is valid for string operations when CONFIG_REGEX=y is enabled. This allows for pattern substitution in strings, as documented in https://docs.u-boot.org/en/latest/usage/cmd/setexpr.html. The syntax is `setexpr <variable> sub <pattern> <replacement> <string>`.
Learnt from: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:182-186
Timestamp: 2025-06-04T23:45:38.860Z
Learning: In U-Boot bootscripts, the `setexpr` command will not fail if a variable is empty - it simply won't perform any substitution operations and leaves the target variable unchanged.
Learnt from: djurny
PR: armbian/build#8287
File: config/bootscripts/boot-sunxi.cmd:38-44
Timestamp: 2025-06-12T21:14:36.024Z
Learning: In config/bootscripts/boot-sunxi.cmd the unconditional "+1" increment in func_align_addr_next (when align_overlap_oboe_avoidance="on") is intentional. It compensates for a known off-by-one error in U-Boot where the end address is calculated as start+size instead of start+size-1, so the extra page prevents overlap. This behavior should not be “optimized away”.
Learnt from: rpardini
PR: armbian/build#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: djurny
PR: armbian/build#8166
File: config/bootscripts/boot-mvebu.cmd:38-50
Timestamp: 2025-05-07T20:49:40.969Z
Learning: In U-Boot scripts, `itest.s` should be used for explicit string comparisons rather than plain `itest` with string operators like `==`.
Learnt from: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:61-63
Timestamp: 2025-06-04T23:52:39.493Z
Learning: In U-Boot shell scripting, the `else ;` construct followed by commands on subsequent lines is valid syntax, as demonstrated by empirical testing. Both `else command` and `else ; command` forms are acceptable in U-Boot's shell implementation.
lib/functions/rootfs/distro-agnostic.sh (16)
Learnt from: rpardini
PR: armbian/build#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: leggewie
PR: armbian/build#8133
File: extensions/apa.sh:1-2
Timestamp: 2025-04-28T08:27:26.890Z
Learning: In the Armbian build system, extension scripts in the `extensions/` directory contain hook functions and are meant to be sourced, not executed directly. These scripts don't require a shebang or `set -euo pipefail`.
Learnt from: djurny
PR: armbian/build#8315
File: lib/functions/rootfs/distro-agnostic.sh:54-68
Timestamp: 2025-06-22T00:53:10.331Z
Learning: The bash construct `export $(set | sed -En '/^BOOTSCRIPT_TEMPLATE__/s/=.*$//p')` is safe from word splitting because it extracts only variable names (by removing everything after `=`), and bash variable names cannot contain whitespace or special characters.
Learnt from: Grippy98
PR: armbian/build#8202
File: config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh:2-3
Timestamp: 2025-05-16T15:34:34.672Z
Learning: The shell scripts in the Armbian build system's desktop configuration files, such as config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh, rely on environment variables like $SRC and $destination that are expected to be set externally before the script is executed.
Learnt from: rpardini
PR: armbian/build#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: djurny
PR: armbian/build#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.
Learnt from: djurny
PR: armbian/build#8315
File: lib/functions/rootfs/distro-agnostic.sh:54-68
Timestamp: 2025-06-22T00:45:26.182Z
Learning: In Armbian build system, external command dependencies like envsubst are handled by adding the required packages (e.g., gettext for envsubst) to host_dependencies in adaptative_prepare_host_dependencies function in lib/functions/host/prepare-host.sh, rather than checking command availability at runtime.
Learnt from: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:182-186
Timestamp: 2025-06-04T23:45:38.860Z
Learning: In U-Boot bootscripts, the `setexpr` command will not fail if a variable is empty - it simply won't perform any substitution operations and leaves the target variable unchanged.
Learnt from: EvilOlaf
PR: armbian/build#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: djurny
PR: armbian/build#8287
File: config/bootscripts/boot-sunxi.cmd:13-23
Timestamp: 2025-06-12T21:08:11.318Z
Learning: In config/bootscripts/boot-sunxi.cmd the variable name `align_overlap_oboe_avoidance` intentionally uses the acronym “OBOE” (Off-By-One Error); it is not a typo and should not be renamed.
Learnt from: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:182-186
Timestamp: 2025-06-04T23:45:38.860Z
Learning: In config/bootscripts/boot-mvebu.cmd, the `fdtfile` variable is mandatory for booting and is pre-set by U-Boot, but can be overridden via armbianEnv.txt. If `fdtfile` is empty, the subsequent device tree file search logic will eventually fail and trigger the critical error "Cannot find DT!" with proper error handling.
Learnt from: EvilOlaf
PR: armbian/build#8298
File: extensions/https_proxy.sh:1-9
Timestamp: 2025-06-14T05:53:10.627Z
Learning: In Armbian build framework, extensions are loaded and executed via lib/functions/general/extensions.sh using call_extension_method. The framework sets set -o errexit and set -o errtrace globally, and individual commands run with pipefail protection through the command execution functions.
Learnt from: Grippy98
PR: armbian/build#8152
File: lib/functions/configuration/interactive.sh:209-266
Timestamp: 2025-05-05T12:35:07.143Z
Learning: For the interactive kernel selection in Armbian, KERNEL_MAJOR_MINOR and KERNEL_DESCRIPTION are parsed from family.conf but deliberately not set as environment variables to avoid potential interference with other parts of the build system.
Learnt from: EvilOlaf
PR: armbian/build#8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:42:09.086Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, KERNELSOURCE is explicitly declared when using unofficial or 3rd party kernel repositories (like the "dev" branch using https://github.com/apritzel/linux), but can be omitted when using the standard mainline kernel (like the "edge" branch) since it will fall back to the default mainline source.
Learnt from: EvilOlaf
PR: armbian/build#8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:40:52.109Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, when KERNELSOURCE is not explicitly declared in a case branch, the system falls back to a default KERNELSOURCE value, so missing KERNELSOURCE declarations are not critical issues.
Learnt from: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:61-63
Timestamp: 2025-06-04T23:52:39.493Z
Learning: In U-Boot shell scripting, the `else ;` construct followed by commands on subsequent lines is valid syntax, as demonstrated by empirical testing. Both `else command` and `else ; command` forms are acceptable in U-Boot's shell implementation.
🧬 Code Graph Analysis (1)
lib/functions/rootfs/distro-agnostic.sh (2)
.github/generate_CODEOWNERS.sh (1)
display_alert(6-6)lib/functions/logging/runners.sh (1)
run_host_command_logged(204-206)
🪛 Shellcheck (0.10.0)
lib/functions/rootfs/distro-agnostic.sh
[warning] 58-58: BOOTSCRIPT_TEMPLATE__CREATE_DATE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 66-66: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (1)
lib/functions/rootfs/distro-agnostic.sh (1)
19-27: Regex incorrectly classifies many serial consoles (e.g.ttyAMA0)The pattern
'tty'[AGSU]*only matchesttyfollowed by any combination of A/G/S/U.
Devices likettyAMA0,ttyUSB0,ttyACM0, etc. are therefore mis-detected:
ttyAMA0fails because the third characterMis not in[AGSU].ttyUSB0fails because the second characterUmatches but the followingS/Bare outside the set.As a result, serial consoles can leak into DISPLAY_CONSOLE or be omitted from SERIAL_CONSOLE, altering the generated kernel cmdline.
- if [[ "${ITEM#*=}" =~ 'tty'[AGSU]* ]] ; then + # Match any tty* serial device (ttyS*, ttyAMA*, ttyUSB*, ttyACM*, ttyGS*, …) + if [[ "${ITEM#*=}" =~ ^tty[A-Za-z0-9]+ ]] ; thenApply the same fix in both export functions.
Also applies to: 41-49
⛔ Skipped due to learnings
Learnt from: djurny PR: armbian/build#8235 File: packages/bsp/mvebu/helios4/helios4-wol.service:0-0 Timestamp: 2025-05-29T01:56:01.604Z Learning: For ARM-based devices like Helios4 that use DeviceTree, the expected systemd network interface naming scheme is "end[0-9]+" (DeviceTree alias index), making the regex "^(eth|en[do])[0-9]+" appropriate as it covers traditional "eth", PCI on-board "eno", and DeviceTree "end" interfaces without unnecessary "enp" (PCI slot) support.Learnt from: djurny PR: armbian/build#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.Learnt from: djurny PR: armbian/build#8166 File: config/bootscripts/boot-mvebu.cmd:38-50 Timestamp: 2025-05-07T20:49:40.969Z Learning: In U-Boot scripts, `itest.s` should be used for explicit string comparisons rather than plain `itest` with string operators like `==`.Learnt from: rpardini PR: armbian/build#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: djurny PR: armbian/build#8203 File: config/bootscripts/boot-sunxi.cmd:34-38 Timestamp: 2025-05-18T00:59:02.274Z Learning: In U-Boot shell environments, `itest` silently handles non-numeric values without producing errors, unlike in bash or other standard shells. Additionally, U-Boot doesn't support stderr redirection with syntax like `2>/dev/null`. The standard pattern for verbosity checking in U-Boot is `if test "${verbosity}" != "" ; then if itest ${verbosity} -gt 0 ; then...`, which correctly handles both numeric and non-numeric values.Learnt from: djurny PR: armbian/build#8315 File: lib/functions/rootfs/distro-agnostic.sh:54-68 Timestamp: 2025-06-22T00:53:10.331Z Learning: The bash construct `export $(set | sed -En '/^BOOTSCRIPT_TEMPLATE__/s/=.*$//p')` is safe from word splitting because it extracts only variable names (by removing everything after `=`), and bash variable names cannot contain whitespace or special characters.Learnt from: Grippy98 PR: armbian/build#8202 File: config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh:2-3 Timestamp: 2025-05-16T15:34:34.672Z Learning: The shell scripts in the Armbian build system's desktop configuration files, such as config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh, rely on environment variables like $SRC and $destination that are expected to be set externally before the script is executed.
djurny
force-pushed
the
feat/bootscript-templating
branch
from
63365b5 to
34f1420
Compare
coderabbitai
bot
added
Ready to merge
* cleanup comments, linebreaks... * two more comments * `BOOTFS_TYPE` has never been set as far as I can see, therefore no need to unset. * some more comments... * comment, simplify logic * even more comments * just a few more * comments, description for elish * merge uboot functions. @SuperKali review
Enable bootscript templating using `envsubst`. Add generic bootscript.
Still work in progress, needs early review.
`test` does not support `==`, but deceptively returns `true` in case you use it.
De-duplicate kernel commandline console options
…y/build into feat/bootscript-templating
djurny
requested review from
ColorfulRhino,
HeyMeco,
JohnTheCoolingFan,
SuperKali,
TheSnowfield,
Tonymac32,
ahoneybun,
amazingfate,
brentr,
catalinii,
chainsx,
efectn,
joekhoobyar and
krachlatte
as code owners
|
Added a story on how to use the generic bootscript template in |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/boards/youyeetoo-r1-v3.csc (1)
57-67: Patchingrockchip-common.hwith a shared BOOTDIR can affect other Rockchip boards
pre_config_uboot_target__youyeetoo_r1_patch_rockchip_common_boot_order()editsinclude/configs/rockchip-common.hin-place:sed -i -e "s/#define BOOT_TARGETS.*/#define BOOT_TARGETS \"${rockchip_uboot_targets[*]}\"/" include/configs/rockchip-common.hbut
post_family_config__youyeetoo_r1_use_mainline_uboot()explicitly keeps a shared U‑Boot source directory (“Don’t set BOOTDIR…”). That means this BOOT_TARGETS change will persist in the shared checkout and can change boot order for any other RK3588/Rockchip board that reuses the same tree, not just this board.Given the comment explicitly mentions Youyeetoo R1 semantics, this feels unintentionally board‑specific behaviour being applied at a family‑wide location.
Consider one of:
- Giving this board its own
BOOTDIR(like the Mekotronics example) so the sed change is isolated to this board’s U‑Boot build, or- Moving this boot‑order override into a shared rockchip family hook if you actually want the change for all rockchip boards, or
- Targeting a more board‑specific header/Kconfig fragment instead of
rockchip-common.h.I’d suggest tightening this before merge to avoid surprising boot‑order changes on other Rockchip boards built in the same workspace.
♻️ Duplicate comments (1)
config/bootscripts/boot-generic.cmd.template.md (1)
35-40: Align markdown with lint rules (tables and fenced code languages).There are a couple of markdown‑lint issues that are easy to fix:
- Add a blank line before the second table (MD058):
- Two exceptions for templating are listed below:
-|Variable|Usage|
- Two exceptions for templating are listed below:
+|Variable|Usage|
2. Specify languages for the fenced code blocks (MD040) — same concern as earlier bot comments: - Orange Pi Zero board config example (treat as a config snippet): ```diff -Example of a board configuration file for the Orange Pi Zero: -``` +Example of a board configuration file for the Orange Pi Zero: +```conf ``` - SERIALCON example (bash‑style assignment): ```diff -Multiple console devices can be defined by separating them with a `,` (comma). Standard Linux kernel arguments are allowed: -``` -SERIALCON="ttyS0:115200,ttyGS0" -``` +Multiple console devices can be defined by separating them with a `,` (comma). Standard Linux kernel arguments are allowed: +```bash +SERIALCON="ttyS0:115200,ttyGS0" +``` ``` Also applies to: 43-45, 83-86 </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (11)</summary><blockquote> <details> <summary>packages/bsp/common/usr/lib/armbian/armbian-ramlog (1)</summary><blockquote> `155-157`: **Good addition to preserve log file timestamps.** The new timestamp synchronization step correctly preserves the source file's modification time when restoring logs from persistent storage to /var/log, which is valuable for log analysis and maintaining accurate chronological records. Consider adding basic error checking to ensure the copy succeeded before updating the timestamp: ```diff dest="/var/log/$file" -cat $file > $dest -touch -r $file $dest +if cat $file > $dest; then + touch -r $file $dest +fiThis prevents touching a potentially incomplete or missing file if the copy operation fails.
config/boards/nanopi-r3s.csc (1)
30-32: Consider quoting variables in dd command.While the current code works in the controlled build environment, shell best practice is to quote variables
$1and$2to safely handle paths with spaces or special characters.Apply this diff:
function write_uboot_platform() { - dd if=$1/u-boot-rockchip.bin of=$2 seek=64 conv=notrunc status=none + dd if="$1/u-boot-rockchip.bin" of="$2" seek=64 conv=notrunc status=none }config/kernel/linux-rockchip64-edge.config (1)
517-517: Question: Do we need QRTR_MHI on Rockchip?CONFIG_QRTR_MHI=m is Qualcomm-specific; likely unused on rockchip64. Safe but adds a module. If no known users, consider dropping to keep footprint minimal.
Can you confirm an actual consumer on rockchip64 targets?
packages/bsp/common/usr/bin/armbian-install (2)
184-184: Good addition of dry-run flag for counting.The
-nflag correctly ensures the file-counting operation performs no modifications. However, consider adding validation after this line to handle cases where the grep or awk might fail (e.g., due to rsync version differences), leavingTODOempty. An emptyTODOwill cause the progress calculation on line 200 to default to 1, resulting in misleading progress percentages.Consider adding validation:
TODO=$(rsync -anvx --delete --stats --exclude-from=$EX_LIST / "${TempDir}"/rootfs | grep "Number of files:"|awk '{print $4}' | tr -d '.,') # Validate TODO is a positive number if [[ ! "$TODO" =~ ^[0-9]+$ ]] || [[ "$TODO" -eq 0 ]]; then dialog --title "$title" --backtitle "$backtitle" --infobox "\n Warning: Could not count files accurately. Progress may be approximate." 5 70 TODO=1000 # reasonable fallback sleep 2 fi
197-206: Progress calculation works but relies on accurate TODO value.The refactored awk-based progress reporting is cleaner than a multi-stage pipeline. The use of
stdbuf -oLensures timely output, and the percentage clamping prevents display overflow. However, the fallback value oftodo="${TODO:-1}"means that if file counting failed earlier (line 184), the progress will immediately jump to 100% and stay there, which is confusing for users.This concern is addressed by the validation suggestion at line 184.
config/boards/rock-3a.conf (1)
33-33: Consider preserving the inline comment for BOOTDIR.The removed comment "do not share u-boot directory" provided useful context explaining why this board needs a dedicated U-Boot directory. Similar patterns in other board configs (e.g., mekotronics-rk3588.hooks.sh line 31) retain this comment for maintainability.
config/boards/orangepi5.conf (1)
29-34: Consider preserving informative inline comments.The removed comments provided valuable context:
- Line 30: "Wait for UART interrupt to enter UMS/RockUSB mode etc" explained BOOTDELAY=1 rationale
- Line 34: "do not share u-boot directory" clarified BOOTDIR board-specificity
While the code functions correctly, these comments aided maintainability. Other board configs (e.g., mekotronics-rk3588.hooks.sh) retain similar documentation.
.github/CODEOWNERS (1)
201-202: Minor: Potential redundant CODEOWNERS pattern.Line 202's pattern
linux-k3-beagle-current-*.configis already covered by line 201's broader patternlinux-k3-beagle-*.config. While CODEOWNERS allows this (last match wins), the duplication doesn't add value unless future differentiation is planned.config/bootscripts/boot-generic.cmd.template.md (1)
21-33: Tighten wording and fix minor typos in the docs.A few small textual issues worth fixing for clarity:
- Line 22: use plural “bootscripts”:
- Differences and deviations in the actions or settings performed by these bootscript have been extracted
- Differences and deviations in the actions or settings performed by these bootscripts have been extracted
- Line 32:
rockchips64looks like a typo; suggest using the vendor name:-|
BOOTSCRIPT_TEMPLATE__BOARD_VENDOR|The vendor of the board, e.g.allwinner,rockchips64.|
+|BOOTSCRIPT_TEMPLATE__BOARD_VENDOR|The vendor of the board, e.g.allwinner,rockchip.|- Line 91: “it's size” → “its size”: ```diff -... if the bootscript can determine it's size. +... if the bootscript can determine its size.Also applies to: 90-92
config/sources/families/include/k3_common.inc (2)
20-28: Confirm thatadd_host_dependencies__k3_python3_depis actually invoked.The host‑deps hook is named
add_host_dependencies__k3_python3_dep, but there is no correspondingenable_extension "k3_python3_dep". Whether this runs depends on howcall_extension_method "add_host_dependencies"discovers functions (by prefix vs.<hook>__${extension}for enabled extensions only).If discovery is extension‑based, this hook will never fire and the extra Python3 packages won’t be installed. Please double‑check the extension manager behavior and either:
- Rename/relocate this into an appropriate enabled extension (e.g. a k3‑specific extension), or
- Adjust the naming to whatever pattern
call_extension_methodactually scans for here.Also applies to: 39-42
95-99: Verifywrite_uboot_platform()semantics with postinst usage.
write_uboot_platform()is defined as:function write_uboot_platform() { cp $1/tiboot3.bin ${MOUNT}/boot cp $1/sysfw.itb ${MOUNT}/boot || true cp $1/tispl.bin ${MOUNT}/boot cp $1/u-boot.img ${MOUNT}/boot }The generic u‑boot postinst calls this as
write_uboot_platform "$DIR" "${root_partition_device}", i.e. with two arguments and no explicitMOUNT. This implementation ignores$2and relies on${MOUNT}already pointing at the correct boot filesystem.Please confirm that
platform_install.shcorrectly setsMOUNTfor K3 before callingwrite_uboot_platform, and that copying into${MOUNT}/boot(rather than writing to$2withdd) matches TI’s boot flow expectations.
| - name: Get changed files | ||
| id: changed-files | ||
| uses: tj-actions/changed-files@0ff001de0805038ff3f118de4875002200057732 # v46.0.3 | ||
| uses: tj-actions/changed-files@70069877f29101175ed2b055d210fe8b1d54d7d7 # v46.0.3 |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM: GitHub Actions dependency update.
The changed-files action has been updated to a newer commit while maintaining the same version tag (v46.0.3). This appears to be a routine dependency update.
Please verify the changes between the old and new commit are non-breaking:
🌐 Web query:
What changes were made in tj-actions/changed-files between commits 0ff001de0805038ff3f118de4875002200057732 and 70069877f29101175ed2b055d210fe8b1d54d7d7?
💡 Result:
I can fetch that commit diff from the repository and show what changed — shall I pull the GitHub compare (diff) between those two SHAs now?
Based on my verification, I've identified a critical security concern that requires immediate attention.
The tj-actions/changed-files GitHub Action was compromised between March 12th and March 15th, 2025, during which time the compromised Action printed repositories' secrets values to build logs. All tagged versions of the Action were compromised during this time.
This has been patched in v46.0.1, released March 16, 2025. However, the review shows two different commit SHAs for the same version tag (v46.0.3), which suggests the tag may have been re-pointed after the incident. If you are using tagged versions (e.g., v35, v44.5.1), no action is required as these tags have been updated and are now safe to use.
Immediate verification needed:
- Confirm that any workflow runs using this action between March 12-15, 2025 should have secrets audited and rotated
- Verify the workflow only uses this updated, patched version going forward (v46.0.1 or later)
🤖 Prompt for AI Agents
.github/workflows/pr-kernel-security-analysis.yml around line 37: the workflow
references tj-actions/changed-files pinned to a specific commit SHA that may
correspond to a compromised release window; update the action reference to a
known-patched release (e.g., use tj-actions/changed-files@v46.0.1 or any later
v46+ tag/sha published after 2025-03-16), and ensure no workflows run with the
compromised versions between 2025-03-12 and 2025-03-15; additionally, audit and
rotate any repository secrets that were exposed by runs during that window and
re-run critical workflows with the updated action to confirm no further
exposure.
| # Resolve base version (prefer workflow input if provided) | ||
| BASE_VERSION="${{ inputs.armbian_version }}" | ||
| if [ -z "$BASE_VERSION" ]; then | ||
| BASE_VERSION="$(jq -r '.version' os/stable.json)" | ||
| fi | ||
|
|
||
| # Preserve optional 'v' prefix but strip it for math | ||
| PREFIX='' | ||
| if [[ "$BASE_VERSION" == v* ]]; then | ||
| PREFIX='v' | ||
| BASE_NO_PREFIX="${BASE_VERSION#v}" | ||
| else | ||
| BASE_NO_PREFIX="$BASE_VERSION" | ||
| fi | ||
|
|
||
| # Drop any pre-release/build metadata (e.g., -rc1, +meta) | ||
| CORE="${BASE_NO_PREFIX%%[-+]*}" | ||
|
|
||
| # Split and increment patch | ||
| IFS='.' read -r MAJOR MINOR PATCH <<<"$CORE" | ||
| PATCH=${PATCH:-0} | ||
| NEXT_PATCH=$((PATCH + 1)) | ||
| NEXT_VERSION="${PREFIX}${MAJOR}.${MINOR}.${NEXT_PATCH}" | ||
|
|
||
| { | ||
| echo "ARMBIAN_VERSION=${NEXT_VERSION}" | ||
| } >> "$GITHUB_ENV" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify version increment logic handles all scenarios correctly.
The new version normalization and increment logic handles several cases well (v-prefix preservation, metadata stripping, default PATCH=0). However, please verify:
- Non-numeric version components: What happens if MAJOR/MINOR/PATCH contain non-digits? The arithmetic
$((PATCH + 1))will fail silently or error. - Input vs stable.json precedence: If
armbian_versioninput is provided and is lower than stable.json, should patch still be incremented? Current logic always increments regardless. - Four-part versions: Versions like "1.2.3.4" will have the 4th component silently dropped.
Consider adding validation or documenting these edge cases.
Example edge case test:
🏁 Script executed:
#!/bin/bash
# Test version parsing with edge cases
BASE_VERSION="1.2.3-rc1"
PREFIX=''; [[ "$BASE_VERSION" == v* ]] && PREFIX='v' && BASE_NO_PREFIX="${BASE_VERSION#v}" || BASE_NO_PREFIX="$BASE_VERSION"
CORE="${BASE_NO_PREFIX%%[-+]*}"
IFS='.' read -r MAJOR MINOR PATCH <<<"$CORE"
PATCH=${PATCH:-0}
NEXT_PATCH=$((PATCH + 1))
NEXT_VERSION="${PREFIX}${MAJOR}.${MINOR}.${NEXT_PATCH}"
echo "Input: $BASE_VERSION → Output: $NEXT_VERSION"
# Test with v prefix
BASE_VERSION="v1.2.3+meta"
PREFIX=''; [[ "$BASE_VERSION" == v* ]] && PREFIX='v' && BASE_NO_PREFIX="${BASE_VERSION#v}" || BASE_NO_PREFIX="$BASE_VERSION"
CORE="${BASE_NO_PREFIX%%[-+]*}"
IFS='.' read -r MAJOR MINOR PATCH <<<"$CORE"
PATCH=${PATCH:-0}
NEXT_PATCH=$((PATCH + 1))
NEXT_VERSION="${PREFIX}${MAJOR}.${MINOR}.${NEXT_PATCH}"
echo "Input: $BASE_VERSION → Output: $NEXT_VERSION"
# Test with missing patch
BASE_VERSION="1.2"
PREFIX=''; [[ "$BASE_VERSION" == v* ]] && PREFIX='v' && BASE_NO_PREFIX="${BASE_VERSION#v}" || BASE_NO_PREFIX="$BASE_VERSION"
CORE="${BASE_NO_PREFIX%%[-+]*}"
IFS='.' read -r MAJOR MINOR PATCH <<<"$CORE"
PATCH=${PATCH:-0}
NEXT_PATCH=$((PATCH + 1))
NEXT_VERSION="${PREFIX}${MAJOR}.${MINOR}.${NEXT_PATCH}"
echo "Input: $BASE_VERSION → Output: $NEXT_VERSION"Length of output: 95
I need to test the remaining edge cases and understand the context. Let me continue verification.
🏁 Script executed:
#!/bin/bash
# Test non-numeric components
echo "=== Testing non-numeric components ==="
for BASE_VERSION in "1.2.a" "1.a.3" "a.2.3"; do
PREFIX=''; [[ "$BASE_VERSION" == v* ]] && PREFIX='v' && BASE_NO_PREFIX="${BASE_VERSION#v}" || BASE_NO_PREFIX="$BASE_VERSION"
CORE="${BASE_NO_PREFIX%%[-+]*}"
IFS='.' read -r MAJOR MINOR PATCH <<<"$CORE"
PATCH=${PATCH:-0}
NEXT_PATCH=$((PATCH + 1)) 2>&1
RESULT=$?
NEXT_VERSION="${PREFIX}${MAJOR}.${MINOR}.${NEXT_PATCH}"
echo "Input: $BASE_VERSION → Output: $NEXT_VERSION (exit code: $RESULT)"
done
echo ""
echo "=== Testing 4+ part versions ==="
for BASE_VERSION in "1.2.3.4" "v1.2.3.4.5"; do
PREFIX=''; [[ "$BASE_VERSION" == v* ]] && PREFIX='v' && BASE_NO_PREFIX="${BASE_VERSION#v}" || BASE_NO_PREFIX="$BASE_VERSION"
CORE="${BASE_NO_PREFIX%%[-+]*}"
IFS='.' read -r MAJOR MINOR PATCH <<<"$CORE"
PATCH=${PATCH:-0}
NEXT_PATCH=$((PATCH + 1))
NEXT_VERSION="${PREFIX}${MAJOR}.${MINOR}.${NEXT_PATCH}"
echo "Input: $BASE_VERSION → Output: $NEXT_VERSION (4+ parts silently dropped)"
done
echo ""
echo "=== Testing empty/invalid input ==="
for BASE_VERSION in "" "v" ".1.2" "1..2"; do
PREFIX=''; [[ "$BASE_VERSION" == v* ]] && PREFIX='v' && BASE_NO_PREFIX="${BASE_VERSION#v}" || BASE_NO_PREFIX="$BASE_VERSION"
CORE="${BASE_NO_PREFIX%%[-+]*}"
IFS='.' read -r MAJOR MINOR PATCH <<<"$CORE"
PATCH=${PATCH:-0}
NEXT_PATCH=$((PATCH + 1))
NEXT_VERSION="${PREFIX}${MAJOR}.${MINOR}.${NEXT_PATCH}"
echo "Input: '$BASE_VERSION' → Output: '$NEXT_VERSION'"
doneLength of output: 446
Now I see concrete issues. Let me check the action.yml context to understand the design intent and determine severity.
🏁 Script executed:
#!/bin/bash
# Find and display action.yml around the version logic
fd -n "action.yml" -x cat {}Length of output: 230
🏁 Script executed:
#!/bin/bash
# Find action.yml and display the full file with context
fd "action.yml" --type f -x cat {}Length of output: 6589
Add input validation before version arithmetic to prevent crashes and invalid output.
Testing confirms the edge cases are real defects:
- 4-part versions crash: Input like "1.2.3.4" causes
$((PATCH + 1))to fail with "invalid arithmetic operator" (line 164) - Non-numeric components silently propagate: "1.a.3" becomes "1.a.4"; "1.2.a" becomes "1.2.1" (no validation)
- Empty/malformed input produces garbage: Empty BASE_VERSION outputs "..1"; ".1.2" outputs ".1.3"
Add validation before line 164 to:
- Reject or handle versions with 4+ parts
- Validate MAJOR/MINOR/PATCH are numeric before arithmetic
- Reject empty or malformed input
Without this, a malformed version in os/stable.json or workflow input will crash the build process.
|
|
||
| BOARD_NAME="BeagleY-AI" | ||
| BOARDFAMILY="k3" | ||
| BOARDFAMILY="k3-beagle" |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM - Family refactoring implemented correctly.
The BOARDFAMILY change from "k3" to "k3-beagle" aligns with the PR's broader refactoring to consolidate Beagle-specific boards into a dedicated family configuration. The removal of the post_family_config_branch_current__beagley_ai_use_beagle_kernel_uboot() function indicates that Beagle-specific kernel and U-Boot configuration is now handled centrally in the k3-beagle family configuration file.
Verify that config/sources/families/k3-beagle.conf exists and properly defines the kernel and bootloader configuration that was previously handled by the removed hook function.
🏁 Script executed:
#!/bin/bash
# Verify k3-beagle family configuration exists and contains appropriate branch definitions
if [[ -f "config/sources/families/k3-beagle.conf" ]]; then
echo "=== k3-beagle.conf exists ==="
echo "=== Checking for current branch configuration ==="
rg -A 5 'current\s*\)' config/sources/families/k3-beagle.conf
else
echo "ERROR: config/sources/families/k3-beagle.conf not found"
exit 1
fiLength of output: 79
---
🏁 Script executed:
```shell
#!/bin/bash
# Inspect complete k3-beagle.conf file content
echo "=== Full k3-beagle.conf content ==="
cat config/sources/families/k3-beagle.conf
echo ""
echo "=== beagley-ai.conf complete content ==="
cat config/boards/beagley-ai.conf
Length of output: 1505
Critical: Kernel branch naming violates Armbian conventions for vendor kernels.
The k3-beagle family configuration uses a BeagleBoard vendor kernel fork, but incorrectly names the branch case as current | current-rt) instead of vendor | vendor-rt). Per Armbian conventions, "current" and "edge" are reserved for mainline kernels; vendor/non-mainline forks must use "vendor" or "vendor-rt" naming.
This naming mismatch requires fixes in two locations:
- config/sources/families/k3-beagle.conf line 22: Change
current | current-rt)tovendor | vendor-rt) - config/boards/beagley-ai.conf line 11: Change
KERNEL_TARGET="current"toKERNEL_TARGET="vendor"
🤖 Prompt for AI Agents
In config/boards/beagley-ai.conf around line 4 and
config/sources/families/k3-beagle.conf around line 22, the kernel branch naming
uses mainline labels instead of vendor labels; update
config/sources/families/k3-beagle.conf to replace the case pattern `current |
current-rt)` with `vendor | vendor-rt)` and update config/boards/beagley-ai.conf
to set KERNEL_TARGET="vendor" instead of "current", ensuring the
vendor/non-mainline naming follows Armbian conventions.
| KERNEL_TARGET="current,current-rt" | ||
| KERNEL_TEST_TARGET="current" |
There was a problem hiding this comment.
🧩 Analysis chain
Significant kernel target change: edge → current,current-rt.
This change removes edge kernel support and adds current-rt support for PocketBeagle 2. This is a significant shift in kernel support strategy.
-
Edge removal: Verify this is intentional and that users relying on edge kernel builds for PocketBeagle 2 are aware of this change. The AI summary indicates linux-k3-beagle-edge.config was removed entirely, confirming edge support is being dropped for the entire k3-beagle family.
-
Family configuration: Confirm that config/sources/families/k3-beagle.conf properly defines the "current" and "current-rt" branches with appropriate kernel sources. Based on learnings, verify these use mainline kernel sources rather than vendor forks (vendor forks should use "vendor" naming convention).
🏁 Script executed:
#!/bin/bash
# Verify k3-beagle family configuration defines current and current-rt branches properly
echo "=== Checking k3-beagle family configuration ==="
if [[ -f "config/sources/families/k3-beagle.conf" ]]; then
echo "=== Branch definitions ==="
rg -A 10 'current\s*\)|current-rt\s*\)' config/sources/families/k3-beagle.conf
echo ""
echo "=== Kernel source verification ==="
rg 'KERNELSOURCE|KERNELBRANCH' config/sources/families/k3-beagle.conf | head -20
else
echo "ERROR: k3-beagle.conf not found"
exit 1
fi
echo ""
echo "=== Verify edge config removal ==="
if [[ -f "config/kernel/linux-k3-beagle-edge.config" ]]; then
echo "WARNING: edge config still exists"
else
echo "Confirmed: linux-k3-beagle-edge.config was removed"
fiLength of output: 738
I've identified a critical issue in the verification. The script output reveals a naming convention violation. Let me search for more context:
🏁 Script executed:
#!/bin/bash
# Check all branch definitions in k3-beagle.conf
echo "=== All branch case patterns in k3-beagle.conf ==="
rg -n 'current|edge|vendor|legacy' config/sources/families/k3-beagle.conf | head -30
echo ""
echo "=== Full k3-beagle.conf content ==="
cat config/sources/families/k3-beagle.confLength of output: 1107
Naming convention violation: k3-beagle.conf incorrectly labels vendor kernel as "current"
The k3-beagle.conf uses "current | current-rt" branch definitions, but these reference a vendor fork (github.com/beagleboard/linux, explicitly labeled "BeagleBoard.org (vendor) kernel"). According to Armbian conventions, vendor fork branches must be named "vendor" or "vendor-rt", not "current". The "current" naming is reserved for mainline kernel branches.
This means pocketbeagle2.conf's KERNEL_TARGET="current,current-rt" is now using vendor-forked branches under mainline naming, violating the established convention.
Action required:
- Rename "current | current-rt" to "vendor | vendor-rt" in config/sources/families/k3-beagle.conf (line 18)
- Update pocketbeagle2.conf's KERNEL_TARGET to
"vendor,vendor-rt"instead of"current,current-rt"
🤖 Prompt for AI Agents
In config/boards/pocketbeagle2.conf around lines 11-12 and
config/sources/families/k3-beagle.conf at line 18, the board and family configs
use "current,current-rt" for what are vendor fork branches; change the family
definition to use "vendor" and "vendor-rt" (replace "current | current-rt" with
"vendor | vendor-rt" at line 18 of k3-beagle.conf) and update
pocketbeagle2.conf's KERNEL_TARGET to "vendor,vendor-rt" (replace
"current,current-rt") so the naming matches Armbian conventions for vendor fork
kernels.
|
|
||
| declare -g LINUXCONFIG="linux-genio-$BRANCH" | ||
| declare -g KERNEL_MAJOR_MINOR="6.14" | ||
| declare -g KERNEL_MAJOR_MINOR="6.18" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify kernel version 6.18 availability in the collabora branch.
The KERNEL_MAJOR_MINOR update from 6.14 to 6.18 is a significant jump (4 minor versions). Please confirm that the "mediatek-next" branch in the collabora repository actually provides kernel 6.18.
🌐 Web query:
Does Collabora's mediatek-next branch at gitlab.collabora.com/mediatek/aiot/linux support Linux kernel 6.18?
💡 Result:
Short answer: No — Collabora's mediatek-next tree does not advertise/support Linux 6.18. Collabora's MediaTek work has been landing into 6.14–6.16 (and ongoing into 6.17/6.19 cycles), but there is no indication their mediatek-next branch is based on or targets upstream 6.18. [1][2][3]
Sources:
- Collabora blog: MediaTek Genio upstream progress / kernels (mentions work merged into 6.14 and follow-ups for 6.15/6.16). [1]
- Collabora blog: 6.15 release notes showing Mediatek contributions. [2]
- Coverage/summary of Collabora’s MediaTek mainline work and kernel status matrix. [3]
Revert KERNEL_MAJOR_MINOR to 6.14 or update KERNELBRANCH to a branch supporting kernel 6.18.
The mediatek-next branch does not support Linux kernel 6.18. Collabora's MediaTek work has been landing into 6.14–6.16 (and ongoing into 6.17/6.19 cycles). The change to KERNEL_MAJOR_MINOR="6.18" on line 23 is incompatible with KERNELBRANCH="branch:mediatek-next" and will cause a version mismatch during build.
🤖 Prompt for AI Agents
In config/sources/families/genio.conf around line 23, KERNEL_MAJOR_MINOR is set
to "6.18" which is incompatible with KERNELBRANCH="branch:mediatek-next"; revert
KERNEL_MAJOR_MINOR to "6.14" to match mediatek-next or, if you must use 6.18,
change KERNELBRANCH to a branch that actually supports kernel 6.18; ensure the
two variables are consistent to avoid build version mismatches.
| @@ -1,18 +1,21 @@ | |||
| # Create UFS aligned image (requires >= Debian 13 (Trixie) Host) | |||
| declare -g DOCKER_ARMBIAN_BASE_IMAGE=debian:trixie | |||
| # declare -g DOCKER_ARMBIAN_BASE_IMAGE=debian:trixie # Use this env variable manually | |||
There was a problem hiding this comment.
🧩 Analysis chain
Commenting out DOCKER_ARMBIAN_BASE_IMAGE may break UFS builds.
The extension previously auto-configured DOCKER_ARMBIAN_BASE_IMAGE=debian:trixie globally. Commenting this out means:
- UFS builds will now use the default Docker image (likely ubuntu:noble)
- sfdisk may be too old, causing build failures
- Users must manually set the variable externally before the extension loads
The comment "Use this env variable manually" is unclear about where/how to set it. If this change is intentional to require explicit opt-in, please document the usage pattern in the file header or extension documentation.
Based on learnings: DOCKER_ARMBIAN_BASE_IMAGE must be set at file load time for Docker operations.
</shell_scripts>
🏁 Script executed:
#!/bin/bash
# Investigate extensions/ufs.sh and related docker configuration
echo "=== Inspecting extensions/ufs.sh (full content) ==="
cat extensions/ufs.sh
echo -e "\n=== Checking git history for ufs.sh (last few commits) ==="
git log --oneline -10 -- extensions/ufs.sh
echo -e "\n=== Searching for DOCKER_ARMBIAN_BASE_IMAGE usage in docker.sh ==="
rg -n "DOCKER_ARMBIAN_BASE_IMAGE" lib/functions/host/docker.sh -B2 -A2
echo -e "\n=== Checking if there are any conditional UFS configurations ==="
rg -n "ufs" extensions/ -i
echo -e "\n=== Looking for UFS documentation or board configs ==="
fd -e "sh" -e "conf" | xargs rg -l "ufs" -i 2>/dev/null | head -20Length of output: 6959
</shell_scripts>
🏁 Script executed:
#!/bin/bash
# Check the git history to understand if/when the line was commented out
echo "=== Git show commit 5ddf398f (when DOCKER_ARMBIAN_BASE_IMAGE was added) ==="
git show 5ddf398f -- extensions/ufs.sh
echo -e "\n=== Git show recent commits to see when it was commented ==="
git show ebc129fa -- extensions/ufs.sh
echo -e "\n=== Git show commit 2e4e238f ==="
git show 2e4e238f -- extensions/ufs.sh
echo -e "\n=== Current PR diff for extensions/ufs.sh (if available) ==="
git diff HEAD~1 extensions/ufs.sh 2>/dev/null || echo "No recent diff available"Length of output: 4002
</shell_scripts>
🏁 Script executed:
#!/bin/bash
# Check for documentation or board configs that manually set DOCKER_ARMBIAN_BASE_IMAGE for UFS
echo "=== Search for manual setup of DOCKER_ARMBIAN_BASE_IMAGE in board configs ==="
rg -n "DOCKER_ARMBIAN_BASE_IMAGE" config/boards/ 2>/dev/null || echo "Not found in board configs"
echo -e "\n=== Check if any board config includes ufs extension ==="
rg -n "ufs" config/boards/ -i -B2 -A2
echo -e "\n=== Search for DOCKER_ARMBIAN_BASE_IMAGE in CI/workflow files ==="
find . -name "*.yml" -o -name "*.yaml" | xargs rg -n "DOCKER_ARMBIAN_BASE_IMAGE" 2>/dev/null | head -20
echo -e "\n=== Check README or docs for UFS setup instructions ==="
find . -type f \( -name "README*" -o -name "*.md" \) | xargs rg -i "ufs\|DOCKER_ARMBIAN_BASE_IMAGE" 2>/dev/null | head -20
echo -e "\n=== Check git log after ebc129fa to see if there are follow-up fixes ==="
git log --oneline -20 -- extensions/ufs.shLength of output: 1310
Document the manual setup requirement or restore automatic configuration for UFS extension.
Verification confirms the intentional change: commit ebc129f deliberately commented out the DOCKER_ARMBIAN_BASE_IMAGE=debian:trixie declaration to require manual setup. However, this creates a breaking change without proper documentation:
- CI workflow pattern exists (
.github/workflows/rewrite-kernel-configs.ymlsets it via environment variable), proving manual setup works - No end-user guidance: The comment "Use this env variable manually" doesn't explain where/how to set it
- Default fallback is ubuntu:noble: Builds will silently fail without the variable set if sfdisk is too old
- No documentation or board configs show users how to manually configure this
- Breaking for existing users: Anyone using the UFS extension without knowing to set
DOCKER_ARMBIAN_BASE_IMAGEexternally will encounter build failures
Either add clear setup instructions to the file header explaining the manual environment variable pattern, or restore the automatic configuration that worked in commit 2e4e238.
| }' > "${nsi_conn_progress}" | ||
|
|
||
| # create empty persistent journal dir if it existed before install | ||
| [ -d /var/log.hdd/journal ] && mkdir -p "${TempDir}/rootfs/var/log/journal" |
There was a problem hiding this comment.
Potential race condition: directory created during active rsync.
Creating "${TempDir}/rootfs/var/log/journal" while rsync is actively copying to "${TempDir}/rootfs" introduces a race condition. If rsync hasn't yet processed the /var/log path, conflicts could occur. While the second rsync pass on line 251 likely mitigates this, it's better practice to create directories either before rsync starts or after it completes.
Consider moving this directory creation to just before the rsync invocation (before line 199) or after the background block completes:
# Before rsync (safer):
[ -d /var/log.hdd/journal ] && mkdir -p "${TempDir}/rootfs/var/log/journal"
# Launch rsync in background and stream progress percentages
{
rsync -avx --delete --exclude-from="$EX_LIST" / "${TempDir}/rootfs" \
| stdbuf -oL awk -v todo="${TODO:-1}" '{
...🤖 Prompt for AI Agents
packages/bsp/common/usr/bin/armbian-install around line 208: the script
currently creates "${TempDir}/rootfs/var/log/journal" while rsync may still be
copying into "${TempDir}/rootfs", causing a race; move the directory creation
out of the active rsync window — either create it immediately before the rsync
invocation (before line 199) so the path exists when rsync runs, or defer
creation until after the background rsync block completes (after line 251);
update the script to perform the mkdir -p check at that single, safer point and
remove the line at 208.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
config/bootscripts/boot-generic.cmd.template.md (1)
29-38: Tighten Markdown examples and vendor naming.A few small documentation nits:
- Line 32:
rockchips64isn’t a standard Armbian family name; suggestrockchip64to match existing Rockchip families.- Lines 43–47: the Orange Pi Zero board-config example fence has no language; markdownlint prefers an explicit one.
- Lines 83–86: same for the
SERIALCONexample.You could address all three with something like:
-|`BOOTSCRIPT_TEMPLATE__BOARD_VENDOR`|The vendor of the board, e.g. `allwinner`, `rockchips64`.| +|`BOOTSCRIPT_TEMPLATE__BOARD_VENDOR`|The vendor of the board, e.g. `allwinner`, `rockchip64`.| -Example of a board configuration file for the Orange Pi Zero: -``` +Example of a board configuration file for the Orange Pi Zero: +```conf # Allwinner H2+ quad core 256/512MB RAM SoC WiFi SPI ... -... -``` +... +``` -``` -SERIALCON="ttyS0:115200,ttyGS0" -``` +```bash +SERIALCON="ttyS0:115200,ttyGS0" +```This keeps content intact while satisfying linting and matching Armbian naming conventions.
Also applies to: 43-48, 83-86
🧹 Nitpick comments (4)
config/kernel/linux-rockchip64-current.config (1)
3152-3152: Cross-branch consistency confirmed — CONFIG_NVMEM_ROCKCHIP_OTP=y matches edge config.The main change aligns with
linux-rockchip64-edge.config(line 3113). For feature parity, consider also enablingCONFIG_QRTR_MHI=mif WWAN support is planned, as it's already present in the edge variant (line 517).config/boards/youyeetoo-r1-v3.csc (1)
13-13: Clarify if BOOT_FDT_FILE declaration is still needed.Line 13 sets
BOOT_FDT_FILE="rockchip/rk3588s-youyeetoo-r1.dtb", but line 43 (in thepost_family_config__youyeetoo_r1_use_mainline_uboot()hook) unsets it. If the hook always runs and the board always uses mainline U-Boot, then line 13's declaration may be unnecessary.Please clarify:
- Does the board support vendor U-Boot builds where line 13's value would be used?
- Or can line 13 be removed since
BOOT_FDT_FILEis always unset for mainline U-Boot?action.yml (1)
143-169: Harden version parsing against degenerate inputs.The overall flow (strip optional
v, drop-rc/+meta, bump patch, preserve prefix) is sound, but a couple of edge cases could produce odd versions:
- If
BASE_VERSIONis just a single component like24(orv24),COREbecomes24, soIFS='.' read -r MAJOR MINOR PATCH <<<"$CORE"yieldsMAJOR=24,MINOR='',PATCH='', andNEXT_VERSIONbecomes24..1.- If
os/stable.jsonis missing/invalid or.versionis empty/null,BASE_VERSIONmay be unusable and you still proceed to computeNEXT_VERSION.To make this more robust while preserving current behavior for normal
MAJOR.MINOR.PATCHvalues, consider defaulting MINOR/PATCH and restoring IFS locally, for example:- IFS='.' read -r MAJOR MINOR PATCH <<<"$CORE" - PATCH=${PATCH:-0} + old_ifs="$IFS" + IFS='.' read -r MAJOR MINOR PATCH <<<"$CORE" + IFS="$old_ifs" + MINOR=${MINOR:-0} + PATCH=${PATCH:-0}…and optionally failing early (or logging) if
MAJORisn’t numeric.This keeps the new scheme but avoids accidental
X..Youtputs when the source version is malformed or too short.config/sources/families/k3-beagle.conf (1)
16-27: Consider aligning branch naming with vendor-branch convention.Functionally this looks fine (separate
LINUXFAMILY, vendorKERNELSOURCE, consistent branches/tags), but thecurrent | current-rt)case is pointing at a BeagleBoard vendor kernel (https://github.com/beagleboard/linux,v6.12.49-ti-arm64-r56). Recent K3 guidance prefers naming such branchesvendor/vendor-rtinstead ofcurrent/edge, with matchingKERNEL_TARGETentries in board configs, to clearly distinguish vendor vs mainline branches and avoid confusion across families. Based on learningsIf you’re planning further K3 cleanups, it may be worth switching this case to
vendor | vendor-rt)and updating the relevant board configs in a follow-up.
| # helios64 points fancontrol to /dev symlinks created by helios64 udev rules to sysfs hwmon and thermal directories | ||
| install -m 644 $SRC/packages/bsp/helios64/fancontrol.service.privatedevices-override $destination/etc/systemd/system/fancontrol.service.d/privatedevices.conf | ||
|
|
There was a problem hiding this comment.
Unrelated change: Move fancontrol PrivateDevices override to a separate PR.
This change adds a systemd override for the helios64 fancontrol service, which is completely unrelated to the PR's stated objective of enabling bootscript templating. While the implementation appears technically correct (proper systemd override mechanism, appropriate file permissions, and clear explanatory comment), bundling unrelated changes makes PRs harder to review, test, and track.
Please move this fancontrol configuration change to a separate PR focused on helios64 board improvements.
🤖 Prompt for AI Agents
In config/boards/helios64.conf around lines 44-46, the added systemd override
install line for fancontrol is unrelated to the bootscript templating PR; remove
the install line and accompanying comment from this commit/branch so the current
PR only contains bootscript templating changes, then create a new separate PR
for the helios64 fancontrol override that adds the install of
$SRC/packages/bsp/helios64/fancontrol.service.privatedevices-override to
$destination/etc/systemd/system/fancontrol.service.d/privatedevices.conf with a
clear commit message describing it as a helios64 board/fancontrol configuration
change.
| CONFIG_MPLS_ROUTING=m | ||
| CONFIG_MPLS_IPTUNNEL=m | ||
| CONFIG_HSR=m | ||
| CONFIG_QRTR_MHI=m |
There was a problem hiding this comment.
🧩 Analysis chain
QRTR over MHI enabled (good fit with WWAN stack).
Looks consistent with existing MHI and WWAN configs here (e.g., MHI_BUS_PCI_GENERIC, MHI_WWAN_*). Consider enabling the base QRTR (if not already set via dependencies) across branches for parity.
Run to confirm related dependencies are enabled in this defconfig:
Based on learnings
🏁 Script executed:
#!/bin/bash
rg -nP -C2 '^(CONFIG_QRTR(_TUN)?|CONFIG_MHI_BUS_PCI_GENERIC|CONFIG_MHI_WWAN_.+)=([my])' config/kernel/linux-rockchip64-edge.configLength of output: 428
Need to verify if the base CONFIG_QRTR option itself is enabled. The previous search checked for suffixed variants but not the base config. Let me run another check:
Now I need to check if the base CONFIG_QRTR is present in the file. The dependencies show it's required for CONFIG_QRTR_MHI to function:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check for base CONFIG_QRTR (without suffix) in the defconfig
grep -n "^CONFIG_QRTR=" config/kernel/linux-rockchip64-edge.config
Length of output: 0
Let me check if the base CONFIG_QRTR is explicitly set in other rockchip64 branches to understand the maintainer pattern:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check for base CONFIG_QRTR in other rockchip64 branch configs
fd -t f 'linux-rockchip64' config/kernel/ -x grep -l "^CONFIG_QRTR=" {} \;
echo "---"
# Also check what versions exist
fd -t f 'linux-rockchip64' config/kernel/
Length of output: 92
Let me check the broader pattern in this config to understand if base options are typically set explicitly for feature stacks like this:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if base options are typically set explicitly for similar feature stacks
rg -nP '^CONFIG_(WWAN|MHI_BUS|NET)=' config/kernel/linux-rockchip64-edge.config | head -20
Length of output: 35
Add base CONFIG_QRTR option for consistency with established defconfig pattern.
Verification confirms CONFIG_QRTR variants (QRTR_MHI, QRTR_TUN) are present, but the base CONFIG_QRTR is missing. This breaks the established pattern where base options like CONFIG_NET and CONFIG_WWAN are explicitly set. Add CONFIG_QRTR=m alongside the existing variants for clarity and parity.
🤖 Prompt for AI Agents
In config/kernel/linux-rockchip64-edge.config around line 517, the file defines
CONFIG_QRTR_MHI=m but omits the base CONFIG_QRTR option; add a line setting
CONFIG_QRTR=m adjacent to the existing QRTR variant(s) so the defconfig follows
the established pattern (explicitly enable the base QRTR symbol as a module
alongside CONFIG_QRTR_MHI and any other QRTR_* variants).
|
Something weird happened here. |
I think this is about me and my habits of using continuous/cmsynergy/clearcase/subversion. Somehow I keep on getting into situations with git that make my head spin. I'm going to close this PR and make a fresh one. |
|
No worries, ok. |
12 participants
envsubstfor bootscripts with the filename extension.template.Description
In order to prepare for a generic bootscript, introduce simple templating for bootscripts using
envsubst. The templating is needed as some things in the generic bootscript do require customization, where templating offers easy customization using already-existing shell variables. For example,load_addr, serial consoles, memory alignment settings andfamily/vendornames to enable proper name resolving for DT and DT overlay files.Documentation summary for feature / change
If bootscript name ends with the
.templateextension (could not find a more suitable extension),envsubstwill be invoked to render the bootscript [template]. Rendering will consist of replacing any occurrance of a variable named$BOOTSCRIPT_TEMPLATE__xxxwith the value in the environment of the build system at the time of rendering, in the bootscript template.Some things to take into consideration,
BOOTSCRIPTin theboards/config/xxxfiles can be overridden bysources/families/xxxorsources/families/includes/xxx. For example, forboards/config/orangepizero.csc, the setting forBOOTSCRIPTwill be overridden bysources/families/includes/sunxi-common.inc. This can be solved with the following construct insources/families/includes/sunxi-common.inc:For every board, this has to be inspected to make sure the bootscript selection is done as expected.
How Has This Been Tested?
BOOTSCRIPTdeclaration insources/families/includes/sunxi-common.inc.boot/boot.cmd:Checklist:
Summary by CodeRabbit
New Features
Documentation
Chores
Other
UX