arduino-uno-q: fix CI build by fetching qcombin at image creation time#9642
Open
SuperKali wants to merge 2 commits intoarmbian:mainfrom
Open
arduino-uno-q: fix CI build by fetching qcombin at image creation time#9642SuperKali wants to merge 2 commits intoarmbian:mainfrom
SuperKali wants to merge 2 commits intoarmbian:mainfrom
Conversation
The qcombin extension used fetch_sources_tools hook which only runs during u-boot compilation. When artifacts are cached (CI), the fetch is skipped and image-output-arduino fails because flash binaries are missing. Move qcombin fetch into image-output-arduino.sh using post_family_config hook which always runs. Remove the standalone qcombin.sh extension. Also improve the image-output extension: - Use sfdisk JSON output for reliable partition parsing - Use run_host_command_logged for proper build logging - Add cleanup handler for loop device on failure - Quote all variables properly
Contributor
📝 WalkthroughWalkthroughThe qcombin fetch was moved from a standalone extension into an Arduino image-output family hook; the qcombin extension file was removed and its enable call dropped from the qrb2210 family. The Arduino image conversion flow was refactored to use sfdisk+Python, loop mounts, explicit cleanup, and different archive layout. Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as Builder
participant ImgExt as image-output-arduino.sh
participant Git as Git(repo: armbian/qcombin)
participant SFDISK as sfdisk+python
participant Kernel as LoopDevice/Mount
participant Archiver as tar/archive
Builder->>ImgExt: trigger post-family config
ImgExt->>Git: fetch armbian/qcombin (post_family_config__fetch_qcombin)
Builder->>ImgExt: trigger post_build_image__900_convert...
ImgExt->>SFDISK: run sfdisk -J to enumerate partitions
SFDISK->>ImgExt: partition JSON
ImgExt->>Kernel: setup loop device (ARDUINO_ROOTFS_LOOP) and mount (ARDUINO_ROOTFS_MOUNT)
ImgExt->>Archiver: create flash layout, rename partitions to disk-sdcard.img.*
Archiver->>ImgExt: produce tar archive under DESTIMG
ImgExt->>ImgExt: add_cleanup_handler(image_output_arduino_cleanup)
alt failure or exit
ImgExt->>Kernel: detach loop / unmount via cleanup
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
github-actions
bot
added
size/medium
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
extensions/image-output-arduino.sh (1)
24-29: Consider cachingsfdiskoutput to avoid redundant invocations.The same
sfdisk -Jcommand is executed four times. Parsing once would be more efficient.♻️ Suggested optimization
# Extract partition offsets using sfdisk JSON output local p1_start p1_size p2_start p2_size - p1_start=$(sfdisk -J "${img}" | python3 -c "import sys,json; p=json.load(sys.stdin)['partitiontable']['partitions']; print(p[0]['start'])") - p1_size=$(sfdisk -J "${img}" | python3 -c "import sys,json; p=json.load(sys.stdin)['partitiontable']['partitions']; print(p[0]['size'])") - p2_start=$(sfdisk -J "${img}" | python3 -c "import sys,json; p=json.load(sys.stdin)['partitiontable']['partitions']; print(p[1]['start'])") - p2_size=$(sfdisk -J "${img}" | python3 -c "import sys,json; p=json.load(sys.stdin)['partitiontable']['partitions']; print(p[1]['size'])") + local sfdisk_json + sfdisk_json=$(sfdisk -J "${img}") + read -r p1_start p1_size p2_start p2_size < <( + echo "${sfdisk_json}" | python3 -c " +import sys, json +p = json.load(sys.stdin)['partitiontable']['partitions'] +print(p[0]['start'], p[0]['size'], p[1]['start'], p[1]['size']) +" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/image-output-arduino.sh` around lines 24 - 29, The four repeated invocations of sfdisk -J are inefficient; capture the JSON once and reuse it when setting p1_start, p1_size, p2_start, and p2_size. Run sfdisk -J "${img}" a single time into a variable or temp file (e.g., sfdisk_json) and then parse that single JSON with the existing python3 one-liners (or jq) to assign p1_start, p1_size, p2_start, p2_size, replacing the repeated sfdisk calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/image-output-arduino.sh`:
- Around line 4-7: The repo fetch function post_family_config__fetch_qcombin
clones qcombin but never sets QCOMBIN_DIR, causing later references to be unset;
after calling
fetch_from_repo("https://github.com/armbian/qcombin","qcombin","branch:main")
set and export QCOMBIN_DIR to the path used by fetch_from_repo (e.g.
${SRC}/cache/sources/qcombin) and optionally validate it exists (fail/alert if
missing) so subsequent uses of QCOMBIN_DIR resolve correctly; update
post_family_config__fetch_qcombin to perform this assignment and export
immediately after the fetch.
---
Nitpick comments:
In `@extensions/image-output-arduino.sh`:
- Around line 24-29: The four repeated invocations of sfdisk -J are inefficient;
capture the JSON once and reuse it when setting p1_start, p1_size, p2_start, and
p2_size. Run sfdisk -J "${img}" a single time into a variable or temp file
(e.g., sfdisk_json) and then parse that single JSON with the existing python3
one-liners (or jq) to assign p1_start, p1_size, p2_start, p2_size, replacing the
repeated sfdisk calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4118036c-28d7-471d-8272-afe2e0972ecf
📒 Files selected for processing (3)
config/sources/families/qrb2210.confextensions/image-output-arduino.shextensions/qcombin.sh
💤 Files with no reviewable changes (2)
- config/sources/families/qrb2210.conf
- extensions/qcombin.sh
Use adbd on Debian where available, fall back to USB gadget network with unudhcpd on Ubuntu where adbd is not packaged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/boards/arduino-uno-q.csc`:
- Around line 22-36: The else branch currently enables usbgadget-rndis.service
for all non-Debian DISTROs causing failure on non-Ubuntu systems because
post_family_tweaks_bsp__arduino-uno-q_usb_gadget() only installs the service on
Ubuntu; change the logic to explicitly detect Ubuntu (e.g., replace the generic
else with an elif checking [[ "${DISTRIBUTION}" == "Ubuntu" ]]) before running
the Armbian repo swap, installing unudhcpd and calling chroot_sdcard systemctl
enable usbgadget-rndis.service, and leave other distributions to the default
path so systemctl enable is only invoked when the service file is present (refer
to the usbgadget-rndis.service name and
post_family_tweaks_bsp__arduino-uno-q_usb_gadget()).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1e35236d-3b10-4a7b-99c6-763e0163e359
📒 Files selected for processing (1)
config/boards/arduino-uno-q.csc
Comment on lines
+22
to
+36
| # USB access: adbd on Debian, USB gadget network on Ubuntu (adbd not available) | ||
| if [[ "${DISTRIBUTION}" == "Debian" ]]; then | ||
| do_with_retries 3 chroot_sdcard_apt_get_install adbd | ||
| chroot_sdcard sed -i 's/"Debian"/"Armbian"/' /usr/lib/android-sdk/platform-tools/adbd-usb-gadget | ||
| chroot_sdcard sed -i 's/"ADB device"/"Arduino UNO Q"/' /usr/lib/android-sdk/platform-tools/adbd-usb-gadget | ||
| chroot_sdcard systemctl enable adbd.service | ||
| else | ||
| # unudhcpd is in the Armbian repo | ||
| mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled "${SDCARD}"/etc/apt/sources.list.d/armbian.sources | ||
| do_with_retries 3 chroot_sdcard_apt_get_update | ||
| do_with_retries 3 chroot_sdcard_apt_get_install unudhcpd | ||
| mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled | ||
| do_with_retries 3 chroot_sdcard_apt_get_update | ||
| chroot_sdcard systemctl enable usbgadget-rndis.service | ||
| fi |
Contributor
There was a problem hiding this comment.
Distribution mismatch: else branch handles all non-Debian, but BSP hook only installs for Ubuntu.
Line 35 enables usbgadget-rndis.service for all non-Debian distributions (the else branch), but the service file is only installed for Ubuntu in post_family_tweaks_bsp__arduino-uno-q_usb_gadget() (line 44: [[ "${DISTRIBUTION}" != "Ubuntu" ]] && return 0).
If a distribution other than Debian or Ubuntu is used, systemctl enable will fail because the service file won't exist.
Consider explicitly checking for Ubuntu:
Proposed fix
- else
+ elif [[ "${DISTRIBUTION}" == "Ubuntu" ]]; then
# unudhcpd is in the Armbian repo
mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled "${SDCARD}"/etc/apt/sources.list.d/armbian.sources
do_with_retries 3 chroot_sdcard_apt_get_update
do_with_retries 3 chroot_sdcard_apt_get_install unudhcpd
mv "${SDCARD}"/etc/apt/sources.list.d/armbian.sources "${SDCARD}"/etc/apt/sources.list.d/armbian.sources.disabled
do_with_retries 3 chroot_sdcard_apt_get_update
chroot_sdcard systemctl enable usbgadget-rndis.service
+ else
+ display_alert "USB gadget not configured" "unsupported distribution: ${DISTRIBUTION}" "wrn"
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/boards/arduino-uno-q.csc` around lines 22 - 36, The else branch
currently enables usbgadget-rndis.service for all non-Debian DISTROs causing
failure on non-Ubuntu systems because
post_family_tweaks_bsp__arduino-uno-q_usb_gadget() only installs the service on
Ubuntu; change the logic to explicitly detect Ubuntu (e.g., replace the generic
else with an elif checking [[ "${DISTRIBUTION}" == "Ubuntu" ]]) before running
the Armbian repo swap, installing unudhcpd and calling chroot_sdcard systemctl
enable usbgadget-rndis.service, and leave other distributions to the default
path so systemctl enable is only invoked when the service file is present (refer
to the usbgadget-rndis.service name and
post_family_tweaks_bsp__arduino-uno-q_usb_gadget()).
igorpecovnik
approved these changes
Apr 8, 2026
Contributor
|
✅ This PR has been reviewed and approved — all set for merge! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
image-output-arduino.sh:25where qcombin flash binaries were not fetchedqcombin.shextension usedfetch_sources_toolshook which only runs during u-boot compilation — when artifacts are cached (CI), the fetch is skippedimage-output-arduino.shusingpost_family_confighook which always runsqcombin.shextensionTest plan
Summary by CodeRabbit
New Features
Refactor