Conversation
clang-format 5.0.1 has no aarch64 release packaging, so the existing ml-check-style image is amd64 only and has to run under emulation on an aarch64 host. This Dockerfile builds clang-format 5.0.1 from LLVM source and asserts the resulting version, giving a native ml-check-style-aarch64 image. Manual image-build tooling alongside build_check_style_image.sh; no CI step references it yet.
|
Pinging @elastic/ml-core (Team:ML) |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect image architecture, version verification, and image cleanup.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds manual tooling for an aarch64 Docker image containing clang-format 5.0.1 built from LLVM sources.
Changes:
- Adds the aarch64 Dockerfile and build script.
- Documents building, testing, and usage.
- Registers the image in the Docker inventory.
File summaries
| File | Summary | Final review comments |
|---|---|---|
dev-tools/docker/README.md |
Adds the image inventory entry. | — |
dev-tools/docker/check_style_image_aarch64/README.md |
Documents building and using the image. | nit (2 votes): plain docker build may produce an amd64 image; document the platform selector. |
dev-tools/docker/check_style_image_aarch64/Dockerfile |
Builds and verifies clang-format. | critical (1 vote): version verification is only a substring check. moderate (2 votes): LLVM and Clang downloads do not consistently use the version argument. moderate (1 vote): build dependencies remain installed. |
dev-tools/docker/build_check_style_image_aarch64.sh |
Builds and publishes the image. | critical (3 votes): the build does not explicitly select the arm64 platform. |
Review details
Suppressed comments (1)
dev-tools/docker/check_style_image_aarch64/Dockerfile:71
libc-devandlinux-headersare explicitly installed above but omitted here, soapk delleaves them in the final image as top-level packages. They are only needed to compile LLVM; retaining them contradicts the cleanup comment and unnecessarily increases the runtime image and its package surface.
apk del build-base cmake ninja python3 wget tar xz
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| . ./prefetch_docker_image.sh | ||
| CONTEXT=check_style_image_aarch64 | ||
| prefetch_docker_base_image $CONTEXT/Dockerfile | ||
| docker build --no-cache -t $HOST/$ACCOUNT/$REPOSITORY:$VERSION $CONTEXT |
| apk del build-base cmake ninja python3 wget tar xz | ||
|
|
||
| # Verify clang-format version | ||
| RUN clang-format --version | grep -q "5.0.1" || (echo "ERROR: clang-format version mismatch" && exit 1) |
Comment on lines
+46
to
+48
| wget -q https://releases.llvm.org/5.0.1/cfe-5.0.1.src.tar.xz && \ | ||
| tar -xJf cfe-5.0.1.src.tar.xz && \ | ||
| mv cfe-5.0.1.src clang && \ |
|
|
||
| ```bash | ||
| cd dev-tools/docker | ||
| docker build --no-cache -t ml-check-style-aarch64:local check_style_image_aarch64 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
clang-format 5.0.1 has no aarch64 release packaging, so the existing
ml-check-styleimage is amd64 only and has to run under emulation on an aarch64 host. This adds anml-check-style-aarch64image that builds clang-format 5.0.1 from LLVM source and then asserts the resulting version, along with the matching build script and an inventory entry indev-tools/docker/README.md.Manual image-build tooling alongside
build_check_style_image.sh. Nothing in CI references it yet;format_and_validation.yml.shstill pinsml-check-style:2.Split out of #2873, where running the style gate locally on Apple Silicon needed amd64 emulation.