Reviewer needed - fix for IcedTea bug#637

[Mark Wielaard]

Hi Pavel,

On Thu, 2011-03-03 at 17:08 +0100, Pavel Tisnovsky wrote:
> I'd like to push following change to IcedTea6 HEAD. This change fixes
> the IcetTea6 bug #637:
> http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=637
> 
> How the fix works:
> 
> - after one regtest suite (hotspot, langtools or jdk) is finished, make
> check (using grep) if there are any test errors and/or failures.
> 
> - if the user runs only selected test suite (by 'make check-hotspot' for
> example), this test ensures that make will fail
> 
> - if the user runs all three test suites (by 'make check -k' where -k is
> mandatory), ALL THREE test suites are run, even if first or second one
> fail. File jtreg-summary.txt is always generated in this case, because
> the step for creating it is now included in new target
> 'finish-jtregcheck' and not in the 'common' target 'jtregcheck'.

I like the setup. Some minor nitpicks below. Also please make sure to
add a little note about make check vs make check -k to the NEWS file
and/or README.

> hg diff generated against recent IcedTea6 HEAD is stored in attachment.

> Addition to Changelog:
> 
> 2011-03-03  Pavel Tisnovsky  <ptisnovs at redhat.com>
> 
>         * Makefile.am:
>         Added checking of JTreg results - if at least one
>         regression test failed, make exits with error code

Make sure to mention PR637 here and in the commit log.
Would be nice to explicitly mention the targets names you changed/added/

> diff -r 0df8f7938769 Makefile.am
> --- a/Makefile.am	Thu Mar 03 15:32:46 2011 +0100
> +++ b/Makefile.am	Thu Mar 03 16:59:31 2011 +0100
> @@ -2069,6 +2069,7 @@
>  		$(ICEDTEA_JTREG_OPTIONS) \
>  		`pwd`/openjdk/hotspot/test \
>  	    | tee test/$@.log
> +	test -z `grep "^Error:\|^FAILED:" test/check-hotspot.log`

Since above the log file is created through the usage of $@.log please
use that in the test also. $@ expands to the target name (check-hotspot
in this case).

>  check-langtools: stamps/jtreg.stamp
>  	mkdir -p test/langtools/JTwork test/langtools/JTreport
> @@ -2079,6 +2080,7 @@
>  		$(ICEDTEA_JTREG_OPTIONS) \
>  		`pwd`/openjdk/langtools/test \
>  	    | tee test/$@.log
> +	test -z `grep "^Error:\|^FAILED:" test/check-langtools.log`

Likewise.
 
>  check-jdk: stamps/jtreg.stamp
>  	mkdir -p test/jdk/JTwork test/jdk/JTreport
> @@ -2095,6 +2097,7 @@
>  		$(ICEDTEA_JTREG_OPTIONS) \
>  		`pwd`/openjdk/jdk/test \
>  	    | tee test/$@.log
> +	test -z `grep "^Error:\|^FAILED:" test/check-jdk.log`

Likewise.

>  clean-jtreg-reports:
>  	rm -rf test/hotspot test/langtools test/jdk
> @@ -2107,7 +2110,9 @@
>  jtreg_pids = ps x --no-headers -ww -o pid,ppid,args \
>  	| awk '$$2 == 1 && $$3 ~ /^$(subst /,\/,$(CURDIR)/$(sdkimg))/ {print $$1}'
>  
> -jtregcheck: jtreg check-hotspot check-langtools check-jdk
> +jtregcheck: jtreg check-hotspot check-langtools check-jdk finish-jtregcheck
> +
> +finish-jtregcheck:
>  	for i in hotspot langtools jdk; do \
>  	  echo "--------------- jtreg console summary for $$i ---------------"; \
>  	  egrep -v '^(Passed:|Directory|Re[a-z]+\ written\ to)' test/check-$$i.log; \

Maybe a nicer target name would be jtregcheck-summary.

You have to handle the case that the check-$$i.log files might not exist
(since you cannot depend on them). Either just touch them (easiest
solution) or explicitly look whether they exist inside the for loop and
not output a summary line for them if not.

Also since this in a way a fake target (you want it to be executed
always and it doesn't create the target as file) you should add it as
a .PHONY target dependency below (see the chapter on Phony Targets in
the gnu make manual).

Cheers,

Mark