Bazaar

Merge lp:~johnf-inodes/bzr/serve-init into lp:~bzr/bzr/trunk-old

  1. serve-init
  2. Merge into trunk-old
Proposed by John A Meinel
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: not available
Merge reported by: Andrew Bennetts
Merged at revision: not available
Proposed branch: lp:~johnf-inodes/bzr/serve-init
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 165 lines
2 files modified
contrib/debian/default (+20/-0)
contrib/debian/init.d (+135/-0)
To merge this branch: bzr merge lp:~johnf-inodes/bzr/serve-init
Reviewer Review Type Date Requested Status
Vincent Ladeuil Approve
John A Meinel Needs Information
Andrew Bennetts Approve
bzr-core Pending
Review via email: mp+12938@code.launchpad.net

This proposal supersedes a proposal from 2009-09-02.

To post a comment you must log in.
Revision history for this message
John Ferlito (johnf-inodes) wrote : Posted in a previous version of this proposal

At a debian init script to contrib to run bzr --serve at boot

Revision history for this message
John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

johnf wrote:
> johnf has proposed merging lp:~johnf-inodes/bzr/serve-init into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> At a debian init script to contrib to run bzr --serve at boot
>

 review: needs_information

If this is just a suggestion script, it seems fine. However, I don't
think we want to default to installing it whenever someone installs bzr.
And from my experience in the past, stuff that gets put into contrib
somehow magically makes its way into my /etc directory. (Witness the
previous 'bash autocompletion' issue.)

So if you can confirm that after adding this the built deb will put it
somewhere in '/usr/share/doc' or something similar rather than directly
into /etc/init.d then I'm happy to bring it in.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkqet9AACgkQJdeBCYSNAANIMACgmJmZky7E07T7UIuVvHoozrqL
bVcAn3YAc91z8X0UH/7HR+eHxkIbicyL
=gVS1
-----END PGP SIGNATURE-----

review: Needs Information
Revision history for this message
John Ferlito (johnf-inodes) wrote : Posted in a previous version of this proposal

> If this is just a suggestion script, it seems fine. However, I don't
> think we want to default to installing it whenever someone installs bzr.
> And from my experience in the past, stuff that gets put into contrib
> somehow magically makes its way into my /etc directory. (Witness the
> previous 'bash autocompletion' issue.)
>
>
> So if you can confirm that after adding this the built deb will put it
> somewhere in '/usr/share/doc' or something similar rather than directly
> into /etc/init.d then I'm happy to bring it in.

So my intention is indeed to put this into the debian package unless there is serious opposition to it.

I've updated my patch with the bit I forgot which is to make it disabled by default. (Not sure if I need to ask for another merge request since it isn't showing up in the diff).

My opinion is that it is the sort of thing that isn't worth bothering with unless it is easily enabled by editing something like /etc/default/bzr. If it sits in /usr/doc then as a sysadmin I need to check that everytime I upgrade for bigfixes and new functionality etc.

I think having it disabled by default, where enabling it is as simple as editing /etc/default/bzr is an acceptable alternative.

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

2009/9/3 johnf <email address hidden>:
> I think having it disabled by default, where enabling it is as simple as editing /etc/default/bzr is an acceptable alternative.

I think you want to be pretty careful that people understand it will
make the relevant directory world-readable, unless limited. But aside
from that, it does seem reasonably consistent with debian practice.

--
Martin <http://launchpad.net/~mbp/>

Revision history for this message
Robert Collins (lifeless) wrote : Posted in a previous version of this proposal

you need to 'resubmit' to make the diff update.

Policy for Ubuntu is that things installed by default should not listen
on the net, and I definitely think that installing bzr shouldn't start a
server by default.

Either a server specific package, or editing /etc/default/bzr sound fine
to me.

-Rob

Revision history for this message
John Ferlito (johnf-inodes) wrote : Posted in a previous version of this proposal

> Policy for Ubuntu is that things installed by default should not listen
> on the net, and I definitely think that installing bzr shouldn't start a
> server by default.

I've added a warning on the defaults file and changed it to listening on localhost by default

Revision history for this message
Andrew Bennetts (spiv) wrote :

I haven't read the script carefully, but the defaults look sane and well commented, and I assume the init.d script works as intended on Debian.

