Add hmftools-redux 1.0_beta #51151
Add hmftools-redux 1.0_beta #51151
Conversation
📝 WalkthroughWalkthroughThis pull request introduces the Changes
Possibly related PRs
Suggested labels
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 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: 5
🧹 Outside diff range and nitpick comments (7)
recipes/hmftools-redux/build.sh (3)
1-5: LGTM! Consider adding error handling for directory creation.The initial setup looks good. The use of environment variables and directory checks are consistent with Bioconda practices.
For improved robustness, consider adding error handling for directory creation:
-[ -d "$TGT" ] || mkdir -p "$TGT" -[ -d "${PREFIX}/bin" ] || mkdir -p "${PREFIX}/bin" +[ -d "$TGT" ] || mkdir -p "$TGT" || { echo "Failed to create $TGT"; exit 1; } +[ -d "${PREFIX}/bin" ] || mkdir -p "${PREFIX}/bin" || { echo "Failed to create ${PREFIX}/bin"; exit 1; }
12-12: LGTM! Consider adding error handling for chmod.The chmod command is correct and necessary to ensure the script is executable.
For completeness, consider adding error handling:
-chmod 0755 "${PREFIX}/bin/redux" +chmod 0755 "${PREFIX}/bin/redux" || { echo "Failed to set permissions for ${PREFIX}/bin/redux"; exit 1; }
1-12: Overall assessment: Good structure, needs improved error handling.The build script for hmftools-redux is well-structured and follows Bioconda practices. However, it would benefit from improved error handling and robustness in several areas:
- Directory creation
- Changing directories
- File operations (moving, copying)
- Symbolic link creation
- Permission setting
Implementing the suggested changes will make the script more resilient to potential issues during the build process.
Consider creating helper functions for common operations (e.g., safe_mkdir, safe_copy) to improve code readability and maintainability.
🧰 Tools
🪛 Shellcheck
[warning] 7-7: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
recipes/hmftools-redux/meta.yaml (2)
12-16: LGTM with a suggestion: Consider adjusting therun_exportsdirective.The build configuration is correct, with
noarch: genericappropriate for a Java JAR file and the build number set to 0 for a new package. However, therun_exportsdirective might be too restrictive for a beta version.Consider modifying the
run_exportsdirective to allow for more flexibility in beta versions:run_exports: - {{ pin_subpackage('hmftools-redux', max_pin="x") }}This change allows for minor version updates without breaking dependencies, which is more suitable for beta releases.
18-25: LGTM with a suggestion: Consider improving the test command.The runtime requirements and test command are generally correct. However, the test command could be more specific to ensure the correct version is installed.
Consider modifying the test command to check for the exact version:
test: commands: - 'redux -version | grep -q "Redux version {{ version }}"'This change ensures that the installed version matches the expected version, providing a more robust test.
recipes/hmftools-redux/redux.sh (2)
1-7: Consider using a more specific locale setting.The script sets
LC_ALL=en_US.UTF-8, which might be too broad. Consider usingLANGorLC_CTYPEinstead, as they are more specific and less likely to interfere with other locale-dependent operations.You could replace line 6 with:
-export LC_ALL=en_US.UTF-8 +export LANG=en_US.UTF-8or
-export LC_ALL=en_US.UTF-8 +export LC_CTYPE=en_US.UTF-8
69-69: Remove unnecessary exit command.The explicit
exitat the end of the script is redundant as the script would naturally terminate at this point.You can safely remove this line:
-exit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- recipes/hmftools-redux/build.sh (1 hunks)
- recipes/hmftools-redux/meta.yaml (1 hunks)
- recipes/hmftools-redux/redux.sh (1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/hmftools-redux/build.sh
[warning] 7-7: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
recipes/hmftools-redux/redux.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-redux/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (7)
recipes/hmftools-redux/meta.yaml (5)
1-2: LGTM: Version and SHA256 hash are correctly defined.The version and SHA256 hash are properly set using Jinja2 variables. The version "1.0_beta" correctly indicates a pre-release version, and the SHA256 hash is provided for package integrity verification.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
4-6: LGTM: Package name and version are correctly defined.The package name "hmftools-redux" follows the convention of using lowercase and hyphens. The version is correctly referenced using the Jinja2 variable, ensuring consistency with the earlier definition.
8-10: LGTM: Source URL and SHA256 hash are correctly defined.The source URL correctly points to a GitHub release JAR file, using Jinja2 variables for easy version updates. The SHA256 hash is properly referenced using the Jinja2 variable, ensuring integrity verification.
27-31: LGTM: About section is correctly defined.The about section provides all necessary metadata, including the correct home URL, license information (GPL-3.0-only), and a concise summary of the tool's functionality.
1-2: Note: Ignore the yamllint syntax error for Jinja2 templates.The yamllint tool reported a syntax error for the '%' character at the beginning of the file. This is a false positive, as the '%' character is part of the Jinja2 template syntax, which is valid and commonly used in Conda recipes. The YAML is processed correctly by the Conda build system, which interprets the Jinja2 templates.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
recipes/hmftools-redux/redux.sh (2)
8-16: LGTM: Robust script directory resolution.The script correctly resolves its own directory, handling potential symlinks. This implementation is a well-established method for determining the script's location.
1-69: LGTM: Well-structured script with good functionality.The overall structure and functionality of the script are sound. It effectively handles various scenarios for Java execution and argument processing. The minor issues identified in previous comments do not significantly impact the script's core functionality.
🧰 Tools
🪛 Shellcheck
[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)
| JAR_DIR=$DIR | ||
| ENV_PREFIX="$(dirname $(dirname $DIR))" | ||
| # Use Java installed with Anaconda to ensure correct version | ||
| java="$ENV_PREFIX/bin/java" | ||
|
|
||
| # if JAVA_HOME is set (non-empty), use it. Otherwise keep "java" | ||
| if [ -n "${JAVA_HOME:=}" ]; then | ||
| if [ -e "$JAVA_HOME/bin/java" ]; then | ||
| java="$JAVA_HOME/bin/java" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Fix potential issue with ENV_PREFIX determination.
The current method of determining ENV_PREFIX assumes a specific directory structure, which may not always hold true. This could lead to incorrect Java executable selection.
Consider using a more robust method to determine the Anaconda environment prefix. Here's a suggested fix:
-ENV_PREFIX="$(dirname $(dirname $DIR))"
+ENV_PREFIX="${CONDA_PREFIX:-$(dirname $(dirname $DIR))}"This change uses the CONDA_PREFIX environment variable if it's set (which it should be in an active Conda environment), falling back to the current method if it's not.
📝 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.
| JAR_DIR=$DIR | |
| ENV_PREFIX="$(dirname $(dirname $DIR))" | |
| # Use Java installed with Anaconda to ensure correct version | |
| java="$ENV_PREFIX/bin/java" | |
| # if JAVA_HOME is set (non-empty), use it. Otherwise keep "java" | |
| if [ -n "${JAVA_HOME:=}" ]; then | |
| if [ -e "$JAVA_HOME/bin/java" ]; then | |
| java="$JAVA_HOME/bin/java" | |
| fi | |
| fi | |
| JAR_DIR=$DIR | |
| ENV_PREFIX="${CONDA_PREFIX:-$(dirname $(dirname $DIR))}" | |
| # Use Java installed with Anaconda to ensure correct version | |
| java="$ENV_PREFIX/bin/java" | |
| # if JAVA_HOME is set (non-empty), use it. Otherwise keep "java" | |
| if [ -n "${JAVA_HOME:=}" ]; then | |
| if [ -e "$JAVA_HOME/bin/java" ]; then | |
| java="$JAVA_HOME/bin/java" | |
| fi | |
| fi |
🧰 Tools
🪛 Shellcheck
[warning] 19-19: Quote this to prevent word splitting.
(SC2046)
| if [[ ${pass_arr[0]:=} == com.hartwig.* ]] | ||
| then | ||
| eval "$java" $jvm_mem_opts $jvm_prop_opts -cp "$JAR_DIR/redux.jar" $pass_args | ||
| else | ||
| eval "$java" $jvm_mem_opts $jvm_prop_opts -jar "$JAR_DIR/redux.jar" $pass_args | ||
| fi |
There was a problem hiding this comment.
Consider alternatives to eval for improved security.
The use of eval to execute the Java command could be potentially dangerous if pass_args contains uncontrolled user input.
Consider using array expansion instead of eval. Here's a suggested alternative:
if [[ ${pass_arr[0]:=} == com.hartwig.* ]]
then
- eval "$java" $jvm_mem_opts $jvm_prop_opts -cp "$JAR_DIR/redux.jar" $pass_args
+ "$java" $jvm_mem_opts $jvm_prop_opts -cp "$JAR_DIR/redux.jar" "${pass_arr[@]}"
else
- eval "$java" $jvm_mem_opts $jvm_prop_opts -jar "$JAR_DIR/redux.jar" $pass_args
+ "$java" $jvm_mem_opts $jvm_prop_opts -jar "$JAR_DIR/redux.jar" "${pass_arr[@]}"
fiThis change uses array expansion to pass arguments, which is safer than using eval.
📝 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 [[ ${pass_arr[0]:=} == com.hartwig.* ]] | |
| then | |
| eval "$java" $jvm_mem_opts $jvm_prop_opts -cp "$JAR_DIR/redux.jar" $pass_args | |
| else | |
| eval "$java" $jvm_mem_opts $jvm_prop_opts -jar "$JAR_DIR/redux.jar" $pass_args | |
| fi | |
| if [[ ${pass_arr[0]:=} == com.hartwig.* ]] | |
| then | |
| "$java" $jvm_mem_opts $jvm_prop_opts -cp "$JAR_DIR/redux.jar" "${pass_arr[@]}" | |
| else | |
| "$java" $jvm_mem_opts $jvm_prop_opts -jar "$JAR_DIR/redux.jar" "${pass_arr[@]}" | |
| fi |
| # extract memory and system property Java arguments from the list of provided arguments | ||
| # http://java.dzone.com/articles/better-java-shell-script | ||
| default_jvm_mem_opts="-Xms512m -Xmx1g" | ||
| jvm_mem_opts="" | ||
| jvm_prop_opts="" | ||
| 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 | ||
| ;; | ||
| esac | ||
| done | ||
|
|
||
| if [ "$jvm_mem_opts" == "" ]; then | ||
| jvm_mem_opts="$default_jvm_mem_opts" | ||
| fi | ||
|
|
||
| pass_arr=($pass_args) |
There was a problem hiding this comment.
Fix potential word splitting issue in argument processing.
The current method of splitting pass_args into an array on line 62 could lead to unexpected behavior if arguments contain spaces.
Consider using a more robust method to split the arguments. Here's a suggested fix:
-pass_arr=($pass_args)
+IFS=' ' read -r -a pass_arr <<< "$pass_args"This change uses read with IFS set to space to properly split the arguments into an array, preserving spaces within individual 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.
| # extract memory and system property Java arguments from the list of provided arguments | |
| # http://java.dzone.com/articles/better-java-shell-script | |
| default_jvm_mem_opts="-Xms512m -Xmx1g" | |
| jvm_mem_opts="" | |
| jvm_prop_opts="" | |
| 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 | |
| ;; | |
| esac | |
| done | |
| if [ "$jvm_mem_opts" == "" ]; then | |
| jvm_mem_opts="$default_jvm_mem_opts" | |
| fi | |
| pass_arr=($pass_args) | |
| # extract memory and system property Java arguments from the list of provided arguments | |
| # http://java.dzone.com/articles/better-java-shell-script | |
| default_jvm_mem_opts="-Xms512m -Xmx1g" | |
| jvm_mem_opts="" | |
| jvm_prop_opts="" | |
| 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 | |
| ;; | |
| esac | |
| done | |
| if [ "$jvm_mem_opts" == "" ]; then | |
| jvm_mem_opts="$default_jvm_mem_opts" | |
| fi | |
| IFS=' ' read -r -a pass_arr <<< "$pass_args" |
🧰 Tools
🪛 Shellcheck
[warning] 62-62: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
Add recipe for hmftools-redux 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>.Summary by CodeRabbit
New Features
hmftools-reduxpackage, enabling streamlined installation and execution of the Redux tool for post-processing read alignments.redux.sh) to facilitate running the Java application.build.sh) for easier setup and execution.Documentation
meta.yaml) detailing package version, dependencies, and metadata for thehmftools-reduxpackage.