Arthur de Jong wrote:
> Ok, could you review my cvsd package for me for correct debconf usage and
> tell me what you do and don't like?
Thanks for taking advantage of that offer. (So far you're the only one.)
I am ccing this to -devel just because.
All of the debconf questions are pretty well worded and clear, but
there's always room for improvement:
- I'm not sure why you capitalized "Chroot" in the second sentence of the
cvsd/rootjail question (and in the short description of that question)
and "filehierarchy" probably needs a space in it.
- cvsd/rootjail:
s/zero (0)/0/ # Apparently writing it out has the possibility to make
# someone enter the number the wrong way so why not just
# not write it out?
- cvsd/nice:
s/too much/too many/
- cvsd/listen:
s/cvsd will listen on/on which cvsd will listen/
# Avoid dangling preposition
- Though it's pretty obvious what you mean by "RootJail" in the
cvsd/repositories question, that is in fact the first time that
particular term has been used. Also, you could use a debconf
substitution here to substitute in the user's choce of the chroot jail
directory directly into the question, to remind them what it is. Oh you
also use the inconsistently capitalized "rootjail" in cvsd/usedebconf.
Wouldn't just "chroot jail" be better? Or at least some consistency
there and with the "Chroot jail" term in cvsd/rootjail.
- cvsd/limits:
s/to limit of pserver process./of pserver process to limit:/
- Most of the questions have short descriptions that do not end in
punctuation, and this has unfortunate results in some debconf frontends,
like this:
Location of Chroot jail /var/lib/cvsd
It's better to put a colon or a question mark at the end.
- It's sort of odd that you use spaces to separate multiple items in one
question, and then colons to separate them in the next.
- It was not clear to me in the cvsd/limits question what would be the
result of picking any of those limits. At first I thought it would
enable some kind of hard-coded limits, which made me not want to mess
with it. I see reading that templates file that it in fact enables a
whole set of additional questions about the limits. Noting that you'll
ask these questions in cvsd/limits would be a good idea.
- cvsd/limit_coredumpsize:
s/filesize/file size/ # don't invent terminology
s/coredump/core dump/ # I know, it sucks when one's spacebarbreaks :-)
- cvsd/limit_cputime:
s/prevent too much cpu time allocated/prevent too much cpu time from being allocated/
s/amount of seconds cputime/amount of cpu time/
# it is not just in seconds form, and "seconds cputime" is
# incorrect English.
- I was fairly confused by cvsd/limit_memorylocked, because I thought that
only programs run as root could lock memory, and why would cvs want to?
Was this just added for (over)completeness?
- cvsd/limit_maxproc:
s/Cvs/cvs/ # for consistency with every other mention of the program
# including other starts of sentences
- You have no translations for the debconf templates in the package. The
DDTP project has a complete German translation at
http://ddtp.debian.org/debconf/source/cvsd. Of course you may want to
make the above changes first, and wait for an updated translation..
If I reconfigure cvsd, pick all the limits, and take the default value
for everything else, it exits with code 20:
debconf (developer): <-- GET cvsd/limits
debconf (developer): --> 0 coredumpsize, cputime, datasize, filesize,
memorylocked, openfiles, maxproc, memoryuse, stacksize, virtmem
debconf (developer): <-- GET cvsd/limit_coredumpsize cputime datasize filesize memorylocked openfiles maxproc memoryuse stacksize virtmem
debconf (developer): --> 20 Incorrect number of arguments
zsh: exit 20 DEBCONF_DEBUG=. dpkg-reconfigure cvsd
Looks like a bug.. After doing this I also did not see all the limits
stuff in cvsd.conf, it had only these config items:
Uid cvsd
Gid cvsd
PidFile /var/run/cvsd.pid
RootJail /var/lib/cvsd
MaxConnections 10
Nice 1
Listen * 2401
You support backing up; very good job there!
Looking at your priorities, I'm not sure why the maximum connection
limit is at priority medium. Surely there is a sane default, and so it
could be low priority? I'm iffy about asking about the jail directory at
medium priority; I suppose many people will want to configure that, but
/var/lib/cvsd seems reasonable. Medium is probably ok. I agree with your
use of high priority for the repositories question, and all the rest
look nice and low.
> I've set it up to ask first whether to use debconf to manage configuration
> of /etc/cvsd/cvsd.conf (not a conffile). If the user chooses to not use
> debconf, he's on his own (an example cvsd.conf is provided).
Of course this is where in my opinion you lose. First of all, as Colin
pointed out recently, any question that asks "use debconf" and defaults
to true results in the package violating debian policy on any system
where the debconf priority is higher than the question, or
noninteractive mode is used. If the question is not shown to the user at
all, and a default config is put into place, and the user edits that
config and loses changes on upgrade, then you've violated policy.
Secondly of course, I hate all "use debconf" questions. It's the wrong
way to use debconf; you should be intelligently parsing answers out of
the file on reconfigure and upgrade, and intelligently editing them back
into the file, so you do not lose an admin's modifications or comments.
No config file in debian should have warnings at the top about not
editing it. I wince every time I see another "do not edit this file"; I
consider every one at least one user lost to gentoo or some other
distribution.
> Some user modifications in the config file are retained and read into
> debconf (but not yet all).
cvsd.conf is a trivial config file to parse and modify from what I can
see.
port=`sed -n 's/^Port *\([^ ]*\).*$/\1/p' < /etc/cvsd/cvsd.conf`
That's a reasonable way to get any value from it. I'm glad you do this
for a few questions, I just wish you would finish it up and do it for
all of them, and get rid of the "manage with debconf" and "do not edit"
crap. Don't waste your time on that stuff, just do it right before the
package enters the archivre in the first place!
The way the postinst creates the config file is all wrong, it loses all
admin modifications and comments. It should, if the file does not exist
on initial install, put in a template. Then it should just edit the
template with sed or awk, or perl or whatever works to edit in changes
on upgrade or reconfigure. See xringd for an exellent and very simple
example of a package that gets this right.
It's a pity that the cvsd.conf file that gets written does not have all
the nice comments explaining everything that are in the cvsd.conf-dist
in the doc directory. If you convert to the approach xringd uses, you
would move cvsd.conf-dist to /usr/share/cvsd/, and link it to
/usr/share/doc. Then the postinst would copy it to /etc on new installs.
The config script would pull setting out of it with sed as you do now,
and the postinst would edit settings back into it using sed or
something. All the nice comments and any additional ones the admin put
in would be preserved.
Hmm, you might have to do something mildly tricky with the limits stuff;
if the user did not turn it on you would have to manage
commenting/uncommenting the lines in the config file. Still seems quite
doable.
I think you should be able to bring this package up to par as a very
good example of using debconf with perhaps 5 hours of work, but you're
not there yet.
--
see shy jo
Attachment:
pgp2n0M6JBuC3.pgp
Description: PGP signature