bug#9400: parallel-tests doesn't support `-' character in test extensions

[Stefano Lattarini]

On Monday 29 August 2011, Stefano Lattarini wrote:
> I catched this while working some more on the Automake testsuite:
> 
>  $ cat Makefile.am
>  TEST_EXTENSIONS = .t-1
>  TESTS = foo.t-1
>  $ cat configure.ac
>  AC_INIT([foo], [1.0])
>  AM_INIT_AUTOMAKE([foreign -Wall parallel-tests])
>  AC_CONFIG_FILES([Makefile])
>  AC_OUTPUT
>  $ aclocal
>  $ autoconf
>  $ : > install-sh
>  $ : > missing
>  $ automake
>  automake: bad characters in variable name `T-1_LOG_COMPILE'
>  automake: T-1_LOG_COMPILER: non-POSIX variable name
>  automake: AM_T-1_LOG_FLAGS: non-POSIX variable name
>  automake: T-1_LOG_FLAGS: non-POSIX variable name
> 
> Do you think it would be worthwhile to lift this limitation, by
> automatically transliterating `-' (and maybe other non-alphabetic
> characters) to `_'?  Or should we simply give an error when something
> in $(TEST_EXTENSIONS) contains non-alphabetic characters?
> 
> Regards,
>   Stefano
> 
For the moment, I've decided to take the easiest route, documenting the
limitation and correcting the diagnostic.  Such restriction could easily
be lifted at a later time, with no backward-compatibility issues.

Attached is the patch I'll push in a couple of days if there is no
objection.  As usual, reviews are welcome.

Regards,
  Stefano

From 90bea64bc5023be075b63bf7c651d0242f35a83c Mon Sep 17 00:00:00 2001
Message-Id: <address@hidden>
From: Stefano Lattarini <address@hidden>
Date: Sat, 1 Oct 2011 21:31:07 +0200
Subject: [PATCH] parallel-tests: automake error our on invalid TEST_EXTENSIONS

This change fixes automake bug#9400.

* automake.in (handle_tests): Bail out if a suffix specified in
TEST_EXTENSIONS would produce an invalid `xxx_LOG_COMPILER'
variable or an invalid suffix rule.  Before this change, automake
would have issued a confusing error messages (about invalid or
non-POSIX variables being defined), and in some situations would
have even produced a broken `Makefile.in' file.
($TEST_EXTENSION_PATTERN): New helper variable.
* doc/automake.texi (Simple Tests using parallel-tests): Document
the limitations on TEST_EXTENSIONS explicitly.
* NEWS: Update.
* tests/test-extensions.test: New test.
* tests/Makefile.am (TESTS): Update.
---
 ChangeLog                  |   17 ++++++++++
 NEWS                       |    6 +++
 automake.in                |   12 ++++++-
 doc/automake.texi          |   12 +++++--
 tests/Makefile.am          |    1 +
 tests/Makefile.in          |    1 +
 tests/test-extensions.test |   73 ++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 118 insertions(+), 4 deletions(-)
 create mode 100755 tests/test-extensions.test

diff --git a/ChangeLog b/ChangeLog
index 47aee92..e52a275 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,20 @@
+2011-10-01  Stefano Lattarini  <address@hidden>
+
+       parallel-tests: automake error our on invalid TEST_EXTENSIONS
+       This change fixes automake bug#9400.
+       * automake.in (handle_tests): Bail out if a suffix specified in
+       TEST_EXTENSIONS would produce an invalid `xxx_LOG_COMPILER'
+       variable or an invalid suffix rule.  Before this change, automake
+       would have issued a confusing error messages (about invalid or
+       non-POSIX variables being defined), and in some situations would
+       have even produced a broken `Makefile.in' file.
+       ($TEST_EXTENSION_PATTERN): New helper variable.
+       * doc/automake.texi (Simple Tests using parallel-tests): Document
+       the limitations on TEST_EXTENSIONS explicitly.
+       * NEWS: Update.
+       * tests/test-extensions.test: New test.
+       * tests/Makefile.am (TESTS): Update.
+
 2011-09-22  Stefano Lattarini  <address@hidden>
 
        tests: fix tests on aclocal search path precedences
diff --git a/NEWS b/NEWS
index b696977..fe78238 100644
--- a/NEWS
+++ b/NEWS
@@ -49,6 +49,12 @@ Bugs fixed in 1.11.0a:
     does not report spurious successes when used with concurrent FreeBSD
     make (e.g., "make check -j3").
 
+  - Automake now explicitly rejects invalid entries in TEST_EXTENSIONS when
+    the parallel-tests diver is in use, instead of issuing confusing and
+    apparently unrelated error messages (about "non-POSIX variable name"
+    or "bad characters in variable name"), or even, in some situations,
+    producing broken `Makefile.in' files.
+
   - The `silent-rules' option now also silences all compile rules if dependency
     tracking is disabled.  Also, when `silent-rules' is not used, the output 
