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

[Dafydd Harries]

Ar 24/06/2008 am 11:39, ysgrifennodd Guillaume Desmottes:
> > 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.

I checked for such a virtual package but there doesn't seem to be one.

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

Good idea. There's a "Homepage" field we can use for that.

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

Indeed, it's based on /etc/init.d/skeleton. It sets DAEMON=twistd, so twistd
is used to start Gadget. That also takes care of creating the PID file and
setting up the log. As far as I can tell, the standard way to stop a Twisted
service is to send SIGTERM to the PID in the PID file.


> > @@ -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?

Files installed to /etc are automatically marked as configuration files, so we
don't need to do anything.

-- 
Dafydd