Bug#659822: RFS: mpd-sima/0.9.0-1 (New upstream version)
Hi Benoît,
First, thanks for your excellent review :)
Le 16/02/2012 12:51, Benoît Knecht a écrit :
> Geoffroy Youri Berret wrote:
> > I am looking for a sponsor for my package "mpd-sima".
> > […]
>
> I had a look at your package, here are my comments:
>
> - In debian/copyright, […]
Corrected, thanks.
> - I think the long description could be reworded a little bit; you
> mention Python twice and "to feed MPD playlist with artist similar
> to your currently playing track" sounds a bit wrong (should be "your
> MPD playlist" and "artists" or "tracks from artists", or something
> like that.
Right, i've changed the description.
> - In debian/control, Recommends is empty; you should remove it.
Ok,
I also remove useless Build-Depends-Indep and the debian/clean files.
They were forgotten settings from previous package version.
In mpd-sima/0.8.0-x I used to build manuals because upstream did only provide
xml source but not build manual. This has changed with 0.9.*.
> - In debian/mpd-sima.postrm, you use awk but you don't Depend on it.
Right, I replaced it with a “grep | cut” alternative.
> You're also checking if /usr/sbin/deluser is executable and
> silently not removing the user if it's not (same thing for
> delgroup). Since you Depend on adduser, you should assume these
> commands exist, and it should be an error visible to the user if
> they don't.
Corrected as well.
> - In debian/mpd-sima.postinst, since mpd-sima is not meant to be run
> as root, it might be a good idea to run it after creating the user
> and group, as the the mpd-sima user.
> On second thought, why even do this during installation? The init
> script seems like a better place to do this.
The purpose of this behaviour in maint script is only to ensure the database
is created within /var/lib/mpd-sima and that proper rights are set on it (ie.
#637192). But this is not mandatory for the dæmon to start, this is just the
vanilla setup I propose for the package.
I don't want to enforce this choice in the init script.
I prefer to leave the init script as plain as possible, for power users to
tweak it.
The init script is only controlling the access to the directory in which lies
the database but the existence of the database itself is not required.
It is created, if missing, at dæmon start.
> - Regarding debian/wrappers, why not intall the python modules
> some place where python can find them by default?
Well, these are not pure python modules but python applications.
Hope I got you right, but they are private module which should stay private
and should not get into python name space. Hence the shell wrappers.
> And I think first line should read "#!/bin/sh", as outlined in
> debian policy 10.4.
I guess I got confused by the python policy recommending a "/usr/bin/env"
sha-bang. Corrected as well.
Thanks for noticing :)
> - Finally, a word on the man pages. I'll focus on mpd-sima.cfg.1, but
> some of it applies to the other man pages too.
> […]
Thanks.
Since these man pages are maintained upstream I'll have these corrections made
for the next release. I don't consider them major problems for the package.
Thanks very much for the review.
I've commited changes on alioth.
A new version of the package is available on mentors.d.e
> dget -x http://mentors.debian.net/debian/pool/main/m/mpd-sima/mpd-sima_0.9.0-1~5.gbpbd26fd.dsc
> git clone git://git.debian.org/pkg-multimedia/mpd-sima.git
Cheers,
Geoff
Reply to: