Hi Hamlin, the key point here, ModuleTargetHelper.java is not a "library" class, it's a part of tools/jlink/plugins/SystemModuleDescriptors/ tests.
thanks for updating webrev, I have one nit --- given ModuleUtils is the only class in jdk/test/lib/module package, I doubt we need to introduce this package, ModuleUtils can be put into j.t.l.util package. -- Igor. > On Oct 10, 2018, at 11:31 PM, Hamlin Li <huaming...@oracle.com> wrote: > > Thank you clarifying Igor. > > Moving ModuleTargetHelper to local folder has a drawback: it's hard for > future maintainer to found it if they need to use it in other places, that > make it an "invisible" library class. > Although I don't fully agree with you, I have updated the webrev as you > suggested: http://cr.openjdk.java.net/~mli/8186610/webrev.01/ > <http://cr.openjdk.java.net/~mli/8186610/webrev.01/> > Thank you > > -Hamlin > > On 2018/10/11 11:38 AM, Igor Ignatyev wrote: >> b/c this will make test library modularization[1] somewhat troublesome, also >> I ain't sure if ModuleTargetHelper really needs to be placed into the >> top-level library regardless of its dependency on non-public API. >> "promoting" test library class to the top-level library comes w/ increased >> maintenance costs, the parent task[2] explains that in more details. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8211358 >> <https://bugs.openjdk.java.net/browse/JDK-8211358> >> [2] https://bugs.openjdk.java.net/browse/JDK-8211290 >> <https://bugs.openjdk.java.net/browse/JDK-8211290> >> >> -- Igor >> >>> On Oct 10, 2018, at 8:26 PM, Hamlin Li <huaming...@oracle.com >>> <mailto:huaming...@oracle.com>> wrote: >>> >>> Hi Igor, >>> >>> Would you please clarify your concern further? I mean why >>> ModuleTargetHelper can be put to top level when it use non-public APIs e.g. >>> jdk.internal.module.*? >>> >>> Thank you >>> >>> -Hamlin >>> >>> On 2018/10/11 11:08 AM, Igor Ignatyev wrote: >>>> Hi Hamlin, >>>> >>>> as ModuleTargetHelper uses non-public API, I'd prefer not to have in a >>>> common test library, and 8211976 suggests moving it closer to tests. could >>>> you please explain why you decided to put it into the library instead? >>>> >>>> Thanks, >>>> -- Igor >>>> >>>> ----- Original Message ----- >>>> From: huaming...@oracle.com <mailto:huaming...@oracle.com> >>>> To: core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net> >>>> Sent: Wednesday, October 10, 2018 7:40:34 PM GMT -08:00 US/Canada Pacific >>>> Subject: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary >>>> >>>> Would you please review the following patch? >>>> >>>> bug: >>>> >>>> https://bugs.openjdk.java.net/browse/JDK-8186610 >>>> <https://bugs.openjdk.java.net/browse/JDK-8186610> >>>> >>>> https://bugs.openjdk.java.net/browse/JDK-8211976 >>>> <https://bugs.openjdk.java.net/browse/JDK-8211976> >>>> >>>> webrev: http://cr.openjdk.java.net/~mli/8186610/ >>>> <http://cr.openjdk.java.net/%7Emli/8186610/> >>>> >>>> >>>> Thank you >>>> >>>> -Hamlin >>>> >>> >> >
