smoothxg: add linux-aarch64 build #51420
Conversation
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
martin-g
force-pushed
the
aarch64-smoothxg
branch
from
39b3ee9 to
d38f5e7
Compare
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes significant modifications to the 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/smoothxg/build.sh (1 hunks)
- recipes/smoothxg/meta.yaml (2 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/smoothxg/build.sh
[error] 14-14: Couldn't parse this case item. Fix to allow more checks.
(SC1073)
[error] 17-17: Fix any mentioned problems and try again.
(SC1072)
[error] 17-17: Did you forget the ;; after the previous case item?
(SC1074)
🔇 Additional comments (5)
recipes/smoothxg/meta.yaml (5)
16-16: Build number increment looks good.The build number has been correctly incremented from 0 to 1, which is appropriate when updating a package.
47-48: Additional platform support looks good.The addition of
linux-aarch64to theadditional-platformssection is in line with the PR objectives to add support for this architecture. This change should enable builds for ARM64-based Linux systems.Let's verify the additional platform configuration:
#!/bin/bash # Description: Verify the additional platforms configuration grep -A 2 'additional-platforms:' recipes/smoothxg/meta.yaml
Line range hint
1-48: Summary of review findings
- There's an inconsistency in the version number: The code shows 0.8.0, but the AI summary mentions 0.8.1.
- The build number increment, changes to host and run requirements, and the addition of linux-aarch64 support are all appropriate and align with the PR objectives.
Next steps:
- Please clarify and correct the version number discrepancy.
- Once the version issue is resolved, this PR looks good to merge, as it successfully adds linux-aarch64 build support for the smoothxg recipe.
Let's do a final check of the package metadata:
#!/bin/bash # Description: Verify key metadata in the meta.yaml file echo "Package version:" grep 'version = ' recipes/smoothxg/meta.yaml echo "Build number:" grep 'number:' recipes/smoothxg/meta.yaml echo "Additional platforms:" grep -A 2 'additional-platforms:' recipes/smoothxg/meta.yaml
27-28: Host requirements changes look appropriate.The movement of
llvm-openmpto host requirements for macOS and the addition oflibgompfor Linux platforms are good changes. These modifications should help address platform-specific compilation needs.Let's verify these changes across different platforms:
✅ Verification successful
To further verify the host requirements changes, let's search specifically for
llvm-openmpandlibgompin thehostsection of therecipes/smoothxg/meta.yamlfile:
Host requirements changes are verified.
The presence of
llvm-openmpfor macOS andlibgompfor Linux in the host requirements has been confirmed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the host requirements for different platforms echo "macOS host requirements:" grep -A 5 'host:' recipes/smoothxg/meta.yaml | grep 'osx' echo "Linux host requirements:" grep -A 5 'host:' recipes/smoothxg/meta.yaml | grep 'linux'Length of output: 388
Script:
#!/bin/bash # Description: Search for llvm-openmp and libgomp in host requirements echo "Searching for llvm-openmp in host requirements:" rg -A 5 'host:' recipes/smoothxg/meta.yaml | rg 'llvm-openmp' echo "Searching for libgomp in host requirements:" rg -A 5 'host:' recipes/smoothxg/meta.yaml | rg 'libgomp'Length of output: 368
34-34: Run requirements changes look appropriate.The addition of
llvm-openmpfor macOS andlibgompfor Linux platforms in the run requirements is a good change. This ensures that the necessary libraries are available at runtime for both platforms.Let's verify these changes across different platforms:
✅ Verification successful
Run requirements verified successfully.
The addition of
llvm-openmpfor macOS andlibgompfor Linux platforms in the run requirements has been confirmed. This ensures that the necessary libraries are available at runtime for both platforms.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the run requirements for different platforms echo "macOS run requirements:" grep -A 5 'run:' recipes/smoothxg/meta.yaml | grep 'osx' echo "Linux run requirements:" grep -A 5 'run:' recipes/smoothxg/meta.yaml | grep 'linux'Length of output: 388
Script:
#!/bin/bash # Description: Extract run requirements for macOS and Linux platforms with extended context echo "macOS run requirements:" grep -A 10 'run:' recipes/smoothxg/meta.yaml | grep 'osx' echo "Linux run requirements:" grep -A 10 'run:' recipes/smoothxg/meta.yaml | grep 'linux'Length of output: 278
recipes/smoothxg/build.sh
Outdated
| ARCH_FLAGS="" | ||
| sed -i 's/-msse4.1/-Ofast/g' deps/spoa/CMakeLists.txt | ||
| *) |
There was a problem hiding this comment.
Add missing ';;' after the first case block in the case statement
In the case statement starting on line 14, the first pattern matching aarch64 | arm64) is missing a terminating ;;. Each pattern in a case statement should be concluded with ;; to prevent syntax errors and ensure the script executes as intended.
Apply this diff to fix the issue:
case $(uname -m) in
aarch64 | arm64)
ARCH_FLAGS=""
sed -i 's/-msse4.1/-Ofast/g' deps/spoa/CMakeLists.txt
+ ;;
*)
ARCH_FLAGS="-march=sandybridge"
sed -i 's/-msse4.1/-march=sandybridge -Ofast/g' deps/spoa/CMakeLists.txt📝 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.
| ARCH_FLAGS="" | |
| sed -i 's/-msse4.1/-Ofast/g' deps/spoa/CMakeLists.txt | |
| *) | |
| ARCH_FLAGS="" | |
| sed -i 's/-msse4.1/-Ofast/g' deps/spoa/CMakeLists.txt | |
| ;; | |
| *) |
🧰 Tools
🪛 Shellcheck
[error] 17-17: Fix any mentioned problems and try again.
(SC1072)
[error] 17-17: Did you forget the ;; after the previous case item?
(SC1074)
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
martin-g
force-pushed
the
aarch64-smoothxg
branch
from
f68991a to
fa74bf8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
recipes/smoothxg/build.sh (2)
18-23: LGTM: Appropriate default configuration for other architecturesThe default case block is well-implemented:
- Using -march=sandybridge is a good default for x86_64 architectures, balancing performance and compatibility.
- Consistently replacing -msse4.1 and -march=native with -march=sandybridge -Ofast in both spoa and abPOA dependencies ensures uniform compiler optimizations.
Consider adding a comment explaining the choice of sandybridge as the default architecture target, e.g.:
# Use sandybridge as the default target for good performance and wide compatibility ARCH_FLAGS="-march=sandybridge"This would improve code maintainability by documenting the rationale behind this choice.
26-27: LGTM: Improved build configuration and performanceThe updated build commands are well-implemented:
- Using ARCH_FLAGS in the cmake command allows for architecture-specific optimizations.
- Adding parallel build support with -j"${CPU_COUNT}" can significantly speed up the build process.
- The use of --verbose is helpful for debugging.
Consider adding a fallback for CPU_COUNT in case it's not set:
cmake --build build --verbose -j"${CPU_COUNT:-1}"This ensures the build will proceed even if CPU_COUNT is not defined, defaulting to a single-threaded build in such cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- recipes/smoothxg/build.sh (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
recipes/smoothxg/build.sh (3)
13-14: LGTM: Architecture-specific build configuration introducedThe introduction of a case statement using
uname -mis a good approach for handling architecture-specific build configurations. This change allows for more flexible and targeted builds across different architectures.
14-17: LGTM: Appropriate handling for aarch64/arm64 architecturesThe case block for aarch64/arm64 architectures is well-implemented:
- Setting ARCH_FLAGS to an empty string is appropriate for these architectures.
- Replacing -msse4.1 with -Ofast in the spoa CMakeLists.txt is a good adaptation for ARM architectures.
- The missing ';;' highlighted in the previous review has been correctly added.
These changes should allow for proper compilation on ARM-based systems.
1-27: Summary: Well-implemented architecture-specific build improvementsOverall, the changes to the build script are well-implemented and align with the PR objectives:
- Architecture-specific optimizations have been introduced, allowing for proper builds on different platforms, including linux-aarch64.
- Build performance has been improved with the addition of parallel build support.
- The script maintains good compatibility while leveraging architecture-specific optimizations where possible.
The minor suggestions provided in the review comments, if implemented, would further enhance the script's clarity and robustness. Great work on these improvements!
|
The Linux ARM64 build fails because several of the dependencies (Git submodules) use Intel intrinsics which are hard to patch |
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>.