Support bindgen in recommended usage

[cr1901]

Okay, it's PR-back-and-forth time!

I've modified libftdi1-sys so that the bindings are generated at compile time. This alleviates ABI issues that come with typedefs resulting in different-type sizes depending on compiler and architecture. If this is undesirable for you, my suggestion would be to keep this bindgen functionality in, and fallback to using pre-generated bindings (that will probably work) on a target basis.

I've successfully generated bindings for ARMv7 Linux and x86_64 Windows 10 using GNU ABI Rust. If using the GNU ABI on Windows, build.rs will now attempt to locate libftdi1.dll using pkg-config; vcpkg does not work with the GNU version of Rust on Windows.

Changes in the API compared to the existing pre-generated bindings include:

• Opaque structs, like all libusb_ types, now seem to generate structs with dummy u8 fields instead of enums.
• Additional functions not in the pregenerated bindings (no existing functions were removed).
• timeval type alias to the type in the libc crate.

[tanriol]

This phrase looks unfinished.

[cr1901]

It is, and I realized it was after I went to sleep for the night :P.

[tanriol]

This is not strictly correct. You don't use the bindgen rust_target option, so the target by default is "the latest stable Rust version". However, as bindgen seems to almost never do compatible version bumps, this is likely to be true.

Still, this requirement sounds a bit suboptimal. I'll discuss it in a bit more detail in a follow-up comment.

[tanriol]

There is an interesting problem here: according to the recent discussion in rust-lang/rust-bindgen#1554, this #[non_exhaustive] would not help with unknown variants if a newer libftdi1 version returns them (it's still UB to have an enum with a value rustc does not know about). We may want to look into the newtype style of enums, it looks interesting.

[tanriol]

This wrapper is not "based on", but generated with bindgen (the previous one, I believe, was manually edited after generation).

[cr1901]

It looked like it was manually edited, but I couldn't confirm or not. The options I passed to bindgen generated bindings that looked the closest to what's in the repo. Will fix.

[tanriol]

Now to general comments.

One change I consider a bit problematic is the new MSRV. I'm fine with bumping it to 1.31 for 2018 edition, but not to the very latest stable version. This question actually splits into two separate sub-questions.

First, the Rust version required for generated code. In the current version with #[non_exhaustive] enums it's 1.40, but if we switch to the newtype enum style, it drops to 1.31. We're probably good here as #[non_exhaustive] seems to be useless anyway.

