Add EGA-Cryptor #51108
Add EGA-Cryptor #51108
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces four new files for 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: 6
🧹 Outside diff range and nitpick comments (9)
recipes/ega-cryptor/build.sh (2)
1-3: Good setup, consider consolidating error handling.The script starts with proper error handling and debugging options, which is excellent. However, you might consider consolidating the error handling options for brevity.
Consider combining lines 2 and 3 into a single line:
set -euxo pipefailThis achieves the same effect more concisely.
5-8: Directory setup looks good, consider version-specific symlinks.The directory structure is set up correctly using environment variables, which is a good practice for versioning. However, the symbolic link created on line 7 might cause issues if multiple versions of the package are installed.
Consider using a version-specific symlink to avoid potential conflicts:
ln -s $PREFIX/share/$PKG_NAME-$PKG_VERSION-$PKG_BUILDNUM $PREFIX/share/$PKG_NAME-$PKG_VERSIONThis allows multiple versions to coexist without overwriting each other's symlinks.
recipes/ega-cryptor/meta.yaml (4)
13-15: Consider versioning the source URL.The source section correctly provides both the URL and SHA256 checksum. However, the URL doesn't include the version number, which might cause issues with future updates.
Consider updating the URL to include the version number if possible. For example:
source: url: https://ega-archive.org/assets/files/EgaCryptor-{{ version }}.zipThis change would make it easier to update the package in the future and ensure that the correct version is always downloaded.
17-20: Specify a Python version range.The runtime requirements correctly include OpenJDK 8 or higher. However, the Python requirement doesn't specify a version range.
Consider specifying a Python version range to ensure compatibility. For example:
requirements: run: - openjdk >=8 - python >=3.6This change would help users understand which Python versions are supported by the package.
22-24: LGTM: Basic test is in place. Consider adding more comprehensive tests.The test section includes a basic command to check if the application runs and can display its help output. This is a good start.
If possible, consider adding more comprehensive tests to verify the functionality of the application. For example:
- Test file encryption
- Verify MD5 checksum generation
- Check if the output directory structure is created correctly
These additional tests would provide more confidence in the package's functionality.
16-16: Remove trailing spaces.There are trailing spaces on lines 16 and 33. While these don't affect functionality, it's good practice to remove them for consistency and cleanliness.
Please remove the trailing spaces from lines 16 and 33.
Also applies to: 33-33
🧰 Tools
🪛 yamllint
[error] 16-16: trailing spaces
(trailing-spaces)
recipes/ega-cryptor/ega-cryptor.py (3)
68-68: Correct typographical error in comment.The word "explictly" is misspelled. It should be "explicitly".
Apply this diff to correct the typo:
# it is important to explictly check for equality with None + # it is important to explicitly check for equality with None
83-83: Fix typographical error in docstring.There's a typo in the docstring: "If the exec_dir dies not exist," should be "If the exec_dir does not exist,".
Apply this diff to correct the typo:
If the exec_dir dies not exist, + If the exec_dir does not exist,
96-98: Check the return value of the subprocess call.Capturing and checking the return value of the subprocess call can provide more controlled exit status and allow for custom error handling if needed.
Consider modifying the code to handle the return value:
java_args = [java] + mem_opts + prop_opts + [jar_arg] + [jar_path] + pass_args - sys.exit(subprocess.call(java_args)) + return_code = subprocess.call(java_args) + if return_code != 0: + sys.stderr.write(f"Java process exited with return code {return_code}\n") + sys.exit(return_code)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- recipes/ega-cryptor/build.sh (1 hunks)
- recipes/ega-cryptor/ega-cryptor.py (1 hunks)
- recipes/ega-cryptor/meta.yaml (1 hunks)
🧰 Additional context used
🪛 Ruff
recipes/ega-cryptor/ega-cryptor.py
52-55: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
89-92: Use ternary operator
jar_arg = '-cp' if pass_args != [] and pass_args[0].startswith('eu') else '-jar'instead ofif-else-blockReplace
if-else-block withjar_arg = '-cp' if pass_args != [] and pass_args[0].startswith('eu') else '-jar'(SIM108)
🪛 yamllint
recipes/ega-cryptor/meta.yaml
[error] 16-16: trailing spaces
(trailing-spaces)
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
[error] 33-33: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
recipes/ega-cryptor/build.sh (2)
12-13: Final setup looks good.The creation of a symbolic link in the bin directory and setting the appropriate permissions for the executable are both correct and follow best practices.
1-14: Overall, the build script is functional but has room for improvement.The script successfully sets up the ega-cryptor package, following the general structure expected for a Bioconda build script. However, there are several areas where it could be optimized:
- Consolidating error handling options for brevity.
- Using version-specific symlinks to avoid potential conflicts with multiple installations.
- Being more selective in file copying to avoid including unnecessary files.
- Clarifying the purpose of the Python script (
ega-cryptor.py).Addressing these points will enhance the script's robustness and clarity. Despite these suggestions, the script is functional and achieves its primary goal of setting up the package correctly.
recipes/ega-cryptor/meta.yaml (1)
1-5: LGTM: Package information is well-defined.The package name and version are correctly specified. Using a variable for the version is a good practice for maintainability.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
recipes/ega-cryptor/ega-cryptor.py
Outdated
| elif arg.startswith('--exec_dir='): | ||
| exec_dir = arg.split('=')[1].strip('"').strip("'") | ||
| if not os.path.exists(exec_dir): | ||
| shutil.copytree(real_dirname(sys.argv[0]), exec_dir, symlinks=False, ignore=None) |
There was a problem hiding this comment.
Add error handling for shutil.copytree.
When copying files with shutil.copytree, it's good practice to handle potential exceptions that may occur, such as FileExistsError or OSError, to prevent the script from crashing unexpectedly.
Apply this diff to add exception handling:
if not os.path.exists(exec_dir):
- shutil.copytree(real_dirname(sys.argv[0]), exec_dir, symlinks=False, ignore=None)
+ try:
+ shutil.copytree(real_dirname(sys.argv[0]), exec_dir, symlinks=False, ignore=None)
+ except Exception as e:
+ sys.stderr.write(f"Error copying files to exec_dir: {e}\n")
+ sys.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.
| shutil.copytree(real_dirname(sys.argv[0]), exec_dir, symlinks=False, ignore=None) | |
| if not os.path.exists(exec_dir): | |
| try: | |
| shutil.copytree(real_dirname(sys.argv[0]), exec_dir, symlinks=False, ignore=None) | |
| except Exception as e: | |
| sys.stderr.write(f"Error copying files to exec_dir: {e}\n") | |
| sys.exit(1) |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
recipes/ega-cryptor/meta.yaml (2)
25-27: Consider adding more comprehensive tests.The current test command
ega-cryptor -his a good basic smoke test. However, to ensure the package functions correctly, consider adding more comprehensive tests, such as:
- Checking if all expected files are present in the installation directory.
- Verifying the version of the installed ega-cryptor.
- Testing basic functionality, like encrypting a small test file.
Example of additional test commands:
test: commands: - "ega-cryptor -h" - "ega-cryptor --version" - "test -f $PREFIX/bin/ega-cryptor" - "echo 'test' > test.txt && ega-cryptor -i test.txt -o test.encrypted.txt"These additional tests will provide more confidence in the package's functionality.
29-38: Consider adding attribution for the description.The about section is now complete with the correct license (Apache-2.0) specified. However, the description appears to be copied directly from the project's website. While comprehensive, it's important to ensure you have permission to use this text or to provide proper attribution.
Consider either:
- Adding an attribution line if you have permission to use the text, e.g., "Description from the EGA-Cryptor website."
- Rewriting the description in your own words while maintaining the key information.
This will help avoid any potential copyright issues and align with best practices for package metadata.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/ega-cryptor/ega-cryptor.py (1 hunks)
- recipes/ega-cryptor/meta.yaml (1 hunks)
🧰 Additional context used
🪛 Ruff
recipes/ega-cryptor/ega-cryptor.py
100-100: SyntaxError: unindent does not match any outer indentation level
101-101: SyntaxError: Unexpected indentation
🪛 yamllint
recipes/ega-cryptor/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (12)
recipes/ega-cryptor/meta.yaml (4)
1-5: LGTM: Package and version are correctly defined.The package name and version are properly set using Jinja2 templating, which is a good practice for maintainability.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
14-16: LGTM: Source URL and checksum are properly defined.The source URL and SHA256 checksum are correctly specified, which is good practice for security and integrity verification.
18-23: LGTM: Requirements are appropriately specified.The build and run requirements are correctly defined:
unzipis needed for the build process to extract the source.openjdk >=8andpython >=3.6are appropriate runtime dependencies for this Java-based application.The use of version constraints for runtime dependencies is a good practice.
7-12:⚠️ Potential issueRevise the
run_exportssection.The build section is mostly correct, but there's still an issue with the
run_exportssection:
- The
pin_subpackagefunction is typically used in therequirementssection, not inrun_exports.- The syntax for version pinning in
run_exportsis incorrect.Consider replacing the
run_exportssection with:run_exports: - {{ pin_compatible('ega-cryptor', max_pin='x.x.x') }}This will ensure that the package is compatible with other packages built against this version of ega-cryptor.
recipes/ega-cryptor/ega-cryptor.py (8)
9-18: LGTM: Imports and global variables are well-defined.The import statements are appropriate for the script's functionality, and the global variables are correctly defined. The default JVM memory options seem reasonable for general use.
23-25: LGTM:real_dirnamefunction is well-implemented.The
real_dirnamefunction correctly resolves symlinks and returns the canonical directory path. The docstring accurately describes the function's purpose.
28-36: LGTM:java_executablefunction is robust and well-implemented.The
java_executablefunction correctly handles both cases whenJAVA_HOMEis set and when it's not. The logic for checking the executable's accessibility is sound, providing a reliable way to determine the Java interpreter to use.
39-74: LGTM:jvm_optsfunction is well-implemented overall.The
jvm_optsfunction correctly processes command-line arguments to construct lists of memory options, property options, and passthrough arguments. The logic for handling default JVM memory options when not explicitly set is sound.
77-101: LGTM:mainfunction is well-implemented overall.The
mainfunction correctly orchestrates the execution of the Java application. It properly uses the helper functions to construct the Java command and executes it usingsubprocess.call. The error handling for non-zero return codes is a good practice.🧰 Tools
🪛 Ruff
100-100: SyntaxError: unindent does not match any outer indentation level
101-101: SyntaxError: Unexpected indentation
103-104: LGTM: Script execution block is correct.The script execution block correctly calls the
mainfunction when the script is run directly, following the standard Python idiom.
1-104: Overall, the script is well-implemented and fulfills its purpose.This Python wrapper script for executing Java applications in Conda environments is well-structured and follows good practices. It correctly handles JVM options, Java executable discovery, and command construction. The separation of concerns into distinct functions makes the code readable and maintainable.
A few minor improvements have been suggested in the review comments, including:
- Combining consecutive
ifstatements in thejvm_optsfunction.- Adding error handling for
shutil.copytree.- Simplifying an
if-elseblock using a ternary operator in themainfunction.- Fixing indentation issues in the
mainfunction.Implementing these suggestions will further enhance the script's quality and robustness.
🧰 Tools
🪛 Ruff
100-100: SyntaxError: unindent does not match any outer indentation level
101-101: SyntaxError: Unexpected indentation
61-61:⚠️ Potential issueAdd error handling for
shutil.copytree.As suggested in the previous review, it's good practice to handle potential exceptions that may occur when using
shutil.copytree. This will prevent the script from crashing unexpectedly.Apply this diff to add exception handling:
if not os.path.exists(exec_dir): - shutil.copytree(real_dirname(sys.argv[0]), exec_dir, symlinks=False, ignore=None) + try: + shutil.copytree(real_dirname(sys.argv[0]), exec_dir, symlinks=False, ignore=None) + except Exception as e: + sys.stderr.write(f"Error copying files to exec_dir: {e}\n") + sys.exit(1)Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
recipes/ega-cryptor/meta.yaml (2)
24-26: LGTM: Basic test command is defined.The test command
ega-cryptor -hprovides a basic smoke test to ensure the application can be executed and display its help information.Consider adding more comprehensive tests if possible, such as:
- Checking for specific expected output from the help command.
- Testing basic encryption/decryption functionality with sample data.
28-38: Description source and formatting.The about section is well-defined with the correct home URL, summary, and license (Apache-2.0). The description is comprehensive, which is good.
- Consider rephrasing the description to avoid direct copying from the website, or ensure you have permission to use it.
- The last line about the description source could be moved to a comment:
description: | The EGACryptor v.2.0.0 is a JAVA-based application which enables submitters to produce EGA compliant encrypted files along with files for the encrypted and unencrypted md5sum for each file to be submitted. The application will generate an output folder that will by default mirror the directory structure containing the original files. This output folder can subsequently be uploaded to the EGA FTP staging area via an FTP or Aspera client. # Description adapted from the EGA-Cryptor website.This keeps the description clean while still attributing the source.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/ega-cryptor/ega-cryptor.py (1 hunks)
- recipes/ega-cryptor/meta.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- recipes/ega-cryptor/ega-cryptor.py
🧰 Additional context used
🪛 yamllint
recipes/ega-cryptor/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (4)
recipes/ega-cryptor/meta.yaml (4)
1-5: LGTM: Package and version are correctly defined.The package name and version are properly set using Jinja2 templating, which is a good practice for maintainability.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
13-15: LGTM: Source URL and checksum are correctly defined.The source URL and SHA256 checksum are properly specified, which ensures the integrity of the downloaded file.
17-22: LGTM: Requirements are appropriately defined.The build and run requirements are correctly specified:
unzipis needed to extract the source zip file.openjdk >=8andpython >=3.6are appropriate for a Java-based application with Python support.The version constraints are reasonable and allow for compatibility with newer versions.
1-1: Note on yamllint warning.The yamllint tool reports 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, which is valid and commonly used in Bioconda recipes.
No action is needed to address this warning.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
recipes/ega-cryptor/meta.yaml (3)
13-15: LGTM with suggestion: Consider using a versioned URL.The source section is well-defined with a correct URL and SHA256 checksum. However, for better reproducibility and version tracking, consider using a versioned URL if available.
For example:
url: https://ega-archive.org/assets/files/EgaCryptor-{{ version }}.zipThis change would make it easier to update the package in the future and ensure that the correct version is always downloaded.
24-26: LGTM with suggestions: Consider adding more comprehensive tests.The current test command is a good basic smoke test to ensure the package is installed correctly. However, to improve the robustness of the package, consider adding more comprehensive tests. For example:
- Check if specific expected output is present in the help message.
- Test basic functionality by encrypting a small test file.
- Verify that MD5 checksums are generated correctly.
Example of a more comprehensive test section:
test: commands: - ega-cryptor -h | grep "EGA-Cryptor" - echo "test" > test.txt - ega-cryptor -i test.txt -o encrypted_test.txt - test -f encrypted_test.txt - test -f encrypted_test.txt.md5 - test -f test.txt.md5These additional tests would provide more confidence in the package's functionality.
28-37: LGTM: About section is comprehensive and improved.Great job on improving the about section:
- The home URL is correct.
- The summary is concise and informative.
- The license has been updated to Apache-2.0, addressing the previous "unknown" license issue.
- The description is comprehensive and accurately describes the package's functionality.
It's good that you've indicated the source of the description with the comment. To further improve, consider adding a reference to the Apache-2.0 license text or a link to it.
recipes/ega-cryptor/ega-cryptor.py (3)
28-29: Add a docstring to the java_executable function.For consistency with other functions and to improve code documentation, consider adding a detailed docstring to the
java_executablefunction.Example implementation:
def java_executable(): """ Determine the path to the Java executable. This function first checks if JAVA_HOME is set and contains a valid Java executable. If not, it falls back to using 'java' and relies on the system PATH. Returns: str: The path to the Java executable. """
63-70: Clarify comment about shell script behavior.The comment explaining the reproduction of shell script behavior could be clearer. Consider rephrasing it to make the logic more explicit.
Suggested rephrasing:
# Reproduce the behavior of the original shell script: # If no JVM memory options were set on the command line, # and the _JAVA_OPTIONS environment variable is not set, # use the default memory options. if not mem_opts and os.environ.get('_JAVA_OPTIONS') is None: mem_opts = default_jvm_mem_optsThis makes the logic more explicit and easier to understand.
77-83: Remove or update outdated comment.The comment mentioning PeptideShaker seems out of place and might be outdated. If it's not relevant to EGA-Cryptor, consider removing it or updating it to reflect the current application.
If the comment is still relevant, consider updating it to:
""" EGA-Cryptor may update files relative to the path of the jar file. In a multiuser setting, the option --exec_dir="exec_dir" can be used as the location for the EGA-Cryptor distribution. If the exec_dir does not exist, we copy the jar file, lib, and resources to the exec_dir directory. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- recipes/ega-cryptor/build.sh (1 hunks)
- recipes/ega-cryptor/ega-cryptor.py (1 hunks)
- recipes/ega-cryptor/meta.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- recipes/ega-cryptor/build.sh
🧰 Additional context used
🪛 yamllint
recipes/ega-cryptor/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (7)
recipes/ega-cryptor/meta.yaml (4)
1-5: LGTM: Package and version section is well-defined.The use of Jinja2 templating for the version is a good practice, and the package name and version are correctly defined.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
7-11: LGTM: Build section is well-defined and improved.The build section is correctly configured:
noarch: genericis appropriate for a Java-based application.- The build number is correctly set to 0 for a new package.
- The
run_exportssection has been improved as per previous suggestions, usingpin_compatiblewhich is the correct approach.Great job on implementing the suggested changes!
17-22: LGTM: Requirements are well-defined.The requirements section is correctly configured:
- The build requirement
unzipis appropriate for extracting the zip file.- The run requirements
openjdk >=8andpython >=3.6are suitable for a Java-based application with Python support.- The version constraints are reasonable and allow for flexibility.
1-1: Note: Ignore yamllint syntax error for Jinja2 templating.The yamllint tool reports a syntax error for the '%' character at the beginning of the file. This is a false positive. The '%' character is part of the Jinja2 templating syntax, which is correctly used in conda-forge and bioconda recipes. No action is required to address this warning.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
recipes/ega-cryptor/ega-cryptor.py (3)
23-26: LGTM: Well-implemented directory resolution function.The
real_dirnamefunction correctly resolves symlinks and returns the canonical directory path. The implementation is concise and uses appropriateos.pathfunctions.
98-99: LGTM: Correct main execution block.The main execution block is correctly implemented, following the standard Python idiom for executing the main function when the script is run directly.
1-99: Overall: Well-structured wrapper script with minor improvement opportunities.This Python script serves as a wrapper for executing the EGA-Cryptor Java application within a Conda environment. It effectively handles JVM options, determines the correct Java executable, and executes the Java command. The overall structure and functionality of the script are sound and it appears to fulfill its purpose well.
There are a few areas where the script could be improved:
- Making the jar file name configurable
- Adding missing docstrings
- Enhancing robustness of --exec_dir handling
- Clarifying comments and removing outdated information
- Improving error handling
These improvements would enhance the script's flexibility, maintainability, and robustness. Overall, the script is well-implemented and, with these minor enhancements, would be even more effective in its role.
recipes/ega-cryptor/ega-cryptor.py
Outdated
| def jvm_opts(argv): | ||
| """Construct list of Java arguments based on our argument list. | ||
|
|
||
| The argument list passed in argv must not include the script name. | ||
| The return value is a 3-tuple lists of strings of the form: | ||
| (memory_options, prop_options, passthrough_options) | ||
| """ | ||
| mem_opts = [] | ||
| prop_opts = [] | ||
| pass_args = [] | ||
| exec_dir = None | ||
|
|
||
| for arg in argv: | ||
| if arg.startswith('-D') or arg.startswith('-XX'): | ||
| prop_opts.append(arg) | ||
| elif arg.startswith('-Xm'): | ||
| mem_opts.append(arg) | ||
| elif arg.startswith('--exec_dir='): | ||
| exec_dir = arg.split('=')[1].strip('"').strip("'") | ||
| if not os.path.exists(exec_dir): | ||
| shutil.copytree(real_dirname(sys.argv[0]), exec_dir, symlinks=False, ignore=None) | ||
| else: | ||
| pass_args.append(arg) | ||
|
|
||
| # In the original shell script the test coded below read: | ||
| # if [ "$jvm_mem_opts" == "" ] && [ -z ${_JAVA_OPTIONS+x} ] | ||
| # To reproduce the behaviour of the above shell code fragment | ||
| # it is important to explicitly check for equality with None | ||
| # in the second condition, so a null envar value counts as True! | ||
|
|
||
| if mem_opts == [] and getenv('_JAVA_OPTIONS') is None: | ||
| mem_opts = default_jvm_mem_opts | ||
|
|
||
| return (mem_opts, prop_opts, pass_args, exec_dir) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring jvm_opts function.
The jvm_opts function is quite long and handles multiple responsibilities. Consider splitting it into smaller, more focused functions to improve readability and maintainability.
You could split the function into:
parse_jvm_args: Handle parsing of JVM-related arguments.handle_exec_dir: Process the --exec_dir argument.apply_default_memory_opts: Apply default memory options if necessary.
This would make the main jvm_opts function shorter and easier to understand.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
recipes/ega-cryptor/meta.yaml (4)
13-15: Consider versioning the download URL.The source URL and SHA256 checksum are correctly provided. However, to improve traceability and ensure consistency across versions, consider including the version number in the URL if possible.
If the EGA-Cryptor website supports versioned downloads, you could update the URL like this:
url: https://ega-archive.org/assets/files/EgaCryptor-{{ version }}.zipThis change would make it easier to update the recipe for future versions.
24-26: Consider enhancing the test section.The current test command is a good basic smoke test. However, to ensure better coverage, consider adding more comprehensive tests.
Here are some suggestions to enhance the test section:
test: commands: - "ega-cryptor -h" - "ega-cryptor --version" - "python -c 'import egacryptor; print(egacryptor.__version__)'" # If there's a Python moduleThese additional tests would verify the version and check if the Python module (if applicable) can be imported correctly.
28-38: LGTM: About section with minor suggestion.The about section is comprehensive and follows Conda packaging best practices:
- Home URL, summary, license, and description are all provided.
- The license is correctly specified as Apache-2.0, addressing a previous concern.
Consider adding an attribution for the description, as it appears to be from the project's website. You could add a comment like this:
description: | The EGACryptor v.2.0.0 is a JAVA-based application which enables submitters to produce EGA compliant encrypted files along with files for the encrypted and unencrypted md5sum for each file to be submitted. The application will generate an output folder that will by default mirror the directory structure containing the original files. This output folder can subsequently be uploaded to the EGA FTP staging area via an FTP or Aspera client. # Description sourced from the EGA-Cryptor website (https://ega-archive.org/submission/data/file-preparation/egacryptor/)This addition provides proper attribution for the description text.
39-39: LGTM: Description attribution.The comment correctly attributes the source of the description, which is good practice.
For completeness, consider including the full URL in the comment:
# Description extracted from the EGA-Cryptor website: https://ega-archive.org/submission/data/file-preparation/egacryptor/This provides a direct link to the source for future reference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- recipes/ega-cryptor/LICENSE (1 hunks)
- recipes/ega-cryptor/ega-cryptor.py (1 hunks)
- recipes/ega-cryptor/meta.yaml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- recipes/ega-cryptor/LICENSE
🧰 Additional context used
🪛 yamllint
recipes/ega-cryptor/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (4)
recipes/ega-cryptor/meta.yaml (3)
1-5: LGTM: Package and version definition.The package name and version are correctly defined using Jinja2 templating. This follows best practices for Conda recipes.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
7-11: LGTM: Build section and run_exports.The build section is correctly configured:
noarch: genericis appropriate for a Java application.- Build number is correctly set to 0 for a new package.
- The
run_exportssection has been corrected as per the previous review suggestion, usingpin_compatibleinstead ofpin_subpackage.
17-22: LGTM: Requirements section.The build and run requirements are appropriate for a Java application with a Python wrapper:
- Build requirement
unzipis necessary to extract the source.- Run requirements
openjdk >=8andpython >=3.6are suitable for the application.The version constraints are reasonable and allow for compatibility with a wide range of environments.
recipes/ega-cryptor/ega-cryptor.py (1)
1-111: Code changes look good overallThe script is well-structured and effectively manages Java execution parameters and directory handling for the EGA-Cryptor application. Previous issues raised in past reviews have been addressed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
recipes/ega-cryptor/build.sh (2)
1-4: Consider enabling stricter error handling.The script currently uses
set -e -xfor error handling and verbosity. There's a commented-out line with stricter options:set -eu -o pipefail. Consider enabling these stricter options to catch more potential issues during the build process.You could uncomment and use the following line instead:
set -eu -o pipefail -xThis will exit on errors, treat unset variables as errors, prevent errors in pipelines from being masked, and print each command as it's executed.
1-13: Overall, the build script is well-structured but could use minor improvements.The script follows good practices for package installation in Conda environments. It correctly sets up directories, handles the JAR file, and creates necessary symlinks. The addition of the Python wrapper script addresses previous concerns about its purpose.
However, there are a couple of areas for improvement:
- Consider enabling stricter error handling options.
- Implement more selective file copying to avoid including unnecessary files in the output directory.
These changes will enhance the robustness and efficiency of the build process.
recipes/ega-cryptor/meta.yaml (2)
28-38: License updated, consider improving description attribution.Great job on updating the license to Apache-2.0, addressing the previous concern about an unknown license.
The description is comprehensive and informative. However, to improve attribution:
- Consider moving the source comment (line 39) to the start of the description.
- Use YAML's block scalar notation for better readability.
Here's a suggested revision for the description:
description: | # Description sourced from the EGA-Cryptor website (https://ega-archive.org/submission/data/file-preparation/egacryptor/) The EGACryptor v.2.0.0 is a JAVA-based application which enables submitters to produce EGA compliant encrypted files along with files for the encrypted and unencrypted md5sum for each file to be submitted. The application will generate an output folder that will by default mirror the directory structure containing the original files. This output folder can subsequently be uploaded to the EGA FTP staging area via an FTP or Aspera client.This change improves the attribution and readability of the description.
39-39: Add a newline character at the end of the file.To comply with YAML best practices and address the yamllint warning, please add a newline character at the end of the file.
🧰 Tools
🪛 yamllint
[error] 39-39: no new line character at the end of file
(new-line-at-end-of-file)
recipes/ega-cryptor/ega-cryptor.py (4)
1-21: LGTM with a minor suggestion for improvement.The import statements and initial variable declarations look good. Using an environment variable for the jar file name provides flexibility. However, to improve clarity, consider adding a brief comment explaining the purpose of the
EGA_CRYPTOR_JARenvironment variable.Add a comment above the jar_file declaration:
# Get the jar file name from the environment variable or use the default jar_file = os.environ.get('EGA_CRYPTOR_JAR', 'ega-cryptor-2.0.0.jar')
29-37: LGTM with a suggestion for improved robustness.The
java_executablefunction correctly determines the Java interpreter path, considering both JAVA_HOME and fallback scenarios. However, to improve robustness, consider adding a check to ensure the 'java' command is available in the system PATH when JAVA_HOME is not set.Add a check for 'java' in the system PATH:
def java_executable(): """Return the executable name of the Java interpreter.""" java_home = getenv('JAVA_HOME') java_bin = os.path.join('bin', 'java') if java_home and access(os.path.join(java_home, java_bin), X_OK): return os.path.join(java_home, java_bin) else: # Check if 'java' is in the system PATH if shutil.which('java'): return 'java' else: raise RuntimeError("Java executable not found. Please install Java or set JAVA_HOME.")This change ensures that the script fails gracefully if Java is not installed or not in the system PATH.
83-108: LGTM with a suggestion for improved clarity.The
mainfunction effectively orchestrates the execution of the Java process, incorporating previous suggestions such as enhanced error handling and the use of a ternary operator forjar_arg. To further improve clarity, consider adding a brief comment explaining the purpose of thejar_argternary operation.Add a comment above the jar_arg assignment:
# Determine whether to use -cp or -jar based on the first argument jar_arg = '-cp' if pass_args and pass_args[0].startswith('eu') else '-jar'This comment will help future maintainers understand the reasoning behind this decision.
1-111: Overall, well-implemented wrapper script with minor suggestions for improvement.This script effectively serves its purpose as a wrapper for Java Conda packages, specifically for EGA-Cryptor. It's well-structured with clear separation of concerns and incorporates most suggestions from previous reviews. Here are some final suggestions for improvement:
- Consider adding a module-level docstring explaining the purpose and usage of this script.
- Add type hints to function parameters and return values for improved code readability and maintainability.
- Consider using argparse for more robust command-line argument handling, especially for the --exec_dir option.
Here's an example of how you could implement these suggestions:
#!/usr/bin/env python """ Wrapper script for Java Conda packages that ensures that the java runtime is invoked with the right options. Specifically designed for EGA-Cryptor. Usage: python ega-cryptor.py [options] <java_args> """ import argparse import os import subprocess import sys import shutil from typing import List, Tuple # ... (rest of the imports and global variables) def jvm_opts(argv: List[str]) -> Tuple[List[str], List[str], List[str], str]: """ Construct list of Java arguments based on our argument list. Args: argv: The argument list (excluding the script name). Returns: A 4-tuple of (memory_options, prop_options, passthrough_options, exec_dir). """ # ... (rest of the function implementation) def main(): parser = argparse.ArgumentParser(description="EGA-Cryptor Java wrapper") parser.add_argument('--exec_dir', help="Location for the EGA-Cryptor distribution") parser.add_argument('java_args', nargs=argparse.REMAINDER, help="Arguments to pass to Java") args = parser.parse_args() # ... (rest of the main function implementation, using args.exec_dir and args.java_args) if __name__ == '__main__': main()These changes would further improve the script's clarity, maintainability, and robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- recipes/ega-cryptor/build.sh (1 hunks)
- recipes/ega-cryptor/ega-cryptor.py (1 hunks)
- recipes/ega-cryptor/meta.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/ega-cryptor/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
[error] 39-39: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (11)
recipes/ega-cryptor/build.sh (4)
5-8: Directory setup looks good.The script correctly sets up the output directory using appropriate environment variables and creates necessary symlinks and directories. This follows good practices for package installation in Conda environments.
11-12: Python wrapper script handling looks good.The script correctly copies the Python wrapper (
ega-cryptor.py) and makes it executable. This addresses the concern raised in the past review about the purpose of the Python script, confirming its role as a wrapper for the Java application.
13-13: Symlink creation is correct.Creating a symlink to the executable in the
$PREFIX/bindirectory is the correct approach. This ensures that theega-cryptorcommand will be available in the user's PATH after installation.
9-10:⚠️ Potential issueBe more selective in file copying.
The current approach of copying all files (
cp -Rf * $outdir/) might include unnecessary files likebuild.shandmeta.yaml. Consider copying only the essential files to optimize the build process and avoid cluttering the output directory.Replace these lines with more selective copying:
cp -f EGA-Cryptor-$PKG_VERSION/ega-cryptor-$PKG_VERSION.jar $outdir/ega-cryptor.jar # Add other necessary files explicitlyThis ensures that only the required files are included in the output.
recipes/ega-cryptor/meta.yaml (5)
1-5: LGTM: Package and version are correctly defined.The package name and version are properly set using Jinja2 templating, which is a good practice for maintainability.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
7-11: LGTM: Build section is correctly configured.The build section is well-defined:
noarch: genericis appropriate for a Java-based application.- The
run_exportssection has been correctly updated to usepin_compatible, addressing the previous review feedback.
13-15: LGTM: Source information is correctly specified.The source section includes:
- A valid URL for the EgaCryptor.zip file.
- The SHA256 checksum for integrity verification.
This is a good practice for ensuring the correct source is downloaded.
17-22: LGTM: Requirements are appropriately defined.The requirements section is well-structured:
- Build requirement
unzipis necessary for extracting the source.- Run requirements
openjdk >=8andpython >=3.6are correctly specified with version constraints.These requirements align well with a Java-based application using a Python wrapper.
24-26: LGTM: Test command is appropriate.The test section includes a basic smoke test:
- The command
ega-cryptor -hverifies that the application is installed and can display help information.- This is a good initial test to ensure the package is accessible after installation.
recipes/ega-cryptor/ega-cryptor.py (2)
24-27: LGTM! Well-implemented utility function.The
real_dirnamefunction is concise, well-documented, and correctly implements its purpose of returning the symlink-resolved, canonicalized directory portion of a path.
110-111: LGTM! Correct implementation of the main execution block.The main execution block is correctly implemented, following the standard Python idiom for scripts that can be both imported and run directly.
| def jvm_opts(argv): | ||
| """Construct list of Java arguments based on our argument list. | ||
|
|
||
| The argument list passed in argv must not include the script name. | ||
| The return value is a 3-tuple lists of strings of the form: | ||
| (memory_options, prop_options, passthrough_options, exec_dir) | ||
| """ | ||
| mem_opts = [] | ||
| prop_opts = [] | ||
| pass_args = [] | ||
| exec_dir = None | ||
|
|
||
| for arg in argv: | ||
| if arg.startswith('-D') or arg.startswith('-XX'): | ||
| prop_opts.append(arg) | ||
| elif arg.startswith('-Xm'): | ||
| mem_opts.append(arg) | ||
| elif arg.startswith('--exec_dir='): | ||
| exec_dir = arg.split('=')[1].strip('"').strip("'") | ||
| if not os.path.isabs(exec_dir): | ||
| sys.stderr.write("Error: --exec_dir must be an absolute path\n") | ||
| sys.exit(1) | ||
| if not os.path.exists(exec_dir): | ||
| try: | ||
| shutil.copytree(real_dirname(sys.argv[0]), exec_dir, symlinks=False, ignore=None) | ||
| except OSError as e: | ||
| sys.stderr.write(f"Error creating exec_dir: {e}\n") | ||
| sys.exit(1) | ||
| else: | ||
| pass_args.append(arg) | ||
|
|
||
| # In the original shell script the test coded below read: | ||
| # if [ "$jvm_mem_opts" == "" ] && [ -z ${_JAVA_OPTIONS+x} ] | ||
| # To reproduce the behaviour of the above shell code fragment | ||
| # it is important to explicitly check for equality with None | ||
| # in the second condition, so a null envar value counts as True! | ||
|
|
||
| if mem_opts == [] and getenv('_JAVA_OPTIONS') is None: | ||
| mem_opts = default_jvm_mem_opts | ||
|
|
||
| return (mem_opts, prop_opts, pass_args, exec_dir) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring jvm_opts function for improved maintainability.
The jvm_opts function is quite long and handles multiple responsibilities. Consider splitting it into smaller, more focused functions to improve readability and maintainability.
You could split the function into:
parse_jvm_args: Handle parsing of JVM-related arguments.handle_exec_dir: Process the --exec_dir argument.apply_default_memory_opts: Apply default memory options if necessary.
This would make the main jvm_opts function shorter and easier to understand. Here's an example of how you could refactor:
def parse_jvm_args(argv):
mem_opts = []
prop_opts = []
pass_args = []
for arg in argv:
if arg.startswith('-D') or arg.startswith('-XX'):
prop_opts.append(arg)
elif arg.startswith('-Xm'):
mem_opts.append(arg)
else:
pass_args.append(arg)
return mem_opts, prop_opts, pass_args
def handle_exec_dir(arg):
exec_dir = arg.split('=')[1].strip('"').strip("'")
if not os.path.isabs(exec_dir):
sys.stderr.write("Error: --exec_dir must be an absolute path\n")
sys.exit(1)
if not os.path.exists(exec_dir):
try:
shutil.copytree(real_dirname(sys.argv[0]), exec_dir, symlinks=False, ignore=None)
except OSError as e:
sys.stderr.write(f"Error creating exec_dir: {e}\n")
sys.exit(1)
return exec_dir
def apply_default_memory_opts(mem_opts):
if mem_opts == [] and getenv('_JAVA_OPTIONS') is None:
return default_jvm_mem_opts
return mem_opts
def jvm_opts(argv):
mem_opts, prop_opts, pass_args = parse_jvm_args(argv)
exec_dir = next((handle_exec_dir(arg) for arg in argv if arg.startswith('--exec_dir=')), None)
mem_opts = apply_default_memory_opts(mem_opts)
return (mem_opts, prop_opts, pass_args, exec_dir)This refactoring improves the overall structure and makes each part of the process more clear and maintainable.
| def jvm_opts(argv): | ||
| """Construct list of Java arguments based on our argument list. | ||
|
|
||
| The argument list passed in argv must not include the script name. | ||
| The return value is a 3-tuple lists of strings of the form: | ||
| (memory_options, prop_options, passthrough_options, exec_dir) | ||
| """ |
There was a problem hiding this comment.
Update the function docstring to reflect actual return values.
The docstring still mentions that the function returns a 3-tuple, but it actually returns a 4-tuple including exec_dir. Please update the docstring to accurately describe the returned values.
Apply this diff to correct the docstring:
The argument list passed in argv must not include the script name.
- The return value is a 3-tuple lists of strings of the form:
- (memory_options, prop_options, passthrough_options)
+ The return value is a 4-tuple of lists of strings of the form:
+ (memory_options, prop_options, passthrough_options, exec_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.
| def jvm_opts(argv): | |
| """Construct list of Java arguments based on our argument list. | |
| The argument list passed in argv must not include the script name. | |
| The return value is a 3-tuple lists of strings of the form: | |
| (memory_options, prop_options, passthrough_options, exec_dir) | |
| """ | |
| def jvm_opts(argv): | |
| """Construct list of Java arguments based on our argument list. | |
| The argument list passed in argv must not include the script name. | |
| The return value is a 4-tuple of lists of strings of the form: | |
| (memory_options, prop_options, passthrough_options, exec_dir) | |
| """ |
Add EGA-Cryptor (https://ega-archive.org/submission/data/file-preparation/egacryptor/) package to bioconda.
The EGACryptor v.2.0.0 is a JAVA-based application which enables submitters to produce EGA compliant encrypted files along with files for the encrypted and unencrypted md5sum for each file to be submitted. The application will generate an output folder that will by default mirror the directory structure containing the original files. This output folder can subsequently be uploaded to the EGA FTP staging area via an FTP or Aspera client.
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>.