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.