-
Notifications
You must be signed in to change notification settings - Fork 67
Improve installation scripts and documentation #1843
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe updates include relaxing version constraints for dependencies in Cargo.toml, significantly expanding and clarifying installation documentation, updating the Anchor CLI version in the README, and enhancing both the build and install scripts with improved error handling, user messages, and clearer usage instructions. No changes were made to exported or public entities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant build.sh
participant install.sh
User->>build.sh: Run build script
build.sh->>build.sh: Set error trap (set -e, handle_error)
build.sh->>build.sh: Check for pnpm/npx
alt pnpm/npx missing
build.sh->>User: Prompt to run install.sh
Note right of build.sh: Exit script
end
build.sh->>build.sh: Install dependencies
alt Dependency install fails
build.sh->>User: Show error, suggest checking internet/access
Note right of build.sh: Exit script
end
build.sh->>build.sh: Copy spl_noop.so
build.sh->>build.sh: Start build process
alt Build fails
build.sh->>User: Output failure message
else
build.sh->>User: Output success message
end
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
scripts/install.sh (1)
425-440
: Helpful usage banner, but missing stderr redirection & exits with confusing code
- Consider printing the usage to stderr so
mycmd 2>/dev/null
behaves as expected in scripts.- The banner exits with status 1 even for
--help
style output; exiting with 0 is more conventional when the user only asked for help.-echo "Unknown option: $1" +echo "Unknown option: $1" >&2 ... -echo "------------------------------------------------" -exit 1 +echo "------------------------------------------------" >&2 +exit 1(and add a dedicated
--help|-h
branch that prints the same banner butexit 0
).INSTALL.md (1)
69-72
: Minor punctuation nitComma missing before but in a compound sentence.
-If you already have Node.js installed but the script doesn't recognize it, try skipping this component: +If you already have Node.js installed, but the script doesn't recognize it, try skipping this component:scripts/build.sh (2)
3-14
: Add-u -o pipefail
for more robust error propagation
set -e
alone misses failures inside pipes and unbound variables. Two-line tweak:-# Exit on any error -set -e +# Strict shell mode +set -Eeuo pipefail(
-E
keeps traps working with-u
).
27-38
:pnpm install
runs every build – use--frozen-lockfile
or skip whennode_modules
existsRe-installing on every invocation slows local builds. Either:
[ -d node_modules ] || pnpm install --frozen-lockfileor rely on the lock-file flag alone (CI).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Cargo.toml
(1 hunks)INSTALL.md
(3 hunks)README.md
(1 hunks)scripts/build.sh
(1 hunks)scripts/install.sh
(1 hunks)
🧰 Additional context used
🪛 LanguageTool
INSTALL.md
[uncategorized] ~69-~69: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...*: If you already have Node.js installed but the script doesn't recognize it, try sk...
(COMMA_COMPOUND_SENTENCE_2)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: lint
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test sdk-libs
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: stateless-js-v2
- GitHub Check: cli-v1
- GitHub Check: test-double-registration
- GitHub Check: test-2-foresters
- GitHub Check: test-address-batched
- GitHub Check: test-state-photon-batched
- GitHub Check: test-state-batched
- GitHub Check: test-e2e
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v2
🔇 Additional comments (2)
Cargo.toml (1)
112-114
: ```shell
#!/usr/bin/env bash
set -e
echo "=== Listing all shell scripts in the repo ==="
fd --type f --extension shecho
echo "=== Searching for 'install.sh' references ==="
rg -n 'install.sh' -g '*.sh' || trueecho
echo "=== Searching for a VERSIONS array in shell scripts ==="
rg -n 'VERSIONS' -A3 -B3 -g '*.sh' || trueecho
echo "=== Searching for pinned anchor versions in shell scripts ==="
rg -n 'anchor-v[0-9]' -A2 -B2 -g '*.sh' || true</details> <details> <summary>README.md (1)</summary> `139-140`: **README is already updated – double-check installation script** Good catch updating the docs to 0.31.1. After merging the previous comment’s fix in `install.sh`, the documentation and tooling will be in sync. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Summary by CodeRabbit
Documentation
Chores
Style