Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add sipros #51091

Merged
merged 10 commits into from
Oct 9, 2024
Merged

add sipros #51091

merged 10 commits into from
Oct 9, 2024

Conversation

xyz1396
Copy link
Contributor

@xyz1396 xyz1396 commented Oct 1, 2024

add sipros for sip proteomics


Please read the guidelines for Bioconda recipes before opening a pull request (PR).

General instructions

  • If this PR adds or updates a recipe, use "Add" or "Update" appropriately as the first word in its title.
  • New recipes not directly relevant to the biological sciences need to be submitted to the conda-forge channel instead of Bioconda.
  • PRs require reviews prior to being merged. Once your PR is passing tests and ready to be merged, please issue the @BiocondaBot please add label command.
  • Please post questions on Gitter or ping @bioconda/core in 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_exports section like this:

build:
  run_exports:
    - ...

with ... being one of:

Case run_exports statement
semantic versioning {{ pin_subpackage("myrecipe", max_pin="x") }}
semantic versioning (0.x.x) {{ pin_subpackage("myrecipe", max_pin="x.x") }}
known breakage in minor versions {{ pin_subpackage("myrecipe", max_pin="x.x") }} (in such a case, please add a note that shortly mentions your evidence for that)
known breakage in patch versions {{ pin_subpackage("myrecipe", max_pin="x.x.x") }} (in such a case, please add a note that shortly mentions your evidence for that)
calendar versioning {{ pin_subpackage("myrecipe", max_pin=None) }}

while replacing "myrecipe" with either name if a name|lower variable 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 Merge the master branch into a PR.
@BiocondaBot please add label Add the please review & merge label.
@BiocondaBot please fetch artifacts Post links to CI-built packages/containers.
You can use this to test packages locally.

Note that the @BiocondaBot please merge command 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>.

Copy link
Contributor

coderabbitai bot commented Oct 1, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

This pull request introduces several new files to the recipes/sipros directory. A new LICENSE file has been added, containing the MIT License text. Additionally, a Raxport.sh Bash script is included to execute a .NET executable named Raxport.exe using the mono runtime. The build.sh script has been updated to automate the installation process, create necessary directories, and set permissions for binaries. Furthermore, a new copyConfigTemplate.sh script is introduced to manage configuration templates based on user input. Finally, a meta.yaml file is added to define the package configuration for "sipros" version "4.01", detailing dependencies, test commands, and other metadata.

Possibly related PRs

  • Add in new package mutamr #51139: The addition of a LICENSE file in the recipes/mutamr directory is similar to the new LICENSE file added in the recipes/sipros directory, both establishing the legal framework for their respective software packages.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between fdbf5e6 and aa33d44.

📒 Files selected for processing (1)
  • recipes/sipros/copyConfigTemplate.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • recipes/sipros/copyConfigTemplate.sh

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (9)
recipes/sipros/build.sh (5)

4-6: Consider a more selective approach to setting executable permissions

While creating the bin directory and copying files is correct, making all files in the bin directory executable (line 5) might be too broad. Some files may not require execution permissions.

Consider modifying line 5 to only set executable permissions for specific files that need it. For example:

