Linux-aarch64 build for Bioconductor bicare #51298
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several changes related to the Bioconductor Affycompatible package. A new shell script named Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
recipes/bioconductor-affycompatible/build.sh (4)
1-3: LGTM! Consider adding error handling.The shebang and DESCRIPTION file manipulation look good. The removal of the 'Priority:' line is likely necessary for Bioconda compatibility.
Consider adding error handling for the file operations. For example:
mv DESCRIPTION DESCRIPTION.old || exit 1 grep -v '^Priority: ' DESCRIPTION.old > DESCRIPTION || exit 1This will ensure the script exits if these critical operations fail.
5-10: LGTM! Consider using a heredoc for better readability.The creation of the Makevars file with compiler settings looks good. Using environment variables ensures flexibility across different build environments.
For improved readability, consider using a heredoc:
cat << EOF > ~/.R/Makevars CC=$CC FC=$FC CXX=$CXX CXX98=$CXX CXX11=$CXX CXX14=$CXX EOFThis approach can make the script more maintainable, especially if more variables are added in the future.
11-11: LGTM! Consider adding error handling.The R package installation command is correct for building and installing the package.
Consider adding error handling to ensure the script exits if the installation fails:
$R CMD INSTALL --build . || exit 1This will make it easier to catch and debug installation issues in the build process.
1-11: Overall, the build script looks good with room for minor improvements.The script correctly handles the necessary steps for building and installing the Bioconductor package in a Bioconda environment. It appropriately modifies the DESCRIPTION file, sets up compiler variables, and runs the R installation command.
To enhance robustness and maintainability:
- Consider adding error handling for critical operations.
- Use a heredoc for the Makevars file creation for better readability.
- Ensure consistent error handling throughout the script.
These improvements will make the build process more reliable and easier to maintain in the long run.
recipes/bioconductor-affycompatible/meta.yaml (2)
20-30: LGTM: Dependencies are well-defined with a minor optimization possibleThe requirements section correctly specifies both host and run dependencies with appropriate version constraints where necessary.
Consider using YAML anchors and aliases to reduce duplication:
requirements: host: &r-deps - bioconductor-biostrings - r-base - 'r-rcurl >=0.8-1' - 'r-xml >=2.8-1' run: *r-depsThis approach maintains the same functionality while improving maintainability.
39-47: LGTM: Helpful extra information provided with a minor formatting issueThe extra section includes valuable additional identifiers and parent recipe information, which is excellent for maintainability and cross-referencing.
Remove the extra blank line at the end of the file to address the YAML linter warning:
path: recipes/bioconductor-affycompatible version: 1.40.0 -🧰 Tools
🪛 yamllint
[warning] 47-47: too many blank lines
(1 > 0) (empty-lines)
recipes/bioconductor-gofuncr/meta.yaml (2)
56-58: Consider adding a comment for clarityWhile the addition of the 'extra' section is correct, it would be helpful to add a comment explaining its purpose. This can improve maintainability and make it easier for other contributors to understand the configuration.
Consider adding a comment like this:
# Specify additional platform support extra: additional-platforms: - linux-aarch64
Bioconductor version is not up-to-date
The recipe
bioconductor-gofuncr/meta.yamlis using Bioconductor version3.18, while the latest available version is3.19. Please update the recipe to use the latest Bioconductor version to maintain compatibility and include any potential bug fixes or improvements.🔗 Analysis chain
Line range hint
1-58: Reminder: Keep dependencies up-to-dateThe package uses pinned versions for its Bioconductor dependencies, which is good for reproducibility. However, please ensure these are kept up-to-date with future Bioconductor releases to maintain compatibility and include any potential bug fixes or improvements.
To help with this, you can run the following script periodically to check for new Bioconductor releases:
To accurately verify the Bioconductor version used in the recipe, please run the following script:
Please run the following revised script to accurately verify the Bioconductor version used in the recipe:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for new Bioconductor releases # Test: Fetch the latest Bioconductor version from the Bioconductor website latest_bioc=$(curl -s https://bioconductor.org/config.yaml | grep release_version | awk '{print $2}') # Compare with the version in the recipe recipe_bioc=$(grep 'set bioc' recipes/bioconductor-gofuncr/meta.yaml | awk '{print $4}' | tr -d '"') echo "Latest Bioconductor version: $latest_bioc" echo "Recipe Bioconductor version: $recipe_bioc" if [ "$latest_bioc" != "$recipe_bioc" ]; then echo "Warning: Recipe is not using the latest Bioconductor version" else echo "Recipe is using the latest Bioconductor version" fiLength of output: 530
Script:
#!/bin/bash # Description: Verify Bioconductor version in the recipe # Fetch the latest Bioconductor version from the Bioconductor website latest_bioc=$(curl -s https://bioconductor.org/config.yaml | grep release_version | awk '{print $2}' | tr -d '"') # Extract the Bioconductor version from the recipe recipe_bioc=$(grep '^set bioc' recipes/bioconductor-gofuncr/meta.yaml | awk -F'"' '{print $2}') echo "Latest Bioconductor version: $latest_bioc" echo "Recipe Bioconductor version: $recipe_bioc" if [ "$latest_bioc" != "$recipe_bioc" ]; then echo "Warning: Recipe is not using the latest Bioconductor version" else echo "Recipe is using the latest Bioconductor version" fiLength of output: 523
Script:
#!/bin/bash # Description: Verify Bioconductor version in the recipe # Fetch the latest Bioconductor version from the Bioconductor website latest_bioc=$(curl -s https://bioconductor.org/config.yaml | grep release_version | awk '{print $2}' | tr -d '"') # Extract the Bioconductor version from the recipe, accounting for leading characters recipe_bioc=$(grep 'set bioc' recipes/bioconductor-gofuncr/meta.yaml | awk -F'"' '{print $2}') echo "Latest Bioconductor version: $latest_bioc" echo "Recipe Bioconductor version: $recipe_bioc" if [ "$latest_bioc" != "$recipe_bioc" ]; then echo "Warning: Recipe is not using the latest Bioconductor version" else echo "Recipe is using the latest Bioconductor version" fiLength of output: 536
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- recipes/bioconductor-affycompatible/build.sh (1 hunks)
- recipes/bioconductor-affycompatible/conda_build_config.yaml (1 hunks)
- recipes/bioconductor-affycompatible/meta.yaml (1 hunks)
- recipes/bioconductor-awfisher/meta.yaml (1 hunks)
- recipes/bioconductor-bacon/meta.yaml (1 hunks)
- recipes/bioconductor-bags/meta.yaml (1 hunks)
- recipes/bioconductor-bambu/meta.yaml (1 hunks)
- recipes/bioconductor-baynorm/meta.yaml (1 hunks)
- recipes/bioconductor-beachmat.hdf5/meta.yaml (1 hunks)
- recipes/bioconductor-bicare/meta.yaml (1 hunks)
- recipes/bioconductor-gofuncr/meta.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/bioconductor-affycompatible/meta.yaml
[warning] 47-47: too many blank lines
(1 > 0) (empty-lines)
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (20)
recipes/bioconductor-affycompatible/build.sh (1)
4-4: LGTM! Directory creation is correct.The creation of the ~/.R directory using
mkdir -pis a good practice. It ensures the directory exists without causing errors if it's already present.recipes/bioconductor-bags/meta.yaml (1)
43-45: LGTM! Verify consistency across other package recipes.The addition of the
extrasection withadditional-platformsspecifyinglinux-aarch64is appropriate and aligns with the PR objective of implementing a build for the Linux ARM64 architecture. This change enhances the package's compatibility without affecting the existing configuration.To ensure consistency across other Bioconductor package recipes, let's verify if similar changes have been made:
This script will help us confirm that the change has been consistently applied across other Bioconductor package recipes, ensuring a uniform approach to extending ARM64 support.
recipes/bioconductor-awfisher/meta.yaml (1)
46-48: LGTM: Addition of linux-aarch64 platform supportThe addition of the
extrasection withadditional-platforms: - linux-aarch64is correct and aligns with the PR objectives. This change extends the package's compatibility to the Linux ARM64 architecture, which is the main goal of this pull request.To ensure consistency across other package recipes, let's verify if similar changes have been made to other Bioconductor packages:
recipes/bioconductor-affycompatible/meta.yaml (4)
1-7: LGTM: Package metadata correctly definedThe package name and version are correctly defined using Jinja2 templating. The use of variables and filters (e.g.,
{{ name|lower }}) ensures consistency and ease of maintenance.🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
14-19: LGTM: Build configuration is correctThe build section is well-configured:
- Build number is correctly set to 0 for the initial release.
- RPaths are properly specified for R libraries.
- The
noarch: genericflag is appropriate for this R package.
34-38: LGTM: Comprehensive package metadata providedThe about section provides thorough metadata for the package:
- The home page URL is correctly templated.
- The license (Artistic-2.0) is specified.
- A concise summary and detailed description are included, clearly explaining the package's purpose and functionality.
1-1: Note on YAML syntax warningThe static analysis tool flagged a syntax error for the '%' character on this line. This is a false positive as the '%' is part of the Jinja2 templating syntax, which is valid and commonly used in conda-forge recipe files. No action is required.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
recipes/bioconductor-bicare/meta.yaml (2)
Line range hint
1-58: Confirmation: No additional changes requiredI've reviewed the entire file, and no additional changes appear to be necessary for adding linux-aarch64 support. The existing package metadata, build information, and dependencies are correctly specified and don't require any architecture-specific adjustments.
57-58: LGTM: Addition of linux-aarch64 platform supportThe addition of
linux-aarch64to theadditional-platformslist is consistent with the PR objective of implementing a Linux-aarch64 build for the Bioconductor bicare package. This change correctly specifies that the package now supports the ARM64 architecture on Linux.To ensure this change is consistent across other related packages, let's run the following script:
✅ Verification successful
Verified: Consistent
linux-aarch64support across Bioconductor packagesThe addition of
linux-aarch64to theadditional-platformsinrecipes/bioconductor-bicare/meta.yamlaligns with other Bioconductor packages that already support this platform.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if other Bioconductor packages have similar linux-aarch64 support # Test: Search for other meta.yaml files with linux-aarch64 support rg --type yaml -g 'recipes/bioconductor-*/meta.yaml' 'additional-platforms:.*linux-aarch64'Length of output: 93
Script:
#!/bin/bash # Description: Verify the presence of 'additional-platforms' in other Bioconductor packages and specifically check for 'linux-aarch64' support. # Step 1: Check for existence of 'additional-platforms' key echo "Checking for 'additional-platforms' key in other meta.yaml files:" rg --type yaml -g 'recipes/bioconductor-*/meta.yaml' 'additional-platforms:' # Step 2: If 'additional-platforms' exists, check for 'linux-aarch64' support echo "Checking for 'linux-aarch64' support in 'additional-platforms':" rg --type yaml -g 'recipes/bioconductor-*/meta.yaml' 'linux-aarch64'Length of output: 1415
recipes/bioconductor-bacon/meta.yaml (1)
54-55: LGTM: Addition of Linux ARM64 supportThe addition of 'linux-aarch64' to the 'additional-platforms' list extends support for the Bioconductor bacon package to the Linux ARM64 architecture. This change aligns with the PR objective of implementing a build for Linux-aarch64.
To ensure consistency across the repository, let's verify if similar changes have been made to other Bioconductor package recipes:
This script will help us understand if the change is consistent across other Bioconductor package recipes and identify any recipes that might need similar updates.
✅ Verification successful
Confirmed: linux-aarch64 support addition is unique to bioconductor-bacon
The addition of 'linux-aarch64' to the 'additional-platforms' list in the bioconductor-bacon recipe appears to be a unique change. Our analysis of all Bioconductor recipes in the repository shows that no other recipes currently include this specific platform support.
This change aligns with the PR objective of implementing a build for Linux-aarch64 and may be part of a broader initiative to extend support for this architecture across Bioconductor packages. As this is potentially the first of such changes, it's advisable to:
- Monitor other Bioconductor recipes for similar updates in the future.
- Consider if this change should be propagated to other compatible Bioconductor packages.
- Ensure that the necessary build and test infrastructure is in place to support linux-aarch64 for Bioconductor packages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for 'additional-platforms: - linux-aarch64' in other Bioconductor package recipes # Test: Count occurrences of the new platform configuration echo "Number of Bioconductor recipes with linux-aarch64 support:" rg --type yaml -c 'additional-platforms:\s*- linux-aarch64' recipes/bioconductor-* # Test: List Bioconductor recipes without linux-aarch64 support echo "Bioconductor recipes without linux-aarch64 support:" fd -t f meta.yaml recipes/bioconductor- | xargs rg --type yaml -L 'additional-platforms:\s*- linux-aarch64'Length of output: 121230
Script:
#!/bin/bash # Description: Check for 'additional-platforms: - linux-aarch64' in other Bioconductor package recipes # Test: Count occurrences of the new platform configuration echo "Number of Bioconductor recipes with linux-aarch64 support:" rg --type yaml -c 'additional-platforms:\s*- linux-aarch64' recipes/bioconductor-* # Test: List Bioconductor recipes without linux-aarch64 support echo "Bioconductor recipes without linux-aarch64 support:" fd -t f meta.yaml recipes/bioconductor-* | xargs rg --type yaml -L 'additional-platforms:\s*- linux-aarch64'Length of output: 241809
Script:
#!/bin/bash # Description: Check for 'additional-platforms: - linux-aarch64' in Bioconductor package recipes # Set the base directory for recipes RECIPE_DIR="recipes" # Test: Count occurrences of the new platform configuration echo "Number of Bioconductor recipes with linux-aarch64 support:" find "$RECIPE_DIR" -name "meta.yaml" -type f | xargs grep -l 'additional-platforms:\s*- linux-aarch64' | wc -l # Test: List Bioconductor recipes without linux-aarch64 support echo "Bioconductor recipes without linux-aarch64 support:" find "$RECIPE_DIR" -name "meta.yaml" -type f | xargs grep -L 'additional-platforms:\s*- linux-aarch64' | sed 's|.*/||' | sort # Error handling if [ $? -ne 0 ]; then echo "Error: Failed to search for linux-aarch64 support in recipes." echo "Please check if the recipe directory structure is correct and the necessary files exist." fiLength of output: 93775
recipes/bioconductor-baynorm/meta.yaml (1)
70-72: LGTM: Addition of linux-aarch64 supportThe addition of the
extrasection withadditional-platformsspecifyinglinux-aarch64is correct and aligns with the PR objective of implementing a build for the Linux-aarch64 architecture. This change enhances the package's compatibility across different architectures without affecting other parts of the configuration.recipes/bioconductor-beachmat.hdf5/meta.yaml (1)
53-55: LGTM: Addition of linux-aarch64 supportThe addition of the
extrasection withadditional-platformsspecifyinglinux-aarch64is correct and aligns with the PR objective of implementing a build for the Linux-aarch64 architecture. This change enhances the package's compatibility across different architectures.recipes/bioconductor-gofuncr/meta.yaml (1)
56-58: LGTM: Addition of Linux ARM64 supportThe changes look good. Adding the 'extra' section with 'additional-platforms' specifying 'linux-aarch64' correctly indicates support for the Linux ARM64 architecture. This aligns with the PR objective of implementing a build for the Linux-aarch64 architecture.
recipes/bioconductor-bambu/meta.yaml (1)
76-78: LGTM: Addition of linux-aarch64 platform supportThe changes appropriately add support for the linux-aarch64 platform, which aligns with the PR objective of implementing a build for this architecture. This addition enhances the package's compatibility across different systems.
recipes/bioconductor-affycompatible/conda_build_config.yaml (6)
1-422: Overall assessment of conda_build_config.yamlThis configuration file provides a comprehensive set of build settings for the Bioconductor Affycompatible package. Here's a summary of the key points:
- The file is well-structured and covers a wide range of dependencies and build options.
- Some settings, like the MACOSX_DEPLOYMENT_TARGET, may need updating to ensure compatibility with modern systems.
- The choice of library versions is generally up-to-date, but some should be verified to ensure they are the latest stable releases.
- The target platform (linux-aarch64) is clearly specified, which is good for ensuring compatibility.
- The pin_run_as_build section provides good version control, but some libraries might benefit from more specific pinning.
Overall, the configuration appears solid, but I recommend addressing the points raised in the previous comments to ensure optimal performance, compatibility, and maintainability of the package.
1-5: Consider updating MACOSX_DEPLOYMENT_TARGET and verifying CONDA_BUILD_SYSROOT
- The
CONDA_BUILD_SYSROOTis set to an empty string. Please verify if this is intentional, as it might affect the build process.- The
MACOSX_DEPLOYMENT_TARGETis set to '10.9', which is quite outdated. Consider updating it to a more recent version to ensure compatibility with modern macOS systems.- Verbose output settings (
VERBOSE_ATandVERBOSE_CM) are enabled. While useful for debugging, they might slow down the build process. Consider disabling them for production builds.To check the impact of these settings, you can run:
#!/bin/bash # Check if CONDA_BUILD_SYSROOT is used elsewhere grep -r "CONDA_BUILD_SYSROOT" . # List files that might be affected by MACOSX_DEPLOYMENT_TARGET find . -type f -name "*.py" -o -name "*.sh" | xargs grep "MACOSX_DEPLOYMENT_TARGET"
411-422: Verify final configuration settings and target platform
The
zip_keysgrouping looks well-structured, linking related configuration options. This is good for maintaining consistency across interdependent settings.The
target_platformis set tolinux-aarch64. Ensure this aligns with the intended deployment environment and that all specified libraries and tools support this architecture.Verify that the final library versions are up-to-date and compatible with the linux-aarch64 platform:
- zlib: 1.2
- zlib_ng: 2.0
- zstd: 1.5
To check the compatibility of these libraries with linux-aarch64 and their latest versions, you can run:
#!/bin/bash # Check latest versions and aarch64 support echo "Latest zlib version:" curl -s https://zlib.net/ | grep -oP "zlib \K[0-9]+\.[0-9]+\.[0-9]+" | head -n 1 echo "Latest zlib-ng version:" curl -s https://github.com/zlib-ng/zlib-ng/releases | grep -oP "v\K[0-9]+\.[0-9]+\.[0-9]+" | head -n 1 echo "Latest zstd version:" curl -s https://github.com/facebook/zstd/releases | grep -oP "v\K[0-9]+\.[0-9]+\.[0-9]+" | head -n 1 echo "Checking aarch64 support in build scripts:" grep -r "aarch64" .
212-352: Review and update pin_run_as_build rulesThe
pin_run_as_buildsection provides comprehensive version pinning rules for dependencies. However, consider the following suggestions:
For critical libraries, consider using more specific version pinning. For example, instead of
x.xfor Python, you might want to usex.x.xto ensure more precise version control.Verify that the pinning rules align with the project's compatibility requirements, especially for key dependencies like Python, R, and commonly used libraries.
Consider adding pinning rules for any missing dependencies that are critical to the project's stability.
To check the current pinning rules against the actual versions used in the project, you can run:
#!/bin/bash # Check actual versions used in the project echo "Python version used:" grep -r "python_version" . echo "R version used:" grep -r "r_version" . # Add more grep commands for other critical libraries
6-52: Verify library versions and consider enabling CUDA support
Most library versions appear to be up-to-date. However, please verify that the following versions are the latest stable releases suitable for your project:
- boost: 1.74.0
- gdal: 3.5
- hdf5: 1.12.1
- numpy: 1.21.*
The CUDA compiler is set to None (line 51). If GPU acceleration is desired for this package, consider enabling CUDA support by specifying a version.
To check for the latest versions of these libraries, you can run:
#!/bin/bash # Check latest versions of key libraries echo "Latest Boost version:" curl -s https://www.boost.org/users/download/ | grep -oP "Version \K[0-9]+\.[0-9]+\.[0-9]+" | head -n 1 echo "Latest GDAL version:" curl -s https://github.com/OSGeo/gdal/releases | grep -oP "v\K[0-9]+\.[0-9]+\.[0-9]+" | head -n 1 echo "Latest HDF5 version:" curl -s https://www.hdfgroup.org/downloads/hdf5/ | grep -oP "HDF5-[0-9]+\.[0-9]+\.[0-9]+" | head -n 1 | sed 's/HDF5-//' echo "Latest NumPy version:" curl -s https://pypi.org/pypi/numpy/json | jq -r '.info.version'
106-108: Verify the necessity of ignoring Python and NumPy as build-only dependenciesThe configuration ignores Python and NumPy as build-only dependencies. This could potentially lead to issues if these libraries are actually required at runtime. Please verify that:
- The package doesn't require Python or NumPy at runtime.
- If they are required, ensure they are properly listed in the runtime dependencies elsewhere in the configuration.
To check if Python or NumPy are used in the package, you can run:
| source: | ||
| url: | ||
| - 'https://bioconductor.org/packages/{{ bioc }}/bioc/src/contrib/{{ name }}_{{ version }}.tar.gz' | ||
| - 'https://bioarchive.galaxyproject.org/{{ name }}_{{ version }}.tar.gz' | ||
| - 'https://depot.galaxyproject.org/software/bioconductor-{{ name|lower }}/bioconductor-{{ name|lower }}_{{ version }}_src_all.tar.gz' | ||
| md5: ba2f2f8dd8d5a678dc41d5fa86b322f8 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using SHA256 instead of MD5 for package integrity
While the inclusion of a checksum is excellent for verifying package integrity, consider using SHA256 instead of MD5. SHA256 provides stronger security and is less prone to collisions.
Replace the md5 field with sha256:
sha256: <insert_sha256_checksum_here>| test: | ||
| commands: | ||
| - '$R -e "library(''{{ name }}'')"' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more comprehensive tests
The current test ensures that the package can be loaded, which is a good basic check. However, consider adding more comprehensive tests to verify the package's functionality.
You could add additional tests like:
test:
commands:
- '$R -e "library(''{{ name }}'')"'
- '$R -e "stopifnot(packageVersion(''{{ name }}'') == ''{{ version }}'')"'
- '$R -e "example(''{{ name }}'')"'These additional tests would verify the package version and run examples (if available) to ensure core functionality works as expected.
| @@ -43,4 +43,6 @@ about: | |||
| summary: 'An R package for fast computing for adaptively weighted fisher''s method' | |||
| description: 'Implementation of the adaptively weighted fisher''s method, including fast p-value computing, variability index, and meta-pattern.' | |||
| license_file: '{{ environ["PREFIX"] }}/lib/R/share/licenses/GPL-3' | |||
|
|
|||
| extra: | |||
There was a problem hiding this comment.
Since you change the meta.yaml files you will need to also bump the build > number.
But I'd suggest to open a separate PR for those dependencies and leave only affycompatible here.
What is bicare in the PR title ?
There was a problem hiding this comment.
Why is this here? Recipes shouldn't override everything.
There was a problem hiding this comment.
I uploaded the wrong file. I deleted it
There was a problem hiding this comment.
Be sure to commit the removal. I'm still seeing this at the moment.
Test Passed!
