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

Remove bindgen dependency #37

Merged
merged 1 commit into from
Oct 19, 2020
Merged

Remove bindgen dependency #37

merged 1 commit into from
Oct 19, 2020

Conversation

yorickpeterse
Copy link
Collaborator

@yorickpeterse yorickpeterse commented Sep 30, 2020

This removes the dependency on bindgen, replacing it with a set of bindings directly included in the repository. The bindings were generated on Linux, cleaned up, then compared with the bindgen output for MacOS and Windows (using MSYS2). Apart from some noise caused by unused constants, the libffi API code was the same.

Removing bindgen cuts down the amount of dependencies, most notably the dependency on libclang. Removing this simplifies the installation procedure, as well as simplifying the build scripts a bit.

The complex FFI types are hidden behind a "complex" feature flag that is disabled by default, similar to the libffi-rs crate. The longdouble type is disabled on GNU/Linux ARM devices, per #37 (comment) and tov/libffi-rs#14.

This fixes #31.

Testing results

Platform Arch cargo test cargo test --features system
Linux x86-64 🆗 🆗
macOS x86-64 🆗 🆗
MSYS2 x86-64 🆗 ⚠️ Unable to find the library, same on master
MSVC x86-64 🆗 🆗
Linux armv7 🆗  🆗

build/msvc.rs Outdated Show resolved Hide resolved
build/msvc.rs Outdated Show resolved Hide resolved
src/binding.rs Outdated Show resolved Hide resolved
@yorickpeterse yorickpeterse marked this pull request as draft September 30, 2020 20:58
@yorickpeterse yorickpeterse changed the title WIP: Remove bindgen dependency Remove bindgen dependency Sep 30, 2020
@timfish
Copy link
Contributor

timfish commented Sep 30, 2020

It's worth pointing out that the bindings are slightly different on ARM because there is no longdouble.

Checkout this PR to try and fix it: tov/libffi-rs#14

But other than that it seems like a great idea. I've been dealing with version issues lately caused by conflicted versions of libclang.

@yorickpeterse
Copy link
Collaborator Author

It's worth pointing out that the bindings are slightly different on ARM because there is no longdouble.

Checkout this PR to try and fix it: tov/libffi-rs#14

But other than that it seems like a great idea. I've been dealing with version issues lately caused by conflicted versions of libclang.

@timfish I don't have any ARM hardware to test this on, but by the looks of it we can just copy the cfg attributes to this PR. Do you happen to have any ARM devices you could test those changes on?

@yorickpeterse
Copy link
Collaborator Author

@timfish I have adopted the cfg checks from your PR, and added a complex feature flag for the complex FFI types. If you have the opportunity, please give this PR a try on ARM and MSVC (if you have access to such a setup).

@yorickpeterse yorickpeterse marked this pull request as ready for review September 30, 2020 21:23
@timfish
Copy link
Contributor

timfish commented Sep 30, 2020

Yep, I've got a Windows machine and a Raspberry Pi that I'll give this a try on.

@yorickpeterse
Copy link
Collaborator Author

yorickpeterse commented Sep 30, 2020

Testing results thus far:

Platform Arch cargo test cargo test --features system
Linux x86-64 🆗 🆗
macOS x86-64 🆗 🆗
MSYS2 x86-64 🆗 ⚠️

