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)
>> +@item 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
