Bug 912048 - Review Request: mpdris2 - Provide MPRIS 2 support to mpd
Review Request: mpdris2 - Provide MPRIS 2 support to mpd
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Gianluca Sforna
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-17 07:40 EST by Ankur Sinha (FranciscoD)
Modified: 2013-12-05 05:29 EST (History)
3 users (show)

See Also:
Fixed In Version: mpdris2-0.4-1.fc19
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-12-05 05:28:45 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
giallu: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Ankur Sinha (FranciscoD) 2013-02-17 07:40:36 EST
Spec URL: http://ankursinha.fedorapeople.org/mpDris2/mpDris2.spec
SRPM URL: http://ankursinha.fedorapeople.org/mpDris2/mpDris2-0.4-1.fc19.src.rpm
Description: 
mpDris2 provides MPRIS 2 support to mpd (Music Player Daemon).

mpDris2 is run in the user session and monitors a local or distant 
mpd server

Fedora Account System Username: ankursinha


Additional info: using it on my local system. Appears to work correctly.
Comment 1 Gianluca Sforna 2013-06-02 09:36:40 EDT
A friend asked me to package this, since you already submitted it I will review this instead :)
Comment 2 Gianluca Sforna 2013-06-02 11:31:23 EDT
Basic things first. Even if the tarball is called mpDris2 I would still prefer to call the package (and spec file) all lowercase. I checked and other distros are already doing that, makes it easier for command line users to get it right.

Package builds in mock for Fedora 19, rpmlint on the results says:

