LGTM -- Igor
> On Oct 11, 2018, at 5:54 PM, Hamlin Li <huaming...@oracle.com> wrote: > > Hi Igor, > > It's updated in place http://cr.openjdk.java.net/~mli/8186610/webrev.01/ > <http://cr.openjdk.java.net/~mli/8186610/webrev.01/>, please help to review > again. > > Thank you > > -Hamlin > > On 2018/10/12 2:51 AM, Igor Ignatyev wrote: >> 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 >>> <mailto: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/%7Emli/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 >>>>>> >>>>> >>>> >>> >> >
