builroot-based flasher images#535
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
## Walkthrough
This update introduces structured support for Device Tree Blob (DTB) variants in the flasher driver and bundle specification. Manifest and driver logic now allow selecting and retrieving DTB-specific boot commands and files. Multiple new test cases, manifests, and build scripts were added to support and verify variant-based flashing and booting for different hardware platforms.
## Changes
| File(s) | Change Summary |
|---------------------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------|
| bundle.py, client.py, driver.py | Refactored DTB handling: introduced `Dtb`/`DtbVariant` models, added bootcmd/file fields, updated method signatures and logic. |
| driver_test.py, test_bundle.py | Added tests for DTB variant boot command retrieval, variant switching, and error handling. |
| oci_bundles/aarch64-itb/automotive.its, buildroot.its | Added DTS files defining multi-variant FIT images for automotive and buildroot kernels. |
| oci_bundles/aarch64-itb/build_fits.sh, replace_kernel.sh, overlay/etc/network/if-pre-up.d/wait-carrier.sh | Added build automation scripts for FIT images, kernel replacement, and network carrier wait. |
| oci_bundles/aarch64-itb/buildroot_defconfig | Added Buildroot configuration for aarch64-itb flasher with kernel, initrd, and DTB settings. |
| oci_bundles/aarch64-itb/manifest-renesas.yaml, manifest-ti.yaml, manifest.yaml | Added/modified manifests for Renesas and TI platforms with structured DTB variants and boot commands. |
| oci_bundles/test/manifest.yaml, ti_j784s4xevm/manifest.yaml | Updated manifest DTB variants to structured mapping with `file` and `bootcmd` keys. |
| build_bundle.sh | Enhanced script to include all YAML manifests and exclude `.gitkeep` files in OCI bundle build. |
| README.md | Documented new `variant`, `manifest`, and `default_target` configuration parameters. |
| jumpstarter_cli/j.py | Added Rich traceback handler installation for improved error output during CLI execution. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant Driver
participant BundleManifest
User->>Driver: use_dtb_variant(variant_name)
Driver->>BundleManifest: get_dtb_file(variant_name)
BundleManifest-->>Driver: DTB file path or error
User->>Driver: get_bootcmd()
Driver->>BundleManifest: get_boot_cmd(variant_name)
BundleManifest-->>Driver: boot command (variant-specific or default)
Driver-->>User: boot commandPossibly related PRs
Poem
Learnt from: bennyz Learnt from: bennyz |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/buildroot_defconfig (2)
7-8: Blank root password – reconsider for production images
BR2_TARGET_GENERIC_ROOT_PASSWD=""leaves the root account without a password.
OK for disposable flashers, but if any board ever ships with this image untouched, it’s an immediate privilege-escalation vector (UART, SSH after user installs it, etc.).If you want password-less console for manufacturing only, consider:
-BR2_TARGET_GENERIC_ROOT_PASSWD="" +# Root remains locked; an overlay or first-boot script can set a password if required +# BR2_TARGET_GENERIC_ROOT_PASSWD is not set…and rely on a provisioning step to unlock it.
19-20: Non-canonical kconfig syntax
BR2_PACKAGE_DROPBEAR=nworks, but Buildroot convention (and whatmake savedefconfigemits) is:# BR2_PACKAGE_DROPBEAR is not setSticking to the canonical form avoids noisy diffs when someone later runs
savedefconfig.packages/jumpstarter-driver-flashers/oci_bundles/buildroot/overlay/etc/network/if-pre-up.d/wait-carrier.sh (1)
5-16: Robustness & POSIX shell nits
ip link set ${IFACE} up– quote${IFACE}to survive funky interface names.$(cat …/carrier)– if the file disappears/misreports you get endlesscat: ...: No such filespam.- The arithmetic decrement inside
:is a Bashism; BusyBoxshsupports it, but plain POSIXdashdoesn’t.Minimal, strictly‐POSIX tweak:
-ip link set ${IFACE} up +ip link set "${IFACE}" up ... - if [ "$(cat /sys/class/net/${IFACE}/carrier)" = "1" ]; then + if [ -r "/sys/class/net/${IFACE}/carrier" ] && + [ "$(cat "/sys/class/net/${IFACE}/carrier")" = 1 ]; then ... - : $((IF_WAIT_DELAY -= 1)) + IF_WAIT_DELAY=$((IF_WAIT_DELAY-1))packages/jumpstarter-driver-flashers/oci_bundles/buildroot/automotive.its (1)
22-24: Typo in DTB description
"DTB Renesas 8779f0-spider"is missing the initialr. Cosmetic, but this string shows up in U-Boot logs.- description = "DTB Renesas 8779f0-spider"; + description = "DTB Renesas r8a779f0-spider";packages/jumpstarter-driver-flashers/oci_bundles/buildroot/build_fits.sh (1)
10-11: ShellCheck SC2164 – harmless hereThe subshell
( cd /buildroot; make … )ignores a failedcd. Withset -eadded earlier, use an explicit guard orcd /buildroot || exit.packages/jumpstarter-driver-flashers/oci_bundles/buildroot/buildroot.its (1)
15-17: Kernel load/entry address hard-coded
0x80000000is fine for many TI boards but double-check the AM69 SPL/U-Boot expects that. Mismatched addresses cause silent hangs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/automotive.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/build_fits.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/buildroot.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/buildroot_defconfig(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/manifest-automotive.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/manifest-buildroot.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/overlay/etc/network/if-pre-up.d/wait-carrier.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/replace_kernel.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/build_fits.sh
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/replace_kernel.sh
[warning] 19-19: Use 'pushd ... || exit' or 'pushd ... || return' in case pushd fails.
(SC2164)
[warning] 24-24: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 25-25: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 26-26: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[error] 31-31: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 36-36: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 46-46: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 50-50: Use 'popd ... || exit' or 'popd ... || return' in case popd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
🔇 Additional comments (4)
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/automotive.its (1)
10-17: Absolute paths makemkimagebrittle
/incbin/("/kernel-automotive/vmlinuz")assumes the build is run from filesystem root with that directory present.
Prefer a relative path so the build can be relocated:- data = /incbin/("/kernel-automotive/vmlinuz"); + data = /incbin/("./kernel-automotive/vmlinuz");Same for the DTB and ramdisk paths below.
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/buildroot.its (1)
32-46: DTB path may be wrong after Buildroot installBuildroot copies DTBs into
output/images/without theti/subdirectory, so:
/buildroot/output/images/k3-j784s4-evm.dtb✅
/buildroot/output/images/ti/k3-j784s4-evm.dtb❌Please verify on a fresh build; if the files land inside a
ti/folder, update the ITS accordingly for both DTBs.packages/jumpstarter-driver-flashers/oci_bundles/buildroot/manifest-automotive.yaml (1)
1-20: Manifest looks syntactically correct – please just runyamllintonce.Nothing blocking, but running a linter will catch accidental indent / quoting issues before they land.
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/replace_kernel.sh (1)
52-58: Module install assumes all objects are.ko.zst; CentOS 9 Automotive kernels ship plain.ko.If a module is not compressed with
zstd,zstd -d …will fail and the script aborts.
Consider detecting the actual extension:mod_path="/kernel-automotive${mod}" if [[ -f "${mod_path}.zst" ]]; then zstd -d "${mod_path}.zst" -o "$ODIR$mod" else cp "${mod_path}" "$ODIR$mod" fi
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/build_fits.sh
Show resolved
Hide resolved
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/build_fits.sh
Show resolved
Hide resolved
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/manifest-buildroot.yaml
Show resolved
Hide resolved
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/replace_kernel.sh
Outdated
Show resolved
Hide resolved
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/replace_kernel.sh
Show resolved
Hide resolved
|
[WIP - still needs the corresponding API changes for selecting the right config] @bennyz @mangelajo this is a combination of #510 and #507, using the single config building both kernel+initrd(6.14 kernel for TI) and kernel-automotive+initrd for S4 The configs are fully reused and the time to build is less as well as the second pass with kernel-automotive is a trivial repackage only (though note the initrds are slightly different due to the extra kernel-automotive modules) This produces two 70MB FITs, the complete bundle would be 140MB. It's not too bad, so I wonder if it's better to split this into two bundles or keep it all together with a single manifest and leave it up to the user to pick one or the other. |
michalskrivanek
force-pushed
the
buildroot_flasher
branch
2 times, most recently
from
8aed798 to
5704735
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/replace_kernel.sh (1)
46-48: Unquoted array expansion – issue already flagged earlier
for mod in ${KMOD[@]}; dore-splits elements and was pointed out in the previous round. Please quote the expansion:-for mod in ${KMOD[@]}; do +for mod in "${KMOD[@]}"; do
🧹 Nitpick comments (2)
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/replace_kernel.sh (2)
53-57: Missing quotes around user-supplied path
$ODIRmay legally contain spaces or glob chars. Quote every occurrence to avoid surprises.-mkdir -p $ODIR/lib/modules $ODIR/etc/init.d || exit 1 +mkdir -p "$ODIR/lib/modules" "$ODIR/etc/init.d" || exit 1 … -zstd -d "/kernel-automotive$mod.zst" -o "$ODIR$mod" +zstd -d "/kernel-automotive$mod.zst" -o "$ODIR$mod"
1-2: Addset -euo pipefailfor safer scripting
Early exit on error, unset vars, and failed pipelines prevents half-baked images from being produced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/automotive.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/build_fits.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/buildroot.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/buildroot_defconfig(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/manifest-automotive.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/manifest-buildroot.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/overlay/etc/network/if-pre-up.d/wait-carrier.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/replace_kernel.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/jumpstarter-driver-flashers/oci_bundles/buildroot/overlay/etc/network/if-pre-up.d/wait-carrier.sh
- packages/jumpstarter-driver-flashers/oci_bundles/buildroot/automotive.its
- packages/jumpstarter-driver-flashers/oci_bundles/buildroot/manifest-automotive.yaml
- packages/jumpstarter-driver-flashers/oci_bundles/buildroot/build_fits.sh
- packages/jumpstarter-driver-flashers/oci_bundles/buildroot/buildroot_defconfig
- packages/jumpstarter-driver-flashers/oci_bundles/buildroot/buildroot.its
- packages/jumpstarter-driver-flashers/oci_bundles/buildroot/manifest-buildroot.yaml
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/replace_kernel.sh
[warning] 19-19: Use 'pushd ... || exit' or 'pushd ... || return' in case pushd fails.
(SC2164)
[warning] 24-24: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 25-25: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 26-26: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[error] 46-46: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 50-50: Use 'popd ... || exit' or 'popd ... || return' in case popd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
🔇 Additional comments (1)
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/replace_kernel.sh (1)
41-41:unzbootutility may be a typo / non-standard
unzbootis not part of common base images (unlikezstd,xz,gzip). If this is a custom tool ensure it’s available in the Buildroot host tools; otherwise the script will fail at runtime.#!/bin/bash command -v unzboot || echo "unzboot missing in PATH"
| pushd /kernel-automotive | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
pushd / popd need failure guards
If the directory change fails the script continues in an unpredictable CWD. Shell-idiom is to bail out:
-pushd /kernel-automotive
+pushd /kernel-automotive || exit 1
…
-popd
+popd || exit 1Also applies to: 50-50
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 19-19: Use 'pushd ... || exit' or 'pushd ... || return' in case pushd fails.
(SC2164)
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-flashers/oci_bundles/buildroot/replace_kernel.sh
at lines 19-20 and 50-51, the pushd commands lack failure checks, which can
cause the script to continue in an unexpected directory if the pushd fails. Add
a conditional check after each pushd to verify it succeeded, and if not, exit
the script immediately with a non-zero status to prevent unpredictable behavior.
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/replace_kernel.sh
Outdated
Show resolved
Hide resolved
michalskrivanek
force-pushed
the
buildroot_flasher
branch
from
5704735 to
a392186
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/replace_kernel.sh (2)
18-20:pushdlacks failure guard – identical to earlier feedback
If/kernel-automotivecannot be entered, the script will continue in an unexpected CWD.-pushd /kernel-automotive +pushd /kernel-automotive || exit 1
50-50:popdshould also be guarded – symmetry with thepushdfix.-popd +popd || exit 1
🧹 Nitpick comments (2)
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/replace_kernel.sh (2)
46-48: Unquoted array expansion – potential word-splitting (SC2068)-for mod in ${KMOD[@]}; do +for mod in "${KMOD[@]}"; doQuoting is cheap, avoids latent bugs, and silences ShellCheck.
52-57: Path handling inside the overlay is brittle – quote$ODIReverywhereSeveral commands concatenate
$ODIRwithout quoting, breaking paths with spaces:-mkdir -p $ODIR/lib/modules $ODIR/etc/init.d || exit 1 +mkdir -p "$ODIR"/lib/modules "$ODIR"/etc/init.d || exit 1 -mkdir -p "$ODIR$(dirname $mod)" -zstd -d "/kernel-automotive$mod.zst" -o "$ODIR$mod" +mkdir -p "$ODIR$(dirname "$mod")" +zstd -d "/kernel-automotive$mod.zst" -o "$ODIR$mod"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/automotive.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/build_fits.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/buildroot.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/buildroot_defconfig(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/manifest-automotive.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/manifest-buildroot.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/overlay/etc/network/if-pre-up.d/wait-carrier.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/buildroot/replace_kernel.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-flashers/oci_bundles/buildroot/manifest-automotive.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/jumpstarter-driver-flashers/oci_bundles/buildroot/overlay/etc/network/if-pre-up.d/wait-carrier.sh
- packages/jumpstarter-driver-flashers/oci_bundles/buildroot/build_fits.sh
- packages/jumpstarter-driver-flashers/oci_bundles/buildroot/automotive.its
- packages/jumpstarter-driver-flashers/oci_bundles/buildroot/buildroot.its
- packages/jumpstarter-driver-flashers/oci_bundles/buildroot/buildroot_defconfig
- packages/jumpstarter-driver-flashers/oci_bundles/buildroot/manifest-buildroot.yaml
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/buildroot/replace_kernel.sh
[warning] 19-19: Use 'pushd ... || exit' or 'pushd ... || return' in case pushd fails.
(SC2164)
[warning] 24-24: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 25-25: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 26-26: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[error] 46-46: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 50-50: Use 'popd ... || exit' or 'popd ... || return' in case popd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
| [ $# -ne 1 ] && { echo "$0 [target_overlay_dir]"; exit 1; } | ||
| ODIR=$1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Usage guard is correct now but message is misleading & $1 should be quoted
[ $# -ne 1 ] correctly enforces “exactly one argument”.
However:
- The help text should follow conventional
Usage: …wording to avoid confusion. $1is later assigned un-quoted (ODIR=$1). If the overlay path contains spaces the script will break.
-if [ $# -ne 1 ]; then
- echo "$0 [target_overlay_dir]"
+if [ "$#" -ne 1 ]; then
+ echo "Usage: $0 <target_overlay_dir>"
exit 1
fi
-ODIR=$1
+ODIR="$1"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-flashers/oci_bundles/buildroot/replace_kernel.sh
at lines 16-17, update the usage message to start with "Usage:" for clarity and
quote the variable assignment by changing ODIR=$1 to ODIR="$1" to handle paths
with spaces correctly.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/build_fits.sh (2)
4-4: Improve package installation readability and add error handling.The package installation command is very long and difficult to read. Consider breaking it into multiple lines or using a package list.
-dnf install --setopt=install_weak_deps=false -y git make gcc gcc-c++ which file diffutils wget cpio rsync bc lzop zip patch perl tar qemu-system-aarch64 qemu-img unzboot uboot-tools kmod awk +# Install build dependencies +PACKAGES=( + git make gcc gcc-c++ which file diffutils wget cpio rsync bc lzop zip + patch perl tar qemu-system-aarch64 qemu-img unzboot uboot-tools kmod awk +) +dnf install --setopt=install_weak_deps=false -y "${PACKAGES[@]}"
6-6: Consider parameterizing the buildroot version and adding cleanup.The buildroot version is hard-coded. Consider making it configurable and adding cleanup on failure.
-git clone --depth 1 --branch 2025.05 https://github.com/buildroot/buildroot /buildroot +BUILDROOT_VERSION=${BUILDROOT_VERSION:-2025.05} +git clone --depth 1 --branch "$BUILDROOT_VERSION" https://github.com/buildroot/buildroot /buildroot + +# Cleanup function +cleanup() { + rm -rf /buildroot +} +trap cleanup EXITpackages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver_test.py (1)
137-143: Test focuses on the wrong method.This test is named
test_drivers_flashers_get_bootcmd_invalid_variantbut only testsuse_dtb_variant. Consider adding an actual test forget_bootcmdwith an invalid variant after setting it.def test_drivers_flashers_get_bootcmd_invalid_variant(complete_flasher): """Test that get_bootcmd raises DriverInvalidArgument for invalid DTB variant""" with serve(complete_flasher) as client: - # Set an invalid variant + # Try to set an invalid variant with pytest.raises(DriverInvalidArgument): client.call("use_dtb_variant", "noexists") + + # Additionally, if a test manifest had an inconsistent state where + # variant is set but not in manifest, test get_bootcmd would failpackages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py (3)
16-16: Add blank line before class definition.PEP 8 requires two blank lines before class definitions.
file: None | str = None + class Dtb(BaseModel):
65-71: Simplify control flow by removing unnecessary else.The else block is redundant after a return statement.
variant_struct = self.spec.dtb.variants.get(variant) if variant_struct: return variant_struct.file - else: - raise ValueError(f"DTB variant {variant} not found in the manifest.") + raise ValueError(f"DTB variant {variant} not found in the manifest.")
80-87: Simplify nested control flow.Remove unnecessary else blocks for cleaner code.
if variant_struct: # If variant has a custom bootcmd, use it; otherwise fall back to default - if variant_struct.bootcmd: - return variant_struct.bootcmd - else: - return self.spec.bootcmd - else: - raise ValueError(f"DTB variant {variant} not found in the manifest.") + return variant_struct.bootcmd if variant_struct.bootcmd else self.spec.bootcmd + raise ValueError(f"DTB variant {variant} not found in the manifest.")packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (1)
182-189: Consider returning None instead of empty string for consistency.Other filename methods (e.g.,
get_initram_filename) returnNonewhen the file doesn't exist. Returning an empty string here breaks consistency.async def get_dtb_filename(self) -> str: """Return the dtb filename""" manifest = await self.get_flasher_manifest() dtb_file = manifest.get_dtb_file(self.variant) if dtb_file: - return Path(dtb_file).name - return "" + return Path(dtb_file).name + return NoneNote: This change would require updating the return type annotation to
str | None.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py(4 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(1 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py(6 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver_test.py(3 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/test_bundle.py(2 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/automotive.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/build_fits.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/buildroot.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/buildroot_defconfig(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest-automotive.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest-buildroot.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/overlay/etc/network/if-pre-up.d/wait-carrier.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/replace_kernel.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/test/manifest.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml
- packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/overlay/etc/network/if-pre-up.d/wait-carrier.sh
- packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/buildroot.its
- packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/buildroot_defconfig
- packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/automotive.its
🧰 Additional context used
🧠 Learnings (3)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver_test.py (3)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:50.888Z
Learning: The TFTP driver (driver.py) already handles error cases and file path security, so the client (client.py) doesn't need to duplicate these safeguards.
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
🧬 Code Graph Analysis (3)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (2)
packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)examples/soc-pytest/jumpstarter_example_soc_pytest/test_on_rpi4.py (1)
console(20-24)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/test_bundle.py (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py (3)
FlasherBundleManifestV1Alpha1(46-114)from_file(106-109)get_boot_cmd(72-87)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver_test.py (3)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (1)
client(63-64)packages/jumpstarter/jumpstarter/client/core.py (1)
DriverInvalidArgument(44-47)packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/replace_kernel.sh
[warning] 19-19: Use 'pushd ... || exit' or 'pushd ... || return' in case pushd fails.
(SC2164)
[warning] 24-24: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 25-25: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 26-26: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[error] 46-46: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 50-50: Use 'popd ... || exit' or 'popd ... || return' in case popd fails.
(SC2164)
🪛 Pylint (3.3.7)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py
[refactor] 16-16: Too few public methods (0/2)
(R0903)
[refactor] 67-70: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 82-85: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py
[refactor] 186-189: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🪛 Flake8 (7.2.0)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py
[error] 16-16: expected 2 blank lines, found 1
(E302)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
🔇 Additional comments (8)
packages/jumpstarter-driver-flashers/oci_bundles/test/manifest.yaml (1)
34-39: LGTM! Enhanced DTB variant structure enables flexible boot command handling.The restructuring from simple file mappings to structured objects with optional
fileandbootcmdfields provides better flexibility for variant-specific boot commands while maintaining clarity.packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
415-418: LGTM! Dynamic boot command retrieval enables variant-aware functionality.The change from static
manifest.spec.bootcmdto dynamicself.call("get_bootcmd")correctly implements the new variant-aware boot command logic, allowing different DTB variants to specify custom boot commands.packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/build_fits.sh (1)
1-21: LGTM! Script follows good practices with room for minor improvements.The script correctly uses
set -euo pipefailfor error handling and follows a logical build process for creating both buildroot and automotive FIT images.packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest-automotive.yaml (1)
1-29: LGTM! Manifest structure aligns with new DTB variant model.The manifest correctly uses the new structured DTB variant format with variant-specific boot commands, enabling flexible configuration for different R-Car S4 board variants.
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/test_bundle.py (2)
3-3: LGTM! Added pytest import for enhanced test functionality.The pytest import is correctly added to support the new test functions that use pytest.raises for error handling tests.
20-54: LGTM! Comprehensive test coverage for new get_boot_cmd functionality.The new test functions provide excellent coverage of the
get_boot_cmdmethod:
- Default behavior without variant specification
- Variant-specific boot commands
- Fallback to default when variant lacks custom bootcmd
- Proper error handling for invalid variants
The tests are well-structured with descriptive names and clear assertions.
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver_test.py (1)
9-143: Comprehensive test coverage for the new boot command functionality!The tests effectively cover all scenarios including default, variant-specific, and error cases. The test suite properly validates the new DTB variant-aware boot command retrieval.
packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/replace_kernel.sh (1)
50-50: Add error handling for popd.-popd +popd || exit 1⛔ Skipped due to learnings
Learnt from: mangelajo PR: jumpstarter-dev/jumpstarter#339 File: packages/jumpstarter-driver-flashers/oci_bundles/build_bundle.sh:12-12 Timestamp: 2025-03-12T16:55:13.031Z Learning: For shell scripts in the Jumpstarter project, prefer using `set -e` at the beginning of scripts to handle errors consistently throughout, rather than adding explicit error checks like `|| exit 1` for individual commands.
packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest-automotive.yaml
Outdated
Show resolved
Hide resolved
| [ $# -ne 1 ] && { echo "$0 [target_overlay_dir]"; exit 1; } | ||
| ODIR=$1 | ||
| mkdir -p /kernel-automotive | ||
| pushd /kernel-automotive |
There was a problem hiding this comment.
Add error handling for directory navigation.
The pushd command can fail if the directory doesn't exist or permissions are insufficient.
-pushd /kernel-automotive
+pushd /kernel-automotive || exit 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pushd /kernel-automotive | |
| pushd /kernel-automotive || exit 1 |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 19-19: Use 'pushd ... || exit' or 'pushd ... || return' in case pushd fails.
(SC2164)
🤖 Prompt for AI Agents
In
packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/replace_kernel.sh
at line 19, the pushd command lacks error handling for directory navigation
failures. Modify the script to check if pushd succeeds by testing its exit
status immediately after the command. If pushd fails, output an error message
and exit the script to prevent further execution in an invalid directory
context.
packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/replace_kernel.sh
Show resolved
Hide resolved
packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/replace_kernel.sh
Show resolved
Hide resolved
packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest-buildroot.yaml
Outdated
Show resolved
Hide resolved
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py
Show resolved
Hide resolved
mangelajo
force-pushed
the
buildroot_flasher
branch
from
7ae7c05 to
b71052e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (1)
185-189: Fix redundant method call and unnecessary else clause.This issue was identified in previous reviews but remains unfixed. The method calls
get_dtb_filetwice unnecessarily.async def get_dtb_filename(self) -> str: """Return the dtb filename""" manifest = await self.get_flasher_manifest() dtb_file = manifest.get_dtb_file(self.variant) if dtb_file: - return Path(manifest.get_dtb_file(self.variant)).name - else: - return "" + return Path(dtb_file).name + return ""
🧹 Nitpick comments (3)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py (3)
16-19: Fix formatting and consider the static analysis warning.The new
Dtbmodel properly encapsulates DTB variant management. However, there are formatting issues to address.+ class Dtb(BaseModel): default: str address: str variants: dict[str, DtbVariant]Note: The static analysis warning about "too few public methods" is a false positive for Pydantic models, which are primarily data containers.
57-70: Simplify control flow by removing unnecessary else clause.The method logic is correct, but the else clause after return is redundant.
def get_dtb_file(self, variant: str | None = None) -> str | None: if not self.spec.dtb: return None # if no variant is provided, use the default variant name from the manifest if not variant: variant = self.spec.dtb.default # look for the variant struct in this manifest variant_struct = self.spec.dtb.variants.get(variant) if variant_struct: return variant_struct.file - else: - raise ValueError(f"DTB variant {variant} not found in the manifest.") + raise ValueError(f"DTB variant {variant} not found in the manifest.")
72-87: Simplify control flow by removing unnecessary else clause.The method implements proper fallback logic for boot commands. However, the else clause after return is redundant.
def get_boot_cmd(self, variant: str | None = None) -> str: if not self.spec.dtb: return self.spec.bootcmd # if no variant is provided, use the default variant name from the manifest if not variant: variant = self.spec.dtb.default # look for the variant struct in this manifest variant_struct = self.spec.dtb.variants.get(variant) if variant_struct: # If variant has a custom bootcmd, use it; otherwise fall back to default if variant_struct.bootcmd: return variant_struct.bootcmd - else: - return self.spec.bootcmd - else: - raise ValueError(f"DTB variant {variant} not found in the manifest.") + return self.spec.bootcmd + raise ValueError(f"DTB variant {variant} not found in the manifest.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/jumpstarter-driver-flashers/README.md(1 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py(4 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(1 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py(6 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver_test.py(3 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/test_bundle.py(2 hunks)packages/jumpstarter-driver-flashers/oci_bundles/test/manifest.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-flashers/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml
- packages/jumpstarter-driver-flashers/oci_bundles/test/manifest.yaml
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/test_bundle.py
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver_test.py
🧰 Additional context used
🧠 Learnings (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
🪛 Pylint (3.3.7)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py
[refactor] 16-16: Too few public methods (0/2)
(R0903)
[refactor] 67-70: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 82-85: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py
[refactor] 186-189: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🪛 Flake8 (7.2.0)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py
[error] 16-16: expected 2 blank lines, found 1
(E302)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
🔇 Additional comments (7)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py (2)
2-2: Good modernization of imports.The addition of
Literalimport supports the new type annotations used throughout the file.
13-15: Type annotations modernized correctly.The explicit union types (
None | str) are clearer than optional type hints and align with modern Python typing practices.packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (5)
20-21: Good refactoring to public attributes.The transition from private
_use_dtbto publicvariantattribute improves the API design and the newmanifestattribute provides flexibility for different manifest files.
88-88: Correct integration with variant-aware DTB handling.The method properly uses the new variant system to get the appropriate DTB file.
102-111: Enhanced validation with clear error messages.The improved variant validation provides better error handling with specific details about available variants.
160-160: Consistent use of configurable manifest attribute.The methods correctly use the
manifestattribute instead of hardcoded filenames, improving flexibility.Also applies to: 164-164
209-213: Well-implemented boot command retrieval.The new method properly delegates to the manifest's variant-aware boot command logic.
A single config building two FIT images supporting similar-enough boards using either upstream or kernel-automotive kernel. Includes TI J784S4EVM, SK-AM69 and Renesas R-Car S4 Spider and SK.
michalskrivanek
force-pushed
the
buildroot_flasher
branch
from
b71052e to
95a7f1f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/replace_kernel.sh (3)
19-19: Wrappushdwith an exit fallback.
Same issue was flagged previously; the fix is still missing.-pushd /kernel-automotive +pushd /kernel-automotive || exit 1
24-27: Quote each element in thepkgsarray.
Prevents glob expansion or word-splitting when$KVERchanges.-pkgs=( - kernel-automotive-core-$KVER.rpm - kernel-automotive-modules-$KVER.rpm - kernel-automotive-modules-core-$KVER.rpm -) +pkgs=( + "kernel-automotive-core-$KVER.rpm" + "kernel-automotive-modules-$KVER.rpm" + "kernel-automotive-modules-core-$KVER.rpm" +)
46-48: Quote theKMODarray expansion in the loop.Unquoted
${KMOD[@]}triggers re-splitting; Shellcheck SC2068 already points this out.-for mod in ${KMOD[@]}; do +for mod in "${KMOD[@]}"; dopackages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest-buildroot.yaml (1)
48-48: Fix YAML syntax error - missing colon.The line is missing a colon after
bootcmd, which will cause YAML parsing to fail.custom: - bootcmd "bootm 0x90000000:kernel 0x90000000:initrd 0x88000000" + bootcmd: "bootm 0x90000000:kernel 0x90000000:initrd 0x88000000"packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (1)
185-189: Optimize method calls and simplify control flow.The code calls
get_dtb_filetwice unnecessarily and has redundant else block.dtb_file = manifest.get_dtb_file(self.variant) if dtb_file: - return Path(manifest.get_dtb_file(self.variant)).name - else: - return "" + return Path(dtb_file).name + return ""
🧹 Nitpick comments (5)
packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/replace_kernel.sh (2)
50-50: Handlepopdfailure symmetrically.-popd +popd || exit 1
52-57: Quote$ODIRto survive spaces and weird paths.-mkdir -p $ODIR/lib/modules $ODIR/etc/init.d || exit 1 +mkdir -p "$ODIR/lib/modules" "$ODIR/etc/init.d" || exit 1 ... - mkdir -p "$ODIR$(dirname $mod)" - zstd -d "/kernel-automotive$mod.zst" -o "$ODIR$mod" + mkdir -p "$ODIR$(dirname "$mod")" + zstd -d "/kernel-automotive$mod.zst" -o "$ODIR$mod"packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py (3)
16-19: Fix formatting and consider adding methods to the Dtb class.The class needs proper spacing and could benefit from utility methods.
+ class Dtb(BaseModel): default: str address: str variants: dict[str, DtbVariant]Consider adding utility methods like
get_variant_names()orhas_variant(name: str)to improve usability.
67-70: Remove unnecessary else block after return.The else block is redundant since the if block returns.
variant_struct = self.spec.dtb.variants.get(variant) if variant_struct: return variant_struct.file - else: - raise ValueError(f"DTB variant {variant} not found in the manifest.") + raise ValueError(f"DTB variant {variant} not found in the manifest.")
72-87: Simplify control flow by removing unnecessary else block.Similar to the previous method, the else block is redundant.
variant_struct = self.spec.dtb.variants.get(variant) if variant_struct: # If variant has a custom bootcmd, use it; otherwise fall back to default if variant_struct.bootcmd: return variant_struct.bootcmd - else: - return self.spec.bootcmd - else: - raise ValueError(f"DTB variant {variant} not found in the manifest.") + return self.spec.bootcmd + raise ValueError(f"DTB variant {variant} not found in the manifest.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/jumpstarter-driver-flashers/README.md(1 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py(4 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(1 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py(6 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver_test.py(3 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/test_bundle.py(2 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/automotive.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/build_fits.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/buildroot.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/buildroot_defconfig(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest-automotive.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest-buildroot.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/overlay/etc/network/if-pre-up.d/wait-carrier.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/replace_kernel.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/test/manifest.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml
- packages/jumpstarter-driver-flashers/oci_bundles/test/manifest.yaml
- packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/buildroot.its
- packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/automotive.its
- packages/jumpstarter-driver-flashers/README.md
- packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/overlay/etc/network/if-pre-up.d/wait-carrier.sh
- packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/build_fits.sh
- packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest-automotive.yaml
- packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/buildroot_defconfig
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/test_bundle.py
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver_test.py
🧰 Additional context used
🧠 Learnings (2)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (2)
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/replace_kernel.sh (2)
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#339
File: packages/jumpstarter-driver-flashers/oci_bundles/build_bundle.sh:12-12
Timestamp: 2025-03-12T16:55:13.031Z
Learning: For shell scripts in the Jumpstarter project, prefer using `set -e` at the beginning of scripts to handle errors consistently throughout, rather than adding explicit error checks like `|| exit 1` for individual commands.
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#339
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:172-172
Timestamp: 2025-03-14T15:52:49.790Z
Learning: Shell command sanitization (using shlex.quote or similar) is not required in the flasher driver context because it operates on devices where full control is already assumed, as the code is flashing the entire disk.
🧬 Code Graph Analysis (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (2)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
manifest(455-462)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py (3)
get_dtb_file(57-70)FlasherBundleManifestV1Alpha1(46-114)get_boot_cmd(72-87)
🪛 Pylint (3.3.7)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py
[refactor] 16-16: Too few public methods (0/2)
(R0903)
[refactor] 67-70: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 82-85: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py
[refactor] 186-189: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🪛 Flake8 (7.2.0)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py
[error] 16-16: expected 2 blank lines, found 1
(E302)
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/replace_kernel.sh
[warning] 19-19: Use 'pushd ... || exit' or 'pushd ... || return' in case pushd fails.
(SC2164)
[warning] 24-24: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 25-25: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 26-26: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[error] 46-46: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 50-50: Use 'popd ... || exit' or 'popd ... || return' in case popd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
🔇 Additional comments (6)
packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/replace_kernel.sh (1)
41-42: Confirmunzbootis available in the build environment.
unzbootisn’t a standard util; if it’s a local helper ensure it’s on$PATHor add an explicit check.packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py (1)
13-14: LGTM - Modern type hint syntax.The migration from
Optional[str]toNone | strfollows modern Python type annotation practices.packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (4)
20-21: LGTM - Well-designed variant support.The addition of
variantandmanifestfields provides flexibility for DTB variant selection and custom manifest files.
88-88: Good integration of variant support in DTB file retrieval.The method correctly passes the variant to
get_dtb_filefor variant-aware DTB selection.
102-111: Excellent validation logic for DTB variants.The method properly validates that the requested variant exists in the manifest and provides helpful error messages with available variants.
209-213: LGTM - Clean implementation of boot command retrieval.The new method provides a clean interface for retrieving variant-aware boot commands from the manifest.
| #!/bin/bash | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add set -euo pipefail right after the shebang.
Fail-fast behaviour is expected for our flasher scripts (see project learning).
This single line removes the need for scattered || exit 1 and gives us safer default handling of unset vars and pipe errors.
#!/bin/bash
+set -euo pipefail🤖 Prompt for AI Agents
In
packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/replace_kernel.sh
at the very beginning (lines 1-2), add the line `set -euo pipefail` immediately
after the shebang `#!/bin/bash`. This will enable fail-fast behavior by causing
the script to exit on any error, unset variable usage, or pipe failure,
improving script safety and removing the need for multiple explicit exit checks.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/jumpstarter-driver-flashers/oci_bundles/build_bundle.sh (1)
24-27: Quote variable expansions or switch to arrays to preserve filenames with spaces
$MANIFESTSand$DATA_FILESare expanded unquoted, so any embedded whitespace will be treated as argument separators by the shell before reachingoras push. After the refactors above, consider building an array and usingoras push "${params[@]}"or, at minimum, wrapping each file entry in double quotes when constructing the strings.This protects against subtle bugs if future artifacts or manifests use filenames containing spaces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest-renesas.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest-ti.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/build_bundle.sh(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest.yaml
- packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest-renesas.yaml
- packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest-ti.yaml
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/build_bundle.sh
[warning] 16-16: Iterating over ls output is fragile. Use globs.
(SC2045)
[warning] 20-20: For loops over find output are fragile. Use find -exec or a while read loop.
(SC2044)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
michalskrivanek
force-pushed
the
buildroot_flasher
branch
2 times, most recently
from
abb6b86 to
4995806
Compare
all *.yaml files are treated as manifests. A default manifest manifest.yaml should still exist
michalskrivanek
force-pushed
the
buildroot_flasher
branch
from
4995806 to
ea20e29
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/jumpstarter-driver-flashers/oci_bundles/build_bundle.sh (2)
15-18:for file in $(ls …)is still unsafe & was already flaggedThe loop continues to rely on command-substituted
ls, which breaks on filenames containing spaces/newlines and trips Shellcheck SC2045. We recommended switching to direct globbing with an existence guard in a previous review but the change hasn’t been applied. Re-using the earlier diff:-MANIFESTS= -for file in $(ls -1 *.yaml); do - MANIFESTS="${MANIFESTS} ${file}:application/yaml " -done +MANIFESTS= +for file in *.yaml; do + [ -e "$file" ] || continue # skip when the glob expands to literal '*.yaml' + MANIFESTS="${MANIFESTS}${file}:application/yaml " +done
20-22: Whitespace-fragilefind … | forloop persistsThe
finditeration still uses plain word-splitting, reproducing SC2044 issues previously reported. Please adopt a null-delimited approach:-DATA_FILES= -for file in $(find ./data -type f -prune -a -not -name .gitkeep); do - DATA_FILES="${DATA_FILES} ${file}:application/octet-stream " -done +DATA_FILES= +while IFS= read -r -d '' file; do + DATA_FILES="${DATA_FILES}${file}:application/octet-stream " +done < <(find ./data -type f ! -name '.gitkeep' -print0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest-renesas.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest-ti.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/overlay/etc/udev/rules.d/10-rename-tsn0.rules(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/build_bundle.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/overlay/etc/udev/rules.d/10-rename-tsn0.rules
- packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest.yaml
- packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest-renesas.yaml
- packages/jumpstarter-driver-flashers/oci_bundles/aarch64-itb/manifest-ti.yaml
🧰 Additional context used
🧠 Learnings (1)
packages/jumpstarter-driver-flashers/oci_bundles/build_bundle.sh (1)
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#339
File: packages/jumpstarter-driver-flashers/oci_bundles/build_bundle.sh:12-12
Timestamp: 2025-03-12T16:55:13.031Z
Learning: For shell scripts in the Jumpstarter project, prefer using `set -e` at the beginning of scripts to handle errors consistently throughout, rather than adding explicit error checks like `|| exit 1` for individual commands.
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/build_bundle.sh
[warning] 16-16: Iterating over ls output is fragile. Use globs.
(SC2045)
[warning] 20-20: For loops over find output are fragile. Use find -exec or a while read loop.
(SC2044)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: e2e
mangelajo
changed the title
mangelajo
force-pushed
the
buildroot_flasher
branch
from
ec96675 to
3d1f825
Compare
3 participants
A single config building two FIT images supporting similar-enough boards using either upstream or kernel-automotive kernel. Includes TI J784S4EVM, SK-AM69 and Renesas R-Car S4 Spider and SK.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores