Bug 2179161 - Review Request: rust-gst-plugin-reqwest - GStreamer reqwest HTTP Source Plugin
Summary: Review Request: rust-gst-plugin-reqwest - GStreamer reqwest HTTP Source Plugin
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kalev Lember
QA Contact: Fedora Extras Quality Assurance
URL: https://crates.io/crates/gst-plugin-r...
Whiteboard:
Depends On: 2179159
Blocks:
TreeView+ depends on / blocked
 
Reported: 2023-03-16 20:10 UTC by Fabio Valentini
Modified: 2023-04-19 21:22 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2023-04-19 21:22:38 UTC
Type: ---
Embargoed:
klember: fedora-review+


Attachments (Terms of Use)

Description Fabio Valentini 2023-03-16 20:10:30 UTC
Spec URL: https://decathorpe.fedorapeople.org/rust-gst-plugin-reqwest.spec
SRPM URL: https://decathorpe.fedorapeople.org/rust-gst-plugin-reqwest-0.10.4-1.fc38.src.rpm

Description:
GStreamer reqwest HTTP Source Plugin.

Fedora Account System Username: decathorpe

Comment 1 Jakub Kadlčík 2023-03-16 20:13:50 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/5652514
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2179161-rust-gst-plugin-reqwest/fedora-rawhide-x86_64/05652514-rust-gst-plugin-reqwest/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 Kalev Lember 2023-04-06 07:48:37 UTC
Taking for review.

Two things stand out to me here. First, the licensing:

> # (Apache-2.0 OR MIT) AND BSD-3-Clause
> # 0BSD OR MIT OR Apache-2.0
> # Apache-2.0
> # Apache-2.0 OR BSL-1.0
> # Apache-2.0 OR MIT
> # MIT
> # MIT OR Apache-2.0
> # MIT OR Apache-2.0 OR Zlib
> # MIT OR Zlib OR Apache-2.0
> # Unlicense OR MIT
> # Zlib OR Apache-2.0 OR MIT
> License:        Apache-2.0 AND BSD-3-Clause AND MIT

My understanding of the new licensing guidelines is that this is not how we are supposed to fill out the License: field. The license field is supposed to be a simple conjunction of all the sub-licenses involved and should not contain further simplifications the way you've done here. See https://docs.fedoraproject.org/en-US/legal/license-field/#_no_effective_license_analysis and the rest of the page.

This should be
License: ((Apache-2.0 OR MIT) AND BSD-3-Clause) AND (0BSD OR MIT OR Apache-2.0) AND Apache-2.0 AND (Apache-2.0 OR BSL-1.0) AND (Apache-2.0 OR MIT) AND MIT AND (MIT OR Apache-2.0) AND (MIT OR Apache-2.0 OR Zlib) AND (MIT OR Zlib OR Apache-2.0) AND (Unlicense OR MIT) AND (Zlib OR Apache-2.0 OR MIT)

See also recent discussion on the legal list, https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/thread/F4MYD7U6D2ROAL3CAOHSYDL3H6TPWZOT/


Secondly, I noticed that the gstreamer plugin binary package is called 'gst-plugin-reqwest'. Existing gstreamer plugins in Fedora use the 'gstreamer1-plugin(s)-$plugin' pattern and I think it would make sense to continue with this here and call the subpackage 'gstreamer1-plugin-reqwest'.

