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 Dssp #50714

Closed
wants to merge 20 commits into from
Closed

Add Dssp #50714

wants to merge 20 commits into from

Conversation

berkeucar
Copy link

@berkeucar berkeucar commented Sep 14, 2024

I added DSSP (https://swift.cmbi.umcn.nl/gv/dssp/index.html) with slightly changing the script from https://anaconda.org/salilab/dssp.


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>.

@berkeucar
Copy link
Author

@BiocondaBot please add label

@BiocondaBot BiocondaBot added the please review & merge set to ask for merge label Sep 19, 2024
@berkeucar berkeucar changed the title Dssp Add Dssp Sep 19, 2024
@berkeucar
Copy link
Author

@BiocondaBot please fetch artifacts

@BiocondaBot
Copy link
Collaborator

Package(s) built are ready for inspection:

Arch Package Zip File / Repodata CI Instructions
linux-64 dssp-3.0.0-h6a68c12_0.tar.bz2 LinuxArtifacts.zip Azure
showYou may also use conda to install after downloading and extracting the zip file. From the LinuxArtifacts directory: conda install -c ./packages <package name>
osx-64 dssp-3.0.0-h43736c0_0.tar.bz2 OSXArtifacts.zip Azure
showYou may also use conda to install after downloading and extracting the zip file. From the OSXArtifacts directory: conda install -c ./packages <package name>

Docker image(s) built:

Package Tag CI Install with docker
dssp 3.0.0--h6a68c12_0 Azure
showImages for Azure are in the LinuxArtifacts zip file above.gzip -dc LinuxArtifacts/images/dssp:3.0.0--h6a68c12_0.tar.gz | docker load

recipes/dssp/build.sh Outdated Show resolved Hide resolved
recipes/dssp/meta.yaml Outdated Show resolved Hide resolved
build:
- {{ compiler('c') }}
- {{ compiler('cxx') }}
- boost-cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

Only 'make' and the compilers should be in 'build'. The libraries should be in 'host'

about:
home: http://swift.cmbi.ru.nl/gv/dssp/
summary: 'Secondary structure assignment'
license: Boost
Copy link
Contributor

Choose a reason for hiding this comment

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

license_file ?

berkeucar and others added 2 commits September 20, 2024 10:05
Co-authored-by: Martin Grigorov <[email protected]>
Co-authored-by: Martin Grigorov <[email protected]>
@martin-g
Copy link
Contributor

There is a recipe in conda-forge for it already: https://anaconda.org/conda-forge/dssp

Copy link
Contributor

coderabbitai bot commented Nov 7, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

This pull request introduces several significant changes to the recipes/dssp directory, including the addition of a new build script (build.sh), a metadata file (meta.yaml), and modifications to the codebase to transition from the deprecated std::tr1 namespace to the standard std namespace. The build.sh script automates the build process by configuring the environment, modifying the makefile, and ensuring compatibility with macOS. It also sets up the installation using parallel jobs. The meta.yaml file defines package metadata, dependencies, and a test command for the DSSP package version 3.0.0. Additionally, the changes in the codebase include updating method signatures in the MResidue and primitives-3d classes to utilize the standard tuple library, enhancing code compliance with modern C++ standards.

Possibly related PRs

  • [add pantax #50983] - The build.sh script in the Pantax PR also modifies the build process for a software package, similar to the build.sh in the main PR for DSSP, indicating a shared focus on automating build processes.
  • [add meta #51349] - The build.sh script in the Meta PR introduces a build process for a software package, paralleling the changes made in the DSSP build.sh, which also focuses on setting up the build environment.
  • [Add genodsp #51488] - The build.sh script in the Genodsp PR automates the build process, akin to the build.sh in the DSSP PR, both emphasizing the creation of necessary directories and handling the installation of binaries.
  • [Add svtopo recipe #51665] - The build.sh script in the Svtopo PR introduces a build process similar to the DSSP build.sh, focusing on directory creation and file copying as part of the installation process.
  • [add ampd-up v1.0.0 #51688] - The build.sh script in the AMPd-Up PR also automates the installation process, similar to the DSSP build.sh, by creating necessary directories and managing file permissions.

Suggested reviewers

  • martin-g

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: 7

🧹 Outside diff range and nitpick comments (11)
recipes/dssp/build.sh (2)

19-21: Quote the uname command to prevent word splitting

The uname command should be quoted.

Apply this fix:

-if [ `uname -s` = "Darwin" ]; then
+if [ "`uname -s`" = "Darwin" ]; then
🧰 Tools
🪛 Shellcheck

[warning] 19-19: Quote this to prevent word splitting.

(SC2046)


1-23: Package placement in bioconda is appropriate

Although DSSP exists in conda-forge, its placement in bioconda is appropriate as it's directly related to biological sciences (protein structure analysis).

🧰 Tools
🪛 Shellcheck

[warning] 19-19: Quote this to prevent word splitting.

(SC2046)

recipes/tamper/meta.yaml (2)

15-16: Consider removing run_exports for this Python package.

The run_exports with pin_subpackage is typically used for compiled libraries to ensure ABI compatibility. For a Python package marked as noarch: generic, this level of version pinning might be unnecessarily restrictive and could complicate dependency resolution.

  build:
    number: 1
    noarch: generic
-   run_exports:
-     - {{ pin_subpackage(name, max_pin='x.x') }}

33-36: Enhance test coverage.

Current tests only verify command existence. Consider adding:

  1. Import tests for Python modules
  2. Basic functionality tests with minimal example data
 test:
+  imports:
+    - tamper
   commands:
     - train_tAMPer -h
     - predict_tAMPer -h
+    - python -c "from tamper import predict; assert predict"
recipes/tamper/build.sh (2)

3-6: Add error checking and verbose output for directory creation.

While the directory structure is correct, the script could be more robust with error checking and verbose output.

-mkdir -p ${PREFIX}/bin/
-mkdir -p ${PREFIX}/share/tamper/
-mkdir -p ${PREFIX}/share/tamper/src/
-mkdir -p ${PREFIX}/share/tamper/checkpoints/trained
+mkdir -pv ${PREFIX}/bin/ || exit 1
+mkdir -pv ${PREFIX}/share/tamper/ || exit 1
+mkdir -pv ${PREFIX}/share/tamper/src/ || exit 1
+mkdir -pv ${PREFIX}/share/tamper/checkpoints/trained || exit 1

11-15: Fix typo and optimize permission commands.

The comment contains a typo, and the chmod commands can be combined for efficiency.

-#This allows src code to be executbale for user and group
-chmod -R u+x ${PREFIX}/share/tamper/src/
-chmod -R g+x ${PREFIX}/share/tamper/src/
+# This allows source code to be executable for user and group
+chmod -R ug+x ${PREFIX}/share/tamper/src/ || exit 1
recipes/dssp/meta.yaml (4)

3-3: Improve attribution comment clarity.

The current comment should be more specific about what modifications were made to the original recipe. This helps future maintainers understand the differences.

-# This file, the patch file and build file were taken from conda salilab channel's recipes - please see https://github.com/salilab/conda-recipes for more.
+# This recipe is based on the salilab channel's DSSP recipe (https://github.com/salilab/conda-recipes) with the following modifications:
+# - Updated for version 3.0.0
+# - Modified dependencies structure
+# For more details, see the original recipe.

12-12: Complete the patch file documentation.

The comment for the patch file is incomplete and should provide more context about its purpose.

-    - dssp-boost-tr1.patch #Rebuild with latest gcc and boost - this was taken from the 
+    - dssp-boost-tr1.patch  # Patch to update std::tr1 namespace usage to std namespace for compatibility with modern gcc and boost

50-52: Enhance test coverage.

The current test only checks if the command runs. Consider adding more comprehensive tests:

  1. Test actual functionality with a small PDB file
  2. Verify output format
  3. Test help command
 test:
+  files:
+    - test.pdb
   commands:
     - mkdssp --version
+    - mkdssp --help
+    - mkdssp -i test.pdb -o test.dssp

Would you like me to help create a minimal test.pdb file and expected output for testing?


1-52: Consider using conda-forge package instead of creating a new recipe.

As mentioned by @martin-g, there's already a DSSP package in conda-forge (https://anaconda.org/conda-forge/dssp). According to Bioconda guidelines, new recipes not directly related to biological sciences should be submitted to conda-forge instead.

Consider either:

  1. Using the existing conda-forge package as a dependency
  2. Contributing improvements to the conda-forge recipe
  3. Providing strong justification for maintaining a separate package in Bioconda
🧰 Tools
🪛 yamllint

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

(syntax)

recipes/dssp/dssp-boost-tr1.patch (1)

65-65: Fix typo in variable name.

There's a typo in the variable name: alhpa should be alpha.

-  double alhpa = 360;
+  double alpha = 360;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between da4e71c and c81df5d.

📒 Files selected for processing (5)
  • recipes/dssp/build.sh (1 hunks)
  • recipes/dssp/dssp-boost-tr1.patch (1 hunks)
  • recipes/dssp/meta.yaml (1 hunks)
  • recipes/tamper/build.sh (1 hunks)
  • recipes/tamper/meta.yaml (1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/dssp/build.sh

[warning] 19-19: Quote this to prevent word splitting.

(SC2046)

🪛 yamllint
recipes/dssp/meta.yaml

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

(syntax)

recipes/tamper/meta.yaml

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

(syntax)

🔇 Additional comments (10)
recipes/dssp/build.sh (5)

1-4: LGTM: Proper script initialization

Good use of set -e to ensure the script fails immediately if any command fails.


5-6: LGTM: Proper dynamic linking setup

Correctly removes static linking which is appropriate for conda packages.


23-23: LGTM: Proper parallel build setup


16-21: Verify the clang warning suppression flag

The -Wno-enum-constexpr-conversion flag is specific to clang. Let's ensure it's supported by the version of clang being used.

#!/bin/bash
# Description: Check if the warning flag is supported by clang
# Test: Try compiling a minimal program with the flag
echo "#include <iostream>" > test.cpp
${CXX} -Wno-enum-constexpr-conversion -c test.cpp 2>&1 || echo "Warning flag not supported"
rm -f test.cpp test.o
🧰 Tools
🪛 Shellcheck

[warning] 19-19: Quote this to prevent word splitting.

(SC2046)


8-14: LGTM: Proper make configuration setup

Configuration correctly uses conda environment variables for paths and compiler settings.

Let's verify the Boost paths are correct:

recipes/tamper/build.sh (1)

1-34: Verify PR scope and dependencies.

This build script is for the tamper package, but the PR objectives mention adding DSSP. Please clarify:

  1. Is this file part of the intended scope of the PR?
  2. If yes, how does this relate to the DSSP addition mentioned in the PR objectives?
  3. Should there be any DSSP-related dependencies or integration points in this script?
recipes/dssp/dssp-boost-tr1.patch (4)

8-9: LGTM: Correctly modernizes tuple unpacking.

The change from std::tr1::tie to std::tie is appropriate as it moves away from the deprecated TR1 namespace to modern C++ standards.


20-21: LGTM: Consistent modernization of tuple usage.

The changes correctly update both the function signature and tuple creation:

  1. Return type changed from std::tr1::tuple to std::tuple
  2. Tuple creation updated from std::tr1::make_tuple to std::make_tuple

Also applies to: 29-30


62-63: LGTM: Consistent method modernization.

The changes correctly update both the method declaration and implementation to use standard C++ tuples instead of TR1.

Also applies to: 71-72, 83-84


41-42: LGTM: Proper header modernization and consistent declarations.

The changes correctly:

  1. Replace the deprecated Boost TR1 tuple header with the standard C++ tuple header
  2. Update the function declaration to match its implementation

Let's verify no other TR1 dependencies remain:

Also applies to: 50-51

✅ Verification successful

The previous search might have been too narrow. Let's try with a broader pattern and also use ast-grep to specifically look for boost/tr1 includes. This will help us verify if the TR1 modernization is complete across the codebase.


The search results show that TR1 dependencies are only present in the patch file recipes/dssp/dssp-boost-tr1.patch which is actually the patch that's making the changes to remove those TR1 dependencies. The patch is replacing:

  1. #include <boost/tr1/tuple.hpp> with #include <tuple>
  2. Several uses of std::tr1:: namespace with standard C++ equivalents

This indicates that the review comment is correct - the changes are properly modernizing the code by replacing Boost TR1 with standard C++ headers and namespaces.

LGTM: TR1 modernization changes are complete and consistent

The patch correctly removes all Boost TR1 dependencies in favor of standard C++ equivalents. No remaining TR1 dependencies were found in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining TR1 dependencies
rg --type cpp "tr1" 

Length of output: 19


Script:

#!/bin/bash
# Search for any remaining TR1 dependencies with case-insensitive and broader pattern
rg -i "boost.*tr1|tr1" 

# Also check specifically for any remaining boost/tr1 includes
ast-grep --pattern '#include <boost/tr1/$_>'

Length of output: 120729

- python >=3.9
run:
- biopython
- dssp
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use conda-forge dssp package instead of adding a new one.

According to the PR comments, DSSP is already available in conda-forge (https://anaconda.org/conda-forge/dssp). Instead of creating a new package in bioconda, consider using the existing conda-forge package by adding the channel specification:

- dssp
+ conda-forge::dssp
📝 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.

Suggested change
- dssp
- conda-forge::dssp

Comment on lines +28 to +31
- pytorch
- pytorch_cluster
- pytorch_geometric
- torch-scatter
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add version constraints for PyTorch dependencies.

The PyTorch ecosystem packages often have strict version compatibility requirements. Missing version constraints could lead to dependency conflicts.

-   pytorch
-   pytorch_cluster
-   pytorch_geometric
-   torch-scatter
+   pytorch>=2.0
+   pytorch_cluster>=1.6
+   pytorch_geometric>=2.3
+   torch-scatter>=2.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.

Suggested change
- pytorch
- pytorch_cluster
- pytorch_geometric
- torch-scatter
- pytorch>=2.0
- pytorch_cluster>=1.6
- pytorch_geometric>=2.3
- torch-scatter>=2.1

Comment on lines +17 to +23
#Adding python executable for the script to work
echo "#!/bin/bash" > ${PREFIX}/bin/predict_tAMPer
echo "python ${PREFIX}/share/tamper/src/predict_tAMPer.py \"\$@\"" >> ${PREFIX}/bin/predict_tAMPer

echo "#!/bin/bash" > ${PREFIX}/bin/train_tAMPer.py
echo "python ${PREFIX}/share/tamper/src/train_tAMPer.py \"\$@\"" >> ${PREFIX}/bin/train_tAMPer

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve script creation and fix inconsistent naming.

Several issues need attention:

  1. train_tAMPer.py has a .py extension despite being a bash script
  2. Using echo for script creation is error-prone
  3. Missing error checking
-echo "#!/bin/bash" > ${PREFIX}/bin/predict_tAMPer
-echo "python ${PREFIX}/share/tamper/src/predict_tAMPer.py \"\$@\"" >> ${PREFIX}/bin/predict_tAMPer
-
-echo "#!/bin/bash" > ${PREFIX}/bin/train_tAMPer.py
-echo "python ${PREFIX}/share/tamper/src/train_tAMPer.py \"\$@\"" >> ${PREFIX}/bin/train_tAMPer
+# Create predict_tAMPer wrapper
+cat > "${PREFIX}/bin/predict_tAMPer" << 'EOF' || exit 1
+#!/bin/bash
+python "${PREFIX}/share/tamper/src/predict_tAMPer.py" "$@"
+EOF
+
+# Create train_tAMPer wrapper (removed .py extension)
+cat > "${PREFIX}/bin/train_tAMPer" << 'EOF' || exit 1
+#!/bin/bash
+python "${PREFIX}/share/tamper/src/train_tAMPer.py" "$@"
+EOF
📝 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.

Suggested change
#Adding python executable for the script to work
echo "#!/bin/bash" > ${PREFIX}/bin/predict_tAMPer
echo "python ${PREFIX}/share/tamper/src/predict_tAMPer.py \"\$@\"" >> ${PREFIX}/bin/predict_tAMPer
echo "#!/bin/bash" > ${PREFIX}/bin/train_tAMPer.py
echo "python ${PREFIX}/share/tamper/src/train_tAMPer.py \"\$@\"" >> ${PREFIX}/bin/train_tAMPer
#Adding python executable for the script to work
# Create predict_tAMPer wrapper
cat > "${PREFIX}/bin/predict_tAMPer" << 'EOF' || exit 1
#!/bin/bash
python "${PREFIX}/share/tamper/src/predict_tAMPer.py" "$@"
EOF
# Create train_tAMPer wrapper (removed .py extension)
cat > "${PREFIX}/bin/train_tAMPer" << 'EOF' || exit 1
#!/bin/bash
python "${PREFIX}/share/tamper/src/train_tAMPer.py" "$@"
EOF

Comment on lines +28 to +33
chmod +x ${PREFIX}/bin/predict_tAMPer
chmod +x ${PREFIX}/bin/train_tAMPer

#Verify permissions
echo "Permissions after chmod:"
ls -l ${PREFIX}/share/tamper/src/
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve permission setting and verification.

The current verification process could be more robust with proper error checking and validation.

-chmod +x ${PREFIX}/bin/predict_tAMPer
-chmod +x ${PREFIX}/bin/train_tAMPer
+chmod +x "${PREFIX}/bin/predict_tAMPer" || exit 1
+chmod +x "${PREFIX}/bin/train_tAMPer" || exit 1
 
-#Verify permissions
-echo "Permissions after chmod:"
-ls -l ${PREFIX}/share/tamper/src/
+# Verify executables are properly set up
+for cmd in predict_tAMPer train_tAMPer; do
+    if [ ! -x "${PREFIX}/bin/${cmd}" ]; then
+        echo "Error: ${cmd} is not executable" >&2
+        exit 1
+    fi
+done
📝 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.

Suggested change
chmod +x ${PREFIX}/bin/predict_tAMPer
chmod +x ${PREFIX}/bin/train_tAMPer
#Verify permissions
echo "Permissions after chmod:"
ls -l ${PREFIX}/share/tamper/src/
chmod +x "${PREFIX}/bin/predict_tAMPer" || exit 1
chmod +x "${PREFIX}/bin/train_tAMPer" || exit 1
# Verify executables are properly set up
for cmd in predict_tAMPer train_tAMPer; do
if [ ! -x "${PREFIX}/bin/${cmd}" ]; then
echo "Error: ${cmd} is not executable" >&2
exit 1
fi
done

Comment on lines +7 to +9
cp -r src/. ${PREFIX}/share/tamper/src/
cp -r checkpoints/trained/. ${PREFIX}/share/tamper/checkpoints/trained
ls ${PREFIX}/share/tamper/src/
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add source directory verification and error checking for file copying.

The current implementation doesn't verify source directories exist or handle copy errors properly.

+# Verify source directories exist
+[ ! -d "src" ] && echo "Error: src directory not found" && exit 1
+[ ! -d "checkpoints/trained" ] && echo "Error: checkpoints/trained directory not found" && exit 1
+
-cp -r src/. ${PREFIX}/share/tamper/src/
-cp -r checkpoints/trained/. ${PREFIX}/share/tamper/checkpoints/trained
-ls ${PREFIX}/share/tamper/src/
+cp -rv src/. ${PREFIX}/share/tamper/src/ || exit 1
+cp -rv checkpoints/trained/. ${PREFIX}/share/tamper/checkpoints/trained || exit 1
+
+# Verify files were copied
+echo "Verifying copied files..."
+[ -z "$(ls -A ${PREFIX}/share/tamper/src/)" ] && echo "Error: No files copied to src directory" && exit 1
+[ -z "$(ls -A ${PREFIX}/share/tamper/checkpoints/trained/)" ] && echo "Error: No files copied to checkpoints directory" && 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.

Suggested change
cp -r src/. ${PREFIX}/share/tamper/src/
cp -r checkpoints/trained/. ${PREFIX}/share/tamper/checkpoints/trained
ls ${PREFIX}/share/tamper/src/
# Verify source directories exist
[ ! -d "src" ] && echo "Error: src directory not found" && exit 1
[ ! -d "checkpoints/trained" ] && echo "Error: checkpoints/trained directory not found" && exit 1
cp -rv src/. ${PREFIX}/share/tamper/src/ || exit 1
cp -rv checkpoints/trained/. ${PREFIX}/share/tamper/checkpoints/trained || exit 1
# Verify files were copied
echo "Verifying copied files..."
[ -z "$(ls -A ${PREFIX}/share/tamper/src/)" ] && echo "Error: No files copied to src directory" && exit 1
[ -z "$(ls -A ${PREFIX}/share/tamper/checkpoints/trained/)" ] && echo "Error: No files copied to checkpoints directory" && exit 1

Comment on lines +30 to +49
about:
home: http://swift.cmbi.ru.nl/gv/dssp/
summary: 'Secondary structure assignment'
license: Boost
description: >
The DSSP program was designed by Wolfgang Kabsch and Chris Sander to
standardize secondary structure assignment. DSSP is a database of secondary
structure assignments (and much more) for all protein entries in the Protein
Data Bank (PDB). DSSP is also the program that calculates DSSP entries from
PDB entries. DSSP does not predict secondary structure.

Note that this is a very old version of DSSP, which we have used in the
past. It is kept here in the hope that others may find it useful - but
it may not work on newer systems (e.g. Apple Silicon Macs). We recommend
using the most recent version of DSSP instead.
See [the DSSP website](http://swift.cmbi.ru.nl/gv/dssp/) for more details.

If you have problems with this conda package, please
[open an issue](https://github.com/salilab/conda-recipes/issues).

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Update package metadata for better compliance.

Several improvements are needed in the about section:

  1. Add license_file field
  2. Use HTTPS for the home URL
  3. Consider adding doc_url and dev_url fields
  4. The warning about old version seems inconsistent with version 3.0.0
 about:
-  home: http://swift.cmbi.ru.nl/gv/dssp/
+  home: https://swift.cmbi.ru.nl/gv/dssp/
   summary: 'Secondary structure assignment'
   license: Boost
+  license_file: LICENSE
+  doc_url: https://swift.cmbi.ru.nl/gv/dssp/DSSP_3.html
+  dev_url: https://github.com/cmbi/dssp
   description: >
     The DSSP program was designed by Wolfgang Kabsch and Chris Sander to
     standardize secondary structure assignment. DSSP is a database of secondary
     structure assignments (and much more) for all protein entries in the Protein
     Data Bank (PDB). DSSP is also the program that calculates DSSP entries from
     PDB entries. DSSP does not predict secondary structure.

-    Note that this is a very old version of DSSP, which we have used in the
-    past. It is kept here in the hope that others may find it useful - but
-    it may not work on newer systems (e.g. Apple Silicon Macs). We recommend
-    using the most recent version of DSSP instead.
+    This is version 3.0.0 of DSSP, which includes modern C++ standards support
+    and compatibility with newer systems.
     See [the DSSP website](http://swift.cmbi.ru.nl/gv/dssp/) for more details.

-    If you have problems with this conda package, please
-    [open an issue](https://github.com/salilab/conda-recipes/issues).
+    If you have problems with this conda package, please open an issue at
+    https://github.com/bioconda/bioconda-recipes/issues

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +19 to +28
requirements:
build:
- {{ compiler('c') }}
- {{ compiler('cxx') }}
- boost-cpp
- zlib
- bzip2
- make
run:
- {{ pin_compatible('boost-cpp') }}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Restructure dependencies sections.

Libraries should be in the host section, not in build. Only build tools like make and compilers should be in the build section. This ensures proper build environment setup.

 requirements:
   build:
     - {{ compiler('c') }}
     - {{ compiler('cxx') }}
-    - boost-cpp
-    - zlib
-    - bzip2
     - make
+  host:
+    - boost-cpp
+    - zlib
+    - bzip2
   run:
     - {{ pin_compatible('boost-cpp') }}
+    - zlib
+    - bzip2
📝 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.

Suggested change
requirements:
build:
- {{ compiler('c') }}
- {{ compiler('cxx') }}
- boost-cpp
- zlib
- bzip2
- make
run:
- {{ pin_compatible('boost-cpp') }}
requirements:
build:
- {{ compiler('c') }}
- {{ compiler('cxx') }}
- make
host:
- boost-cpp
- zlib
- bzip2
run:
- {{ pin_compatible('boost-cpp') }}
- zlib
- bzip2

@berkeucar berkeucar closed this Nov 15, 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