RustAudio / rust-lv2

A safe, fast, and modular framework to create LV2 plugins, written in Rust
Apache License 2.0
171 stars 23 forks source link

Replacement for the `bindgen` build dependency #56

Closed JOOpdenhoevel closed 4 years ago

JOOpdenhoevel commented 4 years ago

The lv2-sys crate currently uses bindgen in it's build script to generate the bindings to the LV2 API headers. The main reason for that is that this always produces bindings that work on your local platform, without the need to maintain a different version of the same crate for every platform.

On the downside, building bindgen itself and generating the bindings takes a lot of time, about half a minute for even the simplest plugins, and requires clang to be properly installed. Especially the last point raises problems, for example because development setups on Windows are always quite tricky.

Therefore, I request an alternative solution that is faster, doesn't rely on clang, and is easy to maintain. The precise requirements are:

  1. It is easily maintainable (updating the LV2 headers only requires replacing the headers).
  2. It is reproducible (can be built, tested and deployed by Travis CI, from the command line).
  3. It supports stable, beta, and nightly Rust on all Tier 1 targets.
  4. It is easily extendable to other Tier 2 targets,
  5. Building a plugin with the solution takes an equal or less amount of time.

These requirements were developed in the discussion issue #55. Please take a look at it for more context.

YruamaLairba commented 4 years ago

Hello, i think it's possible to use a static, portable (but manually implemented) binding that would fullfill point from 2 to 5. Let me explain:

I see 2 issues for this solution:

  1. This may represent a big work to do the replacement, but we don't need all data in headers, and most of the job is already done by bindgen.
  2. it's not fulfill your first point, It need manual maintenance, but in practice, I think the lv2 spec is stable enough to no expect a breaking change. The only change we should expect is new extension, and i think this need anyway more than just adding a header to handle it.

What do you think, is it worth to try it ?

(1): for enum, due to certain limitation, it's frequent to map each variant of the C enum to a Rust constant. I checked the C enum reference an those variant should be mapped to a c int type. The sign issue with bindgen doesn't matter here because we aren't supposed to do arithmetics with an enum. Edit: worse => worth

JOOpdenhoevel commented 4 years ago

What you're suggesting is to do the the job of bindgen manually, and I don't think this is necessary; You can just use the generated bindings as a starting point and then polish it. If you want to take a look at the generated bindings, you can build the workspace and look at target/debug/build/lv2-sys-{hash}/out/bindings.rs :wink:

One of my fears of using a static sys-crate is that we loose touch to the specifications: Right now, the headers are correct per definition and since we can assume that bindgen is always correct, we also know that the generated bindings are always correct. Therefore we know that when something goes wrong, our actual rust-lv2 code is wrong, the code that we know and understand, and not the low-level interoperation of Rust and C code.

Pre-generated or manually implemented bindings, however, might be wrong for some targets and since we don't know all of these targets well, we'll have a hard time figuring out what is wrong. I can't imagine what would happen if we mess up a function pointer signature! :sweat_smile:

Therefore, if you want to try this path, I would suggest to create some sort of test that uses bindgen to generate the correct bindings for the target and compares them to your static sys crate. This would give us at least some sort of verification that the bindings are correct and would move bindgen and clang out of the dependency tree for mere usage.

YruamaLairba commented 4 years ago

look at target/debug/build/lv2-sys-{hash}/out/bindings.rs

Thing have changed here, last time i looked the lv2-sys source (through the doc), it wasn't formatted and all was on the same line.

What you're suggesting is to do the the job of bindgen manually, and I don't think this is necessary; You can just use the generated bindings as a starting point and then polish it.

That how i tried, but i got completly bored around 25% because, except for uri string, i was always checking if bindgen was generating same thing i will do. And except for enum, it do the same.

I also played a lot with bindgen and discovered many thing:

So i give up the idea of manual binding, but i keep the idea of a static binding. I think we can assume binding compatibility between target using 32 bit enum and generate the same static binding for all of them. For other target, i think it's possible to use cfgattribute and Cargo.toml to conditionally enable use of bindgen. And to make this stuff maintainable, i think we could use a xtask style script to generate the static binding and the Cargo.toml

JOOpdenhoevel commented 4 years ago

Go forth, I'm looking forward to your pull request! :+1: