Add fc-virus recipe #51809
Add fc-virus recipe #51809
Conversation
|
Warning Rate limit exceeded@mencian has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several key changes to the Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (1)
recipes/fc/build.sh (1)
1-8: Consider adding build verification stepsThe script should verify the build was successful by:
- Testing if the binary is executable
- Running a basic smoke test
Add these lines at the end of the script:
# Verify installation if ! "${PREFIX}/bin/fc-virus" --help &>/dev/null; then echo "Error: Installation verification failed" >&2 exit 1 fi🧰 Tools
🪛 Shellcheck
[error] 2-2: The shebang must be on the first line. Delete blanks and move comments.
(SC1128)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
recipes/fc/LICENSE(1 hunks)recipes/fc/build.sh(1 hunks)recipes/fc/meta.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- recipes/fc/LICENSE
🧰 Additional context used
🪛 Shellcheck
recipes/fc/build.sh
[error] 2-2: The shebang must be on the first line. Delete blanks and move comments.
(SC1128)
🪛 yamllint
recipes/fc/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (4)
recipes/fc/build.sh (1)
4-4: Verify build flags and add error handling
The make command looks correct for compiling the source, but consider:
- Adding error checking for the compiler variables
- Validating that PREFIX is set
- Adding quotes around variables to handle spaces
recipes/fc/meta.yaml (3)
9-11: LGTM: Source configuration follows best practices
The source URL is properly templated and includes a SHA256 checksum for verification.
31-36: LGTM: About section is complete
The about section includes all required fields and properly references the LICENSE file.
1-6:
Consider aligning package name with source name
The package name "fc" differs from the source name "fc-virus". This inconsistency might be related to the testing issue where "fc virus" couldn't be found. Consider using consistent naming or document the reason for the difference.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
recipes/fc/build.sh
Outdated
|
|
||
| #!/bin/bash | ||
| set -xe |
There was a problem hiding this comment.
Fix shebang line position
The shebang must be on the first line of the script. Remove the blank line 1.
-
#!/bin/bash
+#!/bin/bash
set -xe📝 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.
| #!/bin/bash | |
| set -xe | |
| #!/bin/bash | |
| set -xe |
🧰 Tools
🪛 Shellcheck
[error] 2-2: The shebang must be on the first line. Delete blanks and move comments.
(SC1128)
| #chmod +x ./bin/fc-virus | ||
| chmod 777 ./bin/fc-virus |
There was a problem hiding this comment.
Security: Fix overly permissive file permissions
Using chmod 777 grants read/write/execute permissions to all users, which is a security risk. The executable only needs to be executable by users (755).
-#chmod +x ./bin/fc-virus
-chmod 777 ./bin/fc-virus
+chmod 755 ./bin/fc-virus📝 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.
| #chmod +x ./bin/fc-virus | |
| chmod 777 ./bin/fc-virus | |
| chmod 755 ./bin/fc-virus |
| mkdir -p ${PREFIX}/bin | ||
| cp -f ./bin/fc-virus ${PREFIX}/bin |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for installation steps
The installation commands should:
- Validate that the binary exists before copying
- Use install command instead of cp for proper permission handling
- Add error handling for mkdir and copy operations
-mkdir -p ${PREFIX}/bin
-cp -f ./bin/fc-virus ${PREFIX}/bin
+if [ ! -f ./bin/fc-virus ]; then
+ echo "Error: fc-virus binary not found" >&2
+ exit 1
+fi
+mkdir -p "${PREFIX}/bin" || exit 1
+install -v -m 755 ./bin/fc-virus "${PREFIX}/bin/" || 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.
| mkdir -p ${PREFIX}/bin | |
| cp -f ./bin/fc-virus ${PREFIX}/bin | |
| if [ ! -f ./bin/fc-virus ]; then | |
| echo "Error: fc-virus binary not found" >&2 | |
| exit 1 | |
| fi | |
| mkdir -p "${PREFIX}/bin" || exit 1 | |
| install -v -m 755 ./bin/fc-virus "${PREFIX}/bin/" || exit 1 |
recipes/fc/meta.yaml
Outdated
| requirements: | ||
| build: | ||
| - make | ||
| - {{ compiler('c') }} | ||
| host: | ||
| - zlib | ||
| - boost |
There was a problem hiding this comment.
Add C++ compiler requirement
Since the package depends on boost, which is a C++ library, you should add the C++ compiler to the build requirements.
build:
- make
- {{ compiler('c') }}
+ - {{ compiler('cxx') }}📝 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.
| requirements: | |
| build: | |
| - make | |
| - {{ compiler('c') }} | |
| host: | |
| - zlib | |
| - boost | |
| requirements: | |
| build: | |
| - make | |
| - {{ compiler('c') }} | |
| - {{ compiler('cxx') }} | |
| host: | |
| - zlib | |
| - boost |
recipes/fc/meta.yaml
Outdated
| build: | ||
| number: 0 | ||
| skip: true | ||
| run_exports: | ||
| - {{ pin_subpackage("fc", max_pin="x") }} |
There was a problem hiding this comment.
Remove redundant skip directive
The skip: true on line 14 would prevent building for all platforms, which conflicts with the platform-specific skip at the end of the file. Remove this line and keep only the platform-specific skip.
build:
number: 0
- skip: true
run_exports:
- {{ pin_subpackage("fc", max_pin="x") }}📝 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.
| build: | |
| number: 0 | |
| skip: true | |
| run_exports: | |
| - {{ pin_subpackage("fc", max_pin="x") }} | |
| build: | |
| number: 0 | |
| run_exports: | |
| - {{ pin_subpackage("fc", max_pin="x") }} |
recipes/fc/meta.yaml
Outdated
| skip: | ||
| - osx-64 |
There was a problem hiding this comment.
Move skip configuration to build section
The skip configuration should be under the build section, not at the root level.
build:
number: 0
run_exports:
- {{ pin_subpackage("fc", max_pin="x") }}
+ skip:
+ - osx-64
-skip:
- - osx-64📝 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.
| skip: | |
| - osx-64 | |
| build: | |
| number: 0 | |
| run_exports: | |
| - {{ pin_subpackage("fc", max_pin="x") }} | |
| skip: | |
| - osx-64 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
recipes/fc/meta.yaml (2)
1-6: Consider aligning package name with binary nameThe package is named "fc" while the binary and source repository are named "fc-virus". This inconsistency might cause confusion. Consider using "fc-virus" as the package name for better clarity and consistency.
package: - name: "fc" + name: "fc-virus" version: {{ version }}🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
31-36: Consider adding optional metadata fieldsThe about section could be enhanced with additional metadata fields:
description: A longer description of the packagedev_url: Development URL (same as home)doc_url: Documentation URL if availableabout: home: "https://github.com/qdu-bioinfo/fc-virus" license: MIT license_family: MIT license_file: LICENSE summary: "Accurate Assembly of Full-length Consensus for Viral Quasispecies" + description: | + A detailed description of fc-virus and its capabilities + dev_url: "https://github.com/qdu-bioinfo/fc-virus" + doc_url: "https://github.com/qdu-bioinfo/fc-virus#readme"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/fc/meta.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/fc/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (2)
recipes/fc/meta.yaml (2)
9-11: LGTM!
The source configuration is well-structured with proper templating and includes the SHA256 checksum.
18-24: Add missing dependencies
Based on the build script, please ensure all necessary dependencies are listed:
- Add
automakeandautoconfif the build process requires them - Add
libtoolif shared libraries are being built
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
recipes/fc/build.sh(1 hunks)recipes/fc/meta.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- recipes/fc/build.sh
🧰 Additional context used
🪛 yamllint
recipes/fc/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (6)
recipes/fc/meta.yaml (6)
1-6: LGTM!
The package metadata is properly defined using Jinja2 templating.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
29-35: LGTM!
The package information is well-documented with appropriate license and links.
17-23:
Add missing C++ compiler requirement
Since the package depends on boost-cpp, you need to add the C++ compiler to the build requirements.
This issue was previously raised and still needs to be addressed. Apply this diff:
build:
- make
- {{ compiler('c') }}
+ - {{ compiler('cxx') }}25-27:
Enhance test coverage
Given the reported issue about "fc virus not found in test_tmp", more comprehensive tests are needed.
This issue was previously raised and still needs to be addressed. Apply this diff:
test:
commands:
- - fc-virus --help
+ - which fc-virus
+ - test -f $PREFIX/bin/fc-virus
+ - fc-virus --help
+ - fc-virus --version # Add if supported8-10: Verify source checksum
The source URL and checksum format are correct. Let's verify the checksum matches the source.
37-40: Review platform compatibility
The inclusion of osx-arm64 in additional-platforms seems inconsistent with the need to skip osx-64. Please verify if the package is actually compatible with macOS platforms.
recipes/fc/meta.yaml
Outdated
| build: | ||
| number: 0 | ||
| run_exports: | ||
| - {{ pin_subpackage("fc", max_pin="x") }} |
There was a problem hiding this comment.
Add platform skip configuration
The build section should include platform skip configuration for osx-64 as mentioned in the AI summary.
Apply this diff:
build:
number: 0
run_exports:
- {{ pin_subpackage("fc", max_pin="x") }}
+ skip:
+ - osx-64📝 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.
| build: | |
| number: 0 | |
| run_exports: | |
| - {{ pin_subpackage("fc", max_pin="x") }} | |
| build: | |
| number: 0 | |
| run_exports: | |
| - {{ pin_subpackage("fc", max_pin="x") }} | |
| skip: | |
| - osx-64 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/fc/meta.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/fc/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (6)
recipes/fc/meta.yaml (6)
1-6: LGTM: Package metadata is well-structured.
The package name and version are properly defined using Jinja2 variables, following Bioconda best practices.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
12-15: LGTM: Build configuration is appropriate.
The build number and run_exports configuration are correctly set up. Note that the skip configuration has been addressed in previous comments.
17-24: LGTM: Dependencies are well-defined.
The build and host requirements are appropriate for a C++ package using boost and zlib.
30-36: LGTM: Package metadata is complete.
The about section includes all required fields with appropriate information about the package.
38-41: Verify platform compatibility.
Please ensure that the package can be built and runs correctly on the additional platforms (linux-aarch64 and osx-arm64).
8-10: Verify source accessibility and checksum.
The source URL and checksum look correct, but let's verify:
recipes/fc/meta.yaml
Outdated
| test: | ||
| commands: | ||
| - ./bin/fc-virus --help |
There was a problem hiding this comment.
Fix test command path.
The current test command uses a relative path ./bin/fc-virus which may not work as expected. The binary should be in $PREFIX/bin after installation.
Apply this fix:
test:
commands:
- - ./bin/fc-virus --help
+ - fc-virus --help📝 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.
| test: | |
| commands: | |
| - ./bin/fc-virus --help | |
| test: | |
| commands: | |
| - fc-virus --help |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
recipes/fc/meta.yaml (1)
28-30: Consider adding more comprehensive tests.While the basic help command test is good, consider adding more test commands to verify key functionality. This could help prevent the testing issues mentioned in the PR comments.
test: commands: - fc-virus --help + - fc-virus --version + - command -v fc-virus
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/fc/meta.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/fc/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (6)
recipes/fc/meta.yaml (6)
1-6: LGTM: Package metadata is well-structured.
The package name and version are properly defined using Jinja2 variables.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
19-26: LGTM: Requirements are properly specified.
The build and host requirements are appropriate for a C++ project using boost and zlib.
32-38: LGTM: Package metadata is complete.
The about section includes all required fields and properly references the MIT license.
8-10: Verify source accessibility and checksum.
The source URL and checksum need verification.
40-43: Verify platform compatibility.
Please confirm that the package can build successfully on the additional platforms, especially osx-arm64 given that osx-64 is skipped.
12-17: Verify the rationale for skipping osx-64.
Given the testing issues mentioned in the PR comments, please confirm if skipping osx-64 is intentional. If there are known compatibility issues, consider documenting them in the package description.
✅ Verification successful
Let me try to gather more information about the OS-specific configuration by checking the recipe files and build scripts.
From the output, I can see that the recipe skips both osx-64 and osx-arm64 platforms. Let me check the build script and any potential build issues to understand the rationale.
Let me check the upstream repository to see if there are any documented OS-specific issues or build requirements.
Skipping osx-64 is appropriate - package explicitly requires Linux OS
The upstream documentation explicitly states that the tool is designed for Linux operating systems:
- Requirements section clearly specifies "Operating System: Linux"
- The package is built with Linux-specific compilation and installation instructions
- The extra section in meta.yaml indicates support for linux-aarch64 and osx-arm64, but the current skip is appropriate for osx-64 given the Linux requirement
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any documented OS-specific issues
# Expected: Find references to OS compatibility issues
# Test: Search for OS-related issues in repository documentation
rg -i "osx|darwin|macos" "$(fd -t f -e md)"
Length of output: 247
Script:
#!/bin/bash
# Check recipe files for OS-specific configurations and build requirements
rg -i "osx|darwin|macos|skip" recipes/fc/
# Check if there are any OS-specific build scripts or patches
fd . recipes/fc/ -e sh -e patch
# Check build.sh for any OS-specific logic
cat recipes/fc/build.sh
# Look for similar patterns in other fc-related recipes
fd . recipes/ -d 1 -e yaml | xargs rg -l "fc"
Length of output: 71532
Script:
#!/bin/bash
# Check for any build failures or issues on OSX
rg -i "fc.*build.*fail" build-fail-blacklist
# Check build.sh for any OS-specific code or dependencies
cat recipes/fc/build.sh
# Look for any compiler/build dependencies that might explain platform skipping
cat recipes/fc/meta.yaml
Length of output: 1187
Script:
#!/bin/bash
# Check the repository's README and documentation for OS-specific information
curl -sL "https://raw.githubusercontent.com/qdu-bioinfo/fc-virus/v1.0.1/README.md" | cat
# Check for any OS-specific code or compiler directives in the source
curl -sL "https://raw.githubusercontent.com/qdu-bioinfo/fc-virus/v1.0.1/Makefile" | cat
# Look for any open issues or discussions about OSX compatibility
gh issue list -R qdu-bioinfo/fc-virus
Length of output: 1698
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
recipes/fc/meta.yaml (2)
1-6: Consider using a more specific package nameThe package name "fc" is quite generic and could potentially conflict with other packages. Consider using "fc-virus" as the package name to match the binary name and GitHub repository.
-{% set name = "fc" %} +{% set name = "fc-virus" %}🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
26-28: Enhance test coverageThe current test only verifies that the binary can be executed. Consider adding more test commands to verify key functionality:
- Check version number
- Test with sample input files if available
test: commands: - fc-virus --help + - fc-virus --version # or equivalent version flag + # Add more functional tests if possible
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/fc/meta.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/fc/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (4)
recipes/fc/meta.yaml (4)
30-36: LGTM!
The about section is well-structured and contains all necessary information including license details and a clear description of the package's purpose.
17-24: Consider adding runtime requirements
The package has host requirements for zlib and boost-cpp, but no runtime requirements. If these libraries are dynamically linked, they should be added to the run section.
#!/bin/bash
# Description: Check for dynamic library dependencies
# Search for dynamic linking related code
rg -A 5 "dlopen|LoadLibrary"12-15: Verify the need for run_exports
The run_exports section is typically used for compiled libraries that other packages might link against. If this package only provides a binary executable without any shared libraries, this section might not be necessary.
#!/bin/bash
# Description: Check if the package provides any shared libraries
# Search for library-related files in the source
rg -l "\.so|\.dylib|\.dll" 8-10: Verify source checksum
The URL and checksum format look correct. Let's verify the checksum matches the source.
|
@mencian |
Describe your pull request here
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>.