Added ppc64le support#725
Conversation
|
Click here to review and test in web IDE: |
&message=Dev%20cluster%20(for%20maintainers)&logo=eclipseche&color=525C86&labelColor=FDB940)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request adds Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions
participant Build as Docker Buildx
participant Registry as Container Registry
participant Manifest as Manifest Publisher
CI->>Build: Build architecture-specific image
Build->>Registry: Push architecture image
CI->>Manifest: Create amd64/arm64/ppc64le manifest
Manifest->>Registry: Annotate and push manifest
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
build/dockerfiles/linux-musl.Dockerfile (1)
157-185: 💤 Low valueDead code: TARGETARCH branches for ppc64le/arm64 are unreachable.
When building with
--platform linux/ppc64le, the container runs under QEMU emulation, souname -mreturnsppc64le, notx86_64. The outer guard fails for any non-x86_64 build, making the innerTARGETARCHconditionals for ppc64le and arm64 unreachable.Tests only run for native x86_64 builds where
TARGETARCHwill beamd64. The ppc64le/arm64 branches create the false impression that these architectures have test coverage.Consider simplifying to remove the misleading dead branches:
♻️ Simplified test blocks
# Run integration tests (Browser) with architecture-specific path RUN if [ "$(uname -m)" = "x86_64" ]; then \ - if [ "${TARGETARCH}" = "ppc64le" ]; then \ - VSCODE_REMOTE_SERVER_PATH="/vscode-reh-web-linux-ppc64" \ - retry -v -t 3 -s 2 -- timeout 5m ./scripts/test-web-integration.sh --browser chromium; \ - elif [ "${TARGETARCH}" = "arm64" ]; then \ - VSCODE_REMOTE_SERVER_PATH="/vscode-reh-web-linux-arm64" \ - MACHINE_EXEC_MAX_RETRIES=1 \ - retry -v -t 3 -s 2 -- timeout 5m ./scripts/test-web-integration.sh --browser chromium; \ - else \ - VSCODE_REMOTE_SERVER_PATH="/vscode-reh-web-linux-x64" \ - MACHINE_EXEC_MAX_RETRIES=1 \ - retry -v -t 3 -s 2 -- timeout 5m ./scripts/test-web-integration.sh --browser chromium; \ - fi \ + VSCODE_REMOTE_SERVER_PATH="/vscode-reh-web-linux-x64" \ + MACHINE_EXEC_MAX_RETRIES=1 \ + retry -v -t 3 -s 2 -- timeout 5m ./scripts/test-web-integration.sh --browser chromium; \ fi # Run smoke tests (Browser) with architecture-specific path RUN if [ "$(uname -m)" = "x86_64" ]; then \ - if [ "${TARGETARCH}" = "ppc64le" ]; then \ - VSCODE_REMOTE_SERVER_PATH="/vscode-reh-web-linux-ppc64" \ - retry -v -t 3 -s 2 -- timeout 5m npm run smoketest-no-compile -- --web --headless --electronArgs="--disable-dev-shm-usage --use-gl=swiftshader"; \ - elif [ "${TARGETARCH}" = "arm64" ]; then \ - VSCODE_REMOTE_SERVER_PATH="/vscode-reh-web-linux-arm64" \ - retry -v -t 3 -s 2 -- timeout 5m npm run smoketest-no-compile -- --web --headless --electronArgs="--disable-dev-shm-usage --use-gl=swiftshader"; \ - else \ - VSCODE_REMOTE_SERVER_PATH="/vscode-reh-web-linux-x64" \ - retry -v -t 3 -s 2 -- timeout 5m npm run smoketest-no-compile -- --web --headless --electronArgs="--disable-dev-shm-usage --use-gl=swiftshader"; \ - fi \ + VSCODE_REMOTE_SERVER_PATH="/vscode-reh-web-linux-x64" \ + retry -v -t 3 -s 2 -- timeout 5m npm run smoketest-no-compile -- --web --headless --electronArgs="--disable-dev-shm-usage --use-gl=swiftshader"; \ fi🤖 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 `@build/dockerfiles/linux-musl.Dockerfile` around lines 157 - 185, The integration tests and smoke tests blocks both contain unreachable conditional branches for ppc64le and arm64 architectures due to an outer guard checking if uname returns x86_64, which fails for non-x86_64 builds. The inner TARGETARCH conditionals for ppc64le and arm64 can never execute because the outer condition prevents entry into the entire block for those architectures. Remove the outer if [ "$(uname -m)" = "x86_64" ] guard and the unreachable inner elif branches for ppc64le and arm64 from both the integration tests RUN block and the smoke tests RUN block, keeping only the x86_64 path content with VSCODE_REMOTE_SERVER_PATH="/vscode-reh-web-linux-x64" and the associated retry commands.
🤖 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 @.github/workflows/image-publish.yml:
- Around line 46-47: The docker/setup-qemu-action step is pinned to v2, which
relies on the deprecated Node.js 16 runtime that has been removed from GitHub
Actions runners. Update the action version from `@v2` to `@v4` in the "Set up QEMU"
step to use Node.js 20, which is compatible with current runners and includes
the latest security patches.
In @.github/workflows/pull-request-check.yml:
- Around line 112-113: Update the deprecated docker/setup-qemu-action version
from v2 to v4 in the "Set up QEMU" step. Change the uses value from
docker/setup-qemu-action@v2 to docker/setup-qemu-action@v4 to ensure
compatibility with modern GitHub Actions runners and receive performance
improvements, as v2 is no longer maintained.
- Around line 63-64: Update the docker/setup-qemu-action action from v2 to a
newer version in all workflow files where it appears. Replace
docker/setup-qemu-action@v2 with docker/setup-qemu-action@v4.1.0 if your GitHub
Actions Runner is v2.327.1 or later, otherwise use v3.7.0. Make this change in
pull-request-check.yml, image-publish.yml, rebase-insiders.yml, and
rebase-release-branch.yml wherever the outdated action version is referenced.
---
Nitpick comments:
In `@build/dockerfiles/linux-musl.Dockerfile`:
- Around line 157-185: The integration tests and smoke tests blocks both contain
unreachable conditional branches for ppc64le and arm64 architectures due to an
outer guard checking if uname returns x86_64, which fails for non-x86_64 builds.
The inner TARGETARCH conditionals for ppc64le and arm64 can never execute
because the outer condition prevents entry into the entire block for those
architectures. Remove the outer if [ "$(uname -m)" = "x86_64" ] guard and the
unreachable inner elif branches for ppc64le and arm64 from both the integration
tests RUN block and the smoke tests RUN block, keeping only the x86_64 path
content with VSCODE_REMOTE_SERVER_PATH="/vscode-reh-web-linux-x64" and the
associated retry commands.
🪄 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: 35f27713-e0f2-4854-ab8c-57f250504c7e
📒 Files selected for processing (4)
.github/workflows/image-publish.yml.github/workflows/pull-request-check-publish.yml.github/workflows/pull-request-check.ymlbuild/dockerfiles/linux-musl.Dockerfile
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/image-publish.yml:
- Line 47: The docker/setup-qemu-action is pinned using a mutable version tag
`@v4`, which poses a supply chain risk. Replace the `@v4` tag with the full
immutable commit SHA for this action to ensure consistent and secure builds.
Look up the commit SHA for the v4 release of docker/setup-qemu-action and use it
instead of the mutable version tag.
In @.github/workflows/pull-request-check.yml:
- Line 64: The docker/setup-qemu-action is pinned to a mutable version tag `@v4`
which could be retargeted unexpectedly. Replace the mutable tag `@v4` with the
immutable full commit SHA 06116385d9baf250c9f4dcb4858b16962ea869c3 in both
occurrences of docker/setup-qemu-action (at lines 64 and 113). Change the action
reference from docker/setup-qemu-action@v4 to
docker/setup-qemu-action@06116385d9baf250c9f4dcb4858b16962ea869c3 to ensure the
exact version is always used.
🪄 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: 9a108aaa-cac8-4c25-ac7d-b92e4f1ac46c
📒 Files selected for processing (2)
.github/workflows/image-publish.yml.github/workflows/pull-request-check.yml
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
I noticed the following error in the ppc64le builds :
The problem seems to be microsoft/vscode#282188 :\ |
|
@RomanNikitenko mentions one possible solution is to set Update: Looks like you have a similar block at b73caab#diff-da163e78116ac7679a7d35be1a38bd9a06b4f7d3c24b9aaa0a96b9c515bcb1e7R63 . I guess it just needs to be moved to handle this part of the build as well. |
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 (1)
.github/workflows/pull-request-check.yml (1)
121-126: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winOptimize artifact download by filtering for the current architecture.
Using
pattern: linux-*downloads the Docker image artifacts for all architectures, wasting bandwidth and time during the PR check. Filter by the specific architecture, similar to what is done in theimage-publish.ymlworkflow.🔧 Proposed fix
- name: Download editor artifacts uses: actions/download-artifact@v7 with: - pattern: linux-* + pattern: linux-*-${{env.arch}} merge-multiple: true path: .🤖 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/pull-request-check.yml around lines 121 - 126, Update the “Download editor artifacts” step in the pull-request workflow to replace the broad linux-* artifact pattern with the current architecture-specific pattern, matching the architecture filtering used by the image-publish workflow. Preserve merge-multiple and the existing download path.
🤖 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 @.github/workflows/pull-request-check.yml:
- Around line 121-126: Update the “Download editor artifacts” step in the
pull-request workflow to replace the broad linux-* artifact pattern with the
current architecture-specific pattern, matching the architecture filtering used
by the image-publish workflow. Preserve merge-multiple and the existing download
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b7afd89c-3d02-4eb6-8c5a-9065c841e071
📒 Files selected for processing (3)
.github/workflows/image-publish.yml.github/workflows/pull-request-check-publish.yml.github/workflows/pull-request-check.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pull-request-check-publish.yml
| # Make an assembly including both musl and libc variant to be able to run on all linux systems | ||
| FROM docker.io/node:22-alpine3.22 as linux-musl-builder | ||
| # Use Debian-based Node.js image for all architectures (amd64, arm64, ppc64le) | ||
| FROM docker.io/node:22-bookworm as linux-musl-builder |
There was a problem hiding this comment.
The PR currently adds ppc64le support only for the linux-musl.Dockerfile. However, the final assembly.Dockerfile bundles all three variants (musl, ubi8, ubi9) into a single image, and the entrypoint selects the appropriate one at runtime depending on the user's container (musl vs glibc):
My understanding:
If the ppc64le assembly image only contains a musl build while ubi8/ubi9 are built for a different architecture, che-code won't be able to start in glibc-based containers on ppc64le.
Are changes for linux-libc-ubi8.Dockerfile and linux-libc-ubi9.Dockerfile planned as a follow-up, or I just missed something and my understanding is wrong?
There was a problem hiding this comment.
Thanks for the response @RomanNikitenko , as I can see dockerfiles of ubi8/ubi9 are already having a support of ppc64le, and linux-musl was not having an support, so i was thinking it should work.
Please correct me if my understanding is wrong
There was a problem hiding this comment.
You're right that the ubi8/ubi9 Dockerfiles already have architecture-aware logic.
I just saw changes only for linux-musl.Dockerfile and no changes for linux-libc-ubi8.Dockerfile and linux-libc-ubi9.Dockerfile- that's why I was confused.
Should the @vscode/vsce-sign workaround also be included in those Dockerfiles?
There was a problem hiding this comment.
I guess @vscode/vsce-sign workaround is not needed for ubi8/ubi9, since while building locall it worked only by modifying linux-musl.Dockerfile
There was a problem hiding this comment.
Thanks @RomanNikitenko, I will add the patch to ubi8/ubi9 Dockerfiles as well, other than this, do you think is there anything i need to look into?
Updated package-lock.json paths for non-amd64 and non-arm64 architectures.
3 participants
What does this PR do?
Add ppc64le architecture support across build, assemble, and publish workflows to enable multi-arch image generation and distribution alongside existing amd64 and arm64 support. Adjust buildx configuration, artifact handling, and manifest publishing to include ppc64le without changing existing functionality.
What issues does this PR fix?
How to test this PR?
Does this PR contain changes that override default upstream Code-OSS behavior?
git rebasewere added to the .rebase folderSummary by CodeRabbit
New Features
Chores
Build & Test Improvements