-
Notifications
You must be signed in to change notification settings - Fork 0
Add support for arm64 images #83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for building multi-platform Docker images (amd64 and arm64) by migrating from standard Docker builds to Docker Buildx. The changes introduce a new buildx builder instance and update build targets to support cross-platform compilation.
Key changes:
- Replaces
dockerwithdocker buildxfor multi-platform image builds - Introduces a new buildx builder machine with configurable platform support (default: linux/amd64,linux/arm64)
- Updates the push workflow to use buildx's
--pushaction instead of separatedocker pushcommands for workloads
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
823f27b to
214a721
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4a629ca to
73c5045
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| push-workload-%: | ||
| ACTION=--push \ | ||
| TARGET_PLATFORMS=$(SUPPORTED_PLATFORMS) \ | ||
| $(MAKE) build-workload-$(subst :,/,$*) |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The target substitution uses the wrong separator. This line should use the colon separator to match the pattern variable, not a forward slash. The correct syntax should be build-workload-$* (not build-workload-$(subst :,/,$*)) since the subst operation is already done by the pattern matching.
| $(MAKE) build-workload-$(subst :,/,$*) | |
| $(MAKE) build-workload-$* |
| - name: Setup Docker Buildx | ||
| uses: docker/setup-buildx-action@e468171a9de216ec08956ac3ada2f0791b6bd435 # v3.11.1 | ||
|
|
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setup-buildx-action creates a default buildx builder, but the Makefile's buildx-machine target will create its own builder named 'qubesome' with specific configuration (network=host). This means the builder created by the action may be unused. Consider either removing the setup-buildx-action step since the Makefile manages its own builder, or configuring the action to create a builder with the same name and options to avoid creating multiple builders.
| - name: Setup Docker Buildx | |
| uses: docker/setup-buildx-action@e468171a9de216ec08956ac3ada2f0791b6bd435 # v3.11.1 | |
| $(BUILDER) use $(MACHINE) >/dev/null 2>&1 || \ | ||
| $(BUILDER) create --name=$(MACHINE) --driver-opt network=host --platform=$(SUPPORTED_PLATFORMS) | ||
|
|
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buildx-machine target attempts to use a builder before checking if it exists, which will always fail on the first run. The logic should first check if the builder exists (using docker buildx inspect) before attempting to use it. The current implementation with 'use' followed by 'create' on failure works but produces unnecessary error output on first run.
| $(BUILDER) use $(MACHINE) >/dev/null 2>&1 || \ | |
| $(BUILDER) create --name=$(MACHINE) --driver-opt network=host --platform=$(SUPPORTED_PLATFORMS) | |
| $(BUILDER) inspect $(MACHINE) >/dev/null 2>&1 || \ | |
| $(BUILDER) create --name=$(MACHINE) --driver-opt network=host --platform=$(SUPPORTED_PLATFORMS) >/dev/null 2>&1 | |
| $(BUILDER) use $(MACHINE) >/dev/null 2>&1 |
Makefile
Outdated
| MACHINE = qubesome | ||
|
|
||
| # ACTION can only be --load when TARGET_PLATFORM is the current platform: | ||
| # TARGET_PLATFORMS=linux/amd64 ACTION=--load make push-workload-xorg |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example command calls a push-workload target while trying to use ACTION=--load. However, the push-workload-% target explicitly sets ACTION=--push in its recipe (line 42), which will override the command-line variable in the sub-make invocation. The example should instead demonstrate calling the build-workload target directly, or the comment should clarify that this example won't work as intended with push-workload targets.
| # TARGET_PLATFORMS=linux/amd64 ACTION=--load make push-workload-xorg | |
| # TARGET_PLATFORMS=linux/amd64 ACTION=--load make build-workload-xorg |
| - name: Test build and push of images | ||
| run: make push |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both matrix jobs (amd64 and arm64 runners) build and push images for both platforms (linux/amd64,linux/arm64), which means the same multi-platform images are built and pushed twice. This is redundant and wastes CI resources. Consider either having each runner build only for its native platform, or having only one runner build the multi-platform images. If the intent is to test that the build process works on both runner architectures, this should be documented.
Signed-off-by: Paulo Gomes <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
.github/workflows/pr.yml:55
- The smoke tests run Chrome which is in the AMD64_ONLY list, meaning it will fail on ARM64 runners. The smoke test step should either be conditioned to skip ARM64-incompatible workloads on ARM64 runners, or the tests should be organized to only test workloads compatible with each platform.
- name: Smoke test binaries
run: |
docker run "${REGISTRY}/xorg:${TAG}" awesome --version
docker run "${REGISTRY}/xorg:${TAG}" i3 --version
docker run "${REGISTRY}/code:${TAG}" code --version
docker run "${REGISTRY}/chrome:${TAG}" google-chrome --version
docker run "${REGISTRY}/vivaldi:${TAG}" vivaldi --version
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ARG REGISTRY=ghcr.io/qubesome | ||
| FROM ${REGISTRY}/base:${TAG} | ||
|
|
||
| ARG TARGETARCH |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TARGETARCH build argument is declared but never used in this Dockerfile. If this architecture-specific argument is not needed for the chromium workload, it should be removed to avoid confusion. If it will be needed for conditional logic (e.g., installing different packages based on architecture), then it should be utilized in the build steps.
| ARG TARGETARCH |
No description provided.