Re: [PATCH V4 1/8] libgpiod: Add libgpiod-sys rust crate

[Kent Gibson <warthog618@xxxxxxxxx>]

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.