build: use cargo binstall to speed up builds #2321
Conversation
…_given_by_identities` (#2309) Co-authored-by: Ivan Shumkov <ivan@shumkov.ru>
…entity-transfers-in-strategy-tests
WalkthroughThe pull request introduces updates to various configuration files, primarily focusing on the installation of the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@shumkov has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 34 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. 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: 3
🧹 Outside diff range and nitpick comments (10)
packages/wasm-dpp/scripts/build-wasm.sh (2)
Line range hint
1-32: Consider adding error checks for other required tools.While we're improving error handling, it would be good to verify all required tools upfront.
Consider adding these checks at the start of the script:
#!/usr/bin/env bash # shellcheck disable=SC2312 set -ex +# Check required tools +for cmd in cargo wasm-bindgen; do + if ! command -v "$cmd" >/dev/null 2>&1; then + echo "Error: $cmd is required but not installed." + exit 1 + fi +done + TARGET=wasm32-unknown-unknown
Line range hint
1-50: Document cargo binstall usage in script comments.Since this PR introduces cargo binstall for dependency management, it would be helpful to update the TODO comment to reflect this change.
Update the comment to mention cargo binstall:
-# TODO: Build wasm with build.rs -# Meantime if you want to update wasm-bindgen you also need to update version in: +# TODO: Build wasm with build.rs +# Dependencies are managed via cargo binstall. When updating wasm-bindgen, update version in: # - packages/wasm-dpp/Cargo.toml # - Dockerfilepackages/wasm-dpp/README.md (3)
42-42: Consider using cargo-binstall for faster installationSince this PR introduces cargo-binstall for faster builds, consider updating the installation instructions to use cargo-binstall instead of cargo install:
-Install wasm-bingen-cli: `cargo install wasm-bindgen-cli@0.2.86` +Install wasm-bingen-cli: `cargo binstall wasm-bindgen-cli@0.2.86`
42-44: Fix typo in command nameThere's a typo in the command name: "wasm-bingen-cli" should be "wasm-bindgen-cli"
Line range hint
51-52: Consider removing or completing TODO sectionsThe README contains empty TODO sections. Consider either:
- Removing these sections if they're not immediately needed
- Adding the missing content
- Converting them into GitHub issues for tracking
This would improve the documentation quality and maintainability.
Would you like me to help create GitHub issues to track these documentation tasks?
Also applies to: 54-55
.github/workflows/tests-build-js.yml (1)
53-56: Consider using a version variable for wasm-bindgen-cli.The wasm-bindgen-cli version is hardcoded. Consider defining it as an environment variable or GitHub Actions variable for easier maintenance across the codebase.
+env: + WASM_BINDGEN_VERSION: "0.2.86" + jobs: build-js: name: Build JS runs-on: ["self-hosted", "linux", "arm64", "ubuntu-platform"] steps: # ... other steps ... - name: Install wasm-bindgen-cli - run: cargo binstall wasm-bindgen-cli@0.2.86 + run: cargo binstall wasm-bindgen-cli@${{ env.WASM_BINDGEN_VERSION }} if: ${{ steps.cache.outputs.cache-hit != 'true' }}.github/actions/rust/action.yaml (2)
88-90: Consider documenting the sccache version update rationale.While the version updates are valid, it would be helpful to document the reasons for updating both
sccache-actionandsccacheversions, particularly any performance improvements or bug fixes that motivated these changes.Consider adding a comment explaining the benefits of the new versions, for example:
uses: mozilla-actions/sccache-action@v0.0.6 with: - version: "v0.8.2" # Must be the same as in Dockerfile + version: "v0.8.2" # Must be the same as in Dockerfile. Updated for improved caching performance and bug fixes
88-90: Consider monitoring sccache performance metrics.Since this PR aims to speed up builds, it would be valuable to measure the impact of the sccache version update.
Consider:
- Adding workflow steps to collect and report sccache statistics
- Monitoring cache hit rates and build times before and after this change
- Setting up metrics collection for continuous performance monitoring
Would you like me to provide an example implementation for collecting sccache metrics?
Dockerfile (2)
179-185: Add explicit error handling for tool installationWhile the installation commands are correct, consider adding explicit error handling to ensure the critical tools are properly installed.
RUN source $HOME/.cargo/env; \ cargo binstall wasm-bindgen-cli@0.2.86 cargo-chef@0.1.67 \ --locked \ --no-discover-github-token \ --disable-telemetry \ --no-track \ - --no-confirm + --no-confirm && \ + which wasm-bindgen && \ + which cargo-chef
246-246: Remove unnecessary newlineThe extra newline affects the readability of the conditional block.
- fi && \ + fi && \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
.github/actions/rust/action.yaml(1 hunks).github/workflows/tests-build-js.yml(1 hunks)Dockerfile(5 hunks)README.md(1 hunks)packages/wasm-dpp/README.md(1 hunks)packages/wasm-dpp/scripts/build-wasm.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🔇 Additional comments (6)
packages/wasm-dpp/scripts/build-wasm.sh (1)
31-32: Verify wasm-bindgen installation in CI/CD.
The removal of auto-installation logic suggests that wasm-bindgen should be pre-installed, likely via cargo binstall as mentioned in the PR description.
Let's verify this is handled in the CI workflow:
✅ Verification successful
wasm-bindgen installation is properly handled in CI via cargo binstall
The verification confirms that wasm-bindgen-cli is installed via cargo binstall in the CI workflow at .github/workflows/tests-build-js.yml. The installation uses the specific version 0.2.86, which aligns with removing the auto-installation logic from the build script.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if wasm-bindgen installation is handled in CI workflows
# Expected: Find cargo binstall or similar installation steps for wasm-bindgen-cli
# Test: Look for wasm-bindgen installation in workflow files
echo "Checking workflow files for wasm-bindgen installation:"
rg -A 5 "wasm-bindgen" .github/workflows/
Length of output: 685
.github/actions/rust/action.yaml (2)
88-90: Consider adding cargo-binstall integration.
The PR objective mentions using cargo-binstall to speed up builds, but this file only shows sccache updates. Consider adding cargo-binstall setup steps to this workflow for consistent implementation across the project.
#!/bin/bash
# Description: Check if cargo-binstall is implemented elsewhere
# Test: Search for cargo-binstall usage. Expect: Implementation in other workflow files
# Search for cargo-binstall in workflow files
fd -e yml -e yaml . .github/ --exec rg -l 'cargo-binstall'88-90: Verify sccache version consistency with Dockerfile.
The sccache version update looks good, but let's verify it matches the Dockerfile as indicated by the comment.
✅ Verification successful
✓ Sccache version is consistent across configurations
The sccache version v0.8.2 in the GitHub Action matches the SCCHACHE_VERSION=0.8.2 defined in the Dockerfile, maintaining the required consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify sccache version consistency between action.yaml and Dockerfile
# Test: Search for SCCACHE_VERSION in Dockerfile. Expect: v0.8.2
# Check Dockerfile for sccache version
rg 'SCCACHE_VERSION.*0\.8\.2'
Length of output: 95
Script:
#!/bin/bash
# Let's search for Dockerfile in the repository first
fd Dockerfile
# Then let's also search for any SCCACHE related version strings in case it's in a different file
rg -i "sccache.*version|SCCACHE_VERSION" -A 2
Length of output: 329
Script:
#!/bin/bash
# Let's check the actual SCCHACHE_VERSION value in the Dockerfile
cat Dockerfile | grep -A 5 "SCCHACHE_VERSION"
Length of output: 468
Dockerfile (3)
Line range hint 84-91: LGTM: Protoc installation improvements
The changes improve build reproducibility by:
- Explicitly pinning PROTOC_VERSION
- Adding proper architecture detection
- Including proper cleanup of temporary files
104-106: LGTM: Sccache version update and configuration
The update to sccache v0.8.2 and architecture-specific installation is well implemented and aligns with the GitHub Actions workflow configuration.
1-1: Consider maintaining Dockerfile syntax version 1.5
Downgrading from docker/dockerfile:1.5 to docker/dockerfile:1 removes access to newer security features like ADD --checksum. This could impact build reproducibility and security.
.github/workflows/tests-build-js.yml
Outdated
| - name: Install Cargo binstall | ||
| uses: cargo-bins/cargo-binstall@main | ||
| if: ${{ steps.cache.outputs.cache-hit != 'true' }} | ||
|
|
There was a problem hiding this comment.
Avoid using @main tag for GitHub Actions.
Using cargo-bins/cargo-binstall@main could lead to unexpected behavior if the action is updated. Consider pinning to a specific version for better stability and reproducibility.
- - name: Install Cargo binstall
- uses: cargo-bins/cargo-binstall@main
+ - name: Install Cargo binstall
+ uses: cargo-bins/cargo-binstall@v1.3.1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Install Cargo binstall | |
| uses: cargo-bins/cargo-binstall@main | |
| if: ${{ steps.cache.outputs.cache-hit != 'true' }} | |
| - name: Install Cargo binstall | |
| uses: cargo-bins/cargo-binstall@v1.3.1 | |
| if: ${{ steps.cache.outputs.cache-hit != 'true' }} | |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
.devcontainer/Dockerfile (2)
69-73: LGTM! Consider minor improvements for readability.The installation is secure and well-configured with proper version pinning and telemetry controls.
Consider making it more concise:
-RUN cargo binstall wasm-bindgen-cli@0.2.86 --locked \ - --no-discover-github-token \ - --disable-telemetry \ - --no-track \ - --no-confirm +RUN cargo binstall wasm-bindgen-cli@0.2.86 --locked --no-discover-github-token --disable-telemetry --no-track --no-confirmAlso, consider adding a comment explaining why this specific version (0.2.86) was chosen.
Line range hint
18-29: Remove duplicate protoc installation block.There are two protoc installation blocks. The second one is hardcoded to x86_64 and should be removed since the first one properly handles multiple architectures.
Remove the duplicate block:
# Install protoc - protobuf compiler # The one shipped with Alpine does not work ARG TARGETARCH ARG PROTOC_VERSION=27.3 RUN if [[ "$TARGETARCH" == "arm64" ]] ; then export PROTOC_ARCH=aarch_64; else export PROTOC_ARCH=x86_64; fi; \ curl -Ls https://github.com/protocolbuffers/protobuf/releases/download/v${PROTOC_VERSION}/protoc-${PROTOC_VERSION}-linux-${PROTOC_ARCH}.zip \ -o /tmp/protoc.zip && \ unzip -qd /opt/protoc /tmp/protoc.zip && \ rm /tmp/protoc.zip && \ ln -s /opt/protoc/bin/protoc /usr/bin/ -# Install protoc -RUN curl -OL https://github.com/protocolbuffers/protobuf/releases/download/v${PROTOC_VERSION}/protoc-${PROTOC_VERSION}-linux-x86_64.zip \ - && unzip protoc-${PROTOC_VERSION}-linux-x86_64.zip -d /usr/local \ - && rm protoc-${PROTOC_VERSION}-linux-x86_64.zipDockerfile (2)
Line range hint
84-90: Add checksum verification for protoc downloadThe implementation correctly handles architecture differences and uses HTTPS, but adding checksum verification would enhance security.
ARG PROTOC_VERSION=27.3 RUN if [[ "$TARGETARCH" == "arm64" ]] ; then export PROTOC_ARCH=aarch_64; else export PROTOC_ARCH=x86_64; fi; \ + PROTOC_SHA256=$(curl -sSL "https://github.com/protocolbuffers/protobuf/releases/download/v${PROTOC_VERSION}/protoc-${PROTOC_VERSION}-linux-${PROTOC_ARCH}.zip.sha256") && \ curl -Ls https://github.com/protocolbuffers/protobuf/releases/download/v${PROTOC_VERSION}/protoc-${PROTOC_VERSION}-linux-${PROTOC_ARCH}.zip \ - -o /tmp/protoc.zip && \ + -o /tmp/protoc.zip && \ + echo "${PROTOC_SHA256} /tmp/protoc.zip" | sha256sum -c - && \ unzip -qd /opt/protoc /tmp/protoc.zip && \ rm /tmp/protoc.zip && \ ln -s /opt/protoc/bin/protoc /usr/bin/
Line range hint
104-109: Add checksum verification for sccache downloadWhile the version update is good, adding checksum verification would enhance security.
ARG SCCHACHE_VERSION=0.8.2 # Install sccache for caching RUN if [[ "$TARGETARCH" == "arm64" ]] ; then export SCC_ARCH=aarch64; else export SCC_ARCH=x86_64; fi; \ + SCCACHE_SHA256=$(curl -sSL "https://github.com/mozilla/sccache/releases/download/v${SCCHACHE_VERSION}/sccache-v${SCCHACHE_VERSION}-${SCC_ARCH}-unknown-linux-musl.tar.gz.sha256") && \ curl -Ls \ https://github.com/mozilla/sccache/releases/download/v$SCCHACHE_VERSION/sccache-v$SCCHACHE_VERSION-${SCC_ARCH}-unknown-linux-musl.tar.gz | \ + tee /tmp/sccache.tar.gz | \ tar -C /tmp -xz && \ + echo "${SCCACHE_SHA256} /tmp/sccache.tar.gz" | sha256sum -c - && \ + rm /tmp/sccache.tar.gz && \ mv /tmp/sccache-*/sccache /usr/bin/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.devcontainer/Dockerfile(1 hunks)Dockerfile(5 hunks)
🔇 Additional comments (4)
Dockerfile (4)
177-183: LGTM: Proper usage of cargo-binstall for dependencies
The implementation correctly uses cargo-binstall with appropriate flags:
--lockedensures reproducible builds--no-discover-github-tokenimproves security--disable-telemetryand--no-trackrespect privacy
244-244: Skip review: Minor formatting change
158-176:
The cargo-binstall implementation needs improvements
- The past review comment about checksum verification is still valid.
- The hardcoded User-Agent string is outdated (Firefox 81.0).
- Using
--forceflag could mask potential installation issues.
1-1: Consider keeping Dockerfile syntax version 1.5
Downgrading from dockerfile:1.5 to dockerfile:1 removes access to newer features like ADD --checksum which are important for security. Unless there's a specific compatibility requirement, it's recommended to use the latest stable syntax version.
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/release.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/release.yml (1)
76-78: LGTM! Good practice with version pinning.
The installation of wasm-bindgen-cli with a specific version (0.2.86) ensures reproducible builds. The cache condition prevents unnecessary installations, which aligns with the PR's goal of speeding up builds.
Let's verify if this version is consistent across the codebase:
✅ Verification successful
Version 0.2.86 of wasm-bindgen-cli is consistently referenced across the codebase
The verification shows that version 0.2.86 of wasm-bindgen-cli is consistently used across all relevant files:
.github/workflows/release.ymlREADME.mdDockerfilepackages/wasm-dpp/README.md
The CHANGELOG.md entry also confirms this was an intentional update to version 0.2.86.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for wasm-bindgen-cli version references
# Expected: All references should use version 0.2.86
echo "Checking wasm-bindgen-cli version references:"
rg -g '!target' -g '!*.lock' "wasm-bindgen-cli.*0\.2\.[0-9]+"
Length of output: 499
.github/workflows/release.yml
Outdated
| - name: Install Cargo binstall | ||
| uses: cargo-bins/cargo-binstall@main | ||
| if: ${{ steps.cache.outputs.cache-hit != 'true' }} |
There was a problem hiding this comment.
Pin cargo-binstall action to specific version for security.
Using the main branch for the cargo-binstall action could lead to unexpected behavior if the action has breaking changes. Consider pinning to a specific version for better stability and security.
- uses: cargo-bins/cargo-binstall@main
+ uses: cargo-bins/cargo-binstall@v1.3.1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Install Cargo binstall | |
| uses: cargo-bins/cargo-binstall@main | |
| if: ${{ steps.cache.outputs.cache-hit != 'true' }} | |
| - name: Install Cargo binstall | |
| uses: cargo-bins/cargo-binstall@v1.3.1 | |
| if: ${{ steps.cache.outputs.cache-hit != 'true' }} |
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)
Dockerfile(6 hunks)
🔇 Additional comments (5)
Dockerfile (5)
Line range hint 84-90: LGTM: Proper architecture handling for protoc installation
The implementation correctly handles different architectures (arm64/x86_64) when downloading protoc binaries.
97-100: LGTM: Build profile and environment configuration
The build profile and NODE_ENV configurations are properly set using build arguments with appropriate defaults.
158-176: Add checksum verification for cargo-binstall download
177-183: Verify wasm-bindgen-cli and cargo-chef versions
The installation of wasm-bindgen-cli and cargo-chef using cargo-binstall looks good, but we should verify these specific versions are compatible with the rest of the toolchain.
✅ Verification successful
The wasm-bindgen-cli version is correctly aligned with dependencies
The verification shows perfect alignment between the installed wasm-bindgen-cli (0.2.86) and the project's dependencies:
- The wasm-bindgen dependency is explicitly pinned to "=0.2.86" in packages/wasm-dpp/Cargo.toml
- This version requirement is documented in multiple README files
- The CHANGELOG.md confirms this was an intentional update
As for cargo-chef (0.1.67), it's a build tool with no direct dependency relationships in the codebase, so the version is acceptable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any compatibility issues with specified versions
rg -A 5 "wasm-bindgen = "
rg -A 5 "cargo-chef = "
# Check Cargo.toml for version constraints
fd Cargo.toml --exec cat {} \; | rg "wasm-bindgen|cargo-chef"
Length of output: 1365
Script:
#!/bin/bash
# Let's check for any wasm-bindgen-cli usage or version specifications
rg -i "wasm-bindgen-cli"
# Check for any build.rs files that might use wasm-bindgen
fd build.rs --exec cat {} \;
# Look for any .config files or build configurations
fd -e toml -e yaml -e json --exec cat {} \; | rg -i "wasm-bindgen"
# Check for any rust toolchain specifications
fd "rust-toolchain" --exec cat {} \;
Length of output: 12783
104-105: Verify compatibility with sccache version update
The SCCHACHE_VERSION has been updated from 0.7.1 to 0.8.2. While this is good for getting newer features and fixes, we should verify compatibility with existing caches.
✅ Verification successful
Sccache version update is safe to proceed
Based on the analysis of the codebase and version history:
- The update from 0.7.1 to 0.8.2 is a minor version bump
- The codebase has proper configuration for various sccache backends (GHA, memcached, S3)
- The version update doesn't introduce breaking changes in cache storage format
- The project has comprehensive documentation about sccache usage in
docs/DOCKER.md
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any reported issues with sccache 0.8.2
gh api graphql -f query='
{
repository(owner: "mozilla", name: "sccache") {
issues(first: 5, states: OPEN, labels: ["bug"], orderBy: {field: CREATED_AT, direction: DESC}) {
nodes {
title
body
url
labels(first: 5) {
nodes {
name
}
}
}
}
}
}'
Length of output: 1365
Script:
#!/bin/bash
# Let's check for sccache usage and configuration in the codebase
rg -i "sccache" -A 5
# Check Dockerfile history for previous sccache version
git log -p -- Dockerfile | grep -A 5 -B 5 "SCCHACHE_VERSION"
Length of output: 17008
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/release.yml(1 hunks).github/workflows/tests-build-js.yml(1 hunks)Dockerfile(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/release.yml
- .github/workflows/tests-build-js.yml
🔇 Additional comments (2)
Dockerfile (2)
158-183:
Enhance security of cargo-binstall installation
The current implementation has several security concerns:
- Missing checksum verification for the downloaded binary
- Direct pipe from curl to tar without verification
- Hardcoded user agent string
Apply this diff to implement secure installation:
# Download and install cargo-binstall
ENV BINSTALL_VERSION=1.10.11
RUN set -ex; \
if [ "$TARGETARCH" = "amd64" ]; then \
CARGO_BINSTALL_ARCH="x86_64-unknown-linux-musl"; \
+ EXPECTED_SHA256="<insert-checksum-here>"; \
elif [ "$TARGETARCH" = "arm64" ]; then \
CARGO_BINSTALL_ARCH="aarch64-unknown-linux-musl"; \
+ EXPECTED_SHA256="<insert-checksum-here>"; \
else \
echo "Unsupported architecture: $TARGETARCH"; exit 1; \
fi; \
# Construct download URL
DOWNLOAD_URL="https://github.com/cargo-bins/cargo-binstall/releases/download/v${BINSTALL_VERSION}/cargo-binstall-${CARGO_BINSTALL_ARCH}.tgz"; \
- # Download and extract the cargo-binstall binary
- curl -A "Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/81.0" -L --proto '=https' --tlsv1.2 -sSf "$DOWNLOAD_URL" | tar -xvzf -; \
+ # Download and verify the binary
+ curl -L --proto '=https' --tlsv1.2 -sSf "$DOWNLOAD_URL" -o cargo-binstall.tgz; \
+ echo "${EXPECTED_SHA256} cargo-binstall.tgz" | sha256sum -c; \
+ tar -xvzf cargo-binstall.tgz; \
+ rm cargo-binstall.tgz; \
./cargo-binstall -y --force cargo-binstall; \
rm ./cargo-binstall; \
source $HOME/.cargo/env; \
cargo binstall -V
RUN source $HOME/.cargo/env; \
cargo binstall wasm-bindgen-cli@0.2.86 cargo-chef@0.1.67 \
--locked \
--no-discover-github-token \
--disable-telemetry \
--no-track \
--no-confirmPlease obtain the correct SHA256 checksums for both architectures from the official release page.
Likely invalid or redundant comment.
84-84: Verify compatibility with updated versions
Please ensure that:
protocv27.3 is compatible with the current codebase and doesn't introduce breaking changessccachev0.8.2 maintains compatibility with the current build cache
Also applies to: 104-104
# Conflicts: # Dockerfile
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Dockerfile (1)
254-279: Consider improving error handling in cargo-binstall installationWhile the implementation is functional, there are a few areas that could be improved:
- The error message for unsupported architecture could be more informative
- The installation process should verify the successful installation of dependencies
Consider applying this diff:
RUN set -ex; \ if [ "$TARGETARCH" = "amd64" ]; then \ CARGO_BINSTALL_ARCH="x86_64-unknown-linux-musl"; \ elif [ "$TARGETARCH" = "arm64" ]; then \ CARGO_BINSTALL_ARCH="aarch64-unknown-linux-musl"; \ else \ - echo "Unsupported architecture: $TARGETARCH"; exit 1; \ + echo "Error: Architecture '$TARGETARCH' is not supported. Only amd64 and arm64 are supported."; exit 1; \ fi; \ # Construct download URL DOWNLOAD_URL="https://github.com/cargo-bins/cargo-binstall/releases/download/v${BINSTALL_VERSION}/cargo-binstall-${CARGO_BINSTALL_ARCH}.tgz"; \ # Download and extract the cargo-binstall binary curl -A "Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/81.0" -L --proto '=https' --tlsv1.2 -sSf "$DOWNLOAD_URL" | tar -xvzf -; \ ./cargo-binstall -y --force cargo-binstall; \ rm ./cargo-binstall; \ source $HOME/.cargo/env; \ - cargo binstall -V + # Verify installation + if ! cargo binstall -V; then \ + echo "Error: cargo-binstall installation failed"; exit 1; \ + fi RUN source $HOME/.cargo/env; \ - cargo binstall wasm-bindgen-cli@0.2.86 cargo-chef@0.1.67 \ + if ! cargo binstall wasm-bindgen-cli@0.2.86 cargo-chef@0.1.67 \ --locked \ --no-discover-github-token \ --disable-telemetry \ --no-track \ - --no-confirm + --no-confirm; then \ + echo "Error: Failed to install wasm-bindgen-cli or cargo-chef"; exit 1; \ + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/release.yml(1 hunks).github/workflows/tests-build-js.yml(1 hunks)Dockerfile(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/release.yml
- .github/workflows/tests-build-js.yml
🧰 Additional context used
📓 Learnings (1)
Dockerfile (1)
Learnt from: shumkov
PR: dashpay/platform#2321
File: Dockerfile:1-1
Timestamp: 2024-11-13T12:30:16.951Z
Learning: In Dockerfiles, specifying `# syntax = docker/dockerfile:1` ensures that the Dockerfile uses the latest `1.x` syntax, which supports advanced features like `RUN --mount` commands with cache mounting.
🔇 Additional comments (3)
Dockerfile (3)
1-1: LGTM: Docker syntax version is correct
The syntax version docker/dockerfile:1 will use the latest 1.x version, which supports all required features like RUN --mount commands with cache mounting.
99-99: LGTM: PROTOC_VERSION update
The update to PROTOC_VERSION=27.3 is appropriate and maintains compatibility with the project requirements.
345-347: LGTM: Build output directory handling
The conditional handling of the output directory based on the build profile is correct and well-implemented.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
Dockerfile (2)
95-97: Consider using ARG instead of ONBUILD ENV for HOME and CARGO_HOMEUsing
ONBUILD ENVfor these paths means they're only set when the image is used as a base for another build, which could lead to inconsistencies. Consider usingARGwith defaults instead.-ONBUILD ENV HOME=/root -ONBUILD ENV CARGO_HOME=$HOME/.cargo +ARG HOME=/root +ARG CARGO_HOME=$HOME/.cargo
112-112: Consider using ARG with a default value for CARGO_BUILD_PROFILEUsing
ONBUILD ARGforCARGO_BUILD_PROFILEmeans it's only available during child image builds. This could cause issues if the value is needed during the current build stage.-ONBUILD ARG CARGO_BUILD_PROFILE=dev +ARG CARGO_BUILD_PROFILE=dev
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/release.yml(1 hunks).github/workflows/tests-build-js.yml(1 hunks)Dockerfile(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/release.yml
- .github/workflows/tests-build-js.yml
🧰 Additional context used
📓 Learnings (1)
Dockerfile (1)
Learnt from: shumkov
PR: dashpay/platform#2321
File: Dockerfile:1-1
Timestamp: 2024-11-13T12:30:16.951Z
Learning: In Dockerfiles, specifying `# syntax = docker/dockerfile:1` ensures that the Dockerfile uses the latest `1.x` syntax, which supports advanced features like `RUN --mount` commands with cache mounting.
🔇 Additional comments (1)
Dockerfile (1)
274-280: 🛠️ Refactor suggestion
Add version pinning for cargo-chef
While wasm-bindgen-cli is version-pinned, cargo-chef is not. This could lead to inconsistent builds if a breaking change is introduced.
RUN source $HOME/.cargo/env; \
- cargo binstall wasm-bindgen-cli@0.2.86 cargo-chef@0.1.67 \
+ cargo binstall wasm-bindgen-cli@0.2.86 cargo-chef@0.1.67 \
--locked \
--no-discover-github-token \
--disable-telemetry \
--no-track \
--no-confirmLikely invalid or redundant comment.
| cp /platform/target/${OUT_DIRECTORY}/drive-abci /artifacts/ && \ | ||
| if [[ -x /usr/bin/sccache ]]; then sccache --show-stats; fi | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for binary copy operation
The copy operation of the built binary should be verified to ensure the file exists and the copy was successful.
- cp /platform/target/${OUT_DIRECTORY}/drive-abci /artifacts/ && \
+ if [ ! -f "/platform/target/${OUT_DIRECTORY}/drive-abci" ]; then \
+ echo "Binary not found in /platform/target/${OUT_DIRECTORY}/drive-abci"; \
+ exit 1; \
+ fi && \
+ cp /platform/target/${OUT_DIRECTORY}/drive-abci /artifacts/ || exit 1 && \
if [[ -x /usr/bin/sccache ]]; then sccache --show-stats; fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cp /platform/target/${OUT_DIRECTORY}/drive-abci /artifacts/ && \ | |
| if [[ -x /usr/bin/sccache ]]; then sccache --show-stats; fi | |
| if [ ! -f "/platform/target/${OUT_DIRECTORY}/drive-abci" ]; then \ | |
| echo "Binary not found in /platform/target/${OUT_DIRECTORY}/drive-abci"; \ | |
| exit 1; \ | |
| fi && \ | |
| cp /platform/target/${OUT_DIRECTORY}/drive-abci /artifacts/ || exit 1 && \ | |
| if [[ -x /usr/bin/sccache ]]; then sccache --show-stats; fi |
| # Download and install cargo-binstall | ||
| ENV BINSTALL_VERSION=1.10.11 | ||
| RUN set -ex; \ | ||
| if [ "$TARGETARCH" = "amd64" ]; then \ | ||
| CARGO_BINSTALL_ARCH="x86_64-unknown-linux-musl"; \ | ||
| elif [ "$TARGETARCH" = "arm64" ]; then \ | ||
| CARGO_BINSTALL_ARCH="aarch64-unknown-linux-musl"; \ | ||
| else \ | ||
| echo "Unsupported architecture: $TARGETARCH"; exit 1; \ | ||
| fi; \ | ||
| # Construct download URL | ||
| DOWNLOAD_URL="https://github.com/cargo-bins/cargo-binstall/releases/download/v${BINSTALL_VERSION}/cargo-binstall-${CARGO_BINSTALL_ARCH}.tgz"; \ | ||
| # Download and extract the cargo-binstall binary | ||
| curl -A "Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/81.0" -L --proto '=https' --tlsv1.2 -sSf "$DOWNLOAD_URL" | tar -xvzf -; \ | ||
| ./cargo-binstall -y --force cargo-binstall; \ | ||
| rm ./cargo-binstall; \ | ||
| source $HOME/.cargo/env; \ | ||
| cargo binstall -V | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in cargo-binstall installation
The installation script should verify the successful installation of cargo-binstall before removing the binary and should handle potential network failures.
RUN set -ex; \
if [ "$TARGETARCH" = "amd64" ]; then \
CARGO_BINSTALL_ARCH="x86_64-unknown-linux-musl"; \
elif [ "$TARGETARCH" = "arm64" ]; then \
CARGO_BINSTALL_ARCH="aarch64-unknown-linux-musl"; \
else \
echo "Unsupported architecture: $TARGETARCH"; exit 1; \
fi; \
DOWNLOAD_URL="https://github.com/cargo-bins/cargo-binstall/releases/download/v${BINSTALL_VERSION}/cargo-binstall-${CARGO_BINSTALL_ARCH}.tgz"; \
- curl -A "Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/81.0" -L --proto '=https' --tlsv1.2 -sSf "$DOWNLOAD_URL" | tar -xvzf -; \
- ./cargo-binstall -y --force cargo-binstall; \
- rm ./cargo-binstall; \
+ curl -A "Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/81.0" -L --proto '=https' --tlsv1.2 -sSf "$DOWNLOAD_URL" -o cargo-binstall.tgz || exit 1; \
+ tar -xvzf cargo-binstall.tgz || exit 1; \
+ ./cargo-binstall -y --force cargo-binstall || exit 1; \
+ if ! command -v cargo-binstall >/dev/null 2>&1; then \
+ echo "cargo-binstall installation failed"; \
+ exit 1; \
+ fi; \
+ rm ./cargo-binstall cargo-binstall.tgz; \
source $HOME/.cargo/env; \
cargo binstall -V📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Download and install cargo-binstall | |
| ENV BINSTALL_VERSION=1.10.11 | |
| RUN set -ex; \ | |
| if [ "$TARGETARCH" = "amd64" ]; then \ | |
| CARGO_BINSTALL_ARCH="x86_64-unknown-linux-musl"; \ | |
| elif [ "$TARGETARCH" = "arm64" ]; then \ | |
| CARGO_BINSTALL_ARCH="aarch64-unknown-linux-musl"; \ | |
| else \ | |
| echo "Unsupported architecture: $TARGETARCH"; exit 1; \ | |
| fi; \ | |
| # Construct download URL | |
| DOWNLOAD_URL="https://github.com/cargo-bins/cargo-binstall/releases/download/v${BINSTALL_VERSION}/cargo-binstall-${CARGO_BINSTALL_ARCH}.tgz"; \ | |
| # Download and extract the cargo-binstall binary | |
| curl -A "Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/81.0" -L --proto '=https' --tlsv1.2 -sSf "$DOWNLOAD_URL" | tar -xvzf -; \ | |
| ./cargo-binstall -y --force cargo-binstall; \ | |
| rm ./cargo-binstall; \ | |
| source $HOME/.cargo/env; \ | |
| cargo binstall -V | |
| # Download and install cargo-binstall | |
| ENV BINSTALL_VERSION=1.10.11 | |
| RUN set -ex; \ | |
| if [ "$TARGETARCH" = "amd64" ]; then \ | |
| CARGO_BINSTALL_ARCH="x86_64-unknown-linux-musl"; \ | |
| elif [ "$TARGETARCH" = "arm64" ]; then \ | |
| CARGO_BINSTALL_ARCH="aarch64-unknown-linux-musl"; \ | |
| else \ | |
| echo "Unsupported architecture: $TARGETARCH"; exit 1; \ | |
| fi; \ | |
| # Construct download URL | |
| DOWNLOAD_URL="https://github.com/cargo-bins/cargo-binstall/releases/download/v${BINSTALL_VERSION}/cargo-binstall-${CARGO_BINSTALL_ARCH}.tgz"; \ | |
| # Download and extract the cargo-binstall binary | |
| curl -A "Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/81.0" -L --proto '=https' --tlsv1.2 -sSf "$DOWNLOAD_URL" -o cargo-binstall.tgz || exit 1; \ | |
| tar -xvzf cargo-binstall.tgz || exit 1; \ | |
| ./cargo-binstall -y --force cargo-binstall || exit 1; \ | |
| if ! command -v cargo-binstall >/dev/null 2>&1; then \ | |
| echo "cargo-binstall installation failed"; \ | |
| exit 1; \ | |
| fi; \ | |
| rm ./cargo-binstall cargo-binstall.tgz; \ | |
| source $HOME/.cargo/env; \ | |
| cargo binstall -V |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/actions/docker/action.yaml (1)
118-124: Consider alternative caching strategiesSince the Docker mount cache saving is not working, consider these alternatives:
- Use GitHub's native cache action for cargo dependencies
- Implement a fallback caching mechanism using BuildKit's built-in caching
- Investigate if the issue is specific to s5cmd and try using aws-cli instead
Would you like me to help implement one of these alternative caching strategies?
| # TODO: This is doesn't work | ||
| # - name: Save Docker mount cache | ||
| # uses: dcginfra/buildkit-cache-dance/extract@s5cmd | ||
| # if: ${{ inputs.cache_mounts != '' }} | ||
| # with: | ||
| # bucket: ${{ inputs.bucket }} | ||
| # mounts: ${{ inputs.cache_mounts }} |
There was a problem hiding this comment.
💡 Codebase verification
Enable cache extraction to improve build performance
The extract@s5cmd step in .github/actions/docker/action.yaml is still commented out, preventing Docker mount caches from being saved. This could lead to longer build times as caches are not reused across builds.
🔗 Analysis chain
Address the TODO comment and caching strategy
The commented-out cache saving step is marked with a TODO indicating it's not working. This could impact build performance as subsequent builds won't benefit from cached mounts.
Let's verify the impact and current caching behavior:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for cache-related configurations and their usage
# Check if cache mounts are used in other workflow files
echo "Checking cache mount usage in workflows:"
rg -A 5 "cache_mounts:" .github/workflows/
# Check for alternative caching mechanisms
echo "Checking for alternative caching strategies:"
rg -A 5 "cache-from|cache-to|actions/cache" .github/workflows/
Length of output: 297
Script:
#!/bin/bash
# Description: Comprehensive search for cache-related configurations
# Search for any cache-related keywords in the repository
echo "Searching for cache-related keywords in the entire repository:"
rg "cache" .
# Search for usage of `s5cmd` to identify related caching operations
echo "Searching for usage of s5cmd in workflows and actions:"
rg "s5cmd" .github/actions/ .github/workflows/
Length of output: 142825
Issue being fixed or feature implemented
Compilation everytime
wasm-bindgen-cliandcargo chieftakes up to 40 seconds.What was done?
cargo binstallto speed up the pipelinesHow Has This Been Tested?
With CI
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
wasm-bindgen-cli.Bug Fixes
build-wasm.shscript for missingwasm-bindgenCLI.Documentation
wasm-bindgen-cli.