Update MuSE to latest version 2.1.2 #47957
Conversation
|
@mencian did you manage to build this locally? Are you planning to continue this? Or do you have an idea why the build is still failing? |
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
It builds boost, gperftools and htslib without passing |
|
@jiyunmaths Maybe you can help here as I am not that familiar with cpp. @maxulysse Pinging because you might know who to ask. |
|
@martin-g How do we not use the install-muse.sh file when we use the tar from the release? |
You will need to add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
recipes/muse/build.sh (2)
10-15: Update debug output sectionIf
LIBPATHis removed as suggested above, update the debug section accordingly.Apply this diff:
# Print environment variables for debugging echo "Print env variables" -echo "LIBPATH: ${LIBPATH}" echo "LDFLAGS: ${LDFLAGS}" echo "CFLAGS: ${CFLAGS}"
23-28: Enhance installation verificationAdd checks to ensure successful installation:
- Verify MuSE exists before chmod/copy
- Verify successful installation
Apply this diff:
# List the contents of the PREFIX directory to see where files are placed echo "Listing contents of ${PREFIX}:" ls -R "${PREFIX}" +# Verify MuSE exists +if [ ! -f MuSE ]; then + echo "Error: MuSE executable not found" + exit 1 +fi + chmod 0755 MuSE cp -f MuSE "${PREFIX}/bin" + +# Verify successful installation +if [ ! -x "${PREFIX}/bin/MuSE" ]; then + echo "Error: MuSE installation failed" + exit 1 +fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
recipes/muse/build.sh(1 hunks)recipes/muse/fix-install_muse.sh.patch(1 hunks)recipes/muse/meta.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/muse/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (11)
recipes/muse/build.sh (2)
16-16: Verify Boost directory cleanup
The cleanup of boost_1_70_0 directory seems out of place:
- There's no prior extraction of Boost in this script
- The version might be outdated as we're updating MuSE to 2.1.2
#!/bin/bash
# Check if Boost is handled in install_muse.sh
rg -A 5 "boost_" install_muse.sh21-21: Review install_muse.sh dependencies and build process
Based on PR discussions:
- The script requires refinement
- Build failures were reported
- Consider not relying on
install_muse.shand instead:- Add Boost, gperftools, and htslib as build/host dependencies
- Execute only the final build steps directly
recipes/muse/fix-install_muse.sh.patch (2)
38-44: 🛠️ Refactor suggestion
Verify htslib version compatibility and build configuration
The update to htslib 1.21 is significant. Please ensure:
- This version is compatible with MuSE 2.1.2
- Consider enabling submodule initialization if required
- Add
--prefix="${PREFIX}"to the configure step
#!/bin/bash
# Check MuSE's htslib version requirements
rg -A 2 'htslib.*dependency' recipes/muse/meta.yaml-./configure
+./configure --prefix="${PREFIX}"51-58: Verify impact of removing cleanup steps
The removal of cleanup steps might affect:
- The final package size
- The presence of unnecessary build artifacts
- The conda-build process
Consider if these cleanup steps should be moved to the conda-build cleanup phase instead.
#!/bin/bash
# Check if cleanup is handled in conda-build
rg -A 2 'clean:|cleanup:' recipes/muse/meta.yamlrecipes/muse/meta.yaml (7)
20-22: LGTM! Good addition of run_exports
The addition of run_exports with appropriate pinning will help prevent ABI breakage issues, which aligns well with the PR objectives.
50-51: Consider enhancing test coverage as previously suggested
The existing test only verifies the version output. A previous review comment suggested adding more comprehensive tests, which is still valid.
54-60: LGTM! Well-documented package information
The package information is complete with proper license details and documentation links. The URLs correctly point to the new repository location as mentioned in the PR objectives.
62-65: LGTM! Good addition of DOI identifiers
The addition of DOI identifiers is excellent for academic citation tracking.
13-17: Verify patch files and Boost compatibility
A few points to consider:
- Ensure the patch files are available in the recipe directory
- Verify Boost 1.86.0 compatibility with MuSE 2.1.2
#!/bin/bash
# Description: Check if patch files exist and verify their content
fd -t f ".*\.patch$" recipes/muse/
# Check patch content
for patch in recipes/muse/*.patch; do
echo "=== $patch ==="
cat "$patch"
done1-3: Verify SHA256 checksum for MuSE v2.1.2
The SHA256 checksum differs from the one previously suggested in the review comments. Let's verify it matches the source archive.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
28-47: Dependencies look comprehensive and well-structured
The dependencies align well with the build requirements discussed in the PR comments, particularly the switch from using install-muse.sh to manual building with autotools.
✅ Verification successful
Based on the verification results, I can now provide the final response:
Dependencies are available and properly configured
All required dependencies are available in the conda channels (bioconda/conda-forge) with platform-specific configurations correctly set:
- Platform-specific OpenMP implementations (llvm-openmp for OSX, libgomp for Linux)
- Core dependencies (htslib, bzip2, xz, libcurl) are available in both platforms
- Bioinformatics tools (bwa, samtools, picard-slim, gatk) are present in bioconda
- Build dependencies (autoconf, automake, libtool) support the manual build approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify dependency availability in Bioconda
# Check if all dependencies are available in conda-forge or bioconda
deps=(
"llvm-openmp"
"libgomp"
"libcurl"
"htslib"
"bzip2"
"xz"
"cutadapt"
"bwa"
"samtools"
"picard-slim"
"gatk"
)
for dep in "${deps[@]}"; do
echo "Checking $dep..."
curl -s "https://conda.anaconda.org/bioconda/noarch/repodata.json" | jq -r ".packages | keys[] | select(startswith(\"$dep\"))"
curl -s "https://conda.anaconda.org/conda-forge/noarch/repodata.json" | jq -r ".packages | keys[] | select(startswith(\"$dep\"))"
done
Length of output: 9166
Script:
#!/bin/bash
# Let's check the linux64 and osx64 channels since noarch didn't show all packages
for dep in "llvm-openmp" "libgomp" "libcurl" "htslib" "bzip2" "xz" "bwa" "samtools"; do
echo "Checking $dep..."
curl -s "https://conda.anaconda.org/bioconda/linux-64/repodata.json" | jq -r ".packages | keys[] | select(startswith(\"$dep\"))" | head -n 5
curl -s "https://conda.anaconda.org/conda-forge/linux-64/repodata.json" | jq -r ".packages | keys[] | select(startswith(\"$dep\"))" | head -n 5
curl -s "https://conda.anaconda.org/bioconda/osx-64/repodata.json" | jq -r ".packages | keys[] | select(startswith(\"$dep\"))" | head -n 5
curl -s "https://conda.anaconda.org/conda-forge/osx-64/repodata.json" | jq -r ".packages | keys[] | select(startswith(\"$dep\"))" | head -n 5
echo "---"
done
Length of output: 3081
Script:
#!/bin/bash
# Continue checking remaining packages in platform-specific channels
for dep in "htslib" "bzip2" "xz" "bwa" "samtools"; do
echo "Checking $dep in linux-64..."
curl -s "https://conda.anaconda.org/bioconda/linux-64/repodata.json" | jq -r ".packages | keys[] | select(startswith(\"$dep\"))" | head -n 3
echo "Checking $dep in osx-64..."
curl -s "https://conda.anaconda.org/bioconda/osx-64/repodata.json" | jq -r ".packages | keys[] | select(startswith(\"$dep\"))" | head -n 3
echo "---"
done
# Also check if there are any version constraints in meta.yaml
echo "Checking version constraints in meta.yaml:"
cat recipes/muse/meta.yaml | grep -A20 "requirements:"
Length of output: 3338
recipes/muse/build.sh
Outdated
| ln -sf ${CC} ${PREFIX}/bin/gcc | ||
| ln -sf ${CXX} ${PREFIX}/bin/g++ |
There was a problem hiding this comment.
Remove compiler symlinks
Creating symlinks to compilers in ${PREFIX}/bin is not recommended:
- Could interfere with system compilers
- May cause issues with other packages
- Not a standard practice in Conda recipes
Remove these lines:
-ln -sf ${CC} ${PREFIX}/bin/gcc
-ln -sf ${CXX} ${PREFIX}/bin/g++Instead, ensure install_muse.sh uses ${CC} and ${CXX} directly.
📝 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.
| ln -sf ${CC} ${PREFIX}/bin/gcc | |
| ln -sf ${CXX} ${PREFIX}/bin/g++ |
| export LIBPATH="-L${PREFIX}/lib" | ||
| export LDFLAGS="${LDFLAGS} -L${PREFIX}/lib" | ||
| export CFLAGS="${CFLAGS} -O3" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize environment variable setup
LIBPATHis redundant since the library path is already included inLDFLAGS- Explicit
-O3flag should be removed as Conda-build already sets appropriate optimization flags
Apply this diff:
-export LIBPATH="-L${PREFIX}/lib"
-export LDFLAGS="${LDFLAGS} -L${PREFIX}/lib"
-export CFLAGS="${CFLAGS} -O3"
+export LDFLAGS="${LDFLAGS} -L${PREFIX}/lib"📝 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.
| export LIBPATH="-L${PREFIX}/lib" | |
| export LDFLAGS="${LDFLAGS} -L${PREFIX}/lib" | |
| export CFLAGS="${CFLAGS} -O3" | |
| export LDFLAGS="${LDFLAGS} -L${PREFIX}/lib" |
| cd gperftools | ||
| -git checkout gperftools-2.9.1 | ||
| +git checkout gperftools-2.16 | ||
| ./autogen.sh | ||
| ./configure --libdir="$PWD" | ||
| -make -j4 | ||
| +make -j${CPU_COUNT} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using --prefix for gperftools configure
While updating to gperftools 2.16 is good, the configure step could be improved:
- Consider using
--prefix="${PREFIX}"instead of--libdir="$PWD" - The current approach might cause installation issues in the conda environment
-./configure --libdir="$PWD"
+./configure --prefix="${PREFIX}"📝 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.
| cd gperftools | |
| -git checkout gperftools-2.9.1 | |
| +git checkout gperftools-2.16 | |
| ./autogen.sh | |
| ./configure --libdir="$PWD" | |
| -make -j4 | |
| +make -j${CPU_COUNT} | |
| cd gperftools | |
| git checkout gperftools-2.16 | |
| ./autogen.sh | |
| ./configure --prefix="${PREFIX}" | |
| make -j${CPU_COUNT} |
| +CFLAGS="${CFLAGS} -O3 ${LDFLAGS}" \ | ||
| + CC="${CC}" CXX="${CXX}" CXXFLAGS="${CXXFLAGS} -O3 -I${PREFIX}/include" \ | ||
| + LDFLAGS="${LDFLAGS} -L${PREFIX}/lib" \ | ||
| + make -j"${CPU_COUNT}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve compiler flags handling
The current approach of setting compiler flags could be improved:
- Avoid duplicating
-O3in multiple variables - Consider using
CPPFLAGSfor include paths - Ensure optimization flags don't conflict with conda-forge's defaults
-CFLAGS="${CFLAGS} -O3 ${LDFLAGS}" \
- CC="${CC}" CXX="${CXX}" CXXFLAGS="${CXXFLAGS} -O3 -I${PREFIX}/include" \
- LDFLAGS="${LDFLAGS} -L${PREFIX}/lib" \
- make -j"${CPU_COUNT}"
+CPPFLAGS="-I${PREFIX}/include" \
+ CFLAGS="${CFLAGS}" \
+ CXXFLAGS="${CXXFLAGS}" \
+ LDFLAGS="${LDFLAGS} -L${PREFIX}/lib" \
+ make -j"${CPU_COUNT}"📝 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.
| +CFLAGS="${CFLAGS} -O3 ${LDFLAGS}" \ | |
| + CC="${CC}" CXX="${CXX}" CXXFLAGS="${CXXFLAGS} -O3 -I${PREFIX}/include" \ | |
| + LDFLAGS="${LDFLAGS} -L${PREFIX}/lib" \ | |
| + make -j"${CPU_COUNT}" | |
| CPPFLAGS="-I${PREFIX}/include" \ | |
| CFLAGS="${CFLAGS}" \ | |
| CXXFLAGS="${CXXFLAGS}" \ | |
| LDFLAGS="${LDFLAGS} -L${PREFIX}/lib" \ | |
| make -j"${CPU_COUNT}" |
| +cd boost-1.86.0/ | ||
| +./bootstrap.sh --with-toolset=gcc | ||
| ./b2 --clean | ||
| -./b2 | ||
| +./b2 --prefix="${BOOST_ROOT}" toolset=gcc --layout=system link=static \ | ||
| + threading=multi strip=on include="${PREFIX}/include" \ | ||
| + variant=release address-model=64 install "${BOOST_BUILD_LIBS}" > "${BOOST_LOG}" 2>&1 |
There was a problem hiding this comment.
💡 Codebase verification
Environment variables need to be defined before Boost build
The build script is missing the definition of critical Boost-related environment variables (BOOST_ROOT, BOOST_BUILD_LIBS, BOOST_LOG) that are used in the patch. These variables must be defined before running the Boost build process to ensure proper installation. Please add the following to the build script before the Boost build section:
export BOOST_ROOT="${PREFIX}"
export BOOST_BUILD_LIBS="--with-system --with-thread --with-program_options"
export BOOST_LOG="${PWD}/boost_build.log"- The
PREFIXvariable is properly defined and used throughout the build process - The static linking and multi-threading options in the patch are correct for the package
🔗 Analysis chain
Verify environment variables and build configuration
The Boost build configuration has been improved, but please ensure:
${BOOST_ROOT},${PREFIX},${BOOST_BUILD_LIBS}, and${BOOST_LOG}are properly defined- The static linking and multi-threading options align with the package requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these variables are defined in the build environment
rg -A 1 'export .*BOOST_(ROOT|BUILD_LIBS|LOG)' recipes/muse/
rg -A 1 'PREFIX=' recipes/muse/
Length of output: 217
Script:
#!/bin/bash
# Let's check the entire install script content
cat recipes/muse/install_muse.sh || cat recipes/muse/build.sh
# Also check meta.yaml for any relevant variables
cat recipes/muse/meta.yaml
# Search with simpler patterns
rg "BOOST_" recipes/muse/
rg "PREFIX" recipes/muse/
Length of output: 3686
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
recipes/muse/meta.yaml (1)
34-49: Consider adding recommended bioconda constraints.For better compatibility in the bioconda ecosystem:
- Add
pythonas a host requirement (even if not directly used) to ensure conda-build sets up the environment correctly- Pin samtools and bwa versions using bioconda-utils' pinning system
host: - llvm-openmp # [osx] - libgomp # [linux] - libcurl - libdeflate - biobambam - bzip2 - xz - zlib + - python run: - llvm-openmp # [osx] - libgomp # [linux] - cutadapt - - bwa - - samtools + - bwa {{ bwa }} + - samtools {{ samtools }} - picard-slim - gatk
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
recipes/muse/fix-install_muse.sh.patch(1 hunks)recipes/muse/meta.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- recipes/muse/fix-install_muse.sh.patch
🧰 Additional context used
🪛 yamllint
recipes/muse/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (1)
recipes/muse/meta.yaml (1)
3-3: Verify SHA256 checksum.
The SHA256 checksum differs from the previously suggested value. Please verify the correct checksum for v2.1.2.
| - url: https://github.com/boostorg/boost/releases/download/boost-1.86.0/boost-1.86.0-b2-nodocs.tar.gz | ||
| sha256: 2128a4c96862b5c0970c1e34d76b1d57e4a1016b80df85ad39667f30b1deba26 | ||
| folder: boost-1.86.0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using conda-forge's boost package.
Including Boost source directly in the recipe is unusual. Conda-forge provides pre-built Boost packages that are better maintained and tested. Consider using boost-cpp from conda-forge instead.
- - url: https://github.com/boostorg/boost/releases/download/boost-1.86.0/boost-1.86.0-b2-nodocs.tar.gz
- sha256: 2128a4c96862b5c0970c1e34d76b1d57e4a1016b80df85ad39667f30b1deba26
- folder: boost-1.86.0Then add to host requirements:
host:
- boost-cpp
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
recipes/muse/meta.yaml (2)
13-14: Document the purpose of patches.Please add comments explaining what each patch does:
0001-Makefile.patchfix-install_muse.sh.patch
44-48: Consider adding version constraints for bioinformatics tools.Critical bioinformatics tools should have version constraints to ensure compatibility:
- cutadapt >=4.0 - bwa >=0.7.17 - samtools >=1.15 - picard-slim >=2.27 - gatk >=4.3recipes/muse/0001-Makefile.patch (1)
25-26: LGTM! Improved include path handling.Good changes that:
- Preserve existing paths by using
+=- Explicitly include htslib
- Use
$(PREFIX)for package manager compatibilityConsider adding a comment explaining why htslib needs a separate include path:
-INCLUDES += -Iinc/ -Iinc/htslib -I$(PREFIX)/include +# Explicit htslib include needed for header file resolution +INCLUDES += -Iinc/ -Iinc/htslib -I$(PREFIX)/include
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
recipes/muse/0001-Makefile.patch(1 hunks)recipes/muse/fix-install_muse.sh.patch(1 hunks)recipes/muse/meta.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- recipes/muse/fix-install_muse.sh.patch
🧰 Additional context used
🪛 yamllint
recipes/muse/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (3)
recipes/muse/meta.yaml (1)
3-3: Verify the SHA256 checksum.
The SHA256 checksum appears to be incorrect. A previous review suggested a different value:
0767f6614a6f26146beffa3546eb7c1a7c2f87bd21d6001ac2e5d157182fe14c
recipes/muse/0001-Makefile.patch (2)
6-11: LGTM! Improved compiler configuration.
The change from hardcoded compiler commands to environment variables (${CXX} and ${CC}) is a good practice that:
- Improves cross-platform compatibility
- Allows build system control over compiler selection
- Aligns with Bioconda's build requirements
34-35: LGTM! Enhanced library configuration.
Good improvements:
- Added libdeflate dependency
- Added $(PREFIX)/lib path
- Preserved existing paths with +=
✅ Verification successful
Verification Successful: libdeflate correctly added and referenced.
libdeflateis present inrecipes/muse/meta.yaml.- Consistent addition across multiple recipes ensures proper dependency management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify libdeflate is listed as a dependency in meta.yaml
rg "libdeflate" recipes/muse/meta.yaml
# Check if other recipes using htslib also link against libdeflate
rg -l "libdeflate" --type yaml recipes/
Length of output: 1825
recipes/muse/0001-Makefile.patch
Outdated
| -LINKLINE = $(LINK) -O3 -g -o $(TARGET) $(OBJS) $(COMMONOBJS) $(MATCHOBJS) $(LIBS) | ||
| +LINKLINE = $(LINK) -O3 -g -o $(TARGET) $(OBJS) $(COMMONOBJS) $(MATCHOBJS) $(INCLUDES) $(LIBS) |
There was a problem hiding this comment.
Remove incorrect library flags from compilation commands.
There are issues with the compilation commands:
- $(INCLUDES) is redundant as it's already included via COMMONFLAGS
- $(LIBS) should not be used during compilation, only during linking
Apply this fix:
-LINKLINE = $(LINK) -O3 -g -o $(TARGET) $(OBJS) $(COMMONOBJS) $(MATCHOBJS) $(INCLUDES) $(LIBS)
+LINKLINE = $(LINK) -O3 -g -o $(TARGET) $(OBJS) $(COMMONOBJS) $(MATCHOBJS) $(LIBS)
- $(CC) $(CFLAGS) $(RELEASE_FLAGS) $(INCLUDES) $(LIBS) -c $< -o $@
+ $(CC) $(CFLAGS) $(RELEASE_FLAGS) -c $< -o $@
- $(CPP) $(CPPFLAGS) $(RELEASE_FLAGS) $(OPENMP) $(INCLUDES) $(LIBS) -c $< -o $@
+ $(CPP) $(CPPFLAGS) $(RELEASE_FLAGS) $(OPENMP) -c $< -o $@Explanation:
- Remove redundant $(INCLUDES) as it's already in COMMONFLAGS
- Remove $(LIBS) from compilation as libraries should only be linked in the final step
Also applies to: 50-51, 54-55
| -CPPFLAGS= $(WarningsAsErrors) -Wno-unused-function -std=c++11 | ||
| +CPPFLAGS= $(WarningsAsErrors) -Wno-unused-function -std=c++14 |
There was a problem hiding this comment.
💡 Codebase verification
C++14 compatibility issues detected with dependencies.
recipes/muse/meta.yamlspecifieslibdeflatewhich may require C++11.
🔗 Analysis chain
Verify C++14 compatibility with dependencies.
The upgrade from C++11 to C++14 standard might affect compatibility. Please ensure:
- All dependencies support C++14
- This change is required/recommended for MuSE 2.1.2
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any dependencies explicitly require C++11 or have C++14 compatibility issues
rg -l "std=(c\+\+|c)\s*11" --type cpp
Length of output: 1825
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
recipes/muse/fix-install_muse.sh.patch(1 hunks)recipes/muse/meta.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- recipes/muse/fix-install_muse.sh.patch
🧰 Additional context used
🪛 yamllint
recipes/muse/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (4)
recipes/muse/meta.yaml (4)
1-7: LGTM! Package metadata is well structured.
The use of Jinja2 variables and proper name formatting follows best practices.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
54-65: LGTM! Comprehensive package metadata.
The about section provides complete information including:
- Updated repository URLs
- Proper license information
- DOI identifiers
13-14: Verify patch contents and necessity.
The patches suggest modifications to the build system. Given the discussions about build issues in the PR, let's verify these patches.
#!/bin/bash
# Description: Examine patch contents to understand build modifications
# Check patch contents
for patch in recipes/muse/*.patch; do
echo "=== Contents of $patch ==="
cat "$patch"
done
# Look for related Makefile modifications in other recipes
rg -l "Makefile.patch" recipes/ | grep -v "muse"20-22: LGTM! Proper version pinning strategy.
The run_exports configuration will help prevent ABI breakage issues as mentioned in the PR objectives.
recipes/muse/meta.yaml
Outdated
| - {{ compiler('c') }} | ||
| - make | ||
| - autoconf | ||
| - automake | ||
| - libtool | ||
| host: | ||
| - llvm-openmp # [osx] | ||
| - libgomp # [linux] | ||
| - libcurl | ||
| - icu | ||
| - bzip2 | ||
| - xz | ||
| - zlib | ||
| run: | ||
| - zlib | ||
| - llvm-openmp # [osx] | ||
| - libgomp # [linux] | ||
| - cutadapt | ||
| - bwa | ||
| - samtools | ||
| - picard-slim | ||
| - gatk |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing dependencies mentioned in PR discussion.
Based on the PR discussion, consider adding these dependencies:
htslibin host sectiongperftoolsin host section
host:
- llvm-openmp # [osx]
- libgomp # [linux]
- libcurl
- icu
- bzip2
- xz
- zlib
+ - htslib
+ - gperftools📝 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.
| - {{ compiler('c') }} | |
| - make | |
| - autoconf | |
| - automake | |
| - libtool | |
| host: | |
| - llvm-openmp # [osx] | |
| - libgomp # [linux] | |
| - libcurl | |
| - icu | |
| - bzip2 | |
| - xz | |
| - zlib | |
| run: | |
| - zlib | |
| - llvm-openmp # [osx] | |
| - libgomp # [linux] | |
| - cutadapt | |
| - bwa | |
| - samtools | |
| - picard-slim | |
| - gatk | |
| - {{ compiler('c') }} | |
| - make | |
| - autoconf | |
| - automake | |
| - libtool | |
| host: | |
| - llvm-openmp # [osx] | |
| - libgomp # [linux] | |
| - libcurl | |
| - icu | |
| - bzip2 | |
| - xz | |
| - zlib | |
| - htslib | |
| - gperftools | |
| run: | |
| - llvm-openmp # [osx] | |
| - libgomp # [linux] | |
| - cutadapt | |
| - bwa | |
| - samtools | |
| - picard-slim | |
| - gatk |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
recipes/muse/build.sh (2)
8-8: Add error handling and documentation for Boost cleanupThe cleanup of
boost_1_70_0directory should be more robust and documented.Consider this improvement:
-rm -rf boost_1_70_0/ +# Clean up old Boost if it exists (required for version upgrade from 1.70.0) +if [ -d "boost_1_70_0" ]; then + rm -rf boost_1_70_0/ +fi
28-29: Add error handling for final installationThe installation commands should include verification and error handling.
Apply this improvement:
+if [ ! -f "MuSE" ]; then + echo "Error: MuSE executable not found" + exit 1 +fi + install -d "${PREFIX}/bin" -install -v -m 0755 MuSE "${PREFIX}/bin" +install -v -m 0755 MuSE "${PREFIX}/bin" || { + echo "Error: Failed to install MuSE" + exit 1 +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
recipes/muse/build.sh(1 hunks)recipes/muse/fix-install_muse.sh.patch(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- recipes/muse/fix-install_muse.sh.patch
🔇 Additional comments (2)
recipes/muse/build.sh (2)
1-2: LGTM: Proper error handling setup
The script correctly implements robust error handling with set -euo pipefail after the shebang line.
4-6: 🛠️ Refactor suggestion
Simplify environment variable configuration
The current environment setup can be improved:
LIBPATHis redundant since library paths are already handled byLDFLAGS- Conda-build manages optimization flags, so explicit
-O3should be removed - Consider using
${BUILD_PREFIX}/libinLDFLAGSfor build-time dependencies
Apply this improvement:
-export LIBPATH="-L${PREFIX}/lib"
-export LDFLAGS="${LDFLAGS} -L${PREFIX}/lib"
-export CFLAGS="${CFLAGS} -O3"
+export LDFLAGS="${LDFLAGS} -L${BUILD_PREFIX}/lib -L${PREFIX}/lib"Likely invalid or redundant comment.
| ln -sf ${CXX} ${PREFIX}/bin/g++ | ||
| fi | ||
|
|
||
| ./install_muse.sh |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for install script execution
The installation script execution needs better error handling and verification.
Apply this improvement:
-./install_muse.sh
+if [ ! -f "install_muse.sh" ]; then
+ echo "Error: install_muse.sh not found"
+ exit 1
+fi
+
+if [ ! -x "install_muse.sh" ]; then
+ chmod +x install_muse.sh
+fi
+
+./install_muse.sh || exit 1📝 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.
| ./install_muse.sh | |
| if [ ! -f "install_muse.sh" ]; then | |
| echo "Error: install_muse.sh not found" | |
| exit 1 | |
| fi | |
| if [ ! -x "install_muse.sh" ]; then | |
| chmod +x install_muse.sh | |
| fi | |
| ./install_muse.sh || exit 1 |
| if [[ `uname` == "Darwin" ]]; then | ||
| rm -rf ${PREFIX}/bin/clang | ||
| rm -rf ${PREFIX}/bin/clang++ | ||
| else | ||
| rm -rf ${PREFIX}/bin/gcc | ||
| rm -rf ${PREFIX}/bin/g++ | ||
| fi |
There was a problem hiding this comment.
Remove symlinks cleanup block
This cleanup block should be removed along with the symlinks creation block above.
Remove this entire block as it's related to the dangerous practice of creating compiler symlinks:
-if [[ `uname` == "Darwin" ]]; then
- rm -rf ${PREFIX}/bin/clang
- rm -rf ${PREFIX}/bin/clang++
-else
- rm -rf ${PREFIX}/bin/gcc
- rm -rf ${PREFIX}/bin/g++
-fi📝 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.
| if [[ `uname` == "Darwin" ]]; then | |
| rm -rf ${PREFIX}/bin/clang | |
| rm -rf ${PREFIX}/bin/clang++ | |
| else | |
| rm -rf ${PREFIX}/bin/gcc | |
| rm -rf ${PREFIX}/bin/g++ | |
| fi |
| if [[ `uname` == "Darwin" ]]; then | ||
| ln -sf ${CC} ${PREFIX}/bin/clang | ||
| ln -sf ${CXX} ${PREFIX}/bin/clang++ | ||
| else | ||
| ln -sf ${CC} ${PREFIX}/bin/gcc | ||
| ln -sf ${CXX} ${PREFIX}/bin/g++ | ||
| fi |
There was a problem hiding this comment.
Remove compiler symlinks - Critical Security Risk
Creating compiler symlinks in ${PREFIX}/bin is dangerous and non-standard:
- Could interfere with system compilers
- Potential security risk by modifying system paths
- Not a standard practice in Conda recipes
Instead of creating symlinks, modify install_muse.sh to use ${CC} and ${CXX} directly. Remove this entire block and update install_muse.sh to use environment variables:
-if [[ `uname` == "Darwin" ]]; then
- ln -sf ${CC} ${PREFIX}/bin/clang
- ln -sf ${CXX} ${PREFIX}/bin/clang++
-else
- ln -sf ${CC} ${PREFIX}/bin/gcc
- ln -sf ${CXX} ${PREFIX}/bin/g++
-fiCommittable suggestion skipped: line range outside the PR's diff.
|
Finally able to build |
|
@mencian Thanks you so much 🥳 |
Updated the recipe for MuSE to the latest version 2.1 and changed the repository to its new location: https://github.com/wwylab/MuSE
Please read the guidelines for Bioconda recipes before opening a pull request (PR).
General instructions
@BiocondaBot please add labelcommand.@bioconda/corein a comment.Instructions for avoiding API, ABI, and CLI breakage issues
Conda is able to record and lock (a.k.a. pin) dependency versions used at build time of other recipes.
This way, one can avoid that expectations of a downstream recipe with regards to API, ABI, or CLI are violated by later changes in the recipe.
If not already present in the meta.yaml, make sure to specify
run_exports(see here for the rationale and comprehensive explanation).Add a
run_exportssection like this:with
...being one of:{{ pin_subpackage("myrecipe", max_pin="x") }}{{ pin_subpackage("myrecipe", max_pin="x.x") }}{{ pin_subpackage("myrecipe", max_pin="x.x") }}(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin="x.x.x") }}(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin=None) }}while replacing
"myrecipe"with eithernameif aname|lowervariable is defined in your recipe or with the lowercase name of the package in quotes.Bot commands for PR management
Please use the following BiocondaBot commands:
Everyone has access to the following BiocondaBot commands, which can be given in a comment:
@BiocondaBot please update@BiocondaBot please add labelplease review & mergelabel.@BiocondaBot please fetch artifactsYou can use this to test packages locally.
Note that the
@BiocondaBot please mergecommand is now depreciated. Please just squash and merge instead.Also, the bot watches for comments from non-members that include
@bioconda/<team>and will automatically re-post them to notify the addressed<team>.