Bug 2235088 - Review Request: rust-libheif-rs - Safe wrapper around the libheif-sys crate for parsing heif/heic files
Summary: Review Request: rust-libheif-rs - Safe wrapper around the libheif-sys crate f...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Fabio Valentini
QA Contact: Fedora Extras Quality Assurance
URL: https://crates.io/crates/libheif-rs
Whiteboard:
Depends On: 2234760 2234761
Blocks:
TreeView+ depends on / blocked
 
Reported: 2023-08-26 11:53 UTC by Kalev Lember
Modified: 2023-09-21 21:12 UTC (History)
2 users (show)

Fixed In Version: rust-libheif-rs-0.21.0-2.fc40
Clone Of:
Environment:
Last Closed: 2023-09-21 21:12:39 UTC
Type: ---
Embargoed:
decathorpe: fedora-review+


Attachments (Terms of Use)

Description Kalev Lember 2023-08-26 11:53:52 UTC
Spec URL: https://kalev.fedorapeople.org/rust-libheif-rs.spec
SRPM URL: https://kalev.fedorapeople.org/rust-libheif-rs-0.20.0-1.fc40.src.rpm
Description:
Safe wrapper around the libheif-sys crate for parsing heif/heic files.

Fedora Account System Username: kalev

Comment 1 Fedora Review Service 2023-08-26 11:56:50 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6345574
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2235088-rust-libheif-rs/fedora-rawhide-x86_64/06345574-rust-libheif-rs/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
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 2 Fabio Valentini 2023-08-28 12:55:42 UTC
Package looks good to me, with one exception:

There's test images in the /data/ directory. It's unclear to me where they come from or if they're freely redistributable.
They can be dropped from the package, because tests are not run anyway. But I'm not sure if we'd need a "clean" tarball as well.
If the pictures are *not* redistributable, they should likely be excluded from published crates by upstream (or not committed to git at all). :(

Comment 3 Kalev Lember 2023-08-28 13:15:20 UTC
Thanks Fabio! Let me ask upstream.

Comment 5 Fabio Valentini 2023-09-18 10:01:08 UTC
Looks like the two blocking issues are resolved in the latest version?

Comment 6 Kalev Lember 2023-09-20 16:07:02 UTC
Indeed, and I apparently even had updated locally, but forgot to update the review request:

Spec URL: https://kalev.fedorapeople.org/rust-libheif-rs.spec
SRPM URL: https://kalev.fedorapeople.org/rust-libheif-rs-0.21.0-1.fc40.src.rpm

Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=106439272

There's even newer, 0.22.0 release available from upstream now, but that requires rust-libheif-sys update first, so I've left it out for now to simplify things. Either version should be fine for glycin-loaders.

One thing I am unsure about is how to handle the test data licensing and if we should reflect it in the license tag. Upstream cleaned the test data up nicely and it's all under CC-BY-SA-4.0 now, but didn't reflect the CC-BY-SA-4.0 addition in the crate license. My gut feeling is that this is the right thing to do, because whatever we put in the license field gets included in the license field of the packages that consume the rust crate (e.g. glycin-loaders), and they don't use the test data (and CC-BY-SA-4.0 license) at all. Maybe an option would be to exclude the test data in %files list?

Comment 7 Fabio Valentini 2023-09-21 15:49:28 UTC
This looks good to me, thanks! 

Yes, you can use an %exclude line in the %files list to exclude the test data.
In that case, the license also doesn't have to be listed in the License tag.
Marking the CC-By-SA-4.0 license text as %license is not strictly necessary in this case, I think, but it doesn't hurt either.
However, it would be good to have a comment in the spec file to explain the situation.

===

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.

===

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)

Comment 8 Fabio Valentini 2023-09-21 15:50:06 UTC
Ah, forgot to change this:

-- latest version of the crate is packaged
+! latest version of the crate is packaged (with explanation)

Comment 9 Kalev Lember 2023-09-21 19:28:49 UTC
Thanks! OK, I'll go with the %exclude and not listing the test data in the license tag then.

Comment 10 Fedora Admin user for bugzilla script actions 2023-09-21 19:29:34 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-libheif-rs

Comment 11 Kalev Lember 2023-09-21 21:12:39 UTC
Package imported and built.


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