Bug 2066040 - Review Request: rust-libadwaita-sys - FFI bindings for libadwaita
Summary: Review Request: rust-libadwaita-sys - FFI bindings for libadwaita
Keywords:
Status: CLOSED DUPLICATE of bug 2141725
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Fabio Valentini
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: FE-NEEDSPONSOR
Blocks: 2066122
TreeView+ depends on / blocked
 
Reported: 2022-03-20 07:24 UTC by yuan
Modified: 2022-11-10 16:04 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2022-11-10 16:04:19 UTC
Type: ---
Embargoed:
decathorpe: fedora-review+


Attachments (Terms of Use)

Comment 1 Fabio Valentini 2022-03-21 16:32:19 UTC
Packaging looks good to me, in theory.

You can check out how the missing license file is handled in other packages, like here:
https://src.fedoraproject.org/rpms/rust-libspa-sys/blob/rawhide/f/rust-libspa-sys.spec
(You're already doing something very similar, but the "fetch from cargo" comment doesn't make any sense. If the published crate contains a LICENSE file, it is automatically included by rust2rpm, no need to "fetch" anything.)

You might also want to use a rust2rpm config file like this one:
https://src.fedoraproject.org/rpms/rust-pipewire-sys/blob/rawhide/f/.rust2rpm.conf
(replace "pkgconfig(pipewire)" with "pkgconfig(libadwaita-1)", obviously)
This will automatically insert the BuildRequires / Requires into the .spec file when you use rust2rpm.

Comment 2 yuan 2022-03-21 19:19:33 UTC
Updated:

Spec URL: https://download.copr.fedorainfracloud.org/results/zhangyuannie/rust-libadwaita-sys/fedora-rawhide-x86_64/03810674-rust-libadwaita-sys/rust-libadwaita-sys.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/zhangyuannie/rust-libadwaita-sys/fedora-rawhide-x86_64/03810674-rust-libadwaita-sys/rust-libadwaita-sys-0.1.0-1.fc37.src.rpm
Description: FFI bindings for libadwaita
Fedora Account System Username: zhangyuannie

Thanks for pointing me to the right direction, I have updated the missing license handling to be more idiomatic.

Yeah, that comment was confusing. I meant to say the published crate does not contain a LICENSE file, as such it must be manually added, for now. We should use it from the crate once upstream published a new version.

That's awesome, I have updated my setup to use a .rust2rpm.conf

Comment 3 Fabio Valentini 2022-04-30 10:02:43 UTC
Looks like upstream has released version 0.1.1 which contains a LICENSE file.