Testing was done by running cargo test and cargo test --features system. Using the system feature on MSYS2 produces linker errors (seems the .dll can't be found), which I'm currently looking into.

EDIT 1: This is also happening when you run cargo test --features system on master, so either this is an existing issue or my MSYS2 setup is missing something.

EDIT 2: It seems that on MSYS2 the DLL is called libffi-7.dll, and there's no libffi.dll to be found.

@timfish
Copy link
Contributor

timfish commented Sep 30, 2020

Platform Arch cargo test cargo test --features system
MSVC x86-64 🆗 🆗1
Linux armv7 🆗 🆗
  1. I think this is a false positive (and it's still doing static linking?) because I don't have libffi binaries anywhere on that machine.

This removes the dependency on bindgen, replacing it with a set of
bindings directly included in the repository. The bindings were
generated on Linux, cleaned up, then compared with the bindgen output
for MacOS and Windows (using MSYS2). Apart from some noise caused by
unused constants, the libffi API code was the same.

Removing bindgen cuts down the amount of dependencies, most notably the
dependency on libclang. Removing this simplifies the installation
procedure, as well as simplifying the build scripts a bit.

The complex FFI types are hidden behind a "complex" feature flag that
is disabled by default, similar to the libffi-rs crate. The longdouble
type is disabled on GNU/Linux ARM devices, per
#37 (comment) and
tov/libffi-rs#14.

This fixes #31.
@yorickpeterse
Copy link
Collaborator Author

@timfish

I think this is a false positive (and it's still doing static linking?) because I don't have libffi binaries anywhere on that machine.

Correct, it static links the source install of libffi. I have no idea how dynamic linking and library management works under MSVC, or how you'd even install libffi.

@timfish
Copy link
Contributor

timfish commented Sep 30, 2020

I think on Windows you can use Vcpkg but I wouldn't worry about it. I can't see anybody using it when there's reliable static linking.

@yorickpeterse
Copy link
Collaborator Author

I've been unable to get dynamic linking on MSYS2 to work, even when using the Rust provided by mingw-w64-x86_64-rust. Since this is already happening on master I think this may be an MSYS2 issue, or perhaps Rust isn't able to find the right libraries (even when I explicitly specify them).

Since this is not a new issue, I think we can leave this as-is. As far as I understand, static linking on Windows is preferred anyway.

@yorickpeterse
Copy link
Collaborator Author

@tov could you take a look at these changes? Thanks!

p.s. If you need help maintaining the crates and such, please let me know; I'm more than happy to help increase the bus factor 😃

@yorickpeterse
Copy link
Collaborator Author

For those that want to try this pull request by patching their libffi-sys version, you can do so by adding the following to your Cargo.toml:

[patch.crates-io]
libffi-sys = { git = "https://github.com/YorickPeterse/libffi-sys-rs.git", branch = "no-bindgen-patch" }
libffi = { git = "https://github.com/YorickPeterse/libffi-rs.git", branch = "no-bindgen-patch" }

These branches contain these changes in this PR, in addition to a version change so they can be used to patch existing installations. To apply them, you need to run cargo update -p libffi-sys and cargo update -p libffi, and you should be good to go.

@yorickpeterse
Copy link
Collaborator Author

yorickpeterse commented Oct 2, 2020

Worth mentioning: with these changes I can reduce my clean cargo build --release times by about 1 minute. That's quite significant, and far more than what I expected. Note that this is using the system feature, so it doesn't include the time it takes to compile libffi itself. This was also tested on a laptop, so the timings are not the most reliable.

yorickpeterse added a commit to inko-lang/inko that referenced this pull request Oct 3, 2020
This patches the libffi and libffi-sys crates to use the changes
discussed in tov/libffi-sys-rs#37. This removes
the dependencies on bindgen and libclang, making the installation
process easier and faster.
@yorickpeterse
Copy link
Collaborator Author

@tov Do you have time to look at this PR any time soon? Thanks!

@tov
Copy link
Owner

tov commented Oct 14, 2020

Hey! Sorry. I am going to make you a maintainer, @yorickpeterse. I very much appreciate the offer.

@tov
Copy link
Owner

tov commented Oct 14, 2020

Inlining all the magic numbers from libffi.h frightens me! But you are using this library for real and I’m not, so I think it makes sense for me to defer to your judgment here. And not depending on bindgen is excellent.

@yorickpeterse
Copy link
Collaborator Author

@tov Thanks for adding me! Unfortunately there's no other way to provide these constants, as they are all created using #define in ffi.h, instead of being globals. I don't think these will ever have their value changed though, so it should be fine.

@yorickpeterse
Copy link
Collaborator Author

Since there don't seem to be any particular objections against these changes, I believe we can go ahead and merge them.

@yorickpeterse yorickpeterse merged commit b88007c into tov:master Oct 19, 2020
@yorickpeterse yorickpeterse deleted the no-bindgen branch October 19, 2020 00:47
This was referenced Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removing build-time dependency on bindgen
3 participants