Bug 477542

Summary: Review Request: mpdscribble - A mpd client which submits information about tracks being played to Last.fm
Product: [Fedora] Fedora Reporter: Jaroslaw Gorny <jaroslaw.gorny>
Component: Package ReviewAssignee: Peter Lemenkov <lemenkov>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: atorkhov, avm-xandry, fedora-package-review, lemenkov, notting
Target Milestone: ---Keywords: Reopened
Target Release: ---Flags: lemenkov: fedora-review+
kevin: fedora-cvs+
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-11-10 21:10:10 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Jaroslaw Gorny 2008-12-21 19:40:20 UTC
Spec URL: http://strony.aster.pl/muranow/fedora/mpdscribble.spec
SRPM URL: http://strony.aster.pl/muranow/fedora/mpdscribble-0.13-1.fc10.src.rpm
Description: 
mpdscribble is a music player daemon (mpd) client which submits information
about tracks being played to Last.fm (formerly audioscrobbler).

This is my first package, I'll need a sponsor.

Comment 1 Peter Lemenkov 2009-02-02 11:24:23 UTC
I'll review it.

Comment 2 Peter Lemenkov 2009-02-02 12:08:54 UTC
REVIEW:

- You packaged relatively old version - 0.13, although they released v 0.16.
- rpmlint is not silent:

[petro@host-12-116 SPECS]$ rpmlint ~/Desktop/mpdscribble-0.13-1.fc10.*
mpdscribble.i386: W: incoherent-version-in-changelog 0.13.1 ['0.13-1.fc10', '0.13-1']
2 packages and 0 specfiles checked; 0 errors, 1 warnings.
[petro@host-12-116 SPECS]

Please  use %{version}-%{release} w/o .fc10 suffix in %changelog. For example, if %{version} is 0.13 and release is 1%{?dist} you should use 0.13-1 as version tag in %changelog.

