[Bug 1671571] New: Review Request: playerctl - MPRIS command-line controller and library for spotify, vlc, audacious, bmp, cmus, and others
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Bug ID: 1671571 Summary: Review Request: playerctl - MPRIS command-line controller and library for spotify, vlc, audacious, bmp, cmus, and others Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: low Assignee: nobody@fedoraproject.org Reporter: jflory7@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- External Bug ID: Red Hat Bugzilla 1266594 Classification: Fedora
This is my third package and I am seeking a sponsor.
* Spec URL: https://pagure.io/jflory7-rpm-specs/blob/master/f/rpmbuild/SPECS/playerctl.s... * SRPM URL: https://pagure.io/jflory7-rpm-specs/blob/master/f/rpmbuild/SRPMS/playerctl-2... * Description: Playerctl is a command-line utility and library for controlling media players that implement the MPRIS D-Bus Interface Specification. Playerctl makes it easy to bind player actions, such as play and pause, to media keys. You can also get metadata about the playing track such as the artist and title for integration into statusline generators or other command-line tools. (https://github.com/acrisci/playerctl). * Fedora Account System Username: jflory7
This package is also hosted in COPR and was built in Koji on F28, F29, and Rawhide.
* https://copr.fedorainfracloud.org/coprs/jflory7/playerctl/ * F30: https://koji.fedoraproject.org/koji/taskinfo?taskID=32392575 * F29: https://koji.fedoraproject.org/koji/taskinfo?taskID=32392582 * F28: https://koji.fedoraproject.org/koji/taskinfo?taskID=32392592
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Dridi Boukelmoune dridi.boukelmoune@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dridi.boukelmoune@gmail.com
--- Comment #1 from Dridi Boukelmoune dridi.boukelmoune@gmail.com --- I can't sponsor you, but I will do the formal review once you have a sponsor. And as per bug 1266594 I would like to co-maintain this package since I'm currently using my own build of the package and have a vetted interest here.
Before the formal review, I have a few nits after a quick read of the spec:
For the summary, please stick to "Command-line MPRIS-compatible Media Player Controller" instead of mentioning services like spotify or software like VLC that aren't fit for inclusion in Fedora.
You shouldn't need to "Requires: glib", I didn't for my local build.
For the Source URL, please use upstream's official release archive instead of deriving one from the github API:
https://github.com/acrisci/playerctl/releases/download/v2.0.1/playerctl-2.0....
They also provide a SHA-256 checksum that you should use before you upload the archive to the look-aside cache.
The license should be LGPLv3+ because upstream clearly uses "or (at your option) any later version" in its source files. I didn't check whether other licenses apply.
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #2 from Dridi Boukelmoune dridi.boukelmoune@gmail.com --- Also don't bother with %epoch unless one day upstream breaks the upgrade path.
See the guidelines: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_requiring_base_p...
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #3 from Justin W. Flory jflory7@gmail.com --- Hi Dridi, thanks for the feedback. I pushed a new commit [1] to my RPM spec based on your feedback.
(In reply to Dridi Boukelmoune from comment #1)
I can't sponsor you, but I will do the formal review once you have a sponsor. And as per bug 1266594 I would like to co-maintain this package since I'm currently using my own build of the package and have a vetted interest here.
I'm happy to co-maintain the package with you!
For the Source URL, please use upstream's official release archive instead of deriving one from the github API:
https://github.com/acrisci/playerctl/releases/download/v2.0.1/playerctl-2.0. 1.tar.xz
I wasn't able to do this. Upstream's archives are xz archives and Koji/COPR doesn't recognize the xz compression format, so the build fails [2]. I'm using the tar.gz archive via the GitHub API similar to other RPM packages available in Fedora.
-- [1] https://pagure.io/jflory7-rpm-specs/c/370f06098ee6adc2da025c02116b572e3dc949... [2] https://copr.fedorainfracloud.org/coprs/jflory7/playerctl/build/853607/
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Dridi Boukelmoune dridi.boukelmoune@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |tony@dubstepdish.com
--- Comment #4 from Dridi Boukelmoune dridi.boukelmoune@gmail.com --- More changes for playerctl:
--- playerctl.spec 2019-02-04 09:21:17.102143219 +0100 +++ playerctl.spec 2019-02-04 09:24:00.415760142 +0100 @@ -5,7 +5,7 @@
License: LGPLv3+ URL: https://github.com/acrisci/playerctl -Source0: %{url}/archive/v%{version}/%{name}-%{version}.tar.gz +Source: %{url}/releases/download/v%{version}/%{name}-%{version}.tar.xz
BuildRequires: gcc BuildRequires: meson @@ -62,13 +62,15 @@ %{_bindir}/%{name} %{_datadir}/gir-1.0/Playerctl-2.0.gir %{_libdir}/girepository-1.0/Playerctl-2.0.typelib -%{_libdir}/lib%{name}.* -%doc %{_datadir}/gtk-doc/html/%{name}/* -%doc %{_datadir}/man/man1/%{name}.* +%{_libdir}/lib%{name}.so.2* +%{_datadir}/gtk-doc/html/%{name} +%{_datadir}/man/man1/%{name}.*
%files devel %{_includedir}/%{name}/*.h +%{_libdir}/lib%{name}.a +%{_libdir}/lib%{name}.so %{_libdir}/pkgconfig/%{name}.pc
The xz archive is handled just fine, and you should file a bug for COPR if it's not working there. We are targeting Fedora proper here so I'd rather stick to the upstream release archive.
I also moved the static library and un-versioned shared object to the devel packages, and added the full soname in the main package so we don't miss soname bumps in the future. I built this with mock locally and I'm currently using the package on my system with no regression.
Finally, I removed the %doc indicators because those are well-known locations in Fedora so they already get flagged as such, and I made sure the %{_datadir}/gtk-doc/html/%{name} is owned by the package, not just its contents.
When you update the spec, make sure to add a new comment with corresponding Spec and SRPM URLs so that during the formal review fedora-review picks up your latest update (especially since you updated the release tag, that changes the SRPM URL).
Good luck with your sponsorship, I will patiently wait :)
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |decathorpe@gmail.com
--- Comment #5 from Fabio Valentini decathorpe@gmail.com --- Just some small comments from a Packaging Committee member:
- Please don't include static libraries, unless *absolutely necessary*. In this case, I think it can be safely removed (probably by rm-ing it from the buildroot at the end of %install): rm %{buildroot}/%{_libdir}/lib%{name}.a
See: https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-...
- "gobject-introspection-devel" could be replaced by "pkgconfig(gobject-introspection-1.0)"
Mainly for consistency, since you're already using pkgconfig(foo) BuildRequires for other things.
- Depending on the size of the html docs, they should be moved to a separate, (probably noarch) -doc sub-package.
For example, fedora-review recommends to move documentation to a separate sub-package if it's bigger than 1 MB.
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #6 from Justin W. Flory jflory7@gmail.com --- I pushed a new commit [1] to address the above feedback. The build completed successfully in Koji [2][3] and COPR [4].
(In reply to Dridi Boukelmoune from comment #4)
The xz archive is handled just fine, and you should file a bug for COPR if it's not working there. We are targeting Fedora proper here so I'd rather stick to the upstream release archive.
Odd. Not sure why the build failed before because xz was an unrecognized format. I made this change again and the package built successfully. Maybe the archive was corrupt originally. In either case, this is done now.
I also moved the static library and un-versioned shared object to the devel packages, and added the full soname in the main package so we don't miss soname bumps in the future. I built this with mock locally and I'm currently using the package on my system with no regression.
Finally, I removed the %doc indicators because those are well-known locations in Fedora so they already get flagged as such, and I made sure the %{_datadir}/gtk-doc/html/%{name} is owned by the package, not just its contents.
Thanks for these tips. About the static library / shared objects, could you link me to documentation or explain why this change is helpful? I'm new to building C packages. Understanding why this change is helpful will help me build better packages later too.
When you update the spec, make sure to add a new comment with corresponding Spec and SRPM URLs so that during the formal review fedora-review picks up your latest update (especially since you updated the release tag, that changes the SRPM URL).
Spec: https://pagure.io/jflory7-rpm-specs/blob/master/f/rpmbuild/SPECS/playerctl.s... SRPM: https://pagure.io/jflory7-rpm-specs/blob/master/f/rpmbuild/SRPMS/playerctl-2...
(In reply to Fabio Valentini from comment #5)
Just some small comments from a Packaging Committee member:
- Please don't include static libraries, unless *absolutely necessary*.
In this case, I think it can be safely removed (probably by rm-ing it from the buildroot at the end of %install): rm %{buildroot}/%{_libdir}/lib%{name}.a
It seems the static library is necessary? My Koji build [5] and COPR build [6] failed without it.
- Depending on the size of the html docs, they should be moved to a
separate, (probably noarch) -doc sub-package.
For example, fedora-review recommends to move documentation to a separate sub-package if it's bigger than 1 MB.
The HTML docs were only 500KB or so, but I went ahead and split them out into their own package anyways.
-- [1] https://pagure.io/jflory7-rpm-specs/c/476d05cbafe8913a8d0af4c29bb439a3d1264a... [2] https://koji.fedoraproject.org/koji/taskinfo?taskID=32534256 [3] https://koji.fedoraproject.org/koji/taskinfo?taskID=32534242 [4] https://copr.fedorainfracloud.org/coprs/jflory7/playerctl/build/853863/ [5] https://koji.fedoraproject.org/koji/taskinfo?taskID=32533723 [6] https://copr.fedorainfracloud.org/coprs/jflory7/playerctl/build/853848/
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #7 from Fabio Valentini decathorpe@gmail.com ---
(In reply to Fabio Valentini from comment #5)
Just some small comments from a Packaging Committee member:
- Please don't include static libraries, unless *absolutely necessary*.
In this case, I think it can be safely removed (probably by rm-ing it from the buildroot at the end of %install): rm %{buildroot}/%{_libdir}/lib%{name}.a
It seems the static library is necessary? My Koji build [5] and COPR build [6] failed without it.
It's not. You just forgot to remove the corresponding entry from the %files section - and now it can't find the file that you removed - that's all.
- Depending on the size of the html docs, they should be moved to a
separate, (probably noarch) -doc sub-package.
For example, fedora-review recommends to move documentation to a separate sub-package if it's bigger than 1 MB.
The HTML docs were only 500KB or so, but I went ahead and split them out into their own package anyways.
Nice, thanks!
-- [1] https://pagure.io/jflory7-rpm-specs/c/ 476d05cbafe8913a8d0af4c29bb439a3d1264af5?branch=master [2] https://koji.fedoraproject.org/koji/taskinfo?taskID=32534256 [3] https://koji.fedoraproject.org/koji/taskinfo?taskID=32534242 [4] https://copr.fedorainfracloud.org/coprs/jflory7/playerctl/build/853863/ [5] https://koji.fedoraproject.org/koji/taskinfo?taskID=32533723 [6] https://copr.fedorainfracloud.org/coprs/jflory7/playerctl/build/853848/
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #8 from Dridi Boukelmoune dridi.boukelmoune@gmail.com --- Justin, a static library is not code you run, but code you (statically) link to as part of a build process, that's why I moved it to the -devel sub-package. It could have also lived in a -static sub-packages too, you can find such occurrences in the repositories.
Fabio, I know that our guidelines strongly advise against static linking (and bundling in general) but I find it sad to prevent end-users to statically build software on top of Fedora if that's what they want and upstream provides the means. But if that's our guidelines, I can live without the static library and I personally don't care since I integrate %{_bindir}/playerctl in my local setup.
Thanks for your help with the review, I'll wait for an update until the sponsorship is solved to formally review the package.
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Dridi Boukelmoune dridi.boukelmoune@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |dridi.boukelmoune@gmail.com
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #9 from Justin W. Flory jflory7@gmail.com --- (In reply to Fabio Valentini from comment #7)
It's not. You just forgot to remove the corresponding entry from the %files section - and now it can't find the file that you removed - that's all.
(In reply to Dridi Boukelmoune from comment #8)
Fabio, I know that our guidelines strongly advise against static linking (and bundling in general) but I find it sad to prevent end-users to statically build software on top of Fedora if that's what they want and upstream provides the means. But if that's our guidelines, I can live without the static library and I personally don't care since I integrate %{_bindir}/playerctl in my local setup.
I took the middle-ground approach and built a separate, optional package with the static libraries. The updated commit [1] passed successfully Koji [2][3] and COPR [4]. An updated SRPM is also available [5].
(In reply to Dridi Boukelmoune from comment #8)
Justin, a static library is not code you run, but code you (statically) link to as part of a build process, that's why I moved it to the -devel sub-package. It could have also lived in a -static sub-packages too, you can find such occurrences in the repositories.
Thanks for the explanation.
-- [1] https://pagure.io/jflory7-rpm-specs/c/ec957e11ed6307b08e5031f5a35451a4761cfb... [2] https://koji.fedoraproject.org/koji/taskinfo?taskID=32545025 [3] https://koji.fedoraproject.org/koji/taskinfo?taskID=32545018 [4] https://copr.fedorainfracloud.org/coprs/jflory7/playerctl/build/854366/ [5] https://pagure.io/jflory7-rpm-specs/blob/master/f/rpmbuild/SRPMS/playerctl-2...
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |ngompa13@gmail.com
--- Comment #10 from Neal Gompa ngompa13@gmail.com --- (In reply to Justin W. Flory from comment #9)
(In reply to Fabio Valentini from comment #7)
It's not. You just forgot to remove the corresponding entry from the %files section - and now it can't find the file that you removed - that's all.
(In reply to Dridi Boukelmoune from comment #8)
Fabio, I know that our guidelines strongly advise against static linking (and bundling in general) but I find it sad to prevent end-users to statically build software on top of Fedora if that's what they want and upstream provides the means. But if that's our guidelines, I can live without the static library and I personally don't care since I integrate %{_bindir}/playerctl in my local setup.
I took the middle-ground approach and built a separate, optional package with the static libraries. The updated commit [1] passed successfully Koji [2][3] and COPR [4]. An updated SRPM is also available [5].
This is obviously allowed, but in Fedora, we just use "-static" instead of "-devel-static". The latter is openSUSE's (and long-ago Fedora) naming for static libraries...
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #11 from Neal Gompa ngompa13@gmail.com --- Also, to point out: static libraries do not require the main package, they require the devel package to be useful.
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #12 from Justin W. Flory jflory7@gmail.com --- (In reply to Neal Gompa from comment #11)
Also, to point out: static libraries do not require the main package, they require the devel package to be useful.
Thanks Neal. I pushed a new commit [1] with these changes. An SRPM is available [2]. It passed in Koji [3][4] and COPR [5].
--- [1] https://pagure.io/jflory7-rpm-specs/c/18b94a485fc7a9de47d645c465d739014b24a1... [2] https://pagure.io/jflory7-rpm-specs/blob/master/f/rpmbuild/SRPMS/playerctl-2... [3] https://koji.fedoraproject.org/koji/taskinfo?taskID=32590171 [4] https://koji.fedoraproject.org/koji/taskinfo?taskID=32590164 [5] https://copr.fedorainfracloud.org/coprs/jflory7/playerctl/build/855693/
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #13 from Neal Gompa ngompa13@gmail.com ---
%{_datadir}/gir-1.0/Playerctl-2.0.gir
This belongs in the devel subpackage, since this is used by gobject-introspection to generate bindings.
%{_libdir}/girepository-1.0/Playerctl-2.0.typelib %{_libdir}/lib%{name}.so.2*
These two belong in a libs subpackage, and the main binary package should require the libs subpackage.
The reason to do this is to allow the libraries to be multi-libbed in repos for 64-bit arches with their matching 32-bit arches (such as x86_64 and i686).
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #14 from Justin W. Flory jflory7@gmail.com --- I see, thanks for the pointers. Addressed in this commit [1]. SRPM [2], Koji builds [3][4], and COPR [5] with it.
What is the next step to move forward with this package?
--- [1] https://pagure.io/jflory7-rpm-specs/c/cd47b0aacb8024c05e110298a0c4225bb753ec... [2] https://pagure.io/jflory7-rpm-specs/blob/master/f/rpmbuild/SRPMS/playerctl-2... [3] https://koji.fedoraproject.org/koji/taskinfo?taskID=32635703 [4] https://koji.fedoraproject.org/koji/taskinfo?taskID=32635696 [5] https://copr.fedorainfracloud.org/coprs/jflory7/playerctl/build/856215/
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #15 from Justin W. Flory jflory7@gmail.com --- I misunderstood the packaging guidelines. I do not need a sponsor since I already submitted a package to Fedora. If there is no additional feedback, could someone set the fedora-review flag?
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Justin W. Flory jflory7@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(dridi.boukelmoune | |@gmail.com)
--- Comment #16 from Justin W. Flory jflory7@gmail.com --- (In reply to Dridi Boukelmoune from comment #8)
Thanks for your help with the review, I'll wait for an update until the sponsorship is solved to formally review the package.
Hi Dridi, are you able to complete this package review? I incorrectly assumed I needed sponsorship. I did not realize sponsorship is granted after submitting a first package. This is my second package.
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Dominik 'Rathann' Mierzejewski dominik@greysector.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review+ |
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Dominik 'Rathann' Mierzejewski dominik@greysector.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dominik@greysector.net
--- Comment #17 from Dominik 'Rathann' Mierzejewski dominik@greysector.net --- Is there a particular reason why you're building and including static libraries? Fedora defaults to shared ones only.
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Dridi Boukelmoune dridi.boukelmoune@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(dridi.boukelmoune | |@gmail.com) |
--- Comment #18 from Dridi Boukelmoune dridi.boukelmoune@gmail.com --- Dominik, as I said, even though we package things dynamically, I don't like the idea of removing the possibility from end-users. But I can live without it so really, I don't care.
Justin, the link to the spec file should link to a spec file, not an HTML document. For your next update, please use this URL for the spec:
https://pagure.io/jflory7-rpm-specs/raw/master/f/rpmbuild/SPECS/playerctl.sp...
Now there are a few things I'm not too happy with in this spec at a single glance:
The main package should not need to require the -libs sub-package, it should be detected automatically via linker information in binaries.
If you reuse the summary for the description, at least put a dot at the end:
%description foo %{summary}.
Install the %license file in all packages.
After that I'll be happy to move on to a formal review, but my availability will be limited for a couple weeks.
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #19 from Neal Gompa ngompa13@gmail.com --- (In reply to Dridi Boukelmoune from comment #18)
Now there are a few things I'm not too happy with in this spec at a single glance:
The main package should not need to require the -libs sub-package, it should be detected automatically via linker information in binaries.
This is actually a matter of policy for interdependencies like this. This prevents weird things from happening because often private symbols are used from -libs subpackage from other parts of the same source package. That, of course, is not true for external consumers.
After that I'll be happy to move on to a formal review, but my availability will be limited for a couple weeks.
I can take over the review if you want?
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Justin W. Flory jflory7@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(dridi.boukelmoune | |@gmail.com)
--- Comment #20 from Justin W. Flory jflory7@gmail.com --- Hi, I pushed a new commit [1] with this spec [2] and this SRPM [3] to address remaining feedback. Is there anything left to address in the review?
--- [1] https://pagure.io/jflory7-rpm-specs/c/de0e33bd1c8a992633e79b93aada740f33fdf6... [2] https://pagure.io/jflory7-rpm-specs/raw/master/f/rpmbuild/SPECS/playerctl.sp... [3] https://pagure.io/jflory7-rpm-specs/blob/master/f/rpmbuild/SRPMS
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #21 from Justin W. Flory jflory7@gmail.com --- Hi, any updates? I am hoping to write a Fedora Magazine article this month about playerctl and another package recently made available in Fedora, so it will help to move this review along.
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Dridi Boukelmoune dridi.boukelmoune@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(dridi.boukelmoune | |@gmail.com) |
--- Comment #22 from Dridi Boukelmoune dridi.boukelmoune@gmail.com --- Hi,
I actually tried to run fedora-review (and its python 3 port to compare) this weekend after being unavailable for more than a week. Unfortunately in your last update you didn't fully follow the review process [1] for that last update:
After you have addressed them, please post the URLs to the updated SPEC and SRPM file and remove it from the Whiteboard.
This is even more unfortunate since for previous updates you did post both SPEC and SRPM URLs.
Please do that, and I will pick it up. I will try to be as responsive as possible but this is a volunteer job for me, and I'm also volunteering to try out the python 3 port of fedora-review so it may delay this review a tiny bit more on my end.
[1] https://fedoraproject.org/wiki/Package_Review_Process
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Justin W. Flory jflory7@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(dridi.boukelmoune | |@gmail.com)
--- Comment #23 from Justin W. Flory jflory7@gmail.com --- (In reply to Dridi Boukelmoune from comment #22)
Please do that, and I will pick it up. I will try to be as responsive as possible but this is a volunteer job for me, and I'm also volunteering to try out the python 3 port of fedora-review so it may delay this review a tiny bit more on my end.
Hi Dridi, the links are here:
https://pagure.io/jflory7-rpm-specs/raw/master/f/rpmbuild/SPECS/playerctl.sp... https://pagure.io/jflory7-rpm-specs/blob/master/f/rpmbuild/SRPMS/playerctl-2...
If you do not have time for this package review, Neal mentioned he could finish out the package review for you.
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Dridi Boukelmoune dridi.boukelmoune@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(dridi.boukelmoune | |@gmail.com) |
--- Comment #24 from Dridi Boukelmoune dridi.boukelmoune@gmail.com --- Unfortunately your URL for the SRPM is still leading to a web page that does the redirection and fedora-review doesn't like that...
Now I'm curious to see whether it will pick up the correct URLs if it's me that posts them:
https://pagure.io/jflory7-rpm-specs/raw/master/f/rpmbuild/SPECS/playerctl.sp... https://pagure.io/jflory7-rpm-specs/raw/master/f/rpmbuild/SRPMS/playerctl-2....
I'll do my best, no need to bother Neal, he already does so many reviews (but thanks to him anyway for stepping up) and he can still keep an eye on this review.
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #25 from Fabio Valentini decathorpe@gmail.com --- I think for fedora-review to pick up the links correctly, you need to adhere to this format (as given in the fedora-review bugzilla template).
SPEC URL: ... SRPM URL: ...
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #26 from Dridi Boukelmoune dridi.boukelmoune@gmail.com --- Yes and no. The guidelines ask to do this in the review request description, and then they only say to post a comment with the new URLs if you update the spec as per the review.
I also confirmed that it doesn't have to be the submitter that posts those URLs and was able to run fedora-review and its Python 3 port, but I'm not done going through the report.
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #27 from Neal Gompa ngompa13@gmail.com ---
%package devel Summary: Development libraries and header files for %{name} Requires: %{name}%{?_isa} = %{version}-%{release}
%package docs Summary: Documentation related to %{name} BuildArch: noarch Requires: %{name}%{?_isa} = %{version}-%{release}
%package libs Summary: Libraries and shared code for %{name} Requires: %{name}%{?_isa} = %{version}-%{release}
%package static Summary: Static libraries for %{name} development Requires: %{name}-devel%{?_isa} = %{version}-%{release}
%description devel %{summary}.
%description docs %{summary}.
%description libs %{summary}.
%description static %{summary}.
This layout is actually broken. It's not obvious, but the %summary variable is populated by the _last_ definition of the 'Summary:' field at the time the %description section was evaluated.
So, this leads to quirks like this:
$ rpm -qip /var/lib/mock/fedora-29-x86_64/result/playerctl-devel-2.0.1-7.fc29.x86_64.rpm Name : playerctl-devel Version : 2.0.1 Release : 7.fc29 Architecture: x86_64 Install Date: (not installed) Group : Unspecified Size : 68295 License : LGPLv3+ Signature : (none) Source RPM : playerctl-2.0.1-7.fc29.src.rpm Build Date : Fri 15 Mar 2019 08:24:37 AM EDT Build Host : localhost Relocations : (not relocatable) URL : https://github.com/acrisci/playerctl Summary : Development libraries and header files for playerctl Description : Static libraries for playerctl development.
Make sure the %description section is directly underneath each %package section, unless you want to write the descriptions individually.
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Justin W. Flory jflory7@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(dridi.boukelmoune | |@gmail.com)
--- Comment #28 from Justin W. Flory jflory7@gmail.com --- I updated the RPM SPEC from the above feedback and this is ready for review again.
SPEC URL: https://pagure.io/jflory7-rpm-specs/raw/master/f/rpmbuild/SPECS/playerctl.sp... SRPM URL: https://pagure.io/jflory7-rpm-specs/raw/master/f/rpmbuild/SRPMS/playerctl-2....
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Dridi Boukelmoune dridi.boukelmoune@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(dridi.boukelmoune | |@gmail.com) |
--- Comment #29 from Dridi Boukelmoune dridi.boukelmoune@gmail.com --- Package Review ==============
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed
===== MUST items =====
C/C++: [x]: Package does not contain kernel modules. [!]: Package contains no static executables.
You need an exception to ship the static library. Either get one or remove the static library in the first place.
(Yes, I remember my previous comments on the topic=)
[x]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [x]: Header files in -devel subpackage, if present. [x]: ldconfig not called in %post and %postun for Fedora 28 and later. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. [x]: Development (unversioned) .so files in -devel subpackage, if present.
Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "GNU Lesser General Public License (v3 or later)". 21 files have unknown license. Detailed output of licensecheck in /home/dridi/fedora/FedoraReview/1671571-playerctl/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [!]: Package requires other packages for directories it uses. Note: No known owner of /usr/include/playerctl, /usr/share/gtk- doc/html/playerctl [!]: Package must own all directories that it creates. Note: Directories without known owners: /usr/share/gir-1.0, /usr/share/gtk-doc/html, /usr/lib64/girepository-1.0, /usr/include/playerctl, /usr/share/gtk-doc, /usr/share/gtk- doc/html/playerctl
Use the following patterns instead:
%{_includedir}/%{name}/ %{_datadir}/gtk-doc/
Add the following to the devel sub-package:
equires: pkgconfig(gobject-introspection-1.0)
[x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [x]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict.
The main package may conflict with itself on a multilib system and this is fine.
[x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [!]: Requires correct, justified where necessary.
The noarch docs package depends on the archful main package, which is debatable in the first place. At the very least, which variant of the main package should not be dictated by the build system's architecture.
[x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [ ]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment).
playerctl-devel.x86_64: W: no-documentation
The devel sub-package could "Recommends:" the docs sub-package.
On the other hand the documentation is quite small and could live in the devel package directly.
The perl locale warnings needs more investigation. We either need to set the environment straight or bring the en-us langpack.
[x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: Package does not own files or directories owned by other packages.
It should however own a couple already mentioned above.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: Static libraries in -static or -devel subpackage, providing -devel if present. Note: Package has .a files: playerctl-static.
As noted above, an exception is needed to ship the static sub-package.
[x]: File names are valid UTF-8. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 0 bytes in 0 files.
Not quite true, but the docs sub-package (note: not "doc") is small enough to be folded
[x]: Packages must not store files under /srv, /opt or /usr/local
===== SHOULD items =====
Generic: [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [-]: Final provides and requires are sane (see attachments).
See the note above regarding the noarch devel requiring an archful main package.
[x]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in playerctl , playerctl-static , playerctl-debuginfo , playerctl- debugsource
False-positives.
[x]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [!]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: Package should compile and build into binary rpms on all supported architectures.
https://koji.fedoraproject.org/koji/taskinfo?taskID=33719966
[x]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files.
Handled by meson.
[x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: The placement of pkgconfig(.pc) files are correct. [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified.
===== EXTRA items =====
Generic: [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM.
Rpmlint ------- Checking: playerctl-2.0.1-8.fc31.x86_64.rpm playerctl-devel-2.0.1-8.fc31.x86_64.rpm playerctl-docs-2.0.1-8.fc31.noarch.rpm playerctl-libs-2.0.1-8.fc31.x86_64.rpm playerctl-static-2.0.1-8.fc31.x86_64.rpm playerctl-debuginfo-2.0.1-8.fc31.x86_64.rpm playerctl-debugsource-2.0.1-8.fc31.x86_64.rpm playerctl-2.0.1-8.fc31.src.rpm playerctl.x86_64: W: spelling-error %description -l en_US statusline -> status line, status-line, stateliness playerctl.x86_64: W: spelling-error %description -l en_US introspectable -> introspect able, introspect-able, introspective playerctl.x86_64: W: spelling-error %description -l en_US spotify -> spottily playerctl.x86_64: W: spelling-error %description -l en_US vlc -> vac, VLF playerctl.x86_64: W: spelling-error %description -l en_US bmp -> mp, bump, imp playerctl.x86_64: W: spelling-error %description -l en_US cmus -> cums, mus, emus playerctl-devel.x86_64: W: no-documentation playerctl-libs.x86_64: W: no-documentation playerctl-static.x86_64: W: no-documentation playerctl.src: W: spelling-error %description -l en_US statusline -> status line, status-line, stateliness playerctl.src: W: spelling-error %description -l en_US introspectable -> introspect able, introspect-able, introspective playerctl.src: W: spelling-error %description -l en_US spotify -> spottily playerctl.src: W: spelling-error %description -l en_US vlc -> vac, VLF playerctl.src: W: spelling-error %description -l en_US bmp -> mp, bump, imp playerctl.src: W: spelling-error %description -l en_US cmus -> cums, mus, emus 8 packages and 0 specfiles checked; 0 errors, 15 warnings.
Rpmlint (debuginfo) ------------------- Checking: playerctl-debuginfo-2.0.1-8.fc31.x86_64.rpm playerctl-libs-debuginfo-2.0.1-8.fc31.x86_64.rpm 2 packages and 0 specfiles checked; 0 errors, 0 warnings.
Rpmlint (installed packages) ---------------------------- perl: warning: Setting locale failed. perl: warning: Please check that your locale settings: LANGUAGE = (unset), LC_ALL = (unset), LC_CTYPE = "C.UTF-8", LANG = "en_US.utf8" are supported and installed on your system. perl: warning: Falling back to the standard locale ("C"). perl: warning: Setting locale failed. perl: warning: Please check that your locale settings: LANGUAGE = (unset), LC_ALL = (unset), LC_CTYPE = "C.UTF-8", LANG = "en_US.utf8" are supported and installed on your system. perl: warning: Falling back to the standard locale ("C"). perl: warning: Setting locale failed. perl: warning: Please check that your locale settings: LANGUAGE = (unset), LC_ALL = (unset), LC_CTYPE = "C.UTF-8", LANG = "en_US.utf8" are supported and installed on your system. perl: warning: Falling back to the standard locale ("C"). sh: /usr/bin/python: No such file or directory playerctl-docs.noarch: W: invalid-url URL: https://github.com/acrisci/playerctl <urlopen error [Errno -2] Name or service not known> playerctl.x86_64: W: spelling-error %description -l en_US statusline -> status line, status-line, stateliness playerctl.x86_64: W: spelling-error %description -l en_US introspectable -> introspect able, introspect-able, introspective playerctl.x86_64: W: spelling-error %description -l en_US spotify -> spottily playerctl.x86_64: W: spelling-error %description -l en_US vlc -> vac, VLF playerctl.x86_64: W: spelling-error %description -l en_US bmp -> mp, bump, imp playerctl.x86_64: W: spelling-error %description -l en_US cmus -> cums, mus, emus playerctl.x86_64: W: invalid-url URL: https://github.com/acrisci/playerctl <urlopen error [Errno -2] Name or service not known> playerctl-libs.x86_64: W: invalid-url URL: https://github.com/acrisci/playerctl <urlopen error [Errno -2] Name or service not known> playerctl-libs.x86_64: W: no-documentation playerctl-libs-debuginfo.x86_64: W: invalid-url URL: https://github.com/acrisci/playerctl <urlopen error [Errno -2] Name or service not known> playerctl-devel.x86_64: W: invalid-url URL: https://github.com/acrisci/playerctl <urlopen error [Errno -2] Name or service not known> playerctl-devel.x86_64: W: no-documentation playerctl-debugsource.x86_64: W: invalid-url URL: https://github.com/acrisci/playerctl <urlopen error [Errno -2] Name or service not known> playerctl-static.x86_64: W: invalid-url URL: https://github.com/acrisci/playerctl <urlopen error [Errno -2] Name or service not known> playerctl-static.x86_64: W: no-documentation playerctl-debuginfo.x86_64: W: invalid-url URL: https://github.com/acrisci/playerctl <urlopen error [Errno -2] Name or service not known> 8 packages and 0 specfiles checked; 0 errors, 17 warnings.
Source checksums ---------------- https://github.com/acrisci/playerctl/releases/download/v2.0.1/playerctl-2.0.... : CHECKSUM(SHA256) this package : c3ae6d74ddd951767da4e7852a76a88a2920ed02f126dab6e3b6f0c77549c74f CHECKSUM(SHA256) upstream package : c3ae6d74ddd951767da4e7852a76a88a2920ed02f126dab6e3b6f0c77549c74f
Requires -------- playerctl (rpmlib, GLIBC filtered): libc.so.6()(64bit) libgio-2.0.so.0()(64bit) libglib-2.0.so.0()(64bit) libgobject-2.0.so.0()(64bit) libplayerctl.so.2()(64bit) playerctl-libs(x86-64) rtld(GNU_HASH)
playerctl-devel (rpmlib, GLIBC filtered): /usr/bin/pkg-config libplayerctl.so.2()(64bit) pkgconfig(gio-2.0) pkgconfig(gio-unix-2.0) pkgconfig(gobject-2.0) playerctl(x86-64)
playerctl-docs (rpmlib, GLIBC filtered): playerctl(x86-64)
playerctl-libs (rpmlib, GLIBC filtered): libc.so.6()(64bit) libgio-2.0.so.0()(64bit) libglib-2.0.so.0()(64bit) libgobject-2.0.so.0()(64bit) playerctl(x86-64) rtld(GNU_HASH)
playerctl-static (rpmlib, GLIBC filtered): playerctl-devel(x86-64)
playerctl-debuginfo (rpmlib, GLIBC filtered):
playerctl-debugsource (rpmlib, GLIBC filtered):
Provides -------- playerctl: playerctl playerctl(x86-64)
playerctl-devel: pkgconfig(playerctl) playerctl-devel playerctl-devel(x86-64)
playerctl-docs: playerctl-docs
playerctl-libs: libplayerctl.so.2()(64bit) playerctl-libs playerctl-libs(x86-64)
playerctl-static: playerctl-static playerctl-static(x86-64)
playerctl-debuginfo: debuginfo(build-id) playerctl-debuginfo playerctl-debuginfo(x86-64)
playerctl-debugsource: playerctl-debugsource playerctl-debugsource(x86-64)
Generated by fedora-review 0.6.1 (37b2325) last change: 2019-03-10 Command line :try-fedora-review -b 1671571 -m fedora-rawhide-x86_64 Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, C/C++, Shell-api Disabled plugins: fonts, Java, SugarActivity, R, PHP, Perl, Haskell, Python, Ruby, Ocaml Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Justin W. Flory jflory7@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(dridi.boukelmoune | |@gmail.com)
--- Comment #30 from Justin W. Flory jflory7@gmail.com --- Hi, a few changes:
1. Updated package to latest upstream release, v2.0.2 2. Implemented feedback from fedora-review tool 3. Updated package successfully built in COPR [1] and Koji [2][3][4]
The updated RPM SPEC and SRPM are below and ready for review:
SPEC URL: https://pagure.io/jflory7-rpm-specs/raw/master/f/rpmbuild/SPECS/playerctl.sp... SRPM URL: https://pagure.io/jflory7-rpm-specs/raw/master/f/rpmbuild/SRPMS/playerctl-2....
As for packaging the static library, I do not know how to work around not including it. I reviewed the packaging guidelines and they do not mention a requirement to get an exception to ship the static library, only that the static library should be packaged separately:
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_packaging_static...
I ran this through fedora-review this time before I created this SRPM. Let me know if anything is left for this package review.
--- [1] https://copr.fedorainfracloud.org/coprs/jflory7/playerctl/build/881174/ [2] https://koji.fedoraproject.org/koji/taskinfo?taskID=34085414 [3] https://koji.fedoraproject.org/koji/taskinfo?taskID=34085407 [4] https://koji.fedoraproject.org/koji/taskinfo?taskID=34085421
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Dridi Boukelmoune dridi.boukelmoune@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(dridi.boukelmoune | |@gmail.com) |
--- Comment #31 from Dridi Boukelmoune dridi.boukelmoune@gmail.com --- Unfortunately I still found a few things and ran a few scratch builds myself.
I suspect this will be the last iteration!
Package Review ==============
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed
Issues: ======= - If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. Note: No gcc, gcc-c++ or clang found in BuildRequires See: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/
You get gcc in your build root because in your build dependency tree you eventually drag libtool and it requires gcc. You should still explicitly add one:
Requires: gcc
===== MUST items =====
C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables.
As you pointed out, static libraries are not explicitly prohibited so let's treat this one as a false-positive.
[x]: Header files in -devel subpackage, if present. [x]: ldconfig not called in %post and %postun for Fedora 28 and later. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. [x]: Development (unversioned) .so files in -devel subpackage, if present.
Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "GNU Lesser General Public License (v3 or later)". 21 files have unknown license. Detailed output of licensecheck in /home/dridi/fedora/FedoraReview/1671571-playerctl/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [!]: Package must own all directories that it creates. Note: Directories without known owners: /usr/lib64/girepository-1.0
Change the following line like this: -%{_libdir}/girepository-1.0/Playerctl-2.0.typelib +%{_libdir}/girepository-1.0/
[x]: Package does not own files or directories owned by other packages. Note: Dirs in package are owned also by: /usr/share/gtk-doc( [...])
This is compliant as per the guidelines.
[x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [!]: Changelog in prescribed format.
Despite an update to version 2.0.2 the last changelog entry still refers to 2.0.1 and I'm also not sure about the indentation of changelog entries.
[x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [x]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary.
I think the following from the devel sub-package is superfluous:
Requires: pkgconfig(gobject-introspection-1.0)
I realized that BuildRequires are properly set thanks to the pkg-config file shipped in the devel package and I shouldn't have asked this to be added in the first place.
[x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [ ]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: Package requires other packages for directories it uses. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: Static libraries in -static or -devel subpackage, providing -devel if present. Note: Package has .a files: playerctl-static. [x]: File names are valid UTF-8. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 0 bytes in 0 files. [x]: Packages must not store files under /srv, /opt or /usr/local
===== SHOULD items =====
Generic: [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [x]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in playerctl , playerctl-docs , playerctl-static
False-positives.
[x]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: Package should compile and build into binary rpms on all supported architectures.
https://koji.fedoraproject.org/koji/taskinfo?taskID=34107470
[x]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: The placement of pkgconfig(.pc) files are correct. [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified.
===== EXTRA items =====
Generic: [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM.
Rpmlint ------- Checking: playerctl-2.0.2-1.fc31.x86_64.rpm playerctl-devel-2.0.2-1.fc31.x86_64.rpm playerctl-docs-2.0.2-1.fc31.noarch.rpm playerctl-libs-2.0.2-1.fc31.x86_64.rpm playerctl-static-2.0.2-1.fc31.x86_64.rpm playerctl-debuginfo-2.0.2-1.fc31.x86_64.rpm playerctl-debugsource-2.0.2-1.fc31.x86_64.rpm playerctl-2.0.2-1.fc31.src.rpm playerctl.x86_64: W: spelling-error %description -l en_US statusline -> status line, status-line, stateliness playerctl.x86_64: W: spelling-error %description -l en_US introspectable -> introspect able, introspect-able, introspective playerctl.x86_64: W: spelling-error %description -l en_US spotify -> spottily playerctl.x86_64: W: spelling-error %description -l en_US vlc -> vac, VLF playerctl.x86_64: W: spelling-error %description -l en_US bmp -> mp, bump, imp playerctl.x86_64: W: spelling-error %description -l en_US cmus -> cums, mus, emus playerctl.x86_64: W: incoherent-version-in-changelog 2.0.1-1 ['2.0.2-1.fc31', '2.0.2-1'] playerctl-devel.x86_64: W: no-documentation playerctl-libs.x86_64: W: no-documentation playerctl-static.x86_64: W: no-documentation playerctl.src: W: spelling-error %description -l en_US statusline -> status line, status-line, stateliness playerctl.src: W: spelling-error %description -l en_US introspectable -> introspect able, introspect-able, introspective playerctl.src: W: spelling-error %description -l en_US spotify -> spottily playerctl.src: W: spelling-error %description -l en_US vlc -> vac, VLF playerctl.src: W: spelling-error %description -l en_US bmp -> mp, bump, imp playerctl.src: W: spelling-error %description -l en_US cmus -> cums, mus, emus 8 packages and 0 specfiles checked; 0 errors, 16 warnings.
Rpmlint (debuginfo) ------------------- Checking: playerctl-libs-debuginfo-2.0.2-1.fc31.x86_64.rpm playerctl-debuginfo-2.0.2-1.fc31.x86_64.rpm 2 packages and 0 specfiles checked; 0 errors, 0 warnings.
Rpmlint (installed packages) ---------------------------- playerctl-static.x86_64: W: no-documentation playerctl.x86_64: W: spelling-error %description -l en_US statusline -> status line, status-line, stateliness playerctl.x86_64: W: spelling-error %description -l en_US introspectable -> introspect able, introspect-able, introspective playerctl.x86_64: W: spelling-error %description -l en_US spotify -> spottily playerctl.x86_64: W: spelling-error %description -l en_US vlc -> vac, VLF playerctl.x86_64: W: spelling-error %description -l en_US bmp -> mp, bump, imp playerctl.x86_64: W: spelling-error %description -l en_US cmus -> cums, mus, emus playerctl.x86_64: W: incoherent-version-in-changelog 2.0.1-1 ['2.0.2-1.fc31', '2.0.2-1'] playerctl-libs.x86_64: W: no-documentation playerctl-devel.x86_64: W: no-documentation 8 packages and 0 specfiles checked; 0 errors, 10 warnings.
Source checksums ---------------- https://github.com/acrisci/playerctl/releases/download/v2.0.2/playerctl-2.0.... : CHECKSUM(SHA256) this package : c84637893b3abc52eb7acb196259780af7cf99197aada7f0eab532b61c2d6751 CHECKSUM(SHA256) upstream package : c84637893b3abc52eb7acb196259780af7cf99197aada7f0eab532b61c2d6751
Requires -------- playerctl (rpmlib, GLIBC filtered): libc.so.6()(64bit) libgio-2.0.so.0()(64bit) libglib-2.0.so.0()(64bit) libgobject-2.0.so.0()(64bit) libplayerctl.so.2()(64bit) playerctl-libs(x86-64) rtld(GNU_HASH)
playerctl-devel (rpmlib, GLIBC filtered): /usr/bin/pkg-config libplayerctl.so.2()(64bit) pkgconfig(gio-2.0) pkgconfig(gio-unix-2.0) pkgconfig(gobject-2.0) pkgconfig(gobject-introspection-1.0) playerctl(x86-64)
playerctl-docs (rpmlib, GLIBC filtered): playerctl
playerctl-libs (rpmlib, GLIBC filtered): libc.so.6()(64bit) libgio-2.0.so.0()(64bit) libglib-2.0.so.0()(64bit) libgobject-2.0.so.0()(64bit) playerctl(x86-64) rtld(GNU_HASH)
playerctl-static (rpmlib, GLIBC filtered): playerctl-devel(x86-64)
playerctl-debuginfo (rpmlib, GLIBC filtered):
playerctl-debugsource (rpmlib, GLIBC filtered):
Provides -------- playerctl: playerctl playerctl(x86-64)
playerctl-devel: pkgconfig(playerctl) playerctl-devel playerctl-devel(x86-64)
playerctl-docs: playerctl-docs
playerctl-libs: libplayerctl.so.2()(64bit) playerctl-libs playerctl-libs(x86-64)
playerctl-static: playerctl-static playerctl-static(x86-64)
playerctl-debuginfo: debuginfo(build-id) playerctl-debuginfo playerctl-debuginfo(x86-64)
playerctl-debugsource: playerctl-debugsource playerctl-debugsource(x86-64)
Generated by fedora-review 0.6.1 (37b2325) last change: 2019-03-10 Command line :try-fedora-review -b 1671571 -m fedora-rawhide-x86_64 Buildroot used: fedora-rawhide-x86_64 Active plugins: C/C++, Generic, Shell-api Disabled plugins: R, Java, PHP, Ruby, Perl, Ocaml, Haskell, SugarActivity, Python, fonts Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Justin W. Flory jflory7@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(dridi.boukelmoune | |@gmail.com)
--- Comment #32 from Justin W. Flory jflory7@gmail.com --- (In reply to Dridi Boukelmoune from comment #31)
Unfortunately I still found a few things and ran a few scratch builds myself.
I suspect this will be the last iteration!
Great! Thanks for this round of feedback. The updated RPM SPEC and SRPM are below and ready for review:
SPEC URL: https://pagure.io/jflory7-rpm-specs/raw/master/f/rpmbuild/SPECS/playerctl.sp... SRPM URL: https://pagure.io/jflory7-rpm-specs/raw/master/f/rpmbuild/SRPMS/playerctl-2....
Let me know if anything else is remaining.
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Dridi Boukelmoune dridi.boukelmoune@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(dridi.boukelmoune |fedora-review+ |@gmail.com) |
--- Comment #33 from Dridi Boukelmoune dridi.boukelmoune@gmail.com --- Approved.
Don't forget to add me as a co-maintainer, I will stay around to help if you get any bug reports or if I see an update lingering.
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #34 from Tony Crisci tony@dubstepdish.com --- Thanks for helping me get my software out there. It's a privilege to be in the repo. If you have problems with upstream down the line, send me an email or open an issue on github.
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #35 from Gwyn Ciesla gwync@protonmail.com --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/playerctl
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |MODIFIED
--- Comment #36 from Fedora Update System updates@fedoraproject.org --- playerctl-2.0.2-2.fc30 has been submitted as an update to Fedora 30. https://bodhi.fedoraproject.org/updates/FEDORA-2019-9453c711eb
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #37 from Fedora Update System updates@fedoraproject.org --- playerctl-2.0.2-2.fc29 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2019-1bac0790b1
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #38 from Fedora Update System updates@fedoraproject.org --- playerctl-2.0.2-2.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2019-4249741e9d
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #39 from Fedora Update System updates@fedoraproject.org --- playerctl-2.0.2-2.fc30 has been pushed to the Fedora 30 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-9453c711eb
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #40 from Fedora Update System updates@fedoraproject.org --- playerctl-2.0.2-2.fc28 has been pushed to the Fedora 28 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-4249741e9d
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #41 from Fedora Update System updates@fedoraproject.org --- playerctl-2.0.2-2.fc29 has been pushed to the Fedora 29 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-1bac0790b1
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |ERRATA Last Closed| |2019-04-21 00:40:55
--- Comment #42 from Fedora Update System updates@fedoraproject.org --- playerctl-2.0.2-2.fc28 has been pushed to the Fedora 28 stable repository. If problems still persist, please make note of it in this bug report.
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #43 from Fedora Update System updates@fedoraproject.org --- playerctl-2.0.2-2.fc29 has been pushed to the Fedora 29 stable repository. If problems still persist, please make note of it in this bug report.
https://bugzilla.redhat.com/show_bug.cgi?id=1671571
--- Comment #44 from Fedora Update System updates@fedoraproject.org --- playerctl-2.0.2-2.fc30 has been pushed to the Fedora 30 stable repository. If problems still persist, please make note of it in this bug report.
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com