RFR 8177471: jlink should use the version from java.base.jmod to find modules

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

RFR 8177471: jlink should use the version from java.base.jmod to find modules

Sundararajan Athijegannathan
Please review.

Bug: https://bugs.openjdk.java.net/browse/JDK-8177471

Webrev: http://cr.openjdk.java.net/~sundar/8177471/webrev.00/index.html

Thanks to Mandy for initial (internal) round of review and suggesting me
simplifications on my initial version of test.

Thanks,
-Sundar
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8177471: jlink should use the version from java.base.jmod to find modules

Alan Bateman
On 13/11/2017 08:02, Sundararajan Athijegannathan wrote:
> Please review.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8177471
>
> Webrev: http://cr.openjdk.java.net/~sundar/8177471/webrev.00/index.html
>
> Thanks to Mandy for initial (internal) round of review and suggesting
> me simplifications on my initial version of test.
The update to newModuleFinder looks okay although at L450 then checking
the major version is probably enough (re-creating the module finder when
the versions aren't equal is okay too).

The changes to ImageHelper bring several questions on whether
NoSuchElementException is possible. I think this is closer to what you
want there:

         Runtime.Version v = cf.findModule("java.base")
                 .map(ResolvedModule::reference)
                 .map(ModuleReference::descriptor)
                 .flatMap(ModuleDescriptor::version)
                 .map(ModuleDescriptor.Version::toString)
                 .map(Runtime.Version::parse)
                 .orElse(Runtime.version());

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

Re: RFR 8177471: jlink should use the version from java.base.jmod to find modules

Sundararajan Athijegannathan
Thanks for the review. Updated as per suggestions:

http://cr.openjdk.java.net/~sundar/8177471/webrev.01/index.html

-Sundar

On 13/11/17, 8:39 PM, Alan Bateman wrote:

> On 13/11/2017 08:02, Sundararajan Athijegannathan wrote:
>> Please review.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8177471
>>
>> Webrev: http://cr.openjdk.java.net/~sundar/8177471/webrev.00/index.html
>>
>> Thanks to Mandy for initial (internal) round of review and suggesting
>> me simplifications on my initial version of test.
> The update to newModuleFinder looks okay although at L450 then
> checking the major version is probably enough (re-creating the module
> finder when the versions aren't equal is okay too).
>
> The changes to ImageHelper bring several questions on whether
> NoSuchElementException is possible. I think this is closer to what you
> want there:
>
>         Runtime.Version v = cf.findModule("java.base")
>                 .map(ResolvedModule::reference)
>                 .map(ModuleReference::descriptor)
>                 .flatMap(ModuleDescriptor::version)
>                 .map(ModuleDescriptor.Version::toString)
>                 .map(Runtime.Version::parse)
>                 .orElse(Runtime.version());
>
> -Alan.
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8177471: jlink should use the version from java.base.jmod to find modules

Alan Bateman
On 13/11/2017 16:36, Sundararajan Athijegannathan wrote:
> Thanks for the review. Updated as per suggestions:
>
> http://cr.openjdk.java.net/~sundar/8177471/webrev.01/index.html
I think this looks fine.

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

Re: RFR 8177471: jlink should use the version from java.base.jmod to find modules

Mandy Chung
In reply to this post by Sundararajan Athijegannathan
Looks good.  Thanks for fixing this.

Mandy

On 11/13/17 2:02 AM, Sundararajan Athijegannathan wrote:

> Please review.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8177471
>
> Webrev: http://cr.openjdk.java.net/~sundar/8177471/webrev.00/index.html
>
> Thanks to Mandy for initial (internal) round of review and suggesting
> me simplifications on my initial version of test.
>
> Thanks,
> -Sundar