fix: avoid simd not exist error when make localnet-build-env #23164
Conversation
|
Warning Rate limit exceeded@mmsqe has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 42 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. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request modifies multiple Dockerfiles to update the binary being copied from the build environment. Specifically, the COPY instruction has been changed to copy Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
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
Documentation and Community
|
![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
🔭 Outside diff range comments (1)
Dockerfile (1)
Line range hint
1-13: Update documentation to reflect binary name changeThe comments at the top of the Dockerfile still reference
simdin the usage examples, but the binary being copied is nowsimdv2. This inconsistency should be addressed.Apply this diff to update the documentation:
# > docker run -it -p 26657:26657 -p 26656:26656 -v ~/.simapp:/root/.simapp simapp simd init test-chain -# TODO: need to set validator in genesis so start runs -# > docker run -it -p 26657:26657 -p 26656:26656 -v ~/.simapp:/root/.simapp simapp simd start +# TODO: need to set validator in genesis so start runs (using simdv2 binary) +# > docker run -it -p 26657:26657 -p 26656:26656 -v ~/.simapp:/root/.simapp simapp simd start
🧹 Nitpick comments (1)
Dockerfile (1)
71-71: Consider using ARG/ENV for binary nameTo improve maintainability and reduce the risk of inconsistencies, consider parameterizing the binary name.
Apply this diff to make the binary name configurable:
+# Define binary name as build argument with default value +ARG BINARY_NAME=simdv2 + -COPY --from=build-env /go/src/github.com/cosmos/cosmos-sdk/build/simdv2 /usr/bin/simd +COPY --from=build-env /go/src/github.com/cosmos/cosmos-sdk/build/${BINARY_NAME} /usr/bin/simd
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(1 hunks)
🔇 Additional comments (1)
Dockerfile (1)
71-71: Verify the build output binary nameThe change from
simdtosimdv2addresses the SIMD not exist error, but we should verify that the build process actually produces this binary name.✅ Verification successful
Binary name change is correct and consistent with build configuration
The change from
simdtosimdv2in the Dockerfile is verified to be correct based on the following evidence:
- The build configuration in
scripts/build/build.mkexplicitly setsAPPNAME = simdv2when thev2build option is enabled:ifeq (v2,$(findstring v2,$(COSMOS_BUILD_OPTIONS))) SIMAPP = simapp/v2 APPNAME = simdv2 endif
- The build options include
v2by default:COSMOS_BUILD_OPTIONS += ' v2'
- The binary is correctly copied to
/usr/bin/simdin the Dockerfile, maintaining backward compatibility while using the v2 build output.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the binary name in the build process # Check if Makefile or build configurations specify the binary name # Search for binary name in build configurations rg -A 5 'simdv2' # Check Makefile for build targets and binary names fd -t f "Makefile" --exec cat {} \; | grep -A 5 "build:"Length of output: 8002
Script:
#!/bin/bash # Let's verify the build configuration and binary name in more detail # Check the build configuration in scripts/build/build.mk cat scripts/build/build.mk # Check if there are any make targets or configurations that set the binary name rg -A 5 "APPNAME.*=.*simd" # Check the Dockerfile build stage to understand the binary build process cat DockerfileLength of output: 7908
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
contrib/images/simd-dlv/Dockerfile (1)
Line range hint
1-1: Fix invalid Go version in base imageThe base image
golang:1.23-alpinedoesn't exist as Go 1.23 hasn't been released yet. The latest stable version is Go 1.21.-FROM golang:1.23-alpine AS build +FROM golang:1.21-alpine AS buildcontrib/images/simd-env/Dockerfile (1)
Line range hint
1-1: Fix invalid Go version in base imageThe base image
golang:1.23-alpinedoesn't exist as Go 1.23 hasn't been released yet. The latest stable version is Go 1.21.-FROM golang:1.23-alpine AS build +FROM golang:1.21-alpine AS build
🧹 Nitpick comments (1)
contrib/images/simd-env/Dockerfile (1)
Line range hint
4-24: Consider optimizing module copying stepsThe current approach copies each module's go.mod and go.sum files individually. This could be optimized to reduce the number of layers and improve build time.
-COPY go.mod go.sum /work/ -COPY math/go.mod math/go.sum /work/math/ -COPY api/go.mod api/go.sum /work/api/ -# ... (other module copies) +COPY go.* /work/ +COPY */go.* /work/ +COPY */*/go.* /work/
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Dockerfile(1 hunks)contrib/images/simd-dlv/Dockerfile(1 hunks)contrib/images/simd-env/Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
🔇 Additional comments (3)
contrib/images/simd-dlv/Dockerfile (1)
30-30: Verify binary name handling in wrapper scriptWhile the binary name has been updated to
simdv2, we need to ensure the wrapper script at/usr/bin/wrapper.shcorrectly handles this new binary name.✅ Verification successful
The wrapper script correctly handles the binary name change
The wrapper script is designed to be flexible with binary names through the
BINARYenvironment variable. Specifically:
- It uses
BINARY=/simd/${BINARY:-simd}which means it will use the environment variable if set, or default tosimd- It includes a helpful error message that explicitly mentions using the
BINARYenvironment variable for custom binary names- The script will work with
simdv2when the environment variable is set accordingly🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if wrapper.sh references the binary name cat contrib/images/simd-dlv/wrapper.sh | grep -A 5 "simd"Length of output: 782
contrib/images/simd-env/Dockerfile (2)
48-48: Verify binary name handling in wrapper scriptWhile the binary name has been updated to
simdv2, we need to ensure the wrapper script at/usr/bin/wrapper.shcorrectly handles this new binary name.✅ Verification successful
Binary name handling in wrapper script is flexible and compatible
The wrapper script is designed to handle different binary names through the
BINARYenvironment variable. The script:
- Uses a default path of
/simd/simdif no specific binary name is provided- Allows overriding the binary name through the
BINARYenvironment variable- Includes clear error messages if the specified binary is not found
This means the script will work correctly with the new
simdv2binary when:
- The
BINARYenvironment variable is set tosimdv2, or- The binary is copied to
/simd/simdinstead of/simd/simdv2🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if wrapper.sh references the binary name cat contrib/images/simd-env/wrapper.sh | grep -A 5 "simd"Length of output: 553
48-48: Verify binary name consistency across codebaseLet's ensure the binary name change is consistent across the entire codebase to prevent any mismatches.
✅ Verification successful
Binary name change is consistent across codebase
Based on the search results, the binary name change from
simdtosimdv2is consistently reflected across the codebase, particularly in:
- Main Dockerfile:
COPY --from=build-env /go/src/github.com/cosmos/cosmos-sdk/build/simdv2 /usr/bin/simd- simd-env Dockerfile:
COPY --from=build /work/build/simdv2 /simd/- simd-dlv Dockerfile:
COPY --from=build /work/build/simdv2 /simd/- Build scripts and configurations reference
simdv2appropriately- Documentation and test files have been updated to reference
simdv2where neededThe change appears to be part of a v2 upgrade where the binary is built as
simdv2but still installed assimdin the final container images for backward compatibility.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to both old and new binary names echo "Searching for 'simd' references:" rg --type make --type dockerfile 'simd[^v]' echo -e "\nSearching for 'simdv2' references:" rg --type make --type dockerfile 'simdv2'Length of output: 329
Script:
#!/bin/bash # Search for references to both old and new binary names without file type restrictions echo "Searching for 'simd' references:" rg 'simd[^v]' echo -e "\nSearching for 'simdv2' references:" rg 'simdv2' # Also check Dockerfile specifically echo -e "\nChecking Dockerfiles:" fd Dockerfile --exec cat {}Length of output: 64910
Description
Closes: #23163
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit