Conversation
2d6d05d to
2040d00
Compare
Then next up will be trying cmake
shallow clone wasn't picking it up
This reverts commit 3ec220c.
- Remove qt-arm64-linux job from docker-image.yml (moved to build-qt-arm64-linux.yml) - Remove qt-arm64-linux/Dockerfile (moved to separate PR) - qt-base now only depends on prebuilt qt-arm64-linux image - Makes this PR focused on dev/builder integration with qt-base The QT build workflow will be merged separately via pfeerick/add-qt-build-workflow
2040d00 to
d831f99
Compare
Either needed at compile time or runtime. Also re-add comments for the groups of packages.
The path is wrong anyway (should be /usr) and shouldn't be needed anyway since `libssl-dev` is installed.
SSH was added for devcontainer use, but the EdgeTX devcontainer uses https submodules and needs no SSH client. Per-user SSH customisation is better handled via devcontainer Features. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughCI workflows and Dockerfiles were refactored for multi-architecture builds (amd64/arm64): per-arch matrix jobs produce digests and upload artifacts; merge jobs assemble multi-arch manifests. Dev/builder/wasi Dockerfiles gained TARGETARCH-specific stages, SDL2/Qt handling changed, and README updated with local build examples. ChangesDocker Multi-Architecture Build System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/docker-image.yml (2)
308-308:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncorrect step name.
Step says "Build and Push edgetx-builder" but this is the
wasijob buildingedgetx-wasi.- - name: Build and Push edgetx-builder + - name: Build and Push edgetx-wasi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/docker-image.yml at line 308, The GitHub Actions step name is incorrect: the step currently named "Build and Push edgetx-builder" belongs to the wasi job that builds edgetx-wasi; update the step's name string to accurately reflect the action (e.g., "Build and Push edgetx-wasi") so the step label matches the job purpose and artifacts produced.
291-292:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent image name casing for wasi job.
The
devandbuilderjobs compute lowercase image names using${GITHUB_REPOSITORY_OWNER,,}, butwasiuses${{ github.repository_owner }}directly. If the repository owner contains uppercase characters, this could cause manifest tagging inconsistencies or push failures.Proposed fix
Add image name computation step similar to other jobs:
wasi: runs-on: ubuntu-latest steps: - name: Check out the repo uses: actions/checkout@v4 + - name: Set image name + id: image + run: | + echo "wasi_image=ghcr.io/${GITHUB_REPOSITORY_OWNER,,}/edgetx-wasi" >> "$GITHUB_OUTPUT" + - name: Set up Docker Buildx uses: docker/setup-buildx-action@v3 - name: Docker metadata id: meta uses: docker/metadata-action@v5 with: images: | - name=ghcr.io/${{ github.repository_owner }}/edgetx-wasi + name=${{ steps.image.outputs.wasi_image }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/docker-image.yml around lines 291 - 292, The wasi job uses `${{ github.repository_owner }}` for the images name which is inconsistent with other jobs that lowercase the owner; update the wasi job to compute and use the lowercased owner (same approach as dev/builder) so the images: entry becomes ghcr.io/${GITHUB_REPOSITORY_OWNER,,}/edgetx-wasi (or use the workflow step that sets a lowercased IMAGE_OWNER variable and reference that) to avoid casing mismatches when tagging/pushing manifests.dev/Dockerfile (1)
130-146:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
QT_VERSIONARG not available inbase-with-sdl2stage.Docker ARGs are scoped to individual build stages and must be redeclared after each
FROM. Thebase-with-sdl2stage starts at line 131 but doesn't redeclareQT_VERSION, so line 140's${QT_VERSION}resolves to empty string, producing path/opt/qt//gcc_64instead of/opt/qt/6.9.3/gcc_64.This breaks all Qt environment variables and will cause Qt to not be found at runtime.
Proposed fix
# Main base stage with SDL2 installed FROM base AS base-with-sdl2 +ARG QT_VERSION COPY --from=sdl2-builder /sdl2-install / RUN ldconfig🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/Dockerfile` around lines 130 - 146, The Dockerfile's base-with-sdl2 stage uses ${QT_VERSION} but never declares ARG QT_VERSION in that stage, so redeclare ARG QT_VERSION immediately after the FROM base AS base-with-sdl2 line and then keep the existing ENV definitions (QT_INSTALL_DIR, QT_BASE_DIR, PATH, QT_PLUGIN_PATH, QML_IMPORT_PATH, QML2_IMPORT_PATH, LD_LIBRARY_PATH, PKG_CONFIG_PATH) so ${QT_VERSION} expands correctly; specifically add ARG QT_VERSION (and ARG QT_INSTALL_DIR if you rely on it) in the base-with-sdl2 stage before the ENV QT_BASE_DIR assignment to ensure QT_BASE_DIR=/opt/qt/${QT_VERSION}/gcc_64 is constructed properly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@builder/Dockerfile`:
- Around line 1-6: The ARG GCC_ARM_VERSION is declared after the first FROM so
it's scoped only to the base stage, causing empty values in downstream stages
(base-amd64, base-arm64, final); move the ARG GCC_ARM_VERSION=14.2.rel1
declaration to the top of the Dockerfile before the first FROM so it becomes
global and is inherited by all subsequent stages, then remove any redundant
defaults or re-declarations in the individual stages (or keep ARG
GCC_ARM_VERSION without a default) so wget URLs receive the correct version
string.
In `@README.md`:
- Around line 17-20: The fenced code block containing the Docker/Podman build
examples (the lines with "docker build --build-arg TARGETARCH=$(uname -m) -f
dev/Dockerfile dev" and "podman build --build-arg TARGETARCH=$(uname -m) -f
dev/Dockerfile dev") should include a language identifier for syntax
highlighting; update the opening triple-backticks to "```sh" so the block is
Markdown-compliant and renders as shell script.
---
Outside diff comments:
In @.github/workflows/docker-image.yml:
- Line 308: The GitHub Actions step name is incorrect: the step currently named
"Build and Push edgetx-builder" belongs to the wasi job that builds edgetx-wasi;
update the step's name string to accurately reflect the action (e.g., "Build and
Push edgetx-wasi") so the step label matches the job purpose and artifacts
produced.
- Around line 291-292: The wasi job uses `${{ github.repository_owner }}` for
the images name which is inconsistent with other jobs that lowercase the owner;
update the wasi job to compute and use the lowercased owner (same approach as
dev/builder) so the images: entry becomes
ghcr.io/${GITHUB_REPOSITORY_OWNER,,}/edgetx-wasi (or use the workflow step that
sets a lowercased IMAGE_OWNER variable and reference that) to avoid casing
mismatches when tagging/pushing manifests.
In `@dev/Dockerfile`:
- Around line 130-146: The Dockerfile's base-with-sdl2 stage uses ${QT_VERSION}
but never declares ARG QT_VERSION in that stage, so redeclare ARG QT_VERSION
immediately after the FROM base AS base-with-sdl2 line and then keep the
existing ENV definitions (QT_INSTALL_DIR, QT_BASE_DIR, PATH, QT_PLUGIN_PATH,
QML_IMPORT_PATH, QML2_IMPORT_PATH, LD_LIBRARY_PATH, PKG_CONFIG_PATH) so
${QT_VERSION} expands correctly; specifically add ARG QT_VERSION (and ARG
QT_INSTALL_DIR if you rely on it) in the base-with-sdl2 stage before the ENV
QT_BASE_DIR assignment to ensure QT_BASE_DIR=/opt/qt/${QT_VERSION}/gcc_64 is
constructed properly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 718e9d78-7638-46ef-a9ba-68957326a680
📒 Files selected for processing (5)
.github/workflows/docker-image.ymlREADME.mdbuilder/Dockerfiledev/Dockerfilewasi/Dockerfile
BuildKit carries ARGs across stages leniently, but buildah/podman follow the
documented semantics (defaults inherited only from global scope; each consuming
stage must redeclare). To build identically on both engines:
- dev/builder: declare GCC_ARM_VERSION globally (before the first FROM) and
redeclare QT_VERSION in base-with-sdl2 so ${QT_VERSION} resolves in
QT_BASE_DIR there.
Also from review feedback:
- docker-image.yml: fix the wasi job step label (was "edgetx-builder").
- README: tag the docker/podman build snippet as sh.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…podman Empirically tested on both engines (BuildKit and buildah/podman 5.8): a child stage reads a global ARG's default AND a --build-arg override without needing to redeclare it. The earlier "portability" change was unnecessary, so revert it and keep dev/builder Dockerfiles minimal. The wasi step-label and README fixes stay. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A native `podman build` of dev/ revealed arm-none-eabi-gcc missing from PATH in
the final image: ${GCC_ARM_VERSION} expanded empty in the `final` stage, so
ENV PATH pointed at /opt/arm-gnu-toolchain--aarch64-... (note the empty version).
Root cause (reproduced minimally): a stage-scoped ARG default combined with a
variable parent stage `FROM base-${TARGETARCH}` is not carried into the final
stage by buildah/podman, even though each stage redeclares `ARG GCC_ARM_VERSION`.
BuildKit does carry it, which is why CI/the published images were unaffected.
Fix: declare GCC_ARM_VERSION as a global ARG (before the first FROM) so the empty
redeclares in base-amd64/base-arm64/final inherit it on both engines. QT_VERSION
was already global, so it was unaffected.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
linuxdeploy-plugin-qt fails to package Companion's AppImage because edgetx-qt is built with FEATURE_ffmpeg=ON, producing libffmpegmediaplugin.so linked against libavformat.so.58 and other FFmpeg 4.4 libs - none of which are present in the runtime image.
4ddcf05 to
e6f19b0
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dev/Dockerfile (2)
128-136:⚠️ Potential issue | 🟠 MajorVerify and pin remote artifacts (SDL + Arm toolchain) before installing
dev/Dockerfileclones SDL from a tag (SDL2_TAG) without pinning to an immutable commit, and immediately extracts the Arm*.tar.xztoolchain afterwgetwith no SHA256/GPG verification. Pin SDL to a specific commit (or a tarball digest) and validate the vendor-published SHA256 for each toolchain archive before runningtar -xJ. Also applies to the Arm toolchain download/extract steps in thebase-amd64andbase-arm64stages (around lines 160-170).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/Dockerfile` around lines 128 - 136, The Dockerfile clones SDL using the variable SDL2_TAG and extracts Arm toolchains without integrity checks; update the SDL clone and toolchain download steps to use immutable artifacts and verify checksums: pin the SDL checkout to a specific commit (or fetch and verify a signed tarball/digest instead of a branch), or use a release tarball with a known SHA256 digest, and for each wget + tar -xJ (Arm toolchain in base-amd64 and base-arm64 stages) download the accompanying SHA256 (or GPG) signature and verify it before extracting; replace the direct `git clone ... --branch ${SDL2_TAG}` and immediate `tar -xJ` with commands that fetch the digest, validate it against the downloaded file, and only then proceed to extract/install (referencing SDL2_TAG, the git clone step, the tar -xJ toolchain extraction lines, and the base-amd64/base-arm64 stages).
147-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid hardcoding the Qt host directory to
gcc_64indev/Dockerfile
dev/DockerfilesetsQT_BASE_DIR=${QT_INSTALL_DIR}/${QT_VERSION}/gcc_64and derivesPATH,QT_PLUGIN_PATH,QML_IMPORT_PATH,QML2_IMPORT_PATH, andLD_LIBRARY_PATHfrom it. This assumes the Qt bundle’s host subdirectory is identical across architectures; if the arm64 Qt layout differs, the final image will export broken paths. ComputeQT_BASE_DIRfrom the actual directory present under/opt/qt/${QT_VERSION}(or make it arch-aware) instead of hardcodinggcc_64.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/Dockerfile` around lines 147 - 154, QT_BASE_DIR is hardcoded to use gcc_64 causing broken paths on non-x86 architectures; change the Dockerfile logic that sets QT_BASE_DIR (currently using QT_INSTALL_DIR, QT_VERSION and "gcc_64") to dynamically resolve the actual subdirectory under /opt/qt/${QT_VERSION} (or select based on target arch) and then derive PATH, QT_PLUGIN_PATH, QML_IMPORT_PATH, QML2_IMPORT_PATH, LD_LIBRARY_PATH and PKG_CONFIG_PATH from that resolved QT_BASE_DIR; update the ENV assignment for QT_BASE_DIR and ensure subsequent ENV uses (PATH, QT_PLUGIN_PATH, QML_IMPORT_PATH, QML2_IMPORT_PATH, LD_LIBRARY_PATH, PKG_CONFIG_PATH) reference the computed variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@dev/Dockerfile`:
- Around line 128-136: The Dockerfile clones SDL using the variable SDL2_TAG and
extracts Arm toolchains without integrity checks; update the SDL clone and
toolchain download steps to use immutable artifacts and verify checksums: pin
the SDL checkout to a specific commit (or fetch and verify a signed
tarball/digest instead of a branch), or use a release tarball with a known
SHA256 digest, and for each wget + tar -xJ (Arm toolchain in base-amd64 and
base-arm64 stages) download the accompanying SHA256 (or GPG) signature and
verify it before extracting; replace the direct `git clone ... --branch
${SDL2_TAG}` and immediate `tar -xJ` with commands that fetch the digest,
validate it against the downloaded file, and only then proceed to
extract/install (referencing SDL2_TAG, the git clone step, the tar -xJ toolchain
extraction lines, and the base-amd64/base-arm64 stages).
- Around line 147-154: QT_BASE_DIR is hardcoded to use gcc_64 causing broken
paths on non-x86 architectures; change the Dockerfile logic that sets
QT_BASE_DIR (currently using QT_INSTALL_DIR, QT_VERSION and "gcc_64") to
dynamically resolve the actual subdirectory under /opt/qt/${QT_VERSION} (or
select based on target arch) and then derive PATH, QT_PLUGIN_PATH,
QML_IMPORT_PATH, QML2_IMPORT_PATH, LD_LIBRARY_PATH and PKG_CONFIG_PATH from that
resolved QT_BASE_DIR; update the ENV assignment for QT_BASE_DIR and ensure
subsequent ENV uses (PATH, QT_PLUGIN_PATH, QML_IMPORT_PATH, QML2_IMPORT_PATH,
LD_LIBRARY_PATH, PKG_CONFIG_PATH) reference the computed variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 647a4bed-ccb7-4a71-9e52-9aee555f92a2
📒 Files selected for processing (2)
builder/Dockerfiledev/Dockerfile
💤 Files with no reviewable changes (1)
- builder/Dockerfile
Reworks
edgetx-devandedgetx-builderto build natively on amd64 and arm64 (e.g. Apple Silicon Macs, arm64 Linux), replacing the previous QEMU-based amd64-only approach.Changes
edgetx-dev: Qt is now built from source via a dedicatededgetx-qtimage (already onmain) instead ofaqtinstall, which ships no Linux/arm64 binaries. SDL2 is built from source (release-2.32.4), replacing the amd64-onlysavoury1PPA. Multi-stage layout selects the ARM toolchain and Qt/runtime deps viaTARGETARCH.edgetx-builder: same multi-stage,TARGETARCH-driven pattern for the ARM cross toolchain.docker-image.yml):devandbuildernow build natively per-arch onubuntu-24.04/ubuntu-24.04-arm, push by digest, and a merge job assembles the multi-arch manifest withdocker buildx imagetools create. Added GitHub Actions layer caching (cache-from/cache-to: type=gha) acrossdev,builder, andwasi.OPENSSL_ROOT_DIRand unuseddfu-util.--build-arg TARGETARCH=$(uname -m)for local native builds.Validation
Validated against EdgeTX CI using
edgetx-dev:pr-42—commit-tests/buildand Companion Linux AppImage packaging are green on bothubuntu-24.04(amd64) andubuntu-24.04-arm(arm64).