Re: bug#16302: 1.14.1: check-TESTS is not lazy enough

tags 16302 + patch
severity 16302 minor
stop

On 12/30/2013 03:48 PM, Akim Demaille wrote:
> Hi all,
>
Hi Akim.

> I have this piece of software with several APIs, organized in clear
> layers.  Building the whole package is costly, especially because of
> the top-level layers (dozens of binaries), and the whole test suite
> is even costlier (because it requires to build the whole set of binaries,
> and then it runs all the tests, including the costly top-level tests).
> 
> So, « of course », when developing this piece of software, I seldom run
> the full test-suite, and run selected bits.  In particular I avoid
> « make all », so, of course, I also avoid « make check » since it depends
> on the former.  Rather I use check-TESTS and deal with the dependencies
> myself, like a grownup.
> 
> Alas, for some reason, while it works well to generate individual test logs,
> but when generating (the partial) test-suite.log, « make all-am » is
> invoked.
> 
> Please try to attached tarball.  It features a lazy test suite with two
> test scripts.
> 
>> AM_TESTS_ENVIRONMENT = export PATH=$(abs_top_builddir):$PATH;
>> RECHECK_LOGS =
>> TESTS = fast slow
> 
> « slow » runs a binary built by the package, so
> of course, its log depends on this binary:
> 
>> slow.log: foo
> 
> but ‘fast’ depends on nothing, so to run it and only it, I have
> this piece of syntactic sugar: 
> 
>> check-fast:
>> 	$(MAKE) $(AM_MAKEFLAGS) check-TESTS TESTS='fast'
> 
> As expected ‘make check’ is lazy.  However, if I touch
> fast and foo.c (the dependency of slow), then observe:
> 
>> $ touch ~/src/gnu/test-suite/{foo.c,fast}
>> $ make check-fast
>> /usr/bin/make  check-TESTS TESTS='fast'
>> PASS: fast
>> depbase=`echo foo.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
>> 	ccache gcc-mp-4.8 -DPACKAGE_NAME=\"test-suite\" -DPACKAGE_TARNAME=\"test-suite\" -DPACKAGE_VERSION=\"1.0\" -DPACKAGE_STRING=\"test-suite\ 1.0\" -DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"test-suite\" -DVERSION=\"1.0\" -I. -I/Users/akim/src/gnu/test-suite   -DNDEBUG -isystem /opt/local/include  -O3 -MT foo.o -MD -MP -MF $depbase.Tpo -c -o foo.o /Users/akim/src/gnu/test-suite/foo.c &&\
>> 	mv -f $depbase.Tpo $depbase.Po
>> ccache gcc-mp-4.8  -O3  -L/opt/local/lib -o foo foo.o  
>> ============================================================================
>> Testsuite summary for test-suite 1.0
>> ============================================================================
>> # TOTAL: 1
>> # PASS:  1
>> # SKIP:  0
>> # XFAIL: 0
>> # FAIL:  0
>> # XPASS: 0
>> # ERROR: 0
>> ============================================================================
> 
> As you can see, « fast.log » was recreated appropriately, but then,
> to build test-suite.log, something fires « all-am » (running make
> with -d shows this very well).  It appears to be due to this:
> 
>> $(TEST_SUITE_LOG): $(TEST_LOGS)
>> 	@$(am__set_TESTS_bases); \
>> 	am__f_ok () { test -f "$$1" && test -r "$$1"; }; \
>> 	redo_bases=`for i in $$bases; do \
>> 	              am__f_ok $$i.trs && am__f_ok $$i.log || echo $$i; \
>> 	            done`; \
>> 	if test -n "$$redo_bases"; then \
>> 	  redo_logs=`for i in $$redo_bases; do echo $$i.log; done`; \
>> 	  redo_results=`for i in $$redo_bases; do echo $$i.trs; done`; \
>> 	  if $(am__make_dryrun); then :; else \
>> 	    rm -f $$redo_logs && rm -f $$redo_results || exit 1; \
>> 	  fi; \
>> 	fi; \
>> 	if test -n "$$am__remaking_logs"; then \
>> 	  echo "fatal: making $(TEST_SUITE_LOG): possible infinite" \
>> 	       "recursion detected" >&2; \
>> 	else \
>> 	  am__remaking_logs=yes $(MAKE) $(AM_MAKEFLAGS) $$redo_logs; \
>> 	fi; \
> 
> in this last bit, « $$redo_logs » is empty, so, blam, a full cost
> make all.  What was the point of this piece of code?
>
See the '##'-style comments in lib/am/check.am for detailed explanations
(such comments are stripped from the generated Makefiles, as I'm sure
you know).

> At first sight it seems that it should be guarder by ‘test -n $$redo_log’.
>
Indeed.

> This is *really* costly, IÂ’d be happy to have nice workarounds.
>
Or eve better, to fix the bug :-)  See attached patch.  Does it work
for you?

> Thanks, and happy new year!
> 

Thanks,
  Stefano

From ad9804e1b825ddb304bfaf62b7108f2b6e08dc81 Mon Sep 17 00:00:00 2001
Message-Id: <ad9804e1b825ddb304bfaf62b7108f2b6e08dc81.1388444976.git.stefano.lattarini@gmail.com>
From: Stefano Lattarini <[email protected]>
Date: Mon, 30 Dec 2013 23:21:03 +0100
Subject: [PATCH] parallel-tests: avoid possible implicit "make all" in
 test-suite.log rule

This change fixes automake bug#16302.

* lib/am/check.am ($(TEST_SUITE_LOG)): Avoid running "make $redo_logs"
when $redo_logs expands to empty, since in that case we are actually
ending up invoking a full "make all".  That shouldn't be required, and
can cause slowdowns for people implementing their extra "laziness
wrappers" around check-TESTS (automake bug#16302).
* NEWS: Update.

Signed-off-by: Stefano Lattarini <[email protected]>
---
 NEWS            | 4 ++++
 lib/am/check.am | 4 +++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index dd2812a..a73804a 100644
--- a/NEWS
+++ b/NEWS
@@ -93,6 +93,10 @@ New in 1.15:
     implementation of the TAP driver (that is documented in the manual)
     is more portable and has feature parity with the perl implementation.
 
+  - The rule generating 'test-suite.log' no longer risk incurring in an
+    extra useless "make all" recursive invocation in some corner cases
+    (automake bug#16302).
+
 * Bug fixes:
 
   - The user can now extend the special .PRECIOUS target, the same way
diff --git a/lib/am/check.am b/lib/am/check.am
index 7012d5a..4d2a498 100644
--- a/lib/am/check.am
+++ b/lib/am/check.am
@@ -274,7 +274,9 @@ $(TEST_SUITE_LOG): $(TEST_LOGS)
 	if test -n "$$am__remaking_logs"; then \
 	  echo "fatal: making $(TEST_SUITE_LOG): possible infinite" \
 	       "recursion detected" >&2; \
-	else \
+## Invoking this unconditionally could cause a useless "make all" to
+## be invoked when '$redo_logs' expands to empty (automake bug#16302).
+	elif test -n "$$redo_logs"; then \
 	  am__remaking_logs=yes $(MAKE) $(AM_MAKEFLAGS) $$redo_logs; \
 	fi; \
 	if $(am__make_dryrun); then :; else \
-- 
1.8.5.rc0.335.g7794a68