Comment 3 Fabio Valentini 2023-04-06 15:01:06 UTC
(In reply to Kalev Lember from comment #2)
> Taking for review.
> 
> Two things stand out to me here. First, the licensing:
> 
> > # (Apache-2.0 OR MIT) AND BSD-3-Clause
> > # 0BSD OR MIT OR Apache-2.0
> > # Apache-2.0
> > # Apache-2.0 OR BSL-1.0
> > # Apache-2.0 OR MIT
> > # MIT
> > # MIT OR Apache-2.0
> > # MIT OR Apache-2.0 OR Zlib
> > # MIT OR Zlib OR Apache-2.0
> > # Unlicense OR MIT
> > # Zlib OR Apache-2.0 OR MIT
> > License:        Apache-2.0 AND BSD-3-Clause AND MIT
> 
> My understanding of the new licensing guidelines is that this is not how we
> are supposed to fill out the License: field. The license field is supposed
> to be a simple conjunction of all the sub-licenses involved and should not
> contain further simplifications the way you've done here. See
> https://docs.fedoraproject.org/en-US/legal/license-field/
> #_no_effective_license_analysis and the rest of the page.
> 
> This should be
> License: ((Apache-2.0 OR MIT) AND BSD-3-Clause) AND (0BSD OR MIT OR
> Apache-2.0) AND Apache-2.0 AND (Apache-2.0 OR BSL-1.0) AND (Apache-2.0 OR
> MIT) AND MIT AND (MIT OR Apache-2.0) AND (MIT OR Apache-2.0 OR Zlib) AND
> (MIT OR Zlib OR Apache-2.0) AND (Unlicense OR MIT) AND (Zlib OR Apache-2.0
> OR MIT)

Not really. This is not documented yet, but "(A OR B)" and ("B OR A") are equivalent, so at least those can be deduplicated.

> See also recent discussion on the legal list,
> https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/
> thread/F4MYD7U6D2ROAL3CAOHSYDL3H6TPWZOT/

Yes, I have followed this discussion. However, at least "(Apache-2.0 OR MIT) AND BSD-3-Clause" AND "(Apache-2.0 OR MIT)" are idempotent and can be reduced to "(Apache-2.0 OR MIT) AND BSD-3-Clause". The "AND" operator is associative (i.e. "(A AND B) AND C" is the same as "A AND (B AND C)" and hence can be simplified to "A AND B AND C" (and here, A and C are identical, so one of them can be dropped).

There was also this:

"""
    because we are stubbornly adhering to the view that it is useful to
    reflect all disjunctive license expressions (if only because this was
    a convention in the Callaway system).
"""

Which sounds like there's not a logical basis to this guidance at all :)
Anyway, I can yeet the complete string into the License tag. Not sure if that helps anybody, but oh well.

> Secondly, I noticed that the gstreamer plugin binary package is called
> 'gst-plugin-reqwest'. Existing gstreamer plugins in Fedora use the
> 'gstreamer1-plugin(s)-$plugin' pattern and I think it would make sense to
> continue with this here and call the subpackage 'gstreamer1-plugin-reqwest'.

I disagree. This is against the Naming Guidelines, and there is no documented exception for GStreamer plugins:
https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_addon_packages

I can add "Provides: gstreamer1-plugin-reqwest" to the "gst-plugin-reqwest" subpackage to make it easier to find for users, but I would prefer to have the name of the package match the upstream project. (There's also already the "gst-devtools" and "gst-editing-services" packages, so it wouldn't be the first package that follows this pattern).

Comment 4 Kalev Lember 2023-04-06 20:20:46 UTC
(In reply to Fabio Valentini from comment #3)
> Anyway, I can yeet the complete string into the License tag. Not sure if
> that helps anybody, but oh well.

Yes, probably best to follow the licensing guidelines as they are right now :) On a positive side, having a super verbose license tag like this should make it easy to generate it from cargo2rpm. For what it's worth, I agree with you, but the guidelines are what they are.

> > Secondly, I noticed that the gstreamer plugin binary package is called
> > 'gst-plugin-reqwest'. Existing gstreamer plugins in Fedora use the
> > 'gstreamer1-plugin(s)-$plugin' pattern and I think it would make sense to
> > continue with this here and call the subpackage 'gstreamer1-plugin-reqwest'.
> 
> I disagree. This is against the Naming Guidelines, and there is no
> documented exception for GStreamer plugins:
> https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/
> #_addon_packages

How is it against the Naming Guidelines? The second sentence from your link says "The new package ("child") SHOULD prepend the "parent" package in its name, in the format: %{parent}-%{child}."

In this case, the parent is 'gstreamer1' so it would be exactly as the Naming Guidelines say if we call it 'gstreamer1-plugin-reqwest'.

If you want to make it even more clear, we could update the Naming Guidelines and add a section about gstreamer1. Might avoid confusion in the future.

> I can add "Provides: gstreamer1-plugin-reqwest" to the "gst-plugin-reqwest"
> subpackage to make it easier to find for users, but I would prefer to have
> the name of the package match the upstream project. (There's also already
> the "gst-devtools" and "gst-editing-services" packages, so it wouldn't be
> the first package that follows this pattern).

Yes, sadly "gst-devtools" and "gst-editing-services" got through the review process with inconsistent names :( I don't think that's a reason to follow it here though. We could maybe try to fix the naming for these two to bring them back in line with the rest.

As long as the parent package is called gstreamer1 (and doesn't follow upstream naming), I really think reqwest and all other addon packages should use the same pattern. You could always add 'Provides: gst-plugin-reqwest' there if you want to make it discoverable through the upstream name :)

Comment 5 Neal Gompa 2023-04-06 20:33:27 UTC
I would prefer the actual binary package to be "gstreamer1-plugin-reqwest" to be consistent with the other gst-1.0 plugins. 

I also agree we should go back and fix gst-devtools and gst-editing-services.

The naming consistency is important for being able to identify and use GStreamer components.

I could have sworn we had a guideline about GStreamer packages before, but apparently we don't. it'd be worth getting it documented though, as we *do* try to keep things consistent there.

Comment 6 Neal Gompa 2023-04-06 20:34:51 UTC
I will also point out that *all* other GStreamer components from the GStreamer project go through this "gst-" -> "gstreamer1-" change too. So it's very intentional that we do so.

Comment 7 Fabio Valentini 2023-04-07 11:11:14 UTC
Can one of you submit a PR for the Packaging Guidelines to document this?

Comment 8 Fabio Valentini 2023-04-18 14:18:10 UTC
I've updated the files at the original URLs with two changes:

- License tag contains no more simplifications (other than the "allowed" ones, for example that "(A OR B)" and "(B OR A)" are equivalent).
- Added "Provides: gstreamer1-plugin-reqwest = %{version}-%{release}" to the gst-plugin-reqwest subpackage.

If you want that latter one to be the other way round (i.e. "subpackage name = gstreamer1-plugin-reqwest", and "provides = gst-plugin-reqwest"), then please go through FPC to document this exception to the Packaging Guidelines (i.e. add a section for GStreamer plugins here: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_addon_packages).

Comment 9 Neal Gompa 2023-04-18 16:24:02 UTC
(In reply to Fabio Valentini from comment #8)
> I've updated the files at the original URLs with two changes:
> 
> - License tag contains no more simplifications (other than the "allowed"
> ones, for example that "(A OR B)" and "(B OR A)" are equivalent).
> - Added "Provides: gstreamer1-plugin-reqwest = %{version}-%{release}" to the
> gst-plugin-reqwest subpackage.
> 
> If you want that latter one to be the other way round (i.e. "subpackage name
> = gstreamer1-plugin-reqwest", and "provides = gst-plugin-reqwest"), then
> please go through FPC to document this exception to the Packaging Guidelines
> (i.e. add a section for GStreamer plugins here:
> https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/
> #_addon_packages).

This is unnecessarily punitive given that every other gst plugin package is "gstreamer1-plugin-<pluginname>".

Please just flip it and we can reconcile it in the documentation after the fact.

Comment 10 Fabio Valentini 2023-04-18 16:37:00 UTC
> This is unnecessarily punitive

Punitive? *I* am the one who's waiting for approval here :D I would've been fine with waiting until it's cleared up in the docs ...
I just didn't want to submit a package that - in my opinion - violates the Packaging Guidelines, but if it will be approved like this, fine ... (files updated).

I opened a ticket so we don't forget: https://pagure.io/packaging-committee/issue/1273

Comment 11 Kalev Lember 2023-04-19 18:49:03 UTC
Thanks, the package looks good to me now. APPROVED

+ The package is named according to Fedora packaging guidelines
+ The spec file name matches the base package name.
+ The package meets the Packaging Guidelines
+ The package is licensed with a Fedora approved license and meets the
  Licensing Guidelines.
+ The license field in the spec file matches the actual license
+ The license text is included in %license

> I opened a ticket so we don't forget: https://pagure.io/packaging-committee/issue/1273

Thanks, I commented in the ticket. I'm still confused why you say it needs an exception when the guidelines say to use the %{parent}-%{child}, e.g. gstreamer1-%{child} pattern, but we can continue discussing it in the fpc ticket.

Comment 12 Fabio Valentini 2023-04-19 19:40:15 UTC
Thanks for the review!

Comment 13 Fedora Admin user for bugzilla script actions 2023-04-19 19:40:47 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-gst-plugin-reqwest


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