| Message ID | 44ee8c36d58049de2f653494e16cba04b198fb35.1657279685.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | libgpiod: Add Rust bindings | expand |
On Fri, Jul 08, 2022 at 05:04:54PM +0530, Viresh Kumar wrote: > This adds libgpiod-sys rust crate, which provides FFI (foreign function > interface) bindings for libgpiod APIs. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Just a quick qualifier before we get started - I'm relatively new to Rust and this the first Rust code I've reviewed, so my opinions may not reflect current idiomatic Rust or may even be complete rubbish. > --- > .gitignore | 5 ++ > bindings/rust/libgpiod-sys/Cargo.toml | 15 ++++++ > bindings/rust/libgpiod-sys/build.rs | 69 +++++++++++++++++++++++++++ > bindings/rust/libgpiod-sys/src/lib.rs | 20 ++++++++ > bindings/rust/libgpiod-sys/wrapper.h | 2 + > 5 files changed, 111 insertions(+) > create mode 100644 bindings/rust/libgpiod-sys/Cargo.toml > create mode 100644 bindings/rust/libgpiod-sys/build.rs > create mode 100644 bindings/rust/libgpiod-sys/src/lib.rs > create mode 100644 bindings/rust/libgpiod-sys/wrapper.h > > diff --git a/.gitignore b/.gitignore > index 58e1c5fc7e00..9541482d5efb 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -33,3 +33,8 @@ stamp-h1 > # profiling > *.gcda > *.gcno > + > +# Added by cargo > + > +target > +Cargo.lock > diff --git a/bindings/rust/libgpiod-sys/Cargo.toml b/bindings/rust/libgpiod-sys/Cargo.toml > new file mode 100644 > index 000000000000..77f82719d269 > --- /dev/null > +++ b/bindings/rust/libgpiod-sys/Cargo.toml > @@ -0,0 +1,15 @@ > +[package] > +name = "libgpiod-sys" > +version = "0.1.0" > +edition = "2018" > + > +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html > + > +[dependencies] > + > +[features] > +generate = [ "bindgen" ] > + > +[build-dependencies] > +bindgen = { version = "0.59.1", optional = true } > +cc = "1.0.46" > diff --git a/bindings/rust/libgpiod-sys/build.rs b/bindings/rust/libgpiod-sys/build.rs > new file mode 100644 > index 000000000000..bbcd30f79d23 > --- /dev/null > +++ b/bindings/rust/libgpiod-sys/build.rs > @@ -0,0 +1,69 @@ > +#[cfg(feature = "generate")] > +extern crate bindgen; > +#[cfg(feature = "generate")] > +use std::env; > +#[cfg(feature = "generate")] > +use std::path::PathBuf; > + > +#[cfg(feature = "generate")] > +fn generate_bindings(files: &Vec<&str>) { > + // Tell cargo to invalidate the built crate whenever following files change > + println!("cargo:rerun-if-changed=wrapper.h"); > + > + for file in files { > + println!("cargo:rerun-if-changed={}", file); > + } > + > + // 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(files: Vec<&str>) { > + // Tell Cargo that if the given file changes, to rerun this build script. > + println!("cargo:rerun-if-changed=../../../lib/"); > + > + // 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-sys\"") > + .include("../../../include") > + .compile("gpiod"); > +} > + > +fn main() { > + let files = vec![ > + "../../../lib/chip.c", > + "../../../lib/chip-info.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", > + ]; > + > + #[cfg(feature = "generate")] > + generate_bindings(&files); > + build_gpiod(files); > +} Shouldn't bindings wrap libgpiod and dynamically link against it rather than building and linking statically? > diff --git a/bindings/rust/libgpiod-sys/src/lib.rs b/bindings/rust/libgpiod-sys/src/lib.rs > new file mode 100644 > index 000000000000..3384863a567c > --- /dev/null > +++ b/bindings/rust/libgpiod-sys/src/lib.rs > @@ -0,0 +1,20 @@ > +// 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 > +)] > + Are all these really necessary? Builds mostly clean for me with just: + non_camel_case_types, + non_upper_case_globals, Both non_snake_case and deref_nullptr are only required for tests. The deref_nullptr masks several warnings like this: warning: dereferencing a null pointer --> src/bindings.rs:121:14 | 121 | &(*(::std::ptr::null::<max_align_t>())).__clang_max_align_nonce1 as *const _ as usize | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this code causes undefined behavior when executed | = note: `#[warn(deref_nullptr)]` on by default which is code generated by bindgen, which is a bit of a worry. It is only used for alignment tests, but you'd think they would disable the warning just around that code themselves. Disabling deref_nullptr globally for all builds is at best poor form. Perhaps only disable it for test builds, i.e. #[cfg_attr(test, allow(deref_nullptr, non_snake_case))] > +mod bindings_raw { > + #[cfg(feature = "generate")] > + include!(concat!(env!("OUT_DIR"), "/bindings.rs")); > + > + #[cfg(not(feature = "generate"))] > + include!("bindings.rs"); > +} > +pub use bindings_raw::*; > diff --git a/bindings/rust/libgpiod-sys/wrapper.h b/bindings/rust/libgpiod-sys/wrapper.h > new file mode 100644 > index 000000000000..7bc1158b7d90 > --- /dev/null > +++ b/bindings/rust/libgpiod-sys/wrapper.h > @@ -0,0 +1,2 @@ > +#include <string.h> > +#include "../../../include/gpiod.h" The string.h is just to provide strlen() for the wrapper crate?? (but also pulls in the other string functions) The wrapper crate already depends on libc - why not use libc::strlen() there and drop this include here? And then wrapper.h becomes redundant - call bindgen on gpiod.h directly. Cheers, Kent.
On 27-07-22, 10:57, Kent Gibson wrote: > On Fri, Jul 08, 2022 at 05:04:54PM +0530, Viresh Kumar wrote: > > +fn main() { > > + let files = vec![ > > + "../../../lib/chip.c", > > + "../../../lib/chip-info.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", > > + ]; > > + > > + #[cfg(feature = "generate")] > > + generate_bindings(&files); > > + build_gpiod(files); > > +} > > Shouldn't bindings wrap libgpiod and dynamically link against it rather > than building and linking statically? There are few problems I faced, because of which I had to do it this way. - I couldn't find a way to do a "Make" for libgpiod from here and then link to the resultant library. - libgpiod may not be automatically installed in the environment where the end user of these Rust APIs exists. So I had to build it. - And then the API is changing a lot, maybe down the line once it is stable enough we can change this to something else. > > diff --git a/bindings/rust/libgpiod-sys/src/lib.rs b/bindings/rust/libgpiod-sys/src/lib.rs > > new file mode 100644 > > index 000000000000..3384863a567c > > --- /dev/null > > +++ b/bindings/rust/libgpiod-sys/src/lib.rs > > @@ -0,0 +1,20 @@ > > +// 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 > > +)] > > + > > Are all these really necessary? Actually not, thanks for pointing this out. > Builds mostly clean for me with just: > > + non_camel_case_types, > + non_upper_case_globals, > > Both non_snake_case and deref_nullptr are only required for tests. and if you want to run sanity checks with "fmt" or "clippy". > The deref_nullptr masks several warnings like this: > > warning: dereferencing a null pointer > --> src/bindings.rs:121:14 > | > 121 | &(*(::std::ptr::null::<max_align_t>())).__clang_max_align_nonce1 as *const _ as usize > | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this code causes undefined behavior when executed > | > = note: `#[warn(deref_nullptr)]` on by default > > which is code generated by bindgen, which is a bit of a worry. > It is only used for alignment tests, but you'd think they would disable > the warning just around that code themselves. > > Disabling deref_nullptr globally for all builds is at best poor form. Even with this these will get disabled only for the code present in libgpiod-sys crate, file bindgen.rs (the automatically generated one). This won't cause the warnings to be skipped for the libgpiod rust wrappers in the libgpiod crate. > Perhaps only disable it for test builds, i.e. > > #[cfg_attr(test, allow(deref_nullptr, non_snake_case))] I also run following normally: cargo fmt --all -- --check; cargo clippy --workspace --bins --examples --benches --all-features -- -D warnings to do sanity checks, etc. And this will also generate warnings, not just tests. > > +mod bindings_raw { > > + #[cfg(feature = "generate")] > > + include!(concat!(env!("OUT_DIR"), "/bindings.rs")); > > + > > + #[cfg(not(feature = "generate"))] > > + include!("bindings.rs"); > > +} > > +pub use bindings_raw::*; > > diff --git a/bindings/rust/libgpiod-sys/wrapper.h b/bindings/rust/libgpiod-sys/wrapper.h > > new file mode 100644 > > index 000000000000..7bc1158b7d90 > > --- /dev/null > > +++ b/bindings/rust/libgpiod-sys/wrapper.h > > @@ -0,0 +1,2 @@ > > +#include <string.h> > > +#include "../../../include/gpiod.h" > > The string.h is just to provide strlen() for the wrapper crate?? > (but also pulls in the other string functions) > The wrapper crate already depends on libc - why not use libc::strlen() > there and drop this include here? Right, done. > And then wrapper.h becomes redundant - call bindgen on gpiod.h directly. Rust documentation specifically suggests wrapper.h to be created [1], maybe it it is better to just keep it, even if we have a single entry in there.
On Wed, Jul 27, 2022 at 10:21:58AM +0530, Viresh Kumar wrote: > On 27-07-22, 10:57, Kent Gibson wrote: > > On Fri, Jul 08, 2022 at 05:04:54PM +0530, Viresh Kumar wrote: > > > +fn main() { > > > + let files = vec![ > > > + "../../../lib/chip.c", > > > + "../../../lib/chip-info.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", > > > + ]; > > > + > > > + #[cfg(feature = "generate")] > > > + generate_bindings(&files); > > > + build_gpiod(files); > > > +} > > > > Shouldn't bindings wrap libgpiod and dynamically link against it rather > > than building and linking statically? > > There are few problems I faced, because of which I had to do it this way. > > - I couldn't find a way to do a "Make" for libgpiod from here and then link to > the resultant library. > > - libgpiod may not be automatically installed in the environment where the end > user of these Rust APIs exists. So I had to build it. > > - And then the API is changing a lot, maybe down the line once it is stable > enough we can change this to something else. > Sure, it is a problem, but static isn't the solution. You should be able to get the appropriate paths from autoconf, but I would refer you to Bart on that. Wrt, "API changing a lot", autoconf make build dependencies should sort that out for you. > > > diff --git a/bindings/rust/libgpiod-sys/src/lib.rs b/bindings/rust/libgpiod-sys/src/lib.rs > > > new file mode 100644 > > > index 000000000000..3384863a567c > > > --- /dev/null > > > +++ b/bindings/rust/libgpiod-sys/src/lib.rs > > > @@ -0,0 +1,20 @@ > > > +// 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 > > > +)] > > > + > > > > Are all these really necessary? > > Actually not, thanks for pointing this out. > > > Builds mostly clean for me with just: > > > > + non_camel_case_types, > > + non_upper_case_globals, > > > > Both non_snake_case and deref_nullptr are only required for tests. > > and if you want to run sanity checks with "fmt" or "clippy". > > > The deref_nullptr masks several warnings like this: > > > > warning: dereferencing a null pointer > > --> src/bindings.rs:121:14 > > | > > 121 | &(*(::std::ptr::null::<max_align_t>())).__clang_max_align_nonce1 as *const _ as usize > > | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this code causes undefined behavior when executed > > | > > = note: `#[warn(deref_nullptr)]` on by default > > > > which is code generated by bindgen, which is a bit of a worry. > > It is only used for alignment tests, but you'd think they would disable > > the warning just around that code themselves. > > > > Disabling deref_nullptr globally for all builds is at best poor form. > > Even with this these will get disabled only for the code present in libgpiod-sys > crate, file bindgen.rs (the automatically generated one). This won't cause the > warnings to be skipped for the libgpiod rust wrappers in the libgpiod crate. > By "all builds" I meant build/tests/fmt/clippy etc of this module, not others. My concern being that a subsequent bindgen may introduce a problem into the generated code that the allows would hide. So try to keep them restricted to the problem at hand as much as possible. > > Perhaps only disable it for test builds, i.e. > > > > #[cfg_attr(test, allow(deref_nullptr, non_snake_case))] > > I also run following normally: > > cargo fmt --all -- --check; cargo clippy --workspace --bins --examples --benches --all-features -- -D warnings > > to do sanity checks, etc. And this will also generate warnings, not just tests. > So #[cfg_attr(any(test,fmt,clippy), allow(deref_nullptr, non_snake_case))] ? > > > +mod bindings_raw { > > > + #[cfg(feature = "generate")] > > > + include!(concat!(env!("OUT_DIR"), "/bindings.rs")); > > > + > > > + #[cfg(not(feature = "generate"))] > > > + include!("bindings.rs"); > > > +} > > > +pub use bindings_raw::*; > > > diff --git a/bindings/rust/libgpiod-sys/wrapper.h b/bindings/rust/libgpiod-sys/wrapper.h > > > new file mode 100644 > > > index 000000000000..7bc1158b7d90 > > > --- /dev/null > > > +++ b/bindings/rust/libgpiod-sys/wrapper.h > > > @@ -0,0 +1,2 @@ > > > +#include <string.h> > > > +#include "../../../include/gpiod.h" > > > > The string.h is just to provide strlen() for the wrapper crate?? > > (but also pulls in the other string functions) > > The wrapper crate already depends on libc - why not use libc::strlen() > > there and drop this include here? > > Right, done. > > > And then wrapper.h becomes redundant - call bindgen on gpiod.h directly. > > Rust documentation specifically suggests wrapper.h to be created [1], maybe it > it is better to just keep it, even if we have a single entry in there. > Specifically the tutoral says: "The wrapper.h file will include all the various headers containing declarations of structs and functions we would like bindings for." If you do need to bundle several headers then fair enough, but I don't see any benefit in this case - gpiod.h contains all that. The tutorial is probably written that way so it is easy for them to refer to the general "wrapper.h", but there is nothing in bindgen that requires it. Cheers, Kent.
On 27-07-22, 13:17, Kent Gibson wrote: > Sure, it is a problem, but static isn't the solution. > You should be able to get the appropriate paths from autoconf, but I would > refer you to Bart on that. Sure, if someone can suggest a better way of doing this, I am up for it. I just don't know how to do it as of now. > By "all builds" I meant build/tests/fmt/clippy etc of this module, not > others. Ahh. > My concern being that a subsequent bindgen may introduce a problem into > the generated code that the allows would hide. So try to keep them > restricted to the problem at hand as much as possible. I agree. > #[cfg_attr(any(test,fmt,clippy), allow(deref_nullptr, non_snake_case))] > ? Finally just this was enough for fmt/clippy too :) #[cfg_attr(test, allow(deref_nullptr, non_snake_case))] > Specifically the tutoral says: > "The wrapper.h file will include all the various headers containing > declarations of structs and functions we would like bindings for." > > If you do need to bundle several headers then fair enough, but I don't > see any benefit in this case - gpiod.h contains all that. > > The tutorial is probably written that way so it is easy for them to > refer to the general "wrapper.h", but there is nothing in bindgen that > requires it. Sure nothing will break if the file isn't there. Okay removed it now: diff --git a/bindings/rust/libgpiod-sys/build.rs b/bindings/rust/libgpiod-sys/build.rs index 147daaf6b1da..96f832134431 100644 --- a/bindings/rust/libgpiod-sys/build.rs +++ b/bindings/rust/libgpiod-sys/build.rs @@ -8,7 +8,7 @@ use std::path::PathBuf; #[cfg(feature = "generate")] fn generate_bindings(files: &Vec<&str>) { // Tell cargo to invalidate the built crate whenever following files change - println!("cargo:rerun-if-changed=wrapper.h"); + println!("cargo:rerun-if-changed=../../../include/gpiod.h"); for file in files { println!("cargo:rerun-if-changed={}", file); @@ -24,7 +24,7 @@ fn generate_bindings(files: &Vec<&str>) { let mut builder = bindgen::Builder::default() // The input header we would like to generate // bindings for. - .header("wrapper.h"); + .header("../../../include/gpiod.h"); if cfg!(feature = "gpiosim") { builder = builder.header("gpiosim_wrapper.h"); diff --git a/bindings/rust/libgpiod-sys/wrapper.h b/bindings/rust/libgpiod-sys/wrapper.h deleted file mode 100644 index 32290711642a..000000000000 --- a/bindings/rust/libgpiod-sys/wrapper.h +++ /dev/null @@ -1 +0,0 @@ -#include "../../../include/gpiod.h"
On 27-07-22, 13:17, Kent Gibson wrote: > On Wed, Jul 27, 2022 at 10:21:58AM +0530, Viresh Kumar wrote: > > On 27-07-22, 10:57, Kent Gibson wrote: > > > On Fri, Jul 08, 2022 at 05:04:54PM +0530, Viresh Kumar wrote: > > > > +fn main() { > > > > + let files = vec![ > > > > + "../../../lib/chip.c", > > > > + "../../../lib/chip-info.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", > > > > + ]; > > > > + > > > > + #[cfg(feature = "generate")] > > > > + generate_bindings(&files); > > > > + build_gpiod(files); > > > > +} > > > > > > Shouldn't bindings wrap libgpiod and dynamically link against it rather > > > than building and linking statically? > > > > There are few problems I faced, because of which I had to do it this way. > > > > - I couldn't find a way to do a "Make" for libgpiod from here and then link to > > the resultant library. > > > > - libgpiod may not be automatically installed in the environment where the end > > user of these Rust APIs exists. So I had to build it. > > > > - And then the API is changing a lot, maybe down the line once it is stable > > enough we can change this to something else. > > > > Sure, it is a problem, but static isn't the solution. > You should be able to get the appropriate paths from autoconf, but I would > refer you to Bart on that. I am still looking for some help on how to link this dynamically. FWIW, the problem is that the user crates, like vhost-device, will mention libgpiod as a dependency crate and likely won't have libgpiod installed in environment. So build.rs here needs to do some magic so the definitions are all available to the users.
On Mon, Aug 01, 2022 at 05:41:06PM +0530, Viresh Kumar wrote: > On 27-07-22, 13:17, Kent Gibson wrote: > > On Wed, Jul 27, 2022 at 10:21:58AM +0530, Viresh Kumar wrote: > > > On 27-07-22, 10:57, Kent Gibson wrote: > > > > On Fri, Jul 08, 2022 at 05:04:54PM +0530, Viresh Kumar wrote: > > > > > +fn main() { > > > > > + let files = vec![ > > > > > + "../../../lib/chip.c", > > > > > + "../../../lib/chip-info.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", > > > > > + ]; > > > > > + > > > > > + #[cfg(feature = "generate")] > > > > > + generate_bindings(&files); > > > > > + build_gpiod(files); > > > > > +} > > > > > > > > Shouldn't bindings wrap libgpiod and dynamically link against it rather > > > > than building and linking statically? > > > > > > There are few problems I faced, because of which I had to do it this way. > > > > > > - I couldn't find a way to do a "Make" for libgpiod from here and then link to > > > the resultant library. > > > > > > - libgpiod may not be automatically installed in the environment where the end > > > user of these Rust APIs exists. So I had to build it. > > > > > > - And then the API is changing a lot, maybe down the line once it is stable > > > enough we can change this to something else. > > > > > > > Sure, it is a problem, but static isn't the solution. > > You should be able to get the appropriate paths from autoconf, but I would > > refer you to Bart on that. > > I am still looking for some help on how to link this dynamically. > > FWIW, the problem is that the user crates, like vhost-device, will mention > libgpiod as a dependency crate and likely won't have libgpiod installed in > environment. So build.rs here needs to do some magic so the definitions are all > available to the users. > The Rust bindings themselves should be building against the local build tree, so well known relative paths. For users, require they have libgpiod installed and use pkg_config to locate it? Is that what you mean? Else, how do other Rust crates wrapping dynamic C libraries do it? Cheers, Kent.
On 01-08-22, 23:56, Kent Gibson wrote: > The Rust bindings themselves should be building against the local build > tree, so well known relative paths. Right, when we build everything with "make" we better use the already built library. I agree. > For users, require they have libgpiod installed and use pkg_config to > locate it? > > Is that what you mean? Since we need the latest APIs, we can't trust the packages provided by distributions for now. i.e. "apt-get install libgpiod-dev" won't install the latest stuff we need. The only other option to get it working in environments like rust-vmm-containers (which tests the vhost-device crate currently) is to build / install libgpiod first. As I have understood, people don't really like it there (Maintainers of rust-vmm-containers) as this will have further dependencies and require more tools. I even tried to generate the libgpiod-sys bindings on the fly first, but it required more tooling and there were issues with Musl build specifically. They suggested to use prebuild bindings as a solution, which I have now. What about we have two separate features: - "default" one will be used with "make" and will use prebuild library. - "generate" one will be used by user crates and we will build the files there like it is done now. > Else, how do other Rust crates wrapping dynamic C libraries do it? I think for standard libraries that are stable, the -sys crates just contain the pre-built bindings and wrappers and expect the library to be already installed.
On Tue, Aug 02, 2022 at 02:20:08PM +0530, Viresh Kumar wrote: > On 01-08-22, 23:56, Kent Gibson wrote: > > The Rust bindings themselves should be building against the local build > > tree, so well known relative paths. > > Right, when we build everything with "make" we better use the already > built library. I agree. > > > For users, require they have libgpiod installed and use pkg_config to > > locate it? > > > > Is that what you mean? > > Since we need the latest APIs, we can't trust the packages provided by > distributions for now. i.e. "apt-get install libgpiod-dev" won't > install the latest stuff we need. > For user builds with the Rust bindings you always require an appropriate version of libgpiod to be already installed, or it is the users problem to arrange that. For the time being that means it needs to be built and installed from source. I don't see an alternative to that, or a problem with it. In the longer term it will be provided by the packaging system for the platform. I may well be missing something, but I don't see the problem, at least nothing that requires addressing. Cheers, Kent. > The only other option to get it working in environments like > rust-vmm-containers (which tests the vhost-device crate currently) is > to build / install libgpiod first. As I have understood, people don't > really like it there (Maintainers of rust-vmm-containers) as this will > have further dependencies and require more tools. > > I even tried to generate the libgpiod-sys bindings on the fly first, > but it required more tooling and there were issues with Musl build > specifically. They suggested to use prebuild bindings as a solution, > which I have now. > > What about we have two separate features: > > - "default" one will be used with "make" and will use prebuild library. > > - "generate" one will be used by user crates and we will build the > files there like it is done now. > > > Else, how do other Rust crates wrapping dynamic C libraries do it? > > I think for standard libraries that are stable, the -sys crates just > contain the pre-built bindings and wrappers and expect the library to > be already installed. > > -- > viresh
diff --git a/.gitignore b/.gitignore index 58e1c5fc7e00..9541482d5efb 100644 --- a/.gitignore +++ b/.gitignore @@ -33,3 +33,8 @@ stamp-h1 # profiling *.gcda *.gcno + +# Added by cargo + +target +Cargo.lock diff --git a/bindings/rust/libgpiod-sys/Cargo.toml b/bindings/rust/libgpiod-sys/Cargo.toml new file mode 100644 index 000000000000..77f82719d269 --- /dev/null +++ b/bindings/rust/libgpiod-sys/Cargo.toml @@ -0,0 +1,15 @@ +[package] +name = "libgpiod-sys" +version = "0.1.0" +edition = "2018" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] + +[features] +generate = [ "bindgen" ] + +[build-dependencies] +bindgen = { version = "0.59.1", optional = true } +cc = "1.0.46" diff --git a/bindings/rust/libgpiod-sys/build.rs b/bindings/rust/libgpiod-sys/build.rs new file mode 100644 index 000000000000..bbcd30f79d23 --- /dev/null +++ b/bindings/rust/libgpiod-sys/build.rs @@ -0,0 +1,69 @@ +#[cfg(feature = "generate")] +extern crate bindgen; +#[cfg(feature = "generate")] +use std::env; +#[cfg(feature = "generate")] +use std::path::PathBuf; + +#[cfg(feature = "generate")] +fn generate_bindings(files: &Vec<&str>) { + // Tell cargo to invalidate the built crate whenever following files change + println!("cargo:rerun-if-changed=wrapper.h"); + + for file in files { + println!("cargo:rerun-if-changed={}", file); + } + + // 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(files: Vec<&str>) { + // Tell Cargo that if the given file changes, to rerun this build script. + println!("cargo:rerun-if-changed=../../../lib/"); + + // 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-sys\"") + .include("../../../include") + .compile("gpiod"); +} + +fn main() { + let files = vec![ + "../../../lib/chip.c", + "../../../lib/chip-info.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", + ]; + + #[cfg(feature = "generate")] + generate_bindings(&files); + build_gpiod(files); +} diff --git a/bindings/rust/libgpiod-sys/src/lib.rs b/bindings/rust/libgpiod-sys/src/lib.rs new file mode 100644 index 000000000000..3384863a567c --- /dev/null +++ b/bindings/rust/libgpiod-sys/src/lib.rs @@ -0,0 +1,20 @@ +// 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 { + #[cfg(feature = "generate")] + include!(concat!(env!("OUT_DIR"), "/bindings.rs")); + + #[cfg(not(feature = "generate"))] + include!("bindings.rs"); +} +pub use bindings_raw::*; diff --git a/bindings/rust/libgpiod-sys/wrapper.h b/bindings/rust/libgpiod-sys/wrapper.h new file mode 100644 index 000000000000..7bc1158b7d90 --- /dev/null +++ b/bindings/rust/libgpiod-sys/wrapper.h @@ -0,0 +1,2 @@ +#include <string.h> +#include "../../../include/gpiod.h"
This adds libgpiod-sys rust crate, which provides FFI (foreign function interface) bindings for libgpiod APIs. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- .gitignore | 5 ++ bindings/rust/libgpiod-sys/Cargo.toml | 15 ++++++ bindings/rust/libgpiod-sys/build.rs | 69 +++++++++++++++++++++++++++ bindings/rust/libgpiod-sys/src/lib.rs | 20 ++++++++ bindings/rust/libgpiod-sys/wrapper.h | 2 + 5 files changed, 111 insertions(+) create mode 100644 bindings/rust/libgpiod-sys/Cargo.toml create mode 100644 bindings/rust/libgpiod-sys/build.rs create mode 100644 bindings/rust/libgpiod-sys/src/lib.rs create mode 100644 bindings/rust/libgpiod-sys/wrapper.h