Update hmftools CHORD and CUPPA #52175
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications 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: 1
🧹 Outside diff range and nitpick comments (1)
recipes/hmftools-cuppa/meta.yaml (1)
1-3: Consider consolidating version and beta suffix.Instead of splitting the beta version across two variables, consider using a single version variable for better clarity and maintainability.
-{% set version = "2.3.0_beta" %} -{% set beta_suffix = ".1" %} +{% set version = "2.3.0_beta.1" %}🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
recipes/hmftools-chord/meta.yaml(2 hunks)recipes/hmftools-cuppa/meta.yaml(1 hunks)
🔇 Additional comments (4)
recipes/hmftools-chord/meta.yaml (3)
15-15: LGTM! Build number increment is appropriate
The build number increment follows conda-build guidelines and is appropriate for the version update.
Line range hint 1-38: Package configuration follows bioconda best practices
The package configuration is well-structured with:
- Proper version pinning through run_exports
- Appropriate dependencies specification
- Version verification in test command
- Clear license and metadata
1-3: Verify version and checksum against upstream release
The version update and checksum change should be validated against the official release.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
recipes/hmftools-cuppa/meta.yaml (1)
15-18: LGTM! Build configuration follows best practices.
The build number increment and run_exports configuration are correctly implemented, following Bioconda guidelines for version pinning and ABI compatibility.
recipes/hmftools-cuppa/meta.yaml
Outdated
| url: https://github.com/hartwigmedical/hmftools/releases/download/cuppa-v{{ version }}/cuppa-{{ version|replace(".0", "") }}{{ beta_suffix }}.jar | ||
| sha256: '{{ sha256 }}' |
There was a problem hiding this comment.
💡 Codebase verification
URL pattern in meta.yaml needs correction
The current URL pattern in meta.yaml is incorrect. The actual asset names in the release follow a different pattern:
- Current pattern:
cuppa-2.3_beta.1.jar - Actual pattern:
cuppa_v2.3_beta.1.jar
The differences are:
- Uses underscore (
_) instead of hyphen (-) after "cuppa" - Includes "v" in the version part
🔗 Analysis chain
Verify the GitHub release URL accessibility.
The URL construction involves multiple string manipulations. Let's verify that the release artifact is accessible.
Let me verify the correct URL pattern by checking the repository's releases.
Let me check specifically for CUPPA releases to understand their naming pattern.
Let me check the actual assets in the specific CUPPA release we're interested in.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the GitHub release artifact exists
# Extract version and construct URL
VERSION="2.3.0_beta"
BETA_SUFFIX=".1"
URL="https://github.com/hartwigmedical/hmftools/releases/download/cuppa-v${VERSION}/cuppa-${VERSION/.0/}${BETA_SUFFIX}.jar"
# Check if the URL is accessible
curl -sI "$URL" | head -n 1
Length of output: 278
Script:
#!/bin/bash
# Check recent releases and their asset patterns
gh release list --repo hartwigmedical/hmftools --limit 5
Length of output: 459
Script:
#!/bin/bash
# Search for CUPPA releases
gh release list --repo hartwigmedical/hmftools | grep -i cuppa
Length of output: 865
Script:
#!/bin/bash
# Check assets for CUPPA v2.3 beta release
gh release view cuppa-v2.3.0_beta --repo hartwigmedical/hmftools
Length of output: 478
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/hmftools-cuppa/build.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/hmftools-cuppa/build.sh
[warning] 7-7: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🔇 Additional comments (2)
recipes/hmftools-cuppa/build.sh (2)
4-5: LGTM: Directory creation looks good
The directory creation commands are properly implemented using the -p flag for safe creation.
10-12: LGTM: Script setup is correct
The executable script is properly set up with appropriate permissions.
| cd "${SRC_DIR}" | ||
| mv jar/cuppa*.jar $TGT/cuppa.jar | ||
| ${PYTHON} -m pip install --no-build-isolation --no-deps --no-cache-dir -vvv src/cuppa/src/main/python/pycuppa/ | ||
| mv cuppa*.jar $TGT/cuppa.jar |
There was a problem hiding this comment.
Add error handling for directory change operation
The cd command should handle potential failures to prevent script from continuing in wrong directory.
Apply this diff to add error handling:
-cd "${SRC_DIR}"
+cd "${SRC_DIR}" || exit 1Also, consider making the JAR file pattern more specific to avoid potential conflicts:
-mv cuppa*.jar $TGT/cuppa.jar
+mv cuppa-*.jar $TGT/cuppa.jar📝 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 "${SRC_DIR}" | |
| mv jar/cuppa*.jar $TGT/cuppa.jar | |
| ${PYTHON} -m pip install --no-build-isolation --no-deps --no-cache-dir -vvv src/cuppa/src/main/python/pycuppa/ | |
| mv cuppa*.jar $TGT/cuppa.jar | |
| cd "${SRC_DIR}" || exit 1 | |
| mv cuppa-*.jar $TGT/cuppa.jar |
🧰 Tools
🪛 Shellcheck
[warning] 7-7: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
recipes/hmftools-cuppa/build.sh
Outdated
| mkdir -p /tmp/cuppa_jar/ | ||
| RUN unzip -n cuppa*.jar -d /tmp/cuppa_jar/ | ||
| ${PYTHON} -m pip install --no-build-isolation --no-deps --no-cache-dir -vvv /tmp/cuppa_jar/pycuppa/ | ||
| RUN rm -r /tmp/cuppa_jar/ |
There was a problem hiding this comment.
Fix shell script syntax and add error handling
There are several issues in this section:
- The
RUNprefix is incorrect (appears to be copied from a Dockerfile) - Missing error handling for critical operations
- Temporary directory should use mktemp for safety
Apply this diff to fix the issues:
-mkdir -p /tmp/cuppa_jar/
-RUN unzip -n cuppa*.jar -d /tmp/cuppa_jar/
-${PYTHON} -m pip install --no-build-isolation --no-deps --no-cache-dir -vvv /tmp/cuppa_jar/pycuppa/
-RUN rm -r /tmp/cuppa_jar/
+TEMP_DIR=$(mktemp -d)
+trap 'rm -rf "${TEMP_DIR}"' EXIT
+
+unzip -n cuppa*.jar -d "${TEMP_DIR}" || exit 1
+${PYTHON} -m pip install --no-build-isolation --no-deps --no-cache-dir -vvv "${TEMP_DIR}/pycuppa/" || exit 1The changes:
- Use
mktempfor secure temporary directory creation - Add cleanup trap to ensure temporary directory is removed
- Remove incorrect
RUNprefix - Add error handling for critical operations
📝 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.
| mkdir -p /tmp/cuppa_jar/ | |
| RUN unzip -n cuppa*.jar -d /tmp/cuppa_jar/ | |
| ${PYTHON} -m pip install --no-build-isolation --no-deps --no-cache-dir -vvv /tmp/cuppa_jar/pycuppa/ | |
| RUN rm -r /tmp/cuppa_jar/ | |
| TEMP_DIR=$(mktemp -d) | |
| trap 'rm -rf "${TEMP_DIR}"' EXIT | |
| unzip -n cuppa*.jar -d "${TEMP_DIR}" || exit 1 | |
| ${PYTHON} -m pip install --no-build-isolation --no-deps --no-cache-dir -vvv "${TEMP_DIR}/pycuppa/" || exit 1 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
recipes/hmftools-cuppa/build.sh (1)
8-8: Use more specific JAR file patternThe current pattern
cuppa*.jaris too loose and could match unexpected files.Apply this diff:
-mv cuppa*.jar $TGT/cuppa.jar +mv cuppa-*.jar "$TGT/cuppa.jar" || exit 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/hmftools-cuppa/build.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/hmftools-cuppa/build.sh
[warning] 7-7: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
| [ -d "$TGT" ] || mkdir -p $TGT | ||
| [ -d "${PREFIX}/bin" ] || mkdir -p "${PREFIX}/bin" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper quoting and error handling for directory creation
The directory creation commands should be properly quoted and include error handling to ensure script reliability.
Apply this diff:
-[ -d "$TGT" ] || mkdir -p $TGT
-[ -d "${PREFIX}/bin" ] || mkdir -p "${PREFIX}/bin"
+[ -d "$TGT" ] || mkdir -p "$TGT" || exit 1
+[ -d "${PREFIX}/bin" ] || mkdir -p "${PREFIX}/bin" || 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.
| [ -d "$TGT" ] || mkdir -p $TGT | |
| [ -d "${PREFIX}/bin" ] || mkdir -p "${PREFIX}/bin" | |
| [ -d "$TGT" ] || mkdir -p "$TGT" || exit 1 | |
| [ -d "${PREFIX}/bin" ] || mkdir -p "${PREFIX}/bin" || exit 1 |
|
|
||
| cp $RECIPE_DIR/cuppa.sh $TGT/cuppa | ||
| ln -s $TGT/cuppa ${PREFIX}/bin/ | ||
| chmod 0755 "${PREFIX}/bin/cuppa" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for chmod operation
Critical operations like chmod should include error handling.
Apply this diff:
-chmod 0755 "${PREFIX}/bin/cuppa"
+chmod 0755 "${PREFIX}/bin/cuppa" || 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.
| chmod 0755 "${PREFIX}/bin/cuppa" | |
| chmod 0755 "${PREFIX}/bin/cuppa" || exit 1 |
recipes/hmftools-cuppa/build.sh
Outdated
| mkdir -p /tmp/cuppa_jar/ | ||
| unzip -n cuppa*.jar -d /tmp/cuppa_jar/ | ||
| ${PYTHON} -m pip install --no-build-isolation --no-deps --no-cache-dir -vvv /tmp/cuppa_jar/pycuppa/ | ||
| rm -r /tmp/cuppa_jar/ |
There was a problem hiding this comment.
Critical: Improve temporary directory handling and add error handling
The current implementation has several critical issues:
- Uses hardcoded temporary directory
- Lacks cleanup on script failure
- Missing error handling for critical operations
Apply this diff to fix these issues:
-mkdir -p /tmp/cuppa_jar/
-unzip -n cuppa*.jar -d /tmp/cuppa_jar/
-${PYTHON} -m pip install --no-build-isolation --no-deps --no-cache-dir -vvv /tmp/cuppa_jar/pycuppa/
-rm -r /tmp/cuppa_jar/
+TEMP_DIR=$(mktemp -d) || exit 1
+trap 'rm -rf "${TEMP_DIR}"' EXIT
+
+unzip -n cuppa-*.jar -d "${TEMP_DIR}" || exit 1
+${PYTHON} -m pip install --no-build-isolation --no-deps --no-cache-dir -vvv "${TEMP_DIR}/pycuppa/" || exit 1These changes:
- Use
mktempfor secure temporary directory creation - Add cleanup trap to ensure temporary directory is always removed
- Add error handling for critical operations
- Use more specific JAR file pattern
📝 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.
| mkdir -p /tmp/cuppa_jar/ | |
| unzip -n cuppa*.jar -d /tmp/cuppa_jar/ | |
| ${PYTHON} -m pip install --no-build-isolation --no-deps --no-cache-dir -vvv /tmp/cuppa_jar/pycuppa/ | |
| rm -r /tmp/cuppa_jar/ | |
| TEMP_DIR=$(mktemp -d) || exit 1 | |
| trap 'rm -rf "${TEMP_DIR}"' EXIT | |
| unzip -n cuppa-*.jar -d "${TEMP_DIR}" || exit 1 | |
| ${PYTHON} -m pip install --no-build-isolation --no-deps --no-cache-dir -vvv "${TEMP_DIR}/pycuppa/" || exit 1 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/hmftools-cuppa/build.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/hmftools-cuppa/build.sh
[warning] 7-7: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🔇 Additional comments (4)
recipes/hmftools-cuppa/build.sh (4)
4-5: Previous review comment is still applicable
The issues with directory creation commands regarding proper quoting and error handling have not been addressed.
7-8: Previous review comments are still applicable
The issues regarding error handling for the cd command and the JAR file pattern specificity have not been addressed.
🧰 Tools
🪛 Shellcheck
[warning] 7-7: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
12-12: Previous review comment is still applicable
The issue regarding error handling for the chmod operation has not been addressed.
14-17: Previous review comments about temporary directory handling are still applicable
The issues regarding secure temporary directory creation, cleanup, and error handling have not been addressed.
| mkdir -p /tmp/cuppa_jar/ | ||
| unzip -n $TGT/cuppa.jar -d /tmp/cuppa_jar/ | ||
| ${PYTHON} -m pip install --no-build-isolation --no-deps --no-cache-dir -vvv /tmp/cuppa_jar/pycuppa/ | ||
| rm -r /tmp/cuppa_jar/ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize file operations to avoid unnecessary moves
The current implementation moves the JAR file to the target directory and then unzips it from there, which is inefficient.
Apply this diff to optimize the operations:
-mv cuppa*.jar $TGT/cuppa.jar
+TEMP_DIR=$(mktemp -d) || exit 1
+trap 'rm -rf "${TEMP_DIR}"' EXIT
-mkdir -p /tmp/cuppa_jar/
-unzip -n $TGT/cuppa.jar -d /tmp/cuppa_jar/
-${PYTHON} -m pip install --no-build-isolation --no-deps --no-cache-dir -vvv /tmp/cuppa_jar/pycuppa/
-rm -r /tmp/cuppa_jar/
+# First unzip and install from the source
+unzip -n cuppa-*.jar -d "${TEMP_DIR}" || exit 1
+${PYTHON} -m pip install --no-build-isolation --no-deps --no-cache-dir -vvv "${TEMP_DIR}/pycuppa/" || exit 1
+
+# Then move the JAR to final location
+mv cuppa-*.jar $TGT/cuppa.jar || exit 1Committable suggestion skipped: line range outside the PR's diff.
|
@BiocondaBot please fetch artifacts |
|
Package(s) built are ready for inspection:
|
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>.