It seems like a useful thing to include in contrib/ too, so it looks good to me.

I'm not sure what the copyright assignment policy is for the contents of contrib, though. I suppose either:

 a) you should sign the agreement at <http://www.canonical.com/contributors> (if you haven't already) and put boilerplate "copyright Canonical, available under GPL" comments in the files, or
 b) put boilerplate saying it's copyright you, and some appropriate licence.

I don't know if (b) is an option.

review: Approve
Revision history for this message
Andrew Bennetts (spiv) wrote :

I wrote:
> b) put boilerplate saying it's copyright you, and some appropriate licence.
>
> I don't know if (b) is an option.

Actually, I see that at least contrib/bzr_access explicitly has copyright owned by someone other than Canonical, so I suppose this is allowed, or at least has been in the past.

Revision history for this message
John A Meinel (jameinel) wrote :

> I wrote:
> > b) put boilerplate saying it's copyright you, and some appropriate licence.
> >
> > I don't know if (b) is an option.
>
> Actually, I see that at least contrib/bzr_access explicitly has copyright
> owned by someone other than Canonical, so I suppose this is allowed, or at
> least has been in the past.

So while it may be a bit ambiguous, I think it would be best if we could get the contributor assignment signed.

Johnf is that okay for you? If it is a problem, then certainly we can talk about it more.

The details should be here:
  http://www.canonical.com/contributors

Revision history for this message
John A Meinel (jameinel) wrote :

# Specify startup options for bzr --serve

^- shouldn't that be "for bzr serve"

Also, shouldn't DAEMON_ARGS default to "serve --port..."

The way you have it written, seems like it will try to run:

 "bzr --port=127.0.0.0:4155"

Which wouldn't actually do anything.

So I think it needs a couple small tweaks, and it would probably be good to get this licensed as (c) Canonical if johnf doesn't object. (It makes managing the code base easier when we don't have to think again about the license. if it is a problem for johnf, then we can consider further.)

review: Needs Fixing
Revision history for this message
John Ferlito (johnf-inodes) wrote :

On Thu, Oct 08, 2009 at 05:59:29PM -0000, John A Meinel wrote:
> Review: Needs Fixing
> # Specify startup options for bzr --serve
>
> ^- shouldn't that be "for bzr serve"

Nice catch. Pushed.

> Also, shouldn't DAEMON_ARGS default to "serve --port..."
>
> The way you have it written, seems like it will try to run:
>
> "bzr --port=127.0.0.0:4155"
>
> Which wouldn't actually do anything.

The init script does

    bzr serve $DAEMON_ARGS --directory $REPO

so it should o the right thing.

> So I think it needs a couple small tweaks, and it would probably be good to get this licensed as (c) Canonical if johnf doesn't object. (It makes managing the code base easier when we don't have to think again about the license. if it is a problem for johnf, then we can consider further.)

I'll sort that out shortly.

Cheers,
John

--
John
Blog http://www.inodes.org
LCA2010 http://www.lca2010.org.nz

Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

John Ferlito wrote:
> On Thu, Oct 08, 2009 at 05:59:29PM -0000, John A Meinel wrote:
>> Review: Needs Fixing
>> # Specify startup options for bzr --serve
>>
>> ^- shouldn't that be "for bzr serve"
>
> Nice catch. Pushed.
>
>> Also, shouldn't DAEMON_ARGS default to "serve --port..."
>>
>> The way you have it written, seems like it will try to run:
>>
>> "bzr --port=127.0.0.0:4155"
>>
>> Which wouldn't actually do anything.
>
> The init script does
>
> bzr serve $DAEMON_ARGS --directory $REPO
>
> so it should o the right thing.

Yeah, I missed it because of the line wrap and '--'
+ start-stop-daemon --start --quiet --make-pidfile --pidfile $PIDFILE \
+ --chuid $USER:$GROUP --background --exec $DAEMON --test > /dev/null \
+ || return 1
+ start-stop-daemon --start --quiet --make-pidfile --pidfile $PIDFILE \
+ --chuid $USER:$GROUP --background --exec $DAEMON -- \
+ serve $DAEMON_ARGS --directory $REPO \
+ || return 2

^- --exec $DAEMON -- serve ...

I see it now, but it certainly wasn't obvious at first.

