Add hmftools-chord 2.1.0_beta #51149
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several new components for the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🪛 yamllint
🔇 Additional comments (8)
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 (
|
|
@BiocondaBot please fetch artifacts |
|
Package(s) built are ready for inspection:
Docker image(s) built:
|
|
@BiocondaBot please fetch artifacts |
|
Package(s) built are ready for inspection:
Docker image(s) built:
|
|
@BiocondaBot please add label |
![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 (9)
recipes/hmftools-chord/meta.yaml (6)
9-15: LGTM: Source URLs and checksums are well-defined.The source section correctly defines URLs for both the JAR file and source tarball, using GitHub release links. The use of Jinja2 templating for version and checksums ensures consistency.
Consider adding a comment explaining the purpose of including both JAR and source files, as it's not common practice in all Conda recipes. This can help future maintainers understand the rationale.
17-21: LGTM: Build section is well-configured.The build section is correctly configured:
noarch: genericis appropriate for a Java-based tool.- Build number 0 is correct for a new package.
- The
run_exportssection with pinning helps manage dependency versions, as recommended in the PR objectives.Consider adding a comment explaining the rationale behind the
max_pin="x.x"setting in therun_exportssection. This can help future maintainers understand the version constraint strategy.
23-38: LGTM: Dependencies are well-defined, but consider optimizing.The requirements section correctly separates host and run dependencies, including necessary R packages and Bioconductor resources. The Java requirement is appropriately specified.
Consider using YAML anchors and aliases to reduce duplication between host and run requirements. This can make the recipe more maintainable. For example:
requirements: _r_deps: &r_deps - r-base - r-randomforest - r-stringr - bioconductor-bsgenome - bioconductor-bsgenome.hsapiens.ucsc.hg19 - bioconductor-bsgenome.hsapiens.ucsc.hg38 host: - *r_deps run: - openjdk >=8 - *r_depsThis approach reduces redundancy and makes it easier to update dependencies in the future.
40-44: LGTM: Test commands are comprehensive.The test section includes appropriate commands to verify the installation:
- Loading required R libraries
- Checking the CHORD tool version
These tests ensure that both R dependencies and the main tool are correctly installed and accessible.
Consider adding a test to verify that the JAR file is present in the expected location. This can be done with a simple file existence check. For example:
test: commands: - test -f $PREFIX/share/hmftools-chord-{{ version }}/chord-{{ version }}.jar # ... existing commands ...This additional test would ensure that the JAR file is correctly packaged and installed.
46-50: LGTM: About section provides essential metadata.The about section correctly includes:
- Home page URL
- License information (GPL-3.0-only)
- A concise summary of the tool's functionality
Consider adding a
doc_urlfield pointing to the tool's documentation, if available. This can help users find more detailed information about the tool. For example:about: home: https://github.com/hartwigmedical/hmftools/blob/master/chord/ doc_url: https://github.com/hartwigmedical/hmftools/blob/master/chord/README.md # ... existing fields ...This addition would provide users with quick access to the tool's documentation.
1-1: Note: Ignore yamllint warning about '%' characterThe yamllint tool reported a syntax error for the '%' character at the beginning of the file. This is a false positive, as the '%' is part of the Jinja2 templating syntax commonly used in Conda recipes.
If you want to suppress this warning in future linting, you can add a
# yamllint disable-line rule:document-startcomment at the beginning of the file. However, this is generally not necessary for Conda recipes, as the Conda build system correctly interprets the Jinja2 syntax.🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
recipes/hmftools-chord/chord.sh (3)
1-20: LGTM with a minor suggestion for improvementThe script setup and environment configuration look good. The error handling and symlink resolution are well implemented. However, there's a minor improvement we can make to the
ENV_PREFIXcalculation to handle paths with spaces more robustly.Consider updating line 19 to use quotes:
-ENV_PREFIX="$(dirname $(dirname $DIR))" +ENV_PREFIX="$(dirname "$(dirname "$DIR")")"This change ensures that the
dirnamecommands handle paths with spaces correctly.🧰 Tools
🪛 Shellcheck
[warning] 19-19: Quote this to prevent word splitting.
(SC2046)
21-28: LGTM with a suggestion for additional validationThe Java executable selection logic is flexible and allows for system-specific installations. However, we can improve it by adding validation for the selected Java executable.
Consider adding a check to ensure the selected Java executable exists and is the correct version:
if [ -n "${JAVA_HOME:=}" ]; then if [ -e "$JAVA_HOME/bin/java" ]; then java="$JAVA_HOME/bin/java" fi fi + +# Validate Java executable +if ! command -v "$java" &> /dev/null; then + echo "Error: Java executable not found" >&2 + exit 1 +fi + +# Check Java version (adjust version number as needed) +if ! "$java" -version 2>&1 | grep -q "version \"1.8"; then + echo "Error: Java 8 is required" >&2 + exit 1 +fiThis addition ensures that the script fails early if the Java executable is not found or if it's not the correct version.
30-60: LGTM with a minor improvement for argument handlingThe argument processing logic is well-implemented, handling different types of arguments correctly and using quotes to preserve special characters. However, we can make a small improvement to address a potential word splitting issue.
To address the static analysis warning and improve robustness, consider using an array to store the processed arguments:
-pass_args="" +declare -a pass_args=() for arg in "$@"; do case $arg in '-D'*) jvm_prop_opts="$jvm_prop_opts $arg" ;; '-XX'*) jvm_prop_opts="$jvm_prop_opts $arg" ;; '-Xm'*) jvm_mem_opts="$jvm_mem_opts $arg" ;; *) - if [[ ${pass_args} == '' ]] #needed to avoid preceeding space on first arg e.g. ' MarkDuplicates' - then - pass_args="$arg" - else - pass_args="$pass_args \"$arg\"" #quotes later arguments to avoid problem with ()s in MarkDuplicates regex arg - fi + pass_args+=("$arg") ;; esac doneThis change ensures that all arguments are properly quoted and prevents potential issues with word splitting or globbing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- recipes/hmftools-chord/build.sh (1 hunks)
- recipes/hmftools-chord/chord.sh (1 hunks)
- recipes/hmftools-chord/meta.yaml (1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/hmftools-chord/build.sh
[warning] 7-7: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
recipes/hmftools-chord/chord.sh
[warning] 19-19: Quote this to prevent word splitting.
(SC2046)
[warning] 62-62: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
🪛 yamllint
recipes/hmftools-chord/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (7)
recipes/hmftools-chord/build.sh (4)
3-5: LGTM: Directory setup looks good.The script correctly sets up the target directory using environment variables and creates necessary directories. This approach ensures flexibility and follows best practices.
9-10:⚠️ Potential issueAdd error handling and directory checks for R package installation.
Consider adding error handling and directory checks for the R package installations:
+for dir in src/chord/src/main/R/mutSigExtractor src/chord/src/main/R/CHORD; do + if [ ! -d "$dir" ]; then + echo "Error: $dir not found" >&2 + exit 1 + fi + ${R} CMD INSTALL --build "$dir" || { echo "Error installing $dir" >&2; exit 1; } +done -${R} CMD INSTALL --build src/chord/src/main/R/mutSigExtractor -${R} CMD INSTALL --build src/chord/src/main/R/CHORDThis change will ensure that the required directories exist and that any installation failures are caught and reported.
To verify the existence of the R package source directories, you can run:
#!/bin/bash # Description: Check for the presence of R package source directories # Expected result: Both R package source directories should exist # Test: Search for R package source directories fd -t d '(mutSigExtractor|CHORD)$' recipes/hmftools-chord
12-13:⚠️ Potential issueAdd error handling and file checks for final setup operations.
Consider adding error handling and file checks for the final setup operations:
+if [ ! -f "$RECIPE_DIR/chord.sh" ]; then + echo "Error: $RECIPE_DIR/chord.sh not found" >&2 + exit 1 +fi -cp $RECIPE_DIR/chord.sh $TGT/chord +cp $RECIPE_DIR/chord.sh $TGT/chord || { echo "Error copying chord.sh" >&2; exit 1; } -ln -s $TGT/chord ${PREFIX}/bin/ +ln -s $TGT/chord ${PREFIX}/bin/ || { echo "Error creating symbolic link" >&2; exit 1; }These changes ensure that the required files exist and that any operation failures are caught and reported.
To verify the existence of the
chord.shfile in the recipe directory, you can run:#!/bin/bash # Description: Check for the presence of chord.sh in the recipe directory # Expected result: The chord.sh file should exist in the recipe directory # Test: Search for chord.sh in the recipe directory fd -t f '^chord.sh$' recipes/hmftools-chord
7-8:⚠️ Potential issueImprove error handling for directory changes and file operations.
- The
cdcommand should handle potential failures:-cd "${SRC_DIR}" +cd "${SRC_DIR}" || exit 1
- Consider adding a check for the existence of the
jar/directory:+if [ ! -d "jar" ]; then + echo "Error: jar/ directory not found" >&2 + exit 1 +fi mv jar/chord*.jar $TGT/chord.jarThese changes will make the script more robust and easier to debug if issues occur.
To verify the existence of the
jar/directory in the source package, you can run:🧰 Tools
🪛 Shellcheck
[warning] 7-7: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
recipes/hmftools-chord/meta.yaml (2)
1-3: LGTM: Version and checksums are properly defined.The version is correctly set to "2.1.0_beta", following semantic versioning for pre-release software. The inclusion of SHA256 checksums for both JAR and source files is a good practice for ensuring file integrity.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
5-7: LGTM: Package name and version are correctly defined.The package name "hmftools-chord" is appropriate, and the version is correctly referenced using Jinja2 templating, ensuring consistency with the earlier definition.
recipes/hmftools-chord/chord.sh (1)
69-69: LGTM: Script conclusionThe explicit
exitcommand at the end of the script is fine. While not strictly necessary, it clearly indicates the intended end of the script execution and can be helpful for readability.
| pass_arr=($pass_args) | ||
| if [[ ${pass_arr[0]:=} == com.hartwig.* ]] | ||
| then | ||
| eval "$java" $jvm_mem_opts $jvm_prop_opts -cp "$JAR_DIR/chord.jar" $pass_args | ||
| else | ||
| eval "$java" $jvm_mem_opts $jvm_prop_opts -jar "$JAR_DIR/chord.jar" $pass_args | ||
| fi |
There was a problem hiding this comment.
Improve command execution for better security and robustness
While the logic for determining how to execute the Java command is sound, there are some security and robustness concerns in this section.
-
The use of
evalcan be dangerous if the inputs are not properly sanitized. Consider removingevaland using arrays for argument handling. -
The word splitting warning on line 62 should be addressed.
Here's a suggested refactor to address these issues:
-pass_arr=($pass_args)
-if [[ ${pass_arr[0]:=} == com.hartwig.* ]]
+if [[ ${pass_args[0]:=} == com.hartwig.* ]]
then
- eval "$java" $jvm_mem_opts $jvm_prop_opts -cp "$JAR_DIR/chord.jar" $pass_args
+ "$java" $jvm_mem_opts $jvm_prop_opts -cp "$JAR_DIR/chord.jar" "${pass_args[@]}"
else
- eval "$java" $jvm_mem_opts $jvm_prop_opts -jar "$JAR_DIR/chord.jar" $pass_args
+ "$java" $jvm_mem_opts $jvm_prop_opts -jar "$JAR_DIR/chord.jar" "${pass_args[@]}"
fiThis refactored version:
- Removes the use of
eval, improving security. - Uses the array directly, avoiding word splitting issues.
- Properly expands the
pass_argsarray using"${pass_args[@]}".
These changes make the script more secure and robust against potential injection attacks or unexpected behavior with special characters in arguments.
📝 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.
| pass_arr=($pass_args) | |
| if [[ ${pass_arr[0]:=} == com.hartwig.* ]] | |
| then | |
| eval "$java" $jvm_mem_opts $jvm_prop_opts -cp "$JAR_DIR/chord.jar" $pass_args | |
| else | |
| eval "$java" $jvm_mem_opts $jvm_prop_opts -jar "$JAR_DIR/chord.jar" $pass_args | |
| fi | |
| if [[ ${pass_args[0]:=} == com.hartwig.* ]] | |
| then | |
| "$java" $jvm_mem_opts $jvm_prop_opts -cp "$JAR_DIR/chord.jar" "${pass_args[@]}" | |
| else | |
| "$java" $jvm_mem_opts $jvm_prop_opts -jar "$JAR_DIR/chord.jar" "${pass_args[@]}" | |
| fi |
🧰 Tools
🪛 Shellcheck
[warning] 62-62: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
Add recipe for hmftools-chord 2.1.0_beta
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>.