Review request: JDK-8006753 fix failed for JDK-8002415 White box testing API for HotSpot

[Vladimir Kozlov]

Thank you for confirming it. It is good for push.

Thanks,
Vladimir

On 2/19/13 11:52 AM, Mikael Gerdin wrote:
> Vladimir,
>
> On 2013-02-19 20:35, Vladimir Kozlov wrote:
>> Changes are good for push.
>>
>> I only worried about the first issue but I reproduced your case with
>> next instruction:
>>
>> $ hg rename src/share/tools/whitebox test/testlibrary/whitebox
>> moving src/share/tools/whitebox/sun/hotspot/WhiteBox.java to
>> test/testlibrary/whitebox/sun/hotspot/WhiteBox.java
>> moving
>> src/share/tools/whitebox/sun/hotspot/parser/DiagnosticCommand.java to
>> test/testlibrary/whitebox/sun/hotspot/parser/DiagnosticCommand.java
>>
>> Did your 'hg st' shows the next?:
>>
>> $ hg st
>> A test/testlibrary/whitebox/sun/hotspot/WhiteBox.java
>> A test/testlibrary/whitebox/sun/hotspot/parser/DiagnosticCommand.java
>> R src/share/tools/whitebox/sun/hotspot/WhiteBox.java
>> R src/share/tools/whitebox/sun/hotspot/parser/DiagnosticCommand.java
>
> Yes, hg doesn't seem to be very good at describing moved files.
> But if you run "hg diff --git" then it should tell you about the moved
> file.
>
> /Mikael
>
>>
>> Thanks,
>> Vladimir
>>
>> On 2/19/13 11:21 AM, Mikael Gerdin wrote:
>>> Vladimir,
>>>
>>> On 2013-02-19 20:18, Vladimir Kozlov wrote:
>>>> Mikael,
>>>>
>>>> Did you use 'hg rename' to move WhiteBox.java and
>>>> DiagnosticCommand.java? Or you just copied it? I don't see in
>>>> hsx-rt.patch that they were removed from src/share/tools/whitebox.
>>>
>>> Yes, if you look at the webrev html it says
>>> " test/testlibrary/whitebox/sun/hotspot/WhiteBox.java (was
>>> src/share/tools/whitebox/sun/hotspot/WhiteBox.java"
>>> but you are correct in that the hsx-rt.patch file does not contain that
>>> information, but the changeset in my local repository does have that
>>> information. I believe this is because webrev does not create a "real"
>>> hg patch with all the releavant hg metadata.
>>>
>>>>
>>>> I noticed that changeset's Summary still reference ClassFileDumper:
>>>
>>> Oops, will fix that.
>>>
>>>>
>>>> Summary: Modify WhiteBoxAPI to use interface classes from
>>>> test/testlibrary instead, add ClassFileDumper to resolve the boot class
>>>> path issue
>>>
>>> Given that fix, are you ok with me pushing this?
>>>
>>> /Mikael
>>>
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 2/19/13 10:15 AM, Mikael Gerdin wrote:
>>>>> Vladimir, all
>>>>>
>>>>> I've merged the recent whitebox test additions into my change and
>>>>> added
>>>>> the missing copyright headers.
>>>>> Here's the latest and greatest webrev:
>>>>> http://cr.openjdk.java.net/~mgerdin/8006753/webrev.2/
>>>>>
>>>>> I've also ran these tests with jprt -retest to see that the new
>>>>> variant
>>>>> works on different platforms.
>>>>>
>>>>> When doing the change to PrintNMTStatistics.java I discovered that the
>>>>> test was in fact only compiled with javac and not run, and in fact
>>>>> needed further updates to work properly. I filed a bug for that test
>>>>> and
>>>>> updated it to compile the java sources using the new scheme.
>>>>>
>>>>> /Mikael
>>>>>
>>>>> On 02/06/2013 07:55 PM, Vladimir Kozlov wrote:
>>>>>> On 2/6/13 10:16 AM, Mikael Gerdin wrote:
>>>>>>> Vladimir,
>>>>>>>
>>>>>>> On 2013-02-06 17:30, Vladimir Kozlov wrote:
>>>>>>>> Mikael,
>>>>>>>>
>>>>>>>> I like that all WB functionality is moved to test directory.
>>>>>>>>
>>>>>>>> You missed Copyright header in all new files.
>>>>>>>
>>>>>>> Thanks for the reminder, I'm not used to creating new files :)
>>>>>>
>>>>>> I still don't see it in DiagnosticCommand.java. Also old files
>>>>>> WBApi.java and ParserTest.java do not have it.
>>>>>>
>>>>>> Mikael, we need Copyright header in all files we push into open
>>>>>> sources.
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Could you rename ClassFileDumper to ClassFileInstaller? It will be
>>>>>>>> more
>>>>>>>> clear what it does.
>>>>>>>
>>>>>>> Done. Updated webrev at:
>>>>>>> http://cr.openjdk.java.net/~mgerdin/8006753/webrev.1/
>>>>>>>
>>>>>>> /Mikael
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>> On 2/4/13 5:59 AM, Mikael Gerdin wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Background:
>>>>>>>>> Even from the beginning of the WhiteBox testing API implementation
>>>>>>>>> the
>>>>>>>>> tests have not worked on builds promoted by release engineering
>>>>>>>>> due to
>>>>>>>>> the fact that the wb.jar file containing the Java classes was not
>>>>>>>>> imported by the JDK-level makefiles.
>>>>>>>>> On one level this was ok since we didn't want to ship the
>>>>>>>>> classes to
>>>>>>>>> end
>>>>>>>>> users and we worked around the complexity of somehow
>>>>>>>>> post-processing
>>>>>>>>> the
>>>>>>>>> RE bundles to remove them.
>>>>>>>>>
>>>>>>>>> The problem is that we aren't able to run the WhiteBox tests in
>>>>>>>>> promotion testing, or to verify fixes against earlier builds.
>>>>>>>>>
>>>>>>>>> Solution:
>>>>>>>>> The solution suggested here is to put the WhiteBox helper
>>>>>>>>> classes in
>>>>>>>>> the
>>>>>>>>> testlibrary directory under test/ and
>>>>>>>>> * tell jtreg to first compile the classes
>>>>>>>>> * run a special Java program to copy the classes to the test work
>>>>>>>>> directory
>>>>>>>>> * run the white box test with -Xbootclasspath/a:. to pick up the
>>>>>>>>> copied
>>>>>>>>> classes.
>>>>>>>>>
>>>>>>>>> The solution is kind-of hackish but I've been unable to come up
>>>>>>>>> with a
>>>>>>>>> better solution without performing complex surgery on jtreg.
>>>>>>>>>
>>>>>>>>> The changes to the make/ directory should basically revert the
>>>>>>>>> makefile
>>>>>>>>> changes that were added to support this API.
>>>>>>>>>
>>>>>>>>> Webrev:
>>>>>>>>> http://cr.openjdk.java.net/~mgerdin/8006753/webrev.0/
>>>>>>>>> Bug:
>>>>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8006753
>>>>>>>>>
>>>>>>>>> Note about renamed/moved files:
>>>>>>>>> I will try to make sure that the moved/renamed files will be
>>>>>>>>> tracked
>>>>>>>>> properly by mercurial, but I think that using patch files and MQ
>>>>>>>>> breaks
>>>>>>>>> this so I'll do this after the review is done.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> /Mikael
>>>>>