| Message ID | cover.1667815011.git.viresh.kumar@linaro.org |
|---|---|
| Headers | show |
| Series | libgpiod: Add Rust bindings | expand |
On Mon, Nov 7, 2022 at 10:57 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > 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. > Kent, Miguel: if you are ok with this version - can you add your review tags? Bart
On Mon, Nov 14, 2022 at 10:49:36PM +0100, Bartosz Golaszewski wrote: > On Mon, Nov 7, 2022 at 10:57 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > 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. > > > > 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 <warthog618@gmail.com> Cheers, Kent.
On Mon, Nov 7, 2022 at 10:57 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > 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? Bart
On 16-11-22, 11:29, Bartosz Golaszewski wrote: > 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. > 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.
On Wed, Nov 16, 2022 at 11:29 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > 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
On Thu, Nov 17, 2022 at 9:12 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Wed, Nov 16, 2022 at 11:29 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > 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 Thanks Linus! Makes perfect sense. Bart
On Thu, Nov 17, 2022 at 8:31 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 16-11-22, 11:29, Bartosz Golaszewski wrote: > > 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. > Just use regular SPDX header at the top of the file in a # comment block? > - 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. > The main README.md just has a regular header at the top of the file. I'm fine with this. I guess the bottom of the file is good too. > - 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? When I was first flirting with implementing a dbus daemon for libgpiod, I used gdbus-codegen to generate dbus bindings from xml but I generated them at build-time instead of putting them into the repo. It would also shrink the release checklist for rust if we didn't have to manually regenerate them. Bart > > 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. > I think that's been clarified by Linus below. We should be good. Bartosz
On 17-11-22, 10:06, Bartosz Golaszewski wrote: > > 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 ? > > - 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 ?
On Thu, Nov 17, 2022 at 10:56 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 17-11-22, 10:06, Bartosz Golaszewski wrote: > > > 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 ? > > > > - 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. > 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? Bart > Maybe then add the header manually to them ? > > -- > viresh > > [1] https://github.com/rust-vmm/vhost-device
On 17-11-22, 11:18, Bartosz Golaszewski wrote: > Do these problems you faced apply to libgpiod too? I faced them with libgpiod only :( > Honestly, putting > automatically generated files in the repo just feels wrong. I agree, but ... > 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.
On Thu, Nov 17, 2022 at 11:40 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 17-11-22, 11:18, Bartosz Golaszewski wrote: > > Do these problems you faced apply to libgpiod too? > > I faced them with libgpiod only :( > > > Honestly, putting > > automatically generated files in the repo just feels wrong. > > I agree, but ... > > > 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? Bart
> On Thu, Nov 17, 2022 at 10:56 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > 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? If the library is stable enough, then removing a heavy build dependency on bindgen may be worth it. But yeah, by default I would avoid it unless one is sure it is possible. Though, if one can store it because it is fixed, at that point one may take advantage of that and manually write (or tweak) the bindings instead. In any case, I would explain the rationale one way or the other in the commit message or ideally in the `README.MD`. 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. Cheers, Miguel
On 17-11-22, 11:48, Bartosz Golaszewski wrote: > 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.
On 17-11-22, 11:51, Miguel Ojeda wrote: > 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. gpiosim ? Maybe yeah, it should be gpiosim-sys too.
On Thu, Nov 17, 2022 at 11:55 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 17-11-22, 11:48, Bartosz Golaszewski wrote: > > 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? Bart
On Thu, Nov 17, 2022 at 10:56 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 17-11-22, 10:06, Bartosz Golaszewski wrote: > > > > 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/20220516102615.884180377@linutronix.de/ Cheers, Miguel
On Thu, Nov 17, 2022 at 12:07 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Thu, Nov 17, 2022 at 10:56 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 17-11-22, 10:06, Bartosz Golaszewski wrote: > > > > > > 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/20220516102615.884180377@linutronix.de/ > > Cheers, > Miguel Ha! I wasn't even aware of this spdxexclude and spdxcheck mechanism. What does spdxcheck do better than reuse lint other than having its own ignore list? I'm not sure I want to import it into libgpiod though. Reuse docs say: --- You probably will have files in your project that you do not find particularly copyrightable, for example configuration files such as .gitignore. Intuitively you may not want to license these files, but the fundamental idea of REUSE is that all your files will clearly have their copyright and licensing marked. --- So I'd say - just use CC0-1.0 license in Cargo.toml? Bart
On 17-11-22, 12:05, Bartosz Golaszewski wrote: > 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. > 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.
On 17-11-22, 12:15, Bartosz Golaszewski wrote:
> 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"
On Thu, Nov 17, 2022 at 12:25 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 17-11-22, 12:15, Bartosz Golaszewski wrote: > > 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. Bart
On Thu, Nov 17, 2022 at 12:15 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 17-11-22, 12:05, Bartosz Golaszewski wrote: > > 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. > If build-time is the only issue then I vote for automatic generation at build-time and not storing those files. > > 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. > Leaving space for maintainer errors when preparing releases I suppose. I don't know what the reason is for the Rust world to go that way but more often than not, files that are generated automatically are not checked in into repositories. This is my experience from working with Qt, GLib or the linux kernel (flex and bison files generated by kconfig). > 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. > Yes, if there's no show-stopper then let's generate the files at build-time which also fixes the licensing issue. Bart
On Thu, Nov 17, 2022 at 04:55:17PM +0530, Viresh Kumar wrote: > On 17-11-22, 12:15, Bartosz Golaszewski wrote: > > 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" > That is the license for the resulting crate, not the Cargo.toml file itself. And they aren't necessarily the same. Using CC0-1.0 for config and documentation makes sense to me. Cheers, Kent.
On 17-11-22, 13:18, Bartosz Golaszewski wrote:
> Then let's just add the SPDX identifier on top to make reuse happy.
This looks fine ?
diff --git a/bindings/rust/Cargo.toml b/bindings/rust/Cargo.toml
index 4fdf4e06ff90..85abe70b4aa5 100644
--- a/bindings/rust/Cargo.toml
+++ b/bindings/rust/Cargo.toml
@@ -1,7 +1,12 @@
+# SPDX-License-Identifier: CC0-1.0
+#
+# Copyright 2022 Linaro Ltd. All Rights Reserved.
+# Viresh Kumar <viresh.kumar@linaro.org>
+
[workspace]
diff --git a/bindings/rust/libgpiod-sys/README.md b/bindings/rust/libgpiod-sys/README.md
index ecf75b31c41e..567891a30868 100644
--- a/bindings/rust/libgpiod-sys/README.md
+++ b/bindings/rust/libgpiod-sys/README.md
@@ -1,11 +1,15 @@
+# SPDX-License-Identifier: CC0-1.0
+#
+# Copyright 2022 Linaro Ltd. All Rights Reserved.
+# Viresh Kumar <viresh.kumar@linaro.org>
+
# Generated libgpiod-sys Rust FFI bindings
On Fri, Nov 18, 2022 at 10:36 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 17-11-22, 13:18, Bartosz Golaszewski wrote: > > Then let's just add the SPDX identifier on top to make reuse happy. > > This looks fine ? > > diff --git a/bindings/rust/Cargo.toml b/bindings/rust/Cargo.toml > index 4fdf4e06ff90..85abe70b4aa5 100644 > --- a/bindings/rust/Cargo.toml > +++ b/bindings/rust/Cargo.toml > @@ -1,7 +1,12 @@ > +# SPDX-License-Identifier: CC0-1.0 > +# > +# Copyright 2022 Linaro Ltd. All Rights Reserved. > +# Viresh Kumar <viresh.kumar@linaro.org> > + Should be: # SPDX-License-Identifier: CC0-1.0 # SPDX-FileCopyrightText: 2022 Linaro Ltd. # SPDX-FileCopyrightTest: 2022 Viresh Kumar <viresh.kumar@linaro.org> > [workspace] > > > diff --git a/bindings/rust/libgpiod-sys/README.md b/bindings/rust/libgpiod-sys/README.md > index ecf75b31c41e..567891a30868 100644 > --- a/bindings/rust/libgpiod-sys/README.md > +++ b/bindings/rust/libgpiod-sys/README.md > @@ -1,11 +1,15 @@ > +# SPDX-License-Identifier: CC0-1.0 > +# > +# Copyright 2022 Linaro Ltd. All Rights Reserved. > +# Viresh Kumar <viresh.kumar@linaro.org> > + > # Generated libgpiod-sys Rust FFI bindings > > -- > viresh
On 18-11-22, 10:38, Bartosz Golaszewski wrote: > Should be: > > # SPDX-License-Identifier: CC0-1.0 > # SPDX-FileCopyrightText: 2022 Linaro Ltd. > # SPDX-FileCopyrightTest: 2022 Viresh Kumar <viresh.kumar@linaro.org> For all files ? Or just Cargo.toml and README.md files ?
On Fri, Nov 18, 2022 at 10:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 18-11-22, 10:38, Bartosz Golaszewski wrote: > > Should be: > > > > # SPDX-License-Identifier: CC0-1.0 > > # SPDX-FileCopyrightText: 2022 Linaro Ltd. > > # SPDX-FileCopyrightTest: 2022 Viresh Kumar <viresh.kumar@linaro.org> > > For all files ? Or just Cargo.toml and README.md files ? > All files. We're using unified SPFX-FileCopyrightText instead of custom notices. Bart
On Wed, 16 Nov 2022 09:12:01 +0800 Kent Gibson <warthog618@gmail.com> wrote: > On Mon, Nov 14, 2022 at 10:49:36PM +0100, Bartosz Golaszewski wrote: > > On Mon, Nov 7, 2022 at 10:57 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > 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. > > > > > > > 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 <warthog618@gmail.com> > > 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? Best, Gary
On Fri, Nov 18, 2022 at 04:44:56PM +0000, Gary Guo wrote: > On Wed, 16 Nov 2022 09:12:01 +0800 > Kent Gibson <warthog618@gmail.com> wrote: > > > On Mon, Nov 14, 2022 at 10:49:36PM +0100, Bartosz Golaszewski wrote: > > > On Mon, Nov 7, 2022 at 10:57 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > > > 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. > > > > > > > > > > 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 <warthog618@gmail.com> > > > > 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. > Or you can use `OsStr::to_string_lossy()` though that requires allocation. str is preferable as it is easer to use, and 99.99% of the time will be fine. I have toyed with using OsStr, but having to do the conversion becomes really tedious really fast. > 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? > That is what I meant by "an extreme corner case". The problem with the GPIO uAPI is that it limits names to 32 bytes, and valid UTF-8 passed in may get truncated mid-codepoint, resulting in invalid UTF-8 when read back. Handling that case by truncating the string before the split codepoint would at least ensure that the Rust API would round-trip. Cheers, Kent.
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