Re: [PATCH] tests: avoid false positive due to change in --help formatting

[Stefano Lattarini]

Hi Jim, thanks for the report

On Friday 04 November 2011, Jim Meyering wrote:
> I updated to the latest from master and noticed that "make check"
> showed a single failing test.  Here's the fix:
> 
> [Because --help now looks like this:
> 
>   --disable-maintainer-mode
>                           disable make rules and dependencies not useful (and
>  Presumably, before the description was on the same line
>  as the option name.  ]
>
Yes; in fact, in `maint', the description *still* is on the same line as
the option name; but it's not so anymore in `master', where we've started
using the AS_HELP_STRING macro more consistently.

> 
> From f2be2788fc3ee1710cf9a6f21ddc6ba33be29b90 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <address@hidden>
> Date: Fri, 4 Nov 2011 10:48:31 +0100
> Subject: [PATCH] tests: avoid false positive due to change in --help
>  formatting
> 
> * tests/maintmode-configure-msg.test: Truncate regexp, now that
> --enable-maintainer-mode and its description are on separate lines
> in --help output.  Do the same for --disable-maintainer-mode.
>
Rather than relaxing the test, I'd prefer to introduce in the automake
testsuite a new auxiliary function that knows how to extract the portion
of a configure help screen associated with a given option and/or precious
variable.  We could then start using this function right away in other
similar tests as well, hopefully reducing the risk of similar regression
in the future.

Attached is a patch in this spirit; I'll push in a few hours if there is
no objection.

Regards,
  Stefano

From db93b31d81c7809e49fa874acfd5e3aa3504dc48 Mon Sep 17 00:00:00 2001
Message-Id: <address@hidden>
From: Stefano Lattarini <address@hidden>
Date: Fri, 4 Nov 2011 12:50:49 +0100
Subject: [FYI] {msvc} warnings: fix buglets for portability warnings

* lib/Automake/ChannelDefs.pm (switch_warning): Ensure the
correct implications and inter-dependencies between warnings
in the categories `portability', `extra-portability' and
`recursive-portability' are respected.  Also add detailed
explicative comments, and references to the relevant tests.
* tests/dollarvar2.test: Update and extend.  Also, remove
some unnecessary uses of `--force' option in automake calls.
* tests/extra-portability3.test: New test.
* tests/Makefile.am (TESTS): Add it.
---
 ChangeLog                     |   13 ++++++++
 lib/Automake/ChannelDefs.pm   |   38 ++++++++++++++++++++----
 tests/Makefile.am             |    1 +
 tests/Makefile.in             |    1 +
 tests/dollarvar2.test         |   65 +++++++++++++++++++++++++++++++++++++---
 tests/extra-portability3.test |   63 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 170 insertions(+), 11 deletions(-)
 create mode 100755 tests/extra-portability3.test

diff --git a/ChangeLog b/ChangeLog
index 0629e5f..d3e3502 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,18 @@
 2011-11-04  Stefano Lattarini  <address@hidden>
 
+       warnings: fix buglets for portability warnings
+       * lib/Automake/ChannelDefs.pm (switch_warning): Ensure the
+       correct implications and inter-dependencies between warnings
+       in the categories `portability', `extra-portability' and
+       `recursive-portability' are respected.  Also add detailed
+       explicative comments, and references to the relevant tests.
+       * tests/dollarvar2.test: Update and extend.  Also, remove
+       some unnecessary uses of `--force' option in automake calls.
+       * tests/extra-portability3.test: New test.
+       * tests/Makefile.am (TESTS): Add it.
+
+2011-11-04  Stefano Lattarini  <address@hidden>
+
        tests: extend tests on 'extra-portability' warning category
        * tests/extra-portability.test: Redefine `$AUTOMAKE' to ensure we
        have complete control over the automake options.  Extend by using
diff --git a/lib/Automake/ChannelDefs.pm b/lib/Automake/ChannelDefs.pm
index 61b4ed4..f9f63fc 100644
--- a/lib/Automake/ChannelDefs.pm
+++ b/lib/Automake/ChannelDefs.pm
@@ -288,12 +288,38 @@ sub switch_warning ($)
   elsif (channel_type ($cat) eq 'warning')
     {
       setup_channel $cat, silent => $has_no;
-      setup_channel 'portability-recursive', silent => $has_no
-        if $cat eq 'portability';
-      setup_channel 'extra-portability', silent => $has_no
-        if ($cat eq 'portability' && $has_no);
-      setup_channel 'portability', silent => $has_no
-        if ($cat eq 'extra-portability' && ! $has_no);
+      #
+      # Handling of portability warnings is trickier.  For relevant tests,
+      # see `dollarvar2', `extra-portability' and `extra-portability3'.
+      #
+      # -Wportability-recursive and -Wno-portability-recursive should not
+      # have any effect on other 'portability' or 'extra-portability'
+      # warnings, so there's no need to handle them separately or ad-hoc.
+      #
+      if ($cat eq 'extra-portability' && ! $has_no) # -Wextra-portability
+        {
+          # -Wextra-portability must enable 'portability' and
+          # 'portability-recursive' warnings.
+          setup_channel 'portability', silent => 0;
+          setup_channel 'portability-recursive', silent => 0;
+        }
+      if ($cat eq 'portability') # -Wportability or -Wno-portability
+        {
+          if ($has_no) # -Wno-portability
+            {
+              # -Wno-portability must disable 'extra-portability' and
+              # 'portability-recursive' warnings.
+              setup_channel 'portability-recursive', silent => 1;
+              setup_channel 'extra-portability', silent => 1;
+            }
+          else # -Wportability
+            {
+              # -Wportability must enable 'portability-recursive'
+              # warnings.  But it should have no influence over the
+              # 'extra-portability' warnings.
+              setup_channel 'portability-recursive', silent => 0;
+            }
+        }
     }
   else
     {
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 8056f2d..a27cdaf 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -398,6 +398,7 @@ extra11.test \
 extra12.test \
 extra-portability.test \
 extra-portability2.test \
+extra-portability3.test \
 f90only.test \
 flavor.test \
 flibs.test \
diff --git a/tests/Makefile.in b/tests/Makefile.in
index 8e6eb4c..cdd8de2 100644
--- a/tests/Makefile.in
+++ b/tests/Makefile.in
@@ -682,6 +682,7 @@ extra11.test \
 extra12.test \
 extra-portability.test \
 extra-portability2.test \
+extra-portability3.test \
 f90only.test \
 flavor.test \
 flibs.test \
diff --git a/tests/dollarvar2.test b/tests/dollarvar2.test
index 6fc2737..7a6db37 100755
--- a/tests/dollarvar2.test
+++ b/tests/dollarvar2.test
@@ -21,6 +21,11 @@
 
 set -e
 
+#
+# First, try a setup where we have a `portability-recursive' warning,
+# but no "simple" `portability' warning.
+#
+
 cat >Makefile.am <<'EOF'
 x = 1
 bla = $(foo$(x))
@@ -28,11 +33,61 @@ EOF
 
 $ACLOCAL
 
-# $AUTOMAKE already contains -Wall -Werror.
-AUTOMAKE_fails -Wportability
-$AUTOMAKE --force -Wno-all
-$AUTOMAKE --force -Wno-portability
+# Enabling `portability' warnings should enable `portability-recursive'
+# warnings.
+AUTOMAKE_fails -Wnone -Wportability
+grep 'recursive variable expansion' stderr
+# `portability-recursive' warnings can be enabled by themselves.
+AUTOMAKE_fails -Wnone -Wportability-recursive
+grep 'recursive variable expansion' stderr
+
+# Various ways to disable `portability-recursive'.
+$AUTOMAKE -Wno-all
+$AUTOMAKE -Wno-portability
+$AUTOMAKE -Wall -Wno-portability-recursive
+
+# `-Wno-portability-recursive' after `-Wportability' correctly disables
+# `portability-recursive' warnings.
+$AUTOMAKE -Wportability -Wno-portability-recursive
+
+# `-Wno-portability' disables `portability-recursive' warnings; but
+# a later `-Wportability-recursive' re-enables them.  This time, we
+# use AUTOMAKE_OPTIONS to specify the warning levels.
 echo 'AUTOMAKE_OPTIONS = -Wno-portability' >> Makefile.am
-$AUTOMAKE --force
+$AUTOMAKE
+echo 'AUTOMAKE_OPTIONS += -Wportability-recursive' >> Makefile.am
+AUTOMAKE_fails
+grep 'recursive variable expansion' stderr
+
+#
+# Now try a setup where we have both a `portability' warning and
+# a `portability-recursive' one.
+#
+
+cat >Makefile.am <<'EOF'
+x = 1
+bla = $(foo$(x))
+noinst_PROGRAMS = foo
+foo_CPPFLAGS = -Dwhatever
+EOF
+
+echo AC_PROG_CC >> configure.in
+
+$ACLOCAL --force
+
+# Can disable both `portability' and `portability-recursive' warnings.
+$AUTOMAKE -Wno-portability
+
+# Disabling `portability-recursive' warnings should not disable
+# `portability' warnings.
+AUTOMAKE_fails -Wportability -Wno-portability-recursive
+grep AM_PROG_CC_C_O stderr
+grep 'recursive variable expansion' stderr && Exit 1
+
+# Enabling `portability-recursive' warnings should not enable
+# all the `portability' warning.
+AUTOMAKE_fails -Wno-portability -Wportability-recursive
+grep AM_PROG_CC_C_O stderr && Exit 1
+grep 'recursive variable expansion' stderr
 
 :
diff --git a/tests/extra-portability3.test b/tests/extra-portability3.test
new file mode 100755
index 0000000..7ca19b1
--- /dev/null
+++ b/tests/extra-portability3.test
@@ -0,0 +1,63 @@
+#! /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/>.
+
+# Check interactions between the `portability-recursive' and
+# `extra-portability' warning categories.
+
+. ./defs || Exit 1
+
+set -e
+
+# We want (almost) complete control over automake options.
+# FIXME: use $original_AUTOMAKE here once we are merged into master.
+AUTOMAKE="`(set $AUTOMAKE && echo $1)` --foreign -Werror"
+
+cat >>configure.in <<END
+AC_PROG_CC
+AC_PROG_RANLIB
+AC_OUTPUT
+END
+
+$ACLOCAL
+
+cat >Makefile.am <<'END'
+baz = $(foo$(bar))
+lib_LIBRARIES = libfoo.a
+libfoo_a_SOURCES = foo.c
+END
+
+# 'extra-portability' implies 'portability-recursive'.
+AUTOMAKE_fails -Wextra-portability
+grep 'requires.*AM_PROG_AR' stderr
+grep 'recursive variable expansion' stderr
+
+# We can disable 'extra-portability' while leaving
+# 'portability-recursive' intact.
+AUTOMAKE_fails -Wportability-recursive -Wno-extra-portability
+grep 'requires.*AM_PROG_AR' stderr && Exit 1
+grep 'recursive variable expansion' stderr
+
+# We can disable 'portability-recursive' while leaving
+# 'extra-portability' intact.
+AUTOMAKE_fails -Wextra-portability -Wno-portability-recursive
+grep 'requires.*AM_PROG_AR' stderr
+grep 'recursive variable expansion' stderr && Exit 1
+
+# Disabling 'portability' disables 'portability-recursive' and
+# 'extra-portability'.
+$AUTOMAKE -Wextra-portability -Wno-portability
+
+:
-- 
1.7.2.3