bug#10437: bug#10427: bug#10437: parallel-tests: `recheck' recipe can cause sed to be invoked with too long input lines

[Stefano Lattarini]

On 01/05/2012 07:24 PM, Stefano Lattarini wrote:
> On 01/05/2012 07:06 PM, Paul Eggert wrote:
>> On 01/05/12 06:07, Stefano Lattarini wrote:
>>> Which "sort of thing" exactly?  I could find only one place which suffers
>>> of the problem you've pointed out, i.e., the `recheck recheck-html' rules
>>> in lib/am/check.am.  Am I missing something?
>>
>> Sorry, that appears to have been a miscount on my part:
>> I was counting some files that Automake generates for itself
>> while building.  In Automake source there are only two instances,
>> which your patch caught: the 'recheck recheck-html' rule and
>> the 'check-TESTS' rule (the latter is what actually triggered
>> the problem with coreutils).
>>
> Wait, the `check-TESTS' rules didn't use any sed invocation, so it wouldn't 
> make
> sense for it to trip for a sed failure ...  What am I missing?
>
I will answer myself: I was missing the fact that such a sed invocation had
been added to check-TESTS, *but in master only*.  Anyway, the maint -> master
merge will take care of everything.

Thanks, and sorry for the noise,
  Stefano

bug#10437: parallel-tests: `recheck' recipe can cause sed to be invoked with too long input lines

[Stefano Lattarini]

On 01/05/2012 07:06 PM, Paul Eggert wrote:
> On 01/05/12 06:07, Stefano Lattarini wrote:
>> Which "sort of thing" exactly?  I could find only one place which suffers
>> of the problem you've pointed out, i.e., the `recheck recheck-html' rules
>> in lib/am/check.am.  Am I missing something?
> 
> Sorry, that appears to have been a miscount on my part:
> I was counting some files that Automake generates for itself
> while building.  In Automake source there are only two instances,
> which your patch caught: the 'recheck recheck-html' rule and
> the 'check-TESTS' rule (the latter is what actually triggered
> the problem with coreutils).
>
Wait, the `check-TESTS' rules didn't use any sed invocation, so it wouldn't make
sense for it to trip for a sed failure ...  What am I missing?

Thanks,
  Stefano

bug#10437: parallel-tests: `recheck' recipe can cause sed to be invoked with too long input lines

[Paul Eggert]

On 01/05/12 06:07, Stefano Lattarini wrote:
> Which "sort of thing" exactly?  I could find only one place which suffers
> of the problem you've pointed out, i.e., the `recheck recheck-html' rules
> in lib/am/check.am.  Am I missing something?

Sorry, that appears to have been a miscount on my part:
I was counting some files that Automake generates for itself
while building.  In Automake source there are only two instances,
which your patch caught: the 'recheck recheck-html' rule and
the 'check-TESTS' rule (the latter is what actually triggered
the problem with coreutils).  So this should be OK.

Thanks for the quick fixes, by the way!

bug#10437: parallel-tests: `recheck' recipe can cause sed to be invoked with too long input lines

[Stefano Lattarini]

Reference:
  

On 01/05/2012 03:07 PM, Stefano Lattarini wrote:
>
> Patch coming up soon.
> 
And here it is.  I will push by this evening if there is no objection.

Regards,
  Stefano
>From e3b0e12400f5fa4220fc0aa79dd0989e56def9c6 Mon Sep 17 00:00:00 2001
Message-Id: 
From: Stefano Lattarini 
Date: Thu, 5 Jan 2012 15:13:30 +0100
Subject: [PATCH] parallel-tests: avoid issue with overly long lines in sed
 input

See automake bug#10437:
  
and coreutils bug#10427:
  

* lib/am/check.am (recheck, recheck-html): In order to strip
trailing whitespace from the definition of the `$list' variable,
we used to invoke sed in a way that could cause it to get passed
overly long input lines, causing spurious failures.  So rework
the logic of the recipe to avoid any sed invocation, relying on
simpler shell idioms instead.
(check-TESTS): Reorganize the recipe to be more similar to the
one of `recheck', for consistency and simplicity.
* NEWS: Update.

Report and analysis by Paul Eggert.
---
 NEWS|4 
 lib/am/check.am |   38 +-
 2 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/NEWS b/NEWS
index 2e572e4..7e52d83 100644
--- a/NEWS
+++ b/NEWS
@@ -89,6 +89,10 @@ Bugs fixed in 1.11.0a:
 
 * Bugs introduced by 1.11:
 
+  - The parallel-tests harness doesn't trip anymore on sed implementations
+with stricter limits on the length of input lines (problem seen at
+least on Solaris 8).
+
   - 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/lib/am/check.am b/lib/am/check.am
index 3d0188d..29faa38 100644
--- a/lib/am/check.am
+++ b/lib/am/check.am
@@ -245,15 +245,16 @@ 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.
-##
+	@test -z "$(TEST_SUITE_LOG)" || rm -f $(TEST_SUITE_LOG)
+	@list='' list2='$(TEST_LOGS)'; for f in $$list2; do \
 ## 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' '  '`;	\
