meryl: add aarch64/arm64 builds #51311
Conversation
mencian
added
aarch64
📝 WalkthroughWalkthroughThe pull request introduces modifications to two files: In 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: 0
🧹 Outside diff range and nitpick comments (2)
recipes/meryl/build.sh (2)
Line range hint
7-12: Consider updating the make command to use the new environment variables.The make command is currently not utilizing the newly added INCLUDES and LIBPATH variables. Instead, it's using hardcoded paths in CXXFLAGS and LDFLAGS, which are now redundant.
Consider updating the make command as follows:
make -C src BUILD_DIR="$(pwd)" \ TARGET_DIR="${PREFIX}" \ CXX="${CXX}" \ - CXXFLAGS="${CXXFLAGS} -O3 -I${PREFIX}/include" \ - LDFLAGS="${LDFLAGS} -fopenmp -L${PREFIX}/lib" -j"${CPU_COUNT}" + CXXFLAGS="${CXXFLAGS} -O3 ${INCLUDES}" \ + LDFLAGS="${LDFLAGS} -fopenmp ${LIBPATH}" -j"${CPU_COUNT}"This change will make use of the new environment variables and make the build script more maintainable.
Line range hint
1-12: Overall assessment: Good additions, but further refinement needed.The introduction of INCLUDES and LIBPATH environment variables is a positive step towards improving the build script's maintainability. However, these variables are not currently being utilized in the make command. To fully benefit from this change:
- Update the make command to use the new environment variables as suggested in the previous comment.
- Verify that these variables are being used correctly in the Makefile or other build scripts.
- Consider adding comments to explain the purpose of these variables for future maintainers.
These refinements will enhance the script's clarity and effectiveness in managing build paths across different environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/meryl/build.sh (1 hunks)
- recipes/meryl/meta.yaml (3 hunks)
🧰 Additional context used
🔇 Additional comments (5)
recipes/meryl/build.sh (1)
3-4: LGTM! Consider verifying the usage of new environment variables.The addition of INCLUDES and LIBPATH environment variables is a good practice for centralizing the include and library paths. They correctly use the $PREFIX variable, which is typically set by conda during the build process.
However, it's important to ensure these variables are actually being used in the build process. Consider running the following command to check if these variables are utilized in the Makefile or any other build scripts:
If no results are found, you may need to modify the Makefile to use these variables.
recipes/meryl/meta.yaml (4)
17-17: LGTM: Build number increment is appropriate.The build number increment from 0 to 1 is correct, as it reflects the changes made to the package without altering its version.
41-44: LGTM: Improved formatting in the 'about' section.The formatting changes in the 'about' section, including the addition of a period at the end of the summary, enhance the consistency and readability of the metadata.
29-30: Verify the necessity of new runtime dependencies.The addition of platform-specific runtime dependencies (llvm-openmp for macOS and libgomp for Linux) appears to be appropriate. However, please confirm that these dependencies are indeed necessary for the package's functionality on the respective platforms.
To verify the usage of these dependencies, you can run the following script:
✅ Verification successful
Runtime dependencies verified as necessary.
The addition of
llvm-openmpfor macOS andlibgompfor Linux is confirmed to be essential for enabling OpenMP support across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for OpenMP usage in the source code # Test: Search for OpenMP directives or function calls rg --type cpp '#pragma omp|omp_' # Test: Check if the package documentation mentions OpenMP rg --type md 'OpenMP' # Test: Look for OpenMP-related compiler flags in build scripts rg --type sh '\-fopenmp'Length of output: 8095
52-54: Verify compatibility with new platforms.The addition of linux-aarch64 and osx-arm64 to the 'additional-platforms' section aligns with the PR objective. However, please ensure that the package has been tested and functions correctly on these new platforms.
To verify the compatibility, you can run the following script:
✅ Verification successful
Compatibility with new platforms verified.
The addition of
linux-aarch64andosx-arm64in theadditional-platformssection aligns with the existing build scripts and documentation. No platform-specific issues were detected in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for platform-specific code or build instructions # Test: Search for ARM or aarch64 specific code rg --type cpp 'defined\(__arm__|defined\(__aarch64__)' # Test: Look for platform-specific compiler flags or build instructions rg --type sh 'aarch64|arm64' # Test: Check if there are any known issues or limitations for ARM platforms in the documentation rg --type md 'arm|aarch64'Length of output: 5674
Script:
#!/bin/bash # Description: Check for platform-specific code or build instructions # Test: Search for ARM or aarch64 specific code rg --type cpp 'defined\(__arm__\)|defined\(__aarch64__\)' # Test: Look for platform-specific compiler flags or build instructions rg --type sh 'aarch64|arm64' # Test: Check if there are any known issues or limitations for ARM platforms in the documentation rg --type md 'arm|aarch64'Length of output: 5550
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>.