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.

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

Rebuild bam-readcount recipe #52212

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 9 additions & 13 deletions recipes/bam-readcount/build.sh
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
#!/bin/bash -ex

wget https://github.com/boostorg/boost/releases/download/boost-1.85.0/boost-1.85.0-cmake.tar.gz
wget https://github.com/boostorg/boost/releases/download/boost-1.86.0/boost-1.86.0-cmake.tar.gz

mv boost-1.85.0-cmake.tar.gz vendor/boost-1.55-bamrc.tar.gz
mv boost-1.86.0-cmake.tar.gz vendor/boost-1.55-bamrc.tar.gz

mkdir -p "${PREFIX}/bin"
ln -sf $(which libtool) "${PREFIX}/bin/libtool"
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

Fix command substitution quoting

The command substitution should be quoted to prevent word splitting issues if the path contains spaces.

Apply this fix:

-ln -sf $(which libtool) "${PREFIX}/bin/libtool"
+ln -sf "$(which libtool)" "${PREFIX}/bin/libtool"
📝 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
ln -sf $(which libtool) "${PREFIX}/bin/libtool"
ln -sf "$(which libtool)" "${PREFIX}/bin/libtool"
🧰 Tools
🪛 Shellcheck

[warning] 7-7: Quote this to prevent word splitting.

(SC2046)


# Needed for building utils dependency
export INCLUDES="-I{PREFIX}/include"
Expand All @@ -14,26 +14,22 @@ export CXXFLAGS="${CXXFLAGS} -O3 -I${PREFIX}/include"

if [[ `uname` == Darwin ]]; then
export LDFLAGS="${LDFLAGS} -Wl,-rpath,${PREFIX}/lib"
export CFLAGS="${CFLAGS} -Wno-unguarded-availability -Wdeprecated-non-prototype"
export CFLAGS="${CFLAGS} -Wno-unguarded-availability -Wno-deprecated-non-prototype"
export CMAKE_EXTRA="-DCMAKE_FIND_FRAMEWORK=NEVER -DCMAKE_FIND_APPBUNDLE=NEVER"
else
export CMAKE_EXTRA=""
fi

mkdir -p build
pushd build
cmake -S .. -B . -DCMAKE_INSTALL_PREFIX="${PREFIX}" \
cmake -S . -B build -DCMAKE_INSTALL_PREFIX="${PREFIX}" \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_CXX_COMPILER="${CXX}" \
-DCMAKE_C_COMPILER="${CC}" \
-DCMAKE_CXX_FLAGS="${CXXFLAGS}" \
-DCMAKE_C_FLAGS="${CFLAGS}" \
-Wno-dev -Wno-deprecated --no-warn-unused-cli \
${CMAKE_EXTRA}
"${CMAKE_EXTRA}"

make clean
make CXX="${CXX} ${LDFLAGS}" CC="${CC}" CXXFLAGS="${CXXFLAGS}" CFLAGS="${CFLAGS}"
cmake --build build -j "${CPU_COUNT}" -v

chmod 755 bin/bam-readcount
cp -f bin/bam-readcount "${PREFIX}/bin"
popd
install -d "$PREFIX/bin"
install -v -m 0755 build/bin/bam-readcount "$PREFIX/bin"
15 changes: 11 additions & 4 deletions recipes/bam-readcount/meta.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ package:
version: {{ version }}

source:
url: https://github.com/genome/{{ name }}/archive/v{{ version }}.tar.gz
sha256: 8ebf84d9efee0f2d3b43f0452dbf16b27337c960e25128f6a7173119e62588b8
git_url: https://github.com/genome/bam-readcount.git
git_rev: v{{ version }}
Comment on lines +9 to +10
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

Consider using tarball URL instead of git source for better reproducibility

Using VCS URLs in conda recipes can lead to reproducibility issues. It's recommended to use a tarball URL with a sha256 hash instead.

Apply this diff to improve build reproducibility:

-  git_url: https://github.com/genome/bam-readcount.git
-  git_rev: v{{ version }}
+  url: https://github.com/genome/bam-readcount/archive/v{{ version }}.tar.gz
+  sha256: # Add the appropriate sha256 hash here

Committable suggestion skipped: line range outside the PR's diff.


build:
number: 2
number: 3
run_exports:
- {{ pin_subpackage('bam-readcount', max_pin="x") }}

Expand All @@ -20,6 +20,9 @@ requirements:
- cmake
- {{ compiler('cxx') }}
- {{ compiler('c') }}
- autoconf
- automake
- libtool
host:
- zlib
- pthread-stubs
Expand All @@ -39,11 +42,15 @@ about:
license_file: LICENSE
summary: "bam-readcount generates metrics at single nucleotide positions."
dev_url: "https://github.com/genome/bam-readcount"
doc_url: "https://github.com/genome/bam-readcount/blob/master/README.md"
doc_url: "https://github.com/genome/bam-readcount/blob/v{{ version }}/README.md"

extra:
additional-platforms:
- linux-aarch64
- osx-arm64
identifiers:
- doi:10.21105/joss.03722
- biotools:bam-readcount
skip-lints:
- uses_vcs_url
- missing_hash
Comment on lines +54 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove skip-lints by fixing the source configuration

These lint skips are only necessary because of the git source usage. They can be removed by switching back to a tarball URL with sha256 hash as suggested earlier.

-  skip-lints:
-    - uses_vcs_url
-    - missing_hash
📝 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
skip-lints:
- uses_vcs_url
- missing_hash

Loading