rpmlint /var/lib/mock/fedora-19-x86_64/result/*.rpm
mpDris2.noarch: W: spelling-error Summary(en_US) mpd -> mod, mp, pd
mpDris2.noarch: W: spelling-error %description -l en_US mpd -> mod, mp, pd
mpDris2.noarch: W: non-conffile-in-etc /etc/xdg/autostart/mpDris2.desktop
mpDris2.noarch: W: no-manual-page-for-binary mpDris2
mpDris2.noarch: W: file-not-in-%lang /usr/share/locale/fr/LC_MESSAGES/mpDris2.mo
mpDris2.src: W: spelling-error Summary(en_US) mpd -> mod, mp, pd
mpDris2.src: W: spelling-error %description -l en_US mpd -> mod, mp, pd
2 packages and 0 specfiles checked; 0 errors, 7 warnings.

We can pretty much ignore spelling warnings.

the .desktop in /etc/xdg/autostart is better to be marked as %config(noreplace). AFAIK, this is not on the guidelines but if an admin changes the file he expects changes to be preserved on upgrades (see for instance bug #842318)

Language file, even if it's only one, must be handled by %find_lang as shown in:
http://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files

Please fix these points and I think we are all set to approve the package.
Comment 3 Ankur Sinha (FranciscoD) 2013-06-20 21:23:09 EDT
Hi Gianluca,

Thanks for taking up the review. I've made fixes as requested:

* Fri Jun 21 2013 Ankur Sinha <ankursinha AT fedoraproject DOT org> 0.4-1
- #912048
- Use find_lang
- Rename to lowercase
- Patch desktop file
- Mark autostart file as config

Links:
Spec URL: http://ankursinha.fedorapeople.org/mpdris2/mpdris2.spec
SRPM URL: http://ankursinha.fedorapeople.org/mpdris2/mpdris2-0.4-1.fc19.src.rpm

rpmlint output:
[asinha@localhost  SRPMS]$ rpmlint ./mpdris2-0.4-1.fc19.src.rpm ../SPECS/mpdris2.spec /var/lib/mock/fedora-rawhide-x86_64/result/*.rpm
mpdris2.src: W: spelling-error Summary(en_US) mpd -> mod, mp, pd
mpdris2.src: W: spelling-error %description -l en_US mpd -> mod, mp, pd
mpdris2.noarch: W: spelling-error Summary(en_US) mpd -> mod, mp, pd
mpdris2.noarch: W: spelling-error %description -l en_US mpd -> mod, mp, pd
mpdris2.noarch: W: no-manual-page-for-binary mpDris2
mpdris2.src: W: spelling-error Summary(en_US) mpd -> mod, mp, pd
mpdris2.src: W: spelling-error %description -l en_US mpd -> mod, mp, pd
3 packages and 1 specfiles checked; 0 errors, 7 warnings.
[asinha@localhost  SRPMS]$

Thanks,
Warm regards,
Ankur
Comment 4 Gianluca Sforna 2013-08-19 03:32:59 EDT
Sorry for the delay. just noticed a minor issue: all documentation including COPYING should be packaged as %doc, for details refer to:

https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Documentation

and

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text

this is likely to require some other change to avoid installation under %{_docdir}
Comment 5 Ankur Sinha (FranciscoD) 2013-08-21 05:44:08 EDT
Since we've moved to unversioned docdirs, won't _docdir go to %doc any way?

/me isn't very sure about this himself.
Comment 6 Gianluca Sforna 2013-08-21 06:23:38 EDT
Even if they end up being in the same path, marking as %doc makes it possible to exclude them from package installation. I think this is the main reason why guidelines are very specific about it.
Comment 7 Ankur Sinha (FranciscoD) 2013-08-21 08:34:37 EDT
Updated: http://ankursinha.fedorapeople.org/mpdris2/mpdris2-0.4-1.fc19.src.rpm

I confirmed off the -devel IRC channel. If the documentation is already installed by the configuration system in %files, the %doc macro isn't necessary. For example:

[asinha@ankur  result]$ rpm -qpd ./mpdris2-0.4-1.fc20.noarch.rpm
/usr/share/doc/mpdris2/AUTHORS
/usr/share/doc/mpdris2/COPYING
/usr/share/doc/mpdris2/README
/usr/share/doc/mpdris2/mpDris2.conf

The conf file shows up in docs even though it wasn't added using %doc. 

Thanks,
Warm regards,
Ankur
Comment 8 Gianluca Sforna 2013-08-21 18:29:50 EDT
Fine then. I am approving this now, but please note: the license for the main script is listed in its header as GPLv3+. Please update the spec accordingly before uploading the package.

==== REVIEW CHECKLIST ====
- package named according to package naming guidelines
- package licensed with allowed license (GPLv3+)
- license file included in %doc
- spec written in American english
- spec legible
- sources match upstream sha256sum
- successfully builds in mock for rawhide x86_64
- locales installed with %find_lang
- no shared libraries
- package owns all directories it creates
- default file permissions
- macro usage is consistent
- package contains code
- no large documentation

- rpmlint is mostly silent, the remaining warnings can be ignored
Comment 9 Ankur Sinha (FranciscoD) 2013-08-21 19:45:51 EDT
Great. Thanks a bunch. I'll make the license tweak before publishing to SCM.

New Package SCM Request
=======================
Package Name: mpdris2
Short Description: Provide MPDRIS 2 support to mpd
Owners: ankursinha
Branches: F20 F19 F18
InitialCC:
Comment 10 Gwyn Ciesla 2013-08-22 08:12:49 EDT
Git done (by process-git-requests).
Comment 11 Fedora Update System 2013-08-22 08:59:25 EDT
mpdris2-0.4-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/mpdris2-0.4-1.fc18
Comment 12 Fedora Update System 2013-08-22 08:59:35 EDT
mpdris2-0.4-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/mpdris2-0.4-1.fc19
Comment 13 Fedora Update System 2013-08-22 20:32:37 EDT
Package mpdris2-0.4-1.fc19:
* should fix your issue,
* was pushed to the Fedora 19 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing mpdris2-0.4-1.fc19'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2013-15152/mpdris2-0.4-1.fc19
then log in and leave karma (feedback).
Comment 14 Fedora Update System 2013-12-05 05:28:45 EST
mpdris2-0.4-1.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 15 Fedora Update System 2013-12-05 05:29:58 EST
mpdris2-0.4-1.fc19 has been pushed to the Fedora 19 stable repository.  If problems still persist, please make note of it in this bug report.

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