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. -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]