-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[imgui] sdl3 bindings #42850
base: master
Are you sure you want to change the base?
[imgui] sdl3 bindings #42850
Conversation
@microsoft-github-policy-service agree |
Hi @constcuriosity, thank you for the contribution. Unfortunately, all feature sets in ports should be side by side installable. This is implementing alternatives as features. See -> https://learn.microsoft.com/en-us/vcpkg/contributing/maintainer-guide#do-not-use-features-to-implement-alternatives Currently, Marking this vcpkg-team-review for a path forward. |
Hi @JavierMatosD, thanks for taking a look at this pull request. I'll take a look at seeing if there's a way forward for these two features to compile together. Do you know offhand if all of the directx version features are able to compile together? I'm just looking to extend the pattern that's already been established by the port. |
I spoke with my coworkers and it seems that the SDL project currently calls 2.x "current". See -> https://www.libsdl.org/. We think SDL2 should remain the canonical one for now. We encourage you to create an overlay port with this added functionality for your own purposes. See -> https://learn.microsoft.com/vcpkg/concepts/overlay-ports
These features were probably added erroneously. I'm closing this PR for now. Please feel free to reopen the PR if you feel we are mistaken. |
SDL3 has hit API and ABI lock as listed at https://wiki.libsdl.org/SDL3/FrontPage as well as in the formation of the sdl3 port a bit ago #40867. While not official yet, projects are now encouraged to move towards adoption of SDL3. |
# Conflicts: # ports/imgui/vcpkg.json # versions/baseline.json # versions/i-/imgui.json
While you can have a vcpkg.json file that includes both sdl2 and sdl3, you can't follow the usage lines for both of them in your own cmake files. Doing so will have the 2nd library inclusion complain about a difference between the This feels like it breaks the spirit of Ports in the current baseline must be installable simultaneously, since while you can technically install both SDL2 and SDL3 simultaneously, you can't have both of them as a dependency. If that was possible, then these features would work fine together. Practically though... I don't know when that'd really happen. I think the vast majority of projects would only ever use one or the other. And same would go for these features on the imgui port. I guess the workaround would be to remove the dependency on each of the features, and rely on the project author add the dependencies themselves, but that's counter to the goal of vcpkg. So with the TL;DR "It's not the features' fault, it's SDL2 and SDL3 incompatibility", I'd like to request an exception to the "no mutually exclusive features" policy. Making an overlay port would certainly work but it introduces a continuous maintenance burden by forking a frequently updated port (the version was even bumped within the short lifetime of this PR), and I believe the likelihood of a project purposefully including both SDL versions and both features to be very small. Thanks for your time. |
@JavierMatosD Sorry to ping you directly but it doesn't look like I have permission to re-open the PR myself. |
I want to voice my support for @constcuriosity's request for an exception to the abovementioned rule. |
SDL2 and SDL3 are mutually exclusive ports. They're two versions of the same library with API differences. While the ports can be successfully installed alongside each other, you can't include both as dependencies. The SDL library has a version enforcement mechanism. The best practice though is for vcpkg ports and features to always be installable alongside each other. But we can't do that because of above dependency conflicts. So this commit makes it so that if both the sdl2 and sdl3 features are enabled, the sdl2 ones are effecitvely ignored and a warning is logged to the user.
I've authored a compromise where the sdl2-binding, sdl3-binding, sdl2-renderer-binding, and sdl3-renderer-binding features can all be included as enabled features. When both SDL2 and SDL3 features are enabled, the SDL2 features are basically ignored and a warning is logged that says that the SDL2 dependency was not enforced. So the ports and features now technically satisfy the maintainer guidelines, but I wouldn't say it adheres to the spirit of them. Nevertheless it continues with my statement about that including both SDL2 and SDL3 features is not expected and very much not desired behavior. |
The problem is not with projects, but with ports in vcpkg. AFAIU the conflicting features will cause failures in vcpkg CI as soon as one port depends on feature sdl2 and another oner depends on feature sdl3. |
Please note I am speaking personally, not as 'the maintainers' here. I don't believe we have the power to grant 'exceptions' here, as the rule comes from technical realities of the situation. We can't build the
In addition to what dg0yt says here, I want to note that this is in large part what vcpkg is trying to 'sell'. That you can show up to our curated registry, do As long as SDL's owners consider and document SDL2 as the current version, I believe as far as our curated registry is concerned, that needs to be the version ports use when there are questions like this. SDL2 and SDL3 are in the small group of major version breaks where the upstream developers go out of their way to make the break 'clean' and simultaneously installable. The big example in our registry that also did this was qt5 and qt6. For projects that did not implement the separation qt itself did, like
It makes sense that their development repo would be a leading indicator on what they consider to be the canonical version. vcpkg, because we have to make sure downstream dependencies work when that switch is flipped, will be lagging. |
Thanks for the detailed explanation! After considering it, I agree that you are correct, and I'd like to withdraw my "application" for an exception. 😅 |
I'd like to say I'm not exactly well-versed in CMake and only scrape by with a healthy dose of intuition when working with it. Okay, that being said, I looked at the respective CMakeLists.txt files, and to me, modifying SDL_VERSION to be SDL{2/3]_VERSION wouldn't be near enough to fix this problem. There are many other variables in the respective CMake files that share the same name, i.e. SDL_OPENGL and many, many more, which could run into the same problems. That said, being able to use the ImGui vcpkg port for either SDL2 or SDL3 will be a requirement for the foreseeable future. Many legacy apps use SDL2, and not exposing the bindings for SDL3 in general, and its new GPU API in particular, will be a problem for new projects or projects that want to upgrade. Would anyone happen to have any ideas or input on the above? 🤔 (Particularly the bits about CMake) In the meantime, I'll open an issue with the SDL project and ask for their opinion on this problem. |
Okay, after talking to the maintainers of SDL, it looks like there are three ways forward with the SDL2 <-> SDL3 problem:
What do you think about this? Have I missed any other options? 🙂 |
@vicroms Please take a look. |
Adds new sdl3-binding and sdl3-renderer-binding features to the imgui port.
Imgui already has backends implemented for sdl3. They just hadn't yet been exposed as options like the sdl2 backends were. This change mainly just copies the same operations that are done for sdl2 support, but swaps the 2 for a 3.
./vcpkg x-add-version --all
and committing the result.