-
Notifications
You must be signed in to change notification settings - Fork 482
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
A potential path to fixing bindgen
on Windows
#1059
Comments
Thanks for looking this up! I'll be watching this closely |
Alright. It's been a long day. Here's my setup:
Here's what I gathered. Important note regarding this issue
Fixing the other issues listed below should be enough to get bundled/bindgen builds working on Windows again. I would only implement the I've reached out in Build issues with SDL2
-> Fix: Update SDL2 to 2.0.14 (Changelog) Build issues with bindgen-> Fix: Change the bindgen builder code to only allow types and functions exposed by SDL2, as well as any OS-specific types required by APIs like Build issues with
|
Bumping this because at least SDL2 itself builds properly now. However, I saw that we do not actually include headers for the addon libs (SDL_image, SDL_mixer, SDL_ttf, SDL_gfx), so they do not (and cannot) bindgen on Windows The only reason they bindgen on Linux is that bindgen finds the system headers in /usr/include/ instead, which is not what we want when we use bundled/bindgen. in fact, Honestly I think support for the addon libraries should be dropped entirely. They are barely maintained (sdl_ttf's last release was 2 years ago!), and they mean we need to build not one, but up to five libraries on all platforms, and seeing as they are rarely ever used, especially outside of Linux, the bindings have multiple safety issues (segfaults with sdl_mixer, thread safety with sdl_ttf, among others), and there are better crates (like |
Sorry but I don't have a ton of time to help out with this right now myself :( |
@LunarLambda I agree with mostly everything (updating to 2.0.14, removing add_msvc_bindings, ect), but there are some points where I don't really agree:
Depends how what you mean by "pre-patched" SDL2 source code. If it's a git submodule, then sure, but if it's copying the whole tree and keeping track of it, then no. I do agree that the manual patching is a bit messy, but at least we can clearly see what are the changes from the main tree.
Removing image, ttf, mixer and gfx is a big no-no for backward compatibility. They may not be perfect, but at least they work and they are stable (well, their C versions at least). If you had a project that used one of those libraries and you could only update this crate to 0.34, how would you react? You also say in your other thread:
Well... yes but no. It's mostly as a convenience for those who don't want to deal with the installation of SDL2 at all, either in Windows, another crate that uses SDL2, or as you said, old shipped versions of SDL2. There are people with stable linux or mac installations that still use this feature, and sometimes even worse, crates that depend on SDL2 directly enable "bundled" by default, just for the convenience. For everything else, if you have code or a PR ready, feel free to make one and I'll have a look. Especially if you managed to update to 2.0.14, that seems to be a priority to fix bundled on windows. |
A submodule for SDL2 would be nice but I would have to look into how those interact with cargo and build.rs. Right now there's several problems with the unidiff crate we use, like possibly having issues with DOS lineendings on Windows, and not supporting patching files in place, leaving build.rs with a bunch of largely untested filesystem code I agree that deprecating / dropping the addon libraries is likely infeasible at this stage. Issues like #1063 will have to be addressed though, and may need (semi-)breaking changes anyway. I can look into working on a PR for bumping to 2.0.14, especially since vcpkg updated their SDL2 port a while ago and the |
I believe #1080 addressed the patching issues on Windows fyi. |
The past few days I have been doing a lot of digging to find a way to make the
bindgen
feature work correctly on Windows, necessary particularly for making theSDL_syswm
andSDL_opengl
functionality work better (or at all), without having to rely on hardcoding include paths.I want to summarize what I found, and propose a potential path to fixing things.
Prior Art
First I looked to other crates, like fermium and winapi.
It turns out both crates don't use bindgen at all, but rather settle for handwritten bindings.
While this certainly a way to go, it is a long, arduous, and highly error-prone process, and I think it's in the maintainers' interest to keep
bindgen
around.vswhere
vswhere is a commandline tool used for the express purpose of locating Visual Studio installations, and query information about components within them.
In 2017 it was proposed to be used in rustup / rustc to make finding the MSVC toolchain easier.
To my knowledge, this was rejected, due to adding a dependency on an external tool not present in previous versions of Visual Studio.
rustc
After looking into it, it was found that
rustc
uses the cc crate to locate the MSVC linker, which also includes the appropriate library and include paths.See rust/compile/rustc_codegen_ssa/back/link.rs.
cc
The
cc
crate provides a module, windows_registry, whose entire purpose is to look up MSVC tool paths using Windows registry keys installed by Visual Studio.As of Visual Studio 2017, this system was replaced with a COM API,
ISetupConfiguration
.The
cc
crate implements this, and in fact uses it by default, falling back to Registry lookup for older versions.See vs15plus_instances in windows_registry
But most imoprtantly, this is, essentially, the exactly same thing that
vswhere
does.The Proposed Plan
In short, I propose to find the appropriate include paths by using the
cc
crate.Roughly, the code would look like this:
Note that we are not adding any new dependencies, as we already depend on
cc
throughcmake
.Unresolved Questions
clang_arg
formatting good like this?I'm using debug formatting so that the entire string is quoted and internal quotes are escaped, which is the correct behaviour to me, but perhaps relying on aDebug
impl instd
is not ideal, and something else could be used.arguments passed with
clang_arg
are not subject to shell parsing, so usingPath::display
properly is fine.I'm in the process of setting up a Windows environment to test if this would work as desired.
I'm writing this ahead of time to get feedback and ask @alexcrichton for help and/or ideas.
This will likely come down to testing, and how willing we are to trust the
cc
crate to keep giving us the needed information in the future.I am not currently aware of any better way to look up the location of the Windows SDK headers, beyond filesystem crawling maybe.
Note that Rust for the windows-msvc targets already depends on the Windows SDK being installed, as it needs to link to multiple libraries from it.
The proposed code fulfills a very specialized task, and may be better suited to be its own crate, and be included in sdl2-sys as an optional build depedency for the
bindgen
feature.On that note, a crate called msvc-bunny exists, fulfilling a similar purpose as
cc::windows_registry
. I did not consider using it for this proposal as it is not currently published, and has not seen updates since 2018. Nevertheless it may be useful as a code reference.Alternatives
do not generate bindings for headers that require windows headers. This would include
SDL_syswm.h
,SDL_opengl.h
,SDL_egl.h
,SDL_main.h
, andSDL_config_windows.h
. I do not think this is desirable, however.create a "mock" windows.h that only exposes the information the SDL headers actually need to work correctly. Since Windows headers are very unlikely to change in backwards-comatible ways, this may be a more preferable option to pursue, especially if the
cc
method turns out to not work consistently enough.The text was updated successfully, but these errors were encountered: