Hello,
I submitted https://lore.kernel.org/all/CAKycSdDMxfto6oTqt06TbJxXY=S7p_gtEXWDQv8mz0d9zt3... and my attention was drawn here and have a few comments.
Firstly, I was wondering why you didn't create a separate *-sys crate for these bindings? see https://doc.rust-lang.org/cargo/reference/build-scripts.html#-sys-packages for more information.
Secondly, I noticed when developing my aforementioned, patch that `bindgen` adds quite a few dependencies that probably aren't needed by the average consumer of this crate. So I was wondering what are your thoughts about generating and committing a bindings.rs then optionally using these dependencies via a feature flag?
Lastly, With your `make` integration, it looks like we could also remove the `cc` dependency by allowing `make` to build libgpiod instead and just linking with that, instead of compiling libgpiod twice.
Kind regards,
Gerard.
Hi Gerard,
On 17-12-21, 11:29, Gerard Ryan wrote:
Hello,
I submitted https://lore.kernel.org/all/CAKycSdDMxfto6oTqt06TbJxXY=S7p_gtEXWDQv8mz0d9zt3... and my attention was drawn here and have a few comments.
Firstly, I was wondering why you didn't create a separate *-sys crate for these bindings? see https://doc.rust-lang.org/cargo/reference/build-scripts.html#-sys-packages for more information.
I wasn't aware of it :(
I think yes this should be modified to a sys-crate, followed by wrapper crate to contain the wrappers around it.
Secondly, I noticed when developing my aforementioned, patch that `bindgen` adds quite a few dependencies that probably aren't needed by the average consumer of this crate. So I was wondering what are your thoughts about generating and committing a bindings.rs then optionally using these dependencies via a feature flag?
I don't have a strong preference either way, whatever works best.
Miguel, any suggestions ?
Lastly, With your `make` integration, it looks like we could also remove the `cc` dependency by allowing `make` to build libgpiod instead and just linking with that, instead of compiling libgpiod twice.
I agree, that would be better. It wasn't integrated with Make earlier and so I had to do it separately. But I may have some problem with it:
This is the vhost-device (gpio virtio) crate where I am using these bindings and have the libgpiod as a dependency:
https://github.com/vireshk/vhost-device/blob/gpio/irq/src/gpio/Cargo.toml#L1...
When I do a cargo build there (for vhost-device crate), it will try to build the dependencies as well, i.e. libgpiod, and I need to build the libgpiod's C files as well there. There are good chances that I need to build from source and libgpiod isn't installed there. How do I do it with Make ?
On Fri, Dec 17, 2021 at 6:50 AM Viresh Kumar viresh.kumar@linaro.org wrote:
I don't have a strong preference either way, whatever works best.
Miguel, any suggestions ?
Having optional pre-generated bindings may be good for some users, e.g. libsqlite3-sys does it. I guess the main question is whether you are willing to support/maintain it. Also consider cross-compilation.
But I wouldn't only provide pre-generated ones if you are using `bindgen` anyway.
In any case, I am not a Rust expert, so please take that with a grain of salt :)
Cheers, Miguel
On 17-12-21, 11:38, Miguel Ojeda wrote:
Having optional pre-generated bindings may be good for some users, e.g. libsqlite3-sys does it. I guess the main question is whether you are willing to support/maintain it. Also consider cross-compilation.
But I wouldn't only provide pre-generated ones if you are using `bindgen` anyway.
The pre-generated ones are normally good for kernel headers, where the userspace ABI is stable and so we don't need to change the generated bindings soon.
But in our case here, the ABI isn't that stable and will likely change soon again for the first few months after v2.0 is released for libgpiod.
Perhaps, we should make it compile-only for the time being. Once the ABI is stable enough, we can think of committing something to the source tree.
In any case, I am not a Rust expert, so please take that with a grain of salt :)
:)
On 17-12-2021, 3:50 PM, Viresh Kumar wrote:
followed by wrapper crate to contain the wrappers around it.
If by wrapper you mean the safe/idiomatic wrapper then I agree.
When I do a cargo build there (for vhost-device crate), it will try to build the dependencies as well, i.e. libgpiod, and I need to build the libgpiod's C files as well there. There are good chances that I need to build from source and libgpiod isn't installed there. How do I do it with Make ?
Hmmm, I was thinking `pkg-config` or make from this repo would be enough. I haven't used it myself as I don't do much c/cpp work anymore, but I've looked into https://vcpkg.io/ seems quite good. and there is https://crates.io/crates/vcpkg to integrate it. for now, perhaps `cc` is enough.
On 17-12-2021, 8:49 PM, Viresh Kumar wrote:
Perhaps, we should make it compile-only for the time being. Once the ABI is stable enough, we can think of committing something to the source tree.
Sounds good.
On Fri, Dec 17, 2021 at 8:49 PM Viresh Kumar viresh.kumar@linaro.org wrote:
On 17-12-21, 11:38, Miguel Ojeda wrote:
Having optional pre-generated bindings may be good for some users, e.g. libsqlite3-sys does it. I guess the main question is whether you are willing to support/maintain it. Also consider cross-compilation.
But I wouldn't only provide pre-generated ones if you are using `bindgen` anyway.
The pre-generated ones are normally good for kernel headers, where the userspace ABI is stable and so we don't need to change the generated bindings soon.
But in our case here, the ABI isn't that stable and will likely change soon again for the first few months after v2.0 is released for libgpiod.
Perhaps, we should make it compile-only for the time being. Once the ABI is stable enough, we can think of committing something to the source tree.
In any case, I am not a Rust expert, so please take that with a grain of salt :)
:)
-- viresh
stratos-dev@op-lists.linaro.org
-
Gerard Ryan -
Miguel Ojeda -
Viresh Kumar