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.