cd-hit: add osx-arm64 build #52061
cd-hit: add osx-arm64 build #52061
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several modifications across various files related to the Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
![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 (4)
recipes/cd-hit/build.sh (2)
1-4: LGTM! Minor spacing improvement suggested.The switch to bash and early directory creation are good practices. Consider removing the empty line after mkdir for consistency.
#!/bin/bash mkdir -p $PREFIX/bin -
11-13: Add error checking for sed commands.The Makefile modifications could fail silently if the patterns don't match. Consider adding error checking.
-sed -i.bak 's/^CC =$//g' Makefile -sed -i.bak 's/^#LDFLAGS.*//g' Makefile -rm -rf *.bak +if ! sed -i.bak 's/^CC =$//g' Makefile; then + echo "Error: Failed to modify CC in Makefile" + exit 1 +fi +if ! sed -i.bak 's/^#LDFLAGS.*//g' Makefile; then + echo "Error: Failed to modify LDFLAGS in Makefile" + exit 1 +fi +rm -f *.bakrecipes/cd-hit/cd-hit.patch (2)
618-776: Well-structured new script with room for improvementThe new
OTU_2_taxon_table.plscript is well-documented and includes proper error handling. However, a few improvements could make it even better:Consider these enhancements:
- Add
use warningsfor better error detection- Extract magic numbers (like array indices 0..6) into named constants
- Consider adding input validation for the taxonomy format
#!/usr/bin/env perl +use warnings; use Getopt::Std; + +# Constants for taxonomy levels +use constant { + KINGDOM => 0, + PHYLUM => 1, + CLASS => 2, + ORDER => 3, + FAMILY => 4, + GENUS => 5, + SPECIES => 6 +};
475-493: Add error handling for compression stepsWhile the addition of file compression is good for saving space, consider adding error handling for the compression operations.
Consider adding error checking:
-gzip $SELF/R1.fa & -gzip $SELF/R2.fa & +if ! gzip $SELF/R1.fa; then + echo "Error compressing R1.fa" >&2 + exit 1 +fi & +if ! gzip $SELF/R2.fa; then + echo "Error compressing R2.fa" >&2 + exit 1 +fi & wait
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
recipes/cd-hit/build.sh(1 hunks)recipes/cd-hit/cd-hit.patch(1 hunks)recipes/cd-hit/meta.yaml(2 hunks)
🔇 Additional comments (10)
recipes/cd-hit/build.sh (2)
1-28: Verify arm64 compatibility settings.
Since this PR aims to add osx-arm64 support, we should verify that all necessary architecture-specific settings are in place.
#!/bin/bash
# Check for any arm64-specific settings or known issues
echo "Checking for arm64-specific configurations..."
rg -i "arm64|aarch64" --type yaml
rg -i "darwin.*arm|osx.*arm" --type sh
# Check if similar packages have special handling for arm64
echo "Checking other bioconda recipes for arm64 patterns..."
fd "meta.yaml" recipes/ --exec rg -l "osx-arm64"15-26:
Fix compiler variable inconsistency and clarify OpenMP support.
- The else branch uses
$GXXwhich seems incorrect - should be$CXXfor consistency - The OpenMP comment needs clarification about the status of the fix
if [[ "$OSTYPE" == "darwin"* ]]; then
- #Lines below is commented out until fix provided for OPENMP support on OS X for this program
+ # TODO: OpenMP support is currently disabled on macOS due to compatibility issues
+ # Track progress at: <insert issue URL>
CCFLAGS="$CCFLAGS -Wl,-rpath ${PREFIX}/lib -L${PREFIX}/lib -I${PREFIX}/include -fopenmp"
sed -i.bak 's/CCFLAGS = -fopenmp/CCFLAGS += -fopenmp/g' Makefile
rm -rf *.bak
LDFLAGS="$LDFLAGS -stdlib=libc++"
make CC=$CXX openmp=no MAX_SEQ=1000000
else
- make CC=$GXX MAX_SEQ=1000000
+ make CC=$CXX MAX_SEQ=1000000
fiLet's verify if OpenMP support has been fixed in recent versions:
✅ Verification successful
Let me verify if OpenMP support has been fixed for macOS by checking the build script and related files.
Based on the output, I can now generate the final response:
Update OpenMP configuration comment and fix compiler variable inconsistency
- The
$GXXvariable in the else branch should be$CXXfor consistency - The OpenMP comment should be updated to reflect the current status:
if [[ "$OSTYPE" == "darwin"* ]]; then
- #Lines below is commented out until fix provided for OPENMP support on OS X for this program
+ # OpenMP is explicitly disabled on macOS due to compatibility issues with clang
+ # llvm-openmp is included as a dependency but not used to ensure stable builds
CCFLAGS="$CCFLAGS -Wl,-rpath ${PREFIX}/lib -L${PREFIX}/lib -I${PREFIX}/include -fopenmp"
sed -i.bak 's/CCFLAGS = -fopenmp/CCFLAGS += -fopenmp/g' Makefile
rm -rf *.bak
LDFLAGS="$LDFLAGS -stdlib=libc++"
make CC=$CXX openmp=no MAX_SEQ=1000000
else
- make CC=$GXX MAX_SEQ=1000000
+ make CC=$CXX MAX_SEQ=1000000
fiThe review comment is correct about the compiler variable inconsistency. The OpenMP situation is intentionally disabled on macOS as evidenced by both the openmp=no flag and the llvm-openmp dependency in meta.yaml. This is a deliberate design choice for stability rather than a pending fix.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any recent commits or issues mentioning OpenMP support for macOS
rg -i "openmp.*darwin|openmp.*osx|openmp.*macos" --type md
Length of output: 216783
Script:
#!/bin/bash
# Check build.sh and related files for OpenMP configuration
rg -i "openmp" recipes/cd-hit/
# Check if there are any patches or configuration files related to OpenMP
fd -e patch -e config recipes/cd-hit/
Length of output: 878
recipes/cd-hit/meta.yaml (6)
15-17: LGTM! Build configuration follows best practices
The build number increment and run_exports pinning are correctly configured to maintain ABI compatibility.
44-45: LGTM! Platform support matches PR objective
The addition of osx-arm64 platform support directly addresses the PR objective.
26-27: LGTM! OpenMP dependencies are correctly configured
The platform-specific OpenMP dependencies are properly set up with correct selectors.
34-40: LGTM! Metadata updates improve package documentation
The metadata updates follow conda-forge standards with proper SPDX license identifier and additional URLs.
46-50: LGTM! Comprehensive identifier additions
The added identifiers improve package discoverability and provide valuable references to publications and tools.
11-12: Verify the patch file existence and contents
The patch file needs to be verified to ensure it's properly placed and contains the expected shebang modifications.
recipes/cd-hit/cd-hit.patch (2)
6-7: Improved portability with env-based shebang lines
The change from direct path (#!/usr/bin/perl) to env-based path (#!/usr/bin/env perl) improves script portability across different systems.
Also applies to: 16-17, 26-27, 36-37, 58-59, 68-69, 78-79, 88-89, 98-99, 108-109, 118-119, 128-129, 138-139, 148-149, 158-159, 168-169, 178-179, 188-189, 198-199, 208-209, 218-219, 228-229, 238-239, 248-249, 258-259, 268-269, 278-279, 288-289, 298-299, 308-309, 318-319, 328-329, 338-339, 348-349, 358-359, 368-369, 378-379, 388-389, 398-399, 417-418, 436-437, 446-447, 456-457, 466-467, 781-782, 915-916, 925-926, 935-936, 945-946, 955-956, 965-966, 978-979
42-49: Good bug fix for handling cd-hit-est-2d output format
The fix properly handles the additional format variations in cd-hit-est-2d output that includes +/- signs. The added comment clearly explains the issue.
| mkdir -p $PREFIX/bin | ||
| make install PREFIX=$PREFIX/bin | ||
|
|
||
| make install PREFIX="$PREFIX/bin" |
There was a problem hiding this comment.
Fix PREFIX path and add error handling.
The PREFIX should point to the root prefix directory, not directly to the bin directory. Also, add error checking for the installation step.
-make install PREFIX="$PREFIX/bin"
+if ! make install PREFIX="$PREFIX"; then
+ echo "Error: Installation failed"
+ exit 1
+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.
| make install PREFIX="$PREFIX/bin" | |
| if ! make install PREFIX="$PREFIX"; then | |
| echo "Error: Installation failed" | |
| exit 1 | |
| fi |
| export CFLAGS="${CFLAGS} -O3" | ||
| export CPPFLAGS="${CPPFLAGS} -I$PREFIX/include" | ||
| export CXXFLAGS="${CXXFLAGS} -O3 -I$PREFIX/include" | ||
| export LDFLAGS="${LDFLAGS} -L$PREFIX/lib" | ||
| export CPATH=${PREFIX}/include |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve environment variable handling.
The compiler flags setup looks good, but there are two improvements to consider:
- CPATH should be exported like other variables
- Variables should be quoted to handle paths with spaces
-export CFLAGS="${CFLAGS} -O3"
-export CPPFLAGS="${CPPFLAGS} -I$PREFIX/include"
-export CXXFLAGS="${CXXFLAGS} -O3 -I$PREFIX/include"
-export LDFLAGS="${LDFLAGS} -L$PREFIX/lib"
-export CPATH=${PREFIX}/include
+export CFLAGS="${CFLAGS} -O3"
+export CPPFLAGS="${CPPFLAGS} -I\"${PREFIX}/include\""
+export CXXFLAGS="${CXXFLAGS} -O3 -I\"${PREFIX}/include\""
+export LDFLAGS="${LDFLAGS} -L\"${PREFIX}/lib\""
+export CPATH="${PREFIX}/include"📝 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 CFLAGS="${CFLAGS} -O3" | |
| export CPPFLAGS="${CPPFLAGS} -I$PREFIX/include" | |
| export CXXFLAGS="${CXXFLAGS} -O3 -I$PREFIX/include" | |
| export LDFLAGS="${LDFLAGS} -L$PREFIX/lib" | |
| export CPATH=${PREFIX}/include | |
| export CFLAGS="${CFLAGS} -O3" | |
| export CPPFLAGS="${CPPFLAGS} -I\"${PREFIX}/include\"" | |
| export CXXFLAGS="${CXXFLAGS} -O3 -I\"${PREFIX}/include\"" | |
| export LDFLAGS="${LDFLAGS} -L\"${PREFIX}/lib\"" | |
| export CPATH="${PREFIX}/include" |
Describe your pull request here
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>.