Add ARM64 cross-build support#323
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds ARM64 cross-compilation support across Dockerfiles, build scripts, CMake/Rust wiring, static Qt plugin handling, command aliases, and build documentation. ChangesARM64 Cross-Build Pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmake/ZaparooRust.cmake (1)
55-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded toolchain paths with no existence check.
/opt/qt6-arm64/bin/qmake6and/opt/qt6-arm32/bin/qmake6are hardcoded absolute paths tied to the Docker toolchain image layout (confirmed by theDockerfile.arm64snippet using the same path). There's noEXISTSguard, so if the path is wrong or the toolchain layout changes, the failure surfaces later and less clearly (e.g., during corrosion's Rust build) instead of at configure time.♻️ Optional early validation
if(CMAKE_SYSTEM_PROCESSOR STREQUAL "aarch64") set(_rs_qmake "/opt/qt6-arm64/bin/qmake6") else() set(_rs_qmake "/opt/qt6-arm32/bin/qmake6") endif() + if(NOT EXISTS "${_rs_qmake}") + message(FATAL_ERROR "Cross qmake6 not found at ${_rs_qmake}; check toolchain image layout") + endif()🤖 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 `@cmake/ZaparooRust.cmake` around lines 55 - 59, The qmake path selection in ZaparooRust.cmake uses hardcoded absolute toolchain paths without validating that the chosen qmake6 exists. Update the logic around the CMAKE_SYSTEM_PROCESSOR branch that sets _rs_qmake to add an EXISTS check (or equivalent configure-time validation) for the selected path, and fail early with a clear message if neither /opt/qt6-arm64/bin/qmake6 nor /opt/qt6-arm32/bin/qmake6 is present.Dockerfile.toolchain.arm64 (1)
244-260: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant Rust toolchain installation.
Line 245 bootstraps rustup with
--default-toolchain stable, then lines 259-260 separately install a pinned1.96.0toolchain (with its ownaarch64-unknown-linux-gnutarget). Since rustup stores toolchains under distinct directory names (stable-...vs1.96.0-...), this likely results in two full toolchain downloads/installs even if they resolve to the same version, wasting image size and build time. Since the actual build later relies onrust-toolchain.toml(pinning presumably to1.96.0) for reproducibility, the "stable" bootstrap toolchain appears to serve no further purpose once1.96.0is installed.♻️ Suggested fix — bootstrap without a default toolchain
RUN wget -qO- https://sh.rustup.rs \ - | sh -s -- -y --default-toolchain stable --profile minimal \ - && /root/.cargo/bin/rustup target add aarch64-unknown-linux-gnu + | sh -s -- -y --default-toolchain none --profile minimal ENV PATH="/root/.cargo/bin:${PATH}" ... RUN rustup toolchain install 1.96.0 --profile minimal \ - && rustup target add --toolchain 1.96.0 aarch64-unknown-linux-gnu + && rustup target add --toolchain 1.96.0 aarch64-unknown-linux-gnu \ + && rustup default 1.96.0🤖 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 `@Dockerfile.toolchain.arm64` around lines 244 - 260, The Rust setup in the Dockerfile is installing two separate toolchains: the initial rustup bootstrap uses a default stable toolchain, then the later rustup toolchain install pins 1.96.0, which duplicates downloads and image size. Update the bootstrap step to install rustup without setting stable as the default, and rely on the pinned 1.96.0 toolchain plus its target setup for the build; keep the existing aarch64 target and cargo config setup unchanged. Locate the fix in the rustup initialization and the rustup toolchain install block.Dockerfile.arm64 (1)
46-67: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
build.ninjaexistence check appears to have no effect without a cache mount.The
if [ ! -f build.ninja ]guard (lines 56-65) only skips re-runningqt-cmakeifbuild.ninjaalready exists inbuild/. Since this Dockerfile doesn't use--mount=type=cache(or any other mechanism to persist/src/buildacross builds), eachdocker buildinvocation starts with an emptybuild/directory, making the check always true. If the intent was to speed up iterative local builds, consider adding a BuildKit cache mount for/src/build; otherwise this conditional can be simplified away.♻️ Suggested fix — add a cache mount to make the guard meaningful
-RUN mkdir -p build && cd build && \ +RUN --mount=type=cache,target=/src/build,sharing=locked \ + mkdir -p build && cd build && \ if [ -n "${ZAPAROO_OFFICIAL_BUILD}" ]; then \🤖 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 `@Dockerfile.arm64` around lines 46 - 67, The build.ninja guard in the Dockerfile.arm64 build step is ineffective because the build directory is not persisted between builds. Update the Dockerfile arm64 build stage around the RUN block using the qt-cmake and ninja steps: either add a BuildKit cache mount for the build directory so build.ninja can be reused, or remove the conditional entirely if no caching is intended. Keep the logic centered on the existing qt-cmake and ninja invocation so the change is localized.
🤖 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 `@Dockerfile.toolchain.arm64`:
- Around line 93-111: The host Qt build in the Dockerfile.toolchain.arm64 image
is using ICU-enabled binaries without ensuring the runtime is present in the
final host toolchain image. Update the host toolchain stage so the Qt host tools
built by the qtbase-host-build step (including moc, rcc, and lupdate) have the
ICU runtime available, either by installing the required ICU runtime package in
the toolchain-base image or by explicitly disabling ICU in the qtbase/configure
invocation if that is the intended behavior.
---
Nitpick comments:
In `@cmake/ZaparooRust.cmake`:
- Around line 55-59: The qmake path selection in ZaparooRust.cmake uses
hardcoded absolute toolchain paths without validating that the chosen qmake6
exists. Update the logic around the CMAKE_SYSTEM_PROCESSOR branch that sets
_rs_qmake to add an EXISTS check (or equivalent configure-time validation) for
the selected path, and fail early with a clear message if neither
/opt/qt6-arm64/bin/qmake6 nor /opt/qt6-arm32/bin/qmake6 is present.
In `@Dockerfile.arm64`:
- Around line 46-67: The build.ninja guard in the Dockerfile.arm64 build step is
ineffective because the build directory is not persisted between builds. Update
the Dockerfile arm64 build stage around the RUN block using the qt-cmake and
ninja steps: either add a BuildKit cache mount for the build directory so
build.ninja can be reused, or remove the conditional entirely if no caching is
intended. Keep the logic centered on the existing qt-cmake and ninja invocation
so the change is localized.
In `@Dockerfile.toolchain.arm64`:
- Around line 244-260: The Rust setup in the Dockerfile is installing two
separate toolchains: the initial rustup bootstrap uses a default stable
toolchain, then the later rustup toolchain install pins 1.96.0, which duplicates
downloads and image size. Update the bootstrap step to install rustup without
setting stable as the default, and rely on the pinned 1.96.0 toolchain plus its
target setup for the build; keep the existing aarch64 target and cargo config
setup unchanged. Locate the fix in the rustup initialization and the rustup
toolchain install block.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68e5d061-c01a-48e1-88ee-55a4d2be69b9
📒 Files selected for processing (11)
AGENTS.mdDockerfile.arm64Dockerfile.toolchain.arm64cmake/ZaparooRust.cmakedocs/building.mdjustfilerust/.cargo/config.tomlscripts/build-arm64.shscripts/build-toolchain-arm64.shscripts/toolchain-arm64/VERSIONsrc/app/main.cpp
1 participant
Summary
just arm64and ARM64 toolchain image usageValidation
just lintbash -n scripts/build-arm64.sh scripts/build-toolchain-arm64.shSummary by CodeRabbit
arm64build command and Docker Buildx workflow.