Subject: Re: parallel-tests: avoid command-line length limit issue.

From: Stefano Lattarini <[email-masked]>
Date: Fri, 30 Dec 2011 13:20:15 +0100

On 12/29/2011 12:26 AM, Stefano Lattarini wrote:
> On 12/28/2011 08:27 PM, Bob Friesenhahn wrote:
>> Sorry for top-posting.  I don't want to lose any text of the original mail.
>>
>> I totally forgot that I had applied Ralf's patch to my 1.11.1 install. Now
>> that I have updated GraphicsMagick to using Automake 1.11.2, the patch is no
>> longer present and so 'make check' on MinGW/MSYS leads to this:
>>
>> /usr/bin/csmake  check-TESTS check-local
>> csmake[2]: Entering directory `/home/bfriesen/mingw/GM-16-static'
>> csmake[3]: Entering directory `/home/bfriesen/mingw/GM-16-static'
>> csmake[3]: execvp: /bin/sh: Invalid argument
>> csmake[3]: *** [tests/constitute_char_bgr.log] Error 127
>> csmake[3]: Leaving directory `/home/bfriesen/mingw/GM-16-static'
>> csmake[2]: *** [check-TESTS] Error 2
>> csmake[2]: Leaving directory `/home/bfriesen/mingw/GM-16-static'
>> csmake[1]: *** [check-am] Error 2
>> csmake[1]: Leaving directory `/home/bfriesen/mingw/GM-16-static'
>> csmake: *** [check] Error 2
>>
>> Today I lost more hair because I had totally forgotten about this issue and
>> the problem was first noticed after making some changes to configure.ac.
>>
>> Hopefully this problem can be resolved before Automake 1.12 is released.
>>
> Let's start by exposing the problem once and for all.
> 
> [SNIP]
>
OK, I hope I've finally managed to partially fix this incredibly annoying bug
-- I say "partially" because the fix is sadly *for GNU make*.  Even more sadly,
the solution is pretty hacky and somewhat brittle.   Still, it should cause no
regression with the non-GNU makes (as they will continue to use the old
implementation), and I have verified that it works with at least all the GNU
make versions >= 3.78.

I'm posting the patches here for early feedback (note that the second one still
needs a commit message and possibly some polishing), with the hope that someone
can suggest a less hacky solution.

Regards,
  Stefano
>From e9d7d23529b21511e4d3d7c71bd239a45288e4bc Mon Sep 17 00:00:00 2001
Message-Id: <e9d7d2352[email-masked]>
From: Stefano Lattarini <[email-masked]>
Date: Wed, 28 Dec 2011 22:37:44 +0100
Subject: [PATCH 1/2] coverage: expose automake bug#7868

Expose the command-line length limit issue that can affect the
Automake-generated parallel-tests harness, especially on systems
where this limit is smaller (e.g., MinGW/MSYS).

Suggestion by Bob Friesenhahn.

* tests/parallel-tests-many.test: New test.  We have verified that
it actually exposes the bug#7868, as it passes when we opportunely
reduce the number of test cases in $(TESTS).  Checked on NetBSD 5.1
64bit, Debian unstable 32bit, Solaris 10 64bit and Cygwin 1.5 32bit.
* tests/Makefile.am (TESTS, XFAIL_TESTS): Add it.
---
 tests/Makefile.am              |    2 +
 tests/parallel-tests-many.test |  176 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 178 insertions(+), 0 deletions(-)
 create mode 100755 tests/parallel-tests-many.test

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 6ce7c53..e14d57e 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -26,6 +26,7 @@ gcj6.test \
 java-nobase.test \
 pr8365-remake-timing.test \
 lex-subobj-nodep.test \
+parallel-tests-many.test \
 remake-am-pr10111.test \
 remake-m4-pr10111.test \
 vala-vpath.test \
@@ -648,6 +649,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 \
+parallel-tests-many.test \
 test-extensions.test \
 test-extensions-cond.test \
 parse.test \
diff --git a/tests/parallel-tests-many.test b/tests/parallel-tests-many.test
new file mode 100755
index 0000000..ef01db1
--- /dev/null
+++ b/tests/parallel-tests-many.test
@@ -0,0 +1,176 @@
+#! /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 that the parallel-tests harness does not hit errors due to
+# an exceeded command line length when there are many tests.
+# For automake bug#7868.  This test is currently expected to fail.
+
+parallel_tests=yes
+. ./defs || Exit 1
+
+set -e
+
+cat >> configure.in << 'END'
+AC_OUTPUT
+END
+
+cat > Makefile.am <<'END'
+# Sanity check that the $(TESTS) is going to exceed the system
+# command line length.
+# Extra quoting and indirections below are required to ensure the
+# various make implementations (e.g, GNU make or Sun Distributed Make)
+# will truly spawn a shell to execute this command, instead of relying
+# on optimizations that might mask the "Argument list too long" error
+# we expect.
+this-will-fail:
+	@":" && ":" $(TEST_LOGS)
+TEST_LOG_COMPILER = true
+include list-of-tests.am
+# So that we won't have to create a ton of dummy test cases.
+$(TESTS):
+END
+
+# The real instance will be dynamically created later.
+echo TESTS = foo.test > list-of-tests.am
+
+$ACLOCAL && $AUTOCONF && $AUTOMAKE -a \
+  || framework_failure_ "unexpected autotools failure"
+./configure \
+  || framework_failure_ "unexpected configure failure"
+
+# We want to hit the system command-line length limit without hitting
+# the filename length limit or the PATHMAX limit; so we use longish
+# (but not too long) names for the testcase, and place them in a nested
+# (but not too deeply) directory.
+# We also prefer to use the minimal(ish) number of test cases that can
+# make us hit the command-line length limit, since the more the test
+# cases are, the more time "automake" and "make check" will take to run
+# (especially on Cygwin and MinGW/MSYS).
+
+tname="wow-this-is-a-very-long-name-for-a-simple-dummy-test-case"
+dname="and-this-too-is-a-very-long-name-for-a-dummy-directory"
+
+deepdir=.
+depth=0
+for i in 1 2 3 4 5 6 7 8 9 10 12 13 14 15 16 17 18 19 29 21 22 23 24; do
+  new_deepdir=$deepdir/$dname.d$i
+  mkdir $new_deepdir || break
+  tmpfile=$new_deepdir/$tname-some-more-chars-for-good-measure
+  if touch $tmpfile; then
+    rm -f $tmpfile
+  else
+    rmdir $new_deepdir
+  fi
+  deepdir=$new_deepdir
+  unset tmpfile new_deepdir
+  depth=$i
+done
+
+cat <<END
+*********************************************************************
+Our tests will be in the following directory (depth = $depth)
+*********************************************************************
+$deepdir
+*********************************************************************
+END
+
+setup_data ()
+{
+  awk "
+    BEGIN {
+      print \"TESTS = \\\\\"
+      for (i = 1; i < $count; i = i + 1)
+        print \"  $deepdir/$tname-\" i \".test \\\\\"
+      print \"  $deepdir/$tname-\" i \".test\"
+    }
+  " > list-of-tests.am || Exit 99
+  $AUTOMAKE Makefile \
+    || framework_failure_ "unexpected automake failure"
+  ./config.status Makefile \
+    || framework_failure_ "unexpected config.status failure"
+}
+
+for count in 1 2 4 8 12 16 20 24 28 32 48 64 96 128 E_HUGE; do
+  test $count = E_HUGE && break
+  count=`expr $count '*' 100`
+  setup_data
+  if $MAKE this-will-fail; then
+    continue
+  else
+    # We have managed to find a number of test cases large enough to
+    # hit the system command-line limits; we can stop.  But first, for
+    # good measure, increase the number of tests of some 20%, to be
+    # "even more sure" of really tickling command line length limits.
+    count=`expr '(' $count '*' 12 ')' / 10` || Exit 99
+    setup_data
+    break
+  fi
+done
+
+if test $count = E_HUGE; then
+  framework_failure_ "system has a too-high limit on command line length"
+else
+  cat <<END
+*********************************************************************
+               Number of tests we will use: $count
+*********************************************************************
+END
+fi
+
+env TESTS=$deepdir/$tname-1.test $MAKE -e check \
+  && test -f $deepdir/$tname-1.log \
+  || framework_failure_ "\"make check\" with one single tests"
+
+rm -f $deepdir/* || Exit 99
+
+$MAKE check > stdout || { cat stdout; Exit 1; }
+cat stdout
+
+grep "All $count tests" stdout
+
+grep "^PASS: .*$tname-[0-9][0-9]*\.test" stdout > grp
+ls -1 $deepdir | grep '\.log$' > lst
+
+sed 20q lst # For debugging.
+sed 20q grp # Likewise.
+
+test `cat <grp | wc -l` -eq $count
+test `cat <lst | wc -l` -eq $count
+
+# We need to simulate a failure of two tests.
+st=0
+env TESTS="$deepdir/$tname-1.test $deepdir/$tname-2.test" \
+    TEST_LOG_COMPILER=false $MAKE -e check > stdout && st=1
+cat stdout
+test `grep -c '^FAIL:' stdout` -eq 2 || st=1
+test $st -eq 0 || fatal_ "couldn't simulate failure of two tests"
+unset st
+
+$MAKE recheck > stdout || { cat stdout; Exit 1; }
+cat stdout
+grep "^PASS: .*$tname-1\.test" stdout
+grep "^PASS: .*$tname-2\.test" stdout
+test `LC_ALL=C grep -c "^[A-Z][A-Z]*:" stdout` -eq 2
+grep "All 2 tests" stdout
+
+# "make clean" might ignore some failures, so we prefer to also grep its
+# output to ensure that no "Argument list too long" error was encountered.
+$MAKE clean >output 2>&1 || { cat output; Exit 1; }
+cat output
+grep -i 'list.* too long' output && Exit 1
+ls -1 $deepdir | grep '\.log$' && Exit 1
+
+:
-- 
1.7.7.3

>From ca6000f9cf6c84c516230dc86eba8b9d920dfb20 Mon Sep 17 00:00:00 2001
Message-Id: <ca6000f9c[email-masked]>
In-Reply-To: <e9d7d2352[email-masked]>
References: <e9d7d2352[email-masked]>
From: Stefano Lattarini <[email-masked]>
Date: Thu, 29 Dec 2011 10:46:48 +0100
Subject: [PATCH 2/2] fix the bug for GNU make

---
 NEWS                           |    6 +++
 automake.in                    |   22 +++++-----
 lib/am/check.am                |   86 ++++++++++++++++++++++++++++++++++------
 m4/init.m4                     |   17 ++++++++
 tests/Makefile.am              |    1 -
 tests/parallel-tests-many.test |    4 +-
 6 files changed, 111 insertions(+), 25 deletions(-)

diff --git a/NEWS b/NEWS
index f6eb29f..34948f3 100644
--- a/NEWS
+++ b/NEWS
@@ -73,6 +73,12 @@ Bugs fixed in 1.11.0a:
 
 * Bugs introduced by 1.11:
 
+  - When GNU make is used, the parallel testsuite harness can now work
+    around problems due to exceeded command-line length limits,
+    problems that that could be encountered when issuing "make check"
+    and "make clean".  This is especially important for system with
+    low command-line length limits, like MinGW/MSYS.
+
   - The `parallel-tests' test driver works around a GNU make 3.80 bug with
     trailing white space in the test list (`TESTS = foo $(EMPTY)'), and
     does not report spurious successes when used with concurrent FreeBSD
diff --git a/automake.in b/automake.in
index 309eade..a25c5c9 100644
--- a/automake.in
+++ b/automake.in
@@ -4933,9 +4933,13 @@ sub is_valid_test_extension ($)
   return 0;
 }
 
+# handle_tests ($MAKEFILE)
+# ------------------------
 # Handle TESTS variable and other checks.
-sub handle_tests
+sub handle_tests ($)
 {
+  my ($makefile) = @_;
+
   if (option 'dejagnu')
     {
       &handle_tests_dejagnu;
@@ -4954,7 +4958,8 @@ sub handle_tests
       push (@check_tests, 'check-TESTS');
       $output_rules .= &file_contents ('check', new Automake::Location,
 				       COLOR => !! option 'color-tests',
-				       PARALLEL_TESTS => !! option 'parallel-tests');
+				       PARALLEL_TESTS => !! option 'parallel-tests',
+				       MAKEFILE => basename $makefile);
 
       # Tests that are known programs should have $(EXEEXT) appended.
       # For matching purposes, we need to adjust XFAIL_TESTS as well.
@@ -5046,7 +5051,7 @@ sub handle_tests
 	    {
 	      if ($test_suffix eq $last_suffix)
 	        {
-		  $cur = 'TEST_LOGS';
+		  $cur = 'am__test_logs';
 		}
 	      else
 	        {
@@ -5075,13 +5080,8 @@ sub handle_tests
 						  am__EXEEXT => $am_exeext);
 	        }
 	    }
-
-	  define_variable ('TEST_LOGS_TMP', '$(TEST_LOGS:.log=.log-t)', INTERNAL);
-
-	  $clean_files{'$(TEST_LOGS_TMP)'} = MOSTLY_CLEAN;
-	  $clean_files{'$(TEST_LOGS)'} = MOSTLY_CLEAN;
-	  $clean_files{'$(TEST_SUITE_LOG)'} = MOSTLY_CLEAN;
-	  $clean_files{'$(TEST_SUITE_HTML)'} = MOSTLY_CLEAN;
+	  # This needs to be further postprocessed in lib:am/check.am.
+	  define_variable ('am__test_logs', "\$($cur)", INTERNAL);
 	}
     }
 }
@@ -8269,7 +8269,7 @@ sub generate_makefile ($$)
   handle_tags;
   handle_minor_options;
   # Must come after handle_programs so that %known_programs is up-to-date.
-  handle_tests;
+  handle_tests ($makefile);
 
   # This must come after most other rules.
   handle_dist;
diff --git a/lib/am/check.am b/lib/am/check.am
index 3d0188d..82d9fe2 100644
--- a/lib/am/check.am
+++ b/lib/am/check.am
@@ -148,9 +148,46 @@ echo "$$res: $$f (exit: $$estatus)" |			\
 cat $@-t >>$@;						\
 rm -f $@-t
 
+if am__MAKE_IS_GNU
+## FIXME: This is heineous.  There must be a better way to obtain what we
+## want from GNU make!  But still, this at least works for the moment.
+am__start = <--am--start--here-->
+am__end= <--am--end--here-->
+am__get_list_from_var = \
+  { echo 'am--nil:' \
+      && echo '.PHONY: am--nil' \
+## We use $(warning ...), not $(info ...), since the latter has only been
+## introduced in GNU make 3.81.
+      && echo '$$(warning $(am__start) $$('$$am__varname') $(am__end))'; } \
+  | $(MAKE) $(AM_MAKEFLAGS) -f %MAKEFILE% -f - am--nil 2>&1 \
+  | tr ' 	' '\012\012' | grep . \
+  | sed -n '/^$(am__start)$$/,/^$(am__end)$$/p' | sed -e '1d' -e '$$d'
+am__delete_files_in_var = \
+  $(am__get_list_from_var) | $(am__base_list) | ( \
+    st=0; \
+    while read files; do \
+      test -z "$$files" || rm -f $$files || st=1; \
+    done; \
+    exit $$st)
+am__get_test_logs_list = \
+  am__varname=TEST_LOGS && list=`$(am__get_list_from_var)` || exit 1
+else ! am__MAKE_IS_GNU
+am__get_test_logs_list = list=' $(TEST_LOGS) '
+endif
+
+# Trailing whitespace in "TESTS = foo.test $(empty)" causes at least
+# GNU make 3.80 to erroneously expand $(TESTS_LOGS) to "foo.log .log".
+# Work around this bug.
+if am__MAKE_IS_GNU
+TEST_LOGS = @am__dollar@(filter-out .log, $(am__test_logs))
+else
+TEST_LOGS = $(am__test_logs)
+endif
+TEST_LOGS_TMP = $(TEST_LOGS:.log=.log-t)
+
 $(TEST_SUITE_LOG): $(TEST_LOGS)
 	@$(am__sh_e_setup);						\
-	list='$(TEST_LOGS)';						\
+	$(am__get_test_logs_list);					\
 	results=`for f in $$list; do					\
 		   test -r $$f && read line < $$f && echo "$$line"	\
 		     || echo FAIL;					\
@@ -235,8 +272,11 @@ RECHECK_LOGS = $(TEST_LOGS)
 
 # Run all the tests.
 check-TESTS:
-## Expand $(RECHECK_LOGS) only once, to avoid exceeding line length limits.
+if am__MAKE_IS_GNU
+	@am__varname=RECHECK_LOGS && $(am__delete_files_in_var)
+else
 	@list='$(RECHECK_LOGS)'; test -z "$$list" || rm -f $$list
+endif
 ## We always have to remove TEST_SUITE_LOG, to ensure its rule is run
 ## in any case even in lazy mode: otherwise, if no test needs rerunning,
 ## or a prior run plus reruns all happen within the same timestamp
@@ -245,16 +285,15 @@ check-TESTS:
 ## OTOH, this means that, in the rule for `$(TEST_SUITE_LOG)', we
 ## cannot use `$?' to compute the set of lazily rerun tests, lest
 ## we rely on .PHONY to work portably.
-##
-## Trailing whitespace in `TESTS = foo.test $(empty)' causes GNU make
-## 3.80 to erroneously expand $(TESTS_LOGS) to `foo.log .log'.
-## Work around this bug.
 	@test -z "$(TEST_SUITE_LOG)" || rm -f $(TEST_SUITE_LOG)
-	@list='$(TEST_LOGS)';						\
-	list=`for f in $$list; do					\
-	  test .log = $$f || echo $$f;					\
-	done | tr '\012\015' '  '`;					\
-	$(MAKE) $(AM_MAKEFLAGS) $(TEST_SUITE_LOG) TEST_LOGS="$$list"
+if am__MAKE_IS_GNU
+## No need to pass on '$(TEST_LOGS)' explicitly.  GNU make will take
+## care of that automatically, even if the user has overridden $(TESTS)
+## or $(TEST_LOGS).
+	$(MAKE) $(AM_MAKEFLAGS) $(TEST_SUITE_LOG)
+else
+	$(MAKE) $(AM_MAKEFLAGS) $(TEST_SUITE_LOG) TEST_LOGS='$(TEST_LOGS)'
+endif
 
 AM_RECURSIVE_TARGETS += check
 
@@ -299,7 +338,7 @@ AM_RECURSIVE_TARGETS += check-html
 ## Rerun all FAILed or XPASSed tests.
 recheck recheck-html:
 	@target=`echo $@ | sed 's,^re,,'`;				\
-	list='$(TEST_LOGS)';						\
+	$(am__get_test_logs_list);					\
 	list=`for f in $$list; do					\
 	        test -f $$f || continue;				\
 	        if test -r $$f && read line < $$f; then			\
@@ -309,6 +348,8 @@ recheck recheck-html:
 ## This apparently useless munging helps to avoid a nasty bug (a
 ## segmentation fault!) on Solaris XPG4 make.
 	list=`echo "$$list" | sed 's/ *$$//'`;				\
+## FIXME: this might fail on systems with low command-line length limits
+## FIXME: in case there are many failed tests.  Is it worth worrying about?
 	$(MAKE) $(AM_MAKEFLAGS) $$target AM_MAKEFLAGS='$(AM_MAKEFLAGS) TEST_LOGS="'"$$list"'"'
 
 .PHONY: recheck recheck-html
@@ -316,6 +357,27 @@ recheck recheck-html:
 
 AM_RECURSIVE_TARGETS += recheck recheck-html
 
+## ------------- ##
+##  Cleaning up. ##
+## ------------- ##
+
+## FIXME: the support for variables with contents exceeding the
+## FIXME: command line length limit should be propagated to
+## FIXME: `lib/am/clean.am'.
+mostlyclean-am: am--parallel-tests-mostlyclean
+am--parallel-tests-mostlyclean:
+	@echo ' rm -f $$(TEST_LOGS)'
+if am__MAKE_IS_GNU
+	@am__varname=TEST_LOGS_TMP && $(am__delete_files_in_var)
+	@am__varname=TEST_LOGS && $(am__delete_files_in_var)
+else
+	@list='$(TEST_LOGS_TMP)'; test -z "$$list" || rm -f $$list
+	@list='$(TEST_LOGS)';     test -z "$$list" || rm -f $$list
+endif
+	@echo " rm -f $(TEST_SUITE_LOG) $(TEST_SUITE_HTML)"
+	@rm -f $(TEST_SUITE_LOG) $(TEST_SUITE_HTML)
+.PHONY: am--parallel-tests-mostlyclean
+
 else !%?PARALLEL_TESTS%
 
 check-TESTS: $(TESTS)
diff --git a/m4/init.m4 b/m4/init.m4
index 365c9ac..84ada72 100644
--- a/m4/init.m4
+++ b/m4/init.m4
@@ -83,6 +83,23 @@ AC_REQUIRE([AM_PROG_MKDIR_P])dnl
 AC_REQUIRE([AC_PROG_AWK])dnl
 AC_REQUIRE([AC_PROG_MAKE_SET])dnl
 AC_REQUIRE([AM_SET_LEADING_DOT])dnl
+dnl
+dnl FIXME: is it worth to extract this in a new macro of its own?
+am_make=${MAKE-make}
+AC_MSG_CHECKING([whether $am_make is GNU make])
+if ($am_make -n -f /dev/null --version) >/dev/null 2>&1; then
+  am_using_gmake=yes
+else
+  am_using_gmake=no
+fi
+AC_MSG_RESULT([$am_using_gmake])
+AM_CONDITIONAL([am__MAKE_IS_GNU], [test $am_using_gmake = yes])dnl
+dnl
+dnl Hack useful to write Makefile fragments that might otherwise
+dnl trigger spurious portability warning w.r.t. non-POSIX variable
+dnl names.
+AC_SUBST([am__dollar], [$])_AM_SUBST_NOTMAKE([am__dollar])dnl
+dnl
 _AM_IF_OPTION([tar-ustar], [_AM_PROG_TAR([ustar])],
 	      [_AM_IF_OPTION([tar-pax], [_AM_PROG_TAR([pax])],
 			     [_AM_PROG_TAR([v7])])])
diff --git a/tests/Makefile.am b/tests/Makefile.am
index e14d57e..27305d0 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -26,7 +26,6 @@ gcj6.test \
 java-nobase.test \
 pr8365-remake-timing.test \
 lex-subobj-nodep.test \
-parallel-tests-many.test \
 remake-am-pr10111.test \
 remake-m4-pr10111.test \
 vala-vpath.test \
diff --git a/tests/parallel-tests-many.test b/tests/parallel-tests-many.test
index ef01db1..c470745 100755
--- a/tests/parallel-tests-many.test
+++ b/tests/parallel-tests-many.test
@@ -16,9 +16,11 @@
 
 # Check that the parallel-tests harness does not hit errors due to
 # an exceeded command line length when there are many tests.
-# For automake bug#7868.  This test is currently expected to fail.
+# For automake bug#7868.  This test is currently expected to pass
+# only with GNU make.
 
 parallel_tests=yes
+required=GNUmake
 . ./defs || Exit 1
 
 set -e
-- 
1.7.7.3