Re: [PATCH 2/2] warnings: new 'extra-portability' category, for AM_PROG_AR

[Stefano Lattarini]

Hi Peter.  See my nits and comments inlined ...

> diff --git a/doc/automake.texi b/doc/automake.texi
> index c789e74..eea11d1 100644
> --- a/doc/automake.texi
> +++ b/doc/automake.texi
> @@ -2680,6 +2680,9 @@ user redefinitions of Automake rules or variables
>  @item portability
>  portability issues (e.g., use of @command{make} features that are
>  known to be not portable)
> address@hidden extra-portability
> +extra portability issues related to obscure tools.  One example of such
> +a tool is the Microsoft lib archiver.
>
s/lib/@command{lib}/.

> diff --git a/tests/extra-portability.test b/tests/extra-portability.test
> new file mode 100755
> index 0000000..49b17e0
> --- /dev/null
> +++ b/tests/extra-portability.test
> @@ -0,0 +1,67 @@
> +#! /bin/sh
> +# Copyright (C) 2011  Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2, or (at your option)
> +# any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Make sure enable of extra-portability enables portability and
> +# that disable of portability disables extra-portability
>
Micro-nit: this would be clearer IMHO:

  # Interactions between `portability' and `extra-portability'
  # warning categories:
  #   1. `-Wextra-portability' must imply `-Wportability'.
  #   2. `-Wno-portability' must imply `-Wno-portability'.

> +
> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat >>configure.in <<END
> +AC_PROG_CC
> +AC_PROG_RANLIB
> +AC_OUTPUT
> +END
> +
> +cat >Makefile.am <<END
> +EXTRA_LIBRARIES = libfoo.a
> +libfoo_a_SOURCES = sub/foo.c
> +libfoo_a_CPPFLAGS = -Dwhatever
> +END
> +
> +$ACLOCAL
> +
> +# enable extra-portability enables portability
>
I'd do s/enable/enabling/ (ping, native speakers?).  Similarly
for other usages throughout the file.

> +AUTOMAKE_fails -Wnone -Wextra-portability
> +# The expected diagnostic is
> +#    Makefile.am:2: compiling `foo.c' with per-target flags requires 
> `AM_PROG_CC_C_O' in `configure.in'
> +#    .../lib/am/library.am: `libfoo.a': linking libraries using a non-POSIX
> +#    .../lib/am/library.am: archiver requires `AM_PROG_AR' in `configure.in'
> +#    Makefile.am:1:   while processing library `libfoo.a'
> +grep '^Makefile.am:2:.*requires.*AM_PROG_CC_C_O' stderr
> +grep '/library.am:.*requires.*AM_PROG_AR' stderr
> +
No need to also grep the filenames here.  Similarly for other usages
throughout the file.

> [SNIP]

> diff --git a/tests/extra-portability2.test b/tests/extra-portability2.test
> new file mode 100755
> index 0000000..9f4948b
> --- /dev/null
> +++ b/tests/extra-portability2.test
> @@ -0,0 +1,57 @@
> +#! /bin/sh
> +# Copyright (C) 2011  Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2, or (at your option)
> +# any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Make sure that extra-portability is not enabled by --gnits, --gnu
>
s/extra-portability is/extra-portability warnings are/?

> +# and --foreign
> +
> +. ./defs || Exit 1
> +
> +set -e
> +
> +# satisfy --gnits and --gnu
>
We try to use proper capitalization and full stops in comments:

  # Satisfy --gnits and --gnu.

> +: > INSTALL
> +: > NEWS
> +: > README
> +: > AUTHORS
> +: > ChangeLog
> +: > COPYING
> +: > THANKS
> +
> +cat >>configure.in <<END
> +AC_PROG_CC
> +AC_PROG_RANLIB
> +AC_OUTPUT
> +END
> +
> +cat >Makefile.am <<END
> +EXTRA_LIBRARIES = libfoo.a
> +libfoo_a_SOURCES = foo.c
> +END
> +
> +$ACLOCAL
> +
> +# Make sure the test is useful.
> +AUTOMAKE_fails
>
How is it that automake is failing here, without explicilty
using `-Wextra-portability'?

> +# The expected diagnostic is
> +#    .../lib/am/library.am: `libfoo.a': linking libraries using a non-POSIX
> +#    .../lib/am/library.am: archiver requires `AM_PROG_AR' in `configure.in'
> +grep '/library.am:.*requires.*AM_PROG_AR' stderr
> +
No need to also grep the diagostic IMHO.  The above sanity check
verifying that automake fails when `extra-portability' warnings are
on should be enough.

> +$AUTOMAKE --foreign
> +$AUTOMAKE --gnu
> +$AUTOMAKE --gnits
> +
> +:

> [MEGA-SNIP]

Thanks,
  Stefano