>
>
>> So I think it needs a couple small tweaks, and it would probably be good to get this licensed as (c) Canonical if johnf doesn't object. (It makes managing the code base easier when we don't have to think again about the license. if it is a problem for johnf, then we can consider further.)
>
> I'll sort that out shortly.
>
> Cheers,
> John
>

Thanks.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrP/70ACgkQJdeBCYSNAAO3ZgCeN6eysLXon/LPQm9Qo3Kw+gVc
D84An33yvK/NVEt6+vE5imoiHa/NbWM2
=DwMq
-----END PGP SIGNATURE-----

Revision history for this message
John A Meinel (jameinel) wrote :

I'm marking this back as Needs Review, as it isn't quite clear why it didn't land. Was it just the getting the CCA done? Has johnf done that by now?

review: Needs Information
Revision history for this message
Vincent Ladeuil (vila) wrote :

@johnf: It seems that you're still not part of https://edge.launchpad.net/~contributor-agreement-canonical/+members#active

Is there a related problem ? Can we help ?

Revision history for this message
Vincent Ladeuil (vila) wrote :

@poolie: johnf mentioned on IRC that you should have received his agreement for quite some time.

Please land as soon as this is sorted out, thanks in advance.

review: Approve
Revision history for this message
John Ferlito (johnf-inodes) wrote :

> @johnf: It seems that you're still not part of https://edge.launchpad.net
> /~contributor-agreement-canonical/+members#active
>
> Is there a related problem ? Can we help ?

I emailed this a onth or so ago and CC's @poolie. Let me know if I need to resend it somewhere.

Revision history for this message
Martin Pool (mbp) wrote :

2009/12/18 John Ferlito <email address hidden>:
>> @johnf: It seems that you're still not part of https://edge.launchpad.net
>> /~contributor-agreement-canonical/+members#active
>>
>> Is there a related problem ? Can we help ?
>
> I emailed this a onth or so ago and CC's @poolie. Let me know if I need to resend it somewhere.

No, I have it, I think you probably just sent it just around the time
that we were agreeing to do this through the Launchpad team. I have
now added you.

