Bug 2224960 - Review Request: rust-input-sys - Bindgen generated unsafe libinput wrapper
Summary: Review Request: rust-input-sys - Bindgen generated unsafe libinput wrapper
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Fabio Valentini
QA Contact: Fedora Extras Quality Assurance
URL: https://crates.io/crates/input-sys
Whiteboard:
Depends On:
Blocks: 2224807 2224961
TreeView+ depends on / blocked
 
Reported: 2023-07-24 04:54 UTC by Davide Cavalca
Modified: 2023-08-07 01:26 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2023-07-27 16:33:02 UTC
Type: ---
Embargoed:
decathorpe: fedora-review+


Attachments (Terms of Use)
The .spec file difference from Copr build 6208027 to 6215304 (3.26 KB, patch)
2023-07-26 23:50 UTC, Fedora Review Service
no flags Details | Diff

Description Davide Cavalca 2023-07-24 04:54:30 UTC
Spec URL: https://dcavalca.fedorapeople.org/review/rust-input-sys/rust-input-sys.spec
SRPM URL: https://dcavalca.fedorapeople.org/review/rust-input-sys/rust-input-sys-1.17.0-1.fc39.src.rpm

Description:
Bindgen generated unsafe libinput wrapper.

Fedora Account System Username: dcavalca

Comment 1 Davide Cavalca 2023-07-24 04:54:32 UTC
This package built on koji:  https://koji.fedoraproject.org/koji/taskinfo?taskID=103811534

Comment 2 Fabio Valentini 2023-07-24 12:32:01 UTC
I don't really understand what you're trying to accomplish with the Cargo.toml patch, it's likely not what you want.

