added arm and aarch64 builds #51184
added arm and aarch64 builds #51184
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces changes across multiple files to address build issues related to the The Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (3)
recipes/mbgc/meta.yaml (1)
27-27: Remove trailing spaces.There are trailing spaces on this line. While this doesn't affect functionality, it's good practice to remove them for consistency and to avoid potential issues in some systems.
Please remove the trailing spaces from this line.
🧰 Tools
🪛 yamllint
[error] 27-27: trailing spaces
(trailing-spaces)
recipes/mbgc/build.sh (2)
16-19: Simplify CMake command and verify compiler settingsThe CMake command is explicit, but there are a couple of considerations:
- If
${CXX}is the default compiler, specifying-DCMAKE_CXX_COMPILER="${CXX}"may be unnecessary.- Since
CXXFLAGSalready includes the include path, andCMaketypically handles compiler flags, you might simplify the command.Consider simplifying the CMake command:
cmake -S . -B build -DCMAKE_INSTALL_PREFIX="${PREFIX}" \ - -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_COMPILER="${CXX}" \ - -DCMAKE_CXX_FLAGS="${CXXFLAGS}" \ + -DCMAKE_BUILD_TYPE=Release \ "${CONFIG_ARGS}"This assumes that the default compiler and flags are appropriately set by the environment and that no special compiler options are needed.
22-23: Set executable permissions using CMakeIf you prefer to keep manual installation steps, consider setting the executable permissions within the CMake configuration to avoid the need for a separate
chmodcommand.In your
CMakeLists.txt, you can add the following to set the permissions:set_target_properties(mbgc PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}" OUTPUT_NAME "mbgc" INSTALL_RPATH "${CMAKE_INSTALL_PREFIX}/lib" INSTALL_RPATH_USE_LINK_PATH TRUE INSTALL_MODE OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ GROUP_EXECUTE WORLD_READ WORLD_EXECUTE )This way, when you build the target, the permissions are set correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/mbgc/build.sh (1 hunks)
- recipes/mbgc/meta.yaml (2 hunks)
🧰 Additional context used
🪛 yamllint
recipes/mbgc/meta.yaml
[error] 27-27: trailing spaces
(trailing-spaces)
🔇 Additional comments (8)
recipes/mbgc/meta.yaml (6)
8-8: Build number increment looks good.The build number has been correctly incremented from 0 to 1. This is appropriate when making changes to a package without changing its version.
34-34: License information updates are appropriate and improve clarity.The changes to the license information are positive:
- The license is now specified as "GPL-3.0-or-later", which follows the SPDX identifier standard and is more precise.
- The license family update to "GPL3" is consistent with the license change.
These modifications enhance the clarity and standardization of the license information.
Also applies to: 36-36
38-38: Addition of dev_url is beneficial.The inclusion of the dev_url field, pointing to the GitHub repository, is a positive change. It improves the package metadata by providing a direct link to the development repository, making it easier for users to find the source code and potentially contribute to the project.
41-43: Addition of ARM and AArch64 platforms is aligned with PR objectives.The inclusion of linux-aarch64 and osx-arm64 as additional platforms is consistent with the PR's goal of adding ARM and AArch64 architecture builds. This expands the package's compatibility to include these important architectures.
To ensure proper support for these new platforms, please verify:
- The build system (CMake in this case) is configured to support cross-compilation for these architectures.
- Any architecture-specific code or optimizations are properly handled.
You may want to run test builds for these platforms to confirm everything works as expected.
46-47: Addition of DOI identifier is valuable.The inclusion of the DOI (Digital Object Identifier) in the identifiers field is a positive addition. This improves the package metadata by providing a persistent identifier for academic citation and tracking purposes. It will make it easier for users to cite the software in academic contexts, potentially increasing its visibility and impact in the scientific community.
23-23: Addition of libgomp dependency for Linux looks appropriate.The inclusion of libgomp as both a host and run dependency for Linux platforms is correct. This suggests that the package now requires OpenMP support on Linux.
To ensure this addition is necessary, please run the following script:
If the script finds OpenMP usage, the addition is justified. If not, consider removing this dependency.
Also applies to: 25-26
recipes/mbgc/build.sh (2)
7-8: Correct usage of compiler and linker flagsAppending
${PREFIX}/includetoCXXFLAGSand${PREFIX}/libtoLDFLAGSensures that the compiler and linker can find the necessary headers and libraries during the build process.
10-14: Appropriate configuration for macOS buildsThe conditional setting of
CONFIG_ARGSfor macOS using-DCMAKE_FIND_FRAMEWORK=NEVERand-DCMAKE_FIND_APPBUNDLE=NEVERcorrectly prevents CMake from searching for frameworks and app bundles, which is suitable for this build.
| export INCLUDES="-I${PREFIX}/include" | ||
| export LIBPATH="-L${PREFIX}/lib" |
There was a problem hiding this comment.
Unused environment variables INCLUDES and LIBPATH
The environment variables INCLUDES and LIBPATH are defined but not used elsewhere in the script. These variables can be removed to simplify the script and avoid confusion.
Apply this diff to remove the unused variables:
-export INCLUDES="-I${PREFIX}/include"
-export LIBPATH="-L${PREFIX}/lib"📝 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 LIBPATH="-L${PREFIX}/lib" |
| chmod 0755 build/mbgc | ||
| cp -rf build/mbgc $PREFIX/bin |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use cmake --install for installation
Instead of manually setting permissions and copying the binary, you can use cmake --install to handle the installation process. This ensures that any additional installation steps defined in the CMakeLists.txt are executed correctly.
Replace the manual install steps with:
-chmod 0755 build/mbgc
-cp -rf build/mbgc $PREFIX/bin
+cmake --install buildThis change assumes that the install target is properly configured in your CMakeLists.txt file.
Committable suggestion was skipped due to low confidence.
|
The failure on OSX is caused due to missing value for |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
recipes/mbgc/meta.yaml (2)
25-25: LGTM: Addition of libgomp for Linux is appropriate.The inclusion of
libgompas a host requirement for Linux platforms is correct and consistent with the existingllvm-openmprequirement for macOS. This ensures proper OpenMP support across different platforms.Consider adding a comment explaining why these OpenMP libraries are needed, for clarity:
- llvm-openmp # [osx] # OpenMP support for macOS - libgomp # [linux] # OpenMP support for Linux
39-40: LGTM: Metadata updates enhance package information and discoverability.The changes to the metadata are comprehensive and valuable:
- The summary now properly ends with a period.
- The addition of a
dev_urlprovides a direct link to the development repository.- Specifying additional platforms (linux-aarch64 and osx-arm64) aligns with the PR objective of adding ARM and AArch64 builds.
- The
recipe-maintainersfield replaces the previousmaintainersfield, which is the correct format for Conda-forge recipes.- The inclusion of a DOI in the
identifiersfield enhances the package's discoverability and citability.These updates significantly improve the package's metadata.
Consider adding a
doc_urlfield if documentation is available separately from thedev_url.Also applies to: 43-49
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/mbgc/libamac64.a.patch (1 hunks)
- recipes/mbgc/meta.yaml (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- recipes/mbgc/libamac64.a.patch
🧰 Additional context used
🔇 Additional comments (5)
recipes/mbgc/meta.yaml (5)
8-8: LGTM: Build number increment is appropriate.The build number has been correctly incremented from 0 to 1. This is the expected practice when modifying a recipe without changing the package version.
27-28: LGTM: Inclusion of OpenMP libraries in run requirements is correct.The addition of
llvm-openmpfor macOS andlibgompfor Linux in the run requirements is appropriate. This ensures that the necessary OpenMP support is available at runtime, maintaining consistency with the host requirements and guaranteeing runtime compatibility across different platforms.
36-36: LGTM: License information updates are accurate and follow best practices.The changes to the license information are appropriate:
- Updating the license to "GPL-3.0-or-later" follows the SPDX identifier format, which is a best practice for specifying licenses.
- Changing the license family to "GPL3" is consistent with the updated license.
These modifications provide more precise and standardized license information for the package.
Also applies to: 38-38
Line range hint
1-49: Summary: Recipe updates successfully incorporate ARM builds and improve metadata.This review confirms that the changes to the
meta.yamlfile successfully address the PR objective of incorporating ARM and AArch64 builds. The recipe has been updated with:
- Proper build number increment.
- Addition of a patch for macOS x86_64 (though more information is needed).
- Inclusion of necessary OpenMP libraries for different platforms.
- More precise license specification.
- Comprehensive metadata updates, including support for ARM architectures.
These changes adhere to Conda packaging best practices and significantly improve the recipe. Minor suggestions have been made for further enhancements, but overall, the updates are well-implemented and ready for integration.
To fully address the PR objectives, ensure that the
build.shscript (not reviewed here) properly handles the build process for the new architectures.
11-12: Please provide more information about the patch.A new patch
libamac64.a.patchhas been added for macOS on x86_64 architecture. While the conditional application is correct, it would be helpful to understand:
- What specific issue does this patch address?
- Is there a corresponding issue or pull request in the upstream repository?
Consider adding a comment in the recipe to explain the purpose of this patch for future maintainers.
To verify the existence and content of the patch file, please run:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
recipes/mbgc/asmlib_off.patch (5)
15-16: Approved: Disabling asmlib inclusion.The change effectively disables the inclusion of the asmlib library for all architectures, which aligns with the PR objective of avoiding build problems on macOS.
Consider adding a comment explaining why asmlib is disabled:
-#if !defined(__arm__) && !defined(__aarch64__) && !defined(__ARM_ARCH) +#if false // Disable asmlib to avoid build problems on macOS
32-33: Approved: Consistent disabling of asmlib inclusion.This change is consistent with the modification in MBGC_Encoder.cpp, ensuring that asmlib is not included across the codebase.
For consistency, consider adding the same comment as suggested for MBGC_Encoder.cpp:
-#if !defined(__arm__) && !defined(__aarch64__) && !defined(__ARM_ARCH) +#if false // Disable asmlib to avoid build problems on macOS
49-50: Approved: Ensuring A_memcpy declaration for all architectures.This change ensures that the
A_memcpyfunction declaration is always included, providing consistent behavior across all architectures.Consider adding a comment explaining the change:
-#if defined(__arm__) || defined(__aarch64__) || defined(__ARM_ARCH) +#if true // Include A_memcpy for all architectures to ensure consistent behavior
66-67: Approved: Consistent inclusion of A_memcpy implementation.This change is consistent with the modification in helper.h, ensuring that the
A_memcpyfunction implementation is always included.For consistency, consider adding the same comment as suggested for helper.h:
-#if defined(__arm__) || defined(__aarch64__) || defined(__ARM_ARCH) +#if true // Include A_memcpy for all architectures to ensure consistent behavior
83-84: Approved with suggestion: Prevent linking against ASM_LIB.Commenting out the
link_libraries(${ASM_LIB})command effectively prevents linking against asmlib for all builds, which is consistent with the other changes. However, using CMake's conditional statements would be a more robust solution.Consider using a CMake option to control asmlib linking:
option(USE_ASMLIB "Use asmlib" OFF) if(USE_ASMLIB) link_libraries(${ASM_LIB}) endif()This approach allows easier toggling of asmlib usage if needed in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/mbgc/asmlib_off.patch (1 hunks)
- recipes/mbgc/meta.yaml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- recipes/mbgc/meta.yaml
🧰 Additional context used
🔇 Additional comments (1)
recipes/mbgc/asmlib_off.patch (1)
1-87: Overall patch review: Effectively disables asmlib usage.This patch successfully addresses the objective of disabling asmlib usage to avoid build problems on macOS. The changes are consistent across multiple files, ensuring that asmlib is not included or linked in the build process.
While the changes achieve their purpose, consider implementing the suggested improvements:
- Add explanatory comments to the preprocessor directives for better code readability.
- Use a CMake option for more flexible control over asmlib usage in the build system.
These enhancements will improve code maintainability and provide more explicit control over the build configuration.
|
@BiocondaBot please add label |
Thanks @martin-g . cmake surely could be better, but it was not the failure cause. |
added arm and aarch64 builds to mbgc recipe
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>.