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

[David Holmes]

On 16/09/2013 11:41 AM, David Holmes wrote:
> 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

That should be:

Thanks, Chris.

David
-----

David
-----

:)

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