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

Generate bindings during build #1524

Closed
wants to merge 4 commits into from
Closed

Generate bindings during build #1524

wants to merge 4 commits into from

Conversation

alebastr
Copy link
Contributor

@alebastr alebastr commented Dec 7, 2021

It was pointed to me during a package review that the bindgen output is platform- and architecture-dependent. Type mismatches may cause subtle bugs, especially when the platform bitness differs.
The officially recommended way of using bindgen is to embed it into the build script and run at the build time, as implemented in this PR.
Last 2 commits are fixing the build on non-Intel platforms, where c_char is defined as u8.

cargo test is passing on Linux i686, x86_64, armhfp, aarch64 and ppc64le. I don't have Windows or MacOS development machines, but I hope CI will cover that :)


There are two reasons for sending the PR; one is the technical stated above, and another is that our Rust packaging policy insists on regenerating such code when possible. So we're going to apply the patch downstream in Fedora in any case, but I can try reworking it to depend on environment or feature flags if you think that's necessary.

@alebastr
Copy link
Contributor Author

alebastr commented Dec 7, 2021

  thread 'main' panicked at 'Unable to find libclang: "couldn't find any valid shared libraries matching: ['clang.dll', 'libclang.dll'], set the `LIBCLANG_PATH` environment variable to a path where one of these files can be found (invalid: [(C:\\Program Files\\LLVM\\bin\\libclang.dll: invalid DLL (64-bit))])"', C:\Users\appveyor\.cargo\registry\src\github.com-1285ae84e5963aae\bindgen-0.59.1\src/lib.rs:2117:31
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Apparently Appveyor Windows images have only x64 llvm installed 😒
I guess that's my cue to make it an opt-in flag. Or fix the CI environment.

Edit: and done.
There could be a second part that splits pre-generated binding according to a platform (like it's done here), but I'm not planning to work on that.

@maxbrunsfeld
Copy link
Contributor

maxbrunsfeld commented Dec 10, 2021

There were a couple of stray, unused platform-specific types in the file, which I removed in 8de8c64. Otherwise, I don't think there's anything platform or architecture specific in that file. Am I missing something?

I don't want the build to depend on libclang, so I'd like to avoid running bindgen at build time unless there's a specific, concrete reason to do so.

@alebastr
Copy link
Contributor Author

I already realized that libclang dependency is too much for a default configuration, so I made that an optional feature. The build won't depend on libclang unless the feature is explicitly requested.

Last 2 commits in this PR indicate that there's no regular testing done on some of the platforms I have to support. Which means that it's just as easy to miss an accidental change in the header or bindings that would affect one of those platforms. And since FFI even in Rust is for the most part an unsafe use of raw pointers, the code will quietly compile and run.
Consider that just an additional safety net to opt-in if someone doesn't trust to (or is not permitted to use, as in my case) a pre-generated code.

By the way, I cannot reproduce the bindgen output you committed in 8de8c64 with script/generate-bindings. The resulting file is different and it fails to compile.

Bindgen output is platform- and architecture-dependent. Pre-generated
bindings may cause issues on the machines different from the one used
for generating the code.

The recommended way[1] to use bindgen is to invoke it from `build.rs`.

[1]: https://rust-lang.github.io/rust-bindgen/library-usage.html
Setting up the build environment required by bindgen could be
inconvenient on some platforms. Hide the binding regeneration under
a new feature.
Fixes build on aarch64, ppc64le and other platforms that have different
definition of c_char.
Fixes build on aarch64, ppc64le and other platforms that have different
definition of c_char.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants