Bug 912048

A friend asked me to package this, since you already submitted it I will review this instead :)

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.

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

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}

Since we've moved to unversioned docdirs, won't _docdir go to %doc any way?

/me isn't very sure about this himself.

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.

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

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

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:

Git done (by process-git-requests).

mpdris2-0.4-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/mpdris2-0.4-1.fc18

mpdris2-0.4-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/mpdris2-0.4-1.fc19

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).

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.

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.