Reviewer needed - fix for IcedTea bug#637

[Dr Andrew John Hughes]

On 23:09 Tue 08 Mar     , Mark Wielaard wrote:
> Hi Andrew,
> 
> On Tue, 2011-03-08 at 20:58 +0000, Dr Andrew John Hughes wrote:
> > Given this new target kills off jtreg processes, I don't see how you'd
> > run it to get a current summary.  Neither would such a target be
> > a dependency of jtregcheck as it's never fulfilled.
> > 
> > What you're thinking of is an independent target where the whole thing is
> > just:
> > 
> > jtregcheck-summary:
> > 	for i in hotspot langtools jdk; do \
> > 	  if [ -e test/check-$$i.log ] ; then
> > 	    echo "--------------- jtreg console summary for $$i ---------------"; \
> > 	    egrep -v '^(Passed:|Directory|Re[a-z]+\ written\ to)' test/check-$$i.log; \
> > 	  fi
> > 	done | tee test/jtreg-summary.log
> > 
> > and which nothing depends on.  While the tests are running, make jtregcheck-summary
> > would be called independently and print the current status.  That's a nice
> > idea, but not what this patch is doing.
> 
> No, it has indeed two functions. It does the summary and finishes any
> stray processes from the jtregcheck target(s). I guess you didn't like
> my suggestion for the name, and Pavel's orginal name jtregcheck-finish
> was the better one. I don't think the functionality should be split. But
> they can be if you want.
> 

I think your name is preferable.  I just don't see the point in
splitting this off at all in the way that has been suggested.  All it
does is break things.

> > The current jtregcheck target runs after all required checks have been
> > run, prints a summary and kills off any processes floating around.
> 
> That is what jtregcheck-summary does to. It is the last dependent target
> that gets run after all requested tests have been run. It is just that
> the target has been split to have the last step independently.
> 

For absolutely no reason as far as I can see.  What makes you think it
will run last?  I'm not aware of any rule that says that and didn't
see anything in checking the GNU make manual.  This _will_ break -j
as there is nothing to stop the summary being rule.  At present, the
dependents are all fulfilled then the body of the rule is run.  This
breaks that for no reason.

> > This change breaks this ordering for no apparent gain.  We now have
> > a pointless jtregcheck with no body and a jtregcheck-summary which
> > can run at any time and start killing processes.
> 
> Maybe I am missing how you think the ordering is broken. make check (or
> make jtregcheck) still does the same thing. It does require make check
> -k now though (since the other targets can fail). Is that what you are
> objecting to?
> 

See above.

> Cheers,
> 
> Mark
> 

-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37