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

[Guillaume Desmottes]

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? :)


> diff --git a/MANIFEST.in b/MANIFEST.in
> new file mode 100644
> index 0000000..6c81976
> --- /dev/null
> +++ b/MANIFEST.in
> @@ -0,0 +1,6 @@
> +include AUTHORS
> +include COPYING
> +include gadget.config
> +include gadget.tac
> +include test.py
> +inblude tools
> diff --git a/debian/changelog b/debian/changelog
> new file mode 100644
> index 0000000..8b27c61
> --- /dev/null
> +++ b/debian/changelog
> @@ -0,0 +1,5 @@
> +gadget (0.1-1) unstable; urgency=low
> +
> +  * Initial package. (Closes: #XXXXXX)
> +
> + -- Dafydd Harries <daf at debian.org>  Sat, 14 Jun 2008 01:41:03 +0100
> diff --git a/debian/compat b/debian/compat
> new file mode 100644
> index 0000000..7ed6ff8
> --- /dev/null
> +++ b/debian/compat
> @@ -0,0 +1 @@
> +5
> diff --git a/debian/control b/debian/control
> new file mode 100644
> index 0000000..d545585
> --- /dev/null
> +++ b/debian/control
> @@ -0,0 +1,12 @@
> +Source: gadget
> +Section: net
> +Priority: optional
> +Maintainer: Dafydd Harries <daf at debian.org>
> +Build-Depends: debhelper (>= 5), python-support
> +Standards-Version: 3.7.3
> +
> +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.

> +Description: XMPP server component for tracking people and activities
> + Blah blah blah.


I guess we could at least link http://wiki.laptop.org/go/Gadget in the
full description.


> diff --git a/debian/copyright b/debian/copyright
> new file mode 100644
> index 0000000..cfb68cc
> --- /dev/null
> +++ b/debian/copyright
> @@ -0,0 +1,24 @@
> +
> +Copyright:
> +
> +  2007-2008 One Laptop Per Child.
> +
> +Licence:
> +
> +  This program is free software; you can redistribute it and/or
modify
> +  it under the terms of the GNU General Public License as published
by
> +  the Free Software Foundation; either version 2 of the License, or
> +  (at your option) any later version.
> +
> +  This program is distributed in the hope that it will be useful,
> +  but WITHOUT ANY WARRANTY; without even the implied warranty of
> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +  GNU General Public License for more details.
> +
> +  You should have received a copy of the GNU General Public License
along
> +  with this program; if not, write to the Free Software Foundation,
Inc.,
> +  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> +
> +You can find the full text of the GPL in /usr/share/common-licenses.
The
> +Debian packaging is under the same licence.
> +
> diff --git a/debian/gadget.dirs b/debian/gadget.dirs
> new file mode 100644
> index 0000000..8309ce0
> --- /dev/null
> +++ b/debian/gadget.dirs
> @@ -0,0 +1 @@
> +/var/lib/gadget
> diff --git a/debian/gadget.docs b/debian/gadget.docs
> new file mode 100644
> index 0000000..e845566
> --- /dev/null
> +++ b/debian/gadget.docs
> @@ -0,0 +1 @@
> +README
> diff --git a/debian/gadget.init b/debian/gadget.init
> new file mode 100644
> index 0000000..cc4c762
> --- /dev/null
> +++ b/debian/gadget.init


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

> @@ -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
> +
> +# Exit if the package is not installed
> +[ -r "$TAC" ] || exit 0
> +
> +# Read configuration variable file if it is present
> +[ -r /etc/default/$NAME ] && . /etc/default/$NAME
> +
> +# Load the VERBOSE setting and other rcS variables
> +. /lib/init/vars.sh
> +
> +# Define LSB log_* functions.
> +# Depend on lsb-base (>= 3.0-6) to ensure that this file is present.
> +. /lib/lsb/init-functions
> +
> +export PYTHONPATH=/usr/share/gadget/
> +
> +#
> +# Function that starts the daemon/service
> +#
> +do_start()
> +{
> +	# Return
> +	#   0 if daemon has been started
> +	#   1 if daemon was already running
> +	#   2 if daemon could not be started
> +	start-stop-daemon --start --quiet --pidfile $PIDFILE --exec $DAEMON
--test > /dev/null \
> +		|| return 1
> +	start-stop-daemon --start --quiet --pidfile $PIDFILE --exec $DAEMON
--chdir $RUNDIR -- \
> +		$DAEMON_ARGS \
> +		|| return 2
> +	# Add code here, if necessary, that waits for the process to be
ready
> +	# to handle requests from services started subsequently which depend
> +	# on this one.  As a last resort, sleep for some time.
> +}
> +
> +#
> +# Function that stops the daemon/service
> +#
> +do_stop()
> +{
> +	# Return
> +	#   0 if daemon has been stopped
> +	#   1 if daemon was already stopped
> +	#   2 if daemon could not be stopped
> +	#   other if a failure occurred
> +	start-stop-daemon --stop --quiet --retry=TERM/30/KILL/5 --pidfile
$PIDFILE --name twistd
> +	RETVAL="$?"
> +	[ "$RETVAL" = 2 ] && return 2
> +	# Wait for children to finish too if this is a daemon that forks
> +	# and if the daemon is only ever run from this initscript.
> +	# If the above conditions are not satisfied then add some other code
> +	# that waits for the process to drop all resources that could be
> +	# needed by services started subsequently.  A last resort is to
> +	# sleep for some time.
> +	#start-stop-daemon --stop --quiet --oknodo --retry=0/30/KILL/5
--exec $DAEMON
> +	#[ "$?" = 2 ] && return 2
> +	# Many daemons don't delete their pidfiles when they exit.
> +	rm -f $PIDFILE
> +	return "$RETVAL"
> +}
> +
> +case "$1" in
> +  start)
> +	[ "$VERBOSE" != no ] && log_daemon_msg "Starting $DESC" "$NAME"
> +	do_start
> +	case "$?" in
> +		0|1) [ "$VERBOSE" != no ] && log_end_msg 0 ;;
> +		2) [ "$VERBOSE" != no ] && log_end_msg 1 ;;
> +	esac
> +	;;
> +  stop)
> +	[ "$VERBOSE" != no ] && log_daemon_msg "Stopping $DESC" "$NAME"
> +	do_stop
> +	case "$?" in
> +		0|1) [ "$VERBOSE" != no ] && log_end_msg 0 ;;
> +		2) [ "$VERBOSE" != no ] && log_end_msg 1 ;;
> +	esac
> +	;;
> +  restart|force-reload)
> +	#
> +	# If the "reload" option is implemented then remove the
> +	# 'force-reload' alias
> +	#
> +	log_daemon_msg "Restarting $DESC" "$NAME"
> +	do_stop
> +	case "$?" in
> +	  0|1)
> +		do_start
> +		case "$?" in
> +			0) log_end_msg 0 ;;
> +			1) log_end_msg 1 ;; # Old process is still running
> +			*) log_end_msg 1 ;; # Failed to start
> +		esac
> +		;;
> +	  *)
> +	  	# Failed to stop
> +		log_end_msg 1
> +		;;
> +	esac
> +	;;
> +  *)
> +	echo "Usage: $SCRIPTNAME {start|stop|restart|force-reload}" >&2
> +	exit 3
> +	;;
> +esac
> +
> +:
> 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?

> 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
> new file mode 100755
> index 0000000..78db65b
> --- /dev/null
> +++ b/debian/rules
> @@ -0,0 +1,29 @@
> +#!/usr/bin/make -f
> +
> +clean:
> +	dh_clean
> +
> +build:
> +
> +binary-arch:
> +
> +binary-indep:
> +	dh_testdir
> +	dh_install -X.pyc
> +	dh_installdirs
> +	dh_installdocs
> +	dh_installchangelogs
> +	dh_installinit
> +	dh_installdeb
> +	dh_pysupport
> +	dh_link
> +	dh_compress
> +	dh_fixperms
> +	dh_gencontrol
> +	dh_md5sums
> +	dh_builddeb
> +
> +binary: binary-arch binary-indep
> +
> +.PHONY: clean binary-arch binary-indep binary
> +
> diff --git a/setup.py b/setup.py
> new file mode 100644
> index 0000000..1aa3fea
> --- /dev/null
> +++ b/setup.py
> @@ -0,0 +1,8 @@
> +
> +from distutils.core import setup
> +
> +setup(
> +    name='gadget',
> +    version='0.1',
> +    packages=['gadget'])
> +