pgrc: add linux-aarch64 build #51009
Conversation
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
|
Warning Rate limit exceeded@mencian has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 51 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📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
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: 0
🧹 Outside diff range and nitpick comments (1)
recipes/pgrc/meta.yaml (1)
36-37: Approve addition of linux-aarch64, but suggest build section modification.The addition of linux-aarch64 as an additional platform aligns with the PR objectives and is correctly placed in the
extrasection. However, to fully enable building for this platform, a modification to thebuildsection is necessary.Consider modifying the
buildsection to remove the skip directive for non-OSX platforms. Apply this diff to thebuildsection:build: - skip: True # [osx] + skip: True # [osx or win] number: 1 run_exports: - {{ pin_subpackage('pgrc', max_pin="x") }}This change will allow building for linux-aarch64 while still skipping OSX and Windows builds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- recipes/pgrc/meta.yaml (2 hunks)
🔇 Additional comments (2)
recipes/pgrc/meta.yaml (2)
10-10: LGTM: Build number increment is appropriate.The build number has been correctly incremented from 0 to 1. This is the expected practice when making changes to a package without altering its version, which aligns with the addition of a new platform support.
Line range hint
1-38: LGTM: Overall changes and structure are appropriate.The modifications to this Conda recipe align well with the PR objectives of adding a linux-aarch64 build. The build number has been correctly incremented, and the new platform has been added to the
extrasection. The overall structure and content of the recipe follow Conda packaging best practices.Once the suggested modification to the
buildsection is implemented, this recipe should successfully enable builds for the linux-aarch64 platform while maintaining the existing functionality for other supported platforms.
|
@BiocondaBot please fetch artifacts |
|
Package(s) built are ready for inspection:
Docker image(s) built:
|
LGTM! |
|
@BiocondaBot please add label |
|
@martin-g mind if I hop in here and try to get this package to build on |
|
@mencian Sure! Go ahead! |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
recipes/pgrc/meta.yaml (1)
22-27: LGTM: Improved cross-platform compatibility.The addition of
llvm-openmpfor macOS andlibgompfor Linux in both host and run requirements improves cross-platform compatibility. This change supports the PR objective of adding a linux-aarch64 build and potentially improves macOS compatibility.Consider consolidating the repeated entries for host and run requirements to reduce redundancy:
requirements: host: - {{ compiler('cxx') }} - cmake >=3.5 - make - llvm-openmp # [osx] - libgomp # [linux] run: - {{ pin_compatible('llvm-openmp', max_pin='x') }} # [osx] - {{ pin_compatible('libgomp', max_pin='x') }} # [linux]This approach uses Conda's pinning expressions to ensure compatibility between build and runtime versions.
recipes/pgrc/build.sh (1)
5-6: Remove unusedINCLUDESandLIBPATHvariables.The variables
INCLUDESandLIBPATHare defined but not used later in the script. Removing them will simplify the script and avoid confusion.Apply this diff to remove the unused variables:
-export INCLUDES="-I${PREFIX}/include" -export LIBPATH="-L${PREFIX}/lib"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/pgrc/build.sh (1 hunks)
- recipes/pgrc/meta.yaml (2 hunks)
🔇 Additional comments (5)
recipes/pgrc/meta.yaml (5)
9-9: LGTM: Build number increment is correct.Incrementing the build number from 0 to 1 is appropriate when making changes to the package without changing the version. This aligns with the PR objective of adding a linux-aarch64 build.
35-35: LGTM: Improved metadata in the about section.The changes in the about section enhance the package metadata:
- The license field now uses quotes, which is a good practice for string values in YAML.
- Adding a period at the end of the summary improves readability.
- The new
dev_urlfield provides a direct link to the project's GitHub repository, which is valuable for users and contributors.These improvements align with best practices for Conda package metadata.
Also applies to: 38-39
47-48: LGTM: Added DOI for improved academic referencing.The addition of the
identifierssection with a DOI (Digital Object Identifier) is a valuable improvement:
- It enables proper academic citation of the software.
- It enhances the package's discoverability in academic contexts.
- It provides a persistent link to the research associated with this software.
This change aligns with best practices for scientific software packaging.
42-44: LGTM: Extended platform support.The addition of
linux-aarch64andosx-arm64to theadditional-platformslist is a positive change:
- It aligns with the PR objective of adding a linux-aarch64 build.
- The inclusion of osx-arm64 suggests future support for Apple Silicon, which is a forward-thinking addition.
These changes broaden the package's compatibility across different architectures, enhancing its usability.
To ensure the build process is correctly set up for these new platforms, please run the following script:
This script will help identify any platform-specific configurations or patches that might be necessary for the new architectures.
Line range hint
1-48: Overall LGTM: Comprehensive improvements to the pgrc package recipe.This PR makes several valuable enhancements to the pgrc package recipe:
- Incremented build number, reflecting the changes made.
- Improved cross-platform compatibility by adding appropriate OpenMP libraries for different operating systems.
- Extended platform support to include linux-aarch64 and osx-arm64.
- Enhanced metadata in the about section, including a dev_url and improved formatting.
- Added a DOI for better academic referencing.
These changes collectively improve the package's usability, compatibility, and discoverability. They align well with the PR objectives and Bioconda guidelines.
To ensure all changes are properly reflected in the build process, please run the following script:
This script will help ensure that all aspects of the recipe are consistent with the changes made and follow best practices.
recipes/pgrc/build.sh
Outdated
| export LDFLAGS="$LDFLAGS -L$BUILD_PREFIX/lib" | ||
| cmake .. | ||
| make -j ${CPU_COUNT} PgRC | ||
| export INCLUDES="-I{PREFIX}/include" |
There was a problem hiding this comment.
Fix typo in PREFIX variable expansion in INCLUDES.
In line 5, {PREFIX} should be ${PREFIX} to properly expand the variable. This ensures the include path is set correctly.
Apply this diff to fix the typo:
-export INCLUDES="-I{PREFIX}/include"
+export INCLUDES="-I${PREFIX}/include"📝 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.
| export INCLUDES="-I{PREFIX}/include" | |
| export INCLUDES="-I${PREFIX}/include" |
recipes/pgrc/build.sh
Outdated
| make -j ${CPU_COUNT} PgRC | ||
| export INCLUDES="-I{PREFIX}/include" | ||
| export LIBPATH="-L${PREFIX}/lib" | ||
| export CXXFLAGS="${CXXFLAGS} -O3 -I${PREFIX}/include" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding optimization flags in CXXFLAGS.
Appending -O3 to CXXFLAGS may override optimization levels set by the build environment. It's better to respect the environment's optimization settings to ensure consistent builds across different systems.
Apply this diff to adjust CXXFLAGS:
-export CXXFLAGS="${CXXFLAGS} -O3 -I${PREFIX}/include"
+export CXXFLAGS="${CXXFLAGS} -I${PREFIX}/include"📝 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.
| export CXXFLAGS="${CXXFLAGS} -O3 -I${PREFIX}/include" | |
| export CXXFLAGS="${CXXFLAGS} -I${PREFIX}/include" |
| cmake -S . -B build -DCMAKE_INSTALL_PREFIX="${PREFIX}" \ | ||
| -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_COMPILER="${CXX}" \ | ||
| -DCMAKE_CXX_FLAGS="${CXXFLAGS}" \ | ||
| "${CONFIG_ARGS}" |
There was a problem hiding this comment.
Ensure CONFIG_ARGS is correctly passed to cmake.
Including CONFIG_ARGS within quotes may prevent proper argument parsing if it contains multiple flags. To ensure all arguments are correctly recognized, pass CONFIG_ARGS without quotes.
Apply this diff to adjust the cmake command:
cmake -S . -B build -DCMAKE_INSTALL_PREFIX="${PREFIX}" \
-DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_COMPILER="${CXX}" \
-DCMAKE_CXX_FLAGS="${CXXFLAGS}" \
- "${CONFIG_ARGS}"
+ ${CONFIG_ARGS}📝 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.
| cmake -S . -B build -DCMAKE_INSTALL_PREFIX="${PREFIX}" \ | |
| -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_COMPILER="${CXX}" \ | |
| -DCMAKE_CXX_FLAGS="${CXXFLAGS}" \ | |
| "${CONFIG_ARGS}" | |
| cmake -S . -B build -DCMAKE_INSTALL_PREFIX="${PREFIX}" \ | |
| -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_COMPILER="${CXX}" \ | |
| -DCMAKE_CXX_FLAGS="${CXXFLAGS}" \ | |
| ${CONFIG_ARGS} |
| if [[ `uname` == "Darwin" ]]; then | ||
| export CONFIG_ARGS="-DCMAKE_FIND_FRAMEWORK=NEVER -DCMAKE_FIND_APPBUNDLE=NEVER" | ||
| else | ||
| export CONFIG_ARGS="" | ||
| fi |
There was a problem hiding this comment.
💡 Codebase verification
Update CONFIG_ARGS to Include Cross-Compilation Flags for linux-aarch64
The current configuration sets CONFIG_ARGS to an empty string for non-Darwin systems, which may not accommodate necessary cross-compilation flags for the linux-aarch64 build.
- Recommendation: Modify the
elsebranch inrecipes/pgrc/build.shto include appropriate cross-compilation flags required forlinux-aarch64.
🔗 Analysis chain
Verify that CONFIG_ARGS is set appropriately for cross-compilation.
Since you're adding a linux-aarch64 build, ensure that CONFIG_ARGS handles cross-compilation scenarios correctly. The current script may need adjustments to accommodate cross-compiling to different architectures.
Run the following script to check for cross-compilation flags in CONFIG_ARGS:
Investigate CONFIG_ARGS Configuration for Cross-Compilation
The current execution shows that CONFIG_ARGS is not set. Please verify where and how CONFIG_ARGS is defined and ensure it includes the necessary cross-compilation flags for the linux-aarch64 build.
Further Investigate CONFIG_ARGS Assignments
The previous search did not locate any assignments to CONFIG_ARGS. To ensure all instances are found, please run the following script to search for CONFIG_ARGS assignments across the codebase.
Extract CONFIG_ARGS Assignments in recipes/pgrc/build.sh
Please run the following script to retrieve all instances where CONFIG_ARGS is set in recipes/pgrc/build.sh. This will help verify if the necessary cross-compilation flags for linux-aarch64 are included.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CONFIG_ARGS for cross-compilation settings.
# Test: Search for cross-compilation flags in CONFIG_ARGS. Expect: Appropriate flags set.
echo "CONFIG_ARGS is: ${CONFIG_ARGS}"
# If targeting a different architecture, CONFIG_ARGS may need additional flags.
Length of output: 42
Script:
#!/bin/bash
# Description: Search for all assignments and modifications of CONFIG_ARGS in the codebase.
fd "CONFIG_ARGS" . --type f | xargs grep -H "CONFIG_ARGS"
Length of output: 55
Script:
#!/bin/bash
# Description: Recursively search for assignments and modifications of CONFIG_ARGS in the codebase.
rg "CONFIG_ARGS\s*=" --files-with-matches
Length of output: 427
Script:
#!/bin/bash
# Description: Extract all assignments to CONFIG_ARGS in recipes/pgrc/build.sh
grep "CONFIG_ARGS\s*=" recipes/pgrc/build.sh
Length of output: 149
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>.Summary by CodeRabbit
linux-aarch64andosx-arm64platforms.dev_urllinking to the project's GitHub repository.