--
Martin <http://launchpad.net/~mbp/>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added directory 'contrib/debian'
2=== added file 'contrib/debian/default'
3--- contrib/debian/default 1970-01-01 00:00:00 +0000
4+++ contrib/debian/default 2009-10-06 18:00:29 +0000
5@@ -0,0 +1,20 @@
6+# Specify startup options for bzr --serve
7+
8+# WARNING
9+# When bazaar runs in serve mode there is no authentication. You will
10+# be publishing your tree as readable or potentially writeable to everyone
11+
12+# You need to uncomment the line below to enable this service
13+#ENABLED=1
14+
15+# Specify a user and group to run as
16+#USER=bzr
17+#GROUP=bzr
18+
19+# Specify the path to the repository
20+#REPO=/srv/bzr
21+
22+# Other arguments
23+# Add --allow-writes for RW access
24+# Example config listens on localhost
25+DAEMON_ARGS="--port=127.0.0.1:4155"
26
27=== added file 'contrib/debian/init.d'
28--- contrib/debian/init.d 1970-01-01 00:00:00 +0000
29+++ contrib/debian/init.d 2009-10-06 18:00:29 +0000
30@@ -0,0 +1,135 @@
31+#! /bin/sh
32+### BEGIN INIT INFO
33+# Provides: bzr
34+# Required-Start: $local_fs $remote_fs
35+# Required-Stop: $local_fs $remote_fs
36+# Default-Start: 2 3 4 5
37+# Default-Stop: S 0 1 6
38+# Short-Description: bazaar Smart Server
39+# Description: bazaar Smart Server
40+### END INIT INFO
41+
42+# Author: John Ferlito <johnf@inodes.org>
43+
44+# PATH should only include /usr/* if it runs after the mountnfs.sh script
45+PATH=/usr/sbin:/usr/bin:/sbin:/bin
46+DESC="Bazaar Smart Server"
47+NAME=bzr
48+DAEMON=/usr/bin/$NAME
49+DAEMON_ARGS=""
50+PIDFILE=/var/run/$NAME.pid
51+SCRIPTNAME=/etc/init.d/$NAME
52+
53+USER=bzr
54+GROUP=bzr
55+REPO=/srv/bzr
56+ENABLED=0
57+
58+# Exit if the package is not installed
59+[ -x "$DAEMON" ] || exit 0
60+
61+# Read configuration variable file if it is present
62+[ -r /etc/default/$NAME ] && . /etc/default/$NAME
63+
64+# Load the VERBOSE setting and other rcS variables
65+[ -f /etc/default/rcS ] && . /etc/default/rcS
66+
67+test "$ENABLED" != "0" || exit 0
68+
69+# Define LSB log_* functions.
70+# Depend on lsb-base (>= 3.0-6) to ensure that this file is present.
71+. /lib/lsb/init-functions
72+
73+#
74+# Function that starts the daemon/service
75+#
76+do_start()
77+{
78+ # Return
79+ # 0 if daemon has been started
80+ # 1 if daemon was already running
81+ # 2 if daemon could not be started
82+ start-stop-daemon --start --quiet --make-pidfile --pidfile $PIDFILE \
83+ --chuid $USER:$GROUP --background --exec $DAEMON --test > /dev/null \
84+ || return 1
85+ start-stop-daemon --start --quiet --make-pidfile --pidfile $PIDFILE \
86+ --chuid $USER:$GROUP --background --exec $DAEMON -- \
87+ serve $DAEMON_ARGS --directory $REPO \
88+ || return 2
89+ # Add code here, if necessary, that waits for the process to be ready
90+ # to handle requests from services started subsequently which depend
91+ # on this one. As a last resort, sleep for some time.
92+}
93+
94+#
95+# Function that stops the daemon/service
96+#
97+do_stop()
98+{
99+ # Return
100+ # 0 if daemon has been stopped
101+ # 1 if daemon was already stopped
102+ # 2 if daemon could not be stopped
103+ # other if a failure occurred
104+ start-stop-daemon --stop --quiet --retry=TERM/30/KILL/5 --pidfile $PIDFILE --name $NAME
105+ RETVAL="$?"
106+ [ "$RETVAL" = 2 ] && return 2
107+ # Wait for children to finish too if this is a daemon that forks
108+ # and if the daemon is only ever run from this initscript.
109+ # If the above conditions are not satisfied then add some other code
110+ # that waits for the process to drop all resources that could be
111+ # needed by services started subsequently. A last resort is to
112+ # sleep for some time.
113+ start-stop-daemon --stop --quiet --oknodo --retry=0/30/KILL/5 --exec $DAEMON
114+ [ "$?" = 2 ] && return 2
115+ # Many daemons don't delete their pidfiles when they exit.
116+ rm -f $PIDFILE
117+ return "$RETVAL"
118+}
119+
120+case "$1" in
121+ start)
122+ [ "$VERBOSE" != no ] && log_daemon_msg "Starting $DESC" "$NAME"
123+ do_start
124+ case "$?" in
125+ 0|1) [ "$VERBOSE" != no ] && log_end_msg 0 ;;
126+ 2) [ "$VERBOSE" != no ] && log_end_msg 1 ;;
127+ esac
128+ ;;
129+ stop)
130+ [ "$VERBOSE" != no ] && log_daemon_msg "Stopping $DESC" "$NAME"
131+ do_stop
132+ case "$?" in
133+ 0|1) [ "$VERBOSE" != no ] && log_end_msg 0 ;;
134+ 2) [ "$VERBOSE" != no ] && log_end_msg 1 ;;
135+ esac
136+ ;;
137+ restart|force-reload)
138+ #
139+ # If the "reload" option is implemented then remove the
140+ # 'force-reload' alias
141+ #
142+ log_daemon_msg "Restarting $DESC" "$NAME"
143+ do_stop
144+ case "$?" in
145+ 0|1)
146+ do_start
147+ case "$?" in
148+ 0) log_end_msg 0 ;;
149+ 1) log_end_msg 1 ;; # Old process is still running
150+ *) log_end_msg 1 ;; # Failed to start
151+ esac
152+ ;;
153+ *)
154+ # Failed to stop
155+ log_end_msg 1
156+ ;;
157+ esac
158+ ;;
159+ *)
160+ echo "Usage: $SCRIPTNAME {start|stop|restart|force-reload}" >&2
161+ exit 3
162+ ;;
163+esac
164+
165+: