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]

Overall it looks good. From my understanding you are doing this if pregenerated_bindings are enabled:

• gen bindings in sdl2-sys/
• copy them to OUT_DIR

while I would rather do this

• gen bindings to OUT_DIR (like --use-bindgen)
• if update_pregenerated_bindings is enabled, copy bindings from OUT_DIR to sdl2-sys/

That way the branching of features with update_pregenerated_bindings only happens once. To make the code even simpler you can make update_pregenerated_bindings automatically enable use-bindgen to simplify branching logic as well.

[antonilol]

Copying the bindings to OUT_DIR is also used when use-bindgen is not enabled, that function would become more complicated if it needs to be able to copy from OUT_DIR to sdl2-sys/.
Whatever you want here, I don't really care, it simplifies at one place and complicates at another.

[Cobrand]

Yes you would need to just add a function to copy from the OUT_DIR to sdl2-sys/, but this would be the only time where the new feature would branch. Feature branching becomes very complex when you try to make a mental model out of the code, so let's avoid branching above all else, even if we have slightly more LoC in the end.

update_pregenerated_bindings auto implying that use-bindgen is enabled is a freebie and should also simplify quite a bit of branching.

[antonilol]

... function to copy from the OUT_DIR to sdl2-sys/ ...

I don't think copying all files in OUT_DIR is right, just looking in my target dir there is a case where directories named bin, build, include, lib and share appear there, these should not be copied over.