On 1 Sep 2010, at 00:41, Ralf Wildenhues wrote: > this is a review, not an approval.
No problem; thanks for the review. [[Review comments I agree with elided...]] > * Gary V. Vaughan wrote on Tue, Aug 31, 2010 at 08:43:15AM CEST: >> * libltdl/config/announce-gen.m4sh: Add support for gnulib >> announce-gen options, previously missing from our m4sh >> implementation, and enforce specifying --gnulib-version when >> `gnulib' is listed in --bootstrap-tools. > > I had avoided looking much at announce-gen so far; I've taken another > look now. I found a few issues: > > Using --output to direct output to stdout seems unusual and inconsistent > with both the GNU Coding Standards recommendations (nodes 'Command-Line > Interfaces' and 'Option Table') and other tools like git. Good point. The intention is to have --output vs --post. I'll look at this again after the release. > I personally find the M4SH_GETOPTS rather unreadable; it's a nice table, > but I cannot make sense of the entries, or review them, except by > looking at the generated code, at which point I'd rather just read the > generated code directly; after all, that's how we found several bugs in > the ltmain incarnation of this code. The documentation is in getopts.m4sh, just above the M4SH_GETOPTS definition. Surely you're not suggesting that we continue to hand code, maintain, synchronize the option parsing loop in each of our scripts? > The --rcfile handling code mistreats quoting in the rcfile, and things > like multiple adjacent whitespace. Examples please. I haven't had any issues using this idiom in any of the --rcfile parsing in any of the shell scripts I've used this code with since I wrote it for cvs-utils (if memory serves). > The help text claims that ./.announcerc is the default place for the > rcfile, but the code looks like $top_srcdir/.announcerc is being used. > > The announce-gen script claims: > # This is the .announcerc for GNU Libtool. Announce-gen is pretty > # smart about picking defaults for package-name and current-version > # and often guesses previous-version correctly too. > > and yet the expanded script contains code like: > top_srcdir=`cd "../libtool" && pwd` > test -d "$top_srcdir" || top_srcdir='../libtool' > > that, when used for another package, will luckily cause stderr noise > that will alert me that just maybe things won't go as smooth as > advertised. Or did you really mean for the script to be generated > anew as part of each package? Well, I mostly intended for it to generate Libtool release announcements for me when I roll up a release. But also to be somewhat similar to the gnulib version, with an eye towards contributing it later. I originally tried to fix the gnulib version to work with libtool's release process, but frankly I'd rather stick forks in my eyes than develop in Perl. Cheers, -- Gary V. Vaughan (g...@gnu.org)
PGP.sig
Description: This is a digitally signed message part