Request for Review: Make the Queens test ("test in build") an option that can be disabled

[David Holmes]

Magnus,

This still looks okay to me. My main annoyance is the proliferation of 
the condition check and the duplication. I know this has taken far too 
much time already but it seems to me that we should be able to factor:

+ ifeq ($(TEST_IN_BUILD),true)
           cd $(OSNAME)_$(BUILDARCH)_compiler2/$@ && ./test_gamma
+ endif

into a function so that we do:

$(eval $(call do-test-gamma, dir)

where we pass in the target specific directory. And of course the 
function does nothing if TEST_IN_BUILD is not set.

Just a thought.

David

On 14/09/2012 11:39 PM, Magnus Ihse Bursie wrote:
> I'd like to make a new try at integrating this fix. :-)
>
> I posted two a webrev back in May. After some feedback, I posted a
> second one at
> http://cr.openjdk.java.net/~ihse/make-test-in-build-an-option/webrev.01.
> This second webrev was reviewed and accepted by David Holmes, Dmitry
> Samersoff, Coleen Phillimore and Vladimir Kozlov. However, Kelly O'Hair
> expressed concern that it would not work on Windows due to changes in
> make/defs.make being problematic for Windows nmake. Also, in private
> conversation with me, Kelly found build problems on MacOSX.
>
> I didn't have time at the moment to address those issues, so I dropped
> it for the time being.
>
> However, I now checked more carefully. I'm not using any syntax in
> defs.make that is not already there, so I shouldn't introduce something
> Windows can't handle.
>
> The patch did have a problem though -- the TEST_IN_BUILD argument was
> not properly propagated. So I have made a new patch (third time's a
> charm!), and the webrev is here:
> http://cr.openjdk.java.net/~ihse/make-test-in-build-an-option/webrev.02/
>
> The only difference to the previous one is that I add
> TEST_IN_BUILD=$(TEST_IN_BUILD) to the BUILDTREE_VARS. (But it needs to
> be done three times, once for each duplicated platform file...)
>
> I am currently running a series of tests on all platforms, both using
> the old build system and the new. Not all tests have finished running,
> but it's looking good so far and I believe there should be no more
> problems. (I'll let you know otherwise! :))
>
> It feels like what should be a simple fix has grown somewhat out of
> proportion. I'm hoping I can get some final reviews on this, and some
> advice as to wether this fix should be integrated through the hotspot or
> the build forest. (On one hand, it's hotspot only, but on the other
> hand, it's just affecting build.)
>
> /Magnus
>
>
> On 2012-05-14 15:05, Magnus Ihse Bursie wrote:
>> As part of the new build system created in the build-infra project, we
>> want to make it a configurable option wether or not to run the Queens
>> test as part of the build.
>>
>> Here is a patch that introduces a new make variable, TEST_IN_BUILD,
>> which controls wether to run the Queens test (test_gamma.sh) or not.
>> If the variable is not explicitely set, it will default to true,
>> mening that the default behaviour will be as before, that is, to run
>> the Queens test. However, if you (or configure) explicitely set it to
>> false, the Queens test will be skipped.
>>
>> http://cr.openjdk.java.net/~ihse/make-test-in-build-an-option/webrev.00
>>
>> /Magnus
>>
>
>