-chmod u+x bin/*
+chmod u+x bin/file1 bin/file2 # List only the files that need to be executable

This approach is more secure and follows the principle of least privilege.


8-9: Add existence checks for directories before copying

While copying the EnsembleScripts and V4Scripts directories to $PREFIX is likely correct, it's good practice to verify that these directories exist before attempting to copy them.

Consider adding existence checks:

+[ -d "EnsembleScripts" ] && cp -r EnsembleScripts "$PREFIX" || echo "Warning: EnsembleScripts directory not found"
+[ -d "V4Scripts" ] && cp -r V4Scripts "$PREFIX" || echo "Warning: V4Scripts directory not found"
-cp -r EnsembleScripts "$PREFIX"
-cp -r V4Scripts "$PREFIX"

This change will make the script more robust and provide helpful warnings if the directories are missing.


11-11: Add existence check for configTemplates directory before copying

Similar to the previous comment, it's a good practice to verify that the configTemplates directory exists before attempting to copy it.

Consider modifying the line as follows:

-cp -r configTemplates "$PREFIX"
+[ -d "configTemplates" ] && cp -r configTemplates "$PREFIX" || echo "Warning: configTemplates directory not found"

This change will make the script more robust and provide a helpful warning if the directory is missing.


13-14: LGTM: Proper handling of Raxport script, consider adding a comment

The copying and permission setting for the Raxport script is correct. However, it would be helpful to add a comment explaining the purpose of this script, especially since it's coming from $RECIPE_DIR and not the original package.

Consider adding a comment before these lines:

+# Copy custom Raxport script and make it executable
cp $RECIPE_DIR/Raxport.sh "$PREFIX/bin/Raxport"
chmod +x "$PREFIX/bin/Raxport"

This will help future maintainers understand the purpose of this custom script.


1-14: Overall: Good build script with room for minor improvements

The build script correctly installs the sipros package components, including binaries, scripts, and configuration templates. It also handles a custom Raxport script appropriately.

To further improve the script:

  1. Consider adding error handling and logging. For example:

    log_error() {
      echo "ERROR: $1" >&2
    }
    
    # Use this function for critical operations
    cp -r bin/* "$PREFIX/bin" || log_error "Failed to copy binaries"
  2. Add a clean-up function to handle any temporary files or rollback partially completed operations in case of failure:

    cleanup() {
      # Add clean-up logic here
      echo "Cleaning up..."
    }
    
    trap cleanup EXIT
  3. Consider adding a simple check to ensure $PREFIX is set:

    [ -z "$PREFIX" ] && { echo "ERROR: PREFIX is not set"; exit 1; }

These additions will make the script more robust and easier to maintain.

recipes/sipros/Raxport.sh (2)

3-8: LGTM: Robust symlink resolution with a minor optimization opportunity.

The symlink resolution logic is well-implemented and handles both absolute and relative symlinks correctly. This ensures that the script can determine its true location regardless of how it's invoked.

Consider using readlink -f (if available on your target systems) to simplify the symlink resolution:

DIR="$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")"

This would replace the entire while loop, making the script more concise. However, note that readlink -f is not available on all systems (e.g., macOS), so the current implementation is more portable.


9-9: LGTM: Correct execution of .NET executable with room for error handling.

The script correctly executes the Raxport.exe using mono, leveraging the resolved directory path and properly forwarding all arguments.

Consider adding basic error handling to improve robustness:

if [ ! -f "$DIR/Raxport.exe" ]; then
    echo "Error: Raxport.exe not found in $DIR" >&2
    exit 1
fi

if ! command -v mono &> /dev/null; then
    echo "Error: mono runtime not found" >&2
    exit 1
fi

mono "$DIR/Raxport.exe" "$@"

This addition would check for the existence of Raxport.exe and the availability of the mono runtime before attempting to execute, providing more informative error messages if something is missing.

recipes/sipros/meta.yaml (2)

1-6: Consider using Semantic Versioning (SemVer) for the version number.

The current version "4.01" doesn't strictly follow the Semantic Versioning (SemVer) format. Consider changing it to "4.1.0" to align with SemVer practices, which can help with dependency management and version comparisons.

-{% set version = "4.01" %}
+{% set version = "4.1.0" %}
🧰 Tools
🪛 yamllint

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


12-18: Remove redundant skip parameter and trailing space.

The skip: False line is redundant as it's the default behavior. Also, there's a trailing space on line 18 that should be removed.

Apply this diff to clean up the build section:

 build:
   noarch: generic
   number: 0
-  skip: False
   run_exports:
     - {{ pin_subpackage('sipros', max_pin="x") }}
-    
🧰 Tools
🪛 yamllint

[error] 18-18: trailing spaces

(trailing-spaces)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 65005c7 and 9239825.

📒 Files selected for processing (4)
  • recipes/sipros/LICENSE (1 hunks)
  • recipes/sipros/Raxport.sh (1 hunks)
  • recipes/sipros/build.sh (1 hunks)
  • recipes/sipros/meta.yaml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • recipes/sipros/LICENSE
🧰 Additional context used
🪛 yamllint
recipes/sipros/meta.yaml

[error] 18-18: trailing spaces

(trailing-spaces)


[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)

🔇 Additional comments (7)
recipes/sipros/build.sh (1)

1-3: LGTM: Proper script setup

The script starts with the correct shebang and uses set -e, which is a good practice to ensure the script stops on any error.

recipes/sipros/Raxport.sh (1)

1-2: LGTM: Proper shebang and source file resolution.

The script starts with the correct shebang and properly initializes the SOURCE variable using BASH_SOURCE[0]. This approach ensures the script can determine its own location, which is crucial for the subsequent operations.

recipes/sipros/meta.yaml (5)

8-10: Source section looks good!

The use of a GitHub release URL with the version variable and the inclusion of a SHA256 checksum for integrity verification are both good practices.


30-37: Test section looks comprehensive!

The test commands cover various executables and Python scripts, which is good for ensuring the package is correctly installed and functional. The use of $CONDA_PREFIX in the Python script paths is correct.


19-28: ⚠️ Potential issue

Update Python version and review dependency constraints.

  1. Python 2.7 is no longer supported. Consider updating to a supported Python 3 version.
  2. Some package version constraints might be too restrictive. Review if these specific versions are necessary or if they can be relaxed.
  3. There are no build-time dependencies specified. Ensure this is intentional and add them if needed.

Here's a suggested update for the Python dependency:

-    - python =2.7
+    - python >=3.7

To check if the package is compatible with Python 3, run:

#!/bin/bash
# Check for Python version compatibility
rg -i 'python' recipes/sipros/meta.yaml
ast-grep --lang python --pattern 'print $_' .

This will help identify any Python 2-specific syntax that needs updating.


48-54: Verify the DOI.

The DOI (10.1186/s40168-024-01866-1) appears to be for a future publication (2024). Please verify if this is correct or if it's a typo.

To check the validity of the DOI, you can use the following command:

If the DOI is valid, the command should return a HTTP 200 status. If not, it might return a 404 or another error code.

✅ Verification successful

DOI is valid and correctly points to an existing resource.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check DOI validity
curl -sI "https://doi.org/10.1186/s40168-024-01866-1" | grep -i "HTTP/"

Length of output: 189


Script:

#!/bin/bash
# Check DOI validity by following redirects
curl -sI -L "https://doi.org/10.1186/s40168-024-01866-1" | grep -i "HTTP/"

Length of output: 97


39-46: ⚠️ Potential issue

Address LICENSE file inclusion.

The comment suggests that the LICENSE file is included in the recipe folder because it's not in the release. This is unusual and should be addressed.

Consider the following actions:

  1. Open an issue in the upstream repository (https://github.com/thepanlab/Sipros4) to include the LICENSE file in future releases.
  2. Ensure the LICENSE file is properly included in the recipe folder for now.

To check if the LICENSE file is present in the recipe folder, run:

If the file is not found, it needs to be added to the recipe folder.

✅ Verification successful

LICENSE file is correctly included in the recipe folder.

The recipes/sipros/LICENSE file is present as required.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for LICENSE file in the recipe folder
ls recipes/sipros/LICENSE

Length of output: 189


Script:

#!/bin/bash
# Check for LICENSE file in the recipe folder
ls recipes/sipros/LICENSE

Length of output: 50

recipes/sipros/Raxport.sh Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)
recipes/sipros/meta.yaml (3)

1-6: Consider using Semantic Versioning (SemVer) for the version number.

The current version "4.01" doesn't strictly follow the Semantic Versioning (SemVer) format. Consider changing it to "4.1.0" to align with SemVer guidelines, which use the format MAJOR.MINOR.PATCH.

This change would improve version clarity and help with dependency management.

🧰 Tools
🪛 yamllint

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


12-18: LGTM: Build section is well-defined, with a minor formatting issue.

The build section is correctly configured with noarch: generic and appropriate run_exports for version pinning.

However, there's a minor formatting issue:

Remove the trailing space on line 18 to improve code cleanliness:

 run_exports:
   - {{ pin_subpackage('sipros', max_pin="x") }}
-    
+
🧰 Tools
🪛 yamllint

[error] 18-18: trailing spaces

(trailing-spaces)


30-37: LGTM: Comprehensive test section with a suggestion for improvement.

The test section is well-defined with a good range of commands to verify the installation of various components.

For improved robustness, consider using the $PREFIX environment variable instead of $CONDA_PREFIX in the Python script test commands. This ensures compatibility across different conda-based package managers:

-    - python $CONDA_PREFIX/EnsembleScripts/sipros_psm_tabulating.py -v
-    - python $CONDA_PREFIX/V4Scripts/sipros_peptides_filtering.py -v
+    - python $PREFIX/EnsembleScripts/sipros_psm_tabulating.py -v
+    - python $PREFIX/V4Scripts/sipros_peptides_filtering.py -v

This change makes the recipe more portable across different conda-based environments.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 9239825 and 836ec78.

📒 Files selected for processing (1)
  • recipes/sipros/meta.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/sipros/meta.yaml

[error] 18-18: trailing spaces

(trailing-spaces)


[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)

🔇 Additional comments (4)
recipes/sipros/meta.yaml (4)

8-10: LGTM: Source section is well-defined.

The source section correctly uses a GitHub release URL with Jinja2 templating for the version. The inclusion of a SHA256 checksum is a good practice for ensuring package integrity.


48-54: LGTM: Well-defined extra section with maintainers and identifiers.

The extra section properly includes recipe maintainers and relevant identifiers (biotools and DOI). This information is valuable for users and follows Bioconda best practices.


19-28: ⚠️ Potential issue

Update Python dependency and consider adding build-time requirements.

  1. The recipe specifies Python 2.7, which has reached its end-of-life and is no longer supported. This may pose security risks and compatibility issues. Consider updating to a supported Python version (e.g., 3.7+) if possible.

  2. The recipe only specifies runtime dependencies. If there are any build-time dependencies required for compiling or setting up the package, they should be added under a host: section.

Here's a suggested update to the requirements section:

requirements:
  host:
    - python >=3.7
    # Add any build-time dependencies here
  run:
    - bioconductor-biostrings
    - mono
    - numpy
    - python >=3.7
    - r-base
    - r-stringr
    - r-tidyr
    - scikit-learn

Please verify the compatibility of the package with Python 3 before making this change. If Python 2.7 is absolutely required, consider adding a comment explaining why, and ensure all other dependencies are compatible with this Python version.

To check if the package is compatible with Python 3, you can run:

This will search for any explicit mentions of Python 2 in the source code, which might indicate potential compatibility issues.

✅ Verification successful

Verified: Python 2 usage not detected; updating Python dependency is appropriate.

The verification process found no references to Python 2 in the .py files of the codebase. Therefore, updating the Python dependency from 2.7 to a supported version (e.g., 3.7+) is appropriate and should not introduce compatibility issues.

  • No Python 2 dependencies detected in the source code.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for Python 3 compatibility in the source code
rg -i 'python\s*2' --type py

Length of output: 124


Script:

#!/bin/bash
# Check for Python 2 compatibility in the source code
rg -i 'python\s*2' --type py

Length of output: 30


Script:

#!/bin/bash
# Check for Python 2 compatibility in the source code
rg -i 'python\s*2' --type py

Length of output: 30


39-46: LGTM: Comprehensive about section with a note on the license file.

The about section is well-defined with all necessary information. However, there's a note about including the license file in the recipe folder due to an issue with the upstream repository.

To ensure this is addressed properly:

  1. Verify that the LICENSE file is indeed included in the recipe folder.
  2. Consider opening an issue in the upstream repository to include the LICENSE file in future releases.

You can verify the presence of the LICENSE file with:

If the LICENSE file is missing, please add it to the recipe folder and update the license_file entry to use the local path.

✅ Verification successful

Please run the following corrected shell script to verify if the LICENSE file exists in the recipe folder:


Verified: LICENSE file is present in the recipe folder.

The LICENSE file is correctly included in the recipes/sipros/ directory as required.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if LICENSE file exists in the recipe folder
if [ -f "recipes/sipros/LICENSE" ]; then
    echo "LICENSE file found in the recipe folder."
else
    echo "LICENSE file not found in the recipe folder."
fi

Length of output: 124


Script:

# 
#!/bin/bash
# Check if LICENSE file exists in the recipe folder
if [ -f "recipes/sipros/LICENSE" ]; then
    echo "LICENSE file found in the recipe folder."
else
    echo "LICENSE file not found in the recipe folder."
fi

Length of output: 126

@xyz1396
Copy link
Contributor Author

xyz1396 commented Oct 2, 2024

@bioconda-bot please add label

@BiocondaBot BiocondaBot added the please review & merge set to ask for merge label Oct 2, 2024
Copy link
Contributor

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me @xyz1396 . Just 2 small comments.

recipes/sipros/build.sh Outdated Show resolved Hide resolved
recipes/sipros/build.sh Outdated Show resolved Hide resolved
recipes/sipros/Raxport.sh Show resolved Hide resolved
@bernt-matthias bernt-matthias mentioned this pull request Oct 2, 2024
@bernt-matthias
Copy link
Contributor

Anything to add @xyz1396 ? Or should we merge as it is.

@xyz1396
Copy link
Contributor Author

xyz1396 commented Oct 8, 2024

Anything to add @xyz1396 ? Or should we merge as it is.

I am trying to symlink the python scripts. Could I add it after the merge?

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (8)
recipes/sipros/copyConfigTemplate.sh (2)

1-7: LGTM with a suggestion for improvement

The initial setup looks good. The shebang is correct, and the OUTPUT_PATH logic is sound. However, consider adding input validation for the OUTPUT_PATH to ensure it's a valid directory.

You could add a check like this after line 7:

if [ ! -d "$OUTPUT_PATH" ]; then
    echo "Error: $OUTPUT_PATH is not a valid directory"
    exit 1
fi

1-16: Add documentation and consider further enhancements

The script serves its purpose of copying configuration templates based on user input. However, there are a few areas for improvement:

  1. Add a comment block at the beginning of the script explaining its purpose, usage, and the significance of the configuration templates.
  2. Consider implementing the robustness and error handling improvements suggested in the previous comments.
  3. You might want to add logging or more detailed error messages to aid in troubleshooting.

Overall, the script is a good start but could benefit from these enhancements to make it more robust and maintainable.

recipes/sipros/build.sh (3)

9-13: Consider adding error checking for directory copying operations.

While the copying of script directories is correct, it might be beneficial to add error checking to ensure the directories exist before attempting to copy them. This can prevent silent failures if the source directories are missing.

Consider adding checks like this before each copy operation:

if [ ! -d "EnsembleScripts" ]; then
    echo "Error: EnsembleScripts directory not found"
    exit 1
fi

15-25: LGTM: Proper handling of Python scripts with a minor optimization suggestion.

The processing of Python scripts, including adding shebang lines and creating symbolic links, is done correctly. The naming convention for the links is clear and consistent.

Consider combining the two loops for Python scripts to reduce code duplication:

for dir in EnsembleScripts V4Scripts; do
    for script in "$PREFIX/$dir"/*.py; do
        baseName=$(basename "$script" .py)
        sed -i '1i#!/usr/bin/env python\n' "$script"
        ln -s "$script" "$PREFIX/bin/${dir}_$baseName"
    done
done

This approach is more maintainable and easier to extend if needed.


33-35: LGTM: Proper final file operations with a minor consistency suggestion.

The copying of additional scripts and setting of execute permissions are done correctly.

For consistency, consider using quotes around $PREFIX in the last line:

chmod u+x "$PREFIX"/bin/*

This maintains consistency with the usage in previous lines and is a good practice when dealing with variables that might contain spaces.

recipes/sipros/meta.yaml (3)

12-17: Remove redundant skip: False line.

The build section is well-defined with appropriate noarch setting and run_exports for subpackage pinning. However, the skip: False line is redundant and can be safely removed.

Apply this diff to remove the redundant line:

 build:
   noarch: generic
   number: 0
-  skip: False
   run_exports:
     - {{ pin_subpackage('sipros', max_pin="x") }}

30-40: Improve test command consistency and consider symlinking scripts.

The test commands provide good coverage of the package components. However, there are some inconsistencies in naming conventions (e.g., EnsembleScripts_ vs V4Scripts_). Additionally, as suggested in the past review comments, consider symlinking Python scripts to $PREFIX/bin or using Python entry points for easier access.

  1. Standardize the naming conventions for test commands.
  2. Implement symlinking or Python entry points as suggested in the past review comments.

Example of using Python entry points in the build.sh script:

#!/bin/bash
# ... other build steps ...

# Create entry points for Python scripts
cat << EOF >> $PREFIX/bin/sipros_psm_tabulating
#!/bin/bash
python $PREFIX/EnsembleScripts/sipros_psm_tabulating.py "\$@"
EOF
chmod +x $PREFIX/bin/sipros_psm_tabulating

# Repeat for other scripts...

Then update the test commands to use these entry points:

test:
  commands:
    - configGenerator -h
    - SiprosEnsembleOMP --help
    - SiprosV4OMP --help
    - Raxport -h
    - sipros_psm_tabulating -v
    - sipros_peptides_filtering -v
    - getSpectraCountInEachFT -help
    - refineProteinFDR -help
    - copyConfigTemplate -SIP

This approach allows users to call the scripts directly by their names without using $CONDA_PREFIX.

Would you like assistance in implementing these changes or creating a GitHub issue to track this task?


18-18: Remove trailing spaces.

There are trailing spaces on this line that should be removed.

Apply this diff to remove the trailing spaces:

-    
+
🧰 Tools
🪛 yamllint

[error] 18-18: trailing spaces

(trailing-spaces)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c50f62a and fdbf5e6.

📒 Files selected for processing (3)
  • recipes/sipros/build.sh (1 hunks)
  • recipes/sipros/copyConfigTemplate.sh (1 hunks)
  • recipes/sipros/meta.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/sipros/meta.yaml

[error] 18-18: trailing spaces

(trailing-spaces)


[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)

🔇 Additional comments (7)
recipes/sipros/build.sh (3)

1-7: LGTM: Proper script setup and initial file operations.

The script starts with the correct shebang line and uses set -e for error handling, which are good practices. The initial directory creation and file copying operations are appropriate for the build process.


27-31: LGTM: Proper handling of R scripts.

The processing of R scripts, including adding the correct shebang lines and creating symbolic links, is done correctly. The approach is consistent with the Python script processing, which is good for maintainability.


1-35: Overall, the build script is well-structured and functional.

The script successfully sets up the sipros package, copying necessary files, processing Python and R scripts, and setting appropriate permissions. The suggestions provided in the review are minor optimizations for error handling, code reduction, and consistency. These improvements would enhance the script's robustness and maintainability, but the current implementation is already functional and follows good practices.

recipes/sipros/meta.yaml (4)

1-10: LGTM: Package and source information are well-defined.

The package name and version are correctly set using Jinja2 templating, which is good for maintainability. The source URL and SHA256 checksum are properly specified, ensuring integrity during package installation.

🧰 Tools
🪛 yamllint

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


51-57: LGTM: Extra section is well-defined.

The extra section includes appropriate recipe maintainers and identifiers. The inclusion of the biotools identifier and DOI enhances the package's discoverability and citability.


42-49: ⚠️ Potential issue

Follow up on LICENSE file inclusion in the upstream repository.

The about section is well-defined with comprehensive information. However, the comment about including the LICENSE file in the recipe folder indicates an issue with the upstream repository.

Follow up with the upstream repository (https://github.com/thepanlab/Sipros4) to include the LICENSE file in their releases. This will allow you to remove the LICENSE file from the recipe folder and update the license_file entry accordingly.

To verify if the issue has been resolved, you can run:

#!/bin/bash
# Check if the LICENSE file is present in the latest release
latest_release=$(gh api repos/thepanlab/Sipros4/releases/latest --jq .tag_name)
gh api repos/thepanlab/Sipros4/contents/LICENSE?ref=$latest_release

If the issue is still present, consider opening a GitHub issue in the upstream repository to address this.


19-28: ⚠️ Potential issue

Update dependencies and add build requirements.

The requirements section has some issues:

  1. The package uses Python 2.7, which is no longer supported. Consider updating to a modern Python version.
  2. The numpy and scikit-learn versions are outdated. Update these to more recent versions for better compatibility and security.
  3. There are no build-time dependencies specified, which is unusual for a package with compiled components.

Consider applying these changes:

  1. Update Python dependency to a supported version (e.g., python >=3.7).
  2. Update numpy and scikit-learn to more recent versions.
  3. Add a host section for build-time dependencies.

Example diff:

 requirements:
+  host:
+    - python >=3.7
+    - pip
   run:
     - bioconductor-biostrings
     - mono
-    - numpy =1.16
-    - python =2.7
+    - numpy >=1.19
+    - python >=3.7
     - r-base
     - r-stringr
     - r-tidyr
-    - scikit-learn =0.20.3
+    - scikit-learn >=0.24

Please verify compatibility with these updated versions before applying the changes.

To check for any compatibility issues with the suggested updates, you can run:

recipes/sipros/copyConfigTemplate.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @xyz1396 . I think the suggested refactoring could be useful.

Let me know if I should merge as it is, or if you want to integrate the suggestion.

recipes/sipros/copyConfigTemplate.sh Outdated Show resolved Hide resolved
Co-authored-by: M Bernt <m.bernt@ufz.de>
@bernt-matthias bernt-matthias merged commit 6942d4d into bioconda:master Oct 9, 2024
7 checks passed
@bernt-matthias
Copy link
Contributor

Thanks a lot for all the work @xyz1396

I can start now to work on a Galaxy integration. Let me know if you have any wishes regarding this.

@xyz1396
Copy link
Contributor Author

xyz1396 commented Oct 11, 2024

Thanks a lot for all the work @xyz1396

I can start now to work on a Galaxy integration. Let me know if you have any wishes regarding this.

Feel free to tag me when creating the PR, and I'll assist with it.

This was referenced Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please review & merge set to ask for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants