jmx-dev RFR 8240604: Rewrite sun/management/jmxremote/bootstrap/CustomLauncherTest.java test to make binaries from source file

[Igor Ignatyev]

thanks! LGTM.

-- Igor

> On Mar 18, 2020, at 10:35 AM, Alexander Scherbatiy <alexander.scherbatiy at bell-sw.com> wrote:
> 
> On 18.03.2020 20:02, Igor Ignatyev wrote:
> 
>> +import static jdk.test.lib.Utils.TEST_CLASS_PATH;
>> I'm not a huge fun of 'import static', yet don't insist on removing it either. 
>> 
>> +            System.out.println("  libjvm    : " + jvmLibDir.toString());
>> jvmLibDir doesn't point to libjvm, so you need either update message prefix or use the actual value which will be used as path to libjvm. I personally prefer the latter. 
>> btw, you don't need to explicitly call toString in string concatenation.
>> 
>   Here is the updated fix where the static import is removed and libjvm path is used:
> 
>   http://cr.openjdk.java.net/~alexsch/8240604/webrev.03/ <http://cr.openjdk.java.net/~alexsch/8240604/webrev.03/>
> 
>   Thanks,
> 
>   Alexander.
> 
> 
> 
>> -- Igor
>> 
>>> On Mar 18, 2020, at 9:54 AM, Alexander Scherbatiy <alexander.scherbatiy at bell-sw.com <mailto:alexander.scherbatiy at bell-sw.com>> wrote:
>>> 
>>> On 18.03.2020 19:00, Igor Ignatyev wrote:
>>> 
>>>> Hi Alexander,
>>>> 
>>>>> I also included TEST_NATIVE_PATH to the Utils lib.
>>>> for the sake of clarity and ease of backporting, I'd prefer to have it added by a separate bug and commit.
>>> 
>>> Here is the updated fix where TEST_NATIVE_PATH is not added to the Utils lib.
>>> 
>>>   http://cr.openjdk.java.net/~alexsch/8240604/webrev.02/ <http://cr.openjdk.java.net/~alexsch/8240604/webrev.02/>
>>> 
>>> 
>>> Thanks,
>>> 
>>> Alexander.
>>> 
>>>>> Could I just use "hg remove binary-fie" and run webrev to add the removed binary files into webrev?
>>>> IIRC correctly, webrev will just say 'a binary file got removed', in any case I'll take it as a 'yes, I'm going to remove these files as part of 8240604', so thumbs up.
>>>> 
>>>> -- Igor
>>>>  
>>>>> On Mar 18, 2020, at 4:57 AM, Alexander Scherbatiy <alexander.scherbatiy at bell-sw.com <mailto:alexander.scherbatiy at bell-sw.com>> wrote:
>>>>> 
>>>>> Hello,
>>>>> 
>>>>> Could you review the updated fix:
>>>>> 
>>>>>   http://cr.openjdk.java.net/~alexsch/8240604/webrev.01 <http://cr.openjdk.java.net/~alexsch/8240604/webrev.01>
>>>>> 
>>>>> Utils.TEST_CLASS_PATH, Platform.jvmLibDir(), and /native flag are added to the CustomLauncherTest.java test. I also included TEST_NATIVE_PATH to the Utils lib.
>>>>> 
>>>>> I have not found a history about CustomLauncherTest.sh script in launcher.c so I just updated the comment as "A minature launcher for use by CustomLauncherTest.java test" in the exelauncher.c file.
>>>>> 
>>>>> 
>>>>> The comment that I had about removing the linux-* and solaris-* binary files I wrote because it is not clear for what is the right way to include removed binary files into webrev.
>>>>> 
>>>>> Could I just use "hg remove binary-fie" and run webrev to add the removed binary files into webrev?
>>>>> 
>>>>> 
>>>>> Thanks,
>>>>> 
>>>>> Alexander.
>>>>> 
>>>>> On 17.03.2020 20:11, Igor Ignatyev wrote:
>>>>>> Hi Alexander,
>>>>>> 
>>>>>> overall looks good to me, I have a few comments though:
>>>>>>  - you can use Utils.TEST_CLASSPATH instead of CustomLauncherTest.TEST_CLASSPATH
>>>>>> - CustomLauncherTest::findLibjvm can be simplified by use Platform::jvmLibDir
>>>>>> - exelauncher.c has a comment which refers to the test as CustomLauncherTest.sh, could you please update the comment?
>>>>>> - you have to add /native flag to @run action, otherwise jtreg won't exclude this test from runs w/ test.nativepath being unset
>>>>>> 
>>>>>> I also have a question regarding your statement that
>>>>>>>> The changes for obsolete binary files <...> are not included into the webrev. They needs to be removed manually.
>>>>>> you are planning to remove these files as part of this patch, right?
>>>>>> 
>>>>>> Thanks,
>>>>>> -- Igor
>>>>>> 
>>>>>> 
>>>>>>> On Mar 5, 2020, at 6:27 AM, Daniel Fuchs <daniel.fuchs at oracle.com <mailto:daniel.fuchs at oracle.com>> wrote:
>>>>>>> 
>>>>>>> Hi Alexander,
>>>>>>> 
>>>>>>> Fixes to JMX & management agent are reviewed on the
>>>>>>> seviceability-dev (added in to:) these days.
>>>>>>> 
>>>>>>> best regards,
>>>>>>> 
>>>>>>> -- daniel
>>>>>>> 
>>>>>>> On 05/03/2020 13:17, Alexander Scherbatiy wrote:
>>>>>>>> Hello,
>>>>>>>> Could you review a small enhancement where the test CustomLauncherTest is updated to build binary launcher file from launcher.c file.
>>>>>>>> The file launcher.c is renamed to exelauncher.c to follow the name convention for executable test files building by jdk make system.
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8240604 <https://bugs.openjdk.java.net/browse/JDK-8240604>
>>>>>>>> Webrev: http://cr.openjdk.java.net/~alexsch/8240604/webrev.00 <http://cr.openjdk.java.net/~alexsch/8240604/webrev.00>
>>>>>>>> The changes for obsolete binary files from sun/management/jmxremote/bootstrap/linux-* and solaris-* are not included into the webrev. They needs to be removed manually.
>>>>>>>> The test is passed on Ubuntu 18.04 x86-64, Solaris Sparc v9 11.2, and Solaris x64 11.4 systems.
>>>>>>>> The test is excluded from Windows and Mac Os X systems.
>>>>>>>> Thanks,
>>>>>>>> Alexander.
>> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200318/445f764a/attachment.htm>