+	  test .log = $$f && continue; \
+## Be careful to avoid extra whitespace in the definition of $list.  See
+## comments in `recheck' below for why this might be useful.
+	  if test -z "$$list"; then list=$$f; else list="$$list $$f"; fi; \
+	done; \
 	$(MAKE) $(AM_MAKEFLAGS) $(TEST_SUITE_LOG) TEST_LOGS="$$list"
 
 AM_RECURSIVE_TARGETS += check
@@ -298,17 +299,20 @@ AM_RECURSIVE_TARGETS += check-html
 
 ## Rerun all FAILed or XPASSed tests.
 recheck recheck-html:
-	@target=`echo $@ | sed 's,^re,,'`;\
-	list='$(TEST_LOGS)';		\
-	list=`for f in $$list; do	\
-	test -f $$f || continue;\
-	if test -r $$f && read line < $$f; then			\
-	  case $$line in FAIL*|XPASS*) echo $$f;; esac;		\
-	else echo $$f; fi;	\
-	  done | tr '\012\015' '  '`;\
-## This apparently useless munging helps to avoid a nasty bug (a
-## segmentation fault!) on Solaris XPG4 make.
-	list=`echo "$$list" | sed 's/ *$$//'`;\
+	@target=`echo $@ | sed 's,^re,,'`; \
+	list='' list2='$(TEST_LOGS)'; for f in $$list2; do \
+	  test -f $$f || continue; \
+	  if test -r $$f && read line < $$f; then \
+	case $$line in FAIL*|XPASS*) : ;; *) continue;; esac; \
+	  fi; \
+## Be careful to avoid extra whitespace in the definition of $list, since
+## its value will be passed to the recursive make invocation below through
+## the TEST_LOGS macro, and leading/trailing white space in a make macro
+## definition can be problematic.  In this particular case, trailing white
+## space was known to cause a segmentation fault on Solaris 10 XPG4 make:
+## 
+	  if test -z "$$list"; then list=$$f; else list="$$list $$f"; fi; \
+	done; \
 	$(MAKE) $(AM_MAKEFLAGS) $$target AM_MAKEFLAGS='$(AM_MAKEFLAGS) TEST_LOGS="'"$$list"'"'
 
 .PHONY: recheck recheck-html
-- 
1.7.7.3

bug#10437: parallel-tests: `recheck' recipe can cause sed to be invoked with too long input lines (was: Re: bug#10427: coreutils-8.14.116-1e18d: testsuite failures on NetBSD 5.1)

[Stefano Lattarini]

[adding bug-automake in CC:]

Reference: 

Hi Paul, thanks for the report and diagnosis.

On 01/05/2012 10:00 AM, Paul Eggert wrote:
> The latest coreutils snapshot fail to build
> 
>> On 01/03/2012 06:10 PM, Jim Meyering wrote:
>>> FYI, here's a snapshot of what will soon be coreutils-8.15,
>>> expected on Thursday or Friday.
>>>
>>> coreutils snapshot:
>>>   http://meyering.net/cu/coreutils-ss.tar.xz  5.2 MB
>>>   http://meyering.net/cu/coreutils-ss.tar.xz.sig
>>>   http://meyering.net/cu/coreutils-8.14.116-1e18d.tar.xz
> 
> This snapshot doesn't build on Solaris 8 (sparc) with native tools,
> for a couple of reasons.  I don't expect Solaris 8 is an active
> porting target any more, but these problems could well happen on
> active targets.
>
I agree.

> Second, there's code like this in tests/Makefile.in:
> 
>   @list='$(TEST_LOGS)'; \
>   list=`for i in $$list; do \
> test .log = $$i || echo $$i; \
>   done | tr '\012\015' '  '`; \
>   list=`echo "$$list" | sed 's/ *$$//'`; \
> 
> This generates a long line and sends it to 'sed',
> which complains "Output line too long." and outputs nothing.
>
And if I'm not mistaken, sed is allowed such a behaviour by POSIX, so this
is a portability problem in automake.

> This code is also generated by Automake.  How about changing Automake
> to generate something like this instead?
> 
>   @test_logs='$(TEST_LOGS)'; \
>   list=; \
>   for i in $$test_logs; do \
> test .log = "$$i" || list="$$list $$i"; \
>   done; \
> 
> This avoids the business with echo and tr and ` sed and
> avoids the sed limitation with long lines.
>
Good idea.  I will followed your idea (with some tweaks).
Patch coming up soon.

> Automake does this latter sort of thing in about 4 places,
>
Which "sort of thing" exactly?  I could find only one place which suffers
of the problem you've pointed out, i.e., the `recheck recheck-html' rules
in lib/am/check.am.  Am I missing something?

> and I figure it's done that way for a reason, but I don't
> know what the reason is.
> 
The comments in lib/am/check.am should be explicative enough.  if not,
that's a (minor) bug, so feel free to report it!

Thanks,
  Stefano