[Code-review] review of Daf's debian gadget branch

[Sjoerd Simons]

On Tue, Jun 24, 2008 at 11:39:56AM +0200, Guillaume Desmottes wrote:
> Here is my review of
> https://dev.laptop.org/git?p=users/daf/gadget;a=shortlog;h=debian
> 
> I'm not a packager guru so it would be good if someone with real
> packaging skills could review this branch too. Sjoerd? :)

The branch looks good to me, but i have some comments on your comments :)

> > +Package: gadget
> > +Architecture: all
> > +Depends: ${python:Depends}, ${misc:Depends}, python-twisted-words
> 
> Maybe we should depend on ejabberd or openfire? Don't know if it's
> possible to depend on a "some jabber server" metapackage or something.

You shouldn't depend on a jabber server as you can install the actual jabber
server and components on different machines. A suggest would probably be nice
though. There isn't a jabber server metapackage at this point (although i've
heard some discussions about it), so what a package like pymsnt does is to jsut
Suggest: jabberd2 | jabber | ejabberd (openfire isn't in debian)

> I guess you reused an existing working init file. It doesn't seem to use
> twisted for the daemon management, is that intentional?

All debian systems have skeleton init script in /etc/init.d/skeleton, which is
what has been used to create this one from the looks of it. Do note that twistd
is actually used as the daemon, but maybe you meant something else (i'm not
very familiar with twisted)

> > @@ -0,0 +1,133 @@
> > +#! /bin/sh
> > +### BEGIN INIT INFO
> > +# Provides:          gadget
> > +# Required-Start:    $remote_fs $network
> > +# Required-Stop:     $remote_fs $network
> > +# Default-Start:     2 3 4 5
> > +# Default-Stop:      0 1 6
> > +# Short-Description: Gadget XMPP component
> > +# Description:       Gadget XMPP component
> > +### END INIT INFO
> > +
> > +# Author: Dafydd Harries <daf at debian.org>
> > +
> > +# Do NOT "set -e"
> > +
> > +# PATH should only include /usr/* if it runs after the mountnfs.sh
> script
> > +PATH=/sbin:/usr/sbin:/bin:/usr/bin
> > +DESC="Gadget XMPP component"
> > +NAME=gadget
> > +TAC=/usr/share/gadget/gadget.tac
> > +PIDFILE=/var/run/$NAME.pid
> > +LOGFILE=/var/log/gadget.log
> > +RUNDIR=/var/lib/gadget
> > +DAEMON=/usr/bin/twistd
> > +DAEMON_ARGS="--logfile $LOGFILE --pidfile $PIDFILE --rundir $RUNDIR
> -oy $TAC"
> > +SCRIPTNAME=/etc/init.d/$NAME


> > diff --git a/debian/gadget.install b/debian/gadget.install
> > new file mode 100644
> > index 0000000..a853a6e
> > --- /dev/null
> > +++ b/debian/gadget.install
> > @@ -0,0 +1,3 @@
> > +gadget /usr/share/gadget
> > +gadget.tac /usr/share/gadget
> > +gadget.config /etc/gadget
> 
> IIRC Debian has some magic to manage configuration files. Shouldn't we
> use that for this file?

Everything installed in /etc is marked as a config file automagically and dpkg
will handle them as such. You don't need to mark them specifically. If you want
to check see /var/lib/dpkg/info/<packagename>.conffiles after installation.

> > diff --git a/debian/gadget.links b/debian/gadget.links
> > new file mode 100644
> > index 0000000..481661b
> > --- /dev/null
> > +++ b/debian/gadget.links
> > @@ -0,0 +1 @@
> > +/etc/gadget/gadget.config /var/lib/gadget/gadget.config
> > diff --git a/debian/rules b/debian/rules

/var/lib/gadget/gadget.config ? Does gadget get it's config from
/var/lib/gadget, that seems a bit odd

  Sjoerd
-- 
Parallel lines never meet, unless you bend one or both of them.