from
     `make' does not contain spurious extra lines with only a backslash in them
diff --git a/automake.in b/automake.in
index 215881b..a60bc9f 100755
--- a/automake.in
+++ b/automake.in
@@ -213,6 +213,8 @@ my $DASH_D_PATTERN = "(^|\\s)-d(\\s|\$)";
 # Directories installed during 'install-exec' phase.
 my $EXEC_DIR_PATTERN =
   '^(?:bin|sbin|libexec|sysconf|localstate|lib|pkglib|.*exec.*)' . "\$";
+# Suffixes that can appear in TEST_EXTENSIONS (parallel-tests support).
+my $TEST_EXTENSION_PATTERN = '^(\.[a-zA-Z_][a-zA-Z0-9_]*|@[a-zA-Z0-9_]+@)$';
 
 # Values for AC_CANONICAL_*
 use constant AC_CANONICAL_BUILD  => 1;
@@ -4971,7 +4973,15 @@ sub handle_tests
            }
          define_variable ('TEST_EXTENSIONS', $suff, INTERNAL);
          # FIXME: this mishandles conditions.
-         my @test_suffixes = (var 'TEST_EXTENSIONS')->value_as_list_recursive;
+         my $var = rvar 'TEST_EXTENSIONS';
+         my @test_suffixes = $var->value_as_list_recursive;
+          if ((my @invalid_test_suffixes =
+                  grep { !/$TEST_EXTENSION_PATTERN/o } @test_suffixes) > 0)
+            {
+              error $var->rdef (TRUE)->location,
+                    "invalid test extensions: @invalid_test_suffixes";
+            }
+          @test_suffixes = grep { /$TEST_EXTENSION_PATTERN/o } @test_suffixes;
          if ($handle_exeext)
            {
              unshift (@test_suffixes, $at_exeext)
diff --git a/doc/automake.texi b/doc/automake.texi
index a8233dd..875e191 100644
--- a/doc/automake.texi
+++ b/doc/automake.texi
@@ -8808,9 +8808,15 @@ Each log file is created when the corresponding test has 
completed.
 The set of log files is listed in the read-only variable
 @code{TEST_LOGS}, and defaults to @code{TESTS}, with the executable
 extension if any (@pxref{EXEEXT}), as well as any suffix listed in
address@hidden removed, and @file{.log} appended.
address@hidden defaults to @file{.test}.  Results are undefined
-if a test file name ends in several concatenated suffixes.
address@hidden removed, and @file{.log} appended.  Results
+are undefined if a test file name ends in several concatenated suffixes.
address@hidden defaults to @file{.test}; it can be overridden by
+the user, in which case any extension listed in it must be constituted
+by a dot, followed by a non-digit alphabetic character, followed by any
+number of alphabetic characters.
address@hidden Keep in sync with test-extensions.test.
+For example, @samp{.sh}, @samp{.T} and @samp{.t1} are valid extensions,
+while @samp{.x-y}, @samp{.6c} and @samp{.t.1} are not.
 
 @vindex _LOG_COMPILE
 @vindex _LOG_COMPILER
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 1d258c9..3f9fc08 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -628,6 +628,7 @@ parallel-tests-log-override-1.test \
 parallel-tests-log-override-2.test \
 parallel-tests-log-override-recheck.test \
 parallel-tests-log-compiler-example.test \
+test-extensions.test \
 parse.test \
 percent.test \
 percent2.test \
diff --git a/tests/Makefile.in b/tests/Makefile.in
index 7e9bc20..2e7f39f 100644
--- a/tests/Makefile.in
+++ b/tests/Makefile.in
@@ -906,6 +906,7 @@ parallel-tests-log-override-1.test \
 parallel-tests-log-override-2.test \
 parallel-tests-log-override-recheck.test \
 parallel-tests-log-compiler-example.test \
+test-extensions.test \
 parse.test \
 percent.test \
 percent2.test \
diff --git a/tests/test-extensions.test b/tests/test-extensions.test
new file mode 100755
index 0000000..1d5872c
--- /dev/null
+++ b/tests/test-extensions.test
@@ -0,0 +1,73 @@
+#! /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 Automake diagnose invalid entries in TEST_EXTENSIONS,
+# and do not diagnose valid (albeit more unusual) ones.
+# See automake bug#9400.
+
+parallel_tests=yes
+. ./defs || Exit 1
+
+set -e
+
+cat >> configure.in <<'END'
+AC_SUBST([ext], [".e"])
+AC_OUTPUT
+END
+
+$ACLOCAL
+$AUTOCONF
+
+cat > Makefile.am << 'END'
+TESTS =
+TEST_EXTENSIONS = .sh .T .t1 ._foo .BAR .x_Y_z ._ @ext@
+END
+
+$AUTOMAKE
+
+$EGREP -i 'log|ext' Makefile.in # For debugging.
+
+for lc in sh T t1 _foo BAR x_Y_z _; do
+  uc=`echo $lc | tr '[a-z]' '[A-Z]'`
+  $FGREP "\$(${uc}_LOG_COMPILER)" Makefile.in
+  grep "^${uc}_LOG_COMPILE =" Makefile.in
+  grep "^\.${lc}\.log:" Makefile.in
+done
+grep "address@hidden@\.log:" Makefile.in
+
+# The produced Makefile is not broken.
+./configure
+$MAKE all check
+
+cat > Makefile.am << 'END'
+TESTS = foo.test bar.sh
+TEST_EXTENSIONS = .test mu .x-y a-b .t.1 .sh .6c .0 .11 .@ .t33 .a=b _&_
+END
+
+AUTOMAKE_fails
+for suf in mu .x-y a-b .t.1 .6c .0 .11 '.@' '.a=b' '_&_'; do
+  suf2=`printf '%s\n' "$suf" | sed -e 's/\./\\./'`
+  $EGREP "^Makefile\.am:2:.*invalid test extension.* $suf2( |$)" stderr
+done
+
+# Verify that we accept valid suffixes, even if intermixed with
+# invalid ones.
+$EGREP '\.(sh|test|t33)' stderr && Exit 1
+
+# Verify that we don't try to handle invalid suffixes.
+$EGREP '(LOG_COMPILER|non-POSIX var|bad character)' stderr && Exit 1
+
+:
-- 
1.7.2.3