Conversation
WalkthroughAdds multi-architecture support to CI and build scripts: deploy-manylinux workflow gains an amd64/arm64 matrix with architecture-specific Dockerfiles and tags; main workflow adds an ARM64 CMake install path; CMake and library helper scripts detect and propagate architecture for Boost and COIN-OR builds; one Docker base image tag updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions (matrix job)
participant Runner as Runner (ubuntu/arm64 or ubuntu/amd64)
participant Step as Job steps
participant Docker as Docker build/push
Note over GH,Runner #DDEEFF: Matrix expands for amd64 and arm64
GH->>Runner: spawn job (matrix.arch)
Runner->>Step: Checkout (actions/checkout@v4)
Step->>Step: Set architecture-specific env vars\n(env.dockerfile, platform_tag)
Step->>Docker: Build using ${env.dockerfile} on ${platform_tag}
Docker->>Docker: Tag image ghcr.io/${repo}/contrib_manylinux_2_34:${tag}-${platform_tag}
Docker->>GH: Push image
Note over Docker,GH #E8F5E9: Artifact and tag include platform suffix for distinction
sequenceDiagram
autonumber
participant GH as GitHub Actions (main)
participant Runner as Runner (matrix.os)
participant Step as Job steps
Note over GH,Runner #FFF3E0: Conditional CMake setup for ubuntu-22.04-arm
GH->>Runner: start job
Runner->>Step: if matrix.os == 'ubuntu-22.04-arm' -> apt-get install cmake
alt matrix.os != 'ubuntu-22.04-arm'
Step->>Step: run existing "Setup cmake" step
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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 |
…4", should have gone to add_arm branch This reverts commit 3793bab.
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 (1)
dockerfiles/pyopenms/manylinux/ARM64_Dockerfile (1)
25-25: Update LABEL to match the new base image.The LABEL still references
manylinux_2_34_aarch64_main, but Line 1 now usesmanylinux_2_34_aarch64. Update the LABEL to maintain consistency with the actual base image.Apply this diff:
-LABEL base.image="manylinux_2_34_aarch64_main" +LABEL base.image="manylinux_2_34_aarch64"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/deploy-manylinux-docker.yml(2 hunks).github/workflows/main.yml(1 hunks)CMakeLists.txt(1 hunks)dockerfiles/pyopenms/manylinux/ARM64_Dockerfile(1 hunks)libraries.cmake/boost.cmake(2 hunks)libraries.cmake/coinor.cmake(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/deploy-manylinux-docker.yml
56-56: the runner of "docker/build-push-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (windows-latest)
🔇 Additional comments (8)
libraries.cmake/boost.cmake (2)
101-109: LGTM! Architecture detection is well-implemented.The architecture detection logic correctly maps common processor identifiers to Boost's architecture flags, with a helpful warning for unknown architectures. This enables proper cross-platform builds.
178-182: LGTM! Architecture variable properly propagated.The
BOOST_ARCHITECTUREvariable is correctly integrated into both the command string and the actual execution, replacing the previous hardcodedarchitecture=x86approach.libraries.cmake/coinor.cmake (3)
139-151: LGTM! Build triplet logic correctly handles Linux architectures.The BUILD_TRIPLET is appropriately set only for Linux (where old
config.guessmay need help) and correctly excluded for macOS. The tripletarm-linux-gnufor ARM64 is appropriate for autoconf compatibility.
153-160: LGTM! Shared/static library selection properly centralized.The shared/static build configuration is now clearly defined before the configure step, improving readability and maintainability.
162-190: LGTM! BUILD_TRIPLET properly integrated into configure messages and commands.The triplet is consistently included in the configure invocation and all related log messages, ensuring proper architecture-aware builds and debugging.
CMakeLists.txt (1)
340-345: LGTM! Conditional BOOST_ARG aligns with architecture-aware builds.The change correctly limits
address-model=64to x86_64/AMD64 platforms, allowing ARM64 to use architecture detection from boost.cmake. This maintains consistency with the broader multi-architecture support..github/workflows/deploy-manylinux-docker.yml (2)
12-15: LGTM! Matrix-based architecture support is well-designed.The conditional runner selection (
ubuntu-latestfor amd64,ubuntu-22.04-armfor arm64) combined with the architecture matrix enables proper multi-architecture builds.
46-54: LGTM! Architecture-specific variables enable dynamic build configuration.The conditional logic properly sets
dockerfileandplatform_tagbased on the matrix architecture, supporting both amd64 and arm64 builds with appropriate Docker contexts.
| uses: docker/build-push-action@v3 | ||
| with: | ||
| push: true # Will only build if this is not here | ||
| file: dockerfiles/pyopenms/manylinux/Dockerfile | ||
| push: true | ||
| file: ${{ env.dockerfile }} | ||
| context: . | ||
| build-args: | | ||
| OPENMS_BRANCH=${{ steps.extract_branch.outputs.branch }} | ||
| OPENMS_VERSION=${{ steps.tag_name.outputs.tag }} | ||
| tags: | | ||
| ghcr.io/openms/contrib_manylinux_2_34:${{ steps.tag_name.outputs.tag }} | ||
|
|
||
| ghcr.io/${{ steps.downcase_repo.outputs.repo }}/contrib_manylinux_2_34:${{ steps.tag_name.outputs.tag }}-${{ env.platform_tag }} |
There was a problem hiding this comment.
Update docker/build-push-action to a recent version.
The action uses v3, which is outdated and may not run on current GitHub Actions runners. The dynamic dockerfile and platform_tag integration is correct, but the action version should be updated.
Based on static analysis hints.
Apply this diff:
- - name: Build and Push Docker Image (${{ matrix.arch }})
- uses: docker/build-push-action@v3
+ - name: Build and Push Docker Image (${{ matrix.arch }})
+ uses: docker/build-push-action@v6Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 actionlint (1.7.8)
56-56: the runner of "docker/build-push-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/deploy-manylinux-docker.yml around lines 56 to 65: the
workflow references docker/build-push-action@v3 which is outdated; update the
action to the current major release (for example docker/build-push-action@v4) by
changing the uses reference to the newer version while keeping all existing
with: inputs (push, file, context, build-args, tags) intact so the dynamic
dockerfile and platform_tag continue to work.
…4", should have gone to add_arm branch This reverts commit 3793bab.
|
resolved in #155 |
Summary by CodeRabbit