fix(build): revert to working build + cherry-picks #1715
Conversation
WalkthroughThe project underwent a series of updates affecting build configurations, environment variables, and scripts for release preparation. It includes simplifications and enhancements in the build process, removal of outdated dependencies, and introduction of new utility scripts and makefile targets. The changes aim to streamline the development and deployment process, refine the build environment, and improve the handling of cross-platform releases. 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 X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- go.mod
- go.sum
Files selected for processing (9)
- .github/workflows/goreleaser.yml (1 hunks)
- .goreleaser.yml (3 hunks)
- CHANGELOG.md (2 hunks)
- contrib/bashlib.sh (1 hunks)
- contrib/make/build.mk (2 hunks)
- contrib/make/release.mk (1 hunks)
- contrib/scripts/release_pre_darwin.sh (1 hunks)
- contrib/scripts/release_pre_linux.sh (1 hunks)
- justfile (1 hunks)
Files skipped from review due to trivial changes (1)
- contrib/scripts/release_pre_darwin.sh
Additional comments: 24
.github/workflows/goreleaser.yml (1)
- 14-14: The change to
runs-on: ubuntu-latestmay affect the build environment. Verify that all dependencies and build steps are compatible with the updated runner environment..goreleaser.yml (5)
9-22: The changes to the
darwinbuild configuration, including the update to the pre-hook script and the environment variable for the compiler, seem to align with the PR's goal of fixing build issues. Ensure that the newCCvalue is correct and that therelease_pre_darwin.shscript is properly configured to handle the build environment.40-44: The addition of the
overridessection for thedarwinplatform witharm64architecture is a significant change. Verify that theoa64-clangcompiler is available and properly configured in the build environment, and that this change has been tested to ensure it does not introduce any new issues.46-61: The updates to the
linuxbuild configuration, including the new pre-hook script and the updated compiler environment variable, should be verified. Confirm that therelease_pre_linux.shscript is correctly handling the build requirements and that thex86_64-linux-gnu-gcccompiler is the intended choice for thelinuxbuilds.79-83: The new
overridessection for thelinuxplatform witharm64architecture introduces a different compiler setting. Verify that theaarch64-linux-gnu-gcccompiler is correctly set up in the build environment and that this change has been thoroughly tested to prevent any build issues.37-61: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [9-83]
While the hunks do not display removed code, the AI-generated summary mentions the removal of specific environment variables. Please confirm that any removed environment variables are no longer required and that their removal does not affect the build process or other dependencies.
CHANGELOG.md (2)
51-51: Verify that the entry "fix(build): revert to working build + cherry-picks" is correctly categorized under "State Machine Breaking". If it's not related to state machine changes, it should be placed under the appropriate section.
85-86: Confirm that the release links and commit references for version
v1.1.0are accurate and lead to the correct resources.contrib/make/build.mk (5)
6-9: The addition of
BRANCHandCOMMITvariables is correctly implemented using shell commands to capture the current git branch and commit hash.27-32: The
SDK_PACKvariable is introduced to capture the Cosmos-SDK version, and theTM_VERSIONvariable is updated to reflect the new source repositorycometbft. TheROCKSDB_VERSIONis set to a static value. Ensure that these versions are compatible with the rest of the project.17-35: The use of the new
BRANCHandCOMMITvariables to dynamically set theVERSIONwhen it is not provided by the user is a good practice, ensuring that the build artifacts are always versioned.17-35: The linker flags (
ldflags) are correctly updated to include the newTM_VERSIONand the existingCOMMITvariable, which will be reflected in the built binaries.17-35: The download commands for
rocksdblibandwasmvmlibare correctly using theROCKSDB_VERSIONandWASMVM_VERSIONvariables. The conditional logic for different operating systems and architectures is also correctly implemented.contrib/make/release.mk (1)
- 20-25: The removal of the volume mapping from the
docker runcommand in therelease-snapshottarget may affect the build process if there was a dependency on shared temporary files or directories. Verify that the build process functions correctly without this volume mapping and that no other parts of the build rely on this behavior.justfile (10)
6-8: The
installrecipe is clear and straightforward, assuming themake installcommand is correctly set up in theMakefile.10-12: The
buildrecipe is also clear, assuming themake buildcommand is correctly set up in theMakefile.14-14: The alias
bforbuildis a useful shortcut for developers.16-19: The
proto-genrecipe is clear, assuming themake proto-gencommand is correctly set up in theMakefile.21-21: The alias
protoforproto-genis a useful shortcut for developers.27-35: The
lintrecipe includes installation ofgolangci-lintif not present, which is a good practice. However, the installation command should be verified for correctness and safety.37-39: The
fmtrecipe usesgofumptto format the code, which is a standard practice for Go codebases.41-47: The
tidyrecipe combines multiple formatting and linting steps, which is a good practice for maintaining code quality.49-50: The
release-testrecipe should be verified to ensure that themake release-snapshotcommand is correctly set up in theMakefile.Verification successful
The
release-testrecipe in thejustfileis correctly set up. Themake release-snapshotcommand exists and is defined in thecontrib/make/release.mkfile.* 52-53: The `release-publish` recipe should be verified to ensure that the `make release` command is correctly set up in the `Makefile`.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the `make release-snapshot` command is correctly set up in the `Makefile`. rg --type make "release-snapshot:" MakefileLength of output: 43
Script:
#!/bin/bash # Search for any Makefile or .mk files in the repository fd --type f --extension mk --extension '' 'Makefile' | xargs rg "release-snapshot:"Length of output: 123
CHANGELOG.md
Outdated
| @@ -48,6 +48,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| * [#1682](https://github.com/NibiruChain/nibiru/pull/1682) - feat!: add upgrade handler for v1.1.0 | |||
| * [#1706](https://github.com/NibiruChain/nibiru/pull/1706) - fix: `v1.1.0` upgrade handler | |||
| * [#1712](https://github.com/NibiruChain/nibiru/pull/1712) - refactor(inflation): turn inflation off by default | |||
| * [#xxx](https://github.com/NibiruChain/nibiru/pull/xxx) - fix(build): revert to working build + cherry-picks | |||
There was a problem hiding this comment.
The PR link placeholder [#xxx] should be replaced with the actual PR number that corresponds to the "fix(build): revert to working build + cherry-picks" entry.
| @@ -73,15 +74,13 @@ | |||
| * [#1710](https://github.com/NibiruChain/nibiru/pull/1710) - refactor(perp): Clean and organize module errors for x/perp | |||
There was a problem hiding this comment.
The entry under "Non-breaking/Compatible Improvements" for PR [#1710] lacks a description of what the refactor entails. It would be beneficial for users to have a brief summary of the changes made.
contrib/bashlib.sh
Outdated
| #!/usr/bin/env bash | ||
|
|
||
| # ————————————————————————————————————————————————— | ||
| # COLORS: Terminal colors are set with ANSI escape codes. | ||
|
|
||
| export COLOR_GREEN="\033[32m" | ||
| export COLOR_CYAN="\033[36m" | ||
| export COLOR_RESET="\033[0m" | ||
|
|
||
| export COLOR_BLACK="\033[30m" | ||
| export COLOR_RED="\033[31m" | ||
| export COLOR_YELLOW="\033[33m" | ||
| export COLOR_BLUE="\033[34m" | ||
| export COLOR_MAGENTA="\033[35m" | ||
| export COLOR_WHITE="\033[37m" | ||
|
|
||
| # Bright color definitions | ||
| export COLOR_BRIGHT_BLACK="\033[90m" | ||
| export COLOR_BRIGHT_RED="\033[91m" | ||
| export COLOR_BRIGHT_GREEN="\033[92m" | ||
| export COLOR_BRIGHT_YELLOW="\033[93m" | ||
| export COLOR_BRIGHT_BLUE="\033[94m" | ||
| export COLOR_BRIGHT_MAGENTA="\033[95m" | ||
| export COLOR_BRIGHT_CYAN="\033[96m" | ||
| export COLOR_BRIGHT_WHITE="\033[97m" | ||
|
|
||
| # ————————————————————————————————————————————————— | ||
| # LOGGING | ||
|
|
||
| # log_debug: Simple wrapper for `echo` with a DEBUG prefix. | ||
| log_debug() { | ||
| echo "${COLOR_CYAN}DEBUG${COLOR_RESET}" "$@" | ||
| } | ||
|
|
||
| # log_error: ERROR messages in red, output to stderr. | ||
| log_error() { | ||
| echo "❌ ${COLOR_RED}ERROR:${COLOR_RESET}" "$@" >&2 | ||
| } | ||
|
|
||
| log_success() { | ||
| echo "${COLOR_GREEN}✅ Success:${COLOR_RESET}" "$@" | ||
| } | ||
|
|
||
| # ————————————————————————————————————————————————— | ||
| # OK Suffix: Functions used for error handling or validating inputs. | ||
|
|
||
| # which_ok: Check if the given binary is in the $PATH. | ||
| # Returns code 0 on success and code 1 if the command fails. | ||
| which_ok() { | ||
| if which "$1" >/dev/null 2>&1; then | ||
| return 0 | ||
| else | ||
| log_error "$1 is not present in \$PATH" | ||
| return 1 | ||
| fi | ||
| } | ||
|
|
||
| # source_ok (Function): Sources a bash script if it exists. | ||
| # Usage: source_ok [bash_script] | ||
| source_ok() { | ||
| local bash_script="$1" | ||
| if test -r "$bash_script"; then | ||
| # shellcheck disable=SC1090 | ||
| source "$bash_script" | ||
| fi | ||
| } | ||
|
|
||
| env_var_ok() { | ||
| local env_var="$1" | ||
| if [[ -z "$env_var" ]]; then | ||
| log_error "expected env var to be set" | ||
| return 1 # Return 1 to indicate error (variable is not set) | ||
| else | ||
| return 0 # Return 0 to indicate success (variable is set) | ||
| fi |
There was a problem hiding this comment.
The color definitions and logging functions in contrib/bashlib.sh are well-defined and follow standard bash scripting conventions. The use of ANSI escape codes for color output is correct, and the logging functions provide clear visual cues for different log levels. The which_ok, source_ok, and env_var_ok functions provide basic utility checks with appropriate error handling and logging.
However, the source_ok function on lines 58-66 does not handle the case where the file exists but sourcing it fails (e.g., due to syntax errors in the sourced file). It is recommended to check the exit status of the source command and log an error if it fails.
if test -r "$bash_script"; then
# shellcheck disable=SC1090
- source "$bash_script"
+ if ! source "$bash_script"; then
+ log_error "Failed to source $bash_script"
+ return 1
+ fi
fiCommittable 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.
| #!/usr/bin/env bash | |
| # ————————————————————————————————————————————————— | |
| # COLORS: Terminal colors are set with ANSI escape codes. | |
| export COLOR_GREEN="\033[32m" | |
| export COLOR_CYAN="\033[36m" | |
| export COLOR_RESET="\033[0m" | |
| export COLOR_BLACK="\033[30m" | |
| export COLOR_RED="\033[31m" | |
| export COLOR_YELLOW="\033[33m" | |
| export COLOR_BLUE="\033[34m" | |
| export COLOR_MAGENTA="\033[35m" | |
| export COLOR_WHITE="\033[37m" | |
| # Bright color definitions | |
| export COLOR_BRIGHT_BLACK="\033[90m" | |
| export COLOR_BRIGHT_RED="\033[91m" | |
| export COLOR_BRIGHT_GREEN="\033[92m" | |
| export COLOR_BRIGHT_YELLOW="\033[93m" | |
| export COLOR_BRIGHT_BLUE="\033[94m" | |
| export COLOR_BRIGHT_MAGENTA="\033[95m" | |
| export COLOR_BRIGHT_CYAN="\033[96m" | |
| export COLOR_BRIGHT_WHITE="\033[97m" | |
| # ————————————————————————————————————————————————— | |
| # LOGGING | |
| # log_debug: Simple wrapper for `echo` with a DEBUG prefix. | |
| log_debug() { | |
| echo "${COLOR_CYAN}DEBUG${COLOR_RESET}" "$@" | |
| } | |
| # log_error: ERROR messages in red, output to stderr. | |
| log_error() { | |
| echo "❌ ${COLOR_RED}ERROR:${COLOR_RESET}" "$@" >&2 | |
| } | |
| log_success() { | |
| echo "${COLOR_GREEN}✅ Success:${COLOR_RESET}" "$@" | |
| } | |
| # ————————————————————————————————————————————————— | |
| # OK Suffix: Functions used for error handling or validating inputs. | |
| # which_ok: Check if the given binary is in the $PATH. | |
| # Returns code 0 on success and code 1 if the command fails. | |
| which_ok() { | |
| if which "$1" >/dev/null 2>&1; then | |
| return 0 | |
| else | |
| log_error "$1 is not present in \$PATH" | |
| return 1 | |
| fi | |
| } | |
| # source_ok (Function): Sources a bash script if it exists. | |
| # Usage: source_ok [bash_script] | |
| source_ok() { | |
| local bash_script="$1" | |
| if test -r "$bash_script"; then | |
| # shellcheck disable=SC1090 | |
| source "$bash_script" | |
| fi | |
| } | |
| env_var_ok() { | |
| local env_var="$1" | |
| if [[ -z "$env_var" ]]; then | |
| log_error "expected env var to be set" | |
| return 1 # Return 1 to indicate error (variable is not set) | |
| else | |
| return 0 # Return 0 to indicate success (variable is set) | |
| fi | |
| # source_ok (Function): Sources a bash script if it exists. | |
| # Usage: source_ok [bash_script] | |
| source_ok() { | |
| local bash_script="$1" | |
| if test -r "$bash_script"; then | |
| # shellcheck disable=SC1090 | |
| if ! source "$bash_script"; then | |
| log_error "Failed to source $bash_script" | |
| return 1 | |
| fi | |
| fi | |
| } |
| @@ -0,0 +1,76 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
Consider adding a set -e directive at the beginning of the script to ensure that the script exits immediately if any command exits with a non-zero status. This is a common best practice for bash scripts to prevent unexpected behavior in case of errors. However, if this script is intended to be sourced rather than executed directly, this may not be necessary.
#!/usr/bin/env bash
+ set -eCommittable 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.
| #!/usr/bin/env bash | |
| #!/usr/bin/env bash | |
| set -e |
| wget https://github.com/CosmWasm/wasmvm/releases/download/v${WASMVM_VERSION}/libwasmvm_muslc.x86_64.a -O /usr/lib/x86_64-linux-gnu/libwasmvm_muslc.a | ||
| wget https://github.com/CosmWasm/wasmvm/releases/download/v${WASMVM_VERSION}/libwasmvm_muslc.aarch64.a -O /usr/lib/aarch64-linux-gnu/libwasmvm_muslc.a | ||
| wget https://github.com/NibiruChain/gorocksdb/releases/download/v8.1.1/include.8.1.1.tar.gz -O /root/include.8.1.1.tar.gz | ||
| tar -xvf /root/include.8.1.1.tar.gz -C /usr/include/ | ||
| wget https://github.com/NibiruChain/gorocksdb/releases/download/v8.1.1/librocksdb_8.1.1_linux_amd64.tar.gz -O /root/librocksdb_8.1.1_linux_amd64.tar.gz | ||
| tar -xvf /root/librocksdb_8.1.1_linux_amd64.tar.gz -C /usr/lib/x86_64-linux-gnu/ | ||
| wget https://github.com/NibiruChain/gorocksdb/releases/download/v8.1.1/librocksdb_8.1.1_linux_arm64.tar.gz -O /root/librocksdb_8.1.1_linux_arm64.tar.gz | ||
| tar -xvf /root/librocksdb_8.1.1_linux_arm64.tar.gz -C /usr/lib/aarch64-linux-gnu/ |
There was a problem hiding this comment.
The script directly downloads and installs binaries and libraries to system directories. Consider the following points:
- Ensure the script is executed with appropriate permissions, as writing to
/usr/liband/usr/includetypically requires root access. - To avoid potential versioning issues, consider parameterizing the library versions or deriving them from a source of truth that reflects the project's dependencies.
- Check if the files already exist before downloading to prevent unnecessary network usage and file overwrites.
- It's a good practice to verify the integrity of the downloaded files (e.g., via checksums) before extracting them to system directories to mitigate the risk of malicious tampering.
| # Displays available recipes by running `just -l`. | ||
| setup: | ||
| #!/usr/bin/env bash | ||
| just -l |
There was a problem hiding this comment.
The setup recipe seems to be misnamed or misused, as it only lists available recipes instead of performing any setup tasks.
| # Build protobuf types (Rust) | ||
| proto-rs: | ||
| bash proto/buf.gen.rs.sh |
There was a problem hiding this comment.
💡 NOTE
Codebase verification is a beta feature.
The verification script has confirmed that the script proto/buf.gen.rs.sh does not exist or is not executable in the repository. This means that the proto-rs recipe in the justfile may fail to execute as intended since it relies on this script.
- The script
proto/buf.gen.rs.shreferenced in theproto-rsrecipe of thejustfileis either missing or not marked as executable. This needs to be addressed to ensure theproto-rsrecipe functions correctly.
Analysis chain
The proto-rs recipe should ensure that the script proto/buf.gen.rs.sh exists and is executable.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the script `proto/buf.gen.rs.sh` exists and is executable.
if [[ -x "proto/buf.gen.rs.sh" ]]; then
echo "The script proto/buf.gen.rs.sh is executable."
else
echo "The script proto/buf.gen.rs.sh does not exist or is not executable."
fi
Length of output: 175
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- CHANGELOG.md (2 hunks)
- contrib/bashlib.sh (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- contrib/bashlib.sh
Purpose
The builds and docker image workflows have been broken a while. I think we
should combine the RocksDB changes into Helder's PR (#1713) and merge the
changes in from a working state to make it easier to upgrade versions more
smoothly.
Cherry-picked Commits
The build was working perfectly in 8959862.
This PR works up from there, where I cherry-picked each commit from the
mainbranch in order, evaluated whether the commit was safe with:
If this passed, I kept the commit, and if it did not, I skipped it. Only two commits from our current
main(d3297de) are missing:Here is the full list of cherry-picked commits with working builds:
Summary by CodeRabbit
New Features
justfilerecipes for streamlined development tasks such as installation, building, and release management.Bug Fixes
Documentation
Refactor
goreleaserconfigurations.Chores
ubuntu-latestenvironment.