odgi: add linux-aarch64 build #48682
Conversation
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
|
The Linux ARM64 build passes with warnings: but the tests fail: |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the The Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🔇 Additional comments (4)
|
![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 (4)
recipes/odgi/build.sh (1)
5-12: Architecture-specific compilation options addedThe addition of architecture-specific compilation options is a good improvement:
- It detects the machine architecture using
uname -m.- For x86_64, it sets
ARCH_OPTS="-march=sandybridge".- For other architectures, it sets
ARCH_OPTSto an empty string.This change allows for architecture-specific optimizations, which is beneficial. However, consider the following suggestions:
- You might want to add specific optimizations for other common architectures, such as ARM64 (aarch64).
- Consider using more specific x86_64 architecture flags if targeting newer processors.
- It might be helpful to add a comment explaining why "sandybridge" was chosen as the target for x86_64.
Here's a suggested improvement to handle more architectures:
case $(uname -m) in x86_64) ARCH_OPTS="-march=sandybridge" ;; aarch64) ARCH_OPTS="-march=armv8-a" ;; *) ARCH_OPTS="" ;; esacThis change would add specific optimizations for ARM64 architecture while maintaining the current behavior for other architectures.
recipes/odgi/odgi-linux-aarch64.patch (3)
8-11: Approval: Removed x86-specific optimization flagThe removal of the
-msse4.2flag fromEXTRA_FLAGSis a good change. This flag is specific to x86 architectures and not compatible with ARM64. Its removal will allow the package to build on ARM64 systems while retaining other important optimization flags.This change is crucial for cross-architecture compatibility, especially for the targeted Linux ARM64 (aarch64) build support.
20-23: Approval with suggestion: Update comment for clarityCommenting out the x86-64 specific attribute for the
mainfunction is appropriate for enabling ARM64 support. This change allows the function to be compiled without x86-64 specific optimizations.Consider updating the comment above this line to reflect the change and explain the rationale. For example:
// We previously ensured main was compiled for x86-64 architecture. // This has been disabled to support multiple architectures, including ARM64. //int main(int argc, char *argv[]) __attribute__((__target__("arch=x86-64")));
1-26: Overall approval with considerations for cross-architecture performanceThe changes in this patch file successfully remove x86-specific optimizations and constraints, which is crucial for enabling Linux ARM64 (aarch64) build support. This aligns well with the PR objectives.
Consider the following points for future improvements:
Performance testing: Ensure that removing x86-specific optimizations doesn't significantly impact performance on x86 systems. Consider implementing architecture-specific optimizations that can be conditionally applied at build time.
Documentation: Update any relevant documentation to reflect that the package now supports multiple architectures, including ARM64.
CI/CD: If not already in place, set up continuous integration tests on both x86 and ARM64 systems to catch any architecture-specific issues early.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- recipes/odgi/build.sh (2 hunks)
- recipes/odgi/meta.yaml (2 hunks)
- recipes/odgi/odgi-linux-aarch64.patch (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
recipes/odgi/build.sh (3)
3-3: Improved error handling and debuggingThe addition of
set -xeenhances the script's robustness and debuggability:
-xenables debug mode, printing each command before execution.-ecauses the script to exit immediately if any command fails.This change will make it easier to identify and diagnose issues during the build process.
Line range hint
1-36: Overall assessment of changesThe modifications to
build.share generally positive and improve the build process:
- Enhanced error handling and debugging with
set -xe.- Introduction of architecture-specific optimizations.
- Integration of these optimizations into the cmake command.
These changes should make the build process more robust and potentially more efficient on different architectures. However, consider the suggestions made in the previous comments to further improve the script:
- Handle more specific architectures (e.g., ARM64).
- Verify the necessity and safety of the
-Ofastflag.- Ensure consistency of optimization flags across different packages in the repository.
29-29: Architecture-specific options integrated into cmake commandThe modification to include
${ARCH_OPTS}in the cmake command is appropriate and consistent with the earlier changes:
- It allows the architecture-specific options to be passed to the build system.
- This change enables the use of optimizations tailored to the target architecture.
However, I'd like to draw attention to the continued use of the
-Ofastflag:
-Ofastenables aggressive optimizations that may not be standards-compliant.- It can potentially lead to unexpected behavior in some cases.
Consider verifying if
-Ofastis necessary and safe for this package. If you're unsure, you might want to consult with the package maintainers or run extensive tests to ensure it doesn't introduce any issues.To check if
-Ofastis used consistently across different build scripts in the repository, you can run:This will help ensure consistency in optimization levels across different packages.
recipes/odgi/meta.yaml (3)
53-54: LGTM. Additional platform correctly specified.The addition of 'linux-aarch64' as an additional platform in the 'extra' section is correct and consistent with the PR objective. This explicitly declares support for the Linux ARM64 architecture in the package metadata.
11-12: LGTM. Verify the patch file contents.The addition of the
odgi-linux-aarch64.patchfor the aarch64 architecture is appropriate and aligns with the PR objective. The conditional application# [aarch64]ensures it's only used when necessary.To ensure the patch file is correct and present, please run the following command:
✅ Verification successful
Verified the patch file contents. The
odgi-linux-aarch64.patchappropriately modifiesCMakeLists.txtandsrc/main.cppto support the aarch64 architecture as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and content of the odgi-linux-aarch64.patch file # Test: Check if the patch file exists and display its content if [ -f recipes/odgi/odgi-linux-aarch64.patch ]; then echo "Patch file exists. Content:" cat recipes/odgi/odgi-linux-aarch64.patch else echo "Error: Patch file not found" exit 1 fiLength of output: 1291
Line range hint
1-54: Overall changes look good, but address reported issues.The changes to
meta.yamlare minimal and correctly focused on adding ARM64 support, which aligns with the PR objective. However, there are some concerns that need to be addressed:
Build Warnings: The PR comments mention warnings about odgi being built on an x86_64 machine. This suggests potential compatibility issues.
Test Failures: The unit test in
handle.cppfailed with a segmentation violation (SIGSEGV), indicating a serious issue.These problems suggest that while the build configuration is correct, there might be underlying code issues when running on ARM64.
To investigate further, please run the following commands:
Please review the output of these commands and consider:
- Are there any ARM64-specific code paths that need adjustment?
- Is there a need for additional architecture-specific optimizations or checks in the code?
- Should we add conditional compilation or runtime checks for ARM64 support?
|
Even with 0.9.0 the test still fails with segfault: |
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>.