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

[David Holmes]

On 14/09/2013 4:52 AM, Christian Tornqvist wrote:
> I've added getCompileJDKTool() and getTestJDKTool(), the common function getJDKTool() will still look in test.jdk first and compile.jdk as a fallback.
>
> Changed the TestVerifyDuringStartup.java that Igor changed as part of his change to use getJDKTool() since this is what  ProcessTools.createJavaProcessBuilder() does.
>
> Verified my change locally by running test/runtime and test/gc jtreg tests to verify that they work correctly when test.jdk is set to a JRE.
>
> New webrev can be found at:
> http://cr.openjdk.java.net/~ctornqvi/webrev/8014905/webrev.03/

Looks good to me!

Thanks,
Chris

> Thanks,
> Christian
>
> -----Original Message-----
> From: hotspot-runtime-dev-bounces at openjdk.java.net
> [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of David
> Holmes
> Sent: Friday, September 13, 2013 7:11 AM
> To: Erik Helin
> Cc: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(S): 8014905 - [TESTBUG] Some hotspot tests should be
> updated to divide test jdk and compile jdk
>
> Just to cc the list what I said in other email ...
>
> I think this is a good compromise. Having an API that allows the test to
> select which JDK to use is important; but maintaining compatability with
> existing test behaviour is also important.
>
> Thanks,
> David
>
> On 12/09/2013 11:22 PM, Erik Helin wrote:
>> On 2013-09-12, David Holmes wrote:
>>> 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.
>>
>> I, Igor and Christian discussed this on IM and the solution that we
>> came up with was that JDKToolFinder should have three methods:
>> getJDKTool, getCompilerJDKTool and getTestJDKTool. getJDKTool should,
>> as in Christian's change, first try to make use of getTestJDKTool and
>> then, if that fails, use getCompilerJDKTool.
>>
>> getCompilerJDKTool and getTestJDKTool should be like the ones Igor has
>> written, but the should use make use of a private function to avoid
>> code duplication.
>>
>> This way, writing an simple test will still be easy, using getJDKTool
>> should in most cases be what you want. However, if you are writing a
>> more tricky test, getTestJDKTool and getCompilerJDKTool are still
>> there for those cases.
>>
>> David, what do think about this suggestion?
>>
>>>>> 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/ceda33ff
>>>>>>>> 54a3
>>>>>>>
>>>>>>> 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 guess that I and Christian usually try to monitor the changes to the
>> hotspot testlibrary, and I think Igor will start do it as well :) This
>> way, we at least have one from runtime, one from compiler and one from
>> gc.
>>
>> As for keeping the JDK and HotSpot versions in sync, well that is a
>> problem. One of the issues is that just because we in the hotspot team
>> reviewed and approved changes to HotSpot's testlibrary does not
>> automatically mean that the JDK team wants these patches. Therefore,
>> keeping the JDK and the HotSpot test libraries in sync will require
>> more work than just "moving patches".
>>
>> Unfortunately, I do not have any good solution for this at the moment
>> :(
>>
>>>> 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.
>>
>> I think hotspot-dev is better for changes to the hotspot testlibrary
>> since the changes affect all the hotspot devevelopers.
>>
>> Thanks,
>> Erik
>>
>>> 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
>>>>>>>>>
>