use FIT image for rcar_s4 flasher#507
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis change introduces a complete build system for creating a flasher image for the Renesas R-Car S4 platform. It adds scripts to automate kernel extraction, module preparation, network setup, and Buildroot-based image construction, along with configuration files, a FIT image descriptor, and the resulting binary flasher image. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant build_flasher.sh
participant add_kernel.sh
participant Buildroot
participant mkimage
User->>build_flasher.sh: Execute build_flasher.sh
build_flasher.sh->>System: Install dependencies (dnf)
build_flasher.sh->>Buildroot: Clone repository
build_flasher.sh->>add_kernel.sh: Run add_kernel.sh
add_kernel.sh->>System: Download and extract kernel & modules
add_kernel.sh->>System: Prepare overlay with modules and init script
build_flasher.sh->>Buildroot: Copy overlay and defconfig
build_flasher.sh->>Buildroot: Configure and build image
build_flasher.sh->>mkimage: Create FIT image (flasher.itb)
mkimage->>build_flasher.sh: Output flasher.itb
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🧹 Nitpick comments (5)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its (1)
18-18: Consider using stronger hashing algorithm.While SHA1 is commonly used in FIT images and supported by U-Boot, consider if SHA256 would be more appropriate for enhanced security, especially for production systems.
If the target U-Boot supports it, consider updating to SHA256:
- algo = "sha1"; + algo = "sha256";Also applies to: 30-30, 40-40, 53-53
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh (1)
10-10: Handle failure when changing to the Buildroot directory
Wrapcdin a failure check to avoid continuing the build in the wrong directory:-( cd /buildroot; make renesas_s4_defconfig && make ) +( cd /buildroot || exit 1; make renesas_s4_defconfig && make )🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (3)
18-18: Remove unused variable assignment
TheBASE=$PWDvariable is set but never used. Consider removing it to improve readability and maintainability.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
23-26: Quote array elements to guard against word splitting
AlthoughKVERcontains no spaces today, quoting array assignments improves future robustness.-pkgs=( - kernel-automotive-core-$KVER.rpm +pkgs=( + "kernel-automotive-core-${KVER}.rpm" kernel-automotive-modules-$KVER.rpm kernel-automotive-modules-core-$KVER.rpm )🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 23-23: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[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)
32-32: Quote array expansions in loops
Prevent word re-splitting by quoting${pkgs[@]}and${KMOD[@]}expansions.-for pkg in ${pkgs[@]}; do +for pkg in "${pkgs[@]}"; do -for pkg in ${pkgs[@]}; do +for pkg in "${pkgs[@]}"; do -for mod in ${KMOD[@]}; do +for mod in "${KMOD[@]}"; doAlso applies to: 38-38, 48-48
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 32-32: Double quote array expansions to avoid re-splitting elements.
(SC2068)
🛑 Comments failed to post (5)
packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/client.py (2)
11-17: 🛠️ Refactor suggestion
Remove redundant method overrides.
The
on()andoff()methods are identical to their parent class implementations inPowerClient. These overrides are unnecessary and add maintenance burden.Remove the redundant method overrides:
- def on(self): - """Turn power on""" - self.call("on") - - def off(self): - """Turn power off""" - self.call("off")The parent
PowerClientclass already provides these exact implementations. If GPIO-specific behavior is needed in the future, these methods can be re-added with the specific logic.📝 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.🤖 Prompt for AI Agents
In packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/client.py around lines 11 to 17, the on() and off() methods override the parent PowerClient class without any changes, making them redundant. Remove these method overrides entirely to avoid unnecessary duplication and maintenance overhead, relying on the inherited implementations instead.
3-4: 💡 Verification agent
🧩 Analysis chain
Add click dependency to resolve pipeline failure.
The pipeline failure indicates that the
clickmodule is not available during testing. Since the CLI functionality depends on click, it needs to be added as a dependency.Check if click is properly declared as a dependency:
🏁 Script executed:
#!/bin/bash # Description: Check if click is declared as a dependency in setup files echo "Checking for click dependency declarations..." # Check setup.py files fd "setup.py" --exec grep -l "click" {} \; # Check pyproject.toml files fd "pyproject.toml" --exec grep -l "click" {} \; # Check requirements files fd "requirements*.txt" --exec grep -l "click" {} \; # Also check the specific package's setup/config files find packages/jumpstarter-driver-raspberrypi -name "*.py" -o -name "*.toml" -o -name "*.txt" | xargs grep -l "click" 2>/dev/null || echo "No click dependencies found in package files"Length of output: 990
Add
clickto the Raspberry Pi driver’s dependenciesThe
client.pynow importsclick, but the Raspberry Pi driver package doesn’t declare it—this is why the CI is failing. Please update the package’s dependency file to includeclick.Points to address:
- File:
packages/jumpstarter-driver-raspberrypi/pyproject.toml(or its equivalent)
- Under
[tool.poetry.dependencies](or[project.dependencies]), add:click = "^<appropriate-version>"- If the package uses a
requirements.txtor similar, addclickthere instead.This will ensure the CLI functionality builds and tests pass.
🧰 Tools
🪛 GitHub Actions: Run Tests
[error] 3-3: ModuleNotFoundError: No module named 'click' - ImportError while importing test module. Make sure the 'click' module is installed.
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/client.py lines 3-4, the code imports the click module but the package does not declare click as a dependency, causing pipeline failures. To fix this, add click to the package's dependency file, such as packages/jumpstarter-driver-raspberrypi/pyproject.toml under [tool.poetry.dependencies] or [project.dependencies], specifying an appropriate version like click = "^8.0". If the package uses requirements.txt instead, add click there. This ensures the click module is installed during builds and tests.packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (1)
207-210: 💡 Verification agent
🧩 Analysis chain
LGTM! Consistent implementation pattern.
The
RCarS4Flasherclass follows the same pattern as the existingTIJ784S4Flasherclass, providing a clean interface for the Renesas R-Car S4 platform.Consider verifying that the flasher bundle image is accessible and properly tagged:
🏁 Script executed:
#!/bin/bash # Description: Verify the R-Car S4 flasher bundle image exists and is accessible # Check if the image exists in the registry curl -s "https://quay.io/api/v1/repository/bzlotnik/jumpstarter-flasher-rcars4/tag/latest" | jq '.name, .size'Length of output: 120
Invalid flasher_bundle image reference
The registry lookup for quay.io/bzlotnik/jumpstarter-flasher-rcars4:latest returned
null, indicating that the repository or the “latest” tag doesn’t exist. Please verify and correct the image path or tag:• File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py
Lines: 207–210Suggested update (replace
<correct-tag>with the valid tag name):- flasher_bundle: str = "quay.io/bzlotnik/jumpstarter-flasher-rcars4:latest" + flasher_bundle: str = "quay.io/bzlotnik/jumpstarter-flasher-rcars4:<correct-tag>"Alternatively, push a “latest” tag for this image to quay.io so that the existing reference resolves correctly.
📝 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.class RCarS4Flasher(BaseFlasher): """RCarS4 driver for Jumpstarter""" flasher_bundle: str = "quay.io/bzlotnik/jumpstarter-flasher-rcars4:<correct-tag>"🤖 Prompt for AI Agents
In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py around lines 207 to 210, the flasher_bundle image reference "quay.io/bzlotnik/jumpstarter-flasher-rcars4:latest" is invalid because the "latest" tag does not exist in the registry. Verify the correct tag name for this image in the quay.io repository and update the flasher_bundle string to use that valid tag instead of "latest". Alternatively, push a "latest" tag to the quay.io repository to match the current reference.packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh (1)
1-3: 🛠️ Refactor suggestion
Enable strict mode for safer script execution
Recommend addingset -euo pipefailandIFS=$'\n\t'after the shebang to:
- Fail fast on errors or unset variables
- Prevent word-splitting issues
#!/bin/bash +set -euo pipefail +IFS=$'\n\t'🤖 Prompt for AI Agents
In packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh at the beginning of the script (lines 1 to 3), add strict mode settings by inserting `set -euo pipefail` and `IFS=$'\n\t'` immediately after the shebang line. This will ensure the script fails fast on errors or unset variables and prevents word-splitting issues, improving script safety and reliability.packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (1)
17-17: 🛠️ Refactor suggestion
Enforce error handling on directory changes
Add|| exit 1tocdcommands to prevent the script from proceeding if a directory change fails.- cd "$(dirname "$(readlink -f "$0")")" + cd "$(dirname "$(readlink -f "$0")")" || exit 1 - cd kernel + cd kernel || exit 1 - cd $OLDPWD + cd "$OLDPWD" || exit 1Also applies to: 29-29, 53-53
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 17-17: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh at lines 17, 29, and 53, the cd commands lack error handling. Append `|| exit 1` to each cd command to ensure the script exits immediately if changing directories fails, preventing further execution in an incorrect context.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh (1)
3-3: Handle DNF errors and improve readabilityLeverage the strict mode enabled above to automatically exit on installation failures, and consider splitting long commands across multiple lines for maintainability. For example:
-dnf install --setopt=install_weak_deps=false -y git make gcc gcc-c++ which file diffutils wget cpio rsync bc zip patch perl tar qemu-system-aarch64 qemu-img unzboot uboot-tools kmod +dnf install \ + --setopt=install_weak_deps=false \ + -y git make gcc gcc-c++ which file diffutils wget cpio rsync bc \ + zip patch perl tar qemu-system-aarch64 qemu-img unzboot uboot-tools kmodpackages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (4)
17-19: Ensurecdsucceeds and remove unusedBASEAdd
|| exit 1aftercdto prevent silent failures. Also, theBASEvariable isn't used elsewhere—consider removing it to clean up the script:-cd "$(dirname "$(readlink -f "$0")")" -BASE=$PWD +cd "$(dirname "$(readlink -f "$0")")" || exit 1 +# BASE variable removed (unused)🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 17-17: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
22-26: Quote array elements to avoid word splittingWrap each RPM filename in quotes to prevent issues if
$KVERcontains unexpected characters:-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" +)🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 23-23: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[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)
37-41: Quote array expansions in extraction loopsDouble-quote array expansions in loops to avoid unintended word splitting, and ensure commands fail fast under strict mode:
-if ! [[ -d lib ]]; then - for pkg in ${pkgs[@]}; do +if ! [[ -d lib ]]; then + for pkg in "${pkgs[@]}"; do echo -n "extracting $pkg ... " rpm2cpio "$pkg" | cpio -id done ... - for mod in ${KMOD[@]}; do + for mod in "${KMOD[@]}"; do modprobe -d "$PWD" -S "$KVER" --show-depends "$mod" done | sed "s|$PWD||; s|^builtin|# builtin|; s|\\.ko\\.zst|.ko|" > modlist fiAlso applies to: 48-51
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 38-38: Double quote array expansions to avoid re-splitting elements.
(SC2068)
53-61: Quote variables and handle errors during overlay assemblyPrevent failures from going unnoticed and handle paths containing spaces:
-cd $OLDPWD +cd "$OLDPWD" || exit 1 mkdir -p "$ODIR/lib/modules" "$ODIR/etc/init.d" || exit 1 echo "installing modules into overlay dir ..." -sed -nr 's|^insmod ||p' < kernel/modlist | while read mod; do - mkdir -p "$ODIR$(dirname $mod)" - zstd -d "kernel$mod.zst" -o "$ODIR$mod" +sed -nr 's|^insmod ||p' < kernel/modlist | while read -r mod; do + mkdir -p "$ODIR$(dirname "$mod")" + zstd -d "kernel${mod}.zst" -o "$ODIR$mod" || exit 1 done🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 53-53: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/renesas_s4_defconfig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/renesas_s4_defconfig
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh
[warning] 17-17: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 23-23: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[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] 29-29: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[error] 32-32: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 38-38: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 48-48: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 53-53: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/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
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh
Show resolved
Hide resolved
| git clone --depth 1 --branch 2025.05-rc2 https://github.com/buildroot/buildroot /buildroot | ||
|
|
||
| ./add_kernel.sh | ||
| cp -R overlay /buildroot/overlay | ||
| cp renesas_s4_defconfig /buildroot/configs/ | ||
| ( cd /buildroot; make renesas_s4_defconfig && make ) | ||
| mkimage -f flasher.its flasher.itb |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error checking around directory operations and improve structure
Wrap clone and build steps with || exit 1 or convert to pushd/popd for clarity, ensuring failures are caught immediately:
git clone --depth 1 --branch 2025.05-rc2 https://github.com/buildroot/buildroot /buildroot
-./add_kernel.sh
+./add_kernel.sh || exit 1
-cp -R overlay /buildroot/overlay
-cp renesas_s4_defconfig /buildroot/configs/
-( cd /buildroot; make renesas_s4_defconfig && make )
+pushd /buildroot || exit 1
+ make renesas_s4_defconfig
+ make
+popd
-mkimage -f flasher.its flasher.itb
+mkimage -f flasher.its flasher.itb || exit 1Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh
around lines 5 to 11, the script lacks error checking for critical commands like
git clone and make. Add error checking by appending '|| exit 1' to these
commands to stop execution on failure. Additionally, replace directory changes
with 'pushd' and 'popd' to improve clarity and ensure the script returns to the
original directory after build steps.
| cat >$script <<EOF | ||
| #!/bin/sh | ||
|
|
||
| if [ "\$1" == "start" ]; then | ||
| $(cat kernel/modlist) | ||
| fi | ||
| EOF |
There was a problem hiding this comment.
Fix POSIX compliance in startup script and secure here-doc
Use = instead of == in /bin/sh, and quote the here-doc delimiter to avoid unintended expansions:
-cat >$script <<EOF
-#!/bin/sh
-
-if [ "$1" == "start" ]; then
-$(cat kernel/modlist)
-fi
-EOF
+cat >"$script" <<'EOF'
+#!/bin/sh
+
+if [ "$1" = "start" ]; then
+ # Load required modules
+ $(cat kernel/modlist)
+fi
+EOF🤖 Prompt for AI Agents
In packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh around
lines 64 to 70, replace the string comparison operator `==` with `=` to ensure
POSIX compliance in the `/bin/sh` script. Additionally, quote the here-doc
delimiter (use `<<'EOF'` instead of `<<EOF`) to prevent unintended variable
expansions within the here-doc content, securing the script.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh (2)
1-2: Enable strict mode for safer execution
Immediately after the shebang, add:+# Enable strict error handling +set -euo pipefail +IFS=$'\n\t'
5-11: 🛠️ Refactor suggestionAdd robust error handling and explicit directory navigation
Once strict mode is enabled, failures will be caught early, but it’s clearer to explicitly handle exit codes and usepushd/popdfor build steps:- 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 - ./add_kernel.sh + ./add_kernel.sh || exit 1 - cp -R overlay /buildroot + cp -R overlay /buildroot || exit 1 - cp renesas_s4_defconfig /buildroot/configs/ + cp renesas_s4_defconfig /buildroot/configs/ || exit 1 - ( cd /buildroot; make renesas_s4_defconfig && make ) + pushd /buildroot || exit 1 + make renesas_s4_defconfig && make || exit 1 + popd - mkimage -f flasher.its flasher.itb + mkimage -f flasher.its flasher.itb || exit 1🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (3)
1-2: Enable strict mode for robustness
After the shebang, add strict flags and a safeIFSto catch errors early and prevent word-splitting issues:+# Enable strict error handling +set -euo pipefail +IFS=$'\n\t'
28-34: Quote array expansions and guard directory change
Ensure thecd kernelcommand doesn’t silently fail and prevent unintended word splitting in package loops:- mkdir -p kernel - cd kernel + mkdir -p kernel + cd kernel || exit 1 - for pkg in ${pkgs[@]}; do + for pkg in "${pkgs[@]}"; do🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 29-29: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[error] 32-32: Double quote array expansions to avoid re-splitting elements.
(SC2068)
64-70: 🛠️ Refactor suggestionFix POSIX compliance and secure here-doc
Use a quoted here-doc delimiter, POSIX test operator, and quote the target path:- cat >$script <<EOF + cat >"$script" <<'EOF' #!/bin/sh - if [ "$1" == "start" ]; then + if [ "$1" = "start" ]; then $(cat kernel/modlist) fi EOF
🧹 Nitpick comments (1)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (1)
17-19: Handle directory changes reliably and remove unused variable
- Ensure
cd "$(dirname "$(readlink -f "$0")")"succeeds by appending|| exit 1.- The
BASEvariable is set but never used; consider removing it or using it for absolute paths.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 17-17: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/renesas_s4_defconfig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/renesas_s4_defconfig
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh
[warning] 17-17: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 23-23: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[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] 29-29: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[error] 32-32: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 38-38: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 48-48: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 53-53: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/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 (ubuntu-24.04, 3.12)
- 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)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: e2e
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh (2)
1-2: 🛠️ Refactor suggestionEnable strict mode for safer execution
The script should fail fast on errors, undefined variables, and pipeline failures. Add strict mode and a safe IFS immediately after the shebang.#!/bin/bash +set -euo pipefail +IFS=$'\n\t'
5-11: 🛠️ Refactor suggestionImprove robustness: add error checking and directory context handling
Critical steps like cloning, kernel setup, and the Buildroot build must abort on failure. Usingpushd/popdwith explicit|| exit 1ensures we catch errors immediately.-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 -./add_kernel.sh -cp -R overlay /buildroot -cp renesas_s4_defconfig /buildroot/configs/ -( cd /buildroot; make renesas_s4_defconfig && make ) +pushd /buildroot || exit 1 + ./add_kernel.sh || exit 1 + cp -R ../overlay . || exit 1 + cp ../renesas_s4_defconfig configs/ || exit 1 + make renesas_s4_defconfig && make || exit 1 +popd -mkimage -f flasher.its flasher.itb +mkimage -f flasher.its flasher.itb || exit 1🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (3)
32-41: Quote${pkgs[@]}in fetch and extract loops
Unquoted arrays can be split or globbed unexpectedly. Wrap expansions in quotes in both loops.-for pkg in ${pkgs[@]}; do +for pkg in "${pkgs[@]}"; do ... -done +done # later in the extract loop -for pkg in ${pkgs[@]}; do +for pkg in "${pkgs[@]}"; do ... -done +done🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 32-32: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 38-38: Double quote array expansions to avoid re-splitting elements.
(SC2068)
48-50: Quote${KMOD[@]}to avoid word splitting
Apply the same quoting discipline to the module array.-for mod in ${KMOD[@]}; do +for mod in "${KMOD[@]}"; do🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 48-48: Double quote array expansions to avoid re-splitting elements.
(SC2068)
1-2: 🛠️ Refactor suggestionEnable strict mode for early error detection
Addset -euo pipefailand a safe IFS to catch errors and undefined variables right away.#!/bin/bash +set -euo pipefail +IFS=$'\n\t'
🧹 Nitpick comments (3)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (3)
18-19: Remove unusedBASEvariable
TheBASE=$PWDassignment isn’t referenced later. Consider removing to keep the script lean.-BASE=$PWD🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
53-54: Check success of returning to previous directory
UsingOLDPWDwithout validation may lead to surprises. Quote the variable and bail out on failure.-cd $OLDPWD +cd "$OLDPWD" || exit 1🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 53-53: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
63-70: Secure here-doc and ensure POSIX compliance
Quote the redirection target, use a quoted delimiter to prevent unwanted expansions, and switch==to=.-script=$ODIR/etc/init.d/S01modules -cat >$script <<EOF +script="$ODIR/etc/init.d/S01modules" +cat >"$script" <<'EOF' #!/bin/sh -if [ "\$1" == "start" ]; then +if [ "\$1" = "start" ]; then $(cat kernel/modlist) fi EOF
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/overlay/etc/network/if-pre-up.d/wait-renesas.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/renesas_s4_defconfig(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/overlay/etc/network/if-pre-up.d/wait-renesas.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/renesas_s4_defconfig
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh
[warning] 17-17: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 23-23: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[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] 29-29: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[error] 32-32: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 38-38: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 48-48: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 53-53: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/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.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
michalskrivanek
force-pushed
the
rcars4
branch
2 times, most recently
from
f491462 to
ea76022
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (5)
17-19: Ensure directory change succeeds and clean up unused variableThe
cdcommand should bail out on failure, and theBASEvariable is assigned but not used anywhere.-cd "$(dirname "$(readlink -f "$0")")" +cd "$(dirname "$(readlink -f "$0")")" || exit 1 -BASE=$PWD +# BASE is unused; consider removing this line or leveraging BASE as intended🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 17-17: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
28-35: Quote array expansions and checkcdin download loopTo avoid word splitting/globbing and ensure the
cdsucceeds:-mkdir -p kernel -cd kernel +mkdir -p kernel +cd kernel || exit 1 -for pkg in ${pkgs[@]}; do +for pkg in "${pkgs[@]}"; do🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 29-29: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[error] 32-32: Double quote array expansions to avoid re-splitting elements.
(SC2068)
36-51: Quote array expansions in extraction loopSimilarly, wrap
${pkgs[@]}in quotes when iterating to prevent unintended splitting:-if ! [[ -d lib ]]; then - for pkg in ${pkgs[@]}; do +if ! [[ -d lib ]]; then + for pkg in "${pkgs[@]}"; do🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 38-38: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 48-48: Double quote array expansions to avoid re-splitting elements.
(SC2068)
1-2: 🛠️ Refactor suggestionEnable strict mode for robustness
Add
set -euo pipefailand a safeIFSimmediately after the shebang to catch errors early and prevent undefined variables or failed commands from going unnoticed.#!/bin/bash +set -euo pipefail +IFS=$'\n\t'
62-71: 🛠️ Refactor suggestionSecure here-doc and quote the script path
Quote the target path and use a quoted here-doc delimiter to prevent unintended expansions in the generated init script:
-cat >$script <<EOF +cat >"$script" <<'EOF' #!/bin/sh if [ "$1" = "start" ]; then $(cat kernel/modlist) fi -EOF +EOFpackages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh (2)
1-2: 🛠️ Refactor suggestionEnable strict mode for safe execution
Add strict error handling immediately after the shebang to catch failures and undefined variables:
#!/bin/bash +set -euo pipefail +IFS=$'\n\t'
7-11: 🛠️ Refactor suggestionAdd error handling and improve directory management
Wrap each critical step with failure checks and use
pushd/popdfor clarity:-./add_kernel.sh -cp -R overlay /buildroot -cp renesas_s4_defconfig /buildroot/configs/ -( cd /buildroot; make renesas_s4_defconfig && make ) -mkimage -f flasher.its flasher.itb +./add_kernel.sh || exit 1 +cp -R overlay /buildroot || exit 1 +cp renesas_s4_defconfig /buildroot/configs/ || exit 1 +pushd /buildroot || exit 1 + make renesas_s4_defconfig + make +popd +mkimage -f flasher.its flasher.itb || exit 1🧰 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/rcar_s4/add_kernel.sh (1)
53-61: Handle failure on returning to previous directoryThe
cd $OLDPWDstep should include error handling to avoid proceeding in the wrong directory if it fails:-cd $OLDPWD +cd "$OLDPWD" || exit 1🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 53-53: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitattributes(1 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py(2 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(3 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py(2 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/manifest.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/overlay/etc/network/if-pre-up.d/wait-renesas.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/renesas_s4_defconfig(1 hunks)packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/client.py(1 hunks)packages/jumpstarter-driver-raspberrypi/pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-raspberrypi/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/overlay/etc/network/if-pre-up.d/wait-renesas.sh
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/manifest.yaml
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py
- packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/client.py
- .gitattributes
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/renesas_s4_defconfig
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh
[warning] 17-17: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 23-23: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[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] 29-29: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[error] 32-32: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 38-38: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 48-48: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 53-53: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/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 (3)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
🔇 Additional comments (5)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py (3)
36-36: LGTM! Well-designed flexibility enhancement.Making the
dtbfield optional with a default ofNoneis a good design decision that maintains backward compatibility while adding flexibility for platforms that don't require Device Tree Blob files, such as the new Renesas R-Car S4 platform.
50-53: LGTM! Proper None handling implemented.The early return when
dtbisNoneis correctly implemented, and the return type annotation has been properly updated tostr | Noneto reflect the new behavior.
55-58: LGTM! Consistent None handling pattern.The
get_dtb_filemethod follows the same pattern asget_dtb_addresswith proper early return forNonedtb and correct return type annotation. The implementation maintains the existing logic for variant handling when dtb is present.packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (2)
3-14: Verify module list accuracyPlease confirm that each entry in the
KMODarray (e.g.,fixed,renesas_sdhi_internal_dmac,mmc_block, etc.) exactly matches the module names provided by the kernel packages. A typo here will lead to missing modules or failures when generating the dependency list.
20-27: Dynamic URL construction & package list declarationThe URL assembly via
sedand thepkgsarray declaration are clear and correctly formatted. No issues detected here.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 23-23: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[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)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh (4)
1-2: Enable strict mode for safer execution
Addset -euo pipefailand define a safe IFS immediately after the shebang to catch errors early and avoid unintended word splitting.#!/bin/bash +set -euo pipefail +IFS=$'\n\t'
5-5: Add error handling and use pushd/popd around the Buildroot clone
Ensure thegit clonecommand is checked and the working directory is restored on errors. Usepushd/popdfor clarity.-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 +pushd /buildroot || exit 1 ... +popd
7-11: Ensure each build step fails fast on errors
Wrap critical commands with|| exit 1to prevent silent failures and consider usingpushd/popdaround the Buildroot build.-./add_kernel.sh +./add_kernel.sh || exit 1 -cp -R overlay /buildroot +cp -R overlay /buildroot || exit 1 -cp renesas_s4_defconfig /buildroot/configs/ +cp renesas_s4_defconfig /buildroot/configs/ || exit 1 -( cd /buildroot; make renesas_s4_defconfig && make ) +( pushd /buildroot || exit 1 + make renesas_s4_defconfig && make +popd )🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
11-11: Add error checking for FIT image creation
Ensuremkimagefailures are caught to avoid propagating invalid artifacts.-mkimage -f flasher.its flasher.itb +mkimage -f flasher.its flasher.itb || exit 1packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (4)
1-2: Enable strict mode for robustness
Addset -euo pipefailand define a safe IFS after the shebang to catch errors and undefined variables early.#!/bin/bash +set -euo pipefail +IFS=$'\n\t'
17-18: Check directory change success
Verify thecdinto the script’s directory succeeds to avoid running subsequent commands in the wrong context.-cd "$(dirname "$(readlink -f "$0")")" +cd "$(dirname "$(readlink -f "$0")")" || exit 1🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 17-17: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
32-34: 🛠️ Refactor suggestionQuote array expansions in the download loop
Double-quote${pkgs[@]}and variable references to prevent word splitting or globbing issues.-for pkg in ${pkgs[@]}; do - [[ -f $pkg ]] || wget "$url/$pkg" || exit 1 +for pkg in "${pkgs[@]}"; do + [[ -f "$pkg" ]] || wget "$url/$pkg" || exit 1🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 32-32: Double quote array expansions to avoid re-splitting elements.
(SC2068)
48-50: 🛠️ Refactor suggestionQuote module expansions in the dependency loop
Prevent unwanted splitting by quoting${KMOD[@]}and paths.-for mod in ${KMOD[@]}; do +for mod in "${KMOD[@]}"; do modprobe -d "$PWD" -S "$KVER" --show-depends "$mod" done | sed ... > modlist🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 48-48: Double quote array expansions to avoid re-splitting elements.
(SC2068)
🧹 Nitpick comments (4)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (4)
18-19: Remove or use the unusedBASEvariable
TheBASEvariable is assigned but never used. Either remove this line or leverageBASEfor readability.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
28-29: Ensure kernel directory change is safe
Add error handling oncd kernelto catch failures.-mkdir -p kernel -cd kernel +mkdir -p kernel +cd kernel || exit 1🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 29-29: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
53-53: Handle potential failure when returning to the previous directory
Wrapcd $OLDPWDwith|| exit 1and quote$OLDPWDto ensure safe directory restoration.-cd $OLDPWD +cd "$OLDPWD" || exit 1🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 53-53: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
64-70: Quote the redirection target for the startup script
Quote$scriptincat >$scriptto handle paths with spaces.-cat >$script <<EOF +cat >"$script" <<EOF
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/overlay/etc/network/if-pre-up.d/wait-renesas.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/renesas_s4_defconfig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/overlay/etc/network/if-pre-up.d/wait-renesas.sh
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/renesas_s4_defconfig
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh
[warning] 17-17: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 23-23: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[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] 29-29: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[error] 32-32: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 38-38: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 48-48: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 53-53: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/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 (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
| for pkg in ${pkgs[@]}; do | ||
| echo -n "extracting $pkg ... " | ||
| rpm2cpio $pkg | cpio -id | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Quote array expansions in the extraction loop
Similarly, quote the package names when extracting RPMs.
-for pkg in ${pkgs[@]}; do
+for pkg in "${pkgs[@]}"; do
echo -n "extracting $pkg ... "
rpm2cpio "$pkg" | cpio -id
done📝 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.
| for pkg in ${pkgs[@]}; do | |
| echo -n "extracting $pkg ... " | |
| rpm2cpio $pkg | cpio -id | |
| done | |
| for pkg in "${pkgs[@]}"; do | |
| echo -n "extracting $pkg ... " | |
| rpm2cpio "$pkg" | cpio -id | |
| done |
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 38-38: Double quote array expansions to avoid re-splitting elements.
(SC2068)
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh at
lines 38 to 41, the array expansion ${pkgs[@]} and the variable $pkg should be
quoted to prevent word splitting and globbing issues. Update the for loop to
quote "${pkgs[@]}" and also quote "$pkg" in the rpm2cpio command to ensure
package names with spaces or special characters are handled correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh (2)
1-2: Enable strict mode for safer execution
Addset -euo pipefailandIFS=$'\n\t'right after the shebang to catch errors and undefined variables early.
10-10: Use pushd/popd for directory changes and ensure failure handling
Replace the subshell invocation with explicit directory handling:- ( cd /buildroot; make renesas_s4_defconfig && make ) + pushd /buildroot || exit 1 + make renesas_s4_defconfig + make + popd || exit 1This makes the workflow clearer and guarantees the script stops on any error.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (3)
1-2: Enable strict mode for robustness
Addset -euo pipefailandIFS=$'\n\t'immediately after the shebang to catch errors and undefined variables early.
17-19: Ensure directory change succeeds and streamline restoration
Add an exit check after thecdand either useBASEfor returning or switch topushd/popdinstead of relying on$OLDPWD:- cd "$(dirname "$(readlink -f "$0")")" + cd "$(dirname "$(readlink -f "$0")")" || exit 1Remove the unused
BASEvariable or leverage it consistently.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 17-17: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
64-70: Secure the here-doc and enforce POSIX compliance
Quote the here-doc delimiter, the target file, and use=for string tests:-cat >$script <<EOF +cat >"$script" <<'EOF' #!/bin/sh -if [ "$1" = "start" ]; then +if [ "$1" = "start" ]; then $(cat kernel/modlist) fi -EOF +EOF
🧹 Nitpick comments (9)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/overlay/etc/network/if-pre-up.d/wait-carrier.sh (2)
1-2: Enable strict error handling for robustness
Addset -euo pipefailandIFS=$'\n\t'immediately after the shebang to ensure the script exits on any error and catches undefined variables.
5-9: Quote variable expansions to prevent word splitting
Wrap${IFACE}in quotes for both theip link setcommand and the carrier check:- ip link set ${IFACE} up + ip link set "$IFACE" up - if [ "$(cat /sys/class/net/${IFACE}/carrier)" = "1" ]; then + if [ "$(cat "/sys/class/net/${IFACE}/carrier")" = "1" ]; thenThis ensures paths and interface names with special characters are handled safely.
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (7)
22-26: Quote array elements to prevent word splitting
Wrap each RPM name in quotes to avoid unintended splitting:-pkgs=( - kernel-automotive-core-$KVER.rpm +pkgs=( + "kernel-automotive-core-${KVER}.rpm" ...🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 23-23: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[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)
32-34: Quote array expansion and file names in fetch loop
Ensure the loop and file test handle names safely:-for pkg in ${pkgs[@]}; do +for pkg in "${pkgs[@]}"; do - [[ -f $pkg ]] || wget "$url/$pkg" || exit 1 + [[ -f "$pkg" ]] || wget "$url/$pkg" || exit 1🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 32-32: Double quote array expansions to avoid re-splitting elements.
(SC2068)
38-41: Double-quote package names in extract loop
Prevent word splitting when extracting:-for pkg in ${pkgs[@]}; do +for pkg in "${pkgs[@]}"; do echo -n "extracting $pkg ... " - rpm2cpio $pkg | cpio -id + rpm2cpio "$pkg" | cpio -id🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 38-38: Double quote array expansions to avoid re-splitting elements.
(SC2068)
48-50: Quote module array expansion
Avoid splitting or globbing issues:-for mod in ${KMOD[@]}; do +for mod in "${KMOD[@]}"; do modprobe -d "$PWD" -S "$KVER" --show-depends "$mod"🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 48-48: Double quote array expansions to avoid re-splitting elements.
(SC2068)
53-53: Quote and check directory restoration
Ensure you return to the correct directory or fail:-cd $OLDPWD +cd "$OLDPWD" || exit 1🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 53-53: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
55-55: Quote directory paths in mkdir
Handle directories with spaces:-mkdir -p $ODIR/lib/modules $ODIR/etc/init.d || exit 1 +mkdir -p "$ODIR/lib/modules" "$ODIR/etc/init.d" || exit 1
57-60: Use a safe read loop to handle module names correctly
Read lines without word splitting and preserve whitespace:-sed -nr 's|^insmod ||p' < kernel/modlist | while read mod; do +sed -nr 's|^insmod ||p' < kernel/modlist | while IFS= read -r mod; do mkdir -p "$ODIR$(dirname "$mod")" - zstd -d "kernel$mod.zst" -o "$ODIR$mod" + zstd -d "kernel${mod}.zst" -o "$ODIR$mod" done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/overlay/etc/network/if-pre-up.d/wait-carrier.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/renesas_s4_defconfig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/renesas_s4_defconfig
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh
[warning] 17-17: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 23-23: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[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] 29-29: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[error] 32-32: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 38-38: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 48-48: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 53-53: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/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.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- 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.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
|
Why is there a conflict? hhmm |
michalskrivanek
force-pushed
the
rcars4
branch
2 times, most recently
from
f22a982 to
b756da5
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
♻️ Duplicate comments (8)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh (2)
1-2: Enable strict mode for safer executionThe script lacks strict mode settings which help catch errors early. This is critical for build scripts that can fail silently.
10-10: Add error checking for directory operationsThe subshell execution doesn't handle potential failures properly, and the
cdoperation lacks error checking as flagged by static analysis.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (6)
1-2: Enable strict mode for robustness
17-18: Ensure directory change succeeds🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 17-17: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
28-34: Quote expansions and check directory changes🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 29-29: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[error] 32-32: Double quote array expansions to avoid re-splitting elements.
(SC2068)
38-41: Quote array expansions in the extraction loop🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 38-38: Double quote array expansions to avoid re-splitting elements.
(SC2068)
48-50: Quote module array expansions🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 48-48: Double quote array expansions to avoid re-splitting elements.
(SC2068)
64-70: Fix POSIX compliance in startup script and secure here-doc
🧹 Nitpick comments (1)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (1)
18-18: Remove unused BASE variableThe BASE variable is assigned but never used in the script, creating unnecessary clutter.
-BASE=$PWD🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
🛑 Comments failed to post (2)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh (1)
11-11:
⚠️ Potential issueFix output path for mkimage command
The mkimage command references
data/flasher.itbbut thedata/directory doesn't appear to be created or referenced elsewhere in the script.-mkimage -f flasher.its data/flasher.itb +mkimage -f flasher.its flasher.itb📝 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.mkimage -f flasher.its flasher.itb🤖 Prompt for AI Agents
In packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh at line 11, the mkimage command outputs to data/flasher.itb, but the data/ directory is not created or ensured to exist. Fix this by adding a command before line 11 to create the data/ directory if it doesn't exist, such as using mkdir -p data, so the output path is valid and the command succeeds.packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its (1)
22-22:
⚠️ Potential issueFix syntax error in DTB description
Missing closing quote in the description string will cause compilation failure.
- description = "DTB 8779f0-spider; + description = "DTB r8a779f0-spider";📝 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.description = "DTB r8a779f0-spider";🤖 Prompt for AI Agents
In packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its at line 22, the description string for DTB is missing a closing quote, causing a syntax error. Add the missing closing quotation mark at the end of the description string to fix the compilation failure.
|
rebased properly, added config for R-Car S4 Starter Kit board. kernel-automotive already has the dtb, and it should be the same otherwise. |
....almost:) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh (2)
1-2: Enable strict mode for safer executionThis issue was already identified in previous reviews. The script needs
set -euo pipefailand safe IFS handling for robust error handling.
5-11: Add error checking for critical operationsAs noted in previous reviews, critical commands like
git clone,./add_kernel.sh, andmkimageneed error checking to prevent silent failures. The subshell withcdalso needs error handling.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (6)
1-2: Enable strict mode for robustnessAs identified in previous reviews, the script needs
set -euo pipefailand safe IFS handling for proper error handling and script robustness.
17-18: Ensure directory change succeedsPrevious reviews identified that directory changes need error checking to prevent running commands in the wrong context.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 17-17: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
28-34: Quote expansions and check directory changesAs noted in previous reviews, the
cd kernelcommand needs error checking and the array expansion in the for loop should be quoted to prevent word splitting issues.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 29-29: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[error] 32-32: Double quote array expansions to avoid re-splitting elements.
(SC2068)
38-41: Quote array expansions in the extraction loopPrevious reviews identified that array expansions need to be quoted to prevent word splitting and globbing issues.
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 38-38: Double quote array expansions to avoid re-splitting elements.
(SC2068)
48-50: Quote module array expansionsAs flagged in previous reviews, the array expansion
${KMOD[@]}should be quoted to avoid re-splitting elements.🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 48-48: Double quote array expansions to avoid re-splitting elements.
(SC2068)
64-70: Fix POSIX compliance in startup script and secure here-docPrevious reviews identified the need to use
=instead of==for POSIX compliance and quote the here-doc delimiter to prevent unintended expansions.
🧹 Nitpick comments (6)
packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/client.py (3)
10-10: Add class docstring for better documentation.The class is missing a docstring which would help document its purpose and usage.
@dataclass(kw_only=True) class DigitalOutputClient(PowerClient): + """Digital output client for Raspberry Pi GPIO power control. + + Provides power control functionality through GPIO pins with CLI interface. + """🧰 Tools
🪛 Pylint (3.3.7)
[convention] 10-10: Missing class docstring
(C0115)
11-17: Consider removing redundant method overrides.The
on()andoff()methods now override the parent class methods but implement identical functionality. The parentPowerClientalready provides these exact implementations. Consider removing these overrides and adding the docstrings to the parent class instead, or only override if there's GPIO-specific behavior needed.- def on(self): - """Turn power on""" - self.call("on") - - def off(self): - """Turn power off""" - self.call("off")The parent class already provides these methods with the same implementation.
19-28: Add method docstring and consider removing unnecessary pass.The CLI method implementation looks correct but could benefit from documentation and cleanup.
def cli(self): + """Create GPIO power control CLI command group.""" @click.group() def gpio(): """GPIO power control commands""" - pass for cmd in super().cli().commands.values(): gpio.add_command(cmd) return gpioThe
passstatement is unnecessary since the function body contains other statements.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 19-19: Missing function or method docstring
(C0116)
[warning] 23-23: Unnecessary pass statement
(W0107)
packages/jumpstarter/jumpstarter/config/exporter.py (1)
15-15: Fix import order issue.The static analysis tool correctly identified an import order issue. The third-party import should be placed before local imports according to Python conventions.
Apply this diff to fix the import order:
+from jumpstarter.common.exceptions import ConfigurationError + from .common import ObjectMeta from .grpc import call_credentials from .tls import TLSConfigV1Alpha1 -from jumpstarter.common.exceptions import ConfigurationError🧰 Tools
🪛 Pylint (3.3.7)
[error] 15-15: Unable to import 'jumpstarter.common.exceptions'
(E0401)
[convention] 15-15: third party import "jumpstarter.common.exceptions.ConfigurationError" should be placed before local imports "common.ObjectMeta", "grpc.call_credentials", "tls.TLSConfigV1Alpha1"
(C0411)
packages/jumpstarter/jumpstarter/config/client.py (1)
20-20: Fix import order issue.Similar to the exporter config, the import order should be corrected to follow Python conventions.
Apply this diff to fix the import order:
+from jumpstarter.common.exceptions import ConfigurationError, FileNotFoundError + from .common import CONFIG_PATH, ObjectMeta from .env import JMP_LEASE from .grpc import call_credentials from .tls import TLSConfigV1Alpha1 from jumpstarter.client.grpc import ClientService -from jumpstarter.common.exceptions import ConfigurationError, FileNotFoundError🧰 Tools
🪛 Pylint (3.3.7)
[warning] 20-20: Redefining built-in 'FileNotFoundError'
(W0622)
[error] 20-20: Unable to import 'jumpstarter.common.exceptions'
(E0401)
[convention] 20-20: third party import "jumpstarter.common.exceptions.ConfigurationError" should be placed before local imports "common.CONFIG_PATH", "env.JMP_LEASE", "grpc.call_credentials", "tls.TLSConfigV1Alpha1"
(C0411)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (1)
87-90: Good implementation of optional DTB handlingThe conditional DTB handling correctly checks for the presence of
manifest.spec.dtbbefore attempting to use it, preventing errors when DTB is not defined in the manifest. This aligns well with the changes inbundle.pythat made the DTB field optional.However, consider breaking the long line for better readability:
- dtb_path = await self._get_file_path(manifest.get_dtb_file(self._use_dtb)) if manifest.spec.dtb else None + dtb_path = ( + await self._get_file_path(manifest.get_dtb_file(self._use_dtb)) + if manifest.spec.dtb else None + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 87-87: Line too long (113/100)
(C0301)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.gitattributes(1 hunks).github/workflows/pr_analytics.yaml(1 hunks)docs/source/getting-started/usage/setup-local-mode.md(1 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py(2 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(3 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py(2 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/data/flasher.itb(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/manifest.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/overlay/etc/network/if-pre-up.d/wait-carrier.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/renesas_s4_defconfig(1 hunks)packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/client.py(1 hunks)packages/jumpstarter-driver-raspberrypi/pyproject.toml(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(2 hunks)packages/jumpstarter/jumpstarter/config/client_config_test.py(1 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- .github/workflows/pr_analytics.yaml
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/data/flasher.itb
- docs/source/getting-started/usage/setup-local-mode.md
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/overlay/etc/network/if-pre-up.d/wait-carrier.sh
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/renesas_s4_defconfig
- packages/jumpstarter-driver-raspberrypi/pyproject.toml
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
- .gitattributes
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/manifest.yaml
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/jumpstarter/jumpstarter/config/client.py (1)
packages/jumpstarter/jumpstarter/common/exceptions.py (2)
ConfigurationError(37-40)FileNotFoundError(55-58)
packages/jumpstarter/jumpstarter/config/exporter.py (1)
packages/jumpstarter/jumpstarter/common/exceptions.py (1)
ConfigurationError(37-40)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py (2)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (1)
get_dtb_address(183-186)packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/server.py (1)
address(64-68)
packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/client.py (3)
packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/driver.py (4)
client(14-15)client(49-50)on(36-40)off(29-33)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (9)
PowerClient(10-53)on(11-12)on(37-39)off(14-15)off(42-44)off(57-58)off(65-67)cli(30-53)cli(60-69)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-99)call(36-46)
🪛 Pylint (3.3.7)
packages/jumpstarter/jumpstarter/config/client.py
[warning] 20-20: Redefining built-in 'FileNotFoundError'
(W0622)
[error] 20-20: Unable to import 'jumpstarter.common.exceptions'
(E0401)
[convention] 20-20: third party import "jumpstarter.common.exceptions.ConfigurationError" should be placed before local imports "common.CONFIG_PATH", "env.JMP_LEASE", "grpc.call_credentials", "tls.TLSConfigV1Alpha1"
(C0411)
[convention] 79-79: Missing function or method docstring
(C0116)
packages/jumpstarter/jumpstarter/config/exporter.py
[error] 15-15: Unable to import 'jumpstarter.common.exceptions'
(E0401)
[convention] 15-15: third party import "jumpstarter.common.exceptions.ConfigurationError" should be placed before local imports "common.ObjectMeta", "grpc.call_credentials", "tls.TLSConfigV1Alpha1"
(C0411)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py
[convention] 50-50: Missing function or method docstring
(C0116)
[convention] 55-55: Missing function or method docstring
(C0116)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py
[convention] 87-87: Line too long (113/100)
(C0301)
[warning] 209-209: Method 'set_dtb' is abstract in class 'BaseFlasher' but is not overridden in child class 'RCarS4Flasher'
(W0223)
[warning] 209-209: Method 'set_initram' is abstract in class 'BaseFlasher' but is not overridden in child class 'RCarS4Flasher'
(W0223)
[warning] 209-209: Method 'set_kernel' is abstract in class 'BaseFlasher' but is not overridden in child class 'RCarS4Flasher'
(W0223)
packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/client.py
[error] 4-4: Unable to import 'jumpstarter_driver_power.client'
(E0401)
[error] 6-6: Unable to import 'jumpstarter.client'
(E0401)
[convention] 10-10: Missing class docstring
(C0115)
[convention] 19-19: Missing function or method docstring
(C0116)
[warning] 23-23: Unnecessary pass statement
(W0107)
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh
[warning] 17-17: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 23-23: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[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] 29-29: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[error] 32-32: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 38-38: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 48-48: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 53-53: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🔇 Additional comments (9)
packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/client.py (1)
3-4: LGTM! Appropriate imports for the new functionality.The new imports support the inheritance change to
PowerClientand the CLI command group functionality using Click.🧰 Tools
🪛 Pylint (3.3.7)
[error] 4-4: Unable to import 'jumpstarter_driver_power.client'
(E0401)
packages/jumpstarter/jumpstarter/config/client_config_test.py (1)
78-78: LGTM! Test updated correctly for new validation logic.The test change correctly reflects the updated configuration validation where
endpointandtokenare now optional fields, whileJMP_NAMESPACEandJMP_NAMEremain required. This ensures proper test coverage for the mandatory configuration fields.packages/jumpstarter/jumpstarter/config/exporter.py (2)
82-84: LGTM! Good design pattern for optional configuration fields.Making
endpointandtokenoptional with defaultNonevalues allows for flexible configuration scenarios (e.g., local mode vs remote mode) while maintaining type safety.
164-166: Excellent validation pattern implementation.The runtime validation ensures that required connection parameters are present when actually needed for channel creation. This provides clear error messages and prevents silent failures.
packages/jumpstarter/jumpstarter/config/client.py (2)
72-74: LGTM! Consistent with exporter configuration pattern.The optional field pattern matches the implementation in
exporter.py, providing consistency across client and exporter configurations.
80-82: Excellent runtime validation consistency.The validation logic is identical to the exporter implementation, ensuring consistent error handling across the codebase when required connection parameters are missing.
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (1)
208-212: New RCarS4Flasher class looks goodThe implementation is consistent with the existing
TIJ784S4Flasherpattern and correctly specifies the flasher bundle for the R-Car S4 platform.Note: The Pylint warnings about abstract methods are false positives - the base class methods raise
NotImplementedErrorbut are not truly abstract, so overriding them is optional.🧰 Tools
🪛 Pylint (3.3.7)
[warning] 209-209: Method 'set_dtb' is abstract in class 'BaseFlasher' but is not overridden in child class 'RCarS4Flasher'
(W0223)
[warning] 209-209: Method 'set_initram' is abstract in class 'BaseFlasher' but is not overridden in child class 'RCarS4Flasher'
(W0223)
[warning] 209-209: Method 'set_kernel' is abstract in class 'BaseFlasher' but is not overridden in child class 'RCarS4Flasher'
(W0223)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py (2)
36-36: Well-implemented optional DTB supportMaking the
dtbfield optional withNoneas the default is a good design choice that provides flexibility for flasher bundles that don't require device tree blobs.
50-62: Proper null handling in DTB methodsThe updates to
get_dtb_addressandget_dtb_filecorrectly handle the case wheredtbisNoneby returningNoneimmediately. The return type annotations (str | None) accurately reflect the new behavior.This change integrates well with the conditional DTB handling added in
driver.pyat lines 87-90.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 50-50: Missing function or method docstring
(C0116)
[convention] 55-55: Missing function or method docstring
(C0116)
builds flasher FIT image. Run in a container like e.g. podman run --rm -it -v $PWD:/host -w /host fedora:41 ./build_flasher.sh
|
sigh, if only i remembered how to work with gihub...:) @mangelajo can you check now? |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh (1)
1-2: Enable strict error handling
The script should fail fast on errors, undefined variables, and pipeline failures. Add strict mode and a safe IFS after the shebang to catch issues early.#!/bin/bash +set -euo pipefail +IFS=$'\n\t'packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (4)
1-2: Enable strict mode for robustness
As with the flasher script, addset -euo pipefailand define a safe IFS to prevent silent failures and mishandled whitespace.#!/bin/bash +set -euo pipefail +IFS=$'\n\t'
38-41: 🛠️ Refactor suggestionQuote package names when extracting RPMs
Both the loop and the$pkgvariable should be quoted to avoid globbing issues.-for pkg in ${pkgs[@]}; do +for pkg in "${pkgs[@]}"; do echo -n "extracting $pkg ... " - rpm2cpio $pkg | cpio -id + rpm2cpio "$pkg" | cpio -id done🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 38-38: Double quote array expansions to avoid re-splitting elements.
(SC2068)
32-34: 🛠️ Refactor suggestionQuote array expansion to prevent word splitting
Unquoted${pkgs[@]}can break on package names containing spaces.-for pkg in ${pkgs[@]}; do +for pkg in "${pkgs[@]}"; do [[ -f $pkg ]] || wget "$url/$pkg" || exit 1 done🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 32-32: Double quote array expansions to avoid re-splitting elements.
(SC2068)
28-29: 🛠️ Refactor suggestionVerify entering the kernel directory
Ensurecd kernelsucceeds before fetching or extracting packages.mkdir -p kernel -cd kernel +cd kernel || exit 1🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 29-29: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🧹 Nitpick comments (5)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh (2)
5-7: Add error checking around clone and kernel preparation steps
Critical operations likegit cloneand./add_kernel.shshould immediately abort on failure to avoid cascading errors.git clone --depth 1 --branch 2025.05-rc2 https://github.com/buildroot/buildroot /buildroot || exit 1 -./add_kernel.sh +./add_kernel.sh || exit 1
10-11: Refactor build and image creation for clarity and robustness
Rather than a subshell, usepushd/popdand ensure each command’s exit code is checked.-( cd /buildroot; make renesas_s4_defconfig && make ) +pushd /buildroot || exit 1 + make renesas_s4_defconfig + make +popd -mkimage -f flasher.its data/flasher.itb +mkimage -f flasher.its data/flasher.itb || exit 1🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh (3)
53-53: Ensure returning to previous directory succeeds
Check thecdback to$OLDPWDto avoid silent failures.-cd $OLDPWD +cd "$OLDPWD" || exit 1🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 53-53: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
57-60: Harden overlay module installation loop
Quote the module paths and useread -rto prevent backslash escapes.-sed -nr 's|^insmod ||p' < kernel/modlist | while read mod; do - mkdir -p "$ODIR$(dirname $mod)" - zstd -d "kernel$mod.zst" -o "$ODIR$mod" +sed -nr 's|^insmod ||p' < kernel/modlist | while read -r mod; do + mkdir -p "$ODIR$(dirname "$mod")" + zstd -d "kernel${mod}.zst" -o "$ODIR$mod" done
63-71: Quote script path before redirection
Wrap$scriptin quotes to handle any unexpected whitespace in the path.-script=$ODIR/etc/init.d/S01modules -cat >$script <<EOF +script="$ODIR/etc/init.d/S01modules" +cat >"$script" <<EOF
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/manifest.yaml(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/overlay/etc/network/if-pre-up.d/wait-carrier.sh(1 hunks)packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/renesas_s4_defconfig(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/manifest.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/overlay/etc/network/if-pre-up.d/wait-carrier.sh
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/flasher.its
- packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/renesas_s4_defconfig
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/add_kernel.sh
[warning] 17-17: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 18-18: BASE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 23-23: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[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] 29-29: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[error] 32-32: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 38-38: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 48-48: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 53-53: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/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 (10)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- 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: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: e2e
🔇 Additional comments (2)
packages/jumpstarter-driver-flashers/oci_bundles/rcar_s4/build_flasher.sh (2)
3-3: Installation command looks good
Thednf installline correctly disables weak dependencies and lists needed packages.
8-9: Overlay and config copy steps
Thesecpcommands are straightforward. With strict mode enabled, any failure will be caught automatically.
I retried a flaky issue that we have on MacOS unit testing, there are some uboot/qemu tests that we may want to disable on macos systems. |
2 participants
on top of https://github.com/jumpstarter-dev/jumpstarter/pull/506
Summary by CodeRabbit