Dmitry, Thank you very much for the review.
Best regards, Alexander ----- Original Message ----- From: [email protected] To: [email protected], [email protected] Sent: Tuesday, April 26, 2016 1:12:56 PM GMT +03:00 Iraq Subject: Re: RFR:8153992 (re-review, missed one file) : Some hotspot tests fail on compact2 due to an unnecessary test library dependency Alexander, Looks good for me! -Dmitry On 2016-04-26 12:36, Alexander Kulyakhtin wrote: > Hi, > > I've missed one file in the changeset reviewed before, therefore the push > failed and I have to ask for a quick re-review. > > The missing change is here: > http://cr.openjdk.java.net/~akulyakh/8153992_02/test/testlibrary/jdk/test/lib/dcmd/FileJcmdExecutor.java.udiff.html > > I've verified that all the hotspot tests have passed with that change. > > The full changeset (reviewed before): > > CR: https://bugs.openjdk.java.net/browse/JDK-8153992 "Some SVC tests fail on > compact2 due to an unnecessary test library dependency" > Webrev: http://cr.openjdk.java.net/~akulyakh/8153992_02 > > Best regards, > Alexander > > ----- Forwarded Message ----- > From: [email protected] > To: [email protected], [email protected] > Cc: [email protected] > Sent: Thursday, April 21, 2016 8:48:23 PM GMT +03:00 Iraq > Subject: Re: RFR:8153992:Some hotspot tests fail on compact2 due to an > unnecessary test library dependency > > The patch looks fine to me. > > Due to ProcessTools.getVmInputArguments() dependency, any test using > ProcessTools has @modules java.management even it does not use this method. > > It’d be good to refactor ProcessTools.getVmInputArguments() and maybe other > test library to eliminate unnecessary dependency. Shura has cleaned up > jdk/test/lib/testlibrary in the jdk side: > https://bugs.openjdk.java.net/browse/JDK-8139430 > > Can you file a separate issue to refactor the hotspot test library and > similar fix to JDK-8139430 can be applied in the future? > > thanks > Mandy > >> On Apr 21, 2016, at 5:23 AM, Alexander Kulyakhtin >> <[email protected]> wrote: >> >> Mandy, >> >> Thank you very much for your review. >> >> I have updated the fix per your comments, making ProcessTools.getProcessId() >> return long. >> >> The function ProcessTools.getVmInputArguments(), although does depend on the >> API from the java.management, is not used by any of the tests addressed by >> the CR. >> >> Best regards, >> Alexander >> >> ----- Original Message ----- >> From: [email protected] >> To: [email protected] >> Cc: [email protected] >> Sent: Thursday, April 21, 2016 3:11:46 PM GMT +03:00 Iraq >> Subject: Re: RFR:8153992:Some hotspot tests fail on compact2 due to an >> unnecessary test library dependency >> >> Hi, >> >> Could you, please, review this fix (updated to address the findings of the >> previous review) >> >> CR: https://bugs.openjdk.java.net/browse/JDK-8153992 "Some SVC tests fail on >> compact2 due to an unnecessary test library dependency" >> Webrev: http://cr.openjdk.java.net/~akulyakh/8153992_02/index.html >> >> Before the fix the ProcessTools.getProcessId() used the >> ManagementFactory.getRuntimeMXBean() API. >> The API is not available on compact2 and below. Therefore the tests failed. >> We are changing the ProcessTools.getProcessId() method to use the JDK 9 >> Process.getPid(). This eliminates the unnecessary dependency making the >> tests pass on compact2. >> >> Since, with this change ProcessTools.getProcessId() now returns long we are >> also trivially modifying all the affected tests. >> >> Best regards, >> Alexander >> >> >> >> ----- Original Message ----- >> From: [email protected] >> To: [email protected] >> Cc: [email protected] >> Sent: Thursday, April 21, 2016 12:03:14 AM GMT +03:00 Iraq >> Subject: Re: RFR:8153989:Some SVC tests fail on compact2 due to an >> unnecessary test library dependency >> >> >>> On Apr 20, 2016, at 9:06 AM, Alexander Kulyakhtin >>> <[email protected]> wrote: >>> >>> Hi, >>> >>> Could you, please, review this small tests-only fix: >>> >>> CR: https://bugs.openjdk.java.net/browse/JDK-8153992 "Some SVC tests fail >>> on compact2 due to an unnecessary test library dependency" >>> Webrev: >>> http://cr.openjdk.java.net/~akulyakh/8153992/test/testlibrary/jdk/test/lib/ProcessTools.java.udiff.html >>> >>> Before the fix the ProcessTools.getProcessId() used the >>> ManagementFactory.getRuntimeMXBean() API. >>> The API is not available on compact2 and below. Therefore the tests failed. >>> >>> We are changing the ProcessTools.getProcessId() method to use the JDK 9 >>> Process.getPid(). This eliminates the unnecessary dependency making the >>> tests pass on compact2. >>> >> >> This looks okay. But I see that getVmInputArguments calls >> ManagementFactory.getRuntimeMXBean. So ProcessTools still has a dependency >> on java.management. >> >> The jdk test library ProcessTools::getProcessId has been long ago to call >> Process::getPid and the method is changed to return long. I thought that >> similar change would have been made in the hotspot test library at that time. >> >>> I am not sure how acceptable it is to cast from long to int this change. If >>> it is not acceptable we can change the return type to long. >>> This however, will cause massive changes throughout the hotspot tests which >>> presently expect getProcessId() to return int. >> >> IMO it would be good to return long or have the callsite to call >> ProcessHandle.current().getPid(). >> >> Mandy >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