Removing the versions-specific features is going to create problems, since you're basically turning off all bindings that are version dependent.
This is usually handled differently (like here in glib-sys: https://src.fedoraproject.org/rpms/rust-glib-sys/blob/rawhide/f/rust2rpm.conf).

Comment 3 Davide Cavalca 2023-07-24 15:55:49 UTC
It's totally possible I misunderstood things, but here's what I was trying to do. tiny-dfr (which is what I'm ultimately trying to package here) needs input which needs input-sys. input-sys provides versioned bindings for a bunch of libinput versions, but _not_ for the one currently in Fedora (1.23). It also provides a feature to generate the bindings instead of using prebuilt ones, so I figured making that the default (and adjusting input accordingly to make it the default) would result in things being more maintainable. Lemme try the glib-sys approach instead, thanks.

Comment 5 Davide Cavalca 2023-07-24 16:08:57 UTC
For the record, this is the rust2rpm.conf that I used here:

[DEFAULT]
buildrequires =
  pkgconfig(libinput)
lib.requires =
  pkgconfig(libinput)
lib+libinput_1_11.requires =
  pkgconfig(libinput) >= 1.11
lib+libinput_1_14.requires =
  pkgconfig(libinput) >= 1.14
lib+libinput_1_15.requires =
  pkgconfig(libinput) >= 1.15
lib+libinput_1_19.requires =
  pkgconfig(libinput) >= 1.19
lib+libinput_1_21.requires =
  pkgconfig(libinput) >= 1.21

Comment 6 Fedora Review Service 2023-07-24 16:16:13 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6208027
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2224960-rust-input-sys/fedora-rawhide-x86_64/06208027-rust-input-sys/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 7 Fabio Valentini 2023-07-25 12:52:01 UTC
(In reply to Davide Cavalca from comment #3)
> It's totally possible I misunderstood things, but here's what I was trying
> to do. tiny-dfr (which is what I'm ultimately trying to package here) needs
> input which needs input-sys. input-sys provides versioned bindings for a
> bunch of libinput versions, but _not_ for the one currently in Fedora
> (1.23). It also provides a feature to generate the bindings instead of using
> prebuilt ones, so I figured making that the default (and adjusting input
> accordingly to make it the default) would result in things being more
> maintainable. Lemme try the glib-sys approach instead, thanks.

Ah, looking into the crate's source code, I see why you did what you did now. This crate is doing things in a quite unusual way - they ship pre-generated bindings for different libinput versions and architectures. This is certainly not what we want ... so your previous approach of removing those features and using bindgen to force regenerating the bindings for the current architecture + version should be much better (not even sure if using pregenerated bindings would work, since they're missing for ppc64le and s390x). Sorry about the confusion!

(The reason I thought this was weird is that usually the features for versioned bindings are additive (i.e. only add bindings for things that have been added compared to previous versions), so using the latest one is fine (since it should usually imply all the previous ones). This is not the case for this crate, because they ship pre-generated bindings for different versions, and use different files depending on the feature flags that were passed.)

Comment 9 Davide Cavalca 2023-07-26 23:43:01 UTC
This is the rust2rpm.conf I used:

[DEFAULT]
buildrequires =
  pkgconfig(libinput)
  pkgconfig(libudev)
lib.requires =
  pkgconfig(libinput)
  pkgconfig(libudev)
unwanted-features =
  libinput_1_11
  libinput_1_14
  libinput_1_15
  libinput_1_19
  libinput_1_21

Comment 10 Fedora Review Service 2023-07-26 23:50:17 UTC
Created attachment 1980201 [details]
The .spec file difference from Copr build 6208027 to 6215304

Comment 11 Fedora Review Service 2023-07-26 23:50:19 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6215304
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2224960-rust-input-sys/fedora-rawhide-x86_64/06215304-rust-input-sys/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 12 Fabio Valentini 2023-07-27 09:54:16 UTC
Package was generated with rust2rpm, simplifying the review.

- package builds and installs without errors on rawhide
- test suite is run and all unit tests pass
- latest version of the crate is packaged
- license matches upstream specification (MIT) and is acceptable for Fedora
~ license file is included with %license in %files (included separately from upstream for now)
- package complies with Rust Packaging Guidelines

Package APPROVED.

===

Recommended post-import rust-sig tasks:

- set up package on release-monitoring.org:
  project: $crate
  homepage: https://crates.io/crates/$crate
  backend: crates.io
  version scheme: semantic
  version filter: alpha;beta;rc;pre
  distro: Fedora
  Package: rust-$crate

- add @rust-sig with "commit" access as package co-maintainer
  (should happen automatically)

- set bugzilla assignee overrides to @rust-sig (optional)

- track package in koschei for all built branches
  (should happen automatically once rust-sig is co-maintainer)

===

Looks good to me, with one exception:

s/# FIXME: no license files detected/%license %{crate_instdir}/LICENSE/

Though I'm confused why the crate includes copies of the libinput header files instead of using the standard "run bindgen against a wrapper header file that just includes what you want" trick ... at least the header files are MIT as well, so it's not a problem from that point of view. But the whole crate is very weird from a -sys bindings perspective. I'm not sure that it works as intended, but that might be a question for upstream.

Comment 13 Fedora Admin user for bugzilla script actions 2023-07-27 15:45:37 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-input-sys

Comment 14 Fedora Update System 2023-07-27 16:31:04 UTC
FEDORA-2023-0cc36570c9 has been submitted as an update to Fedora 39. https://bodhi.fedoraproject.org/updates/FEDORA-2023-0cc36570c9

Comment 15 Fedora Update System 2023-07-27 16:33:02 UTC
FEDORA-2023-0cc36570c9 has been pushed to the Fedora 39 stable repository.
If problem still persists, please make note of it in this bug report.

Comment 16 Fedora Update System 2023-08-06 21:01:10 UTC
FEDORA-2023-c2d333f341 has been submitted as an update to Fedora 38. https://bodhi.fedoraproject.org/updates/FEDORA-2023-c2d333f341

Comment 17 Fedora Update System 2023-08-07 01:26:50 UTC
FEDORA-2023-c2d333f341 has been pushed to the Fedora 38 stable repository.
If problem still persists, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.