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.
> Hi *!
>
> The easier part of this problem is addressed by the attached patch:
> wwwoffle.config tries to preseed the debconf db with yes/no for boolean values
> which expect true/false.
Yes, that looks reasonable.
> 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?
> 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:
I suggest something along these lines:
@@ -254,8 +253,18 @@
if grep -qsw 'cache-control-max-age-0' $CONFIG; then
config_version=2.8e
fi
- 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
+ tempfile=`mktemp`
+ perl -pe 's/^(\# WWWOFFLE - World Wide Web Offline Explorer -
Version).*/$1'" $config_version/" $CONFIG > $tempfile
+ # if the files are unchanged, remove tempfile;
+ if cmp -s $CONFIG $tempfile; then
+ rm $tempfile
+ #otherwise, cat it into $CONFIG. (We don't move to preserve a possible
symlink)
+ else
+ # is there already a backup file from last time?
+ if [ -f $CONFIG.postinst-bak ]; then rm $CONFIG.postinst-bak; fi
+ mv $CONFIG $CONFIG.postinst-bak
+ cat $tempfile > $CONFIG
+ fi
fi
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
}
All the backup stuff is done before, and removed if unchanged afterwards.
> 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.
> 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.
> 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.
> Line 383:
> | if grep '^PPP' $OPTIONS >/dev/null 2>&1 ; then
>
> 'PPP' is lowercased everywhere else.
will be lowercased.
> Line 402:
> | if [ "$FREQ" != off ]; then
> | # convert to digits only
> | FREQ=$( expr "$FREQ" : '\([0-9]*\)' )
>
> In the .config, there is already complex code to clean this value and
> (without obvious cause) to cap it to 30 minutes. Also this would silently
> eat input errors.
Yes, this can be safely removed.
> 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.
> 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.
> Line 516, ff:
> | rm -f nohup.out
> | nohup chown -R proxy:proxy /var/cache/wwwoffle/. /var/lib/wwwoffle/.
> >/dev/null 2>&1 &
> | sleep 1
> | rm -f nohup.out
>
> This deletes nohup.out in the current directory, which could be not from
> wwwoffle, which could also be created in $HOME, but is not created by nohup
> anyway because of the redirection.
I'll just remove the nohup.out removals.
So, this is already long enough. I'm sending it now, and start a new
mail for things I discovered newly.
Regards, Frank
--
Frank Küster
Inst. f. Biochemie der Univ. Zürich
Debian Developer
