Repository navigation
Conversation
2040d00 to
d831f99
Compare
Walkthrough
Pre-merge checks:
|
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
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
e6f19b0 to
c84373e
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
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>
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.
checkout v7, upload-artifact v7, download-artifact v8, setup-buildx-action v4, login-action v4, metadata-action v6, build-push-action v7 (Node 24 runtime). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- drop Ubuntu's FFmpeg 4.4 runtime libs; Qt now ships FFmpeg 7.1 in its lib/ - add runtime libs for the XCB (libSM/libICE), Wayland and GTK3 plugins, which ldd reported missing against the new Qt image Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7039923 to
8afdf38
Compare
The source-built SDL2 only had OSS/disk/dummy audio and no Wayland or udev support, as the backend -dev packages weren't installed - so the simulator in Companion would be silent. Install them in the SDL build stage (SDL dlopen()s the backends, so the runtime image doesn't need them), fail the build if a required backend isn't detected, and bump to 2.32.8 to match the Windows Companion builds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The savoury1 PPA build this replaced was a 2.32.11 development snapshot; use the latest SDL2 release rather than shipping an older one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reworks
edgetx-devandedgetx-builderto build natively on amd64 and arm64 (e.g. Apple Silicon Macs, arm64 Linux), replacing the previous amd64-only images.Why Qt is self-built
The official Qt
linux_gcc_arm64binaries (viaaqtinstall) are built on Ubuntu 24.04 and need glibc 2.38. Our images are based on Ubuntu 22.04 (glibc 2.35), so they won't run.edgetx-devnow copies Qt from theedgetx-qtimage (qt/Dockerfile, already onmain) on both arches. That keeps a single Qt recipe and runtime dependency set rather than aqt on amd64 plus a self-build on arm64.The self-built Qt was brought in line with the official aqt package in #57, #58 and #60 (merged):
linux_gcc_64+qtmultimedia qtserialport, including Qt translations and the Wayland, GTK3 and Vulkan pluginslib/with theQt6FFmpegStub-*libs) instead of Ubuntu's FFmpeg 4.4/opt/qt(444 MB, down from ~6.9 GB)macOS and Windows Companion builds are unaffected and keep using the official Qt via aqt.
Changes
edgetx-devedgetx-qt:qt-<version>instead of being installed withaqtinstallrelease-2.32.10, the latest SDL2 release), replacing the amd64-onlysavoury1PPA. The PulseAudio/PipeWire/ALSA audio, X11/Wayland/KMSDRM video and udev backends are all built in, loaded at runtime, and the build fails if any of them is missing.TARGETARCHlibSM/libICE), Wayland and GTK3 plugins. Ubuntu's FFmpeg 4.4 libs are no longer needed.OPENSSL_ROOT_DIRand the unuseddfu-utiledgetx-builder: the same multi-stage,TARGETARCH-driven pattern for the ARM cross toolchaindocker-image.yml)devandbuilderbuild natively per arch onubuntu-24.04/ubuntu-24.04-arm, push by digest, and a merge job assembles the multi-arch manifestdev,builderandwasi--build-arg TARGETARCH=$(uname -m)for local native buildsValidation
edgetx-qt:qt-6.9.3frommain.lddfinds no missing dependencies across all Qt libs and plugins.tools/build-companion.shfrom edgetxmainbuilds Companion andEdgeTX_Companion_3.0.0-aarch64.AppImage. This includes fix(cpn): bundle Wayland platform plugins in Linux AppImage edgetx#7930's Wayland plugin bundling.edgetx-dev:pr-42.Sunset: when the self-built Qt becomes redundant
The self-build exists only because the base OS is too old for the official arm64 Qt. It stops being needed when the minimum supported distro moves past Ubuntu 22.04 / Debian 12. At that point the images can move to Ubuntu 24.04 (glibc 2.39) and go back to aqt on both arches (
linux_gcc_64/linux_gcc_arm64). That would mean:qt/andbuild-qt.ymldev/DockerfileBecause the AppImage is built on the image's base OS, moving to 24.04 also raises the Linux Companion AppImage minimum to glibc 2.39. That drops Ubuntu 22.04 and Debian 12 users.
Suggested cutoff: the first EdgeTX release after 31 May 2027, when Ubuntu 22.04 leaves standard support. Debian 12 will already be on LTS by then, which covers only a subset of packages and is volunteer-run. A conservative alternative is 30 Jun 2028 (end of Debian 12 LTS), but that keeps the Qt self-build around for roughly another year after Ubuntu 22.04 itself stops being supported.