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

[Mikael Gerdin]

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