-
-
Notifications
You must be signed in to change notification settings - Fork 490
imgui-sfml: fix imgui dependency version #8606
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
Conversation
Summary of ChangesHello @pewpewnor, 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 addresses a dependency issue by refining how 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 correctly fixes the imgui dependency version for imgui-sfml v3.0 by conditionally adding version constraints. The changes are consistent across the on_load and on_install functions. My review includes a suggestion to refactor the code to improve maintainability by removing a duplicated version string and to make the logic more concise.
packages/i/imgui-sfml/xmake.lua
Outdated
| local imgui_version = "" | ||
| if package:version():eq("v3.0") then | ||
| imgui_version = " >=1.91.1 <=1.91.9" | ||
| end |
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.
This logic for determining the imgui version string is duplicated from the on_load function (lines 23-27). To improve maintainability, you could define the version constraint string as a variable in the package scope and reuse it in both places.
Additionally, this block can be written more concisely using a ternary-like expression in Lua.
local imgui_version = package:version():eq("v3.0") and " >=1.91.1 <=1.91.9" or ""
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.
This code can be removed, because we can use policy package.sync_requires_to_deps
xmake-io/xmake#5745 (comment)
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.
This code can be removed, because we can use policy
package.sync_requires_to_depsxmake-io/xmake#5745 (comment)
Can you show me how? I tried setting set_policy("package.sync_requires_to_deps", true) but it doesn’t seem to work as add_requires("imgui") still resolves to the newest imgui version for some reason.
However, I found that refactoring it to:
local imgui_dep = package:dep("imgui")
local imgui_version = imgui_dep and " " .. imgui_dep:version_str() or ""works and I added it to the commit.
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.
tried setting set_policy("package.sync_requires_to_deps", true) but it doesn’t seem to work as add_requires("imgui") still resolves to the newest imgui version for some reason.
it should be fixed. xmake-io/xmake#7030
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.
tried setting set_policy("package.sync_requires_to_deps", true) but it doesn’t seem to work as add_requires("imgui") still resolves to the newest imgui version for some reason.
it should be fixed. xmake-io/xmake#7030
Using the latest dev version of xmake v3.0.4+dev.91e10df41, it could not resolve package:add("deps", "imgui >=1.91.1<=1.91.9") properly without the space in between the version constraints.
I also tried set_policy("package.sync_requires_to_deps", true) with spaces between constraints again and it still did not work, add_requires("imgui") still resolves to the latest version?
/usr/bin/git clone https://github.com/ocornut/imgui.git -b >=1.91.1<=1.91.9 --depth 1 --recursive --shallow-submodules -c core.fsmonitor=false source.tmp/imgui
Cloning into 'source.tmp/imgui'...
warning: Could not find remote branch >=1.91.1<=1.91.9 to clone.
fatal: Remote branch >=1.91.1<=1.91.9 not found in upstream origin
error: @programdir/core/sandbox/modules/os.lua:378: execv(/usr/bin/git clone https://github.com/ocornut/imgui.git -b >=1.91.1<=1.91.9 --depth 1 --recursive --shallow-submodules -c core.fsmonitor=false source.tmp/imgui) failed(128)
stack traceback:
[C]: in function 'error'
[@programdir/core/base/os.lua:1125]:
[@programdir/core/sandbox/modules/os.lua:378]:
[@programdir/core/sandbox/modules/os.lua:291]: in function 'vrunv'
[@programdir/modules/devel/git/clone.lua:127]:
[...modules/private/action/require/impl/actions/download.lua:114]: in function '_checkout'
[...modules/private/action/require/impl/actions/download.lua:392]:
=> clone https://github.com/ocornut/imgui.git >=1.91.1<=1.91.9 .. failed
we can also download these packages manually:
- https://github.com/ocornut/imgui.git
to the local search directories:
- imgui
and we can run `xmake g --pkg_searchdirs=/xxx` to set the search directories.
error: @programdir/core/main.lua:274: @programdir/modules/async/runjobs.lua:394: ...modules/private/action/require/impl/actions/download.lua:456:
stack traceback:
[C]: in function 'error'
[@programdir/core/base/os.lua:1125]:
[...modules/private/action/require/impl/actions/download.lua:456]: in function 'catch'
[@programdir/core/sandbox/modules/try.lua:123]: in function 'try'
[...modules/private/action/require/impl/actions/download.lua:367]:
[...modules/private/action/require/impl/install_packages.lua:503]: in function 'job_func'
[@programdir/modules/async/runjobs.lua:207]:
stack traceback:
[C]: in function 'error'
@programdir/core/base/os.lua:1125: in function 'os.raiselevel'
(...tail calls...)
@programdir/core/main.lua:274: in upvalue 'cotask'
@programdir/core/base/scheduler.lua:514: in function <@programdir/core/base/scheduler.lua:507>
error: execv(/home/braum/.local/bin/xmake require -f -y --build -v -D --shallow --extra={configs={shared=false}} imgui-sfml) failed(255)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.
Yes it does work for me, so does the user need to add:
add_requires("imgui >=1.91.1 <=1.91.9")
set_policy("package.sync_requires_to_deps", true)themselves everytime they want to add_requires("imgui-sfml") or is this PR still needed so that the user doesn't have to do that?
I think the original code review issue here is that adding version constraints to the add_requires("imgui") for the second time during the on_install hook is said to be not needed since set_policy("package.sync_requires_to_deps", true) can be used instead.
But from what I can tell, we still need to add the version constraints again regardless since it is independent of the package:add("deps", "imgui >=1.91.1 <=1.91.9") in the code for defining the target build, therefore switching the logic with set_policy("package.sync_requires_to_deps", true) in the package definition code will still have no effect.
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 think you can limit the version range in package add_deps, and xmake supports automatic version resolution to select the most suitable version. However, there's no need to pass the version to port/xmake.lua.
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.
If I try to remove the version constraints from the package's xmake.lua, it will still try to use the latest version of imgui when building the imgui-sfml target since that runs in a different isolation from what I can tell. It does not care about the previous add_deps version constraints at all, the same reason why in the original code (before any of my commits) checks the linux and shared again (twice) for requiring sfml.
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.
It will pass all deps configs. https://github.com/xmake-io/xmake/blob/660450b6adb49c5a8b792a0c844a414d0a1b7379/xmake/modules/package/tools/xmake.lua#L437
But it seems to have stopped working.
you can try this patch. xmake-io/xmake#7041
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.
Thanks, it's working now.
I've updated the PR.
A potential solution fix for #8605
Note: this version fix is only for
imgui-sfmlversion 3.0.0, other versions might still be broken...