https://bugzilla.redhat.com/show_bug.cgi?id=2179161
Bug ID: 2179161 Summary: Review Request: rust-gst-plugin-reqwest - GStreamer reqwest HTTP Source Plugin Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: decathorpe@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
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.sr...
Description: GStreamer reqwest HTTP Source Plugin.
Fedora Account System Username: decathorpe
https://bugzilla.redhat.com/show_bug.cgi?id=2179161
Jakub Kadlčík jkadlcik@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- URL| |https://crates.io/crates/gs | |t-plugin-reqwest
--- Comment #1 from Jakub Kadlčík jkadlcik@redhat.com --- Copr build: https://copr.fedorainfracloud.org/coprs/build/5652514 (failed)
Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2179161
Kalev Lember klember@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |klember@redhat.com Flags| |fedora-review? Assignee|nobody@fedoraproject.org |klember@redhat.com Status|NEW |ASSIGNED
--- Comment #2 from Kalev Lember klember@redhat.com --- 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_lice... 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/...
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'.
https://bugzilla.redhat.com/show_bug.cgi?id=2179161
--- Comment #3 from Fabio Valentini decathorpe@gmail.com --- (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_pac...
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).
https://bugzilla.redhat.com/show_bug.cgi?id=2179161
--- Comment #4 from Kalev Lember klember@redhat.com --- (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 :)
https://bugzilla.redhat.com/show_bug.cgi?id=2179161
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |ngompa13@gmail.com
--- Comment #5 from Neal Gompa ngompa13@gmail.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2179161
--- Comment #6 from Neal Gompa ngompa13@gmail.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2179161
--- Comment #7 from Fabio Valentini decathorpe@gmail.com --- Can one of you submit a PR for the Packaging Guidelines to document this?
https://bugzilla.redhat.com/show_bug.cgi?id=2179161
--- Comment #8 from Fabio Valentini decathorpe@gmail.com --- 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_pac...).
https://bugzilla.redhat.com/show_bug.cgi?id=2179161
--- Comment #9 from Neal Gompa ngompa13@gmail.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=2179161
--- Comment #10 from Fabio Valentini decathorpe@gmail.com ---
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
https://bugzilla.redhat.com/show_bug.cgi?id=2179161
Kalev Lember klember@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #11 from Kalev Lember klember@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2179161
--- Comment #12 from Fabio Valentini decathorpe@gmail.com --- Thanks for the review!
https://bugzilla.redhat.com/show_bug.cgi?id=2179161
--- Comment #13 from Fedora Admin user for bugzilla script actions fedora-admin-xmlrpc@fedoraproject.org --- The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-gst-plugin-reqwest
https://bugzilla.redhat.com/show_bug.cgi?id=2179161
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |ERRATA Status|ASSIGNED |CLOSED Last Closed| |2023-04-19 21:22:38
--- Comment #14 from Fabio Valentini decathorpe@gmail.com --- Imported and built:
https://bodhi.fedoraproject.org/updates/FEDORA-2023-44ef1c2688 https://bodhi.fedoraproject.org/updates/FEDORA-2023-6e7c97ae7e https://bodhi.fedoraproject.org/updates/FEDORA-2023-06c6cf7491 https://bodhi.fedoraproject.org/updates/FEDORA-2023-85d903ab93
https://bugzilla.redhat.com/show_bug.cgi?id=2179161 Bug 2179161 depends on bug 2179159, which changed state.
Bug 2179159 Summary: Review Request: rust-gst-plugin-version-helper - Build.rs helper function for GStreamer plugin metadata https://bugzilla.redhat.com/show_bug.cgi?id=2179159
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |ERRATA
package-review@lists.fedoraproject.org