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
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.
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'.
(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).
(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 :)
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.
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.
Can one of you submit a PR for the Packaging Guidelines to document this?
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).
(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.
> 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
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.
Thanks for the review!
The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-gst-plugin-reqwest
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