RFR 8185130: jlink should throw error if target image and current JDK versions don't match

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

RFR 8185130: jlink should throw error if target image and current JDK versions don't match

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

Re: RFR 8185130: jlink should throw error if target image and current JDK versions don't match

Claes Redestad
Hi,

just a note that the check you're removing in GenerateJLIClassesPlugin
was stricter
(also required same minor version), but the choice to do that was more
or less an
arbitrary one. Checking only major is likely going to be sufficient.

Apart from that I think this is a welcome change, as making the more
complex jlink
plugins work across multiple JDK versions are going to be very
error-prone and
complicated (this might change as tools and APIs evolve...)

Thanks!

/Claes

On 2017-12-05 14:34, Sundararajan Athijegannathan wrote:
> Please review.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8185130
> Webrev: http://cr.openjdk.java.net/~sundar/8185130/webrev.00/index.html
>
> Thanks,
> -Sundar

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8185130: jlink should throw error if target image and current JDK versions don't match

Alan Bateman
In reply to this post by Sundararajan Athijegannathan
On 05/12/2017 13:34, Sundararajan Athijegannathan wrote:
> Please review.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8185130
> Webrev: http://cr.openjdk.java.net/~sundar/8185130/webrev.00/index.html
The check in newModuleFinder needs to check the minor version too
(changing to interim soon). It can't assume that there will only be
differences in major/feature versions.

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

Re: RFR 8185130: jlink should throw error if target image and current JDK versions don't match

Sundararajan Athijegannathan
Updated to check minor version as well ->
http://cr.openjdk.java.net/~sundar/8185130/webrev.01/index.html

Thanks
-Sundar

On 05/12/17, 7:07 PM, Alan Bateman wrote:

> On 05/12/2017 13:34, Sundararajan Athijegannathan wrote:
>> Please review.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8185130
>> Webrev: http://cr.openjdk.java.net/~sundar/8185130/webrev.00/index.html
> The check in newModuleFinder needs to check the minor version too
> (changing to interim soon). It can't assume that there will only be
> differences in major/feature versions.
>
> -Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8185130: jlink should throw error if target image and current JDK versions don't match

Alan Bateman
On 05/12/2017 14:42, Sundararajan Athijegannathan wrote:
> Updated to check minor version as well ->
> http://cr.openjdk.java.net/~sundar/8185130/webrev.01/index.html
I think this looks okay. I assume the java9.home in the test is for
manual test.

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

Re: RFR 8185130: jlink should throw error if target image and current JDK versions don't match

Sundararajan Athijegannathan
Yes, that is right.

Thanks for the review.


-Sundar

On 05/12/17, 8:53 PM, Alan Bateman wrote:
> On 05/12/2017 14:42, Sundararajan Athijegannathan wrote:
>> Updated to check minor version as well ->
>> http://cr.openjdk.java.net/~sundar/8185130/webrev.01/index.html
> I think this looks okay. I assume the java9.home in the test is for
> manual test.
>
> -Alan.
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8185130: jlink should throw error if target image and current JDK versions don't match

Mandy Chung
In reply to this post by Sundararajan Athijegannathan


On 12/5/17 6:42 AM, Sundararajan Athijegannathan wrote:
> Updated to check minor version as well ->
> http://cr.openjdk.java.net/~sundar/8185130/webrev.01/index.html

454 throw new IllegalArgumentException(String.format("jlink version
%d.%d != java.base version %d.%d",
455 Runtime.version().major(), Runtime.version().minor(),
456 version.major(), version.minor())); This should be a localized
message. Otherwise, looks okay.

Mandy

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8185130: jlink should throw error if target image and current JDK versions don't match

Sundararajan Athijegannathan
Updated: http://cr.openjdk.java.net/~sundar/8185130/webrev.02/

-Sundar

On 05/12/17, 9:55 PM, mandy chung wrote:

>
>
> On 12/5/17 6:42 AM, Sundararajan Athijegannathan wrote:
>> Updated to check minor version as well ->
>> http://cr.openjdk.java.net/~sundar/8185130/webrev.01/index.html
>
>   454                 throw new IllegalArgumentException(String.format("jlink version %d.%d != java.base version %d.%d",
>   455                     Runtime.version().major(), Runtime.version().minor(),
>   456                     version.major(), version.minor()));
>
> This should be a localized message.  Otherwise, looks okay.
> Mandy
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8185130: jlink should throw error if target image and current JDK versions don't match

Mandy Chung


On 12/5/17 9:43 AM, Sundararajan Athijegannathan wrote:
> Updated: http://cr.openjdk.java.net/~sundar/8185130/webrev.02/
>

Looks good.  Thanks for the update.

Mandy