search

RustAudio / deepspeech-rs

Rust bindings for the deepspeech library
Other
296 stars 24 forks source link

Proposal: Check bindgen output into the repo #22

Closed maxbrunsfeld closed 4 years ago

maxbrunsfeld commented 4 years ago

⚠️ This is a semi-big change, so I totally understand if it's not the way you want to do things. ⚠️

Thanks for creating this binding!

The DeepSpeech C API doesn't contain any platform-specific definitions, so it's not necessary for every user of the library to re-run bindgen on their machine when building deepspeech-rs. Instead, we can run bindgen once, whenever the deepspeech submodule is bumped, check the generated rust code into this repository, and include it in the published crate.

This saves users of the crate a lot of headaches, because bindgen has a bunch of dependencies, requires certain versions of clang, and generates annoying warning messages whenever I build deepspeech locally. It also avoids some unnecessary complexity in the build script. There's at least one issue open currently in which people are finding the bindgen dependency to be a stumbling block.

In this PR, I've added a simple shell script that can be run whenever the core library is upgraded, to regenerate the binding file. I've checked the result into the repo as bindings.rs, and removed the sys crate.

I'll be curious to hear your thoughts.

est31 commented 4 years ago

Checking in the generated bindings seems like a good idea, but please don't remove the sys crate. The two crate separation system is a tradition inside Rust world.

maxbrunsfeld commented 4 years ago

Yeah, that's reasonable. My thinking was that the sys crate no longer contained anything much, but I agree it was an unnecessarily large change. I amended it to be less intrusive.

drahnr commented 4 years ago

It tends to be a better approach to be able to feature gate generation which would then update a in-tree file, which yields easier updates in case of viable bindgen changes or for people on platforms where the generated code of bindgen deviates from the checked in generated file content. I fully understand if the status quo sufficient for the given API (i.e. no platform dependent types/type sizes in the public API being wrapped).

est31 commented 4 years ago

@drahnr In ff5327dd39d3f4bced6844b630f4411e330479e0 I've added a project to the workspace dedicated to regenerating the file. It's invoked like cargo run --release -p bg.

As for platforms where the generated bindgen code is wrong, there are none I know of. As in: bindgen already emits platform independent types like c_char, c_int, etc that follow the C spec instead of being platform independent like u32, u64, etc. If the bindgen output poses any problems to you, please file an issue!