ARM builder image #373
ARM builder image #373
Conversation
WalkthroughThe pull request includes several modifications across multiple files, primarily focusing on updates to development and CI configurations. It removes a development container configuration file, updates CI workflows for Elixir, introduces a new Dockerfile for a Livebook environment, and adjusts dependency specifications in various files. Notably, the project version is incremented, and new instructions for ARM architecture builds are added. The changes enhance the build and deployment processes while maintaining the overall structure of the project. Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🪛 actionlint (1.7.4).github/workflows/release.yml176-176: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue (action) 181-181: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue (action) 189-189: shellcheck reported issue in this script: SC2086:info:1:55: Double quote to prevent globbing and word splitting (shellcheck) 192-192: shellcheck reported issue in this script: SC2086:info:1:15: Double quote to prevent globbing and word splitting (shellcheck) 🔇 Additional comments (9)README.md (1)
The version update to "~> 0.4.0" is consistent with changes in other files. Let's verify the package version is available: ✅ Verification successfulVersion 0.4.0 is available in Hex.pm registry The version update to "~> 0.4.0" in the README.md is valid as the package is published and available in the Hex.pm registry. Users will be able to install this version successfully. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check if the package version exists in Hex.pm registry
# Expected: Version 0.4.0 should be available
# Check available versions
curl -s https://hex.pm/api/packages/beaver | jq '.releases[].version'
Length of output: 355 lib/beaver/pass_runner.ex (2)
The addition of Logger is appropriate for the new error handling implementation.
The error handling has been modified to log errors instead of re-raising them. While logging is good for observability, swallowing exceptions might hide issues from upstream error handlers. Let's verify the impact of this change: Consider one of these approaches:
test/pass_test.exs (2)
The
The test case has been appropriately modified to work with the new error handling behavior while maintaining the same verification of error conditions. .github/workflows/release.yml (4)
The addition of
The matrix strategy effectively handles both ARM64 and AMD64 architectures with appropriate Zig binary URLs for each platform.
Update checkout action versions The checkout actions should be updated to the latest version. - - uses: actions/checkout@v3
+ - uses: actions/checkout@v4
- - uses: actions/checkout@v3
+ - uses: actions/checkout@v4🧰 Tools🪛 actionlint (1.7.4)176-176: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue (action) 181-181: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue (action)
Fix shell script quoting issues The Docker run commands need proper quoting to prevent word splitting. - run: docker run --platform ${{ matrix.image.platform }} -v $PWD:/src -w /src/beaver jackalcooper/beaver-livebook-${{ matrix.image.suffix }}:latest bash scripts/build-for-publish.sh
+ run: docker run --platform "${{ matrix.image.platform }}" -v "$PWD:/src" -w /src/beaver "jackalcooper/beaver-livebook-${{ matrix.image.suffix }}:latest" bash scripts/build-for-publish.sh
- run: docker run -v $PWD:/src -w /src/beaver jackalcooper/beaver-livebook-${{ matrix.image.suffix }}:latest bash scripts/build-for-publish.sh
+ run: docker run -v "$PWD:/src" -w /src/beaver "jackalcooper/beaver-livebook-${{ matrix.image.suffix }}:latest" bash scripts/build-for-publish.sh🧰 Tools🪛 actionlint (1.7.4)189-189: shellcheck reported issue in this script: SC2086:info:1:55: Double quote to prevent globbing and word splitting (shellcheck) 192-192: shellcheck reported issue in this script: SC2086:info:1:15: Double quote to prevent globbing and word splitting (shellcheck) 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 using PR comments)
Other keywords and placeholders
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: 5
🧹 Outside diff range and nitpick comments (4)
docker/livebook.dockerfile (2)
2-8: Optimize package installation layerThe current package installation can be optimized to reduce image size.
Suggested improvement:
-RUN apt-get upgrade -y \ - && apt-get update \ - && apt-get install --no-install-recommends -y \ - ninja-build \ - python3-pip \ - && apt-get clean \ - && rm -rf /var/lib/apt/lists/* +RUN apt-get update && \ + apt-get install --no-install-recommends -y \ + ninja-build \ + python3-pip && \ + apt-get clean && \ + rm -rf /var/lib/apt/lists/*Note: Removed
apt-get upgradeas it's not typically needed in container builds.
1-31: Consider multi-stage build optimizationThe current Dockerfile could be optimized further using multi-stage builds.
Consider:
- Separating build dependencies from runtime dependencies
- Using a slimmer base image for the final stage
- Adding proper Docker metadata labels
- Implementing health checks
Would you like me to provide a fully optimized version of the Dockerfile?
CONTRIBUTING.md (1)
107-110: Enhance ARM build documentationWhile the Docker command is correct, the documentation could be improved by adding:
- Prerequisites (Docker installation, architecture requirements)
- Expected output and verification steps
- Troubleshooting guide for common issues
Consider adding this additional documentation:
### Linux -Run docker image to build for ARM: +#### Building for ARM + +Prerequisites: +- Docker installed and running +- Docker with ARM64 support enabled + +Steps: ```bash docker run -it --rm -v $PWD/..:/src -w /src/beaver --env MIX_BUILD_ROOT='_build/arm' jackalcooper/beaver-livebook-arm64:latest bash scripts/build-for-publish.sh
+Expected output:
+- Successful build will create artifacts in_build/armdirectory
+- Generated files should include ARM-specific.sofiles
+
+Troubleshooting:
+- If Docker fails to pull the image, ensure you have ARM64 support enabled
+- If build fails, check the Docker container's architecture withuname -m</blockquote></details> <details> <summary>.github/workflows/release.yml (1)</summary><blockquote> `167-170`: **Consider adding Docker image versioning** Currently only using 'latest' tag. Consider: - Adding version tags based on releases - Implementing a tag cleanup strategy ```diff - tags: jackalcooper/beaver-livebook-${{ matrix.image.suffix }}:latest + tags: | + jackalcooper/beaver-livebook-${{ matrix.image.suffix }}:latest + jackalcooper/beaver-livebook-${{ matrix.image.suffix }}:${{ github.ref_name }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
mix.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.devcontainer/devcontainer.json(0 hunks).github/workflows/elixir.yml(2 hunks).github/workflows/release.yml(3 hunks)CONTRIBUTING.md(1 hunks)dev-requirements.txt(1 hunks)docker/livebook.dockerfile(1 hunks)mix.exs(3 hunks)native/Makefile(1 hunks)scripts/build-for-publish.sh(1 hunks)
💤 Files with no reviewable changes (1)
- .devcontainer/devcontainer.json
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yml
177-177: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
182-182: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
190-190: shellcheck reported issue in this script: SC2086:info:1:55: Double quote to prevent globbing and word splitting
(shellcheck)
193-193: shellcheck reported issue in this script: SC2086:info:1:15: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (9)
scripts/build-for-publish.sh (1)
4-4: LGTM! Verify dependency fetching behavior.
Adding mix deps.get before precompilation is a good practice to ensure all dependencies are available.
Let's verify this doesn't cause issues with cached dependencies:
✅ Verification successful
LGTM! Dependencies are properly managed
The mix deps.get addition is safe and follows best practices:
- Dependencies are properly defined in
mix.exswith specific version constraints - The
mix.lockfile ensures reproducible builds by locking exact versions - The script sets
ELIXIR_MAKE_CACHE_DIR=.which properly handles cached dependencies - Dependencies are fetched before precompilation, which is the correct order
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any dependency-related issues reported in recent commits
git log -n 10 --grep="dep\|dependencies\|mix" --pretty=format:"%h %s"
Length of output: 0
Script:
#!/bin/bash
# Let's check the mix.lock file and mix.exs for dependency configuration
fd "mix\.(lock|exs)$" --exec cat {}
# Also check if there are any other build scripts that handle dependencies
fd "build.*\.sh$" --exec cat {}
Length of output: 7636
native/Makefile (1)
1-1: Verify LLVM config path availability on ARM
The simplified LLVM_CONFIG_PATH assignment assumes llvm-config is available in PATH. While this change makes the Makefile cleaner, we should ensure it works consistently across ARM environments.
Consider documenting the LLVM installation requirements for ARM builds in the README or CONTRIBUTING guide.
✅ Verification successful
LLVM config path change is safe and well-documented
The simplified LLVM_CONFIG_PATH assignment is appropriate because:
- The project provides clear installation instructions for LLVM/MLIR in CONTRIBUTING.md
- The Makefile retains the ability to override the path using
?=operator - ARM builds are handled through documented Docker-based builds with proper environment setup
The change aligns with the project's documented build process and maintains compatibility across all platforms, including ARM.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if llvm-config is available and accessible in common paths
# Test 1: Check if llvm-config exists in standard paths
which llvm-config || echo "llvm-config not found in PATH"
# Test 2: Check llvm-config version and target architecture
llvm-config --version
llvm-config --host-target
Length of output: 440
Script:
#!/bin/bash
# Let's check the Makefile history and documentation for ARM-specific configurations
# and any existing LLVM-related documentation
# Check if there are any ARM-specific instructions in documentation
fd -t f "README|CONTRIBUTING|INSTALL" -X grep -i -l "arm\|aarch64\|llvm"
# Look for any ARM or LLVM related configuration in the Makefile
cat native/Makefile | grep -i "arm\|aarch64\|llvm"
# Check if there are any CI configurations that might handle ARM builds
fd ".github/workflows" -t f -X cat
Length of output: 1179
Script:
#!/bin/bash
# Let's check the content of README and CONTRIBUTING files for LLVM setup instructions
# and examine if there are any GitHub workflows
cat README.md CONTRIBUTING.md
# Check for workflow files with the correct fd syntax
fd . '.github/workflows'
Length of output: 16281
mix.exs (2)
7-7: Version bump aligns with ARM support feature
The increment to version 0.4.0 appropriately reflects the addition of ARM architecture support as a significant feature.
22-22: Verify ARM artifacts in precompiler URL
The updated precompiler URL points to new build artifacts. Let's verify the ARM-specific artifacts are available.
✅ Verification successful
ARM artifacts are available in the release
The release contains the necessary ARM artifacts:
beaver-nif-2.17-aarch64-apple-darwin-0.4.0.tar.gz(ARM for macOS)beaver-nif-2.17-aarch64-linux-gnu-0.4.0.tar.gz(ARM for Linux)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ARM artifacts existence in the new release
# Test: Check if ARM artifacts exist in the release
gh api \
-H "Accept: application/vnd.github+json" \
repos/beaver-lodge/beaver-prebuilt/releases/tags/2024-12-04-0908 | \
jq -r '.assets[].name' | \
grep -i "arm"
Length of output: 153
Script:
#!/bin/bash
# Let's first check what artifacts are actually available in the release
gh api \
-H "Accept: application/vnd.github+json" \
repos/beaver-lodge/beaver-prebuilt/releases/tags/2024-12-04-0908 | \
jq -r '.assets[].name'
Length of output: 328
.github/workflows/elixir.yml (2)
37-46: LGTM: Matrix strategy updates look good
The matrix configuration changes make sense:
- Pinning to Ubuntu 22.04 provides better stability than using latest
- Adding OTP 27.0.1 with Elixir 1.17.2 ensures testing on newer versions
70-72: Verify the Zig action downgrade
The change from goto-bus-stop/setup-zig@v2 to mlugg/setup-zig@v1 appears to be a version downgrade.
✅ Verification successful
The action change appears to be well-maintained
The switch to mlugg/setup-zig@v1 is actually not concerning as both actions are actively maintained, with the new action having more recent commits (November 2024) compared to the previous one (September 2024). Let's verify the stability and usage of both actions.
The action change is appropriate and well-maintained
The switch to mlugg/setup-zig@v1 is a sound decision. Both actions are actively maintained, with mlugg/setup-zig showing:
- More recent commits (November 2024)
- Fewer open issues (6 vs 12)
- Stable releases with v1.2.1 released in September 2024
- Healthy community adoption (96 stars)
While it may appear as a downgrade from v2 to v1, the version numbers are independent between different repositories, and the actual implementation is what matters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the new action version is actively maintained
gh api repos/mlugg/setup-zig/commits --jq '.[0].commit.committer.date'
# Compare with the old action
gh api repos/goto-bus-stop/setup-zig/commits --jq '.[0].commit.committer.date'
Length of output: 195
Script:
#!/bin/bash
# Check stars and open issues for both actions
echo "mlugg/setup-zig stats:"
gh api repos/mlugg/setup-zig --jq '[.stargazers_count, .open_issues_count]'
echo "goto-bus-stop/setup-zig stats:"
gh api repos/goto-bus-stop/setup-zig --jq '[.stargazers_count, .open_issues_count]'
# Check recent releases for mlugg/setup-zig
gh api repos/mlugg/setup-zig/releases --jq '.[0:2] | .[] | {tag_name, published_at}'
Length of output: 512
.github/workflows/release.yml (3)
7-7: LGTM: Added release branch trigger
Adding release* branch trigger is appropriate for handling release branches.
136-152: LGTM: Matrix strategy for multi-architecture builds
The matrix strategy for ARM64 and AMD64 builds is well-structured:
- Proper platform specifications
- Architecture-specific Zig binaries
- Clear naming conventions with suffixes
154-158: Verify Docker Hub credentials usage
Ensure Docker Hub credentials are properly configured in repository secrets.
Summary by CodeRabbit
Release Notes
New Features
Updates
kinda.mlirpackage.Bug Fixes