RTI dockerfile support for multi-architecture builds #464
Conversation
|
Warning Rate limit exceeded@lhstrh has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 32 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughRecent updates enhance the RTI's Dockerfile and GitHub Actions workflow, introducing multi-platform support and optimizing build efficiency. The modifications refine CI/CD job configurations, adding a new job for Docker builds while maintaining the existing native build functionality. This restructuring improves flexibility and performance across various deployment environments. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- core/federated/RTI/rti.Dockerfile (1 hunks)
Additional comments not posted (3)
core/federated/RTI/rti.Dockerfile (3)
10-22: LGTM! Verify the build process.The builder stage correctly sets up the build environment, installs necessary dependencies, and builds the application.
Ensure that the build process completes successfully for each platform.
1-8: LGTM! Verify base image compatibility.The initial setup correctly defines a base image and derives from architecture-specific base images to support multiarchitecture builds.
Ensure that the specified base images are compatible with the respective platforms.
Verification successful
Verified: Base image compatibility confirmed
The initial setup correctly defines a base image and derives from architecture-specific base images to support multiarchitecture builds. The specified base images are compatible with the respective platforms.
alpine:latestexists and supportsamd64.arm64v8/alpine:latestexists and supportsarm64.arm32v7/alpine:latestexists and supportsarm.riscv64/alpine:latestexists and supportsriscv64.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify compatibility of the specified base images with the respective platforms. # Test: Pull the base images for each platform. Expect: Successful pull for each base image. docker pull alpine:latest docker pull arm64v8/alpine:latest docker pull arm32v7/alpine:latest docker pull riscv64/alpine:latestLength of output: 431
Script:
#!/bin/bash # Description: Verify the existence of the specified base images in the Docker Hub registry. # Function to check if an image exists on Docker Hub check_image() { local image=$1 local tag=${2:-latest} local url="https://hub.docker.com/v2/repositories/${image}/tags/${tag}" # Using curl to check the image existence response=$(curl --silent --write-out "HTTPSTATUS:%{http_code}" -X GET "${url}") http_status=$(echo $response | tr -d '\n' | sed -e 's/.*HTTPSTATUS://') if [ "$http_status" -eq 200 ]; then echo "Image ${image}:${tag} exists." else echo "Image ${image}:${tag} does not exist." fi } # Check the base images check_image "library/alpine" "latest" check_image "arm64v8/alpine" "latest" check_image "arm32v7/alpine" "latest" check_image "riscv64/alpine" "latest"Length of output: 12795
24-32: LGTM! Verify the runtime environment.The application stage correctly sets up the runtime environment and defines the entry point.
Ensure that the runtime environment is correctly set up and the application starts as expected for each platform.
lhstrh
changed the title
There was a problem hiding this comment.
This looks great! The problem is that we're not currently testing Docker builds, only native builds (see https://github.com/lf-lang/reactor-c/blob/main/.github/workflows/build-rti.yml). We should add a simple test to make sure that building using Docker succeeds. Could you take care of this?
As a side note, we need to regularly push updates to DockerHub, which is currently done manually, and way too infrequently. We should automate this, but I'm unsure how to considering that neither the RTI nor reactor-c are versioned.
|
I'm not sure I'll have time to create a test for Docker, but this is something we could consider getting some help with. Since it's not part of the development workflow and only used by a small group, could we get by without adding a test? In a sense RTI is versioned, when building xronos-inc/lf-dev-docker, the Dockerfile builds the RTI submodule that is associated with the LF release. |
|
I could add a simple test that ensures the docker build works correctly. |
|
I'm actually having trouble building manually: and |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/build-rti.yml (1 hunks)
Additional comments not posted (4)
.github/workflows/build-rti.yml (4)
7-7: LGTM! Approve the job rename and matrix strategy.The job rename from
runtonative-buildand the addition of the matrix strategy to run on multiple platforms look good.
21-21: LGTM! Approve the addition of thedocker-buildjob.The addition of the
docker-buildjob to handle multi-architecture Docker builds looks good.
25-25: Verify compatibility ofactions/checkout@v4.Ensure that the
actions/checkout@v4is compatible with the rest of the workflow and does not introduce any breaking changes.
15-15: Verify compatibility ofactions/checkout@v4.Ensure that the
actions/checkout@v4is compatible with the rest of the workflow and does not introduce any breaking changes.
|
Testing for multiarchitecture builds would be more involved. Running |
But |
|
@lhstrh it looks like you're working with an older build of Docker? See the output Your docker installation shows you're using a deprecated builder that predates BuildKit (and the notation for build stages and multiarchitecure builds). |
lhstrh
force-pushed
the
rti-docker-multiarch
branch
2 times, most recently
from
294164b to
bffdaf9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/build-rti.yml (1 hunks)
Additional comments not posted (1)
.github/workflows/build-rti.yml (1)
7-20: LGTM!The
native-buildjob correctly implements a matrix strategy for different platforms and includes steps to check out the repository and build the RTI with both AUTH options.
lhstrh
force-pushed
the
rti-docker-multiarch
branch
from
bffdaf9 to
6778493
Compare
lhstrh
force-pushed
the
rti-docker-multiarch
branch
from
6778493 to
0252cc8
Compare
…educe image size by 20x. Add maintainer documentation to Dockerfile.
lhstrh
force-pushed
the
rti-docker-multiarch
branch
from
0252cc8 to
658c28a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/build-rti.yml (1 hunks)
- core/federated/RTI/rti.Dockerfile (1 hunks)
Additional comments not posted (4)
.github/workflows/build-rti.yml (2)
7-20: LGTM!The
native-buildjob is well-structured and maintains the original functionality. The upgrade of the checkout action to versionv4is appropriate.
21-35: LGTM!The
docker-buildjob is well-structured and follows best practices for multi-architecture Docker builds. The use of Docker Buildx is appropriate.core/federated/RTI/rti.Dockerfile (2)
1-21: LGTM!The use of
ARG BASEIMAGEand multiple base images for different architectures is appropriate. Thebuilderstage is well-structured and follows best practices for multi-stage builds.
22-32: LGTM!The
appstage is well-structured and follows best practices for multi-stage builds. The overall structure of the Dockerfile is clear and efficient.
Co-authored-by: Shulu Li <[email protected]>
Co-authored-by: Shulu Li <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Add multiarchitecture support and use build stages to reduce image size.
docker build . --file core/federated/RTI/rti.Dockerfileworks as it did before.To build for multiple architectures:
docker build . --file core/federated/RTI/rti.Dockerfile --platform=linux/amd64,linux/aarch64,linux/arm/7,linux/riscv64Summary by CodeRabbit