Re: debconf review of cvsd (was Re: stop abusing debconf already)
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
> 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:
Thanks for the review. I've changed most of the wording according to your
suggestions (English isn't my native language).
> 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?
I spelled out zero because some (most) fonts don't represent 0 differently
from O. Same goes for 1 and l.
> - 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 would be nice if debconf would have some unified way of entering lists.
I'm also not very happy with the way it is currently done, but I've
modeled it after the cvs questions and a colon is the "standard" seporator
for directories, while using : in addresses may provide problems with IPv6
addresses (improving this is on my TODO list).
> - 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?
Probably. I provided questions for all the limits I could find. The code
also covers limits for things that are not settable on linux (there is
even a debconf question about it that is never asked).
> - 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..
I have received a Brazillian translation of the debconf questions that I'm
merging into cvsd (bug #187795). I saw the German translation at
http://ddtp.debian.org/cgi-bin/ddtp.cgi?part=debconf&package=cvsd
before but I never saw the page you linked (very useful page but I can't
find it linked from the site). And since the site says "(Now) we don't
need any action from the debian package maintainer." I didn't take any
action regarding the 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
This looks like it may be due to a bug (or incompatibility) in zsh. Do you
have /bin/sh set to zsh? I have some strange results if I use zsh to
process the postinst. I'll do some more testing. Somehow the result of the
'GET cvsd/limits' is passed to the next command.
> Looking at your priorities, I'm not sure why the maximum connection
> limit is at priority medium.
I already changed it to low for the next release (couldn't remember
either why it was set to medium).
> > 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.
I also don't like the question and would rather see it removed. I was
working to that goal but ran into some small problems that made editing
the existing configuration a little too complex.
> > 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!
Reading the current configuration is almost finished and will go into the
next release.
> 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.
I'll look into editing the existing configuration file rather than
overwriting it. I ran into some problems earlier though (especially things
like where do you put new configuration options that were not there
before). Also Limit, Repos and Listen may be specified multiple times so
that may be tricky.
- -- arthur - arthur@tiefighter.et.tudelft.nl - http://tiefighter.et.tudelft.nl/~arthur --
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.6 (GNU/Linux)
iD8DBQE+onCDVYan35+NCKcRAgnGAJ4j9u/Pz1P+H2Uzbr3tFsWWdd3LfgCfcfWD
e/IY8J+0TZaYmmCoqEuTVCw=
=jfJ7
-----END PGP SIGNATURE-----
Reply to: