Re: [PATCH 1/6] Add --gnulib-version and --news options to announce-gen.

[Gary V. Vaughan]

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 (address@hidden)

PGP.sig
Description: This is a digitally signed message part