libgpiod: Add Rust bindings

This adds libgpiod-sys rust crate, which provides FFI (foreign function interface) bindings for libgpiod APIs.

Signed-off-by Viresh Kumar <[email protected]>

---
 .gitignore                            |  5 ++++
 bindings/rust/Cargo.toml              |  5 ++++
 bindings/rust/libgpiod-sys/Cargo.toml | 20 +++++++++++++
 bindings/rust/libgpiod-sys/build.rs   | 41 +++++++++++++++++++++++++++
 bindings/rust/libgpiod-sys/src/lib.rs | 13 +++++++++
 5 files changed, 84 insertions(+)
 create mode 100644 bindings/rust/Cargo.toml
 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

.gitignore +0 -0

index 6c08415b390d..9f2fcf440c5d 100644

Quoted 17 lines · Viresh Kumar

This adds libgpiod-sys rust crate, which provides FFI (foreign function interface) bindings for libgpiod APIs.

Signed-off-by Viresh Kumar <[email protected]>

---
 .gitignore                            |  5 ++++
 bindings/rust/Cargo.toml              |  5 ++++
 bindings/rust/libgpiod-sys/Cargo.toml | 20 +++++++++++++
 bindings/rust/libgpiod-sys/build.rs   | 41 +++++++++++++++++++++++++++
 bindings/rust/libgpiod-sys/src/lib.rs | 13 +++++++++
 5 files changed, 84 insertions(+)
 create mode 100644 bindings/rust/Cargo.toml
 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

.gitignore stamp-h1 · line 42

Quoted 13 lines

.gitignore +5 -0

index 6c08415b390d..9f2fcf440c5d 100644
@@ -35,3 +35,8 @@ stamp-h1
 # profiling
 *.gcda
 *.gcno
+
+# Added by cargo
+
+target
+Cargo.lock

This should be put into bindings/rust/.gitignore

Quoted 18 lines · Bartosz Golaszewski

Quoted 17 lines · Viresh Kumar

This adds libgpiod-sys rust crate, which provides FFI (foreign function interface) bindings for libgpiod APIs.

Signed-off-by Viresh Kumar <[email protected]>

---
 .gitignore                            |  5 ++++
 bindings/rust/Cargo.toml              |  5 ++++
 bindings/rust/libgpiod-sys/Cargo.toml | 20 +++++++++++++
 bindings/rust/libgpiod-sys/build.rs   | 41 +++++++++++++++++++++++++++
 bindings/rust/libgpiod-sys/src/lib.rs | 13 +++++++++
 5 files changed, 84 insertions(+)
 create mode 100644 bindings/rust/Cargo.toml
 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

Quoted 15 lines

.gitignore stamp-h1 · line 42

Quoted 13 lines

.gitignore +5 -0

index 6c08415b390d..9f2fcf440c5d 100644
@@ -35,3 +35,8 @@ stamp-h1
 # profiling
 *.gcda
 *.gcno
+
+# Added by cargo
+
+target
+Cargo.lock

This should be put into bindings/rust/.gitignore

Should I resend the series with this one change ? Or just an incremental fix is enough ?

--
viresh

bindings/rust/libgpiod/examples/gpioset.rs new file +41 -0

