cuttlefish: add arm64 build #51956
cuttlefish: add arm64 build #51956
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several changes to the Cuttlefish recipe files, focusing on the build configuration and metadata. 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 (
|
mencian
added
aarch64
![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 (4)
recipes/cuttlefish/meta.yaml (1)
37-41: LGTM! Metadata improvements enhance package quality.Good improvements:
- Added license family for better license categorization
- Version-specific documentation URL ensures users get correct documentation
- Proper quoting of URLs
Consider adding a brief description of platform support in the summary:
- summary: "Construction of the compacted de Bruijn graph efficiently." + summary: "Construction of the compacted de Bruijn graph efficiently. Supports x86_64 and ARM architectures."recipes/cuttlefish/build.sh (3)
3-4: Remove unused variablesINCLUDESandLIBPATH.The variables
INCLUDESandLIBPATHare exported but not used in the script. Removing them will clean up the code.Apply this diff to remove the unused variables:
-export INCLUDES="-I${PREFIX}/include" -export LIBPATH="-L${PREFIX}/lib"
8-8: Use$(uname)instead of backticks for command substitution.Using
$(uname)enhances readability and is the recommended modern syntax.Apply this diff to update the command substitution:
-if [[ `uname` == 'Darwin' ]]; then +if [[ $(uname) == 'Darwin' ]]; then
13-13: Rephrase the comment to maintain professionalism.The comment contains informal language. Rewriting it will maintain a professional tone.
Apply this diff to rephrase the comment:
-# It's dumb and absurd that the KMC build can't find the bzip2 header <bzlib.h> +# Note: KMC build requires explicit paths to locate the bzip2 header <bzlib.h>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
recipes/cuttlefish/build.sh(1 hunks)recipes/cuttlefish/build_failure.osx-64.yaml(0 hunks)recipes/cuttlefish/meta.yaml(2 hunks)
💤 Files with no reviewable changes (1)
- recipes/cuttlefish/build_failure.osx-64.yaml
🔇 Additional comments (4)
recipes/cuttlefish/meta.yaml (3)
20-29: LGTM! Requirements are architecture-agnostic.
The build and host requirements are properly structured for cross-platform compilation, with no architecture-specific dependencies that would block aarch64/arm64 builds.
44-46: LGTM! Platform support and identifiers properly declared.
The additional-platforms section correctly declares ARM64 support, and the biotools identifier enhances package discoverability.
Let's verify the build system support for these architectures:
#!/bin/bash
# Description: Verify build system support for ARM architectures
# Expected: Build configuration for ARM architectures in conda-forge or bioconda
# Check for ARM architecture support in conda configuration
rg -l "linux-aarch64|osx-arm64" --type yamlAlso applies to: 53-53
9-11: LGTM! Appropriate version pinning strategy.
The run_exports section with max_pin="x" ensures ABI compatibility by constraining updates to the same major version, which is suitable for C++ libraries.
Let's verify if other bioconda C++ packages follow similar pinning strategy:
recipes/cuttlefish/build.sh (1)
19-25: Build configuration updates look good.
The changes to the cmake command streamline the build process and support the addition of aarch64/arm64 builds.
mencian
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/cuttlefish/build.sh(1 hunks)
🔇 Additional comments (2)
recipes/cuttlefish/build.sh (2)
13-15: Address underlying build system issues with KMC
The comment suggests underlying issues with the KMC build system. Instead of working around it with environment variables, consider fixing the root cause.
Consider:
- Using pkg-config to locate bzip2
- Adding proper find_package() calls in CMake
- Setting up proper target_include_directories()
19-25: 🛠️ Refactor suggestion
Enhance CMake configuration for conda and ARM64 builds
The CMake configuration needs additional flags for proper conda integration and ARM64 support.
cmake -S . -B build -DCMAKE_BUILD_TYPE=Release \
-DCMAKE_CXX_COMPILER="${CXX}" \
-DCMAKE_CXX_FLAGS="${CXXFLAGS}" \
- -DINSTANCE_COUNT=64 \
+ -DINSTANCE_COUNT=$([[ "${target_platform}" == *"arm64"* ]] && echo "32" || echo "64") \
-DCMAKE_INSTALL_PREFIX="${PREFIX}" \
+ -DCMAKE_INSTALL_LIBDIR=lib \
+ -DCMAKE_BUILD_WITH_INSTALL_RPATH=ON \
+ -DCMAKE_INSTALL_RPATH_USE_LINK_PATH=ON \
+ -DCMAKE_INSTALL_RPATH="${PREFIX}/lib" \
"${CONFIG_ARGS}"The changes:
- Adjust INSTANCE_COUNT based on architecture (ARM typically has fewer cores)
- Add proper RPATH handling for conda environments
- Ensure libraries are installed to the correct location
| export INCLUDES="-I${PREFIX}/include" | ||
| export LIBPATH="-L${PREFIX}/lib" | ||
| export LDFLAGS="${LDFLAGS} -L${PREFIX}/lib" | ||
| export CXXFLAGS="${CXXFLAGS} -O3 -fcommon -I${PREFIX}/include" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize environment variable configuration
Several suggestions to improve the build configuration:
INCLUDESandLIBPATHappear redundant as they're already covered byCXXFLAGSandLDFLAGS- The
-fcommonflag suggests potential symbol collision issues. Consider fixing the multiple symbol definitions instead - Consider adding architecture-specific optimizations for ARM64 builds
-export INCLUDES="-I${PREFIX}/include"
-export LIBPATH="-L${PREFIX}/lib"
export LDFLAGS="${LDFLAGS} -L${PREFIX}/lib"
-export CXXFLAGS="${CXXFLAGS} -O3 -fcommon -I${PREFIX}/include"
+export CXXFLAGS="${CXXFLAGS} -O3 -I${PREFIX}/include"
+if [[ "${target_platform}" == "osx-arm64" ]]; then
+ export CXXFLAGS="${CXXFLAGS} -mcpu=apple-m1"
+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.
| export INCLUDES="-I${PREFIX}/include" | |
| export LIBPATH="-L${PREFIX}/lib" | |
| export LDFLAGS="${LDFLAGS} -L${PREFIX}/lib" | |
| export CXXFLAGS="${CXXFLAGS} -O3 -fcommon -I${PREFIX}/include" | |
| export LDFLAGS="${LDFLAGS} -L${PREFIX}/lib" | |
| export CXXFLAGS="${CXXFLAGS} -O3 -I${PREFIX}/include" | |
| if [[ "${target_platform}" == "osx-arm64" ]]; then | |
| export CXXFLAGS="${CXXFLAGS} -mcpu=apple-m1" | |
| fi |
| if [[ `uname` == 'Darwin' ]]; then | ||
| export MACOSX_DEPLOYMENT_TARGET=10.11 | ||
| export CXXFLAGS="${CXXFLAGS} -D_LIBCPP_DISABLE_AVAILABILITY" | ||
| export CONFIG_ARGS="-DCMAKE_FIND_FRAMEWORK=NEVER -DCMAKE_FIND_APPBUNDLE=NEVER" |
There was a problem hiding this comment.
Review macOS deployment target for ARM64 compatibility
Setting MACOSX_DEPLOYMENT_TARGET=10.11 might be too old for ARM64 support, as Apple Silicon requires at least macOS 11.0 (Big Sur).
if [[ `uname` == 'Darwin' ]]; then
- export MACOSX_DEPLOYMENT_TARGET=10.11
+ if [[ "${target_platform}" == "osx-arm64" ]]; then
+ export MACOSX_DEPLOYMENT_TARGET=11.0
+ else
+ export MACOSX_DEPLOYMENT_TARGET=10.11
+ 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 | |
| export MACOSX_DEPLOYMENT_TARGET=10.11 | |
| export CXXFLAGS="${CXXFLAGS} -D_LIBCPP_DISABLE_AVAILABILITY" | |
| export CONFIG_ARGS="-DCMAKE_FIND_FRAMEWORK=NEVER -DCMAKE_FIND_APPBUNDLE=NEVER" | |
| if [[ `uname` == 'Darwin' ]]; then | |
| if [[ "${target_platform}" == "osx-arm64" ]]; then | |
| export MACOSX_DEPLOYMENT_TARGET=11.0 | |
| else | |
| export MACOSX_DEPLOYMENT_TARGET=10.11 | |
| fi | |
| export CXXFLAGS="${CXXFLAGS} -D_LIBCPP_DISABLE_AVAILABILITY" | |
| export CONFIG_ARGS="-DCMAKE_FIND_FRAMEWORK=NEVER -DCMAKE_FIND_APPBUNDLE=NEVER" |
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>.