On Tuesday 05 April 2005 18:34, Frank K�ster wrote:
> David Schmitt <[EMAIL PROTECTED]> wrote:
>
> I am looking into this, trying to provide a patch - and NMU if Paul
> doesn't show up, I really think wwwoffle should go back into sarge.
Indeed. Great. Thanks!
> > The "overwwrites local config" part is way harder. Looking at the
> > postinst gives me the creeps. Just a few quotes which I think would need
> > fixing:
> >
> > Everything in debian/wwwoffle.postinst of 2.8e-1:
> >
> > Line 202:
> > | # Here the [...] link is made if it either doesn't exist or it points
> > | to a # non-existent file.
>
> for i in gif png js; do
> - if [ ! -f /etc/wwwoffle/debian-replacement.$i ]; then
> - rm -f /etc/wwwoffle/debian-replacement.$i
> + if [ ! -e /etc/wwwoffle/debian-replacement.$i ]; then
> ln -s /usr/share/wwwoffle/html/en/local/dontget/standard.replacement.$i
> /etc/wwwoffle/debian-replacement.$i fi
>
> This will create a symlink when it is missing, but not remove it if it
> point to a nonexistent target - the target might be only temporarily
> missing. I would not take "preserve user modifications" too literally
> here; in fact I doubt that these files are really configuration files -
> who would want to configure a 1x1 pixel png file?
Well, one could want to remove it altogether or add a local substitute to
alert the user to the missing picture. Using -e is already better, but the
code should only run on first install.
Medium term it might be better to make the path-to-replacement configurable
instead of putting it as a symlink somewhere.
> > The following code is AFAICS not conditional upon first installation,
> > thus overriding a local admin who has intentionally removed the link.
> >
> > Line 257:
> > | perl -i.bak -pe 's/^(\# WWWOFFLE - World Wide Web Offline Explorer -
> > | Version).*/$1'" $config_version/" $CONFIG if cmp -s $CONFIG
> > | $CONFIG.bak; then mv -f $CONFIG.bak $CONFIG; fi
> >
> > If the perl doesn't modify the config, this will kill the symlink:
> However, since there are many more places where perl's in-place-editing
> is used, and not only on $CONFIG, I have written a shell function for
> that:
>
> safe_edit_file(){
> # this function allows perl or sed commands to change a file, even if it
> # is a symlink
> filename="$1"
> shift
> replacecommand="$@"
> tempfile=`mktemp`
> $replacecommand $filename > $tempfile
> if cmp -s $filename $tempfile; then
> # if the files are unchanged, remove tempfile;
> rm $tempfile
> else
> #otherwise, cat it into $filename. (We don't move to preserve a
> #possible symlink)
> cat $tempfile > $filename
> fi
> }
Looks good. $replacecommand has to be changed from inplace-editing to
stdout-putting, but you have surely thought of that.
> All the backup stuff is done before, and removed if unchanged afterwards.
Now that you mention it, the "if cmp" is missing a rm $tempfile and can be
rewritten as
if !cmp ... ; then
cat $tempfile > $filename
fi
rm $tempfile
> > Line 354:
> > | if [ ! -f $OPTIONS ]; then
> > | cp -p /usr/share/wwwoffle/default/wwwoffle.options $OPTIONS
> > | fi
> >
> > This too is not special cased to the not-upgrading case.
>
> I'm currently having a blackout - how can one test for the special case
> of installing after removal, but not purge? In this case, there's no
> "last configured version" to read from $2.
Thinking about it more, one could argue that "remove"ing a package and
manually deleting a config file does constitute "purge"ing the package.
To you question, see:
http://www.debian.org/doc/debian-policy/ch-maintainerscripts.html#s-unpackphase
point 3.2 answers it, I believe.
> > The following two problems apply to all debconf -> $OPTIONS
> > transformations
> >
> > Line 365:
> > | if grep '^htdig' $OPTIONS >/dev/null 2>&1 ; then
> >
> > This is probably correct, but it highlights that other places testing
> > for options are too strict when using grep -x 'htdig'.
>
> I'll look into that later. Since those greps are not only used in
> maintainer scripts, but also e.g. in /etc/init.d/wwwoffle, it is at
> least consistent. Don't know whether it is documented; I'd say it isn't
> RC.
I believe to have found them not to be consistent:
$ grep -h grep * | sed -e 's/^[ \t]*//g' | sort -u
shows at least usages of -x (whole line) vs. -w (whole word) which should be
investigated for semantic correctness. Especially this worries me:
wwwoffle.postinst:134: if grep -qsx ppp $OPTIONS; then
wwwoffle.postinst:383: if grep '^PPP' $OPTIONS >/dev/null 2>&1 ; then
> > Line 368 and 372:
> > | perl -i -e 'while (<>) { next if /htdig/; print; }' $OPTIONS
> >
> > This skips _all_ lines containing 'htdig' including comments
>
> So it should be /^\s*htdig/, right? I'd prefer to comment it out, this
> would be
>
> perl -i -e 's/^(\s*htdig)/# $1/'
>
> modulo the changes for -i. The current code does not preserve comments
> after the htdig option.
Indeed. in the face of the grep problems it'd should perhaps be required,
documented and checked (use -x or '^opt' everywhere) that all options start
on the first column.
> > Line 397, 412, 423:
> > |CRONTAB=/etc/cron.d/wwwoffle
> >
> > [..]
> >
> > | if [ -s $CRONTAB ]; then
> >
> > This too is not special cased to the not-upgrading case and fails if
> > $CRONTAB is a symlink.
> >
> > The postinst goes on to munge or recreate the crontab. Again without
> > checking for symlinks or admin-removal of the file.
>
> What do you mean with "fails if it is a symlink"?
>
> $ ln -s foo bar
> [EMAIL PROTECTED]:~/area$ ll
> total 12
> lrwxrwxrwx 1 frank frank 3 2005-04-05 17:37 bar -> foo
> [EMAIL PROTECTED]:~/area$ test -s bar; echo $?
> 0
>
> So it should not matter whether the file is a real file with nonzero
> size, or a symlink to such a file.
>
> And in this case, I think, it is *not* a problem if the file is created,
> because it is created with only comments.
Indeed.
> > Line 448:
> > | # now duplicate directory structure of /usr/share/wwwoffle/html to
> > | /etc/wwwoffle/html
> >
> > Followed by approximatly 20 lines of shell code which probably amount to
> > a 'cp -s' which I _think_ could be replaced by a 'ln -s
> > /usr/share/wwwoffle/html /etc/wwwoffle/html' iff the package is not
> > upgrading and there is no /etc/wwwoffle/html.
>
> I am not sure that I understand the purpose of the code, but it seems to
> me that
>
> - the directories in /etc/wwwoffle/html should be shipped in the deb
>
> - all those symlinks are in fact not configuration "files". Of course
> it alters the behaviour of the package if RefreshFormError.html points
> to something else than the file shipped in the deb, but this is not a
> usual configuration task - it's rather something that one would
> normally achieve by patching the package.
>
> I do not think it is something for an NMU to clean this up, which would
> amount to a very big change. And I think if the maintainer script do
> not respect local changes for something that shouldn't be a
> configuration file at all, it is not release-critical.
Your call. As the "replacement" thingy above, this should probably be replaced
by a configurable path-to-shipped-files. Perhaps file a separate bug about
this stuff that it's not forgotten?
> So, this is already long enough. I'm sending it now, and start a new
> mail for things I discovered newly.
Thank you for your time and work.
Regards, David
--
- hallo... wie gehts heute?
- *hust* gut *rotz* *keuch*
- gott sei dank kommunizieren wir �ber ein septisches medium ;)
-- Matthias Leeb, Uni f. angewandte Kunst, 2005-02-15
