RFR(M) : 8211171 : move JarUtils to top-level testlibrary
http://cr.openjdk.java.net/~iignatyev//8211171/webrev.00/index.html
182 lines changed: 0 ins; 152 del; 30 mod;
Hi all, could you please review this small and straightforward patch which moves JarUtils from jdk testlibrary to the top-level testlibrary, removes JarUtils from jaxp testlibrary, and updates all the affected tests? please note, there is another version of JarUtils in jdk.test.lib.util package, but these two versions have different signatures and behavior, merging them is a bit tricker and will be done separately. JBS: https://bugs.openjdk.java.net/browse/JDK-8211171 webrev: http://cr.openjdk.java.net/~iignatyev//8211171/webrev.00/index.html testing: tier1-3 Thanks, -- Igor
On 26/09/2018 19:16, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8211171/webrev.00/index.html Looks good but maybe it should be put into a named package (see below).
: please note, there is another version of JarUtils in jdk.test.lib.util package, but these two versions have different signatures and behavior, merging them is a bit tricker and will be done separately.
The JarUtils that are moving today to test/lib is one that I added as test infrastructure some time ago. I don't where test/lib/jdk/test/lib/util/JarUtils.jav came from but it seems less useful. It makes me wonder if it would be better to just convert tests rather than merging. A few convenience methods added to the JarUtils you are moving would make that easier. -Alan
Hi Alan, it appears that I have to put into a named package as the jaxp test which uses it is in a named package. although I don't want to group simple moving and more sophisticated test conversion for the sake of reviewers, also there is another "duplicate" of JarUtils in test/jdk/lib/testlibrary/java/util/jar/JarBuilder.java. I'd prefer to "merge" all three instances together, where merge might be implemented as conversion of tests. so as an intermediate step, I propose to move all the methods of JarUtils into jdk.test.lib.util.JarUtils. Thanks, -- Igor
On Sep 26, 2018, at 1:11 PM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 26/09/2018 19:16, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8211171/webrev.00/index.html Looks good but maybe it should be put into a named package (see below).
: please note, there is another version of JarUtils in jdk.test.lib.util package, but these two versions have different signatures and behavior, merging them is a bit tricker and will be done separately.
The JarUtils that are moving today to test/lib is one that I added as test infrastructure some time ago. I don't where test/lib/jdk/test/lib/util/JarUtils.jav came from but it seems less useful. It makes me wonder if it would be better to just convert tests rather than merging. A few convenience methods added to the JarUtils you are moving would make that easier.
-Alan
here is the webrevs w/ JarUtils from default package inserted into jdk.test.lib.util.JarUtils: whole patch: http://cr.openjdk.java.net/~iignatyev//8211171/webrev.01/index.html <http://cr.openjdk.java.net/~iignatyev//8211171/webrev.01/index.html>
655 lines changed: 239 ins; 355 del; 61 mod; incremental: http://cr.openjdk.java.net/~iignatyev//8211171/webrev.0-1/index.html <http://cr.openjdk.java.net/~iignatyev//8211171/webrev.0-1/index.html> 476 lines changed: 239 ins; 203 del; 34 mod;
doing that, I noticed that both updateJarFile and createJarFile don't close Stream<Path> from Files::find, the current patch fixes that. -- Igor
On Sep 26, 2018, at 2:25 PM, Igor Ignatyev <igor.ignatyev@oracle.com> wrote:
Hi Alan,
it appears that I have to put into a named package as the jaxp test which uses it is in a named package.
although I don't want to group simple moving and more sophisticated test conversion for the sake of reviewers, also there is another "duplicate" of JarUtils in test/jdk/lib/testlibrary/java/util/jar/JarBuilder.java. I'd prefer to "merge" all three instances together, where merge might be implemented as conversion of tests.
so as an intermediate step, I propose to move all the methods of JarUtils into jdk.test.lib.util.JarUtils.
Thanks, -- Igor
On Sep 26, 2018, at 1:11 PM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 26/09/2018 19:16, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8211171/webrev.00/index.html Looks good but maybe it should be put into a named package (see below).
: please note, there is another version of JarUtils in jdk.test.lib.util package, but these two versions have different signatures and behavior, merging them is a bit tricker and will be done separately.
The JarUtils that are moving today to test/lib is one that I added as test infrastructure some time ago. I don't where test/lib/jdk/test/lib/util/JarUtils.jav came from but it seems less useful. It makes me wonder if it would be better to just convert tests rather than merging. A few convenience methods added to the JarUtils you are moving would make that easier.
-Alan
On 27/09/2018 00:38, Igor Ignatyev wrote:
here is the webrevs w/ JarUtils from default package inserted into jdk.test.lib.util.JarUtils: whole patch: http://cr.openjdk.java.net/~iignatyev//8211171/webrev.01/index.html <http://cr.openjdk.java.net/%7Eiignatyev//8211171/webrev.01/index.html>
655 lines changed: 239 ins; 355 del; 61 mod; incremental: http://cr.openjdk.java.net/~iignatyev//8211171/webrev.0-1/index.html <http://cr.openjdk.java.net/%7Eiignatyev//8211171/webrev.0-1/index.html> 476 lines changed: 239 ins; 203 del; 34 mod;
doing that, I noticed that both updateJarFile and createJarFile don't close Stream<Path> from Files::find, the current patch fixes that.
I see you've also deprecated the String methods in the old class - good! I'd probably carry over test/jdk/lib/testlibrary/JarUtils.java without changing the format but your IDE must be setup differently and it will get changed again by whoever next changes it so I think it's okay. The update to tests using this look fine. -Alan
Hi Alan, thanks for your review! -- Igor
On Sep 30, 2018, at 8:00 AM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 27/09/2018 00:38, Igor Ignatyev wrote:
here is the webrevs w/ JarUtils from default package inserted into jdk.test.lib.util.JarUtils: whole patch: http://cr.openjdk.java.net/~iignatyev//8211171/webrev.01/index.html <http://cr.openjdk.java.net/%7Eiignatyev//8211171/webrev.01/index.html>
655 lines changed: 239 ins; 355 del; 61 mod; incremental: http://cr.openjdk.java.net/~iignatyev//8211171/webrev.0-1/index.html <http://cr.openjdk.java.net/%7Eiignatyev//8211171/webrev.0-1/index.html> 476 lines changed: 239 ins; 203 del; 34 mod;
doing that, I noticed that both updateJarFile and createJarFile don't close Stream<Path> from Files::find, the current patch fixes that.
I see you've also deprecated the String methods in the old class - good! I'd probably carry over test/jdk/lib/testlibrary/JarUtils.java without changing the format but your IDE must be setup differently and it will get changed again by whoever next changes it so I think it's okay.
The update to tests using this look fine.
-Alan
Do we have a plan to move away from the deprecated methods? Is there a flag I can set to check how many classes are using them? --Max
On Sep 30, 2018, at 11:00 PM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 27/09/2018 00:38, Igor Ignatyev wrote:
here is the webrevs w/ JarUtils from default package inserted into jdk.test.lib.util.JarUtils: whole patch: http://cr.openjdk.java.net/~iignatyev//8211171/webrev.01/index.html <http://cr.openjdk.java.net/%7Eiignatyev//8211171/webrev.01/index.html>
655 lines changed: 239 ins; 355 del; 61 mod; incremental: http://cr.openjdk.java.net/~iignatyev//8211171/webrev.0-1/index.html <http://cr.openjdk.java.net/%7Eiignatyev//8211171/webrev.0-1/index.html> 476 lines changed: 239 ins; 203 del; 34 mod;
doing that, I noticed that both updateJarFile and createJarFile don't close Stream<Path> from Files::find, the current patch fixes that.
I see you've also deprecated the String methods in the old class - good! I'd probably carry over test/jdk/lib/testlibrary/JarUtils.java without changing the format but your IDE must be setup differently and it will get changed again by whoever next changes it so I think it's okay.
The update to tests using this look fine.
-Alan
Hi Max, we do have a plan to remove these deprecated methods as part of 8211289[1]. I don't know if there a flag which you can use to get all classes which use deprecated methods. you can however analyze source or bytecode of tests, to see if they have dependency on these methods, but again I ain't aware of tools (rather than grep) which can help you to do that in jtreg test suite. [1] https://bugs.openjdk.java.net/browse/JDK-8211289 <https://bugs.openjdk.java.net/browse/JDK-8211289> -- Igor
On Oct 10, 2018, at 8:47 PM, Weijun Wang <weijun.wang@oracle.com> wrote:
Do we have a plan to move away from the deprecated methods? Is there a flag I can set to check how many classes are using them?
--Max
On Sep 30, 2018, at 11:00 PM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 27/09/2018 00:38, Igor Ignatyev wrote:
here is the webrevs w/ JarUtils from default package inserted into jdk.test.lib.util.JarUtils: whole patch: http://cr.openjdk.java.net/~iignatyev//8211171/webrev.01/index.html <http://cr.openjdk.java.net/%7Eiignatyev//8211171/webrev.01/index.html>
655 lines changed: 239 ins; 355 del; 61 mod; incremental: http://cr.openjdk.java.net/~iignatyev//8211171/webrev.0-1/index.html <http://cr.openjdk.java.net/%7Eiignatyev//8211171/webrev.0-1/index.html> 476 lines changed: 239 ins; 203 del; 34 mod;
doing that, I noticed that both updateJarFile and createJarFile don't close Stream<Path> from Files::find, the current patch fixes that.
I see you've also deprecated the String methods in the old class - good! I'd probably carry over test/jdk/lib/testlibrary/JarUtils.java without changing the format but your IDE must be setup differently and it will get changed again by whoever next changes it so I think it's okay.
The update to tests using this look fine.
-Alan
Hi Igor, The IDEs including IntelliJ can pretty easily search for usages of various methods whereever they occur in the source tree. And they can do the refactoring as well. $.02, Roger On 10/11/2018 02:55 PM, Igor Ignatyev wrote:
Hi Max,
we do have a plan to remove these deprecated methods as part of 8211289[1].
I don't know if there a flag which you can use to get all classes which use deprecated methods. you can however analyze source or bytecode of tests, to see if they have dependency on these methods, but again I ain't aware of tools (rather than grep) which can help you to do that in jtreg test suite.
[1] https://bugs.openjdk.java.net/browse/JDK-8211289 <https://bugs.openjdk.java.net/browse/JDK-8211289>
-- Igor
On Oct 10, 2018, at 8:47 PM, Weijun Wang <weijun.wang@oracle.com> wrote:
Do we have a plan to move away from the deprecated methods? Is there a flag I can set to check how many classes are using them?
--Max
On Sep 30, 2018, at 11:00 PM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 27/09/2018 00:38, Igor Ignatyev wrote:
here is the webrevs w/ JarUtils from default package inserted into jdk.test.lib.util.JarUtils: whole patch: http://cr.openjdk.java.net/~iignatyev//8211171/webrev.01/index.html <http://cr.openjdk.java.net/%7Eiignatyev//8211171/webrev.01/index.html>
655 lines changed: 239 ins; 355 del; 61 mod; incremental: http://cr.openjdk.java.net/~iignatyev//8211171/webrev.0-1/index.html <http://cr.openjdk.java.net/%7Eiignatyev//8211171/webrev.0-1/index.html> 476 lines changed: 239 ins; 203 del; 34 mod; doing that, I noticed that both updateJarFile and createJarFile don't close Stream<Path> from Files::find, the current patch fixes that.
I see you've also deprecated the String methods in the old class - good! I'd probably carry over test/jdk/lib/testlibrary/JarUtils.java without changing the format but your IDE must be setup differently and it will get changed again by whoever next changes it so I think it's okay.
The update to tests using this look fine.
-Alan
-- Thanks, Roger
Hi Roger, that's true, but due to layout of tests in jtreg test suites, it requires special configurations and still doesn't work in all cases. with that being said, in case of JarUtils methods, it should work fine. -- Igor
On Oct 11, 2018, at 12:14 PM, Roger Riggs <Roger.Riggs@Oracle.com> wrote:
Hi Igor,
The IDEs including IntelliJ can pretty easily search for usages of various methods whereever they occur in the source tree. And they can do the refactoring as well.
$.02, Roger
On 10/11/2018 02:55 PM, Igor Ignatyev wrote:
Hi Max,
we do have a plan to remove these deprecated methods as part of 8211289[1].
I don't know if there a flag which you can use to get all classes which use deprecated methods. you can however analyze source or bytecode of tests, to see if they have dependency on these methods, but again I ain't aware of tools (rather than grep) which can help you to do that in jtreg test suite.
[1] https://bugs.openjdk.java.net/browse/JDK-8211289 <https://bugs.openjdk.java.net/browse/JDK-8211289>
-- Igor
On Oct 10, 2018, at 8:47 PM, Weijun Wang <weijun.wang@oracle.com> wrote:
Do we have a plan to move away from the deprecated methods? Is there a flag I can set to check how many classes are using them?
--Max
On Sep 30, 2018, at 11:00 PM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 27/09/2018 00:38, Igor Ignatyev wrote:
here is the webrevs w/ JarUtils from default package inserted into jdk.test.lib.util.JarUtils: whole patch: http://cr.openjdk.java.net/~iignatyev//8211171/webrev.01/index.html <http://cr.openjdk.java.net/%7Eiignatyev//8211171/webrev.01/index.html>
655 lines changed: 239 ins; 355 del; 61 mod; incremental: http://cr.openjdk.java.net/~iignatyev//8211171/webrev.0-1/index.html <http://cr.openjdk.java.net/%7Eiignatyev//8211171/webrev.0-1/index.html> 476 lines changed: 239 ins; 203 del; 34 mod; doing that, I noticed that both updateJarFile and createJarFile don't close Stream<Path> from Files::find, the current patch fixes that.
I see you've also deprecated the String methods in the old class - good! I'd probably carry over test/jdk/lib/testlibrary/JarUtils.java without changing the format but your IDE must be setup differently and it will get changed again by whoever next changes it so I think it's okay.
The update to tests using this look fine.
-Alan
-- Thanks, Roger
participants (4)
-
Alan Bateman
-
Igor Ignatyev
-
Roger Riggs
-
Weijun Wang