salmon: add arm64 build #51788
salmon: add arm64 build #51788
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Possibly related PRs
Suggested labels
📜 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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/salmon/build.sh (1 hunks)
- recipes/salmon/meta.yaml (3 hunks)
🔇 Additional comments (8)
recipes/salmon/build.sh (1)
17-18: Verify compiler flags propagation.
Since CFLAGS includes linker flags, we should verify that this doesn't cause issues when passed to CMAKE_C_FLAGS, especially on ARM64 builds.
✅ Verification successful
Let me analyze the results and generate a final response.
Passing CFLAGS to CMAKE_C_FLAGS is a common and safe practice
The search results show this is a standard practice across multiple bioconda packages including sra-tools, wfa2-lib, svdss, spades, and others. These packages have been successfully building on various architectures, indicating this approach is safe and well-tested.
Key observations:
- Multiple established packages like sra-tools and spades use identical CMake configuration
- Some packages like bustools even add additional optimization flags (-O3) on top of CFLAGS
- No evidence of ARM64-specific issues in similar packages using this approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other bioconda packages pass CFLAGS directly to CMAKE_C_FLAGS
rg -B2 -A2 "CMAKE_C_FLAGS.*CFLAGS" recipes/
Length of output: 4328
recipes/salmon/meta.yaml (7)
14-14: LGTM: Build number increment is appropriate
The build number increment is correct given the significant changes to dependencies and platform support.
26-26: LGTM: pkg-config addition is appropriate
Adding pkg-config as a build dependency is correct for proper library detection during the build process.
61-61: LGTM: ARM64 platform support correctly added
The addition of osx-arm64 platform support aligns with the PR objective and complements the existing linux-aarch64 support.
65-65: LGTM: New maintainer added correctly
The addition of a new maintainer enhances project sustainability.
69-69: LGTM: Galaxy tool identifier added correctly
The addition of the Galaxy tool identifier follows the established pattern and improves tool discoverability.
15-16: Verify run_exports configuration
The current run_exports configuration uses max_pin="x", which pins the major version. Given the new dependencies and ARM64 support, please verify this is sufficient to prevent ABI/API breakage.
✅ Verification successful
Let me analyze the output from the ripgrep search of run_exports patterns in bioconda recipes. The search revealed that most bioconda packages use max_pin="x.x" for version pinning, while the salmon recipe uses max_pin="x". Let me gather more information about salmon's API stability and versioning practices.
Based on the analysis of the output, I can now generate a final response:
max_pin="x" is appropriate for salmon's run_exports configuration
The current pinning strategy is correct because:
- Salmon follows semantic versioning where major version changes indicate breaking API changes
- Other tools in the ecosystem (like trinity, asgal, metav) pin to specific major versions of salmon
- The package has explicit version requirements in its dependencies, suggesting careful API compatibility management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check run_exports patterns in similar bioinformatics tools
echo "Checking run_exports patterns in other bioconda recipes:"
rg -g "meta.yaml" "run_exports:.*pin_subpackage" recipes/
Length of output: 440198
Script:
#!/bin/bash
# Check salmon's version constraints and dependencies
cat recipes/salmon/meta.yaml
# Look for other related recipes that might inform pinning strategy
rg -g "meta.yaml" "salmon" recipes/
Length of output: 2953
31-35: Consider adding version constraints for new dependencies
While the addition of these dependencies is appropriate, consider adding minimum version constraints for libgff, cereal, libiconv, and libdeflate to ensure build reproducibility and prevent potential compatibility issues.
Let's check if these packages typically use version constraints in other recipes:
| -DSTADEN_INCLUDE_DIR="${PREFIX}/include" \ | ||
| -DSTADEN_LIBRARY="${PREFIX}/lib/libstaden-read.a" \ | ||
| -DSTADEN_VERSION="1.15.0" \ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding Staden version.
The Staden version is hardcoded to "1.15.0". This could lead to maintenance issues if the version needs to be updated in multiple places.
Consider using a variable that can be passed from meta.yaml:
- -DSTADEN_VERSION="1.15.0" \
+ -DSTADEN_VERSION="${staden_version}" \Then ensure this variable is exported in the script:
staden_version=$(conda list staden-io-lib | grep staden-io-lib | awk '{print $2}')
recipes/salmon/build.sh
Outdated
| export LDFLAGS="${LDFLAGS} -L${PREFIX}/lib -Wl,-R${PREFIX}/lib" | ||
| export CFLAGS="${CFLAGS} -O3 ${LDFLAGS}" |
There was a problem hiding this comment.
Separate compiler and linker flags.
The current setup includes linker flags (LDFLAGS) within the compiler flags (CFLAGS), which is not recommended. This could cause compilation warnings or errors, especially on ARM64 platforms.
Suggested fix:
export LDFLAGS="${LDFLAGS} -L${PREFIX}/lib -Wl,-R${PREFIX}/lib"
-export CFLAGS="${CFLAGS} -O3 ${LDFLAGS}"
+export CFLAGS="${CFLAGS} -O3"📝 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 LDFLAGS="${LDFLAGS} -L${PREFIX}/lib -Wl,-R${PREFIX}/lib" | |
| export CFLAGS="${CFLAGS} -O3 ${LDFLAGS}" | |
| export LDFLAGS="${LDFLAGS} -L${PREFIX}/lib -Wl,-R${PREFIX}/lib" | |
| export CFLAGS="${CFLAGS} -O3" |
| -DSTADEN_INCLUDE_DIR="${PREFIX}/include" \ | ||
| -DSTADEN_LIBRARY="${PREFIX}/lib/libstaden-read.a" \ | ||
| -DSTADEN_VERSION="1.15.0" \ | ||
| -DNO_IPO=TRUE \ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enabling IPO for better performance.
Interprocedural Optimization (IPO) is disabled with NO_IPO=TRUE. While this might help with build stability, it could prevent potential performance optimizations that would be particularly beneficial for ARM64 builds.
Consider conditionally enabling IPO based on the target architecture:
- -DNO_IPO=TRUE \
+ -DNO_IPO=$([[ "${target_platform}" == "osx-arm64" ]] && echo "FALSE" || echo "TRUE") \📝 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.
| -DNO_IPO=TRUE \ | |
| -DNO_IPO=$([[ "${target_platform}" == "osx-arm64" ]] && echo "FALSE" || echo "TRUE") \ |
recipes/salmon/build.sh
Outdated
| if [[ `uname` == "Darwin" ]]; then | ||
| export CFLAGS="${CFLAGS} -Wunused-command-line-argument -Wunused-parameter" | ||
| export CONFIG_ARGS="-DCMAKE_OSX_DEPLOYMENT_TARGET=10.11 -DCMAKE_FIND_FRAMEWORK=NEVER -DCMAKE_FIND_APPBUNDLE=NEVER" | ||
| else | ||
| export CONFIG_ARGS="" | ||
| fi |
There was a problem hiding this comment.
💡 Codebase verification
Update macOS deployment target to 10.15
The current setting of CMAKE_OSX_DEPLOYMENT_TARGET=10.11 is outdated. Modern packages in the codebase use either 10.15 (guidescan) or no specific target (diamond). Setting an old target like 10.11 prevents utilizing newer OS features and optimizations, particularly for ARM64 builds. Consider updating to 10.15 to align with other modern packages while still maintaining broad compatibility.
🔗 Analysis chain
Verify macOS deployment target.
The script sets CMAKE_OSX_DEPLOYMENT_TARGET=10.11 which is quite old (from 2015). While this maximizes compatibility, it might prevent utilizing newer OS features that could benefit ARM64 builds.
Let's check if this aligns with other bioconda packages:
Let me check the actual deployment target values in the successful builds to make a proper assessment.
Based on the search results, I can now make a proper assessment. Let me check the deployment target values in other active packages to ensure we recommend the right value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for deployment target settings in other recipes
rg -l "CMAKE_OSX_DEPLOYMENT_TARGET" recipes/
Length of output: 3719
Script:
#!/bin/bash
# Check deployment target values in build.sh files
rg -A 1 "CMAKE_OSX_DEPLOYMENT_TARGET" recipes/*/build.sh
Length of output: 193429
Script:
#!/bin/bash
# Check deployment target values in active build.sh files (excluding build_failure.* files)
rg -l "CMAKE_OSX_DEPLOYMENT_TARGET" recipes/ | grep -v "build_failure" | xargs rg "CMAKE_OSX_DEPLOYMENT_TARGET.*=.*"
Length of output: 730
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>.