[Bug 2141070] New: Review Request: rust-yeslogic-fontconfig-sys - Raw bindings to Fontconfig without a vendored C library
https://bugzilla.redhat.com/show_bug.cgi?id=2141070
Bug ID: 2141070 Summary: Review Request: rust-yeslogic-fontconfig-sys - Raw bindings to Fontconfig without a vendored C library Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: klember@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://kalev.fedorapeople.org/rust-yeslogic-fontconfig-sys.spec SRPM URL: https://kalev.fedorapeople.org/rust-yeslogic-fontconfig-sys-2.11.2-1.fc38.sr... Description: Raw bindings to Fontconfig without a vendored C library.
Fedora Account System Username: kalev
Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=93951048
Note that there's a newer upstream version available, but I specifically packaged the older 2.11.2 for librsvg2 use as that's the version it requires. The newer version also adds a license file that we can package. I'll look into updating it once we get an initial librsvg2 build with non-bundled rust dependencies out.
https://bugzilla.redhat.com/show_bug.cgi?id=2141070
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |decathorpe@gmail.com CC| |decathorpe@gmail.com Flags| |fedora-review? Status|NEW |ASSIGNED
--- Comment #1 from Fabio Valentini decathorpe@gmail.com --- Taking this review.
Initial comments:
1. Remove the "without a vendored C library" part from Summary
It's nonsense for Fedora packages, as we wouldn't use vendored C sources anyway.
2. If the upstream project has added a license with newer versions, include that file manually in the package for now (i.e. as %SOURCE1 and cp it into the build directory in %prep).
3. Missing Requires in the -devel subpackage:
Requires: pkgconfig(fontconfig)
Is missing from the -devel subpackage.
If you don't add this, then the package might build fine, but using it in dependent packages will fail because BuildRequires are not transitive.
Once the package is imported, you can use a rust2rpm.conf file with these contents to automate this:
``` [DEFAULT] buildrequires = pkgconfig(fontconfig) lib.requires = pkgconfig(fontconfig) ```
4. The project's metadata claims it's MIT licensed, but headers in source code disagrees (MIT OR Apache-2.0).
The metadata seems to be wrong, since the crate looks like a fork of an originally "MIT OR Apache-2.0" licensed Servo sub-project.
https://bugzilla.redhat.com/show_bug.cgi?id=2141070
--- Comment #2 from Kalev Lember klember@redhat.com --- Thanks! I filed https://github.com/yeslogic/fontconfig-rs/issues/29 for the license issue.
https://bugzilla.redhat.com/show_bug.cgi?id=2141070
--- Comment #3 from Kalev Lember klember@redhat.com --- OK, upstream reply in the ticket above was that they are going to use only the MIT license going forward and remove all the references to Apache-2.0, so we can use that for the Fedora package as well.
I've fixed the other two packaging issues that you pointed out and left the licenses alone since we already listed MIT. Thanks for the rust2rpm.conf tip!
As for your suggestion of including the license text manually I don't think it's appropriate to do since our guidelines are clear to only include it if it's included in the source package (it's not in the 2.11.2 crate) or manually include a copy when the license requires a copy of the full license text to be included (MIT does not require that). It's going to get fixed anyway as soon as we update to a newer version where the license text is included.
* Tue Nov 15 2022 Kalev Lember klember@redhat.com 2.11.2-4 - Add runtime requires on fontconfig devel package (#2141070)
* Tue Nov 15 2022 Kalev Lember klember@redhat.com 2.11.2-3 - Re-generate the spec file with rust2rpm 23
* Tue Nov 15 2022 Kalev Lember klember@redhat.com 2.11.2-2 - Shorten description and summary
Spec URL: https://kalev.fedorapeople.org/rust-yeslogic-fontconfig-sys.spec SRPM URL: https://kalev.fedorapeople.org/rust-yeslogic-fontconfig-sys-2.11.2-4.fc38.sr...
Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=94222583
https://bugzilla.redhat.com/show_bug.cgi?id=2141070
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST Flags|fedora-review? |fedora-review+
--- Comment #4 from Fabio Valentini decathorpe@gmail.com --- Thanks for confirming with upstream that the intended license is MIT. Though I'm still unsure whether just relicensing a forked project like that is actually sound ... At least in this case, the license is "compatible" in the sense that it only takes away the Apache-2.0 choice.
As for your suggestion of including the license text manually I don't think it's appropriate to do since our guidelines are clear to only include it if it's included in the source package (it's not in the 2.11.2 crate) or manually include a copy when the license requires a copy of the full license text to be included (MIT does not require that). It's going to get fixed anyway as soon as we update to a newer version where the license text is included.
I'm very sure that you're misremembering things here. MIT is indeed one of the licenses where full license text in redistributed sources is required. Additionally, the Packaging Guidelines only forbid including license texts that are separate from *upstream*, but including the actual upstream license text is OK if it's not in the tarball but only in the git repo (as is the case here).
But other than that, the package looks good to me.
===
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 and is acceptable for Fedora ! license file is included with %license in %files (will be fixed) - package complies with Rust Packaging Guidelines
Package APPROVED.
===
Recommended post-import rust-sig tasks:
- add @rust-sig with "commit" access as package co-maintainer
- set bugzilla assignee overrides to @rust-sig (optional)
- 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
- track package in koschei for all built branches
https://bugzilla.redhat.com/show_bug.cgi?id=2141070
--- Comment #5 from Gwyn Ciesla gwync@protonmail.com --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-yeslogic-fontconfig-sys
https://bugzilla.redhat.com/show_bug.cgi?id=2141070
--- Comment #6 from Kalev Lember klember@redhat.com --- (In reply to Fabio Valentini from comment #4)
I'm very sure that you're misremembering things here. MIT is indeed one of the licenses where full license text in redistributed sources is required. Additionally, the Packaging Guidelines only forbid including license texts that are separate from *upstream*, but including the actual upstream license text is OK if it's not in the tarball but only in the git repo (as is the case here).
Oh, right you are. I'll fix that and add the license file before importing the package.
Thanks a lot (again!) for the review :)
https://bugzilla.redhat.com/show_bug.cgi?id=2141070
Kalev Lember klember@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |RAWHIDE Status|POST |CLOSED Fixed In Version| |rust-yeslogic-fontconfig-sy | |s-2.11.2-1.fc38 Last Closed| |2022-11-16 16:49:07
--- Comment #7 from Kalev Lember klember@redhat.com --- Package imported and built. (I managed to lose my git history during the process but I think it doesn't really matter.)
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com