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. -- viresh [1] https://rust-lang.github.io/rust-bindgen/tutorial-2.html