+ The package is named according to the Package Naming Guidelines .
+ The spec file name matches the base package %{name}, in the format %{name}.spec unless your package has an exemption.
+/- The package meets the Packaging Guidelines.  After brief chacking, I found only one *possible* issue - this package ships (and was built against) libmpdclient library ( http://mpd.wikia.com/wiki/ClientLib:libmpdclient ). The Guidelines has statement that we must avoid using such libraries if they already exists in system. This library, libmpdclient still not included in main Fedora repository (therefore it doesn't a problem), however it might be included in the future (and will be a problem). You should watch this issue.
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
+ The License field in the package spec file matches the actual license.
+ The file, containing the text of the license(s), is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package matches the upstream source, as provided in the spec URL.

[petro@host-12-116 SOURCES]$ md5sum mpdscribble-0.13.tar.gz*
884717131356ad2f62918d458258b443  mpdscribble-0.13.tar.gz
884717131356ad2f62918d458258b443  mpdscribble-0.13.tar.gz.from_srpm
[petro@host-12-116 SOURCES]$


+ The package MUST successfully compile and build into binary rpms on at least one primary architecture.

http://koji.fedoraproject.org/koji/taskinfo?taskID=1098823

+ All build dependencies are listed in BuildRequires.
+ No need to handle locales.
+ No shared library files.
+ The package is not designed to be relocatable.
+ The package owns all directories that it creates.
+ The package does not contain any duplicate files in the %files listing.
+ Permissions on files are set properly.
+ The package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT.
+ The package consistently uses macros.
+ The package contains code, or permissable content.
+ No large documentation files.
+ Everything, the package includes as %doc, does not affect the runtime of the application.
+ No header files
+ No static libraries
+ No pkgconfig(.pc) files
+ No library files with a suffix
+ No devel packages
+ No .la libtool archives
+ Not a GUI application
+ The package does not own files or directories already owned by other packages.
+ At the beginning of %install, the package runs rm -rf $RPM_BUILD_ROOT.
+ All filenames in the package are valid UTF-8.

So here are my suggestions:

* update srpm to latest 0.16 (or explain why you won't do it).
* fix %changelog versioning entry
* consider packaging libmpdclient also (not a blocker)

Comment 3 Jaroslaw Gorny 2009-02-03 17:53:22 UTC
* I've updated to latest version (0.16) - 0.13 was latest when I packaged it.
* %changelog fixed
So rpmlint is quiet - at least here.
Results are here:
SRPM URL: http://strony.aster.pl/muranow/fedora/mpdscribble-0.16-1.fc10.src.rpm
SPEC URL: http://strony.aster.pl/muranow/fedora/mpdscribble.spec

OK, I'll consider to package libmpdclient too ;)

Comment 4 Peter Lemenkov 2009-02-04 08:54:44 UTC
OK, some additional steps (because of updated srpm):

+ The sources used to build the package matches the upstream source, as
provided in the spec URL.

[petro@host-12-116 SOURCES]$ md5sum mpdscribble-0.16.tar.bz2*
deeeb403ebd50b7abacd1eeabc96b320  mpdscribble-0.16.tar.bz2
deeeb403ebd50b7abacd1eeabc96b320  mpdscribble-0.16.tar.bz2.from_srpm
[petro@host-12-116 SOURCES]$

+ The package successfully compiles and builds into binary rpms on all primary architectures.

http://koji.fedoraproject.org/koji/taskinfo?taskID=1103246

This package is 

==============
==============
== APPROVED ==
==============
==============


Your next steps are:

* Wait for someone, who sponsors you (I cannot sponsor you).
* Request cvs branches (see https://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure )
* Proceed from step 8 at this page - https://fedoraproject.org/wiki/PackageMaintainers/NewPackageProcess

Comment 5 Peter Lemenkov 2009-03-01 08:37:31 UTC
Ping! Any news?

Comment 6 Jaroslaw Gorny 2009-03-01 09:46:29 UTC
Seems like nobody wants to sponsor me (this package). Without that I think making this 2 next steps doesn't make any sense. Am I right?

Comment 7 Jason Tibbitts 2009-09-22 23:41:05 UTC
I was cleaning up the sponsorship queue and found this package.   Jaroslaw, are you still interested in submitting this package?  Peter technically has the ability to sponsor you; I don't know if he gained that within the last six months, but at this point it makes no difference.  Peter, Jaroslaw is still in the sponsorship queue (FAS ID jaroslav); did you want to get this moving?  What else would be required to force some progress here?

Comment 8 xandry 2009-10-20 16:52:00 UTC
Hello all. The new version of mpdscribble:
SPEC URL: http://narod.ru/disk/14320486000/mpdscribble.spec.html
SRPM URL: http://narod.ru/disk/14320467000/mpdscribble-0.18.1-1.fc11.src.rpm.html

Comment 9 Peter Lemenkov 2009-10-21 05:12:53 UTC
OK, since xandry decided to continue Jaroslaw's work, I'm closing this now.

Jaroslaw, thanks for your efforts in packaging this.

*** This bug has been marked as a duplicate of bug 529917 ***

Comment 10 Jaroslaw Gorny 2009-10-21 10:26:29 UTC
I feel somehow dissappointed. I wanted to maintain this package. But if Fedora doesn't need new people - very well.

Comment 11 Peter Lemenkov 2009-10-21 10:51:29 UTC
I thought, that you're gone, Jaroslaw. I'm sorry for inconvinience, and, actually, I'm glad to see you back.

Returning to topic - now, I can sponsor you.

Comment 12 Peter Lemenkov 2009-10-21 11:50:32 UTC
Removing FE_NEEDSPONSOR - I just sponsored Jaroslaw.

Comment 13 Peter Lemenkov 2009-10-21 12:00:19 UTC
Jaroslaw, please, update package up to the latst version. Also, please, consider adding init-script, since mpdscribble can act as a daemon. I created one, but didn't test it yet (however it looks sane):

http://peter.fedorapeople.org/stuff/mpdscribble.init

Comment 14 Jaroslaw Gorny 2009-10-26 18:21:35 UTC
Peter, I need your help,

I've packaged new version (0.18.1) but there's one problem. They (upstream) set permissions for /etc/mpdscribble.conf to be "0600". We have usually "0644" and this is what rpmlint expects. So I have:
<code>
[makerpm@moonstone i686]$ rpmlint mpdscribble-0.18.1-1.fc12.i686.rpm
mpdscribble.i686: E: non-readable /etc/mpdscribble.conf 0600
1 packages and 0 specfiles checked; 1 errors, 0 warnings.
</code>

I have two possibilities - make a patch, or ignore rpmlint. Which one to chose?

Comment 15 xandry 2009-10-26 19:15:40 UTC
There is no need to expose to this file of the right. There by default all becomes as it is necessary.

Comment 16 Jaroslaw Gorny 2009-10-26 19:55:16 UTC
(In reply to comment #15)
> There is no need to expose to this file of the right. There by default all
> becomes as it is necessary.  

@xandry: I changed this using %attr(). Now rpmlint does not complain. I've installed this package and file permissions are correct.

@Peter: Init script is attached, looks sane, appropriate %post and %preun sections are added.

New version of package is available:
RPM:
http://strony.aster.pl/muranow/fedora/mpdscribble-0.18.1-1.fc12.i686.rpm
http://strony.aster.pl/muranow/fedora/mpdscribble-debuginfo-0.18.1-1.fc12.i686.rpm

SRPM:
http://strony.aster.pl/muranow/fedora/mpdscribble-0.18.1-1.fc12.src.rpm

SPEC:
http://strony.aster.pl/muranow/fedora/mpdscribble.spec

Peter, can I set 'fedora-cvs' flag to '?' now?

Comment 17 Peter Lemenkov 2009-10-27 08:05:12 UTC
(In reply to comment #16)

> @xandry: I changed this using %attr(). Now rpmlint does not complain. I've
> installed this package and file permissions are correct.

First, I'm afraid, that you should package it as is, w/o changing permissions. There is one possible user case - running mpdscribble as a server on an iron with multiple user accounts, and server's admin would like to hide last.fs account settings from others.

Second, I don't see a reason in using killall in %preun section. I think that killing all instances of the app, started by other users, is somewhat improper.

Also, I advice you a little simplification. Instead of

#init scripts
install -d $RPM_BUILD_ROOT/%{_sysconfdir}/rc.d/init.d
install -m 0755 %{SOURCE1} $RPM_BUILD_ROOT/%{_sysconfdir}/rc.d/init.d/%{name}

you may write

install -D -m 0755 %{SOURCE1} $RPM_BUILD_ROOT/%{_sysconfdir}/rc.d/init.d/%{name}

or even

install -D -m 0755 %{SOURCE1} $RPM_BUILD_ROOT/%{_initddir}/%{name}

Please, note, that %{_initddir} is unavailable on RHEL/CentOS, so if you plan to push the application into EPEL also, then you should consider using %{_initrddir} instead.


Other things looks sane.

Please, comment my notes, and we'll continue our journey.

Comment 18 Jaroslaw Gorny 2009-10-27 08:37:52 UTC
Peter,
find my answers below:

(In reply to comment #17)
> (In reply to comment #16)
> 
> > @xandry: I changed this using %attr(). Now rpmlint does not complain. I've
> > installed this package and file permissions are correct.
> 
> First, I'm afraid, that you should package it as is, w/o changing permissions.
> There is one possible user case - running mpdscribble as a server on an iron
> with multiple user accounts, and server's admin would like to hide last.fs
> account settings from others.
> 

You're absolutely right. But on the other hand rpmlint is mandatory step in package approval. So how are we going to deal with it?


> Second, I don't see a reason in using killall in %preun section. I think that
> killing all instances of the app, started by other users, is somewhat improper.
>

Yes, your're  right. I'll change this. I took this %preun scriptlet from wpa_supplicant package - it makes sense there ;)

> Also, I advice you a little simplification. Instead of
> 
> #init scripts
> install -d $RPM_BUILD_ROOT/%{_sysconfdir}/rc.d/init.d
> install -m 0755 %{SOURCE1} $RPM_BUILD_ROOT/%{_sysconfdir}/rc.d/init.d/%{name}
> 
> you may write
> 
> install -D -m 0755 %{SOURCE1}
> $RPM_BUILD_ROOT/%{_sysconfdir}/rc.d/init.d/%{name}


OK. I'll merge those two lines into one.

> Please, note, that %{_initddir} is unavailable on RHEL/CentOS, so if you plan
> to push the application into EPEL also, then you should consider using
> %{_initrddir} instead.
> 

Hmm. This is the reason I used %{_sysconfdir}/rc.d/init.d
Even if I'll not push the app into EPEL I think it's better to use generic macros. And personally I hate %{_initrddir} because the macro is confusing - it suggests it has something to do with initrd :/
So I'd rather keep %{_sysconfdir}/rc.d/init.d but if you insist - I'll change it to %{_initddir}.

I'll prepare new package in the evening (don't have Fedora here).

Thanks once again for your help,
J.

Comment 19 Peter Lemenkov 2009-10-27 08:45:54 UTC
(In reply to comment #18)

> > > @xandry: I changed this using %attr(). Now rpmlint does not complain. I've
> > > installed this package and file permissions are correct.

> > First, I'm afraid, that you should package it as is, w/o changing permissions.
> > There is one possible user case - running mpdscribble as a server on an iron
> > with multiple user accounts, and server's admin would like to hide last.fs
> > account settings from others.

> You're absolutely right. But on the other hand rpmlint is mandatory step in
> package approval. So how are we going to deal with it?

In this particular case we may ignore this rpmlint message.


> > Please, note, that %{_initddir} is unavailable on RHEL/CentOS, so if you plan
> > to push the application into EPEL also, then you should consider using
> > %{_initrddir} instead.

> Hmm. This is the reason I used %{_sysconfdir}/rc.d/init.d

Then you should (for consistency) change %files section. Instead of 

%{_initddir}/%{name}

you should add

%{_sysconfdir}/rc.d/init.d/%{name}


> So I'd rather keep %{_sysconfdir}/rc.d/init.d but if you insist - I'll change
> it to %{_initddir}.

No, I'm not insisting here - feel free to decide which one to choose by yourself :)

Comment 20 Peter Lemenkov 2009-10-27 08:58:06 UTC
*** Bug 529917 has been marked as a duplicate of this bug. ***

Comment 21 Jaroslaw Gorny 2009-10-27 20:50:45 UTC
OK, I've corrected spec file and rebuilt package.

I haven't changed release number, I'd like to keep rel=1 as initial build in official system.

New version of package is available under the same address:
RPM:
http://strony.aster.pl/muranow/fedora/mpdscribble-0.18.1-1.fc12.i686.rpm
http://strony.aster.pl/muranow/fedora/mpdscribble-debuginfo-0.18.1-1.fc12.i686.rpm

SRPM:
http://strony.aster.pl/muranow/fedora/mpdscribble-0.18.1-1.fc12.src.rpm

SPEC:
http://strony.aster.pl/muranow/fedora/mpdscribble.spec

Comment 22 Peter Lemenkov 2009-10-28 09:10:19 UTC
One small issue still remains (fortunately, the last) - a path for the pid-file is set to /var/run/$prog/$prog.pid, but you didn't list /var/run/${name} in the %files section.

Creating extra directory in /var/run, for storing pid-file, is an obvious solution for permissions issue, when main application runs from different user. Since mpdscribble runs from root, it doesn't make any sense.

So, I advice you to change variable $pidfile in the init-script to 
/var/run/$prog.pid

Comment 23 Jaroslaw Gorny 2009-10-28 13:00:17 UTC
OK, done. Fresh packages are available under the same links as before.

Comment 24 Peter Lemenkov 2009-10-28 13:10:12 UTC
Ok, looks fine.

Please, proceed with fedora-cvs flag.

Comment 25 Kevin Fenzi 2009-10-29 00:11:06 UTC
Please enter a cvs request template here so we know what branches you want and such. 

https://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure

Set the fedora-cvs flag again when you have done so.

Comment 26 Jaroslaw Gorny 2009-10-29 20:25:18 UTC
New Package CVS Request
=======================
Package Name: mpdscribble
Short Description: A mpd client which submits information about tracks being played to last.fm
Owners: jaroslav
Branches: F-11
InitialCC: peter

Comment 27 Kevin Fenzi 2009-10-31 23:53:04 UTC
cvs done with F-12 branch added.

Comment 28 Jaroslaw Gorny 2009-11-10 21:10:10 UTC
Package: mpdscribble-0.18.1-1.fc13
Tag: dist-f13
Status: complete
Built by: jaroslav
ID: 140666
Started: Tue, 10 Nov 2009 21:02:33 UTC
Finished: Tue, 10 Nov 2009 21:05:32 UTC

Comment 29 Fedora Update System 2009-11-13 20:13:38 UTC
mpdscribble-0.18.1-1.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/mpdscribble-0.18.1-1.fc12

Comment 30 Fedora Update System 2009-11-13 20:13:46 UTC
mpdscribble-0.18.1-1.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/mpdscribble-0.18.1-1.fc11

Comment 31 Fedora Update System 2009-11-25 15:26:07 UTC
mpdscribble-0.18.1-1.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 32 Fedora Update System 2009-11-25 15:33:17 UTC
mpdscribble-0.18.1-1.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.