Bug#447611: update-initramfs triggerisation
maximilian attems writes ("Re: Bug#447611: update-initramfs
triggerisation"):
> [stuff]
Thanks for looking at my patch.
> > + dpkg --compare-versions "$DPKG_RUNNING_VERSION" ge '1.14.5ubuntu10~~'
> > +then
>
> ubuntu specific, would with high probab trigger on the wrong versions,
> also why is this needed?
> has there been a buggy dpkg trigger support!?
There was indeed a buggy dpkg trigger support version in some versions
of Ubuntu gutsy before it was released. Early in the release cycle of
a particular Ubuntu version many features are thrown into the Ubuntu
archive in a fairly ill-tested state - much more so than you would
typically do in Debian unstable.
So it shouldn't be seen as alarming or unusual that some bugs were in
the Ubuntu archive version at that stage, any more than it would be
alarming or unusual for a Debian developer to locally try out buggy
code, or share it with co-developers eg via experimental, while doing
pre-sid testing.
To be honest I don't have complete records of which versions were
buggy in the relevant way. That test is harmless in Debian because
none of the buggy versions were ever in Debian, and all
trigger-supporting versions in Debian will have a higher version
number. (We're not going to be backporting this large change, I
assume.)
At the time when I wrote the patch, that test served to protect Ubuntu
testers and developers who had the buggy dpkg from the consequences of
the bug. Using that particular syntax (with the ~~) also meant that I
was able to test the effect using some private versions I had built
locally without editing the version string just before upload (which
is obviously error-prone).
I obviously thought it would be better to send you the patch we were
actually using rather than some `special' version which we hadn't
tested. You are of course free to remove tests of this kind, but in
the general that just makes a bit more work for the Ubuntu developer
who next has to merge initramfs-tools from Debian.
In this particular case since the buggy dpkg existed only in early
versions of Ubuntu gutsy, I think this test has served its purpose and
can be discarded from Ubuntu as well as Debian.
> need to get a deeper look in git-dch as debian changelog only generates
> conflicts, better snipped for now also as ubuntu specific:
> patching file debian/changelog
> Hunk #1 FAILED at 1.
> 1 out of 1 hunk FAILED -- saving rejects to file debian/changelog.rej
Changes to debian/changelog always tend to get rejected if you apply a
patch to a different version because the context is the previous
version which contains the previous version number.
> > +if [ "x$1" != xtriggered ] && \
>
> if [ "$1" != "triggered" ] && \
>
> seems more readable, don't know why the oldstyle x$string = string
> remains in usage?
test(1) has really strange argument parsing, which is sometimes
ambiguous. Prefixing unknown input strings with x avoids accidentally
treating those strings as operators or syntactic elements.
For example, consider:
test '(' '=' ')'
Is this
test '(' EXPR ')' where EXPR is '=', equivalent to -n '='
or
test A '=' B where A is '(' and B is ')'
?
It may be possible in particular cases to show that the problem cannot
arise. SuSv3 has a crazy set of explicit rules which only go as far
as up to 4 arguments (and so answers my question above). So both to
avoid having to do a detailed analysis of test(1)'s argument parsing
and of the possible input strings, and as a matter of good programming
habit, it is better to write code the way it will always work (even if
a subsequent programmer makes it more complex).
In your version the "..." around `triggered' are superfluous. When I
see that in a shell script I start to wonder whether the author knows
the proper sh quoting rules. I trust that you do :-) but I would
suggest avoiding idioms which tend to undermine the reader's
confidence in the author.
> > update-initramfs -u
> > + # ... this activates the trigger, if triggers are working
>
> please comment before command.
Sorry, I evidently didn't look down far enough to see your existing
coding style, which I should of course have copied.
> > +interest update-initramfs
> "interest" ???
`interest' is the directive name used in the triggers file to specify
that this package needs to know about activations of the named
trigger.
I chose the explicit trigger name `update-initramfs' since it seemed
like an obvious choice. The guideance in the triggers spec (which I
wrote, of course) says that it's often good to choose a command or
package name and the command name seems like a good choice here.
Do you have a different view ?
> > +binary-install/initramfs-tools::
> > + install -m 644 -o 0 -g 0 debian/initramfs-tools.triggers \
> > + debian/initramfs-tools/DEBIAN/triggers
>
> no i-t uses cdbs,
> please add to debian/initramfs-tools.install
> but maybe i misread and there is no cdbs support for that yet??
Stuffing files into DEBIAN/ with .install seemed a bit wrong to me,
and I thought this approach with a hook was better.
In the future I imagine debhelper will copy <package>.triggers into
the right place itself.
> > +if $USETRIGGERS \
> > + && test x"$DPKG_MAINTSCRIPT_PACKAGE" != x \
> > + && test $# = 1 \
> > + && test x"$1" = x-u \
> > + && dpkg-trigger --check-supported 2>/dev/null
>
> why not using built-ins?
Sorry ? What here is not a built-in ? dpkg-trigger obviously isn't
but we need the logic implemented in that command.
> also this seems to be at the wrong place,
> if you want it to only affect the update-initramfs -u code path
> than make it a function and call it a line before update instead
> of an opencoded getopt.
I agree that this isn't the best style. One reason why I did it this
way because I wanted to be sure that it did only and exactly what I
intended it to. Another way of looking at it is that I regarded
myself as wrapping update-initramfs. Obviously since update-initramfs
is a script this could be done in code at the top of the script.
The result is at least correct in the sense that it will never merely
activate the trigger when it ought to do something more.
> also doesn't apply:
> patching file update-initramfs
> Hunk #1 FAILED at 4.
> 1 out of 1 hunk FAILED -- saving rejects to file update-initramfs.rej
Oh. I'll look into that.
> you may want to checkout latest git
> git clone git://git.debian.org/git/kernel/initramfs-tools.git
Would you like me to prepare a new patch based on your comments ?
> don't get me wrong, i highly like the idea of that work
> and i'm happy to use it soon.
Great.
Thanks,
Ian.
Reply to: