Hello Gary,
this is a review, not an approval.
* 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.
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 help text has typos: extra "any" in "to any every", missing "the" in
"often guesses previous-version", s/for/with/ in "but for `-p'", and
missing final period.
The --rcfile handling code mistreats quoting in the rcfile, and things
like multiple adjacent whitespace.
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?
In another package, I might not have any bootstrap tools; in that case,
announce-gen will refuse to work.
Passing '--signature -foo' seems to not complain about a missing
argument to --signature, but also not treat -foo as signature file.
Hmm, the help text could be interpreted as to indicate that plain
'--signature' is needed in order to enable signature appending at all,
but the code sees no state change when --signature -foo is passed, and
the option parsing handling will bail out if --signature is the last
command line argument, contradicting the help text of allowing an
optional argument. Actually, in the code below the command-line
handling, a signature does not seem to ever be added in any case.
> --- a/libltdl/config/announce-gen.m4sh
> +++ b/libltdl/config/announce-gen.m4sh
> @@ -32,9 +32,12 @@ AS_INIT[]m4_divert_push([HEADER-COPYRIGHT])dnl
> # autoconf,automake,bison,gnulib
> # --current-version=VER override current release version number
> # --debug enable verbose shell tracing
> +# --gnulib-version=VER set the version string if gnulib is named
> +# in the --bootstrap-tools list
> # --gpg-key-id=ID gnupg ID of signature key for release
> files
> # -h STRING --header=STRING insert mail header into announcement
> # -m STRING --message=STRING insert into message blurb for
> announcement
> +# --news=NEWS_FILE path to the NEWS file [../NEWS]
The default path for the NEWS file is $top_srcdir/NEWS rather than
../NEWS as stated in the help text. Even more, the --news switch is
ineffective in that the actual code will parse $top_srcdir/NEWS even
if I specify --news=/oops.
> # -c --no-print-checksums do not emit MD5 and SHA1 checksums
> # -o --output output to stdout
> # --package-name=NAME override default package name
> @@ -128,13 +131,18 @@ M4SH_GETOPTS(
> mailnotify_flags="${mailnotify_flags+$mailnotify_flags }$opt"],
> [!], [--bootstrap-tools|--bootstrap|--boots], [],
> [
> for tool in `echo "$optarg"|$SED 's|,| |g'`; do
> - ($tool --version) >/dev/null 2>&1 || {
> - func_error "$opt \`$optarg' does not respond to --version
> option."
> - cmd_exit=exit
> - }
> - done],
> + test gnulib = "$tool" ||
> + ($tool --version) >/dev/null 2>&1 || {
> + func_error "$opt \`$optarg' does not respond to --version
> option."
> + cmd_exit=exit
> + }
> + done
> + # squash spaces so that delimiter is just `,' and nothing else:
> + opt_bootstrap_tools=`echo "$optarg" |$SED 's|, *|,|g'`],
> [!], [--current-version|--current|--curr], [...@version@],
> [],
> + [!], [--gnulib-version|--gnulib], [],
> [],
> [!], [--gpg-key-id|--gpg], [],
> [],
> + [!], [--news], [],
> [],
> [], [--no-print-checksums], [],
> [],
> [!], [--previous-version|--previous|--prev], [...@lastrelease@],
> [],
> [!], [--release-type], [],
> [],
> @@ -152,6 +160,14 @@ M4SH_GETOPTS(
> func_error "you must specify some tools with --bootstrap-tools."
> exit_cmd=exit
> }
> + case ,gnulib, in
> + ,$opt_bootstrap_tools,)
> + test -n "$opt_gnulib_version" || {
> + func_error "you must specify a --gnulib-version when gnulib is
> in --bootstrap-tools."
> + exit_cmd=exit
> + }
> + ;;
> + esac
The $opt_gnulib_version seems to not be used for anything in the script,
neither in this patch nor any of the other patches in this series. So I
concur with Eric that this is not needed now and can wait until it is.
> # validate $opt_blurb and $opt_message
> test -n "$opt_blurb" && test -n "$opt_message" && {
Cheers,
Ralf
