bayestyper: add arm64/aarch64 builds #52076
Conversation
mencian
added
aarch64
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant updates to the Possibly related PRs
Suggested labels
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 (6)
recipes/bayestyper/meta.yaml (1)
43-50: Consider enhancing test coverage while platform additions look goodThe addition of arm64 platforms aligns with the PR objectives. However, the test commands could be more comprehensive.
Consider adding these additional test aspects:
- Version number verification
- Basic functionality tests
- Help command verification
Example enhancement:
commands: - bayesTyper | grep "BayesTyper" - bayesTyperTools | grep "BayesTyperTools" + - bayesTyper --version | grep "{{ version }}" + - bayesTyper --help + - bayesTyperTools --helprecipes/bayestyper/bayestyper.patch (1)
24-25: Consider making debug messages conditional.While these debug messages are helpful for troubleshooting build issues (especially during arm64 port), consider making them conditional:
-message(STATUS "======= DEBUG Boost_INCLUDE_DIRS: " ${Boost_INCLUDE_DIRS}) -message(STATUS "======= DEBUG Boost_LIB_DIRS: " ${Boost_LIB_DIRS}) +if(CMAKE_DEBUG_OUTPUT) + message(STATUS "Boost_INCLUDE_DIRS: " ${Boost_INCLUDE_DIRS}) + message(STATUS "Boost_LIB_DIRS: " ${Boost_LIB_DIRS}) +endif()recipes/bayestyper/0001-CMakeLists.patch (3)
10-17: LGTM! Good handling of Boost path configuration.The added logic properly handles user-specified Boost paths, which is crucial for cross-compilation scenarios like arm64/aarch64. The conditions appropriately check both direct variables and environment variables.
For arm64/aarch64 builds, ensure that the Boost libraries are available in the build environment and their paths are correctly specified through these variables.
26-28: Consider conditionally enabling debug output.The build options are well-configured, particularly the use of dynamic libraries which is appropriate for conda packages. However, consider making the debug output conditional.
set( Boost_USE_STATIC_LIBS OFF ) set( Boost_USE_MULTITHREADED TRUE ) -set( Boost_DEBUG ON ) +option(BOOST_DEBUG "Enable Boost debug output" OFF) +set( Boost_DEBUG ${BOOST_DEBUG} )
1-45: Well-structured CMake configuration for cross-compilation.The overall approach to Boost configuration is robust and well-suited for cross-compilation scenarios. The combination of path handling, build options, and diagnostic output provides a solid foundation for arm64/aarch64 builds.
Consider adding a CI job specifically for testing arm64/aarch64 builds to catch any architecture-specific issues early. This could be implemented using GitHub Actions with arm64 runners or QEMU-based emulation.
recipes/bayestyper/build.sh (1)
5-9: Simplify environment variable settings for CMake buildsThe variables
INCLUDES,CPLUS_INCLUDE_PATH, andLIBPATHmay not be necessary for CMake builds. CMake usesCMAKE_INCLUDE_PATHandCMAKE_LIBRARY_PATHto locate headers and libraries. Consider setting these CMake variables directly or verify if these environment variables are required for this build.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
build-fail-blacklist(0 hunks)recipes/bayestyper/0001-CMakeLists.patch(1 hunks)recipes/bayestyper/bayestyper.patch(1 hunks)recipes/bayestyper/build.sh(1 hunks)recipes/bayestyper/kmer_defs.patch(1 hunks)recipes/bayestyper/meta.yaml(2 hunks)recipes/bayestyper/stdafx.patch(1 hunks)
💤 Files with no reviewable changes (1)
- build-fail-blacklist
✅ Files skipped from review due to trivial changes (1)
- recipes/bayestyper/stdafx.patch
🧰 Additional context used
🪛 yamllint
recipes/bayestyper/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (9)
recipes/bayestyper/kmer_defs.patch (1)
9-15: LGTM! Proper compiler-specific header handling for arm64 support.
The change correctly implements cross-compiler compatibility by:
- Using standard
<algorithm>for Clang which is common on arm64 - Maintaining GCC support with
<ext/algorithm>and explicitcopy_nimport
Let's verify the compiler detection works as expected:
recipes/bayestyper/meta.yaml (4)
17-20: LGTM! Good use of run_exports for ABI compatibility
The addition of run_exports with appropriate pinning helps maintain ABI compatibility across different architectures.
Line range hint 22-31: LGTM! Requirements are properly structured
The requirements are well-organized with appropriate separation between build, host, and run dependencies.
34-39: LGTM! Enhanced package metadata
Good improvements to the metadata section with additional URLs and proper license information.
11-15: Verify the necessity and impact of the added patches
The addition of multiple patches, especially platform-specific ones for OSX, warrants verification of their necessity and impact.
recipes/bayestyper/bayestyper.patch (2)
9-12: LGTM! Improved include statements for better cross-platform compatibility.
The change from quoted includes to angle brackets for Boost headers is a good practice for external dependencies and will help with consistent path resolution across different platforms, including arm64.
23-26: Verify arm64 build requirements.
While these changes improve build system portability, please verify:
- That Boost libraries are correctly detected on arm64 platforms
- Whether any additional platform-specific configurations are needed for arm64 support
recipes/bayestyper/0001-CMakeLists.patch (2)
30-41: Verify Boost version requirement for arm64/aarch64.
The Boost version requirement (1.71.0) needs verification for arm64/aarch64 compatibility. The configuration output is comprehensive and will help with debugging build issues.
Let's verify the Boost version availability in the conda-forge channel for arm64:
✅ Verification successful
Boost version requirement verified for arm64/aarch64.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Boost version availability in conda-forge
curl -s https://conda.anaconda.org/conda-forge/linux-aarch64/repodata.json | \
jq -r '.packages | to_entries[] | select(.key | contains("boost-")) | .value.version' | \
sort -V | uniq
Length of output: 430
19-24: LGTM! Proper handling of specific Boost path variables.
The use of OLD-style Boost lookup is appropriate here as it respects BOOST_INCLUDEDIR and BOOST_LIBRARYDIR variables, which is essential for cross-compilation scenarios.
Let's verify if there are any other CMake files that might need similar adjustments:
recipes/bayestyper/build.sh
Outdated
| chmod 0755 bin/* | ||
| mv bin/bayesTyper ${PREFIX}/bin | ||
| mv bin/bayesTyperTools ${PREFIX}/bin | ||
| cp -f src/bayesTyperTools/scripts/* ${PREFIX}/bin |
There was a problem hiding this comment.
Correct the paths to the binaries and set permissions
Since the build directory is specified as build, the binaries are likely located in build/bin/ rather than bin/. Update the paths accordingly to ensure the binaries are correctly moved. Additionally, set execute permissions for all files in ${PREFIX}/bin.
Apply this diff to fix the paths and permissions:
-chmod 0755 bin/*
-mv bin/bayesTyper ${PREFIX}/bin
-mv bin/bayesTyperTools ${PREFIX}/bin
+chmod 0755 build/bin/*
+mv build/bin/bayesTyper ${PREFIX}/bin
+mv build/bin/bayesTyperTools ${PREFIX}/bin
cp -f src/bayesTyperTools/scripts/* ${PREFIX}/bin
+chmod 0755 ${PREFIX}/bin/*📝 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.
| chmod 0755 bin/* | |
| mv bin/bayesTyper ${PREFIX}/bin | |
| mv bin/bayesTyperTools ${PREFIX}/bin | |
| cp -f src/bayesTyperTools/scripts/* ${PREFIX}/bin | |
| chmod 0755 build/bin/* | |
| mv build/bin/bayesTyper ${PREFIX}/bin | |
| mv build/bin/bayesTyperTools ${PREFIX}/bin | |
| cp -f src/bayesTyperTools/scripts/* ${PREFIX}/bin | |
| chmod 0755 ${PREFIX}/bin/* |
| cmake -S . -B build -DCMAKE_BUILD_TYPE=Release \ | ||
| -DCMAKE_INSTALL_PREFIX="${PREFIX}" \ | ||
| -DCMAKE_CXX_COMPILER="${CXX}" \ | ||
| -DCMAKE_CXX_FLAGS="${CXXFLAGS}" \ | ||
| -DBOOST_ROOT="${PREFIX}" \ | ||
| "${CONFIG_ARGS}" |
There was a problem hiding this comment.
Remove unnecessary quotes around ${CONFIG_ARGS}
Enclosing ${CONFIG_ARGS} in quotes can lead to issues when the variable is empty or contains multiple arguments. This can cause CMake to interpret it incorrectly. Remove the quotes to allow proper argument expansion.
Apply this diff to fix the issue:
cmake -S . -B build -DCMAKE_BUILD_TYPE=Release \
-DCMAKE_INSTALL_PREFIX="${PREFIX}" \
-DCMAKE_CXX_COMPILER="${CXX}" \
-DCMAKE_CXX_FLAGS="${CXXFLAGS}" \
-DBOOST_ROOT="${PREFIX}" \
- "${CONFIG_ARGS}"
+ ${CONFIG_ARGS}📝 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.
| cmake -S . -B build -DCMAKE_BUILD_TYPE=Release \ | |
| -DCMAKE_INSTALL_PREFIX="${PREFIX}" \ | |
| -DCMAKE_CXX_COMPILER="${CXX}" \ | |
| -DCMAKE_CXX_FLAGS="${CXXFLAGS}" \ | |
| -DBOOST_ROOT="${PREFIX}" \ | |
| "${CONFIG_ARGS}" | |
| cmake -S . -B build -DCMAKE_BUILD_TYPE=Release \ | |
| -DCMAKE_INSTALL_PREFIX="${PREFIX}" \ | |
| -DCMAKE_CXX_COMPILER="${CXX}" \ | |
| -DCMAKE_CXX_FLAGS="${CXXFLAGS}" \ | |
| -DBOOST_ROOT="${PREFIX}" \ | |
| ${CONFIG_ARGS} |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
recipes/bayestyper/build.sh (1)
11-16: Reconsider blanket warning suppression on DarwinSuppressing
-Wno-deprecated-declarationsand-Wno-unused-command-line-argumentmight hide important issues. Consider handling these warnings properly instead of suppressing them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
recipes/bayestyper/bayestyper.patch(1 hunks)recipes/bayestyper/build.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- recipes/bayestyper/bayestyper.patch
🔇 Additional comments (3)
recipes/bayestyper/build.sh (3)
18-23: Remove quotes around ${CONFIG_ARGS}
This issue was previously identified in a past review. The quotes around ${CONFIG_ARGS} can cause issues with argument expansion.
26-28: Update binary paths and permissions
This issue was previously identified in a past review. The binaries are likely in build/bin/ instead of bin/, and proper permissions need to be set.
18-24: Add arm64/aarch64-specific build configurations
Since this PR aims to add arm64/aarch64 builds, consider:
- Adding CPU feature detection for arm64-specific optimizations
- Verifying CMake toolchain settings for cross-compilation
- Testing with different optimization levels on arm64 hardware
| export INCLUDES="-I${PREFIX}/include" | ||
| export CPLUS_INCLUDE_PATH="${PREFIX}/include" | ||
| export LIBPATH="-L${PREFIX}/lib" | ||
| export LDFLAGS="${LDFLAGS} -L${PREFIX}/lib" | ||
| export CXXFLAGS="${CXXFLAGS} -O3 -std=c++14 -I${PREFIX}/include" |
There was a problem hiding this comment.
💡 Codebase verification
Make -O3 optimization configurable for arm64 builds
Currently, -O3 is hardcoded in CXXFLAGS for arm64/aarch64 builds in recipes/bayestyper/build.sh. Consider using -O2 for better stability or making the optimization level configurable based on the target architecture.
🔗 Analysis chain
Consider architecture-specific optimizations for arm64
The hardcoded -O3 optimization level might be too aggressive for arm64/aarch64 builds. Consider using -O2 for better stability or making it configurable based on the target architecture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing arm64 builds using O3 optimization
rg -l 'CXXFLAGS.*-O3' recipes/*/build.sh | xargs rg 'host.*aarch64|arm64'
Length of output: 192942
mencian
removed
aarch64
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>.