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

[David Holmes]

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