The Wayback Machine - https://web.archive.org/web/20201221114830/https://github.com/RustAudio/rust-lv2/issues/56
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replacement for the `bindgen` build dependency #56

Closed
Janonard opened this issue Mar 20, 2020 · 4 comments
Closed

Replacement for the `bindgen` build dependency #56

Janonard opened this issue Mar 20, 2020 · 4 comments

Comments

@Janonard
Copy link
Member

@Janonard Janonard commented Mar 20, 2020

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
Copy link
Contributor

@YruamaLairba YruamaLairba commented Mar 30, 2020

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:

  • std::os::raw define equivalent for almost all C basic type
  • rust have a #[repr(C)] attribute that can used on struct, enum(1) and union, to make them correspond to their C equivalent.
  • the LV2 API doesn't almost have any conditional definition.
  • the LV2 API take a special care to use only standard, well defined and widely used datatype, so there is very few chance to encounter a type that can't be manually mapped once for all target.

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

@Janonard
Copy link
Member Author

@Janonard Janonard commented Apr 5, 2020

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 😉

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! 😅

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
Copy link
Contributor

@YruamaLairba YruamaLairba commented Apr 7, 2020

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:

  • you can use filter to control what is generated. we can use it to reduce the amount of useless binding.
  • you can do cross-plaform bindgen, but in practice i'm only able to do it on very simple example (i think it's a header/configuration issue).
  • for most platform of the tier 1 and 2, regular enum binds map to i32 or u32 representation/layout.
  • for the rest of the tier 1 and 2, i just can't generate the binding.
  • for enum with when an variant is out of range the underlaying type, there is different behavior (we aren't concerned):
    • it's overflow/underflow a i32 when targeting MSVC,
    • range of the underlaying type is adjusted when targeting gnu:
      • default is u32,
      • it become i32 if a variant is negative
      • if the range doesn't fit u32, it become u64 or i64

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

@Janonard
Copy link
Member Author

@Janonard Janonard commented Apr 8, 2020

Go forth, I'm looking forward to your pull request! 👍

@Janonard Janonard closed this May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.