RFR 8189671: jlink should clearly report error when an automatic module is used as root

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR 8189671: jlink should clearly report error when an automatic module is used as root

Sundararajan Athijegannathan
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189671: jlink should clearly report error when an automatic module is used as root

Alan Bateman
On 19/10/2017 15:04, Sundararajan Athijegannathan wrote:
> Please review.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8189671
> Webrev: http://cr.openjdk.java.net/~sundar/8189671/webrev.00/
Why do you want to treat the root modules differently?  Shouldn't it
look at all modules in the configuration (cf, not config) and fail with
a useful message if any of the modules are automatic modules?

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189671: jlink should clearly report error when an automatic module is used

Sundararajan Athijegannathan
Right. Updated: http://cr.openjdk.java.net/~sundar/8189671/webrev.01/

Thanks,
-Sundar

On 19/10/17, 8:05 PM, Alan Bateman wrote:

> On 19/10/2017 15:04, Sundararajan Athijegannathan wrote:
>> Please review.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189671
>> Webrev: http://cr.openjdk.java.net/~sundar/8189671/webrev.00/
> Why do you want to treat the root modules differently?  Shouldn't it
> look at all modules in the configuration (cf, not config) and fail
> with a useful message if any of the modules are automatic modules?
>
> -Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189671: jlink should clearly report error when an automatic module is used as root

Mandy Chung
In reply to this post by Sundararajan Athijegannathan


