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

Rebuild bam-readcount recipe #52212

Closed
wants to merge 4 commits into from
Closed

Rebuild bam-readcount recipe #52212

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
10c1b48
Rebuild bam-readcount recipe
mencian Nov 20, 2024
f88c7ce
Update meta.yaml
mencian Nov 20, 2024
81676be
Update meta.yaml
mencian Nov 20, 2024
70c23be
Update build.sh
mencian Nov 20, 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
22 changes: 8 additions & 14 deletions recipes/bam-readcount/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
#!/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

mkdir -p "${PREFIX}/bin"
mv boost-1.86.0-cmake.tar.gz vendor/boost-1.55-bamrc.tar.gz
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

Version mismatch in Boost tarball renaming needs attention

The script downloads Boost 1.86.0 but renames it to include "1.55" in the target filename. This version mismatch in the filename could lead to confusion and maintenance issues.

Consider:

  1. Using consistent version numbers
  2. Using variables for version numbers to make updates easier
-wget https://github.com/boostorg/boost/releases/download/boost-1.86.0/boost-1.86.0-cmake.tar.gz
-mv boost-1.86.0-cmake.tar.gz vendor/boost-1.55-bamrc.tar.gz
+BOOST_VERSION="1.86.0"
+wget https://github.com/boostorg/boost/releases/download/boost-${BOOST_VERSION}/boost-${BOOST_VERSION}-cmake.tar.gz
+mv boost-${BOOST_VERSION}-cmake.tar.gz vendor/boost-${BOOST_VERSION}-bamrc.tar.gz
📝 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
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
mkdir -p "${PREFIX}/bin"
mv boost-1.86.0-cmake.tar.gz vendor/boost-1.55-bamrc.tar.gz
BOOST_VERSION="1.86.0"
wget https://github.com/boostorg/boost/releases/download/boost-${BOOST_VERSION}/boost-${BOOST_VERSION}-cmake.tar.gz
mv boost-${BOOST_VERSION}-cmake.tar.gz vendor/boost-${BOOST_VERSION}-bamrc.tar.gz


# Needed for building utils dependency
export INCLUDES="-I{PREFIX}/include"
Expand All @@ -14,26 +12,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 bin/bam-readcount "$PREFIX/bin"
17 changes: 12 additions & 5 deletions recipes/bam-readcount/meta.yaml
View file
Open in desktop
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,11 +20,14 @@ requirements:
- cmake
- {{ compiler('cxx') }}
- {{ compiler('c') }}
- autoconf
- automake
- libtool
- clangdev
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Remove unnecessary build dependencies

The build.sh script shows that the package uses CMake for building, and there's no usage of autoconf, automake, libtool, or any clang-specific features. The build system:

  • Uses standard CMake configuration and build commands
  • Sets compiler via CMAKE_CXX_COMPILER and CMAKE_C_COMPILER variables
  • Doesn't require autotools (autoconf, automake, libtool)
  • Doesn't have any clang-specific requirements

The following dependencies should be removed as they are not used:

  • autoconf
  • automake
  • libtool
  • clangdev
🔗 Analysis chain

Verify the necessity of new build dependencies

Please confirm that these new build dependencies are actually used in the build process.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the new build dependencies are used in build.sh

# Look for autoconf/automake usage
echo "Checking for autoconf/automake usage:"
rg -l 'auto(conf|make|reconf|header)' recipes/bam-readcount/build.sh

# Look for libtool usage
echo "Checking for libtool usage:"
rg -l 'libtool' recipes/bam-readcount/build.sh

# Look for clang-specific usage
echo "Checking for clang usage:"
rg -l 'clang(\+\+)?|llvm' recipes/bam-readcount/build.sh

Length of output: 387

Script:

#!/bin/bash
# Let's first check the content of build.sh to understand the build process
echo "Content of build.sh:"
cat recipes/bam-readcount/build.sh

# Also check if there's a configure script or CMake files
echo -e "\nChecking for configure/CMake files:"
fd -t f "configure|CMakeLists.txt|autogen.sh" recipes/bam-readcount/

Length of output: 1323

host:
- zlib
- pthread-stubs
- wget
- clangdev
run:
- python

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