release: test#23
Conversation
WalkthroughThe pull request introduces a comprehensive restructuring of the project's directory and build configurations. The primary focus is on moving shared libraries and bindings from Changes
Poem
🪧 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: 8
🧹 Nitpick comments (4)
builders/guest/build_gnu_x86_64.sh (1)
4-5: Consider using a version-agnostic environment variable name.The environment variable
LLVM_SYS_180_PREFIXis version-specific. This might require updates when LLVM versions change.Consider using a version-agnostic name or making the version configurable:
-export LLVM_SYS_180_PREFIX=$prefix +export LLVM_VERSION=180 +export LLVM_SYS_${LLVM_VERSION}_PREFIX=$prefixbenchmark/Makefile (1)
1-2: Consider using semantic versioning for Docker imagesThe current version tag
0001is not very descriptive. Consider using semantic versioning (e.g.,v1.0.0) for better version tracking.-BENCHMARK_PREFIX := 0xeyrie/benchmark:0001 +BENCHMARK_PREFIX := 0xeyrie/benchmark:v1.0.0🧰 Tools
🪛 GitHub Actions: Build rust librevmapi
[error] 85: release-build-linux target failed with exit code 2
Makefile (2)
29-29: Consider removing --allow-dirty flag.The
--allow-dirtyflag incargo fixmight mask potential issues by allowing fixes on uncommitted changes.-cargo fix --allow-dirty --allow-staged +cargo fix🧰 Tools
🪛 GitHub Actions: Build rust librevmapi
[error] 85: release-build-linux target failed with exit code 2
72-73: Ensure consistent cleanup in clean target.The clean target should handle both old and new paths to prevent stale artifacts:
clean: cargo clean - @-rm core/vm/bindings.h - @-rm librevm/bindings.h - @-rm core/vm/$(SHARED_LIB_DST) + @-rm -f core/vm/bindings.h core/revm/bindings.h librevm/bindings.h + @-rm -f core/vm/$(SHARED_LIB_DST) core/revm/$(SHARED_LIB_DST) @echo cleaned.Also applies to: 78-80
🧰 Tools
🪛 GitHub Actions: Build rust librevmapi
[error] 85: release-build-linux target failed with exit code 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
core/vm/librevmapi.dylibis excluded by!**/*.dylib
📒 Files selected for processing (15)
.gitattributes(1 hunks).github/workflows/build.yml(3 hunks).github/workflows/release.yml(2 hunks).github/workflows/setup.yml(0 hunks)Makefile(6 hunks)benchmark/Makefile(1 hunks)builders/Dockerfile.cross(1 hunks)builders/Dockerfile.debian(2 hunks)builders/Makefile(1 hunks)builders/guest/build_gnu_aarch64.sh(1 hunks)builders/guest/build_gnu_x86_64.sh(1 hunks)builders/guest/build_macos.sh(1 hunks)core/vm/bindings.h(1 hunks)core/vm/vm.go(2 hunks)scripts/contractsgen.sh(0 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/setup.yml
- scripts/contractsgen.sh
✅ Files skipped from review due to trivial changes (2)
- .gitattributes
- builders/Makefile
🧰 Additional context used
🪛 GitHub Actions: Build rust librevmapi
builders/guest/build_macos.sh
[error] 6-6: Command 'llvm-config' not found. Required dependency LLVM is missing in the build environment.
benchmark/Makefile
[error] 85: release-build-linux target failed with exit code 2
Makefile
[error] 85: release-build-linux target failed with exit code 2
🔇 Additional comments (7)
core/vm/vm.go (1)
66-66: LGTM! Function renaming improves code clarity.The renaming of
unmarshalEvmResulttounmarshalResultis consistent across all usages and maintains the same functionality.Also applies to: 71-71, 94-94, 99-99, 102-103
builders/guest/build_gnu_aarch64.sh (2)
4-5: Consider using a version-agnostic environment variable name.Similar to the x86_64 build script, consider making the LLVM version configurable.
8-8: LGTM! Path update aligns with new directory structure.The DYLD_LIBRARY_PATH update correctly reflects the move of shared libraries to core/vm.
builders/Dockerfile.cross (3)
7-7: LGTM: Build dependencies look appropriate.The additional packages (lsb-release, wget, software-properties-common) are necessary prerequisites for the LLVM installation.
17-17: LGTM: Appropriate WORKDIR placement.Moving WORKDIR before osxcross operations ensures correct working directory context.
9-12: Verify LLVM installation script security.Downloading and executing a script directly from apt.llvm.org poses potential security risks. Consider:
- Validating the script's checksum
- Pinning to a specific script version
- Using HTTPS for the download
✅ Verification successful
The LLVM installation approach is secure and follows best practices
The current implementation is secure because:
- The script is downloaded over HTTPS from the official LLVM repository
- Version 18 is explicitly pinned in the Dockerfile
- The script includes GPG key verification for package signatures
- Built-in validation checks ensure system compatibility and dependency verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify LLVM script source and contents curl -s https://apt.llvm.org/llvm.sh | sha256sum wget -qO- https://apt.llvm.org/llvm.sh | grep -i "version"Length of output: 304
Script:
#!/bin/bash # Fetch and display LLVM installation script contents curl -s https://apt.llvm.org/llvm.shLength of output: 5849
Makefile (1)
107-107: LGTM: Protobuf generation script.The protobuf generation command looks correct.
🧰 Tools
🪛 GitHub Actions: Build rust librevmapi
[error] 85: release-build-linux target failed with exit code 2
| .PHONY: profiling | ||
| profiling: | ||
| @echo "Running Profiling..." | ||
| @export BENCHMARK_PREFIX=$(BENCHMARK_PREFIX) && \ | ||
| export REV_CONTAINER_NAME=revmffi && \ | ||
| export GEV_CONTAINER_NAME=gevm && \ | ||
| docker-compose -f ./docker-compose.yml up -d | ||
| docker exec -it revmffi sh -c "cd /app/revmffi && go test -v --count 1000" | ||
| docker exec -it gevm sh -c "cd /app/gevm && go test -v --count 1000" No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve profiling target robustness
The profiling target has several potential issues:
- Hardcoded container names could cause conflicts in parallel runs
- No cleanup after profiling
- No error handling for container startup
Consider this improved version:
.PHONY: profiling
profiling:
@echo "Running Profiling..."
+ @export TIMESTAMP=$$(date +%s) && \
@export BENCHMARK_PREFIX=$(BENCHMARK_PREFIX) && \
- export REV_CONTAINER_NAME=revmffi && \
- export GEV_CONTAINER_NAME=gevm && \
+ export REV_CONTAINER_NAME=revmffi-$$TIMESTAMP && \
+ export GEV_CONTAINER_NAME=gevm-$$TIMESTAMP && \
docker-compose -f ./docker-compose.yml up -d
+ @echo "Waiting for containers to be ready..."
+ @sleep 5
docker exec -it revmffi sh -c "cd /app/revmffi && go test -v --count 1000"
docker exec -it gevm sh -c "cd /app/gevm && go test -v --count 1000"
+ @echo "Cleaning up containers..."
+ @docker-compose -f ./docker-compose.yml downCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Build rust librevmapi
[error] 85: release-build-linux target failed with exit code 2
| with: | ||
| name: librevmapi-linux-glibc | ||
| path: core/*.so | ||
| path: core/vm/*.so |
There was a problem hiding this comment.
Fix path mismatch in commit step
The artifact upload steps use core/vm/* but the commit step still uses old paths in the core directory.
Update the paths in the commit step to match the new directory structure:
- name: Commit shared libraries
uses: EndBug/add-and-commit@v9
with:
- add: '["core/librevmapi.dylib", "core/librevmapi.aarch64.so", "core/librevmapi.x86_64.so" ]'
+ add: '["core/vm/librevmapi.dylib", "core/vm/librevmapi.aarch64.so", "core/vm/librevmapi.x86_64.so" ]'
default_author: github_actions
message: "update shared libraries"Also applies to: 48-48, 76-76
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/build.yml (1)
34-34: Fix formatting issues in commit stepThe path updates correctly reflect the new directory structure. However, there are formatting issues in line 75:
- Extra spaces after colon
- Trailing spaces
Apply this diff to fix the formatting:
- add: '["core/vm/librevmapi.dylib", "core/vm/librevmapi.aarch64.so", "core/vm/librevmapi.x86_64.so" ]' + add: '["core/vm/librevmapi.dylib", "core/vm/librevmapi.aarch64.so", "core/vm/librevmapi.x86_64.so"]'Also applies to: 48-48, 75-75
builders/Dockerfile.debian (1)
35-35: Remove extra blank lineThere's an unnecessary blank line that should be removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(4 hunks)Makefile(6 hunks)builders/Dockerfile.cross(1 hunks)builders/Dockerfile.debian(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- builders/Dockerfile.cross
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/build.yml
[warning] 75-75: too many spaces after colon
(colons)
[error] 75-75: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: MacOS glibc
- GitHub Check: Linux glibc
🔇 Additional comments (7)
.github/workflows/build.yml (1)
12-15: LGTM: New workflow triggers addedThe addition of
builders/*path and PR trigger forrelease/testbranch aligns with the project's restructuring.builders/Dockerfile.debian (2)
6-12: LGTM: Improved LLVM installation processThe LLVM installation process has been improved with proper repository setup and package installation.
15-17: LGTM: Added LLVM verificationGood addition of the verification step to ensure
llvm-configis properly installed and accessible.Makefile (4)
28-28: LGTM: Added --allow-dirty flagThe addition of
--allow-dirtyflag tocargo fixallows fixing issues even when there are uncommitted changes.
38-38: LGTM: Updated paths to new directory structureAll paths have been consistently updated from
core/revmtocore/vmacross the Makefile.Also applies to: 63-63, 71-72, 77-79, 86-87, 97-97
106-106: LGTM: Removed empty line in protobuf-gen targetGood cleanup of empty line in the protobuf-gen target.
5-5: Verify Docker image availabilityThe Docker image prefix has been updated from
rethmintto0xeyrie. Let's verify the image availability.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
builders/guest/build_macos.sh (1)
6-9:⚠️ Potential issueAdd error handling for LLVM dependency and improve logging
The LLVM configuration still lacks proper error handling as suggested in the previous review.
- Add LLVM installation check
- Improve the log message clarity
+# Check if llvm-config is available +if ! command -v llvm-config &> /dev/null; then + echo "Error: llvm-config not found. Please install LLVM first." + exit 1 +fi + prefix=$(llvm-config --prefix) export LLVM_SYS_180_PREFIX=$prefix -echo "Prefix" -echo $LLVM_SYS_180_PREFIX +echo "LLVM prefix path: $LLVM_SYS_180_PREFIX"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
builders/guest/build_gnu_aarch64.sh(1 hunks)builders/guest/build_gnu_x86_64.sh(1 hunks)builders/guest/build_macos.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- builders/guest/build_gnu_x86_64.sh
🔇 Additional comments (1)
builders/guest/build_gnu_aarch64.sh (1)
4-9: Verify directory structure changesLet's verify that the directory restructuring from
apitocore/vmis consistent across the codebase.✅ Verification successful
Directory structure changes verified successfully
The migration from
./apito./core/vmis complete and consistent across the codebase. The new directory contains all required library files and all build scripts have been properly updated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining references to the old './api' path echo "Checking for old './api' references:" rg -l "\.\/api" # Verify the existence and contents of the new core/vm directory echo -e "\nChecking core/vm directory structure:" if [ -d "core/vm" ]; then ls -la core/vm/ else echo "Warning: core/vm directory not found" fi # Check for any mismatched library paths in build configurations echo -e "\nChecking for library paths in build configs:" rg -l "librevmapi\.(so|dylib)"Length of output: 1557
Script:
#!/bin/bash # Check for any remaining references to the old './api' path with fixed regex echo "Checking for old './api' references:" rg -l "[.]/api" # Double check the library path references echo -e "\nChecking library path references:" rg -l "DYLD_LIBRARY_PATH"Length of output: 316
| echo $LLVM_SYS_180_PREFIX | ||
| export CARGO_REGISTRIES_CRATES_IO_PROTOCOL=sparse | ||
| export DYLD_LIBRARY_PATH="./api" | ||
| export DYLD_LIBRARY_PATH="./core/vm" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate library directory existence
The script should verify that the library directory exists before setting DYLD_LIBRARY_PATH.
+# Verify library directory exists
+if [ ! -d "./core/vm" ]; then
+ echo "Error: Library directory './core/vm' not found"
+ exit 1
+fi
+
export DYLD_LIBRARY_PATH="./core/vm"📝 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.
| export DYLD_LIBRARY_PATH="./core/vm" | |
| # Verify library directory exists | |
| if [ ! -d "./core/vm" ]; then | |
| echo "Error: Library directory './core/vm' not found" | |
| exit 1 | |
| fi | |
| export DYLD_LIBRARY_PATH="./core/vm" |
|
|
||
| # No stripping implemented (see https://github.com/CosmWasm/wasmvm/issues/222#issuecomment-2260007943). | ||
| export DYLD_LIBRARY_PATH="./api" | ||
| export DYLD_LIBRARY_PATH="./core/vm" |
There was a problem hiding this comment.
Review library path configuration
There are several concerns with the library path configuration:
- Using DYLD_LIBRARY_PATH in a Linux build script (it's macOS specific)
- Using a relative path without directory verification
- No fallback mechanism if the directory doesn't exist
Consider this correction:
-export DYLD_LIBRARY_PATH="./core/vm"
+# Ensure the library directory exists
+lib_dir="$(pwd)/core/vm"
+if [ ! -d "$lib_dir" ]; then
+ echo "Error: Library directory not found: $lib_dir" >&2
+ exit 1
+fi
+
+# Set library path for both Linux and macOS
+export LD_LIBRARY_PATH="$lib_dir"
+export DYLD_LIBRARY_PATH="$lib_dir"📝 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.
| export DYLD_LIBRARY_PATH="./core/vm" | |
| # Ensure the library directory exists | |
| lib_dir="$(pwd)/core/vm" | |
| if [ ! -d "$lib_dir" ]; then | |
| echo "Error: Library directory not found: $lib_dir" >&2 | |
| exit 1 | |
| fi | |
| # Set library path for both Linux and macOS | |
| export LD_LIBRARY_PATH="$lib_dir" | |
| export DYLD_LIBRARY_PATH="$lib_dir" |
| prefix=$(llvm-config --prefix) | ||
| export LLVM_SYS_180_PREFIX=$prefix | ||
| echo $LLVM_SYS_180_PREFIX |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for LLVM configuration
The LLVM configuration lacks error handling and version verification. This could lead to silent failures if llvm-config is not available or if an incompatible LLVM version is used.
Consider adding these safety checks:
-prefix=$(llvm-config --prefix)
-export LLVM_SYS_180_PREFIX=$prefix
-echo $LLVM_SYS_180_PREFIX
+if ! command -v llvm-config &> /dev/null; then
+ echo "Error: llvm-config not found" >&2
+ exit 1
+fi
+
+llvm_version=$(llvm-config --version)
+if [[ ! $llvm_version =~ ^18\. ]]; then
+ echo "Error: Unexpected LLVM version: $llvm_version (expected 18.x)" >&2
+ exit 1
+fi
+
+prefix=$(llvm-config --prefix)
+if [ ! -d "$prefix" ]; then
+ echo "Error: LLVM installation directory not found: $prefix" >&2
+ exit 1
+fi
+
+export LLVM_SYS_180_PREFIX=$prefix
+echo "Using LLVM installation at: $LLVM_SYS_180_PREFIX (version $llvm_version)"📝 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.
| prefix=$(llvm-config --prefix) | |
| export LLVM_SYS_180_PREFIX=$prefix | |
| echo $LLVM_SYS_180_PREFIX | |
| if ! command -v llvm-config &> /dev/null; then | |
| echo "Error: llvm-config not found" >&2 | |
| exit 1 | |
| fi | |
| llvm_version=$(llvm-config --version) | |
| if [[ ! $llvm_version =~ ^18\. ]]; then | |
| echo "Error: Unexpected LLVM version: $llvm_version (expected 18.x)" >&2 | |
| exit 1 | |
| fi | |
| prefix=$(llvm-config --prefix) | |
| if [ ! -d "$prefix" ]; then | |
| echo "Error: LLVM installation directory not found: $prefix" >&2 | |
| exit 1 | |
| fi | |
| export LLVM_SYS_180_PREFIX=$prefix | |
| echo "Using LLVM installation at: $LLVM_SYS_180_PREFIX (version $llvm_version)" |
1 participant
Summary by CodeRabbit
Workflow Updates
pull_requesttrigger for the release/test branchBuild Configuration
Code Structure
core/vmdirectorycommitfunctionChores