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

Fri Sep 13 11:52:07 PDT 2013

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/

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
>>>>>>>>