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

[Peter Rosin]

Stefano Lattarini skrev 2011-10-20 17:11:
> 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}/.

done

>> 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'.

fine with me (but with -Wno-extra-portability at the end of 2).

>> +
>> +. ./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.

yes, right

>> +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.

Do you mean /library.am only, or do you also mean Makefile.am:2:?
If you do not think Makefile.am:2: is needed, then why did you want me
to grep `FILE:LINENO' for the other patch, but not here?

>> [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/?

ok

>> +# 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.

right

>> +: > 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'?

-Wall implies *all* warnings, remember?

>> +# 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.

ok

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

Cheers,
Peter