fix: add downloading dependencies to install script #524
Conversation
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
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
CodeRabbit Configuration File (
|
RollkitBot
requested review from
Manav-Aggarwal,
tuxcanfly and
yarikbratashchuk
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
public/install.sh (2)
Line range hint
17-19: Add post-installation verificationGiven the PR objectives about the binary not working after installation, we should add verification steps.
Add verification after installation:
make install + +# Verify installation +echo "Verifying installation..." +if ! command -v rollkit &> /dev/null; then + echo "Error: rollkit binary not found in PATH after installation" + exit 1 +fi + +# Test the binary +if ! rollkit version &> /dev/null; then + echo "Error: rollkit binary failed to execute. Please check system resources and permissions" + exit 1 +fi + cd .. -echo "Installation completed successfully." +echo "Installation completed and verified successfully."
Line range hint
1-1: Consider adding system requirements checkThe "zsh: killed" error mentioned in the PR objectives often indicates insufficient system resources (particularly memory).
Consider adding a system requirements check at the start of the script:
#!/bin/bash + +# Check system requirements +MIN_MEMORY_MB=2048 +AVAILABLE_MEMORY_MB=$(free -m | awk '/^Mem:/{print $2}') + +if [ "$AVAILABLE_MEMORY_MB" -lt "$MIN_MEMORY_MB" ]; then + echo "Error: Insufficient memory. Rollkit requires at least ${MIN_MEMORY_MB}MB of RAM" + echo "Available memory: ${AVAILABLE_MEMORY_MB}MB" + exit 1 +fi
| echo "Installing dependencies..." | ||
| go mod tidy | ||
| go mod download |
There was a problem hiding this comment.
Add error handling for dependency management steps
While adding dependency management is crucial for fixing the binary issues, these steps need proper error handling to prevent silent failures.
Consider this improvement:
echo "Installing dependencies..."
-go mod tidy
-go mod download
+go mod tidy || { echo "Failed to tidy Go modules. Please check your Go environment."; exit 1; }
+go mod download || { echo "Failed to download dependencies. Please check your network connection."; exit 1; }
+echo "Dependencies installed successfully."📝 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.
| echo "Installing dependencies..." | |
| go mod tidy | |
| go mod download | |
| echo "Installing dependencies..." | |
| go mod tidy || { echo "Failed to tidy Go modules. Please check your Go environment."; exit 1; } | |
| go mod download || { echo "Failed to download dependencies. Please check your network connection."; exit 1; } | |
| echo "Dependencies installed successfully." |
| @@ -10,6 +10,9 @@ | |||
|
|
|||
| cd rollkit || { echo "Failed to find the downloaded repository."; exit 1; } | |||
| git fetch && git checkout $1 | |||
There was a problem hiding this comment.
Validate version parameter before checkout
The script uses $1 without validation, which could lead to checkout failures or unexpected versions.
Add version validation:
+if [ -z "$1" ]; then
+ echo "Error: Version parameter is required (e.g., v0.1.0)"
+ exit 1
+fi
+
+# Validate if the version exists
+if ! git ls-remote --tags origin | grep -q "refs/tags/$1$"; then
+ echo "Error: Version $1 not found"
+ exit 1
+fi
+
git fetch && git checkout $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.
| git fetch && git checkout $1 | |
| if [ -z "$1" ]; then | |
| echo "Error: Version parameter is required (e.g., v0.1.0)" | |
| exit 1 | |
| fi | |
| # Validate if the version exists | |
| if ! git ls-remote --tags origin | grep -q "refs/tags/$1$"; then | |
| echo "Error: Version $1 not found" | |
| exit 1 | |
| fi | |
| git fetch && git checkout $1 |
|
nit [not blocking]: i'm realizing that the reason for the script failing for me was probably because I'm not on the right go version. I suggest either adding a check for this or adding a step to install golang if it's not there or the wrong version |
|
closing for #525 instead |
Pull request was closed
Overview
Closes #523
Summary by CodeRabbit