RFR(S): 8014905 - [TESTBUG] Some hotspot tests should be updated to divide test jdk and compile jdk

[David Holmes]

On 12/09/2013 9:27 PM, Igor Ignatyev wrote:
> On 09/12/2013 02:13 PM, David Holmes wrote:
>> On 12/09/2013 8:07 PM, Igor Ignatyev wrote:
>>> On 09/12/2013 01:54 PM, David Holmes wrote:
>>>> On 12/09/2013 4:20 PM, Igor Ignatyev wrote:
>>>>> Christian,
>>>>>
>>>>> I have made some changes in JDKToolFinder w/ patch for JDK-8012447[*]:
>>>>>   - getJDKTool uses 'compile.jdk'
>>>>>   - getCurrentJDKTool uses 'test.jdk'
>>>>>
>>>>> So, I'm not sure that your change is necessary.
>>>>
>>>> I prefer Christian's approach as it is behaviour preserving. With your
>>>> change any test that actually wants/needs to use the test JDK will have
>>>> to be modified.
>>> I agree, but w/ Christian's approach we could get situation that we test
>>> binary from another JDK, e.g. 'test/gc/TestVerifyDuringStartup.java'
>>> which used 'java' from 'compile.jdk' instead of 'test.jdk'.
>>
>> I don't see how as the test JDK is checked first. The gap in Christian's
>> approach is the more rare case of where the test JDKs tool must not be
>> used for that part of the test.
> Oh sorry, I misread the code. Anyway, I don't think that this code is
> good, because if I want to get one of jdk tool, I expect that I will get
> it from '-compilejdk' if this option's specified, and from '-jdk',
> otherwise. But w/  Christian's patch I will get tool from '-jdk' if tool
> exists there, that may confuse.

Anyone using this function has to know what it returns regardless of 
whether that is from compile-jdk or test-jdk. With your change a bunch 
of tests that were using the test-jdk tools will now be using the 
compile-jdk tools, and while that might not cause a test failure it 
might negate the purpose of the test. So unless you've examined all the 
tests that use this API I can't agree that your change was appropriate.

>> So I think
>>> that any test that actually wants/needs to use the test JDK must do it
>>> explicitly.
>>>>
>>>>> [*]
>>>>> http://hg.openjdk.java.net/hsx/hotspot-comp/hotspot/rev/ceda33ff54a3
>>>>
>>>> Was this reviewed on hotspot-dev/hotspot-runtime-dev or only
>>>> hotspot-compiler-dev?
>>>
>>> It was reviewed only on hotspot-compiler-dev.
>>
>> These kinds of changes really have to be reviewed more broadly as they
>> affect everyone. We wouldn't even have known about your change if
>> Christian hadn't done this one. Plus the testlibrary really should have
>> a nominal owner to coordinate things - and we need to make sure the
>> hotspot and JDK versions are kept in sync.
> I fully agree w/ you and I'm really sorry about the situation.
> But I don't see big difference between 'hotspot-runtime-dev' and
> 'hotspot-compiler-dev', so *this* change "really have to be reviewed
> more broadly as they affect everyone".

I meant reviewing on hotspot-runtime-dev as well as 
hotspot-compiler-dev, not in place of. hotspot-dev could have been in place.

Cheers,
David

>> Thanks,
>> David
>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Best regards,
>>>>> Igor Ignatyev
>>>>>
>>>>> On 09/12/2013 06:38 AM, Christian Tornqvist wrote:
>>>>>> Hi everyone,
>>>>>>
>>>>>> Small change in JDKToolFinder so that it will now look in
>>>>>> compile.jdk if
>>>>>> the tool is not found in test.jdk. I’ve tested it locally by running
>>>>>> tests with test.jdk set to a JRE and compile.jdk set to JDK to see
>>>>>> that
>>>>>> they work correctly.
>>>>>>
>>>>>> Webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~ctornqvi/webrev/8014905/webrev.02/
>>>>>>
>>>>>> Bug is unfortunately not visible externally at this time L
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Christian
>>>>>>