Moves low level ffi into sys and updates bindgen #765
Conversation
Closes Rust-SDL2#744 Defines were not set correctly as target matching was wrong. Removed hide_type deprecated binding builder method use, in favor of blacklist_type. Added rustified_enum call, since newer bindgen versions will make constants instead of enums breaking the sdl2-sys crate. Bindings were regenerated using the following command: cargo build --features bindgen
The 'mixer' feature is re-exported so compiling without it will leave out the mixer function definitions in sdl2-sys crate as well. Progresses on Rust-SDL2#647
Progresses on Rust-SDL2#647
Progresses on Rust-SDL2#647
I'm not sure if this is the intended way of using bindgen, but the SDL_image bindings now piggy-back on the core SDL bindings by blacklisting some types that gets dragged into the image module. The solution probably does not scale, but it might be preferable to have autoupdateable bindings rather than having to do it manually. Progresses on Rust-SDL2#647
Had to change some mutability of some raw pointers used, but the ttf demo seem to work. Progresses on Rust-SDL2#647
Renamed the binding files to be consistent. The build script could probably benefit from a good cleanup. Closes Rust-SDL2#647
It should now be what the old behaviour was. If either mac_framework or use_mac_framework feature is set then it will use kind=framework, if not then it will use normal library name link argument.
There was a problem hiding this comment.
Awesome work!
I kept the same module structure sort of, but it may seem like an even cleaner approach would be to put SDL_image, SDL_ttf, SDL_mixer, SDL_gfx into their own crates
FYI It used to be like that, SDL_image, mixer gfx and ttf had their separate crates. It was a mess because they were all maintained by different people (and everytime something changed on the main repo we had to update everything else with PRs).
Now that we have an organization we could simply separate those libs (image, ttf, ...) into separate crates under the same organization. I'm thinking of this as a potential alternative instead of the features we're using now, but I'd like to avoid breaking changes as much as possible unless we have a very good reason to do so, and I feel this isn't good enough to break sdl2 that much.
I have only a small question about rustified_enums -- at first I had some remarks but you fixed them yourself in later commits, so it's all good overall!
Before merging this I would like to make sure that every major platform works well - macOS (with and without framework), Windows & Linux. Can you tell me which platforms did you test already?
| .hide_type("FP_SUBNORMAL") | ||
| .hide_type("FP_NORMAL") // Until https://github.com/rust-lang-nursery/rust-bindgen/issues/687 gets fixed | ||
| .hide_type("max_align_t") // Until https://github.com/rust-lang-nursery/rust-bindgen/issues/550 gets fixed | ||
| .rustified_enum(".*") |
There was a problem hiding this comment.
According to https://docs.rs/bindgen/0.35.0/bindgen/struct.Builder.html#method.rustified_enum we should be warry when using this.
You should not be using Rust enums unless you have complete control of the C/C++ code that you're binding to.
Since the SDL2 headers and source code may change in the future (especially enums), can you explain why we can safely use "rustified_enum" here? This is more a question than an actual remark, I'm trying to understand what this does exactly and what the impact may be.
There was a problem hiding this comment.
What happened when I updated bindgen was that some types previously generated as enums like SDL_EventType was now being generated as "unwrapped" constants, so I started googling why that was and found these issues:
rust-lang/rust-bindgen#758
rust-lang/rust-bindgen#667
rust-lang/rust-bindgen#225
From what I understand not all C enums can be generated as rust enums, and it might depend on their usage whether they can or not. The reason why I added the rustified_enum method here was to not break the old API regarding those types, because then I had to update tons of places were the enum types in SDL was used to now use constants instead.
I do not know whether it is safe to continue generating the types as enum or not as I did not quite understand all of the issue. It seemed to be related to some nonzero optimization, and enums without a zero value, however I'm not sure why bindgen can't just detect whether the enum has a zero value or not, but perhaps it's a complicated issue. For the users of bindgen at least the effect is that previously generated enums are not generated as enums anylonger, it seems like one have to specify which one to generate as enums.
As a side note bindgen has added a feature which might be what one might want in some of the modules like mixer, there is a way to specify that an enum should be generated as bitflags, however from the looks of it the majority in the core SDL seem to not be bitflags.
For the latter part of the question, I only tested the various examples that used the different extra SDL modules for linux, fedora 27 to be exact. The only issue I found was that rust fails to link against anything but the core SDL because the library files in the rpm packed SDL modules are forgetting to create links like SDL_mixer.so, and instead creates links named SDL_mixer-2.0.so.0, the .0 at the end throws makes 'ld' fail to link if one tries to use '-lSDL_mixer-2.0', so I figured it was a SDL packaging or distribution problem rather than related to this wrapper. That's why I left it as '-lSDL_mixer' because that's how the core sdl library is named.
It seemed like a few people had issues with the current bindings so I updated the bindings and moved the ffi stuff into the sys create as suggested by #647.
I have not done much testing of the other targets and features in the build script other than linux. So mac and windows targets have to be figured out, I don't think this will break it but it was inconsistent before and this review makes the build script do a lot more work than previously. So I would not be surprised if it breaks any of them.
I couldn't get the bundle and static link cargo features to work, static linking failed because it tries to link with a library not available(perhaps really old), and the bundle failed compilation, for some other reason.
In general I think the build script probably could use a rewrite, and I'm unsure about the split of the SDL modules. I kept the same module structure sort of, but it may seem like an even cleaner approach would be to put SDL_image, SDL_ttf, SDL_mixer, SDL_gfx into their own crates. When that is said, the same features as before work now as well.
The new bindgen seemed to increase the size of the generated core SDL bindings a lot, probably by dragging in a lot of other things as well, I filtered the feature gated SDL modules but I did not bother trying to filter any of the core bindings.
I noticed that the bindings generated for the structure causing the issue in #744 depended on the defines, which was not detected because the target matching was wrong, I sort of hacked that, but I believe an even better option might be to use .contains() for all the target matching.
rustified_enum(".*") was added to the bindgen because the enums in use now is not generated by default with the new bindgen.