Second, the Rust version required for bindgen itself. I failed to find official documentation on this (and filed rust-lang/rust-bindgen#1758 about that), but it's at least 1.34 (it seems to build on 1.34, but at least clang-sys documentation says 1.36 is required).

I've modified libftdi1-sys so that the bindings are generated at compile time. This alleviates ABI issues that come with typedefs resulting in different-type sizes depending on compiler and architecture. If this is undesirable for you, my suggestion would be to keep this bindgen functionality in, and fallback to using pre-generated bindings (that will probably work) on a target basis.

Following the above, compile-time generation both introduces a dependency on clang and bumps the minimum Rust version.

I've successfully generated bindings for ARMv7 Linux and x86_64 Windows 10 using GNU ABI Rust. If using the GNU ABI on Windows, build.rs will now attempt to locate libftdi1.dll using pkg-config; vcpkg does not work with the GNU version of Rust on Windows.

Could you check whether the bindings themselves actually differ, or only layout tests in them?

If the bindings are not platform-dependent, I'd consider the following set of options:

• baseline: use pregenerated bindings;
• default-enabled feature: generate bindings for the local version;
• (possibly in the future) a default-disabled feature to use the bundled source instead of linking to the system library.

Changes in the API compared to the existing pre-generated bindings include:

* Opaque structs, like all `libusb_` types, now seem to generate structs with dummy `u8` fields instead of enums.

* Additional functions not in the pregenerated bindings (_no existing functions were removed_).

* `timeval` type alias to the type in the `libc` crate.

Sounds good to me.

[cr1901]

Well, it's good you asked me to keep you in the loop, because I'm learning a lot today. :P

First, the Rust version required for generated code. In the current version with #[non_exhaustive] enums it's 1.40, but if we switch to the newtype enum style, it drops to 1.31. We're probably good here as #[non_exhaustive] seems to be useless anyway.

Will take a look and fix; didn't realize #[non_exhaustive] was that new.

Second, the Rust version required for bindgen itself. I failed to find official documentation on this (and filed rust-lang/rust-bindgen#1758 about that), but it's at least 1.34 (it seems to build on 1.34, but at least clang-sys documentation says 1.36 is required).

Is it okay if the MSRV is different depending on whether we use pre-generated bindgen bindings or bindings generated at compile-time?

Following the above, compile-time generation both introduces a dependency on clang and bumps the minimum Rust version.

On my end, the clang dependency is very much undesirable, but Rust went all-in on LLVM. The bindgen team intends at some point to bundle libclang with the bindgen crate.

My to-be-completed (oops!) comment discusses how to set up build.rs to use libclang without installing the entirity of LLVM/Clang; in principle, once libclang is bundled, neither clang nor LLVM will need to be installed. E.g. I don't have the clang binary installed on my ARM machine, and I got generated bindings.

Could you check whether the bindings themselves actually differ, or only layout tests in them?

Will check for ARM Linux and GNU Windows; I'm not in a position to check other bindings at present.

If the bindings are not platform-dependent, I'd consider the following set of options:

If the bindings actually differ, we can generate a set of bindings for each arch using the bindgen portion of build.rs, and users w/ libclang like myself can cp them from the target dir and git add them.

The proliferation of bindings that need to be regenerated during version bumps is one reason to prefer compile-time bindings. For instance, I'm going to have to get someone else to generate OS X bindings for me, because even w/ libclang installed I don't have a copy of the relevant OS X system headers, such as stddef.h. But x86_64/x86 Windows, x86_64/x86 Linux, x86_64 OS X, armv7 Linux should be good to start with and satisfy literally 95% of use cases.

[tanriol]

Is it okay if the MSRV is different depending on whether we use pre-generated bindgen bindings or bindings generated at compile-time?

Yes, IMHO it's okay to say that old Rust versions require a workaround of using pre-generated bindings.

On my end, the clang dependency is very much undesirable, but Rust went all-in on LLVM.

I've looked at the ftdi.h header and it looks rather clean in terms of cross-platform changes. My hope is that the bindings are actually the same, except for the layout tests, so they can be shared between all platforms.

Another question that I'd like to look into but don't have time right now is how to make this crate work with cross-compiling. In theory for Linux-based systems this may require as little as falling back to libftdi1.so and hoping it's there. However, this is a topic for a separate discussion/PR, let's not cram it into this one.

[cr1901]

I've looked at the ftdi.h header and it looks rather clean in terms of cross-platform changes. My hope is that the bindings are actually the same, except for the layout tests, so they can be shared between all platforms.

Just checked: at least on ARM Linux vs Windows GNU, they are identical except for the layout tests, FWIW.

[tanriol]

Sorry, almost forgot about this one. Will merge in a day or two unless you want to introduce other changes.

[cr1901]

@tanriol I still need to apply your suggestions, but I've not been multitasking well the past few days. I'll try to get to this tonight.

[cr1901]

@tanriol Update: As you can imagine from the state of the world right now, it's not a great time for most of us. I'll make an effort to incorporate your changes this morning after I rest.

Err, scratch that. I had a second wind tonight and managed to get my changes out the door :).

[cr1901]

@tanriol Friendly ping... this PR is ready I think if you have no other changes.

[tanriol]

Sorry, I've missed the push. Will take a look today or tomorrow.

[tanriol]

Looks good to me. It compiles in both modes, and it works (with a new example as I don't have the hardware for the previous example at hand).

[tanriol]

@cr1901 Anything else you think we should get in?

[cr1901]

@tanriol

Anything else you think we should get in?

I don't need anything else right now, but I'd like to comment on two future ideas:

1. (possibly in the future) a default-disabled feature to use the bundled source instead of linking to the system library.

   I think this is a decent idea. I would also opt to include an environment variable that would choose whether to download/unzip the source or use an existing copy.

   Linked example shows one way to do this; the debug code probably isn't necessary anymore since I got it working right.

2. I'd like a related static feature or environment variable (I'm not sure which is better/used more frequently in Rust C bindings) for completely static build, disabled by default. This will be additive regardless if the bundled feature above is set or not.

   Compare "sending static arguments to the libftdi build system/C compiler" vs "sending static arguments to pkg-config, vs "not sending static arguments at all".

[tanriol]

1. (possibly in the future) a default-disabled feature to use the bundled source instead of linking to the system library.

I think this is a decent idea. I would also opt to include an environment variable that would choose whether to download/unzip the source or use an existing copy.
Linked example shows one way to do this; the debug code probably isn't necessary anymore since I got it working right.

You have an interesting configuration, but I'll probably make it work the other way around for libftdi1-sys (some version of the source is included with the crate by default and can be overridden to use an external directory instead). My main considerations are that the crate in the vendored configuration still should not require network access for build and also should really build some fixed version and not "whatever the head of the upstream git is at build time".

2. I'd like a related static feature or environment variable (I'm not sure which is better/used more frequently in Rust C bindings) for completely static build, disabled by default. This will be additive regardless if the bundled feature above is set or not.

Compare "sending static arguments to the libftdi build system/C compiler" vs "sending static arguments to pkg-config, vs "not sending static arguments at all".

How do you intend to use this feature? Do you want to link to libftdi1-sys statically even on glibc systems that already use dynamic linking for other purposes? Also, do you mean the vendored configuration to always build statically or do you want to somehow build it as a dynamic library if not static?

[cr1901]

Btw, thank you in advance for dealing w/ my bikeshedding :P.

Vendored Feature

