Re: [PATCH V2 1/4] libgpiod: Generate rust FFI bindings

Re: [PATCH V2 1/4] libgpiod: Generate rust FFI bindings

[Gerard Ryan]

Hello,

I submitted https://lore.kernel.org/all/CAKycSdDMxfto6oTqt06TbJxXY=S7p_gtEXWDQv8mz0d9zt3Gvw@mail.gmail.com/
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.

Re: [PATCH V2 1/4] libgpiod: Generate rust FFI bindings

[Viresh Kumar]

Hi Gerard,

On 17-12-21, 11:29, Gerard Ryan wrote:
> Hello,
> 
> I submitted https://lore.kernel.org/all/CAKycSdDMxfto6oTqt06TbJxXY=S7p_gtEXWDQv8mz0d9zt3Gvw@mail.gmail.com/
> 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#L18

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 ?

-- 
viresh

Re: [PATCH V2 1/4] libgpiod: Generate rust FFI bindings

[Miguel Ojeda]

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

Re: [PATCH V2 1/4] libgpiod: Generate rust FFI bindings

[Viresh Kumar]

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

Re: [PATCH V2 1/4] libgpiod: Generate rust FFI bindings

[Gerard Ryan]

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

[PATCH V2 1/4] libgpiod: Generate rust FFI bindings

[Viresh Kumar]

Build FFI bindings for the libgpiod helpers.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 .gitignore                    |  5 +++
 bindings/rust/Cargo.toml      | 12 +++++++
 bindings/rust/build.rs        | 60 +++++++++++++++++++++++++++++++++++
 bindings/rust/src/bindings.rs | 16 ++++++++++
 bindings/rust/src/lib.rs      |  0
 bindings/rust/wrapper.h       |  2 ++
 6 files changed, 95 insertions(+)
 create mode 100644 bindings/rust/Cargo.toml
 create mode 100644 bindings/rust/build.rs
 create mode 100644 bindings/rust/src/bindings.rs
 create mode 100644 bindings/rust/src/lib.rs
 create mode 100644 bindings/rust/wrapper.h

diff --git a/.gitignore b/.gitignore
index 2d7cc7fc0758..0d3a842734bf 100644
--- a/.gitignore
+++ b/.gitignore
@@ -29,3 +29,8 @@ libtool
 *-libtool
 m4/
 stamp-h1
+
+# Added by cargo
+
+bindings/rust/target
+bindings/rust/Cargo.lock
diff --git a/bindings/rust/Cargo.toml b/bindings/rust/Cargo.toml
new file mode 100644
index 000000000000..62f3d52ddb0f
--- /dev/null
+++ b/bindings/rust/Cargo.toml
@@ -0,0 +1,12 @@
+[package]
+name = "libgpiod"
+version = "0.1.0"
+edition = "2021"
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]
+
+[build-dependencies]
+bindgen = "0.59.1"
+cc = "1.0.46"
diff --git a/bindings/rust/build.rs b/bindings/rust/build.rs
new file mode 100644
index 000000000000..cd776332bbb9
--- /dev/null
+++ b/bindings/rust/build.rs
@@ -0,0 +1,60 @@
+extern crate bindgen;
+
+use std::env;
+use std::path::PathBuf;
+
+fn generate_bindings() {
+    // Tell cargo to invalidate the built crate whenever the wrapper changes
+    println!("cargo:rerun-if-changed=wrapper.h");
+
+    // The bindgen::Builder is the main entry point
+    // to bindgen, and lets you build up options for
+    // the resulting bindings.
+    let bindings = bindgen::Builder::default()
+        // The input header we would like to generate
+        // bindings for.
+        .header("wrapper.h")
+        // Tell cargo to invalidate the built crate whenever any of the
+        // included header files changed.
+        .parse_callbacks(Box::new(bindgen::CargoCallbacks))
+        // Finish the builder and generate the bindings.
+        .generate()
+        // Unwrap the Result and panic on failure.
+        .expect("Unable to generate bindings");
+
+    // Write the bindings to the $OUT_DIR/bindings.rs file.
+    let out_path = PathBuf::from(env::var("OUT_DIR").unwrap());
+    bindings
+        .write_to_file(out_path.join("bindings.rs"))
+        .expect("Couldn't write bindings!");
+}
+
+fn build_gpiod() {
+    // Tell Cargo that if the given file changes, to rerun this build script.
+    println!("cargo:rerun-if-changed=../../lib/");
+
+    let files = vec![
+        "../../lib/chip.c",
+        "../../lib/edge-event.c",
+        "../../lib/info-event.c",
+        "../../lib/internal.c",
+        "../../lib/line-config.c",
+        "../../lib/line-info.c",
+        "../../lib/line-request.c",
+        "../../lib/misc.c",
+        "../../lib/request-config.c",
+    ];
+
+    // Use the `cc` crate to build a C file and statically link it.
+    cc::Build::new()
+        .files(files)
+        .define("_GNU_SOURCE", None)
+        .define("GPIOD_VERSION_STR", "\"libgpio-rust\"")
+        .include("../../include")
+        .compile("gpiod");
+}
+
+fn main() {
+    generate_bindings();
+    build_gpiod();
+}
diff --git a/bindings/rust/src/bindings.rs b/bindings/rust/src/bindings.rs
new file mode 100644
index 000000000000..7d6caa7d9c11
--- /dev/null
+++ b/bindings/rust/src/bindings.rs
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#[allow(
+    clippy::all,
+    deref_nullptr,
+    dead_code,
+    non_camel_case_types,
+    non_upper_case_globals,
+    non_snake_case,
+    improper_ctypes
+)]
+
+mod bindings_raw {
+    include!(concat!(env!("OUT_DIR"), "/bindings.rs"));
+}
+pub use bindings_raw::*;
diff --git a/bindings/rust/src/lib.rs b/bindings/rust/src/lib.rs
new file mode 100644
index 000000000000..e69de29bb2d1
diff --git a/bindings/rust/wrapper.h b/bindings/rust/wrapper.h
new file mode 100644
index 000000000000..50dc5f4db406
--- /dev/null
+++ b/bindings/rust/wrapper.h
@@ -0,0 +1,2 @@
+#include <string.h>
+#include "../../include/gpiod.h"
-- 
2.31.1.272.g89b43f80a514