Update oarfish to 0.6.3 #52102
Update oarfish to 0.6.3 #52102
Conversation
📝 WalkthroughWalkthroughThis pull request updates the Possibly related PRs
Warning Rate limit exceeded@rob-p has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 4 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
Force latest rustc
|
I think I need the latest rust compiler 1.82, but am I sure how to force this. |
![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 (1)
recipes/oarfish/build.sh (1)
7-8: Remove extra blank lineThere's an unnecessary extra blank line that can be removed.
-
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
recipes/oarfish/build.sh(1 hunks)recipes/oarfish/meta.yaml(2 hunks)
🧰 Additional context used
🪛 yamllint
recipes/oarfish/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (4)
recipes/oarfish/build.sh (1)
Line range hint 11-11: LGTM: Build command looks good
The cargo install command with RUST_BACKTRACE=1 is appropriate for debugging potential build issues.
recipes/oarfish/meta.yaml (3)
1-1: LGTM: Version update is consistent
The version bump to 0.6.3 is properly reflected in the recipe.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
Line range hint 3-44: LGTM: Recipe structure follows bioconda best practices
The recipe maintains proper structure with all required sections and follows bioconda guidelines for version updates.
🧰 Tools
🪛 yamllint
[error] 11-11: trailing spaces
(trailing-spaces)
14-14: Verify source tarball checksum
Let's verify that the SHA256 checksum matches the upstream source tarball.
recipes/oarfish/meta.yaml
Outdated
| #- rust >=1.82.0 | ||
| #- {{ compiler('rust') }} |
There was a problem hiding this comment.
Critical: Uncomment and update Rust compiler requirement
Both Rust requirements are currently commented out, but according to rob-p's comment, the package requires Rust >=1.82.0. This could lead to build failures or runtime issues.
Apply this diff to fix the Rust requirements:
- #- rust >=1.82.0
- #- {{ compiler('rust') }}
+ - {{ compiler('rust') }} >=1.82.0Note: Using the compiler('rust') selector with a version constraint is the recommended approach in bioconda to ensure proper Rust compiler version requirements.
📝 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.
| #- rust >=1.82.0 | |
| #- {{ compiler('rust') }} | |
| - {{ compiler('rust') }} >=1.82.0 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/oarfish/meta.yaml(2 hunks)
🧰 Additional context used
🪛 yamllint
recipes/oarfish/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (3)
recipes/oarfish/meta.yaml (3)
1-1: LGTM: Version update looks good
Version bump from 0.6.2 to 0.6.3 is properly implemented.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
21-22: LGTM: C/C++ compiler requirements
The addition of both C and C++ compiler requirements is appropriate for this package.
14-14: Verify SHA256 checksum
Let's verify the SHA256 checksum matches the source tarball.
| #- rust >=1.82.0 | ||
| #- {{ compiler('rust') }} >=1.82.0 |
There was a problem hiding this comment.
Blocking: Rust compiler configuration must be resolved
The PR cannot be merged until the Rust compiler requirements are properly configured. Please address the previous comment about uncommenting and updating the Rust compiler requirement.
Reference rob-p's comment: "we need the latest rust compiler (1.82)."
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
recipes/oarfish/build.sh (1)
15-16: Add Rust version verificationSince oarfish requires Rust >=1.82.0, add version verification after installation.
Add these lines after the PATH export:
export PATH="$HOME/.cargo/bin:$PATH" +# Verify Rust version meets minimum requirement +rustc --version | awk -F. '{ if ($2 >= 82) exit 0; exit 1 }' || (echo "Rust 1.82+ is required"; exit 1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/oarfish/build.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/oarfish/build.sh
[error] 6-6: Did you forget the 'then' for this 'if'?
(SC1049)
[error] 6-6: Couldn't parse this if expression. Fix to allow more checks.
(SC1073)
[error] 9-9: Expected 'then'.
(SC1050)
[error] 9-9: Unexpected keyword/token. Fix any mentioned problems and try again.
(SC1072)
🔇 Additional comments (1)
recipes/oarfish/build.sh (1)
14-15: Previous security concerns about Rust installation are still valid
The concerns about Rust installation security and version control raised in the previous review remain applicable.
recipes/oarfish/build.sh
Outdated
| # build statically linked binary with Rust | ||
| RUST_BACKTRACE=1 cargo install --verbose --root $PREFIX --path . | ||
| RUST_BACKTRACE=1 RUSTFLAGS=" cargo install --verbose --root $PREFIX --path . |
There was a problem hiding this comment.
Fix malformed cargo installation command
The current command is incomplete and has syntax errors.
Apply this diff to fix the cargo installation command:
-RUST_BACKTRACE=1 RUSTFLAGS=" cargo install --verbose --root $PREFIX --path .
+RUST_BACKTRACE=1 RUSTFLAGS="-C target-feature=+crt-static" cargo install --verbose --root $PREFIX --path .This:
- Properly formats the RUSTFLAGS for static linking
- Completes the command with proper syntax
📝 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.
| RUST_BACKTRACE=1 RUSTFLAGS=" cargo install --verbose --root $PREFIX --path . | |
| RUST_BACKTRACE=1 RUSTFLAGS="-C target-feature=+crt-static" cargo install --verbose --root $PREFIX --path . |
recipes/oarfish/build.sh
Outdated
| unamestr=`uname` | ||
| if [ "$unamestr" == 'Darwin' ]; | ||
| export CC=clang | ||
| export CXX=clang++ | ||
| else | ||
| export CC=gcc | ||
| export CXX=g++ | ||
| fi |
There was a problem hiding this comment.
Fix shell syntax in the if condition
The if condition has syntax errors that need to be corrected.
Apply this diff to fix the shell syntax:
unamestr=`uname`
-if [ "$unamestr" == 'Darwin' ];
+if [ "$unamestr" = 'Darwin' ]; then
export CC=clang
export CXX=clang++
else
export CC=gcc
export CXX=g++
fiNote: Using = instead of == for POSIX shell compatibility.
📝 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.
| unamestr=`uname` | |
| if [ "$unamestr" == 'Darwin' ]; | |
| export CC=clang | |
| export CXX=clang++ | |
| else | |
| export CC=gcc | |
| export CXX=g++ | |
| fi | |
| unamestr=`uname` | |
| if [ "$unamestr" = 'Darwin' ]; then | |
| export CC=clang | |
| export CXX=clang++ | |
| else | |
| export CC=gcc | |
| export CXX=g++ | |
| fi |
🧰 Tools
🪛 Shellcheck
[error] 6-6: Did you forget the 'then' for this 'if'?
(SC1049)
[error] 6-6: Couldn't parse this if expression. Fix to allow more checks.
(SC1073)
[error] 9-9: Expected 'then'.
(SC1050)
[error] 9-9: Unexpected keyword/token. Fix any mentioned problems and try again.
(SC1072)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
recipes/oarfish/build.sh (1)
Line range hint
1-20: Enhance build reproducibilityConsider adding version checks for all required compilers (C, C++, Rust) at the start of the script to fail fast if requirements aren't met. This would improve build reproducibility and provide clearer error messages.
Would you like me to provide an implementation for compiler version checks?
🧰 Tools
🪛 Shellcheck
[error] 20-20: Couldn't parse this double quoted string. Fix to allow more checks.
(SC1073)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/oarfish/build.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/oarfish/build.sh
[error] 20-20: Couldn't parse this double quoted string. Fix to allow more checks.
(SC1073)
🔇 Additional comments (2)
recipes/oarfish/build.sh (2)
5-13: Past review comment is still applicable
The shell syntax issues in the if condition were already identified in a previous review.
20-20: Past review comment is still applicable
The malformed cargo installation command issues were already identified in a previous review.
🧰 Tools
🪛 Shellcheck
[error] 20-20: Couldn't parse this double quoted string. Fix to allow more checks.
(SC1073)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
recipes/oarfish/build.sh (1)
Line range hint
1-24: Consider following bioconda build script best practicesThe build script could benefit from following bioconda's best practices:
- Use
#!/usr/bin/env bashfor better portability- Consider using bioconda's build environment variables (e.g.,
$PREFIX,$BUILD_PREFIX)- Add error handling and logging
Would you like me to provide an example of a more robust build script following these practices?
🧰 Tools
🪛 Shellcheck
[warning] 15-15: This expands when defined, not when used. Consider escaping.
(SC2139)
[warning] 16-16: This expands when defined, not when used. Consider escaping.
(SC2139)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/oarfish/build.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/oarfish/build.sh
[warning] 15-15: This expands when defined, not when used. Consider escaping.
(SC2139)
[warning] 16-16: This expands when defined, not when used. Consider escaping.
(SC2139)
recipes/oarfish/build.sh
Outdated
| unamestr=`uname` | ||
| if [ "$unamestr" == 'Darwin' ]; | ||
| then | ||
| export CC=clang | ||
| export CXX=clang++ | ||
| else | ||
| export CC=gcc | ||
| export CXX=g++ | ||
| fi |
There was a problem hiding this comment.
Fix POSIX shell compatibility issue in if condition
The if condition uses == which is bash-specific. For better portability across different shells, use = instead.
Apply this diff:
-if [ "$unamestr" == 'Darwin' ];
+if [ "$unamestr" = 'Darwin' ];📝 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.
| unamestr=`uname` | |
| if [ "$unamestr" == 'Darwin' ]; | |
| then | |
| export CC=clang | |
| export CXX=clang++ | |
| else | |
| export CC=gcc | |
| export CXX=g++ | |
| fi | |
| unamestr=`uname` | |
| if [ "$unamestr" = 'Darwin' ]; | |
| then | |
| export CC=clang | |
| export CXX=clang++ | |
| else | |
| export CC=gcc | |
| export CXX=g++ | |
| fi |
| curl https://sh.rustup.rs -sSf | sh -s -- --default-toolchain stable --profile=minimal -y | ||
| export PATH="$HOME/.cargo/bin:$PATH" |
There was a problem hiding this comment.
Pin Rust version and improve installation security
- The script doesn't enforce the required Rust 1.82 version mentioned in PR objectives
- Using
curl | shwithout verification is potentially unsafe
Apply this diff:
-curl https://sh.rustup.rs -sSf | sh -s -- --default-toolchain stable --profile=minimal -y
+# Download rustup-init first
+curl -o rustup-init.sh https://sh.rustup.rs -sSf
+# Verify the download (add checksum verification here)
+chmod +x rustup-init.sh
+# Install with specific version
+./rustup-init.sh --default-toolchain 1.82.0 --profile=minimal -y
+rm rustup-init.sh📝 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.
| curl https://sh.rustup.rs -sSf | sh -s -- --default-toolchain stable --profile=minimal -y | |
| export PATH="$HOME/.cargo/bin:$PATH" | |
| # Download rustup-init first | |
| curl -o rustup-init.sh https://sh.rustup.rs -sSf | |
| # Verify the download (add checksum verification here) | |
| chmod +x rustup-init.sh | |
| # Install with specific version | |
| ./rustup-init.sh --default-toolchain 1.82.0 --profile=minimal -y | |
| rm rustup-init.sh | |
| export PATH="$HOME/.cargo/bin:$PATH" |
recipes/oarfish/build.sh
Outdated
| alias cc=$CC | ||
| alias cxx=$CXX |
There was a problem hiding this comment.
Fix alias expansion timing
The current aliases expand the variables when defined, not when used. This could cause issues if CC/CXX are modified later.
Apply this diff to use proper shell functions instead:
-alias cc=$CC
-alias cxx=$CXX
+cc() { $CC "$@"; }
+cxx() { $CXX "$@"; }📝 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.
| alias cc=$CC | |
| alias cxx=$CXX | |
| cc() { $CC "$@"; } | |
| cxx() { $CXX "$@"; } |
🧰 Tools
🪛 Shellcheck
[warning] 15-15: This expands when defined, not when used. Consider escaping.
(SC2139)
[warning] 16-16: This expands when defined, not when used. Consider escaping.
(SC2139)
Update
oarfish: 0.6.2 → 0.6.3recipes/oarfish(click to view/edit other files)@COMBINE-labThis pull request was automatically generated (see docs).