Introducing bindgen

[dariost]

In order to fix #694 and others I've tried to replace sdl2-sys with a bindgen dynamically generated -sys crate.
In the sdl2 crate only one method has changed (data1 and data2 of event::Event::User are now ::std::os::raw::c_void instead of ::libc::c_void), so the API practically hasn't changed.
The sdl2-sys crate has an almost completely new API (begin now generated by bindgen).
For now I don't feel like the other sdl related libraries (TTF, Mixer, Image and GFX) should be integrated in the sdl2-sys crate with this PR; maybe this will happen with a future one.
sdl2 pass all the tests, and sdl2-sys pass all the bindgen generated tests.
The examples which work on master (for some reason animation.rs doesn't work on my machine) also work with this PR.

[Cobrand]

Wow, that's a lot of stuff! I didn't expect that for sure! Great work overall!

I have a few concerns though, I haven't look into it in details but it looks like someone on Linux without the SDL2 headers installed wouldn't be able to compile at all, because there is no alternative to bindgen with that PR. On Windows, people would be even more screwed, since headers and stuff like that are a pain to install on WIndows.

Same comment with the feature pkg-config being the default: what would happen on Windows?

I think this PR is great, but overall I would still like people to be able to build stuff with SDL2 without the bindgen dependency if they choose to. So I actually have 2 ideas:

• Either we generate a pre-compiled sdl2-sys and get rid of the bindgen dependency in another feature (say a feature bundled-bindings)
• Or we include the default headers in sdl2-sys under a subfolder (say include), which would by default be the latest version of the SDL2 headers. A feature latest-headers could be enabled by default and use the headers in include of this repo, but once disabled it could search you default include paths (like what you have currently).

[dariost]

About your first proposal: this is hingly impractical, beacuse you would need a pregenerated sdl2-sys for each target (and there are just too many).
About your second proposal: I think that if we go in this direction we should go a step further and add a bundled feature (like the one in #693) and it would be better, after merging this PR, to rebase #693 on this instead of adding this feature to this PR.

In the meantime I've added SDL2_INCLUDE_PATH environment variable to the include search path, so if someone hasn't pkg-config and has the SDL headers on a non-standard path (like most of the time on Windows) they can add it through that variable.

Also now the fail of pkg-config is not a panic!, instead it fallbacks to the other methods (like looking at SDL2_INCLUDE_PATH), so that should work also on Windows.

I've also moved the default use-pkgconfig feature to the sdl2 crate because someone using that crate wouldn't have a way to disable it for sdl2-sys otherwise.

[Cobrand]

While rebasing with the "bundled" feature would be an acceptable idea, I just want to make sure we are on the same page about what is going to be done there.

Basically we will have only one feature: bundled.

• If you enable bundled, bindgen will use the headers downloaded from the source archive
• If you disable bundled, bindgen will look for the header files in this order:
  • via pkg-config
  • in the SDL2_INCLUDE_PATH folder if it's defined
  • in the include/ subfolder, included in that repository.

That should cover every case: even if I don't want to compile the SDL2 source myself or/and I am not on Linux, I can still compile this crate with the included headers in the include/ directory

If I want a custom version of the headers, I can always set myself the SDL2_INCLUDE_PATH environment variable beforehand.

How does that sound to you?

[dariost]

I almost completely agree: I just think the priority of SDL2_INCLUDE_PATH should be higher than pkg-config because if you set SDL2_INCLUDE_PATH you're actively saying "I want to use those headers"; on the opposite the program should go in the pkg-config path only if the user hasn't made an active decision, so the program tries its best to find those headers, first searching in the system using pkg-config, then using the bundled headers as a last resort.
Generally speaking, if I manually set an environment variable I usually expect to override stuff, so I'd not want pkg-config getting in my way.

All of those changes have landed with the last commit.

[Cobrand]

Sounds good to me! Let me just check locally that it works correctly on Linux (I'll try to get myself a Windows as well), and I think we should be good to go.

Ideally I would like someone with OSX to test with and without framework as well (and make sure it's still functional).

[dariost]

I should have mentioned that I've worked on this PR using Linux (Arch Linux to be specific) with rustc 1.21.0-nightly, so I've only tested it for this platform.

[Cobrand]

This may sound like a dumb question but can bindgen detect cross compiles? What happens if I use rustup and say compile to a win32 target from a Linux?

[Cobrand]

This last matter is especially going to be useful for those who cross compile to Android and iOS, as you simply cannot compile rust programs on those platforms without cross compiling.

[dariost]

Just tried to cross-compile some of the examples from Arch Linux with target x86_64-pc-windows-gnu: works like a charm (it produces the .exe and I tried to open some of them with wine, and it works).

[Cobrand]

@dariost What I meant is the SDL2 headers part. Do the generated headers look like they are for the target platform, or the host platform?

Take this for instance: #649, you have parts of the code that is Windows specific, and I'd like to know if that is included as well when cross-compiled.

[dariost]

As you suspected the generated headers were the same, because bindgen doesn't handle cross-compiling by itself. It was easily fixed by manually setting the target when cross-compiling. I've manually checked and now SDL_SysWMinfo has x11 and wl when compiling for x86_64-unknown-linux-gnu and it has win when compiling for x86_64-pc-windows-gnu (when before it had the right ones for Linux and only a dummy one for Windows).

[dariost]

If there is anything else I can do for this PR let me know.

[Cobrand]

I would like to test this on OSX before moving on, if you know someone or if you have a Mac yourself available (I don't), then I'd love to make sure everything work both with and without framework with this PR.

[Cobrand]

After having tested it, it works perfectly, however I'm getting this message: "warning: redundant linker flag specified for library SDL2" and if I recall correctly it has happened in the past but has been fixed. Could you check what's up with that?

Also, I'm getting some warnings that can be very easily fixed:

warning: variable does not need to be mutable
    --> src/sdl2/event.rs:1249:16
     |
1249 |     fn from_ll(mut raw: sys::SDL_Event) -> Event {
     |                ^^^^^^^
     |
     = note: #[warn(unused_mut)] on by default

warning: unnecessary `unsafe` block
    --> src/sdl2/event.rs:1620:34
     |
1620 |                     let event = *unsafe { raw.common.as_ref() };
     |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unnecessary `unsafe` block
     |
     = note: #[warn(unused_unsafe)] on by default
note: because it's nested under this `unsafe` block
    --> src/sdl2/event.rs:1254:9
     |
1254 | /         unsafe { match event_type {
1255 | |             EventType::Quit => {
1256 | |                 let event = raw.quit.as_ref();
1257 | |                 Event::Quit { timestamp: event.timestamp }
...    |
1638 | |             }
1639 | |         }}                      // close unsafe & match
     | |__________^

warning: unnecessary `unsafe` block
    --> src/sdl2/event.rs:1627:34
     |
1627 |                     let event = *unsafe { raw.user.as_ref() };
     |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unnecessary `unsafe` block
     |
     = note: #[warn(unused_unsafe)] on by default
note: because it's nested under this `unsafe` block
    --> src/sdl2/event.rs:1254:9
     |
1254 | /         unsafe { match event_type {
1255 | |             EventType::Quit => {
1256 | |                 let event = raw.quit.as_ref();
1257 | |                 Event::Quit { timestamp: event.timestamp }
...    |
1638 | |             }
1639 | |         }}                      // close unsafe & match
     | |__________^

warning: unused variable: `unknown`
   --> src/sdl2/keyboard/keycode.rs:509:13
    |
509 |         let unknown = sys::SDLK_UNKNOWN as i32;
    |             ^^^^^^^
    |
    = note: #[warn(unused_variables)] on by default

warning: unused variable: `unknown`
   --> src/sdl2/keyboard/keycode.rs:512:17
    |
512 |                 unknown => None,
    |                 ^^^^^^^
    |
    = note: #[warn(unused_variables)] on by default

warning: unused variable: `unknown`
   --> src/sdl2/keyboard/keycode.rs:519:13
    |
519 |         let unknown = sys::SDLK_UNKNOWN as i32;
    |             ^^^^^^^
    |
    = note: #[warn(unused_variables)] on by default

warning: unused variable: `unknown`
   --> src/sdl2/keyboard/keycode.rs:523:21
    |
523 |                     unknown => None,
    |                     ^^^^^^^
    |
    = note: #[warn(unused_variables)] on by default

warning: unreachable pattern
   --> src/sdl2/render.rs:120:18
    |
120 |                  _ => return None,
    |                  ^
    |
    = note: #[warn(unreachable_patterns)] on by default

warning: unreachable pattern
   --> src/sdl2/render.rs:159:18
    |
159 |                  _ => return Option::None,
    |                  ^
    |
    = note: #[warn(unreachable_patterns)] on by default

warning: unreachable pattern
   --> src/sdl2/audio.rs:219:13
    |
219 |             _                     => return None,
    |             ^
    |
    = note: #[warn(unreachable_patterns)] on by default

Would you mind cleaning as much as possible before moving on?

Thank you!

[dariost]

I've resolved all cargo build warnings and most cargo clippy warnings (I left some because I'm not sure on how to fix them).
For the warning: redundant linker flag specified for library SDL2 I didn't find how to fix it, but at least it seems harmless.
I don't have access to a macOS system, so I can't really help with that.

[Cobrand]

After discussing with some people on IRC, it looks like it's a bad idea to force everyone to use this by default; bindgen uses llvm, and while on linux systems it's pretty easy to obtain via package managers, on Windows it's not.

I don't really want to force everyone to make a hello_world test to have to install llvm dependencies on top of of everything else.

Bindgen is pretty accurate in what it generates, so in most of the times a single bindgen on a linux will work out very well for other systems. I think one of the only exception to that rule is when system-specific defines are used in the headers; as far as I can recall, this is only used with the WM* methods in SDL2 -- it's not something everyone is going to need.

So on one hand we can support everything and force users to install llvm just to build this crate, while on the other hand we could include a bindgen'd version that would be enough for 90% of the users without installing anything else.

So the idea here is to have this as an optional feature, and if this feature is disabled, use a pre-generated bindgen output (that we could include directly in this repo). Everything else would stay the same.

What do you think?

[dariost]

As discussed on IRC, I've made the bindgen dependency an optional one (adding pregenerated bindings to the crate), through a use-bindgen feature.
I didn't include this feature in the default ones, but as soon as rust-lang/cargo#1197 is resolved, I strongly suggest to set this feature as default for all non-Windows operating systems.
Also, with this PR, since now sdl2-sys uses bindings generated by bindgen regardless of the use-bindgen feature, users will need at least Rust v1.19.0, because bindgen uses unions.

[Cobrand]

As discussed on IRC, before merging this I would like to add more documentation, especially in the README (and the changelog), concerning this new feature.

I would have preferred #693 to be merged before this so that we could rebase and take the headers directly from the "source" package if we can, but that will be for another day.

[dariost]

I've sync'd with the latest commit on master, but it seems that cargo test --all-features is now broken:

error[E0107]: wrong number of lifetime parameters: expected 0, found 1
  --> examples/resource-manager.rs:73:58
   |
73 | type TextureManager<'l, T> = ResourceManager<'l, String, Texture<'l>, TextureCreator<T>>;
   |                                                          ^^^^^^^^^^^ unexpected lifetime parameter

error[E0107]: wrong number of lifetime parameters: expected 0, found 1
   --> examples/resource-manager.rs:116:32
    |
116 | impl<'l, T> ResourceLoader<'l, Texture<'l>> for TextureCreator<T> {
    |                                ^^^^^^^^^^^ unexpected lifetime parameter

I don't think it has to do with this PR, but please confirm this.

[Cobrand]

Yes it's to be expected, please do cargo test --features "gfx mixer ttf image" instead. Thanks!

[dariost]

Yep, I confirm that with cargo test --features "gfx mixer ttf image" everything works fine (all tests passed).

[dariost]

I've updated the pregenerated bindings to SDL2 version 2.0.6

[Cobrand]

Cheers! Sorry for the delay!

I added an entry to the changelog and some documentation in the README, but otherwise it was good to go, at least on Linux. Like usual I would have liked to have some more testing on Windows & macOS, but I think it shouldn't affect them too much for now as the bindgen is an opt-in and not an opt-out.