Update diamond recipe #51515
Update diamond recipe #51515
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant changes to the Possibly related PRs
Suggested labels
Suggested reviewers
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: 2
🧹 Outside diff range and nitpick comments (2)
recipes/diamond/meta.yaml (1)
42-45: LGTM: Improved identifiers with a minor suggestion.The changes to the identifiers section are beneficial:
- The casing change for 'biotools:diamond' improves consistency with standard naming conventions.
- The addition of Galaxy-related identifiers provides valuable information for Galaxy platform users.
These updates enhance the discoverability and integration of the package with various platforms.
Consider alphabetizing the list of identifiers for better readability and easier maintenance. Here's a suggested order:
identifiers: - biotools:diamond - doi:10.1038/s41592-021-01101-x - usegalaxy-eu:bg_diamond - usegalaxy-eu:bg_diamond_makedb - usegalaxy-eu:bg_diamond_viewrecipes/diamond/build.sh (1)
6-6: Use$(uname)instead of backticks for command substitutionFor better readability and to avoid issues with nested commands, it's recommended to use
$(...)instead of backticks for command substitution in Bash scripts.Apply this diff:
-if [[ `uname` == "Darwin" ]]; then +if [[ $(uname) == "Darwin" ]]; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/diamond/build.sh (1 hunks)
- recipes/diamond/meta.yaml (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
recipes/diamond/meta.yaml (4)
13-13: LGTM: Build number increment is appropriate.The build number has been correctly incremented from 1 to 2. This is the proper approach when making changes to a package without altering its version.
32-33: LGTM: Improvements to the 'about' section.The changes to the 'about' section are beneficial:
- Adding quotes to 'home' and 'license' fields improves consistency and prevents potential YAML parsing issues.
- The period at the end of the summary improves readability.
- The new 'dev_url' field provides useful information for developers.
These changes enhance the metadata quality and user experience.
Also applies to: 36-36, 38-38
46-48: LGTM: Improved formatting for 'additional-platforms'.The indentation adjustment in the 'additional-platforms' section enhances the readability of the YAML file. The listed platforms (osx-arm64 and linux-aarch64) remain correct and relevant.
Line range hint
19-30: Verify the removal of zlib from run requirements.The 'zlib' dependency has been removed from the run requirements while being retained in the host requirements. This suggests that zlib is needed during build time but not at runtime. However, given that the package uses 'run_exports', it's important to ensure that zlib is indeed not needed at runtime.
Please confirm that:
- Diamond doesn't require zlib at runtime.
- The removal of this run requirement won't cause any runtime issues.
If zlib is actually needed at runtime, consider adding it back to the run requirements or using
run_exportsfor zlib in its recipe to automatically add it as a runtime dependency.To help verify this change, you can run the following command to check if the diamond binary has any runtime dependencies on zlib:
If this command returns any results, it indicates that zlib is still a runtime dependency and should be included in the run requirements.
| -DZSTD_LIBRARY="${PREFIX}/lib/libzstd.a" \ | ||
| -DZSTD_INCLUDE_DIR="${PREFIX}/include" \ | ||
| -DBLAST_LIBRARY_DIR="${PREFIX}/lib/ncbi-blast+" \ | ||
| -DCMAKE_OSX_DEPLOYMENT_TARGET="" \ |
There was a problem hiding this comment.
Do not override CMAKE_OSX_DEPLOYMENT_TARGET
Setting CMAKE_OSX_DEPLOYMENT_TARGET to an empty string may lead to macOS compatibility issues. Conda-build manages this variable appropriately. It's best to rely on the defaults provided by the build environment.
Apply this diff:
-DCMAKE_OSX_DEPLOYMENT_TARGET="" \Committable suggestion was skipped due to low confidence.
| cmake -S. -B build -DCMAKE_BUILD_TYPE=Release \ | ||
| -DCMAKE_INSTALL_PREFIX="${PREFIX}" \ | ||
| -DCMAKE_CXX_COMPILER="${CXX}" \ | ||
| -DCMAKE_CXX_FLAGS="${CXXFLAGS} -O3 -I${PREFIX}/include" \ |
There was a problem hiding this comment.
Avoid overriding optimization flags in CXXFLAGS
Conda-build sets appropriate optimization flags in CXXFLAGS. Adding -O3 may conflict with these settings and affect portability. It's recommended not to override optimization levels explicitly.
Apply this diff:
-DCMAKE_CXX_FLAGS="${CXXFLAGS} -O3 -I${PREFIX}/include" \
+-DCMAKE_CXX_FLAGS="${CXXFLAGS} -I${PREFIX}/include" \📝 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.
| -DCMAKE_CXX_FLAGS="${CXXFLAGS} -O3 -I${PREFIX}/include" \ | |
| -DCMAKE_CXX_FLAGS="${CXXFLAGS} -I${PREFIX}/include" \ |
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>.