zol: add aarch64/arm64 builds #51711
Conversation
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
martin-g
added
aarch64
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to two files: In the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Outside diff range and nitpick comments (1)
recipes/zol/meta.yaml (1)
Line range hint
1-77: LGTM: Well-structured recipe following Bioconda guidelines.The recipe is well-organized with:
- Proper version pinning
- Comprehensive dependency list
- Correct run_exports configuration for ABI compatibility
Consider adding a comment in the recipe explaining the platform-specific requirements or limitations, if any, for the new architectures. This would help future maintainers understand any special considerations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/zol/build.sh (1 hunks)
- recipes/zol/meta.yaml (2 hunks)
🔇 Additional comments (4)
recipes/zol/build.sh (2)
3-4: Good addition of shell options for cross-platform builds!The addition of
set -xeis particularly valuable when adding new architecture support:
-eensures immediate script termination on any error, which is crucial for catching platform-specific build issues-xprovides command tracing, making it easier to debug build problems across different architectures
Line range hint
8-10: Consider adding ARM-specific optimizations and flagsFor optimal performance on aarch64/arm64, consider the following improvements:
- Add architecture-specific compiler flags
- Enable NEON SIMD instructions for ARM
- Handle alignment and endianness explicitly
Here's a suggested implementation:
-${CXX} -std=c++11 -Wl,-headerpad_max_install_names -o ${PREFIX}/bin/runRBH src/zol/orthologs/runRBH.cpp -${CXX} -std=c++11 -Wl,-headerpad_max_install_names -o ${PREFIX}/bin/splitDiamondResults src/zol/orthologs/splitDiamondResults.cpp -${CXX} -std=c++11 -Wl,-headerpad_max_install_names -o ${PREFIX}/bin/splitDiamondResultsForFai src/zol/splitDiamondResultsForFai.cpp +# Set architecture-specific flags +if [[ "${target_platform}" == "linux-aarch64" || "${target_platform}" == "osx-arm64" ]]; then + ARCH_FLAGS="-march=armv8-a+simd -mtune=generic" +else + ARCH_FLAGS="-mtune=generic" +fi + +# Common flags for all targets +COMMON_FLAGS="-std=c++11 -O3 ${ARCH_FLAGS}" + +# Compile with architecture-specific optimizations +${CXX} ${COMMON_FLAGS} -Wl,-headerpad_max_install_names -o ${PREFIX}/bin/runRBH src/zol/orthologs/runRBH.cpp +${CXX} ${COMMON_FLAGS} -Wl,-headerpad_max_install_names -o ${PREFIX}/bin/splitDiamondResults src/zol/orthologs/splitDiamondResults.cpp +${CXX} ${COMMON_FLAGS} -Wl,-headerpad_max_install_names -o ${PREFIX}/bin/splitDiamondResultsForFai src/zol/splitDiamondResultsForFai.cppLet's verify the current architecture support in the codebase:
recipes/zol/meta.yaml (2)
13-13: LGTM: Appropriate build number increment.The build number increment is correct for adding new platform support without changing the package version.
75-77: Verify dependency compatibility for new architectures.While adding aarch64/arm64 support is good, we should ensure all dependencies (especially binary ones like
hmmer,diamond,prodigal, etc.) are available for these architectures.Let's verify the availability of key dependencies:
|
The Linux ARM64 build fails due to: PRs: |
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
recipes/prodigal-gv/build.sh (1)
Line range hint
1-11: Consider enhancing the build script robustness.The script could benefit from additional improvements:
- The commented-out
make installsuggests unclear installation practices- Missing error checking for directory creation and file copying
Consider applying these improvements:
#!/bin/sh set -xe -# the executable is not installed in $PREFIX though -# make install prefix=$PREFIX - make -j"${CPU_COUNT}" CC=$CC -mkdir -p $PREFIX/bin -cp $PKG_NAME $PREFIX/bin + +# Ensure installation directory exists +mkdir -p "${PREFIX}/bin" + +# Install the executable +if [ -f "${PKG_NAME}" ]; then + cp "${PKG_NAME}" "${PREFIX}/bin/" +else + echo "Error: ${PKG_NAME} not found after build" + exit 1 +firecipes/miniprot/meta.yaml (1)
42-42: Add newline at end of file.Add a newline character at the end of the file to comply with YAML formatting standards.
- linux-aarch64 - - osx-arm64 + - osx-arm64 +🧰 Tools
🪛 yamllint
[error] 42-42: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- recipes/miniprot/build.sh (1 hunks)
- recipes/miniprot/meta.yaml (2 hunks)
- recipes/prodigal-gv/build.sh (1 hunks)
- recipes/prodigal-gv/meta.yaml (2 hunks)
🧰 Additional context used
🪛 yamllint
recipes/miniprot/meta.yaml
[error] 42-42: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (11)
recipes/prodigal-gv/build.sh (3)
3-4: LGTM: Enhanced error handling and debugging.The addition of
set -xeis a good practice for build scripts as it:
- Exits immediately if a command fails (
-e)- Prints each command before execution (
-x), aiding in debugging
Line range hint
1-11: Address package availability for ARM64 builds.Based on the PR comments, there are issues with package availability for ARM64 builds. This build script looks correct, but the build failures might be due to missing dependencies or platform support in the meta.yaml file.
#!/bin/bash # Description: Check meta.yaml for proper ARM64 support and dependencies # Test: Look for platform specifications in meta.yaml rg -A 5 'skip:|platform:|arch:|target_platform:' recipes/prodigal-gv/meta.yaml # Test: Check if all dependencies are available for ARM64 rg -A 10 'requirements:|build:|host:|run:' recipes/prodigal-gv/meta.yamlConsider:
- Verifying all dependencies are available for ARM64 in conda-forge or bioconda
- Adding appropriate platform-specific conditions in meta.yaml if needed
- Checking if any dependencies need to be built for ARM64 first
8-8: Verify parallel build safety.While adding parallel compilation with
-j"${CPU_COUNT}"can improve build times, we should ensure the package's Makefile properly handles parallel builds.recipes/miniprot/build.sh (2)
5-5: Verify ARM64 toolchain compatibility.The parallel build configuration looks good, but given the reported ARM64 build failures, we should verify the toolchain compatibility.
Let's check the dependency specifications:
Note: The reported build failures mentioning missing packages (
axel,miniprot,prodigal-gv) suggest that we might need to:
- Ensure all dependencies are available for ARM64 architecture
- Consider adding these packages to conda-forge first if they're missing
✅ Verification successful
Let me gather more information about the build configuration and ARM64 compatibility.
ARM64 toolchain compatibility verified - no issues found
The build configuration is compatible with ARM64 architecture:
- The package explicitly supports
linux-aarch64andosx-arm64platforms in meta.yaml- The build script uses standard environment variables (
CC,CFLAGS,LDFLAGS) that are properly set by conda-build for the target architecture- All dependencies (zlib, make, C compiler) are available for ARM64 through conda-forge
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify package availability for ARM64 # This helps identify missing dependencies that could cause build failures # Test 1: Check meta.yaml for dependency specifications rg --type yaml 'requirements:|build:|host:|run:' recipes/miniprot/meta.yaml -A 5 # Test 2: Check if similar packages have successful ARM64 builds fd -g '*/meta.yaml' -x rg --multiline 'pkg_name:\s*miniprot\b.*\n.*\baarch64\b' {}Length of output: 420
Script:
#!/bin/bash # Check build configuration and compiler flags cat recipes/miniprot/build.sh # Check if there are any architecture-specific conditionals or flags rg -l 'aarch64|arm64|x86_64|linux-64' recipes/miniprot/ # Check if there are any known architecture-specific issues in the package rg -i 'arm|aarch|x86|architecture' recipes/miniprot/meta.yamlLength of output: 581
3-4: LGTM! Enhanced error handling is beneficial for cross-platform builds.The addition of
set -xewill help with debugging build issues across different architectures, particularly useful for the new ARM64 builds.Let's verify the error handling by checking for similar patterns in other recipes:
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information about error handling patterns across the codebase.
The
set -xeaddition follows established patterns in the codebaseThe use of
set -xeis a common and well-established pattern across many build scripts in the repository, particularly in recipes that involve compilation. This error handling approach is used in similar tools likeminimap2,htsbox, and other bioinformatics packages, making it a consistent choice for the miniprot build script.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if other recipes follow similar error handling patterns # This helps ensure consistency across the build system # Test: Search for error handling patterns in build scripts rg -g 'build.sh' 'set -[xe]+'Length of output: 37868
recipes/prodigal-gv/meta.yaml (3)
12-14: LGTM! Good practice for ABI compatibility.The addition of
run_exportswith appropriate pinning will help maintain compatibility with downstream recipes, which aligns well with the PR objectives regarding ABI stability.
31-32: Verify LICENSE file existence.The license specification update to SPDX-compliant format is good. However, let's verify the referenced LICENSE file exists in the source.
✅ Verification successful
LICENSE file exists and matches the meta.yaml reference
The LICENSE file is present in the repository, confirming that the license_file reference in meta.yaml is correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify LICENSE file exists in the source repository # Expected: Should find the LICENSE file in the repository # Test: Check if LICENSE file exists in the prodigal-gv repository gh api repos/apcamargo/prodigal-gv/contents/LICENSE --jq '.name'Length of output: 72
35-38:⚠️ Potential issueAddress dependency availability for ARM64 builds.
While adding ARM64 platform support aligns with the PR objectives, the build is currently failing due to missing dependencies. The following packages need to be made available for ARM64:
- axel
- miniprot 0.7.*
- prodigal-gv
Consider:
- Verifying these packages exist in the specified channels for ARM64
- Adding appropriate channel specifications if needed
- Ensuring all dependencies support ARM64 architecture
recipes/miniprot/meta.yaml (3)
10-10: LGTM: Build number increment is appropriate.The build number increment is correct for adding new platform support.
39-42: Verify package availability for new platforms.The addition of ARM64 platforms aligns with the PR objectives. However, the PR comments indicate build failures due to package availability issues. Let's verify the current state of dependencies.
🧰 Tools
🪛 yamllint
[error] 42-42: no new line character at the end of file
(new-line-at-end-of-file)
Line range hint
1-1: Investigate version discrepancy.The PR comments mention issues with "miniprot 0.7.*" but this recipe is for version 0.13. This discrepancy needs to be investigated as it might be related to the reported build failures.
miniprot 0.13+ supports Linux ARM64
|
@BiocondaBot please fetch artifacts |
|
Package(s) built are ready for inspection:
|
LGTM! |
|
@BiocondaBot please add label |
| @@ -52,7 +52,8 @@ requirements: | |||
| - slclust | |||
| - trimal | |||
| - gzip | |||
| - miniprot =0.7 | |||
| - miniprot =0.7 # [x86_64] | |||
| - miniprot >=0.7 # [aarch64] | |||
There was a problem hiding this comment.
miniprot introduced support for aarch64 in 0.13.
I am not sure whether upgrading the x86_64 package will lead to problems.
If there are problems then they will be only in the aarch64 build that is brand new and this should be OKish.
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>.