-
-
Notifications
You must be signed in to change notification settings - Fork 476
omath: update to 3.8.1 #8277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
omath: update to 3.8.1 #8277
Conversation
Summary of ChangesHello @luadebug, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on upgrading the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the omath
package to version 3.8.1. The changes include adding the new version and its build patch, refactoring the package script to use add_patches
instead of io.replace
for cleaner modifications, and making the build configuration more robust by conditionally enabling AVX2 support only on compatible architectures. Overall, these are good improvements. I have one major concern regarding a potentially missing patch for the new v3.8.1
which could cause build failures. Please see my detailed comment.
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the omath
package to version 3.8.1. The changes to xmake.lua
are well-executed, correctly updating the version, adding a necessary build patch, and simplifying the installation logic. The new build patch improves the build process by dynamically detecting AVX2 support. However, I've identified a critical logic error in the patch concerning the application of AVX2 compiler flags, which would cause AVX2 to be disabled on MSVC builds. I have provided a detailed comment with a suggested fix for this issue.
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the omath
package to version 3.8.1. The changes involve adding a patch to fix the build process for the new version and updating the xmake.lua
file accordingly. The patch correctly detects AVX2 support and sets compiler flags, though there are some redundant flags that could be removed for clarity. The xmake.lua
file is simplified, but contains an incorrect architecture check for enabling the AVX2 option, which should be restricted to x86/x86_64 architectures. Overall, a good update with a few areas for improvement.
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the omath
package to version 3.8.1, removing the older version and simplifying the build script. The update includes a patch to improve the CMake build process, particularly for AVX2 detection and compiler flag handling. Additionally, a fix is introduced for the imgui
package to correctly handle 32-bit cross-compilation by patching its generated CMake configuration file. My review includes a suggestion to refactor a small part of the imgui
fix to improve code readability and maintainability.
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the omath
package to version 3.8.1 and removes the previous version. The changes include adding a patch to fix the build for the new version and simplifying the xmake.lua
script for omath
. Additionally, a fix for 32-bit cross-compilation has been added to the imgui
package. The changes for omath
are well-implemented and improve the package definition. However, I've found a high-severity issue in the imgui
package change where a string replacement is incorrect and has no effect, which I've detailed in a specific comment.
BTW #8280 has similiar issue but not for
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the omath
package to version 3.8.1, removing the previous version. The changes include adding a patch to improve the CMake build process, particularly for AVX2 detection and compiler flag handling, which enhances portability. The package's xmake script is also cleaned up, removing old workarounds and improving the test snippet.
Additionally, a fix for 32-bit cross-compilation has been added to the imgui
package. My review includes a minor suggestion to simplify this fix by removing a redundant line.
Overall, the changes are well-structured and improve the robustness and maintainability of the omath
package.
|
||
os.cp(path.join(package:scriptdir(), "port", "xmake.lua"), "xmake.lua") | ||
import("package.tools.xmake").install(package, configs) | ||
local config_version_file = path.join(package:installdir("lib"), "cmake", "imgui", "imguiConfigVersion.cmake") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think modifying the cmake config file in the package description is the right choice. If modifications are needed, then a PR should be submitted to xmake. If the cmake config is unstable, pkgconfig should be prioritized instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imgui cmake config was supposed to be stable, the problem is that XMake repository itself need fix for auto-generated CMakeConfigs during cross build as I show that so expected fix here is to improve auto-generation mechanism itself, we would just keep getting similiar issue over and over as #8280 minizip auto-generate cmake config for cross?
add_rules("utils.install.cmake_importfiles")
is what generate 64 bit instead of 32 bit for cross builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you need to improve here.
https://github.com/xmake-io/xmake/blob/3c3f90e4407257bacdeb6939aabdeb7fc732f355/xmake/scripts/cmake_importfiles/xxxConfigVersion.cmake#L39
https://github.com/xmake-io/xmake/blob/5670477e415e24962c57eac21194c63946ee5e1e/xmake/modules/target/action/install/cmake_importfiles.lua#L50
Drop old version. Keep only latest version for now.