Remove X11 and Wayland specific items in pregenerated bindings, refactor build.rs, clean and update bindings

[antonilol]

fixes #1511

Remove X11 and Wayland specific items in pregenerated bindings: these bindings cause compile errors on windows, where they are not even usable. SysWM functions are pretty niche, so people using them should use bindgen

[antonilol]

For review: please look at the TODO questions (with the checkboxes) too

[Cobrand]

I am not against the idea but what does this specifically solve? Current bindings are generated for linux and 99% of users will never know the difference if it was generated for windows or something else.

The only exception is when using platform-specific stuff such as raw-window-handle, but then the feature flag use-bindgen is here for that, and I assume it's only going to be a problem for a very specific minority, but a lot of complexity added.

[antonilol]

I am not against the idea but what does this specifically solve? Current bindings are generated for linux and 99% of users will never know the difference if it was generated for windows or something else.

I dont use windows, but on master currently all windows tests fail because of a layout incompatibility found by bindgen (the generated layout tests) on windows (example). This PR fixes this, but can be overkill. I dont expect a layout incompatibility because primitive types have the same layout on linux as on windows.

The only exception is when using platform-specific stuff such as raw-window-handle, but then the feature flag use-bindgen is here for that, and I assume it's only going to be a problem for a very specific minority, but a lot of complexity added.

Agreed.

Does this crate currently build on windows?

[renski-dev]

Does this crate currently build on windows?

It does, but not with default features. I use this in my Cargo.toml:

sdl2 = { git = "https://github.com/Rust-SDL2/rust-sdl2.git", features = [
  "static-link",
  "bundled",
  "use-bindgen",
] }

Without bindgen, all the X11 types and SDL types that include X11 types fail layout checks. As far as I can tell, there is no reasonable way to solve this without target specific bindings or bindgen.

Though, why not use bindgen by default?

Edit
The cause of the mismatch is the size of type XID = libc::c_ulong. From the libc crate:

cfg_if! {
    if #[cfg(all(target_pointer_width = "64", not(windows)))] {
        pub type c_long = i64;
        pub type c_ulong = u64;
    } else {
        // The minimal size of `long` in the C standard is 32 bits
        pub type c_long = i32;
        pub type c_ulong = u32;
    }
}

[antonilol]

Without bindgen, all the X11 types and SDL types that include X11 types fail layout checks.

I saw exactly this in the CI too on windows.

Though, why not use bindgen by default?

Using git blame to see which PR added the pregenerated bindings, I found this: #695 (comment), dependencies for bindgen are hard to install on windows, but (ironically) the pregenerated bindings only work on linux (on master currently).

[antonilol]

So it looks like we might want target specific pregenerated bindings but only for the target specific items (functions, structs, etc). We could also remove that target specific stuff from the pregenerated bindings and ask people to use bindgen when they want to interact with things like the windows manager directly. This would technically be breaking for the few people using this on linux, but given that bindgen runs pretty much out of the box on linux this does not sound too bad.

[Cobrand]

Ok so this issue comes from commit adeb978 which was from #1506 . I manually bisected it myself on my windows machine. It was probably merged a bit hastily, as the CI fails but with a different error than previously.

The issue seems to come from the new sdl_bindings being wrong for windows, but something strange is: why now? Why wasn't it an issue before?

EDIT: I think it's probably fixed by doing this:

fn generate_bindings(target: &str, host: &str, headers_paths: &[String]) {
    let target_os = get_os_from_triple(target).unwrap();
    let mut bindings = bindgen::Builder::default()
        // enable no_std-friendly output by only using core definitions
        .use_core()
        .allowlist_item("(SDL|AUDIO|RW)_.*")
        .opaque_type("XEvent") // <- this
        // ....

in sdl2-sys/build.rs

XEvent is in the bindings when it has no place to be there, which is a cascade for all the other issues in the CI. It only affects SysWM bindings, which are all platform-specific anyway and you would need bindgen for that.

[renski-dev]

dependencies for bindgen are hard to install on windows

The instructions from the bindgen docs have worked every time for me, and it's basically the same as running apt install libclang-dev. I guess winget is relatively new, maybe it used to be more difficult.

[antonilol]

The issue seems to come from the new sdl_bindings being wrong for windows, but something strange is: why now? Why wasn't it an issue before?

Bindgen has layout checks that were "test functions" (#[test] fn ...) in the past that did not run in the CI, now they are compile time assertions, which do run. The layout inconsistency was not new I think.

dependencies for bindgen are hard to install on windows

The instructions from the bindgen docs have worked every time for me, and it's basically the same as running apt install libclang-dev. I guess winget is relatively new, maybe it used to be more difficult.

I was just paraphrasing there, I have never installed bindgen on windows.
I am fine with both keeping and removing the pregenerated bindings, but removing them is a big breaking change for everyone that does not have the dependencies for bindgen installed, and can be argued an unnecessary change because it already worked.

[antonilol]

EDIT: I think it's probably fixed by doing this: ...

It did not work, there were SDL structs that have fields for x11 and wayland that used types like XEvent.
What worked at the end was not providing SDL_VIDEO_DRIVER_X11 and SDL_VIDEO_DRIVER_WAYLAND. Some SysWM stuff is still in there but it is not specific to some platform or display server.

[Cobrand]

I thought it would be fixed after I mentioned it once but apparently not, but maybe this feature of Cargo.toml is not very obvious. In Cargo.toml you can add:

[features]
update_pregenerated_bindings = ["bindgen"]

And enabling update_pregenerated_bindings will auto enable the feature bindgen, saving you from a lot of branching of features or this compile_error message. 2nd point is, bindgen does not have to work with only bundled, but can also work with use-pkgconfig for instance (also I haven't tested it recently), so you shouldn't require bundled to be always on.

[antonilol]

I at first tried a feature flag, but later realized that a feature is part of the public API, can be enabled from the outside and is listed in the documentation. update_pregenerated_bindings is really for internal use.
bundled is here because I think the bindings should be generated from the bundled SDL2 headers so the versions of the SDL2 submodule and bindings are in sync. (Bindings for the other libs are generated using system headers still because they are not bundled (yet).)