use FIT image for ti_j784s4 flasher#510
use FIT image for ti_j784s4 flasher#510michalskrivanek wants to merge 1 commit intojumpstarter-dev:mainfrom
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughSeveral new files and scripts were added to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant build_flasher.sh
participant Buildroot
participant mkimage
User->>build_flasher.sh: Execute script
build_flasher.sh->>Buildroot: Clone repository and copy overlay/defconfig
build_flasher.sh->>Buildroot: Configure and build kernel/rootfs
build_flasher.sh->>mkimage: Create flasher.itb from flasher.its
mkimage-->>build_flasher.sh: flasher.itb created
build_flasher.sh-->>User: Build complete, FIT image ready
sequenceDiagram
participant U-Boot
participant flasher.itb
participant Kernel
participant Initrd
participant DTB
U-Boot->>flasher.itb: Load FIT image at 0x90000000
U-Boot->>Kernel: Extract and boot kernel
U-Boot->>Initrd: Pass initrd to kernel
U-Boot->>DTB: Pass DTB to kernel
Kernel->>System: Boot system with initrd and DTB
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/overlay/etc/network/if-pre-up.d/wait-carrier.sh (3)
5-7: Quote variable expansions for safety
Commands likeip link set ${IFACE} upand theprintfcall should quote$IFACEto prevent word-splitting or globbing if the variable is empty or contains special characters:-ip link set ${IFACE} up +ip link set "${IFACE}" up
8-16: Use portable arithmetic and guard comparisons
Relying on assignment inside an arithmetic expansion (: $((IF_WAIT_DELAY -= 1))) isn’t guaranteed in all shells. Also, wrap numeric comparisons in quotes:-while [ ${IF_WAIT_DELAY} -gt 0 ]; do +while [ "$IF_WAIT_DELAY" -gt 0 ]; do … - : $((IF_WAIT_DELAY -= 1)) + IF_WAIT_DELAY=$((IF_WAIT_DELAY - 1)) done
17-18: Improve timeout message formatting
The timeout message is concatenated to the dots. Prepend a newline and include the interface in the message for clarity:-printf " timeout!\n" +printf "\nTimeout waiting for carrier on %s!\n" "${IFACE}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/flasher.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml(2 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/overlay/etc/network/if-pre-up.d/wait-carrier.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/ti_defconfig(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e
🔇 Additional comments (3)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/flasher.its (1)
1-74: New FIT image tree definition looks solid
The FIT source correctly embeds the kernel, two DTBs, and the initrd with proper metadata and configurations. No issues detected.packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/ti_defconfig (1)
1-29: Verify external toolchain and overlay integration
This defconfig enablesBR2_TOOLCHAIN_EXTERNALbut doesn’t set an external toolchain path or prefix, which can cause build failures unless provided elsewhere (e.g., CI environment variables). Also, confirm thatBR2_ROOTFS_OVERLAY="$(CONFIG_DIR)/overlay"correctly resolves to your overlay directory.Could you confirm that the external toolchain location is specified in your build environment and that the overlay path resolves as expected?
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml (1)
28-29: Verify FIT image file path and memory address
Ensuredata/flasher.itbmatches the bundled artifact location and thataddress: "0x90000000"aligns with your memory map.
| # - "dd if=/dev/zero of=/dev/mmcblk0 bs=512 count=34" | ||
| # - "dd if=/dev/zero of=/dev/mmcblk1 bs=512 count=34" | ||
| kernel: | ||
| file: data/J784S4XEVM.flasher.img |
There was a problem hiding this comment.
Ok, so there is a problem with itb images.
We stop declaring the dtb section, which tells the flasher what the expected address is,
also I suspect that the current code does not support dtb to be empty.
The reason why we at least want that info and a separate dtb, is that we want to let customers switch the DTB
in realtime, for example, when they have a derivative of the board that they have evolved into product.
i.e. j flasher flash image.raw --dtb my-dtb.dtb
The flasher needs to know the address where to put the dtb provided by customer in that case.
Also we may want to let customers derive the OCI images and put their dtb in they want.
Can we provide an image with just the kernel + initrd , and then two separate .dtbs, one for
J784S4EVM and another for SK-AM69?
The other option is that we rewire the reflasher to fully understand fit, but it would still miss the ability to change the bundled dtb, kernel or ramdisk in "realtime".
There was a problem hiding this comment.
empty dtb has been added by jumpstarter-dev/jumpstarter@f44dfec
I think it is quite flexible this way, the bundling allows us to "ship" dtbs that we anyway ship in kernel-automotive, but it also allows anything else to be loaded. each FIT component is extractable by imxtract and swapped with anything else
There was a problem hiding this comment.
No file dtb could be ok, but, if we want to let customers pushing a custom dtb, we will need the loading address at least.
With FIT is it possible to know where the .dtb address will land?
There was a problem hiding this comment.
Ah I see, yes 0x88000000 in this case.
Can we keep the dtb structure, with just the address? we may need to verify that the client/driver are fine with empty file, just address.
There was a problem hiding this comment.
added example of custom dtb
There was a problem hiding this comment.
it may work with am69 as well, supposedly it should try to load a compatible configuration. I havent' tried that. If it doesn't work the only change is to boot the right config with bootm 0x90000000#am69 or to supply another DTB externally
There was a problem hiding this comment.
> v = cls.model_validate(yaml.safe_load(f))
E pydantic_core._pydantic_core.ValidationError: 1 validation error for FlasherBundleManifestV1Alpha1
E spec.dtb
E Field required [type=missing, input_value={'manufacturer': 'Texas I...address': '0x90000000'}}, input_type=dict]
E For further information visit https://errors.pydantic.dev/2.11/v/missing
It's failing the unit tests there because it's looking for the .dtb field,
I will propose some changes on top to make it compatible with embedded dtb, and also allow dtb injection in runtime (note this is not supported yet on the CLI, only planned and the drivers have a notion about it..)
There was a problem hiding this comment.
Can you add for now the dtb section of the yaml with the two variants for now? (just add a comment saying that it's redundant)
I know it's redundant with the embedded itb?, but I would like to merge it now, and then later on we can add to the code the ability to understand embedded itb dtbs, externally provided, in the yaml, etc.. to make it flexible.
But currently the code will fail if the dtb section is missing, and if we don't add it the later injection won't be possible.
There was a problem hiding this comment.
I don't see a test failure. did benny fix that in his rcars4 mr?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh (2)
1-1: 🛠️ Refactor suggestionEnable strict shell mode
Addset -euo pipefailimmediately after the shebang to catch errors, unset variables, and pipeline failures early.Proposed diff:
#!/bin/bash +set -euo pipefail
9-10:⚠️ Potential issueCorrect defconfig reference and handle
cdfailure
The script currently usesrenesas_s4_defconfig, but this directory providesti_defconfig. Also wrapcdin a failure guard.Proposed diff:
-cp renesas_s4_defconfig /buildroot/configs/ -( cd /buildroot; make renesas_s4_defconfig && make ) +cp ti_defconfig /buildroot/configs/ti_defconfig +cd /buildroot || { echo "Failed to enter /buildroot"; exit 1; } +make ti_defconfig +make🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🧹 Nitpick comments (3)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh (3)
3-3: Improve package installation reproducibility
Consider splitting the longdnf installinto multiple lines for readability and pinning metadata or disabling plugins (e.g.,--disableplugin=subscription-manager) to ensure consistent CI builds.
7-8: Checkadd_kernel.shand overlay before use
Ensureadd_kernel.shsucceeds and thatoverlayexists to avoid silent failures:./add_kernel.sh || { echo "add_kernel.sh failed"; exit 1; } [ -d overlay ] || { echo "overlay directory missing"; exit 1; } cp -R overlay /buildroot
11-11: Verify FIT image tool availability
Check thatmkimage(fromuboot-tools) is present before invoking it, to fail fast if prerequisites are missing:command -v mkimage >/dev/null 2>&1 || { echo "mkimage not found"; exit 1; } mkimage -f flasher.its flasher.itb
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/flasher.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml(2 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/overlay/etc/network/if-pre-up.d/wait-carrier.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/ti_defconfig(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/overlay/etc/network/if-pre-up.d/wait-carrier.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/flasher.its
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/ti_defconfig
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e
🔇 Additional comments (1)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh (1)
5-5: Guard against existing/buildrootdirectory
If/buildrootalready exists,git clonewill fail. You may want to remove or rename any preexisting directory, or clone into a fresh path and then symlink.Would you like assistance adding an idempotent clone wrapper?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh (1)
1-2: Enable strict mode at script start
Add error-handling flags immediately after the shebang to catch pipeline failures, unset variables, and any command errors early.#!/bin/bash +set -euo pipefail
🧹 Nitpick comments (4)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh (4)
3-3: Refactor long package install command for readability
The single-linednf installis hard to scan and maintain. Consider splitting the package list across multiple lines with backslashes or grouping related packages together.
5-5: Ensure a clean Buildroot clone
If/buildrootalready exists,git clonewill fail. Add a cleanup or existence check before cloning to make this step idempotent. For example:- git clone --depth 1 --branch 2025.05-rc2 https://github.com/buildroot/buildroot /buildroot + rm -rf /buildroot + git clone --depth 1 --branch 2025.05-rc2 https://github.com/buildroot/buildroot /buildroot
7-9: Validate and invokeadd_kernel.shexplicitly
Before running, verifyadd_kernel.shexists and is executable. Then invoke it with an explicit shell to ensure consistent behavior:- ./add_kernel.sh + if [[ ! -x ./add_kernel.sh ]]; then + echo "Error: add_kernel.sh not found or not executable" >&2 + exit 1 + fi + bash ./add_kernel.sh
11-11: Specify target architecture in FIT image generation
Be explicit about the architecture when invokingmkimageto avoid defaults and improve clarity:-mkimage -f flasher.its flasher.itb +mkimage -A aarch64 -f flasher.its flasher.itb
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/flasher.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml(2 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/overlay/etc/network/if-pre-up.d/wait-carrier.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/ti_defconfig(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/overlay/etc/network/if-pre-up.d/wait-carrier.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/flasher.its
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/ti_defconfig
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e
|
the current version has initramfs embedded in the kernel, exceeding the default size uboot can handle. i'll try with separate initrd as well.... |
|
keeping separate initrd the size is within the default CONFIG_SYS_BOOTM_LEN (64MB) so let's go with that, we can boot with a simple "bootm" |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh (2)
1-2: 🛠️ Refactor suggestionEnable strict Bash mode
Addset -euo pipefailimmediately after the shebang to catch errors, treat unset variables as failures, and handle pipeline errors reliably.
10-10: 🛠️ Refactor suggestionHarden and parallelize the Buildroot build step
Add a directory-change failure check and use all CPU cores:-( cd /buildroot; make ti_defconfig && make ) +( cd /buildroot || exit 1 + make ti_defconfig + make -j"$(nproc)" +)This guarantees the script fails fast if
cdfails and speeds up the build.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🧹 Nitpick comments (4)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh (4)
3-3: Refactor long package install command for readability
Consider breaking thednf installinvocation into multiple lines or loading the package list from an array/variable. This will make future updates (adding/removing packages) much easier to maintain.
5-5: Guard against existing/buildrootdirectory
To make this script idempotent, check if/buildrootalready exists before cloning. You could exit with a clear message or remove the directory if it's safe to do so.
7-7: Remove or document commented-out code
The#./add_kernel.shline is orphaned. Either delete it or add aTODO/HACKcomment explaining when it should be enabled to avoid confusion.
11-11: Add input validation and logging formkimage
Verify thatflasher.itsexists before runningmkimageand emit a log line:[ -f flasher.its ] || { echo "ERROR: flasher.its not found"; exit 1; } echo "Building FIT image: flasher.itb" mkimage -f flasher.its flasher.itbThis makes failures easier to diagnose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/flasher.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml(2 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/overlay/etc/network/if-pre-up.d/wait-carrier.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/ti_defconfig(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/overlay/etc/network/if-pre-up.d/wait-carrier.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/ti_defconfig
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/flasher.its
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh (3)
1-2: 🛠️ Refactor suggestionEnable strict error handling
Addset -euo pipefailimmediately after the shebang to catch errors early, treat unset variables as failures, and propagate pipeline errors.#!/bin/bash +set -euo pipefail
7-9: 🛠️ Refactor suggestionUse script‐absolute paths for resources
Relativecpcalls can break when invoked from another CWD. Capture the script directory and reference assets via$SCRIPT_DIR.#!/bin/bash +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" - cp -R overlay /buildroot - cp ti_defconfig /buildroot/configs/ +cp -R "${SCRIPT_DIR}/overlay" /buildroot +cp "${SCRIPT_DIR}/ti_defconfig" /buildroot/configs/
10-10: 🛠️ Refactor suggestionHarden and parallelize the Buildroot build
Checkcdsuccess and leverage all cores for faster builds:-( cd /buildroot; make ti_defconfig && make ) +( cd /buildroot || exit 1 + make ti_defconfig + make -j"$(nproc)" +)🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🧹 Nitpick comments (2)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh (2)
11-11: Quote FIT recipe and validate paths
To avoid ambiguity, reference the ITS file via the script directory and ensure the output directory exists:-mkimage -f flasher.its data/flasher.itb +mkdir -p data +mkimage -f "${SCRIPT_DIR}/flasher.its" data/flasher.itb
7-7: Remove or document commented-outadd_kernel.sh
The#./add_kernel.shline appears stale. Either remove it or add a comment explaining why it remains disabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/flasher.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml(2 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/overlay/etc/network/if-pre-up.d/wait-carrier.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/ti_defconfig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/ti_defconfig
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/flasher.its
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/overlay/etc/network/if-pre-up.d/wait-carrier.sh
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
🔇 Additional comments (1)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh (1)
3-3: Thednf installline is straightforward and uses-yand disables weak deps. No immediate action needed here.
|
|
||
| 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 | ||
|
|
||
| git clone --depth 1 --branch 2025.05-rc2 https://github.com/buildroot/buildroot /buildroot |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden the git clone step
Ensure the script exits if the clone fails, preventing downstream errors:
-git clone --depth 1 --branch 2025.05-rc2 https://github.com/buildroot/buildroot /buildroot
+git clone --depth 1 --branch 2025.05-rc2 https://github.com/buildroot/buildroot /buildroot || 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.
| git clone --depth 1 --branch 2025.05-rc2 https://github.com/buildroot/buildroot /buildroot | |
| git clone --depth 1 --branch 2025.05-rc2 https://github.com/buildroot/buildroot /buildroot || exit 1 |
🤖 Prompt for AI Agents
In
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh
at line 5, the git clone command does not currently check for failure. Modify
the script to immediately exit if the git clone fails by adding error handling,
such as using 'set -e' at the script start or appending '|| exit 1' after the
git clone command to prevent downstream errors.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh (4)
1-2: Enable strict mode at script start
Addset -euo pipefailimmediately after the shebang to exit on errors, treat unset variables as errors, and catch pipeline failures.
5-5: Harden Buildroot clone step
Check the exit status of thegit clonecommand to avoid downstream failures:-git clone --depth 1 --branch 2025.05-rc2 https://github.com/buildroot/buildroot /buildroot +git clone --depth 1 --branch 2025.05-rc2 https://github.com/buildroot/buildroot /buildroot || exit 1
8-9: Use absolute paths for overlay and defconfig copies
Relative paths can break if the script is invoked from another directory. Capture the script’s directory and use it:SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" cp -R "${SCRIPT_DIR}/overlay" /buildroot cp "${SCRIPT_DIR}/ti_defconfig" /buildroot/configs/
10-10: Improve build reliability and performance
Ensurecdfailure is caught and parallelize the Buildroot compilation:-( cd /buildroot; make ti_defconfig && make ) +( cd /buildroot || exit 1 + make ti_defconfig + make -j"$(nproc)" +)🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🧹 Nitpick comments (1)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh (1)
11-11: Validate FIT image creation
Verify thatmkimagesucceeds and provide feedback:-mkimage -f flasher.its data/flasher.itb +mkimage -f flasher.its data/flasher.itb || exit 1 +echo "FIT image created at data/flasher.itb"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/flasher.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml(2 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/overlay/etc/network/if-pre-up.d/wait-carrier.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/ti_defconfig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/overlay/etc/network/if-pre-up.d/wait-carrier.sh
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/flasher.its
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/ti_defconfig
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
| @@ -0,0 +1,11 @@ | |||
| #!/bin/bash | |||
|
|
|||
| 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 | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure script is run with root privileges
The dnf install command requires root. Exit early if the script is not executed as root:
if [ "$(id -u)" -ne 0 ]; then
echo "Please run as root" >&2
exit 1
fi🤖 Prompt for AI Agents
In
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh
at line 3, the script runs a dnf install command that requires root privileges.
Add a check at the start of the script to verify if the user ID is 0 (root). If
not, print "Please run as root" to stderr and exit with status 1 to prevent the
script from continuing without necessary permissions.
using latest Fedora kernel. include DTBs for J784S4XEVM and SK-AM69
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh (5)
10-10: Checkcdsuccess and parallelise the Buildroot build
ShellCheck (SC2164) still complains. Also,make -j"$(nproc)"dramatically reduces build time.-( cd /buildroot; make kernel_and_rootfs_defconfig && make ) +( cd /buildroot || exit 1 + make kernel_and_rootfs_defconfig + make -j"$(nproc)" +)
1-2: Enable strict & safe-execution mode at script start
set -euo pipefail(plus an optionalIFS=$'\n\t') catches command failures, unset variables, and broken pipelines early. This was flagged in earlier reviews but is still missing.#!/bin/bash +set -euo pipefail
1-3: Verify root privileges before running package installs
dnf installwill fail for non-root users. Add a quick UID check and exit with a clear message.set -euo pipefail + +if [ "$(id -u)" -ne 0 ]; then + echo "Please run as root" >&2 + exit 1 +fi
5-5: Harden thegit clonestep (again)
Either rely on strict mode (preferred) or append|| exit 1so a network/auth failure stops the build. Also trim the double space after the branch specifier.-git clone --depth 1 --branch 2025.05 https://github.com/buildroot/buildroot /buildroot +git clone --depth 1 --branch 2025.05 https://github.com/buildroot/buildroot /buildroot(strict mode will catch failures)
8-9: Use script directory for asset copying to avoid path issues
Running the script from outside its directory breaks these relativecpcommands.-SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -cp -R overlay /buildroot -cp kernel_and_rootfs_defconfig /buildroot/configs/ +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +cp -R "${SCRIPT_DIR}/overlay" /buildroot +cp "${SCRIPT_DIR}/kernel_and_rootfs_defconfig" /buildroot/configs/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/flasher.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/kernel_and_rootfs_defconfig(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml(2 hunks)packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/overlay/etc/network/if-pre-up.d/wait-carrier.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/kernel_and_rootfs_defconfig
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/overlay/etc/network/if-pre-up.d/wait-carrier.sh
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/manifest.yaml
- packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/flasher.its
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd 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.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
| cp -R overlay /buildroot | ||
| cp kernel_and_rootfs_defconfig /buildroot/configs/ | ||
| ( cd /buildroot; make kernel_and_rootfs_defconfig && make ) | ||
| mkimage -f flasher.its data/flasher.itb |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure output directory exists before running mkimage
mkimage -f flasher.its data/flasher.itb will fail if data/ is absent. Guard against this:
mkdir -p data
mkimage -f flasher.its data/flasher.itb🤖 Prompt for AI Agents
In
packages/jumpstarter-driver-flashers/oci_bundles/ti_j784s4xevm/build_flasher.sh
at line 11, the command mkimage writes to data/flasher.itb but will fail if the
data directory does not exist. Fix this by adding mkdir -p data before the
mkimage command to ensure the output directory exists.
|
This was finally implemented in https://github.com/jumpstarter-dev/jumpstarter/pull/535/files |
2 participants
using latest Fedora kernel.
It doesn't boot with bootm properly, perhaps due to some uboot misconfig, but we can still use the FIT format.
Summary by CodeRabbit