Skip to content
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.

Sign up for GitHub

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

Update oarfish to 0.6.3 #52102

Merged
merged 15 commits into from
Nov 14, 2024
Merged

Update oarfish to 0.6.3 #52102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a3d5b7b
Update oarfish to 0.6.3
BiocondaBot Nov 14, 2024
6414742
Update meta.yaml
rob-p Nov 14, 2024
1e3e769
Update build.sh
rob-p Nov 14, 2024
6951f2f
Update meta.yaml
rob-p Nov 14, 2024
c97fa7e
Update meta.yaml
rob-p Nov 14, 2024
ba46020
Update meta.yaml
rob-p Nov 14, 2024
5b970fd
Update build.sh
rob-p Nov 14, 2024
ad590fb
Update meta.yaml
rob-p Nov 14, 2024
c9a022f
Update build.sh
rob-p Nov 14, 2024
586b286
Update build.sh
rob-p Nov 14, 2024
fc93ae5
Update build.sh
rob-p Nov 14, 2024
5eaddb3
Update build.sh
rob-p Nov 14, 2024
007bdf9
Update build.sh
rob-p Nov 14, 2024
7e93f88
Update build.sh
rob-p Nov 14, 2024
ff95fdd
Update build.sh
rob-p Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion recipes/oarfish/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,9 @@

set -xe

curl https://sh.rustup.rs -sSf | sh -s -- --default-toolchain stable --profile=minimal -y
export PATH="$HOME/.cargo/bin:$PATH"
Comment on lines +5 to +6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Pin Rust version and improve installation security

  1. The script doesn't enforce the required Rust 1.82 version mentioned in PR objectives
  2. Using curl | sh without 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.

Suggested change
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"



# build statically linked binary with Rust
RUST_BACKTRACE=1 cargo install --verbose --root $PREFIX --path .
RUST_BACKTRACE=1 RUSTFLAGS="-C linker=$CC" cargo install --verbose --root $PREFIX --path .
8 changes: 5 additions & 3 deletions recipes/oarfish/meta.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{% set version = "0.6.2" %}
{% set version = "0.6.3" %}

package:
name: oarfish
Expand All @@ -11,13 +11,15 @@ build:

source:
url: https://github.com/COMBINE-lab/oarfish/archive/v{{ version }}.tar.gz
sha256: 78b523fc459fec5ae3680395925862b4d367bd56d051120f28c689dd387e1758
sha256: 98cc4b939e81cd0018c38d47f4596dc3079680a7b076a7dfca2e80b67094c783

requirements:
build:
- make
- {{ compiler('rust') }}
#- rust >=1.82.0
#- {{ compiler('rust') }} >=1.82.0
Comment on lines +19 to +20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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)."

- {{ compiler('c') }}
- {{ compiler('cxx') }}
host:
- zlib
run:
Expand Down