Comment 4 yuan 2022-04-30 16:22:40 UTC
(In reply to Fabio Valentini from comment #3)
> Looks like upstream has released version 0.1.1 which contains a LICENSE file.

That is for rust-libadwaita. rust-libadwaita-sys hasn't receive a new release yet. I plan to update rust-libadwaita.spec when rust-libadwaita-sys.spec went through.

Comment 5 Fabio Valentini 2022-05-01 14:03:29 UTC
> That is for rust-libadwaita.

Oh, you're right. Sorry, I looked at the wrong one. Adding it manually is fine for now.
Have you reported 

I wonder, what are you trying to package that requires the Rust libadwaita bindings?

Just a minor improvement left: Use
%license %{crate_instdir}/LICENCE
instead of
%license LICENCE

This matches what rust2rpm will generate for you once the LICENCE file is present in the published crate itself.

===

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
- package complies with Rust Packaging Guidelines

Package APPROVED.

===

You'll still need to find a sponsor, I think?

Comment 6 yuan 2022-05-02 23:46:42 UTC
Thank you!

> I wonder, what are you trying to package that requires the Rust libadwaita bindings?

My personal goal would be to package https://github.com/zhangyuannie/butter when it is ready.
We have rust-gtk4 in our repo but not rust-libadwaita, which is a bit weird as most apps that depend on rust-gtk4 also depend on rust-libadwaita as well (Fractal, Shortwave, etc).

> Just a minor improvement left: Use
> %license %{crate_instdir}/LICENCE
> instead of
> %license LICENCE

I may be wrong here, but I think that would also mean that we have to cp LICENCE into %{crate_instdir} first since LICENCE is not in the crate?
I have not tried this yet but I am not sure if adding files into %{crate_instdir} will have any side effects.
Maybe it might be better to stay like this for now?
Once upstream releases a new version, rerunning rust2rpm should fix everything.

> You'll still need to find a sponsor, I think?

Yes!

Comment 7 Fabio Valentini 2022-05-04 11:10:37 UTC
> I may be wrong here, but I think that would also mean that we have to cp LICENCE into %{crate_instdir} first since LICENCE is not in the crate?

You're both right and wrong. ;)

In this case, %cargo_install will copy all files that are included in the crate into %{crate_instdir} anyway, so as long as you do "cp %{SOURCE1} ." in %prep it will end up in %{crate_instdir} either way. Specifying it with just "%license LICENSE" then will only result in two copies of the file being included in the RPM, which is why we changed rust2rpm to generate "%license %{crate_instdir}/LICENSE" instead.

Comment 8 yuan 2022-05-13 03:00:21 UTC
(In reply to Fabio Valentini from comment #7)
Sorry for the delay. Thank you for the explanation! I have updated the spec (https://copr-dist-git.fedorainfracloud.org/cgit/zhangyuannie/rust-libadwaita-sys/rust-libadwaita-sys.git/commit/?id=24dc00ddf90a1288269cd883e23a830b91f5a731).

Comment 9 Fabio Valentini 2022-05-13 08:47:53 UTC
Great, thanks!

Have you tried to reach out to potential sponsors?

Comment 10 Alessio 2022-08-27 06:42:41 UTC
Butter looks promising.
Any news on this?

Comment 11 Fabio Valentini 2022-08-27 10:41:52 UTC
Packaging the libadwaita bindings should be relatively straightforward.

But it doesn't look like zhangyuannie still hasn't found a sponsor / mentor yet.
And I cannot commit the time necessary to do this myself right now, sorry.

The next steps would probably be for zhangyuannie to do some non-binding / unofficial package reviews so potential sponsors can see that they've done their homework (please correct me if this has already happened, but I cannot see links to any of those reviews on this ticket or the one for rust-libadwaita).

Comment 12 Kalev Lember 2022-11-09 06:13:00 UTC
What can we do to move this along? Fabio, is there any chance you could sponsor the package submitter since you're the rust domain expert in Fedora and you've already worked with the submitter?

Alternatively, can we close this review request (and also rust-libadwaita) and I re-submit both myself so that we could get the libadwaita bindings finally into Fedora?

Comment 13 Fabio Valentini 2022-11-09 16:03:27 UTC
I'm sorry, but I cannot commit myself to any more responsibilities right now.
I don't want to sponsor new packagers at this time, given that I wouldn't have the necessary time for mentoring a new packager.
Assuming the original submitter is fine with you continuing their work, feel free to submit a new review request, .

Comment 14 Kalev Lember 2022-11-09 16:12:26 UTC
Fair enough. Thanks anyway!

zhangyuannie, would you be ok if I try to move this along and submit the packages under my own name as I'm already a packager?

Comment 15 yuan 2022-11-10 04:50:02 UTC
(In reply to Kalev Lember from comment #14)
> Fair enough. Thanks anyway!
> 
> zhangyuannie, would you be ok if I try to move this along and submit the
> packages under my own name as I'm already a packager?

Please go ahead! And thanks for taking care of this. :D

Comment 16 Kalev Lember 2022-11-10 16:04:19 UTC
OK, thanks! I went ahead and opened a new ticket for rust-libadwaita-sys: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=2141725

Fabio, do you want to +1 it since you've already reviewed it here?

*** This bug has been marked as a duplicate of bug 2141725 ***


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