new file mode 100644
index 000000000000..f72a623ab28c
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause
+//
+// Copyright 2022 Linaro Ltd. All Rights Reserved.
+//     Viresh Kumar <[email protected]>
+//
+// Simplified Rust implementation of the gpioset tool.
+
+use std::env;
+use std::io::{stdin, Read};
+
+use libgpiod::{
+    chip::Chip,
+    line::{self, Direction, Offset, SettingVal, Value},
+    request, Error, Result,
+};
+
+fn usage(name: &str) {
+    println!("Usage: {} <chip> <line_offset0>=<value0> ...", name);
+}
+
+fn main() -> Result<()> {
+    let args: Vec<String> = env::args().collect();
+    if args.len() < 3 {
+        usage(&args[0]);
+        return Err(Error::InvalidArguments);
+    }
+
+    let lconfig = line::Config::new()?;
+
+    for arg in &args[2..] {
+        let pair: Vec<&str> = arg.split('=').collect();
+        if pair.len() != 2 {
+            usage(&args[0]);
+            return Err(Error::InvalidArguments);
+        }
+
+        let offset = pair[0]
+            .parse::<Offset>()
+            .map_err(|_| Error::InvalidArguments)?;
+        let value = pair[1]
+            .parse::<i32>()

I'm looking at it and thinking that it would look much better as:

let settings = line::Settings::new()

    .set_direction(Direction::Output)
    .set_output_value(Value::new(value))
    .build()?;

settings would not need to be mutable (we'd have some intermediate SettingsBuilder object?) and could be directly passed to add_line_settings()?

We now have chained mutators for LineSettings in C++ and keyword arguments in Python - somehow I think that the former suits rust much better than passing an array of properties.

Quoted 7 lines · Bartosz Golaszewski

I'm looking at it and thinking that it would look much better as:

let settings = line::Settings::new()

    .set_direction(Direction::Output)
    .set_output_value(Value::new(value))
    .build()?;

Quoted 7 lines

settings would not need to be mutable (we'd have some intermediate SettingsBuilder object?) and could be directly passed to add_line_settings()?

We now have chained mutators for LineSettings in C++ and keyword arguments in Python - somehow I think that the former suits rust much better than passing an array of properties.

We already support chained mutators in the Rust bindings. This example can also be written as:

         let mut lsettings = line::Settings::new()?;

         lsettings
             .set_direction(Direction::Output)?
             .set_output_value(Value::new(value)?)?;

--
viresh

Ah, I missed that, my bad.

Would it also work in non-mutable way like

let lsettings = line::Settings::new.set_direction(Direction::Output)?;

Add tests for the rust bindings, quite similar to the ones in cxx bindings.

Signed-off-by Viresh Kumar <[email protected]>

Quoted 4 lines · Viresh Kumar

Add tests for the rust bindings, quite similar to the ones in cxx bindings.

Quoted 1 line

Signed-off-by Viresh Kumar <[email protected]>

Without modifying udev rules on my system - is there a one-liner I could use to launch the tests? I guess using cargo as sudo is wrong but the executables are there so can I somehow run them all at once as if running cargo test?

Quoted 5 lines · Bartosz Golaszewski

Quoted 4 lines · Viresh Kumar

Add tests for the rust bindings, quite similar to the ones in cxx bindings.

Quoted 7 lines

Quoted 1 line

Signed-off-by Viresh Kumar <[email protected]>

Without modifying udev rules on my system - is there a one-liner I could use to launch the tests? I guess using cargo as sudo is wrong but the executables are there so can I somehow run them all at once as if running cargo test?

I use:

sudo /home/dev/.cargo/bin/cargo test

so using the cargo installed in the dev account on my test machine.

That way I can build, clippy and generate docs etc as dev, but require sudo for the tests.

I would love to know how to do it without the sudo, but as you suggest, that would probably require some udev magic or something to set the configfs and sysfs file permissions, as well as the gpiochips themselves, and given the above works for me I've not had sufficient incentive to look into it.

Quoted 5 lines · Viresh Kumar

Add tests for the rust bindings, quite similar to the ones in cxx bindings.

Signed-off-by Viresh Kumar <[email protected]>

Without modifying udev rules on my system - is there a one-liner I could use to launch the tests? I guess using cargo as sudo is wrong but the executables are there so can I somehow run them all at once as if running cargo test?

Quoted 4 lines · On Mon, Nov 7, 2022 at 10:58 AM Viresh Kumar [email protected] wrote:

Add tests for the rust bindings, quite similar to the ones in cxx bindings.

Signed-off-by Viresh Kumar [email protected]

Without modifying udev rules on my system - is there a one-liner I could use to launch the tests? I guess using cargo as sudo is wrong but the executables are there so can I somehow run them all at once as if running cargo test?

Bartosz

Hello,

Here is another version of the rust bindings, based of the master branch.

Pushed here:

https://github.com/vireshk/libgpiod v9

V8->V9:
- Merged the last patch (supporting Events) with the other patches.
- Events implementation is simplified and made efficient. nth() is also implemented for the iterator.
- Unnecessary comment removed from Cargo.toml files.
- Updated categories in libgpiod's Cargo.toml.
- Updated gpio_events example to show cloned events live past another read_edge_events().
- Implement AsRawFd for Chip.
- Other minor changes.

Quoted 9 lines · Viresh Kumar

Hello,

Here is another version of the rust bindings, based of the master branch.

Pushed here:

https://github.com/vireshk/libgpiod v9

Quoted 11 lines

V8->V9:
- Merged the last patch (supporting Events) with the other patches.
- Events implementation is simplified and made efficient. nth() is also implemented for the iterator.
- Unnecessary comment removed from Cargo.toml files.
- Updated categories in libgpiod's Cargo.toml.
- Updated gpio_events example to show cloned events live past another read_edge_events().
- Implement AsRawFd for Chip.
- Other minor changes.

Kent, Miguel: if you are ok with this version - can you add your review tags?

Bart

Quoted 22 lines · Bartosz Golaszewski

Quoted 9 lines · Viresh Kumar

Hello,

Here is another version of the rust bindings, based of the master branch.

Pushed here:

https://github.com/vireshk/libgpiod v9

Quoted 11 lines

V8->V9:
- Merged the last patch (supporting Events) with the other patches.
- Events implementation is simplified and made efficient. nth() is also implemented for the iterator.
- Unnecessary comment removed from Cargo.toml files.
- Updated categories in libgpiod's Cargo.toml.
- Updated gpio_events example to show cloned events live past another read_edge_events().
- Implement AsRawFd for Chip.
- Other minor changes.

Quoted 3 lines

Kent, Miguel: if you are ok with this version - can you add your review tags?

Bart

As mentioned elsewhere, I'm a bit iffy about the handling of non-UTF-8 names, which are treated as errors, but are valid in the C API. But that is an extreme corner case that can be addressed later, so I have no objection to this version being merged.

Reviewed-by Kent Gibson <[email protected]>

Cheers,
Kent.

Quoted 34 lines · On Wed, 16 Nov 2022 09:12:01 +0800 Kent Gibson <[email protected]> wrote:

Quoted 22 lines · Bartosz Golaszewski

Quoted 9 lines · Viresh Kumar

Hello,

Here is another version of the rust bindings, based of the master branch.

Pushed here:

https://github.com/vireshk/libgpiod v9

Quoted 11 lines

V8->V9:
- Merged the last patch (supporting Events) with the other patches.
- Events implementation is simplified and made efficient. nth() is also implemented for the iterator.
- Unnecessary comment removed from Cargo.toml files.
- Updated categories in libgpiod's Cargo.toml.
- Updated gpio_events example to show cloned events live past another read_edge_events().
- Implement AsRawFd for Chip.
- Other minor changes.

Quoted 3 lines

Kent, Miguel: if you are ok with this version - can you add your review tags?

Bart

As mentioned elsewhere, I'm a bit iffy about the handling of non-UTF-8 names, which are treated as errors, but are valid in the C API. But that is an extreme corner case that can be addressed later, so I have no objection to this version being merged.

Reviewed-by Kent Gibson <[email protected]>

Quoted 2 lines

Cheers,
Kent.

I have glanced through the code and I find no reason that the str used couldn't be replaced with [u8] or OsStr. The former might be a little bit difficult to use, but OsStr could be easily converted into str by just .to_str().expect("name should be utf-8") if the user don't care about non-UTF-8 or is certain that names are indeed UTF-8.

I am not sure about whether this would be worthwhile though, because I doubt if anyone is actually using non-UTF-8 strings in those places. Non-ASCII usages should already be rare?

Hello,

Here is another version of the rust bindings, based of the master branch.

Pushed here:

https://github.com/vireshk/libgpiod v9

V8->V9:
- Merged the last patch (supporting Events) with the other patches.
- Events implementation is simplified and made efficient. nth() is also implemented for the iterator.
- Unnecessary comment removed from Cargo.toml files.
- Updated categories in libgpiod's Cargo.toml.
- Updated gpio_events example to show cloned events live past another read_edge_events().
- Implement AsRawFd for Chip.
- Other minor changes.

V7->V8:
- Several updates to cargo.toml files, like license, version, etc.
- Removed Sync support for chip and gpiosim.
- Implemented, in a separate patch, iterator support for Events.
- Fixed missing SAFETY comments.
- Fixed build for 32 bit systems.
- Use errno::Errno.
- Removed Clone derive for many structures, that store raw pointers.
- line setting helpers return the object back, so another helper can be called directly on them. Also made all helpers public and used the same in tests and example for single configurations.
- Enums for gpiosim constants.
- New examples to demonstrate parallelism and event handling.
- Separated out HTE tests and marked as #[ignore] now.
- Updated commit subjects.
- Other minor changes.

V6->V7:
- Don't let buffer read new events if the earlier events are still referenced.
- BufferIntenal is gone now, to make the above work.
- Update example and tests too for the same.

V5->V6:
- Updates according to the new line-settings interface.
- New file, line_settings.rs.
- Renamed 'enum Setting' as 'SettingVal' to avoid conflicting names, as we also have 'struct Settings' now.
- Support for HTE clock type.
- Implement 'Eq' for public structure/enums (reported by build).
- Remove 'SettingKindMap' and 'SettingMap' as they aren't required anymore.
- Updated tests based on new interface.

V4->V5:
- Arrange as workspace with crates for libgpiod-sys, libgpiod, gpiosim.
- Use static libgpiod and libgpiosim libraries instead of rebuilding again.
- Arrange in modules instead of flattened approach.
- New enums like Setting and SettingKind and new types based on them SettingMap and SettingKindMap.
- New property independent helpers for line_config, like set_prop_default().
- Improved tests/examples, new example for gpiowatch.
- Add pre-built bindings for gpiosim too.
- Many other changes.

V3->V4:
- Rebased on top of new changes, and made changes accordingly.
- Added rust integration tests with gpiosim.
- Found a kernel bug with tests, sent a patch for that to LKML.

V2->V3:
- Remove naming redundancy, users just need to do this now
use libgpiod:{Chip, Direction, LineConfig} now (Bartosz);
- Fix lifetime issues between event-buffer and edge-event modules, the event buffer is released after the last edge-event reference is dropped (Bartosz).
- Allow edge-event to be copied, and freed later (Bartosz).
- Add two separate rust crates, sys and wrapper (Gerard).
- Null-terminate the strings passed to libgpiod (Wedson).
- Drop unnecessary checks to validate string returned from chip:name/label/path.
- Fix SAFETY comments (Wedson).
- Drop unnecessary clone() instances (Bartosz).

V1->V2:
- Added examples (I tested everything except gpiomon.rs, didn't have right hardware/mock device to test).
- Build rust bindings as part of Make, update documentation.

Thanks.

--
Viresh

Viresh Kumar (8):
bindings: rust: Add libgpiod-sys rust crate
bindings: rust: Add pre generated bindings for libgpiod-sys
bindings: rust: Add gpiosim crate
bindings: rust: Add pre generated bindings for gpiosim
bindings: rust: Add libgpiod crate
bindings: rust: Add examples to libgpiod crate
bindings: rust: Add tests for libgpiod crate
bindings: rust: Integrate building of bindings with make

 .gitignore                                    |    5 +
 README                                        |    8 +-
 TODO                                          |    8 -
 bindings/Makefile.am                          |    6 +
 bindings/rust/Cargo.toml                      |    7 +
 bindings/rust/Makefile.am                     |   18 +
 bindings/rust/gpiosim/Cargo.toml              |   22 +
 bindings/rust/gpiosim/README.md               |   11 +
 bindings/rust/gpiosim/build.rs                |   43 +
 bindings/rust/gpiosim/src/bindings.rs         |  180 +++
 bindings/rust/gpiosim/src/lib.rs              |   79 ++
 bindings/rust/gpiosim/src/sim.rs              |  331 +++++
 bindings/rust/libgpiod-sys/Cargo.toml         |   20 +
 bindings/rust/libgpiod-sys/README.md          |   11 +
 bindings/rust/libgpiod-sys/build.rs           |   41 +
 bindings/rust/libgpiod-sys/src/bindings.rs    | 1173 +++++++++++++++++
 bindings/rust/libgpiod-sys/src/lib.rs         |   13 +
 bindings/rust/libgpiod/Cargo.toml             |   21 +
 .../rust/libgpiod/examples/gpio_events.rs     |   89 ++
 .../examples/gpio_threaded_info_events.rs     |  133 ++
 bindings/rust/libgpiod/examples/gpiodetect.rs |   31 +
 bindings/rust/libgpiod/examples/gpiofind.rs   |   37 +
 bindings/rust/libgpiod/examples/gpioget.rs    |   46 +
 bindings/rust/libgpiod/examples/gpioinfo.rs   |   98 ++
 bindings/rust/libgpiod/examples/gpiomon.rs    |   75 ++
 bindings/rust/libgpiod/examples/gpioset.rs    |   64 +
 bindings/rust/libgpiod/examples/gpiowatch.rs  |   54 +
 bindings/rust/libgpiod/src/chip.rs            |  310 +++++
 bindings/rust/libgpiod/src/edge_event.rs      |   93 ++
 bindings/rust/libgpiod/src/event_buffer.rs    |  167 +++
 bindings/rust/libgpiod/src/info_event.rs      |   69 +
 bindings/rust/libgpiod/src/lib.rs             |  479 +++++++
 bindings/rust/libgpiod/src/line_config.rs     |  135 ++
 bindings/rust/libgpiod/src/line_info.rs       |  162 +++
 bindings/rust/libgpiod/src/line_request.rs    |  227 ++++
 bindings/rust/libgpiod/src/line_settings.rs   |  297 +++++
 bindings/rust/libgpiod/src/request_config.rs  |   95 ++
 bindings/rust/libgpiod/tests/chip.rs          |   98 ++
 bindings/rust/libgpiod/tests/common/config.rs |  143 ++
 bindings/rust/libgpiod/tests/common/mod.rs    |   10 +
 bindings/rust/libgpiod/tests/edge_event.rs    |  298 +++++
 bindings/rust/libgpiod/tests/info_event.rs    |  167 +++
 bindings/rust/libgpiod/tests/line_config.rs   |   96 ++
 bindings/rust/libgpiod/tests/line_info.rs     |  276 ++++
 bindings/rust/libgpiod/tests/line_request.rs  |  510 +++++++
 bindings/rust/libgpiod/tests/line_settings.rs |  204 +++
 .../rust/libgpiod/tests/request_config.rs     |   39 +
 configure.ac                                  |   16 +
 48 files changed, 6504 insertions(+), 11 deletions(-)
 create mode 100644 bindings/rust/Cargo.toml
 create mode 100644 bindings/rust/Makefile.am
 create mode 100644 bindings/rust/gpiosim/Cargo.toml
 create mode 100644 bindings/rust/gpiosim/README.md
 create mode 100644 bindings/rust/gpiosim/build.rs
 create mode 100644 bindings/rust/gpiosim/src/bindings.rs
 create mode 100644 bindings/rust/gpiosim/src/lib.rs
 create mode 100644 bindings/rust/gpiosim/src/sim.rs
 create mode 100644 bindings/rust/libgpiod-sys/Cargo.toml
 create mode 100644 bindings/rust/libgpiod-sys/README.md
 create mode 100644 bindings/rust/libgpiod-sys/build.rs
 create mode 100644 bindings/rust/libgpiod-sys/src/bindings.rs
 create mode 100644 bindings/rust/libgpiod-sys/src/lib.rs
 create mode 100644 bindings/rust/libgpiod/Cargo.toml
 create mode 100644 bindings/rust/libgpiod/examples/gpio_events.rs
 create mode 100644 bindings/rust/libgpiod/examples/gpio_threaded_info_events.rs
 create mode 100644 bindings/rust/libgpiod/examples/gpiodetect.rs
 create mode 100644 bindings/rust/libgpiod/examples/gpiofind.rs
 create mode 100644 bindings/rust/libgpiod/examples/gpioget.rs
 create mode 100644 bindings/rust/libgpiod/examples/gpioinfo.rs
 create mode 100644 bindings/rust/libgpiod/examples/gpiomon.rs
 create mode 100644 bindings/rust/libgpiod/examples/gpioset.rs
 create mode 100644 bindings/rust/libgpiod/examples/gpiowatch.rs
 create mode 100644 bindings/rust/libgpiod/src/chip.rs
 create mode 100644 bindings/rust/libgpiod/src/edge_event.rs
 create mode 100644 bindings/rust/libgpiod/src/event_buffer.rs
 create mode 100644 bindings/rust/libgpiod/src/info_event.rs
 create mode 100644 bindings/rust/libgpiod/src/lib.rs
 create mode 100644 bindings/rust/libgpiod/src/line_config.rs
 create mode 100644 bindings/rust/libgpiod/src/line_info.rs
 create mode 100644 bindings/rust/libgpiod/src/line_request.rs
 create mode 100644 bindings/rust/libgpiod/src/line_settings.rs
 create mode 100644 bindings/rust/libgpiod/src/request_config.rs
 create mode 100644 bindings/rust/libgpiod/tests/chip.rs
 create mode 100644 bindings/rust/libgpiod/tests/common/config.rs
 create mode 100644 bindings/rust/libgpiod/tests/common/mod.rs
 create mode 100644 bindings/rust/libgpiod/tests/edge_event.rs
 create mode 100644 bindings/rust/libgpiod/tests/info_event.rs
 create mode 100644 bindings/rust/libgpiod/tests/line_config.rs
 create mode 100644 bindings/rust/libgpiod/tests/line_info.rs
 create mode 100644 bindings/rust/libgpiod/tests/line_request.rs
 create mode 100644 bindings/rust/libgpiod/tests/line_settings.rs
 create mode 100644 bindings/rust/libgpiod/tests/request_config.rs

--
2.31.1.272.g89b43f80a514

Hi Viresh,

There are some licensing issues I noticed now: can you make sure reuse lint doesn't return errors for rust bindings?

One other thing is the license of the rust bindings themselves - I'm not a lawyer but it seems to me that if you link against LGPL code statically, your code must be licensed under an LGPL-compatible license. It seems that BSD-3-Clause and Apache-2.0 are compatible but it would be great to have someone knowledgeable comment on that. Is there anyone at linaro we could contact?

Quoted 4 lines · Bartosz Golaszewski

Hi Viresh,

There are some licensing issues I noticed now: can you make sure reuse lint doesn't return errors for rust bindings?

I have fixed couple of those now, but there are few which I am not sure about.

# MISSING COPYRIGHT AND LICENSING INFORMATION

The following files have no copyright and licensing information:
* ../../bindings/rust/Cargo.toml
* ../../bindings/rust/gpiosim/Cargo.toml
* ../../bindings/rust/gpiosim/README.md
* ../../bindings/rust/gpiosim/src/bindings.rs
* ../../bindings/rust/libgpiod/Cargo.toml
* ../../bindings/rust/libgpiod-sys/Cargo.toml
* ../../bindings/rust/libgpiod-sys/README.md
* ../../bindings/rust/libgpiod-sys/src/bindings.rs

File types:
- Cargo.toml

Most of these have a different style for versioning, though the workspace specific files doesn't have a version set. I checked few other projects and they didn't mention it as well.

- README.md

Here also version is mentioned differently (added now), based on how I found it elsewhere, i.e. towards the bottom of the file.

- bindings.rs

These are automatically genrated files, with bindgen. Not sure if we should edit them to add Licensing info.

Quoted 6 lines

One other thing is the license of the rust bindings themselves - I'm not a lawyer but it seems to me that if you link against LGPL code statically, your code must be licensed under an LGPL-compatible license. It seems that BSD-3-Clause and Apache-2.0 are compatible but it would be great to have someone knowledgeable comment on that. Is there anyone at linaro we could contact?

Hmm, not sure. Cc'ing Arnd, in case he can help.

Quoted 7 lines

File types:
- Cargo.toml

Most of these have a different style for versioning, though the workspace specific files doesn't have a version set. I checked few other projects and they didn't mention it as well.

Just use regular SPDX header at the top of the file in a # comment block?

Quoted 5 lines

- bindings.rs

These are automatically genrated files, with bindgen. Not sure if we should edit them to add Licensing info.

Can we not generate them at build-time then?

Quoted 9 lines · Bartosz Golaszewski

Quoted 7 lines

File types:
- Cargo.toml

Most of these have a different style for versioning, though the workspace specific files doesn't have a version set. I checked few other projects and they didn't mention it as well.

Just use regular SPDX header at the top of the file in a # comment block?

Miguel ? Kent ?

Quoted 7 lines

Quoted 5 lines

- bindings.rs

These are automatically genrated files, with bindgen. Not sure if we should edit them to add Licensing info.

Can we not generate them at build-time then?

We can, and we do when we do "cargo build --features=generate".

There are reasons to keep them merged though. It lets the users avoid generating them on the go, like the vhost-device [1] crate in my case. I also faced issues with the rust-vmm containers, where vhost-device stuff gets auto-tested, in enabling bindgen support and finally went back to keeping them generated in advance. This is also the normal practice it seems, from whatever I have seen from the rust-vmm community at least.

Maybe then add the header manually to them ?

--
viresh

[1] https://github.com/rust-vmm/vhost-device

Do these problems you faced apply to libgpiod too?

Honestly, putting
automatically generated files in the repo just feels wrong.

It defeats
the whole purpose of code generation. If we can't reliably regenerate them, then it sounds like a problem with the tools, not the library. Maybe we don't need to worry about that just yet?

Quoted 1 line · Bartosz Golaszewski

Do these problems you faced apply to libgpiod too?

I faced them with libgpiod only :(

Quoted 2 lines

Honestly, putting
automatically generated files in the repo just feels wrong.

I agree, but ...

Quoted 4 lines

It defeats
the whole purpose of code generation. If we can't reliably regenerate them, then it sounds like a problem with the tools, not the library. Maybe we don't need to worry about that just yet?

it isn't about reliability of the generated code, but making everyone do it, even if they don't need to.

Also, the code generated here is Rust code wrappers and other declarations, which let us call the C helpers eventually. It can be considered like hand written code here, for the argument that it is automatically generated stuff. Just that we have a tool (bindgen) here which lets us generate it automatically, without introducing bugs.

Anyway, I am fine with whatever you decide.

Let me propose a different solution. When preparing release tarballs with autotools, certain files are generated automatically that go into the tarball but are never part of the repository. This way developers don't have to deal with the automatically generated files polluting the repo while end-users get a tarball with less dependencies and reproducible results.

It's called the dist-local hook in automake.

Does it sound like something we could use here?

Quoted 10 lines · Bartosz Golaszewski

Let me propose a different solution. When preparing release tarballs with autotools, certain files are generated automatically that go into the tarball but are never part of the repository. This way developers don't have to deal with the automatically generated files polluting the repo while end-users get a tarball with less dependencies and reproducible results.

It's called the dist-local hook in automake.

Does it sound like something we could use here?

These auto-generated bindings are used only by the code present in bindings/rust/libgpiod/src/, and are never visible to end user code.

The end user will use the new interface provided by the libgpiod Rust bindings instead and that works over the web and will be picked from libgpiod's git tree or crates.io (later).

Not sure how the tarball solution will work here.

So it already only impacts developers exclusively and not users who'd for example want to install libgpiod from crates.io? If so then I don't really see a reason to keep those files in the repo really.

I'm not familiar with rust-vmm containers but the fact that bindgen support is missing or causes problems sounds like a problem with rust-vmm and not users of bindgen, right?

Quoted 3 lines · Bartosz Golaszewski

So it already only impacts developers exclusively and not users who'd for example want to install libgpiod from crates.io? If so then I don't really see a reason to keep those files in the repo really.

Users are impacted in the sense that they will be forced to build the bindings using bindgen now, automatically of course. It is an extra, time consuming, operation which can be avoided. For rust-vmm projects, every pull request results in fresh rebuild of the entire crate, which would mean two additional bindgen builds too, just for gpio now. It isn't a huge problem, but it is time that could have been saved.

Quoted 3 lines

I'm not familiar with rust-vmm containers but the fact that bindgen support is missing or causes problems sounds like a problem with rust-vmm and not users of bindgen, right?

Yeah it can be, but IIUC, in the Rust world, more often than not such bindings are pre-generated, as this basically is Rust code only. These bindings will only change if we make a change to the gpiod API.

Perhaps that's why I was asked to avoid generating the bindings there, but I can ask again and try fixing the rust-vmm containers if it doesn't work.

Do these problems you faced apply to libgpiod too? Honestly, putting automatically generated files in the repo just feels wrong. It defeats the whole purpose of code generation. If we can't reliably regenerate them, then it sounds like a problem with the tools, not the library. Maybe we don't need to worry about that just yet?

One thing I may have missed: why is there a -sys crate for one of the bindings but not the other? It may be a good idea to document the intention as well.

Quoted 2 lines · Bartosz Golaszewski

Just use regular SPDX header at the top of the file in a # comment block?

Miguel ? Kent ?

Quoted 6 lines · Viresh Kumar

Quoted 2 lines · Bartosz Golaszewski

Just use regular SPDX header at the top of the file in a # comment block?

Miguel ? Kent ?

In the kernel at least, all files (including docs) should have an SPDX line.

However, configuration files which use a publicly documented format (things like dot files) are not copyrightable apparently, and it was proposed that the scripts/spdxexclude kernel script excludes those [1], but the patch has not been merged (yet?) since I raised a couple questions about how to handle those (sorry...).

So I am not sure if e.g. Cargo.toml should have a SPDX license identifier or not.

[1] https://lore.kernel.org/lkml/[email protected]/

Cheers,
Miguel

So I'd say - just use CC0-1.0 license in Cargo.toml?

Quoted 1 line · Bartosz Golaszewski

So I'd say - just use CC0-1.0 license in Cargo.toml?

The Cargo.toml files already have following currently:

license = "Apache-2.0 OR BSD-3-Clause"

Then let's just add the SPDX identifier on top to make reuse happy.

Quoted 1 line · Bartosz Golaszewski

Then let's just add the SPDX identifier on top to make reuse happy.

This looks fine ?

bindings/rust/Cargo.toml +0 -0

index 4fdf4e06ff90..85abe70b4aa5 100644

bindings/rust/Cargo.toml +0 -0

 [workspace]
 
 

bindings/rust/libgpiod-sys/README.md +5 -1

index ecf75b31c41e..567891a30868 100644
@@ -1,11 +1,15 @@
+# SPDX-License-Identifier: CC0-1.0
+#
+# Copyright 2022 Linaro Ltd. All Rights Reserved.
+#     Viresh Kumar <[email protected]>
+
 # Generated libgpiod-sys Rust FFI bindings
 
--

viresh

Should be:

# SPDX-License-Identifier: CC0-1.0
# SPDX-FileCopyrightText: 2022 Linaro Ltd.
# SPDX-FileCopyrightTest: 2022 Viresh Kumar <[email protected]>

Quoted 5 lines · Bartosz Golaszewski

Should be:

# SPDX-License-Identifier: CC0-1.0
# SPDX-FileCopyrightText: 2022 Linaro Ltd.
# SPDX-FileCopyrightTest: 2022 Viresh Kumar <[email protected]>

For all files ? Or just Cargo.toml and README.md files ?

Quoted 1 line · Bartosz Golaszewski

So I'd say - just use CC0-1.0 license in Cargo.toml?

The Cargo.toml files already have following currently:

license = "Apache-2.0 OR BSD-3-Clause"

One other thing is the license of the rust bindings themselves - I'm not a lawyer but it seems to me that if you link against LGPL code statically, your code must be licensed under an LGPL-compatible license.

Quoted 4 lines · Bartosz Golaszewski

One other thing is the license of the rust bindings themselves - I'm not a lawyer but it seems to me that if you link against LGPL code statically, your code must be licensed under an LGPL-compatible license.

Nope. The LGPL was created exactly for clarifying and avoiding that situation.

https://en.wikipedia.org/wiki/GNU_Lesser_General_Public_License

It is a common misunderstanding that GPL overall has anything to do with whether you link things this or that way, the legal term used is "derivative work" and the meaning of that can only be determined in court. The meaning can depend on the intent of the author and misc legal ramifications.

In many ways LGPL is unnecessary, but it was created exactly to make non-legal people less weary about situations relating to linking of libraries.

It is fine to link an LGPL statically into whatever software, but one needs to provide header files and linkable static objects (.a files) of the library to the user.

Yours,
Linus Walleij