update d4tools and d4binding #48077
update d4tools and d4binding #48077
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a series of changes primarily focused on managing build failures across various recipes. A comprehensive blacklist of recipes has been created, categorizing issues such as compilation failures, missing dependencies, outdated source URLs, and excessive resource consumption. Specific recipes have been flagged for problems including compilation errors on Linux and the need for migration to conda-forge. Additionally, the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 yamllint
🔇 Additional comments (6)
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: 8
🧹 Outside diff range and nitpick comments (4)
recipes/d4binding/build.sh (2)
Line range hint
14-15: Consider adding error handling for install.sh execution.While the script uses
-eflag for error handling, explicit error handling for the installation step would provide better debugging information.Consider wrapping the installation step:
-cd d4binding -./install.sh +cd d4binding || exit 1 +if ! ./install.sh; then + echo "Error: d4binding installation failed" + exit 1 +fi
I can now generate the final response based on the meta.yaml content.
The TODO comment about Rust compiler pinning needs to be addressed
The meta.yaml file currently uses
rust >=1.30in the build requirements instead of the recommended{{ compiler('rust') }}syntax. This confirms the validity of the TODO comment. The current approach:
- Doesn't follow Bioconda's best practices for compiler pinning
- May lead to less reproducible builds
- Is acknowledged in skip-lints section, but specifically for gcc usage, not Rust
Required changes:
- Replace
rust >=1.30with{{ compiler('rust') }}in meta.yaml's build requirements- Remove the TODO comment and related environment variable exports in build.sh once the change is made
🔗 Analysis chain
Line range hint
4-7: Address the TODO comment about Rust compiler pinning.The TODO comment indicates that the
CARGO_*environment variables are a temporary solution. According to Bioconda best practices, using the conda-forge's Rust compiler through{{ compiler('rust') }}in meta.yaml is preferred for better build reproducibility.Let's verify if the Rust compiler is properly specified in meta.yaml:
Let me gather more information about the meta.yaml file to better understand the build requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if meta.yaml properly specifies the Rust compiler rg --type yaml 'compiler.*rust' recipes/d4binding/meta.yamlLength of output: 61
Script:
#!/bin/bash # Let's first check if the meta.yaml file exists and see its content cat recipes/d4binding/meta.yamlLength of output: 1148
recipes/d4binding/meta.yaml (1)
Line range hint
14-14: TODO comment needs attention.The TODO comment references waiting for version 0.3.5, but the package is currently at version 0.3.4. This comment may be outdated since we're using a formal release tag v0.3.4-3.
Consider removing or updating this TODO comment to reflect the current status.
recipes/d4binding/build_htslib.sh (1)
1-63: Consider adding better error handling and logging.For a build script in a bioconda recipe, consider these improvements:
- Add a logging function to standardize output
- Add more descriptive error messages
- Consider adding cleanup trap for interrupted builds
Example logging function:
log() { echo "[$(date +'%Y-%m-%dT%H:%M:%S%z')]: $*" >&2 } trap 'log "Error: Build failed on line $LINENO"' ERRThis will help with debugging failed builds in the bioconda infrastructure.
🧰 Tools
🪛 Shellcheck
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- build-fail-blacklist (0 hunks)
- recipes/d4binding/build.sh (1 hunks)
- recipes/d4binding/build_htslib.sh (1 hunks)
- recipes/d4binding/meta.yaml (1 hunks)
- recipes/d4tools/meta.yaml (1 hunks)
💤 Files with no reviewable changes (1)
- build-fail-blacklist
🧰 Additional context used
🪛 Shellcheck
recipes/d4binding/build_htslib.sh
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (5)
recipes/d4binding/meta.yaml (2)
8-10: LGTM: Appropriate version pinning strategy added.The addition of
run_exportswith strict version pinning (x.x.x) aligns with Bioconda's guidelines for preventing API/ABI breakage. This ensures that downstream packages will be constrained to the exact version they were built against.
Line range hint
15-16: Verify the source URL and checksum.The source URL uses a specific tag format
v{{ version }}-3. Let's verify this is the correct release.recipes/d4tools/meta.yaml (3)
Line range hint
1-58: LGTM! The recipe updates look good.The changes include:
- Version bump to 0.3.11
- Build number increment
- Added run_exports for better dependency management
- Added linux-aarch64 platform support
The recipe maintains all necessary build requirements and follows Bioconda's best practices.
12-14: Verify the run_exports pinning strategy.The addition of
run_exportswithmax_pin="x.x"means downstream packages will be constrained to the same minor version. This is generally good practice for maintaining ABI compatibility, but let's verify this is the intended constraint level for d4tools.
Line range hint
8-9: Verify SHA256 checksum for the new version.Let's verify the SHA256 checksum matches the source tarball.
| @@ -8,5 +8,7 @@ export \ | |||
| CARGO_NET_GIT_FETCH_WITH_CLI=true \ | |||
| CARGO_HOME="${BUILD_PREFIX}/.cargo" | |||
|
|
|||
| cp $RECIPE_DIR/build_htslib.sh d4-hts/build_htslib.sh | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance robustness of the build_htslib.sh copy operation.
The current copy operation lacks error handling and directory verification. Consider adding these safety checks to prevent build failures.
Here's a more robust implementation:
-cp $RECIPE_DIR/build_htslib.sh d4-hts/build_htslib.sh
+if [ ! -f "$RECIPE_DIR/build_htslib.sh" ]; then
+ echo "Error: build_htslib.sh not found in recipe directory"
+ exit 1
+fi
+mkdir -p d4-hts
+cp "$RECIPE_DIR/build_htslib.sh" d4-hts/build_htslib.sh
+chmod +x d4-hts/build_htslib.sh📝 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.
| cp $RECIPE_DIR/build_htslib.sh d4-hts/build_htslib.sh | |
| if [ ! -f "$RECIPE_DIR/build_htslib.sh" ]; then | |
| echo "Error: build_htslib.sh not found in recipe directory" | |
| exit 1 | |
| fi | |
| mkdir -p d4-hts | |
| cp "$RECIPE_DIR/build_htslib.sh" d4-hts/build_htslib.sh | |
| chmod +x d4-hts/build_htslib.sh |
|
|
||
| set -ex | ||
|
|
||
| pushd ${OUT_DIR} |
There was a problem hiding this comment.
Validate required environment variables.
The script assumes OUT_DIR is set but doesn't validate it. Add validation to prevent unclear errors:
+if [ -z "${OUT_DIR}" ]; then
+ echo "ERROR: OUT_DIR environment variable is not set"
+ exit 1
+fi
+
pushd ${OUT_DIR}📝 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.
| pushd ${OUT_DIR} | |
| if [ -z "${OUT_DIR}" ]; then | |
| echo "ERROR: OUT_DIR environment variable is not set" | |
| exit 1 | |
| fi | |
| pushd ${OUT_DIR} |
|
|
||
| rm -rf ${OUT_DIR}/htslib | ||
|
|
||
| git clone -b ${HTSLIB_VERSION} http://github.com/samtools/htslib.git |
There was a problem hiding this comment.
Use HTTPS instead of HTTP for git clone.
Using HTTP for git operations is less secure. Switch to HTTPS:
-git clone -b ${HTSLIB_VERSION} http://github.com/samtools/htslib.git
+git clone -b ${HTSLIB_VERSION} https://github.com/samtools/htslib.git📝 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.
| git clone -b ${HTSLIB_VERSION} http://github.com/samtools/htslib.git | |
| git clone -b ${HTSLIB_VERSION} https://github.com/samtools/htslib.git |
| exit 0 | ||
| fi | ||
|
|
||
| set -ex |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance shell script safety settings.
Consider adding additional safety flags to prevent common shell script issues:
-set -ex
+set -euxo pipefailThis change will:
-u: Exit on undefined variables-o pipefail: Ensure pipeline failures are caught
📝 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.
| set -ex | |
| set -euxo pipefail |
| function is_musl() { | ||
| if [ ! -z $(echo $TARGET | grep musl) ]; then | ||
| return 0 | ||
| else | ||
| return 1 | ||
| fi | ||
| } |
There was a problem hiding this comment.
Fix shell quoting in musl detection.
The current implementation is vulnerable to word splitting. Quote the grep result:
function is_musl() {
- if [ ! -z $(echo $TARGET | grep musl) ]; then
+ if [ ! -z "$(echo "$TARGET" | grep musl)" ]; then
return 0
else
return 1
fi
}📝 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.
| function is_musl() { | |
| if [ ! -z $(echo $TARGET | grep musl) ]; then | |
| return 0 | |
| else | |
| return 1 | |
| fi | |
| } | |
| function is_musl() { | |
| if [ ! -z "$(echo "$TARGET" | grep musl)" ]; then | |
| return 0 | |
| else | |
| return 1 | |
| fi | |
| } |
🧰 Tools
🪛 Shellcheck
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
|
|
||
| is_musl || perl -i -pe 's/CFLAGS *=/CFLAGS = -fPIC/g' Makefile | ||
|
|
||
| make -j8 lib-static |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize parallel compilation.
Instead of hardcoding -j8, use the available CPU cores:
-make -j8 lib-static
+nproc_bin="$(command -v nproc)"
+if [ -n "$nproc_bin" ]; then
+ make -j"$($nproc_bin)" lib-static
+else
+ make -j8 lib-static
+fi📝 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.
| make -j8 lib-static | |
| nproc_bin="$(command -v nproc)" | |
| if [ -n "$nproc_bin" ]; then | |
| make -j"$($nproc_bin)" lib-static | |
| else | |
| make -j8 lib-static | |
| fi |
| #The original file in the repo is lacking -L in the curl command so it doen't work | ||
| curl -L http://sourceware.org/pub/bzip2/bzip2-1.0.8.tar.gz | tar xz | ||
| cd bzip2-1.0.8 |
There was a problem hiding this comment.
Fix typo in comment and enhance bzip2 download safety.
The comment has a typo and the download lacks safety measures:
-#The original file in the repo is lacking -L in the curl command so it doen't work
-curl -L http://sourceware.org/pub/bzip2/bzip2-1.0.8.tar.gz | tar xz
+# The original file in the repo is lacking -L in the curl command so it doesn't work
+BZIP2_SHA256="ab5a03176ee106d3f0fa90e381da478ddae405918153cca248e682cd0c4a2269"
+curl --retry 3 --retry-delay 3 -L 'https://sourceware.org/pub/bzip2/bzip2-1.0.8.tar.gz' -o bzip2.tar.gz
+echo "${BZIP2_SHA256} bzip2.tar.gz" | sha256sum -c
+tar xzf bzip2.tar.gz
+rm bzip2.tar.gz📝 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.
| #The original file in the repo is lacking -L in the curl command so it doen't work | |
| curl -L http://sourceware.org/pub/bzip2/bzip2-1.0.8.tar.gz | tar xz | |
| cd bzip2-1.0.8 | |
| # The original file in the repo is lacking -L in the curl command so it doesn't work | |
| BZIP2_SHA256="ab5a03176ee106d3f0fa90e381da478ddae405918153cca248e682cd0c4a2269" | |
| curl --retry 3 --retry-delay 3 -L 'https://sourceware.org/pub/bzip2/bzip2-1.0.8.tar.gz' -o bzip2.tar.gz | |
| echo "${BZIP2_SHA256} bzip2.tar.gz" | sha256sum -c | |
| tar xzf bzip2.tar.gz | |
| rm bzip2.tar.gz | |
| cd bzip2-1.0.8 |
| if [ "x${ZLIB_SRC}" != "x" ] | ||
| then | ||
| tar xz ${ZLIB_SRC} | ||
| else | ||
| curl -L 'http://github.com/madler/zlib/archive/refs/tags/v1.2.11.tar.gz' | tar xz | ||
| fi |
There was a problem hiding this comment.
Enhance download safety for zlib.
The current implementation lacks important safety measures:
else
- curl -L 'http://github.com/madler/zlib/archive/refs/tags/v1.2.11.tar.gz' | tar xz
+ ZLIB_SHA256="629380c90a77892b5c0119a3be631f21e391faa5e93c7b5a5e2ad35d84f7f178"
+ curl --retry 3 --retry-delay 3 -L 'https://github.com/madler/zlib/archive/refs/tags/v1.2.11.tar.gz' -o zlib.tar.gz
+ echo "${ZLIB_SHA256} zlib.tar.gz" | sha256sum -c
+ tar xzf zlib.tar.gz
+ rm zlib.tar.gz
fiChanges:
- Added SHA256 checksum verification
- Added retry logic for network resilience
- Switched to HTTPS
- Added explicit download instead of piping to tar
📝 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.
| if [ "x${ZLIB_SRC}" != "x" ] | |
| then | |
| tar xz ${ZLIB_SRC} | |
| else | |
| curl -L 'http://github.com/madler/zlib/archive/refs/tags/v1.2.11.tar.gz' | tar xz | |
| fi | |
| if [ "x${ZLIB_SRC}" != "x" ] | |
| then | |
| tar xz ${ZLIB_SRC} | |
| else | |
| ZLIB_SHA256="629380c90a77892b5c0119a3be631f21e391faa5e93c7b5a5e2ad35d84f7f178" | |
| curl --retry 3 --retry-delay 3 -L 'https://github.com/madler/zlib/archive/refs/tags/v1.2.11.tar.gz' -o zlib.tar.gz | |
| echo "${ZLIB_SHA256} zlib.tar.gz" | sha256sum -c | |
| tar xzf zlib.tar.gz | |
| rm zlib.tar.gz | |
| fi |
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>.