search

Rust-SDL2 / rust-sdl2

SDL2 bindings for Rust
MIT License
2.77k stars 473 forks source link

A potential path to fixing `bindgen` on Windows #1059

Open LunarLambda opened 3 years ago

LunarLambda commented 3 years ago

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 the SDL_syswm and SDL_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:

let tool = cc::windows_registry::find_tool(
    std::env::var("TARGET").unwrap(),
    "cl.exe"
).expect("unable to locate compiler!");

let include = tool
    .env()
    .iter()
    .find_map(|(k, v)| if k == "INCLUDE" { Some(v) } else { None })
    .expect("uh oh, cc didn't provide include paths");

for dir in std::env::split_paths(include) {
    bindgen_builder.clang_arg(format!("-I{}", dir.display()));
}

Note that we are not adding any new dependencies, as we already depend on cc through cmake.

Unresolved Questions

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 a Debug impl in std is not ideal, and something else could be used.

arguments passed with clang_arg are not subject to shell parsing, so using Path::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

1) 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, and SDL_config_windows.h. I do not think this is desirable, however.

2) 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.

Cobrand commented 3 years ago

Thanks for looking this up! I'll be watching this closely

LunarLambda commented 3 years ago

Alright. It's been a long day.

Here's my setup:

Windows 10 (Version 20H2)
Build Tools for Visual Studio 2019 (Including Windows UCRT SDK, and Windows 10 SDK 10.0.18362.0)
LLVM 11.0.0
CMake 3.19.2
Rust Stable 1.48.0
Bindgen 0.56.0

Here's what I gathered. This may or may not be a complete list of what needs fixing, this is everything I could find.

Important note regarding this issue

bindgen appears to have absolutely no issues finding the Windows SDK headers, even in standalone invocations with no extra arguments provided. While I would appreciate additional testing, this means that my proposed fix is actually redundant.

Fixing the other issues listed below should be enough to get bundled/bindgen builds working on Windows again. I would only implement the cc-header grabbing if it turns out that bindgen cannot consistently locate the Windows headers.

I've reached out in #wg-bindgen for confirmation.

Build issues with SDL2

-> Fix: Update SDL2 to 2.0.14 (Changelog) Note that this adds several new APIs, but does not contain any breaking changes, so upgrading is safe.

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 syswm A good start would be these

Build issues with build.rs (sdl2-sys)

Suggestion regarding patching

The downloading and patching code in build.rs is far too error prone, and I think it would be much better to instead check pre-patched SDL2 source code into the repository. We already check in the headers, after all. This would also make it easier to check when, why, and how modifications are made to the included source code.

Progress

Bundled

Removing the build.rs patching code, and manually applying this patch allows sdl2-sys to successfully build with the bundled feature.

Bindgen

The bindgen feature requires pretty extensive blacklisting of Windows types.

The following blacklist, taken from here with the addition of .blacklist_function("_seh.*"), lead to a successful compile with --features bundled,bindgen.

rust-sdl2

Building rust-sdl2 itself was possible with the following modifications:

linking

Running the demo with cargo run --examlpe demo --features bundled,use-bindgen works! And so does throwing static-link in the mix!

So now we just need apply these fixes proper and get more people to test it!

LunarLambda commented 3 years ago

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, bundled doesn't work for these anywhere, because they're not included in the main SDL2 archive we download. they're separate projects that require their own build step.

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 image, rodio, rusttype), I don't see a good reason to continue supporting them.

alexcrichton commented 3 years ago

Sorry but I don't have a ton of time to help out with this right now myself :(

Cobrand commented 3 years ago

@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:

Suggestion regarding patching

The downloading and patching code in build.rs is far too error prone, and I think it would be much better to instead check pre-patched SDL2 source code into the repository. We already check in the headers, after all. This would also make it easier to check when, why, and how modifications are made to the included source code.

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.

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 image, rodio, rusttype), I don't see a good reason to continue supporting them.

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? bundled is a second tier citizen (but has always been since as a convenience and so been used first), so it's fine if it does not include gfx, image, ttf or mixer; it wasn't even intended for it to be used that widely anyway.

You also say in your other thread:

The bundled feature exists to allow building SDL2 from source on systems that do not provide the correct version of SDL2 (i.e. LTS systems with <= 2.0.5) [citation needed]

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.

LunarLambda commented 3 years ago

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 https://github.com/Rust-SDL2/rust-sdl2/issues/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 vcpkg feature is now subtly broken on Windows because of it. See https://github.com/Rust-SDL2/rust-sdl2/issues/1061 also.

waych commented 3 years ago

I believe #1080 addressed the patching issues on Windows fyi.