pydustmasker: add aarch64 build #51158
Conversation
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
martin-g
added
aarch64
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/pydustmasker/build.sh (1 hunks)
- recipes/pydustmasker/meta.yaml (2 hunks)
🔇 Additional comments (3)
recipes/pydustmasker/meta.yaml (3)
13-13: LGTM: Build number increment is appropriate.The build number has been correctly incremented from 0 to 1. This is the right approach when making changes to a package without altering its version, which aligns with the PR objective of adding aarch64/arm64 builds.
Line range hint
1-41: Summary: Changes align well with PR objectives.The modifications to this meta.yaml file are consistent with the PR's goal of adding aarch64/arm64 builds for the pydustmasker package:
- The build number has been incremented (line 13).
- An extra section has been added to specify additional platforms (lines 38-41).
These changes are appropriate and sufficient for enabling builds for the new architectures. The rest of the file, including version, source, requirements, test, and about sections, remains unchanged, which is correct for this type of update.
38-41: LGTM: Additional platforms correctly specified.The extra section has been added correctly, specifying the additional platforms (linux-aarch64 and osx-arm64) for the build. This change directly addresses the PR objective of adding aarch64/arm64 builds to the pydustmasker package.
To ensure the correctness of the added platforms, let's verify if these are standard platform identifiers used in Conda:
✅ Verification successful
Verified: Added platforms are standard in Conda recipes.
The platforms
linux-aarch64andosx-arm64are consistently used across multiple recipes, confirming their standard usage in Conda.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the added platform identifiers are standard in Conda # Test: Search for usage of these platform identifiers in other recipe files # Expect: Multiple occurrences of these identifiers in other recipes rg --type yaml 'linux-aarch64|osx-arm64' recipes/Length of output: 59383
recipes/pydustmasker/build.sh
Outdated
| export CARGO_TARGET_AARCH64_UNKNOWN_LINUX_GNU_LINKER="$CC" | ||
| export CARGO_TARGET_AARCH64_APPLE_DARWIN_GNU_LINKER="$CC" | ||
| export CARGO_TARGET_ARM64_APPLE_DARWIN_GNU_LINKER="$CC" |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Remove unnecessary ARM64 export statements
The build environment is currently configured for x86_64 on Linux, and ARM64 builds are not being performed. Therefore, the ARM64 export statements in recipes/pydustmasker/build.sh are unnecessary and can be removed to streamline the configuration.
🔗 Analysis chain
Adjust export statements for comprehensive architecture support
The added export statements are a good step towards supporting aarch64/arm64 builds. However, there are a couple of adjustments that could improve the configuration:
-
Add support for ARM64 on Linux:
The current configuration doesn't include an export for ARM64 on Linux, which might be necessary for complete architecture support. -
Simplify macOS configuration:
On macOS, AArch64 and ARM64 are synonymous, so having separate exports for both might be redundant.
Consider applying the following changes:
export CARGO_TARGET_AARCH64_UNKNOWN_LINUX_GNU_LINKER="$CC"
+export CARGO_TARGET_ARM64_UNKNOWN_LINUX_GNU_LINKER="$CC"
export CARGO_TARGET_AARCH64_APPLE_DARWIN_GNU_LINKER="$CC"
-export CARGO_TARGET_ARM64_APPLE_DARWIN_GNU_LINKER="$CC"This modification ensures support for both AArch64 and ARM64 on Linux while simplifying the macOS configuration.
To verify the necessity of these changes, you can run the following script:
This script will help determine if the package is being built for different architectures and operating systems, which can guide the decision on which export statements are necessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the package is being built for different architectures
# Test: Check if the package is being built for ARM64 on Linux
if [[ "$(uname -m)" == "aarch64" && "$(uname -s)" == "Linux" ]]; then
echo "Building for ARM64 on Linux"
# Add your ARM64 Linux-specific build commands here
fi
# Test: Check if the package is being built for ARM64 on macOS
if [[ "$(uname -m)" == "arm64" && "$(uname -s)" == "Darwin" ]]; then
echo "Building for ARM64 on macOS"
# Add your ARM64 macOS-specific build commands here
fi
# Output the current architecture and OS for reference
echo "Current architecture: $(uname -m)"
echo "Current OS: $(uname -s)"
Length of output: 221
martin-g
changed the title
|
@BiocondaBot please fetch artifacts |
|
Package(s) built are ready for inspection:
Docker image(s) built:
|
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>.