[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: jmx-dev RFR 8240604: Rewrite sun/management/jmxremote/bootstrap/CustomLauncherTest.java test to
From: Igor Ignatyev <igor.ignatyev () oracle ! com>
Date: 2020-03-18 18:00:32
Message-ID: 40924AA7-C53C-4F56-8CE4-25672C58540C () oracle ! com
[Download RAW message or body]
thanks! LGTM.
-- Igor
> On Mar 18, 2020, at 10:35 AM, Alexander Scherbatiy \
> <alexander.scherbatiy@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@bell-sw.com <mailto:alexander.scherbatiy@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@bell-sw.com \
> > > > > <mailto:alexander.scherbatiy@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@oracle.com \
> > > > > > > <mailto:daniel.fuchs@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.
> >
[Attachment #3 (unknown)]
<html><head><meta http-equiv="Content-Type" content="text/html; \
charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: \
space; line-break: after-white-space;" class="">thanks! LGTM.<div class=""><br \
class=""></div><div class="">-- Igor<br class=""><div class=""><div class=""><div><br \
class=""><blockquote type="cite" class=""><div class="">On Mar 18, 2020, at 10:35 AM, \
Alexander Scherbatiy <<a href="mailto:alexander.scherbatiy@bell-sw.com" \
class="">alexander.scherbatiy@bell-sw.com</a>> wrote:</div><br \
class="Apple-interchange-newline"><div class="">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252" class="">
<div class=""><p class="">On 18.03.2020 20:02, Igor Ignatyev wrote:<br class="">
</p>
<blockquote type="cite" \
cite="mid:BC07CAFB-0C57-4932-8818-08113A10856C@oracle.com" class=""> <meta \
http-equiv="Content-Type" content="text/html; charset=windows-1252" class="">
<div class="">
<pre style="background-color: rgb(238, 238, 238);" class=""><span class="new" \
style="color: blue;">+import static jdk.test.lib.Utils.TEST_CLASS_PATH;</span></pre> \
</div> I'm not a huge fun of 'import static', yet don't insist on
removing it either.
<div class=""><br class="">
</div>
<div class="">
<pre style="background-color: rgb(238, 238, 238);" class=""><span class="new" \
style="color: blue;">+ System.out.println(" libjvm : " + \
jvmLibDir.toString());</span></pre> <div class="">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. </div>
<div class="">btw, you don't need to explicitly call toString in
string concatenation.</div>
<div class=""><br class="">
</div>
</div>
</blockquote><p class=""> Here is the updated fix where the static import \
is removed and libjvm path is used:<br class="">
</p><p class=""> <a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~alexsch/8240604/webrev.03/">http://cr.openjdk.java.net/~alexsch/8240604/webrev.03/</a></p><p \
class=""><br class=""> </p><p class=""> Thanks,</p><p class=""> \
Alexander.<br class=""> </p><p class=""><br class="">
</p>
<blockquote type="cite" \
cite="mid:BC07CAFB-0C57-4932-8818-08113A10856C@oracle.com" class=""> <div class="">
<div class="">-- Igor</div>
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">On Mar 18, 2020, at 9:54 AM, Alexander
Scherbatiy <<a href="mailto:alexander.scherbatiy@bell-sw.com" \
class="" moz-do-not-send="true">alexander.scherbatiy@bell-sw.com</a>> \
wrote:</div> <br class="Apple-interchange-newline">
<div class="">
<div class="">On 18.03.2020 19:00, Igor Ignatyev wrote:<br class="">
<br class="">
<blockquote type="cite" class="">Hi Alexander,<br class="">
<br class="">
<blockquote type="cite" class="">I also included
TEST_NATIVE_PATH to the Utils lib.<br class="">
</blockquote>
for the sake of clarity and ease of backporting, I'd
prefer to have it added by a separate bug and commit.<br class="">
</blockquote>
<br class="">
Here is the updated fix where TEST_NATIVE_PATH is not
added to the Utils lib.<br class="">
<br class="">
<a \
href="http://cr.openjdk.java.net/~alexsch/8240604/webrev.02/" class="" \
moz-do-not-send="true">http://cr.openjdk.java.net/~alexsch/8240604/webrev.02/</a><br \
class=""> <br class="">
<br class="">
Thanks,<br class="">
<br class="">
Alexander.<br class="">
<br class="">
<blockquote type="cite" class="">
<blockquote type="cite" class="">Could I just use "hg
remove binary-fie" and run webrev to add the removed
binary files into webrev?<br class="">
</blockquote>
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.<br class="">
<br class="">
-- Igor<br class="">
<br class="">
<blockquote type="cite" class="">On Mar 18, 2020, at
4:57 AM, Alexander Scherbatiy <<a \
href="mailto:alexander.scherbatiy@bell-sw.com" class="" \
moz-do-not-send="true">alexander.scherbatiy@bell-sw.com</a>> wrote:<br class="">
<br class="">
Hello,<br class="">
<br class="">
Could you review the updated fix:<br class="">
<br class="">
<a \
href="http://cr.openjdk.java.net/~alexsch/8240604/webrev.01" class="" \
moz-do-not-send="true">http://cr.openjdk.java.net/~alexsch/8240604/webrev.01</a><br \
class=""> <br class="">
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.<br class="">
<br class="">
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.<br class="">
<br class="">
<br class="">
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.<br class="">
<br class="">
Could I just use "hg remove binary-fie" and run
webrev to add the removed binary files into webrev?<br class="">
<br class="">
<br class="">
Thanks,<br class="">
<br class="">
Alexander.<br class="">
<br class="">
On 17.03.2020 20:11, Igor Ignatyev wrote:<br class="">
<blockquote type="cite" class="">Hi Alexander,<br class="">
<br class="">
overall looks good to me, I have a few comments
though:<br class="">
- you can use Utils.TEST_CLASSPATH instead of
CustomLauncherTest.TEST_CLASSPATH<br class="">
- CustomLauncherTest::findLibjvm can be simplified
by use Platform::jvmLibDir<br class="">
- exelauncher.c has a comment which refers to the
test as CustomLauncherTest.sh, could you please
update the comment?<br class="">
- you have to add /native flag to @run action,
otherwise jtreg won't exclude this test from runs
w/ test.nativepath being unset<br class="">
<br class="">
I also have a question regarding your statement
that<br class="">
<blockquote type="cite" class="">
<blockquote type="cite" class="">The changes for
obsolete binary files <...> are not
included into the webrev. They needs to be
removed manually.<br class="">
</blockquote>
</blockquote>
you are planning to remove these files as part of
this patch, right?<br class="">
<br class="">
Thanks,<br class="">
-- Igor<br class="">
<br class="">
<br class="">
<blockquote type="cite" class="">On Mar 5, 2020,
at 6:27 AM, Daniel Fuchs <<a \
href="mailto:daniel.fuchs@oracle.com" class="" \
moz-do-not-send="true">daniel.fuchs@oracle.com</a>> wrote:<br class="">
<br class="">
Hi Alexander,<br class="">
<br class="">
Fixes to JMX & management agent are reviewed
on the<br class="">
seviceability-dev (added in to:) these days.<br class="">
<br class="">
best regards,<br class="">
<br class="">
-- daniel<br class="">
<br class="">
On 05/03/2020 13:17, Alexander Scherbatiy wrote:<br class="">
<blockquote type="cite" class="">Hello,<br class="">
Could you review a small enhancement where the
test CustomLauncherTest is updated to build
binary launcher file from launcher.c file.<br class="">
The file launcher.c is renamed to
exelauncher.c to follow the name convention
for executable test files building by jdk make
system.<br class="">
Bug: <a \
href="https://bugs.openjdk.java.net/browse/JDK-8240604" class="" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8240604</a><br \
class=""> Webrev: <a href="http://cr.openjdk.java.net/~alexsch/8240604/webrev.00" \
class="" moz-do-not-send="true">http://cr.openjdk.java.net/~alexsch/8240604/webrev.00</a><br \
class=""> 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.<br class="">
The test is passed on Ubuntu 18.04 x86-64,
Solaris Sparc v9 11.2, and Solaris x64 11.4
systems.<br class="">
The test is excluded from Windows and Mac Os X
systems.<br class="">
Thanks,<br class="">
Alexander.<br class="">
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</blockquote>
</div>
</div></blockquote></div><br class=""></div></div></div></body></html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic