Update mosdepth recipe #48070
Update mosdepth recipe #48070
Conversation
|
@BiocondaBot please fetch artifacts |
|
Package(s) built are ready for inspection:
Docker image(s) built:
|
|
Hey @brentp, do you happen to know why the compilation is not successful on osx? It seems that d4 is installed correctly, but the compilation with nim is failing. |
|
Tried to compile the package on my laptop, following the steps defined in the build.sh. I got this |
|
The build passes now but it fails at runtime with: |
I have the feeling this error is related to brentp/mosdepth#232 |
|
The same error on |
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant updates to the 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: 1
🧹 Outside diff range and nitpick comments (1)
recipes/mosdepth/build.sh (1)
3-3: Add quotes around the variable in platform check.While
target_platformis set by conda-build, it's safer to quote the variable to handle potential spaces or special characters:-if [[ ${target_platform} == "osx-64" ]] ; then +if [[ "${target_platform}" == "osx-64" ]] ; then🧰 Tools
🪛 Shellcheck
[warning] 3-3: target_platform is referenced but not assigned.
(SC2154)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/mosdepth/build.sh (1 hunks)
- recipes/mosdepth/meta.yaml (2 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/mosdepth/build.sh
[warning] 3-3: target_platform is referenced but not assigned.
(SC2154)
[warning] 6-6: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 11-11: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🪛 yamllint
recipes/mosdepth/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (9)
recipes/mosdepth/meta.yaml (5)
1-2: LGTM! Good use of Jinja2 templating.The version bump and use of variables for package metadata follows best practices.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
Line range hint
13-16: LGTM! Appropriate run_exports configuration.The run_exports configuration with x.x pin will help prevent API/ABI breakage issues.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
32-44: LGTM! Improved documentation and metadata.Good improvements:
- Version-specific README link
- Complete license information
- Additional tool identifier
8-10: Verify source URL and checksum.The source configuration looks correct, but let's verify the URL accessibility and checksum.
#!/bin/bash # Description: Verify source URL accessibility and checksum URL="https://github.com/brentp/mosdepth/archive/refs/tags/v0.3.9.tar.gz" EXPECTED_SHA="9171ea9a6ddaccd0091db5b85fa9e6cb79516bbe005c47ffc8dcfe49c978eb69" # Test: Check if URL is accessible curl -sIL "$URL" | head -n1 # Test: Verify SHA256 checksum echo "Expected SHA256: $EXPECTED_SHA" echo "Actual SHA256: $(curl -sL "$URL" | sha256sum | cut -d' ' -f1)"
21-25: Verify htslib version requirement.The addition of d4binding addresses the runtime errors mentioned in the PR comments. However, let's verify if the htslib version bump to >=1.21 is necessary.
recipes/mosdepth/build.sh (4)
22-24: LGTM! Installation steps follow best practices.The final installation steps correctly set up the binary with appropriate permissions.
1-24: Verify runtime dependencies are properly handled.Given the reported runtime errors with
libd4binding.dylibmentioned in the PR comments, we should ensure that all runtime dependencies are properly specified in the correspondingmeta.yamlfile.#!/bin/bash # Description: Check meta.yaml for d4binding dependency and related configurations # Look for d4binding in meta.yaml echo "Checking meta.yaml for d4binding configuration..." rg "d4binding" recipes/mosdepth/meta.yaml # Look for run_exports specifications echo "Checking for run_exports..." rg "run_exports" recipes/mosdepth/meta.yaml🧰 Tools
🪛 Shellcheck
[warning] 3-3: target_platform is referenced but not assigned.
(SC2154)
[warning] 6-6: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 11-11: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
12-15: Verify the build flags are sufficient for OSX compilation.The build flags look correct and include the necessary paths for dependencies. However, let's verify if these flags resolve the compilation issues mentioned in the PR comments.
#!/bin/bash # Description: Check if there are any open issues about OSX compilation failures # with similar build flags rg -l "passC.*PREFIX.*include.*passL.*PREFIX.*lib.*OSX" . rg -l "CFLAGS.*PREFIX.*include.*LDFLAGS.*PREFIX.*lib.*OSX" .
17-19: Verify binary compatibility across Linux distributions.The script downloads a pre-built binary for Linux platforms. We should ensure this binary is compatible with the target Linux distributions.
✅ Verification successful
Let me gather more information about the binary compatibility by checking the meta.yaml and related files for any runtime requirements or platform specifications.
Let me check the referenced GitHub issue and examine the binary's dependencies to better understand the compatibility requirements.
Based on the gathered information, I can now provide a final response:
Binary compatibility is properly handled through package requirements
The recipe appropriately manages binary compatibility through:
- Runtime dependency on
d4bindingwhich ensures necessary shared libraries are available- Platform-specific build logic (separate handling for OSX vs Linux)
- Proper conda build requirements that ensure ABI compatibility through run_exports
The pre-built binary approach is specifically chosen to address issue #232 and the package's requirements ensure it will work across Linux distributions through conda's dependency resolution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any reported compatibility issues with the pre-built binary # Look for related discussions about binary compatibility rg -l "mosdepth.*binary.*compatibility" . rg -l "mosdepth_d4.*binary.*compatibility" .Length of output: 354
Script:
#!/bin/bash # Check meta.yaml for platform specifications and requirements cat recipes/mosdepth/meta.yaml # Check if there are any platform-specific build scripts fd build.sh recipes/mosdepth --exec grep -l "platform:" {} # Look for any discussions about the mosdepth_d4 binary in issues or documentation rg -i "mosdepth_d4" . # Check for any platform constraints or skip-related configurations rg "skip.*\[(.*linux.*)\]" recipes/mosdepth/meta.yamlLength of output: 1405
Script:
#!/bin/bash # Check the binary's dependencies and architecture curl -sL https://github.com/brentp/mosdepth/releases/download/v0.3.9/mosdepth_d4 -o mosdepth_binary file mosdepth_binary ldd mosdepth_binary || echo "ldd not available" # Look for any skip directives or platform-specific conditions in the recipe rg "skip|selector" recipes/mosdepth/ # Check build.sh for any platform-specific logic cat recipes/mosdepth/build.sh # Look for any related discussions in README or documentation rg -i "compatibility|requirements|linux" recipes/mosdepth/Length of output: 1513
recipes/mosdepth/build.sh
Outdated
| cd nim-2.2.* | ||
| export PATH="$PWD/bin:$PATH" | ||
| cd .. | ||
| curl -SL https://github.com/brentp/mosdepth/archive/refs/tags/v${PKG_VERSION}.tar.gz -o mosdepth-latest.tar.gz | ||
| tar -xzf mosdepth-latest.tar.gz | ||
| cd mosdepth-${PKG_VERSION} |
There was a problem hiding this comment.
Add error handling for directory changes.
The cd commands should handle potential failures:
- cd nim-2.2.*
+ cd nim-2.2.* || exit 1
export PATH="$PWD/bin:$PATH"
- cd ..
+ cd .. || exit 1
curl -SL https://github.com/brentp/mosdepth/archive/refs/tags/v${PKG_VERSION}.tar.gz -o mosdepth-latest.tar.gz
tar -xzf mosdepth-latest.tar.gz
- cd mosdepth-${PKG_VERSION}
+ cd mosdepth-${PKG_VERSION} || exit 1📝 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.
| cd nim-2.2.* | |
| export PATH="$PWD/bin:$PATH" | |
| cd .. | |
| curl -SL https://github.com/brentp/mosdepth/archive/refs/tags/v${PKG_VERSION}.tar.gz -o mosdepth-latest.tar.gz | |
| tar -xzf mosdepth-latest.tar.gz | |
| cd mosdepth-${PKG_VERSION} | |
| cd nim-2.2.* || exit 1 | |
| export PATH="$PWD/bin:$PATH" | |
| cd .. || exit 1 | |
| curl -SL https://github.com/brentp/mosdepth/archive/refs/tags/v${PKG_VERSION}.tar.gz -o mosdepth-latest.tar.gz | |
| tar -xzf mosdepth-latest.tar.gz | |
| cd mosdepth-${PKG_VERSION} || exit 1 |
🧰 Tools
🪛 Shellcheck
[warning] 6-6: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 11-11: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
recipes/mosdepth/build.sh (2)
3-3: Improve platform check robustnessThe platform check could be more robust:
- The
target_platformvariable should be verified- String comparison could use
=instead of==for POSIX compatibility-if [[ ${target_platform} == "osx-64" ]] ; then +if [ -n "${target_platform}" ] && [ "${target_platform}" = "osx-64" ]; then🧰 Tools
🪛 Shellcheck
[warning] 3-3: target_platform is referenced but not assigned.
(SC2154)
27-28: Improve installation robustnessThe installation steps could be more robust by checking the binary exists and verifying the move operation.
-chmod 0755 mosdepth -mv mosdepth "${PREFIX}/bin/" +[ -f mosdepth ] || exit 1 +chmod 0755 mosdepth +mv mosdepth "${PREFIX}/bin/" || exit 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/mosdepth/build.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/mosdepth/build.sh
[warning] 3-3: target_platform is referenced but not assigned.
(SC2154)
[warning] 6-6: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 16-16: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🔇 Additional comments (1)
recipes/mosdepth/build.sh (1)
21-23: Verify binary compatibility across platforms
The script downloads a pre-built binary. We should verify it works across all supported non-OSX platforms.
| tar -xzf macosx_x64.tar.xz | ||
| cd nim-1.6.* | ||
| cd nim-2.2.* |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for directory changes
Directory changes should handle potential failures.
- cd nim-2.2.*
+ cd nim-2.2.* || exit 1
export PATH="$PWD/bin:$PATH"
- cd ..
+ cd .. || exit 1
...
- cd mosdepth-0.3.9
+ cd mosdepth-0.3.9 || exit 1Also applies to: 8-8, 16-16
🧰 Tools
🪛 Shellcheck
[warning] 6-6: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
| curl -SL https://github.com/38/d4-format/archive/refs/tags/v0.3.11.tar.gz -o d4-format.tar.gz | ||
| tar -xzf d4-format.tar.gz | ||
| cp d4-format-*/d4binding/include/d4.h $PREFIX/include/ |
There was a problem hiding this comment.
Handle d4-format library installation
The script copies the header file but doesn't handle the corresponding library file. This could lead to runtime linking issues as mentioned in the PR comments about missing libd4binding.dylib.
curl -SL https://github.com/38/d4-format/archive/refs/tags/v0.3.11.tar.gz -o d4-format.tar.gz
tar -xzf d4-format.tar.gz
cp d4-format-*/d4binding/include/d4.h $PREFIX/include/
+# Build and install the d4binding library
+cd d4-format-* || exit 1
+make -C d4binding
+cp d4binding/libd4binding.* $PREFIX/lib/
+cd .. || exit 1📝 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.
| curl -SL https://github.com/38/d4-format/archive/refs/tags/v0.3.11.tar.gz -o d4-format.tar.gz | |
| tar -xzf d4-format.tar.gz | |
| cp d4-format-*/d4binding/include/d4.h $PREFIX/include/ | |
| curl -SL https://github.com/38/d4-format/archive/refs/tags/v0.3.11.tar.gz -o d4-format.tar.gz | |
| tar -xzf d4-format.tar.gz | |
| cp d4-format-*/d4binding/include/d4.h $PREFIX/include/ | |
| # Build and install the d4binding library | |
| cd d4-format-* || exit 1 | |
| make -C d4binding | |
| cp d4binding/libd4binding.* $PREFIX/lib/ | |
| cd .. || exit 1 |
This update relates to the issue descibed at brentp/mosdepth#232
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>.