You have an interesting configuration, but I'll probably make it work the other way around for libftdi1-sys (some version of the source is included with the crate by default and can be overridden to use an external directory instead).

My understanding so far is that you want a default-enabled feature- I'm calling it vendored instead of bundled, as above- that links against a source tarball/zip we supply. If this feature is disabled, an environment variable controls where to find the source directory (or tarball) to use. Please correct me if I'm wrong.

This leaves out the case where you want to link to the system libraries instead. Call this a hypothetical system feature. system and vendored are mutually exclusive, but unfortunately Cargo by design only really meaningfully supports additive features. This is why I think vendored should choose between linking against system libraries and compiling our own source, and an environment variable controls whether we use bundled source or an external source tree.

My main considerations are that the crate in the vendored configuration still should not require network access for build and also should really build some fixed version and not "whatever the head of the upstream git is at build time".

Oh, oops. Right, there's no choice but to do that for bearSSL, but this is perfectly reasonable for libftdi1-sys.

Static Feature

How do you intend to use this feature? Do you want to link to libftdi1-sys statically even on glibc systems that already use dynamic linking for other purposes?

Short version... I wanted to be able to deploy releases transitively depending on libftdi1-sys into tarballs and zip files for statically linked applications that "just work". I know glibc has some very weird definition of what constitutes a static link though. For instance, I have another application in Rust w/ release artifacts, using hidapi bindings, and far as I can tell, the resultant binaries are completely static, even on Linux.

EDIT: Just kidding. See the libusb dynamic link:

william@xubuntu-dtrain:~/Downloads/prog-vb-v0.10.0$ ldd prog-vb
        linux-vdso.so.1 =>  (0x00007ffe843f7000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f414cffb000)
        libusb-1.0.so.0 => /lib/x86_64-linux-gnu/libusb-1.0.so.0 (0x00007f414cde3000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f414cbdf000)
        librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007f414c9d7000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f414c7ba000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f414c5a4000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f414c29b000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f414d750000)
        libudev.so.1 => /lib/x86_64-linux-gnu/libudev.so.1 (0x00007f414d936000)

Also, do you mean the vendored configuration to always build statically or do you want to somehow build it as a dynamic library if not static?

I'm saying that the static and vendored (that's a better name than bundled, tbh) features should "compose"- i.e. (static=true, vendored=true), (static=false, vendored=true), (static=true, vendored=false), (static=false, vendored=false).

In light of the above, having a static link feature isn't important.

[tanriol]

I'm saying that the static and vendored (that's a better name than bundled, tbh) features should "compose"- i.e. (static=true, vendored=true), (static=false, vendored=true), (static=true, vendored=false), (static=false, vendored=false).

How do you expect the product for (static=false, vendored=true) look? A binary and a separate libftdi1.so?

My understanding so far is that you want a default-enabled feature- I'm calling it vendored- that links against a source tarball/zip we supply.

A default-disabled feature. The default should be to use system libraries, at least if they are present.

This is why I think vendored should choose between linking against system libraries and compiling our own source, and an environment variable controls whether we use bundled source or an external source tree.

Matches my understanding.

There's one more complication with static linking, however, that I've just noticed. libftdi1 is licensed under LGPL 2.0, which means our options are a bit limited. I'm still trying to wrap my head around all this, but we definitely shall ensure that tools like cargo license provide correct license info. The LGPL provisions on distribution are non-obvious, but bumping libftdi1-sys to LGPL and/or GPL would be really unfortunate IMHO. I'm not a lawyer, but I'm pretty sure that generating bindings during compilation should be fine for LGPL even if the library is MIT licensed (by the way, do you as a contributor agree to aligning with the Rust ecosystem by making it MIT OR Apache-2.0?); I'm pretty sure that static linking forces the resulting binary to LGPL (and this needs to be somehow reflected in metadata). Pregenerated bindings is an interesting middle ground that I'm not sure about... IMHO it should be possible to keep the library crate non-LGPL, but I have to read once again the license. Actually, I need to consider asking upstream about this.

[cr1901]

How do you expect the product for (static=false, vendored=true) look? A binary and a separate libftdi1.so?

Yes, but in retrospect I guess this is pretty silly.

A default-disabled feature. The default should be to use system libraries, at least if they are present.

Matches my understanding.

Ack.

by the way, do you as a contributor agree to aligning with the Rust ecosystem by making it MIT OR Apache-2.0?)

Need to mull over the rest, but yes I agree to this. I'll see what others Rustaceans think...

[cr1901]

@tanriol In the interim, do you mind if I start hashing out the details of the ftdi-rs API in an issue/PR on that repo? I think it might be best to merge ftdi-rs and safe-ftdi into one repo (still at ftdi-rs), and deprecate my own crate. But there are things I'd like to add/change in ftdi-rs to incorporate what I've done in safe-ftdi.

[tanriol]

@cr1901 Sure, go ahead! I don't remember the API at the moment, just that it was pretty minimal :-)