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