Re: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary
Hi Hamlin, the patch looks good, just note to update Copyright dates. -Felix Yang From: core-libs-dev on behalf of Hamlin Li Sent: Friday, October 12, 2018 8:54:51 AM To: Igor Ignatyev Cc: core-libs-dev@openjdk.java.net Subject: Re: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary Hi Igor, It's updated in place 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 > <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/ >> >> 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 >>> [2] https://bugs.openjdk.java.net/browse/JDK-8211290 >>> >>> -- Igor >>> >>>> On Oct 10, 2018, at 8:26 PM, Hamlin Li >>> <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-8211976 >>>>> >>>>> webrev: http://cr.openjdk.java.net/~mli/8186610/ >>>>> <http://cr.openjdk.java.net/%7Emli/8186610/> >>>>> >>>>> >>>>> Thank you >>>>> >>>>> -Hamlin >>>>> >>>> >>> >> >
Re: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary
LGTM -- Igor > On Oct 11, 2018, at 5:54 PM, Hamlin Li 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 >> <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 >>>> <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 >>>>>> >>>>> >>>> >>> >> >
Re: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary
Hi Igor, It's updated in place 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 <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/ 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 [2] https://bugs.openjdk.java.net/browse/JDK-8211290 -- Igor On Oct 10, 2018, at 8:26 PM, Hamlin Li <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-8211976 webrev: http://cr.openjdk.java.net/~mli/8186610/ <http://cr.openjdk.java.net/%7Emli/8186610/> Thank you -Hamlin
Re: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary
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 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 >> <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 >>>> >>> >> >
Re: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary
On 11/10/2018 04:08, 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? Right, ModuleTargetHelper is just a helper classes in the jlink tests, it's not general purpose and not suitable to put into the test infrastructure directory. -Alan
Re: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary
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/ 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 [2] https://bugs.openjdk.java.net/browse/JDK-8211290 -- Igor On Oct 10, 2018, at 8:26 PM, Hamlin Li <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-8211976 webrev: http://cr.openjdk.java.net/~mli/8186610/ <http://cr.openjdk.java.net/%7Emli/8186610/> Thank you -Hamlin
Re: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary
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 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 >> To: 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-8211976 >> >> webrev: http://cr.openjdk.java.net/~mli/8186610/ >> >> >> Thank you >> >> -Hamlin >> >
Re: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary
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 To: 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-8211976 webrev: http://cr.openjdk.java.net/~mli/8186610/ Thank you -Hamlin
Re: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary
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 To: 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-8211976 webrev: http://cr.openjdk.java.net/~mli/8186610/ Thank you -Hamlin
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-8211976 webrev: http://cr.openjdk.java.net/~mli/8186610/ Thank you -Hamlin