On 10/19/17 7:04 AM, Sundararajan Athijegannathan wrote:
> Please review.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8189671
> Webrev: http://cr.openjdk.java.net/~sundar/8189671/webrev.00/
>
441 ModuleFinder finder = config.finder();
442 for (String root : config.getModules()) { This should check the
resolved modules rather than just the root modules since a resolved
module may be an automatic module but not a root. You may want to add
such a test case too. (A side note: JlinkConfiguration::getModules
perhaps should be renamed to `roots` since it returns the root modules
specified in jlink --add-modules)
447 throw new
IllegalArgumentException(taskHelper.getMessage("err.automatic.module.as.root",
modDesc.name())); It may be useful to include the pathname to the
exception message. Nit: long line and good to wrap it into multiple
lines. Mandy

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189671: jlink should clearly report error when an automatic module is used as root

Mandy Chung
Hi Sunder,

> Updated: http://cr.openjdk.java.net/~sundar/8189671/webrev.01/

This version checks the resolved modules which is good.  One other comment:

444 throw new
IllegalArgumentException(taskHelper.getMessage("err.automatic.module",
modDesc.name()));It may be useful to include the pathname to the
exception message. Nit: long line and good to wrap it into multiple lines.

Mandy
P.S.  My previous reply lost the formatting and I fixed the copy below
(just as a record for easy viewing).



On 10/19/17 2:58 PM, mandy chung wrote:

>
> On 10/19/17 7:04 AM, Sundararajan Athijegannathan wrote:
>> Please review.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189671
>> Webrev: http://cr.openjdk.java.net/~sundar/8189671/webrev.00/
>>
>
> 441 ModuleFinder finder = config.finder();
> 442 for (String root : config.getModules()) {
> This should check the resolved modules rather than just the root
> modules since a resolved module may be an automatic module but not
> a root. You may want to add such a test case too.
>
> (A side note: JlinkConfiguration::getModules perhaps should be
> renamed to `roots` since it returns the root modules specified
> in jlink --add-modules)
>
> 447    throw new IllegalArgumentException(taskHelper.getMessage("err.automatic.module.as.root", modDesc.name()));
>
> It may be useful to include the pathname to the exception message.
> Nit: long line and good to wrap it into multiple lines.
> Mandy

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189671: jlink should clearly report error when an automatic module is used

Sundararajan Athijegannathan
Hi,

Updated to include location (when available) and formatting:
http://cr.openjdk.java.net/~sundar/8189671/webrev.02/

-Sundar

On 20/10/17, 3:37 AM, mandy chung wrote:

> Hi Sunder,
>
>> Updated: http://cr.openjdk.java.net/~sundar/8189671/webrev.01/
>
> This version checks the resolved modules which is good.  One other
> comment:
>
> 444 throw new
> IllegalArgumentException(taskHelper.getMessage("err.automatic.module",
> modDesc.name()));It may be useful to include the pathname to the
> exception message. Nit: long line and good to wrap it into multiple
> lines.
>
> Mandy
> P.S.  My previous reply lost the formatting and I fixed the copy below
> (just as a record for easy viewing).
>
>
>
> On 10/19/17 2:58 PM, mandy chung wrote:
>>
>> On 10/19/17 7:04 AM, Sundararajan Athijegannathan wrote:
>>> Please review.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189671
>>> Webrev: http://cr.openjdk.java.net/~sundar/8189671/webrev.00/
>>>
>>
>> 441 ModuleFinder finder = config.finder();
>> 442 for (String root : config.getModules()) {
>> This should check the resolved modules rather than just the root
>> modules since a resolved module may be an automatic module but not
>> a root. You may want to add such a test case too.
>>
>> (A side note: JlinkConfiguration::getModules perhaps should be
>> renamed to `roots` since it returns the root modules specified
>> in jlink --add-modules)
>>
>> 447    throw new
>> IllegalArgumentException(taskHelper.getMessage("err.automatic.module.as.root",
>> modDesc.name()));
>>
>> It may be useful to include the pathname to the exception message.
>> Nit: long line and good to wrap it into multiple lines.
>> Mandy
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189671: jlink should clearly report error when an automatic module is used

Alan Bateman
On 20/10/2017 04:53, Sundararajan Athijegannathan wrote:
> Hi,
>
> Updated to include location (when available) and formatting:
> http://cr.openjdk.java.net/~sundar/8189671/webrev.02/
This looks right now. As jlink only supports packaged modules on the
file system then each module should have a location. Another way to
handle this case is to map it to "unknown", this should do it:

         cf.modules().stream()
                 .map(ResolvedModule::reference)
                 .filter(mref -> mref.descriptor().isAutomatic())
                 .findAny()
                 .ifPresent(mref -> {
                     String loc =
mref.location().map(URI::toString).orElse("<unknown>");
                     ..
                 });

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189671: jlink should clearly report error when an automatic module is used

Sundararajan Athijegannathan
Updated:  http://cr.openjdk.java.net/~sundar/8189671/webrev.03/

-Sundar

On 20/10/17, 12:46 PM, Alan Bateman wrote:

> On 20/10/2017 04:53, Sundararajan Athijegannathan wrote:
>> Hi,
>>
>> Updated to include location (when available) and formatting:
>> http://cr.openjdk.java.net/~sundar/8189671/webrev.02/
> This looks right now. As jlink only supports packaged modules on the
> file system then each module should have a location. Another way to
> handle this case is to map it to "unknown", this should do it:
>
>         cf.modules().stream()
>                 .map(ResolvedModule::reference)
>                 .filter(mref -> mref.descriptor().isAutomatic())
>                 .findAny()
>                 .ifPresent(mref -> {
>                     String loc =
> mref.location().map(URI::toString).orElse("<unknown>");
>                     ..
>                 });
>
> -Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189671: jlink should clearly report error when an automatic module is used

Alan Bateman
On 20/10/2017 09:03, Sundararajan Athijegannathan wrote:
> Updated: http://cr.openjdk.java.net/~sundar/8189671/webrev.03/
Looks good.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189671: jlink should clearly report error when an automatic module is used

Mandy Chung
In reply to this post by Sundararajan Athijegannathan


On 10/20/17 1:03 AM, Sundararajan Athijegannathan wrote:
> Updated: http://cr.openjdk.java.net/~sundar/8189671/webrev.03/
>

Looks good.

Mandy