RFR 8189777: jlink --module-path default value and automatic addition of $JAVA_HOME/jmods if java.base is missing

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

RFR 8189777: jlink --module-path default value and automatic addition of $JAVA_HOME/jmods if java.base is missing

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

Re: RFR 8189777: jlink --module-path default value and automatic addition of $JAVA_HOME/jmods if java.base is missing

Alan Bateman
On 23/10/2017 06:37, Sundararajan Athijegannathan wrote:
> Please review.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8189777
> Webrev: http://cr.openjdk.java.net/~sundar/8189777/webrev.00/
This patch is very welcome as it makes jlink easier to use without any
impact to the cross building case.

Two small comments:

- getStandardModulePath might be better renamed getDefaultModulePath as
it isn't standard.

- JlinkConfiguration calls moduleFinder() during construction. You can
avoid this by adding a static ModuelFinder moduleFinder(List<Path>).

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

Re: RFR 8189777: jlink --module-path default value and automatic addition of $JAVA_HOME/jmods if java.base is missing

Sundararajan Athijegannathan
Updated for getDefaultModulePath. moduleFinder uses three instance
fields - modulepaths, limitmods and modules. We may have to pass all to
the static method...

http://cr.openjdk.java.net/~sundar/8189777/webrev.01

Thanks
-Sundar

On 23/10/17, 1:11 PM, Alan Bateman wrote:

> On 23/10/2017 06:37, Sundararajan Athijegannathan wrote:
>> Please review.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189777
>> Webrev: http://cr.openjdk.java.net/~sundar/8189777/webrev.00/
> This patch is very welcome as it makes jlink easier to use without any
> impact to the cross building case.
>
> Two small comments:
>
> - getStandardModulePath might be better renamed getDefaultModulePath
> as it isn't standard.
>
> - JlinkConfiguration calls moduleFinder() during construction. You can
> avoid this by adding a static ModuelFinder moduleFinder(List<Path>).
>
> -Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189777: jlink --module-path default value and automatic addition of $JAVA_HOME/jmods if java.base is missing

Alan Bateman
On 23/10/2017 11:37, Sundararajan Athijegannathan wrote:
> Updated for getDefaultModulePath. moduleFinder uses three instance
> fields - modulepaths, limitmods and modules. We may have to pass all
> to the static method...
Yes, if you want to avoid operating on a partly initialized
JlinkConfiguration.

The other alternative is to create the ModuleFinder before creating the
JlinkConfiguration, that might be clearer overall.

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

Re: RFR 8189777: jlink --module-path default value and automatic addition of $JAVA_HOME/jmods if java.base is missing

Mandy Chung
On 10/23/17 3:37 AM, Sundararajan Athijegannathan wrote:
> Updated for getDefaultModulePath. moduleFinder uses three instance
> fields - modulepaths, limitmods and modules. We may have to pass all
> to the static method...
>
> http://cr.openjdk.java.net/~sundar/8189777/webrev.01 

What happens to this command?
    $ jlink --add-modules ALL-MODULE-PATH -output image

It will use the default module path but there is no root module.

On 10/23/17 7:33 AM, Alan Bateman wrote:
>
> The other alternative is to create the ModuleFinder before creating
> the JlinkConfiguration, that might be clearer overall.

I like the idea passing ModuleFinder to JlinkConfiguration constructor
which is cleaner.

Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189777: jlink --module-path default value and automatic addition of $JAVA_HOME/jmods if java.base is missing

Sundararajan Athijegannathan
Hi,

* ALL-MODULE-PATH case seems to be handled fine (in  private
JlinkConfiguration initJlinkConfig() throws BadArgs { method). When no
--module-path is specified, default module path is used and all default
observable modules are added as root. I ran the command you mentioned
and it worked fine.

* refactored JlinkConfiguration.moduleFinder as a static method. Note
that we've to create ModuleFinder inside JlinkContiguration because we
may change modulepaths in constructor (if java.base module is not found)
and so new ModuleFinder has to be created anyway.

Updated webrev: http://cr.openjdk.java.net/~sundar/8189777/webrev.02/

Thanks,
-Sundar


On 24/10/17, 2:57 AM, mandy chung wrote:

> On 10/23/17 3:37 AM, Sundararajan Athijegannathan wrote:
>> Updated for getDefaultModulePath. moduleFinder uses three instance
>> fields - modulepaths, limitmods and modules. We may have to pass all
>> to the static method...
>>
>> http://cr.openjdk.java.net/~sundar/8189777/webrev.01 
>
> What happens to this command?
>    $ jlink --add-modules ALL-MODULE-PATH -output image
>
> It will use the default module path but there is no root module.
>
> On 10/23/17 7:33 AM, Alan Bateman wrote:
>>
>> The other alternative is to create the ModuleFinder before creating
>> the JlinkConfiguration, that might be clearer overall.
>
> I like the idea passing ModuleFinder to JlinkConfiguration constructor
> which is cleaner.
>
> Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189777: jlink --module-path default value and automatic addition of $JAVA_HOME/jmods if java.base is missing

Mandy Chung


On 10/23/17 9:05 PM, Sundararajan Athijegannathan wrote:
> Hi,
>
> * ALL-MODULE-PATH case seems to be handled fine (in  private
> JlinkConfiguration initJlinkConfig() throws BadArgs { method). When no
> --module-path is specified, default module path is used and all
> default observable modules are added as root. I ran the command you
> mentioned and it worked fine.
>

That's right.  The default module path is added before initJlinkConfig
is called.  Thanks for confirming that.

> * refactored JlinkConfiguration.moduleFinder as a static method. Note
> that we've to create ModuleFinder inside JlinkContiguration because we
> may change modulepaths in constructor (if java.base module is not
> found) and so new ModuleFinder has to be created anyway.
>
There are three places creating a ModuleFinder,
JlinkConfiguration::moduleFinder, JlinkTask::initJlinkConfig, and
JlinkTask::newModuleFinder.   It'd be good to  refactor this e.g. all
call the newModuleFinder method.

Add a new JlinkConfiguration constructor takes a ModuleFinder and have
the initJlinkConfig method to create a ModuleFinder with the default
module path appended, if appropriate.  line 254-265 in JlinkTask - the
check of an empty module path is not needed. Instead it can test if
java.base is found from JlinkConfiguration::finder.

Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189777: jlink --module-path default value and automatic addition of $JAVA_HOME/jmods if java.base is missing

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

Thanks
-Sundar

On 24/10/17, 10:40 PM, mandy chung wrote:

>
>
> On 10/23/17 9:05 PM, Sundararajan Athijegannathan wrote:
>> Hi,
>>
>> * ALL-MODULE-PATH case seems to be handled fine (in  private
>> JlinkConfiguration initJlinkConfig() throws BadArgs { method). When
>> no --module-path is specified, default module path is used and all
>> default observable modules are added as root. I ran the command you
>> mentioned and it worked fine.
>>
>
> That's right.  The default module path is added before initJlinkConfig
> is called.  Thanks for confirming that.
>
>> * refactored JlinkConfiguration.moduleFinder as a static method. Note
>> that we've to create ModuleFinder inside JlinkContiguration because
>> we may change modulepaths in constructor (if java.base module is not
>> found) and so new ModuleFinder has to be created anyway.
>>
> There are three places creating a ModuleFinder,
> JlinkConfiguration::moduleFinder, JlinkTask::initJlinkConfig, and
> JlinkTask::newModuleFinder.   It'd be good to  refactor this e.g. all
> call the newModuleFinder method.
>
> Add a new JlinkConfiguration constructor takes a ModuleFinder and have
> the initJlinkConfig method to create a ModuleFinder with the default
> module path appended, if appropriate.  line 254-265 in JlinkTask - the
> check of an empty module path is not needed. Instead it can test if
> java.base is found from JlinkConfiguration::finder.
>
> Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189777: jlink --module-path default value and automatic addition of $JAVA_HOME/jmods if java.base is missing

Alan Bateman
On 25/10/2017 11:23, Sundararajan Athijegannathan wrote:
> Updated: http://cr.openjdk.java.net/~sundar/8189777/webrev.03/
This looks better. A few comments/questions:

Does the JlinkConfiguration constructor that takes the ModuleFinder
still need the module path? I assume it shouldn't be needed now
(getModulepaths() seems unused). Also is the second constructor needed?
I ask because the second constructor as it callbacks back to JlinkTask
which seems a bit odd.

Is JlinkConfiguration the right place for getDefaultModulePath? It might
be clearer to do that in JlinkTask.

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

Re: RFR 8189777: jlink --module-path default value and automatic addition of $JAVA_HOME/jmods if java.base is missing

Sundararajan Athijegannathan
Second constructor is used by packager (internal) api. I could move
getDefaultModulePath to JlinkTask..

-Sundar

On 25/10/17, 6:25 PM, Alan Bateman wrote:

> On 25/10/2017 11:23, Sundararajan Athijegannathan wrote:
>> Updated: http://cr.openjdk.java.net/~sundar/8189777/webrev.03/
> This looks better. A few comments/questions:
>
> Does the JlinkConfiguration constructor that takes the ModuleFinder
> still need the module path? I assume it shouldn't be needed now
> (getModulepaths() seems unused). Also is the second constructor
> needed? I ask because the second constructor as it callbacks back to
> JlinkTask which seems a bit odd.
>
> Is JlinkConfiguration the right place for getDefaultModulePath? It
> might be clearer to do that in JlinkTask.
>
> -Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189777: jlink --module-path default value and automatic addition of $JAVA_HOME/jmods if java.base is missing

Alan Bateman
On 25/10/2017 14:43, Sundararajan Athijegannathan wrote:
> Second constructor is used by packager (internal) api. I could move
> getDefaultModulePath to JlinkTask..
Ugh, we need to get packager moved away from using this.

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

Re: RFR 8189777: jlink --module-path default value and automatic addition of $JAVA_HOME/jmods if java.base is missing

Mandy Chung
In reply to this post by Sundararajan Athijegannathan
jlink --add-modules ALL-MODULE-PATH does not work in this patch since
the default module is added after the roots set is computed.

AppRuntimeImageBuilder could calls its moduleFinder method (that calls
JlinkTask::newModuleFinder).   packager may already provide the default
module path and we will leave it for them to create the ModuleFinder.

JlinkConfiguration constructor taking ModuleFinder does not need to take
modulepaths and limitmods (I think getModulePaths and getLimitedmods are
not used).   If we change AppRuntimeImageBuilder to pass ModuleFinder to
JlinkConfiguration constructor, tests are the remaining one using the
existing constructor and perhaps we should consider updating the test
and drop the existing constructor.

I agree that getDefaultModulePath should be moved JlinkTask.

Mandy

On 10/25/17 6:43 AM, Sundararajan Athijegannathan wrote:

> Second constructor is used by packager (internal) api. I could move
> getDefaultModulePath to JlinkTask..
>
> -Sundar
>
> On 25/10/17, 6:25 PM, Alan Bateman wrote:
>> On 25/10/2017 11:23, Sundararajan Athijegannathan wrote:
>>> Updated: http://cr.openjdk.java.net/~sundar/8189777/webrev.03/
>> This looks better. A few comments/questions:
>>
>> Does the JlinkConfiguration constructor that takes the ModuleFinder
>> still need the module path? I assume it shouldn't be needed now
>> (getModulepaths() seems unused). Also is the second constructor
>> needed? I ask because the second constructor as it callbacks back to
>> JlinkTask which seems a bit odd.
>>
>> Is JlinkConfiguration the right place for getDefaultModulePath? It
>> might be clearer to do that in JlinkTask.
>>
>> -Alan

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189777: jlink --module-path default value and automatic addition of $JAVA_HOME/jmods if java.base is missing

Sundararajan Athijegannathan
Updated webrev:
http://cr.openjdk.java.net/~sundar/8189777/webrev.04/index.html

-Sundar

On 26/10/17, 10:17 PM, mandy chung wrote:

> jlink --add-modules ALL-MODULE-PATH does not work in this patch since
> the default module is added after the roots set is computed.
>
> AppRuntimeImageBuilder could calls its moduleFinder method (that calls
> JlinkTask::newModuleFinder).   packager may already provide the
> default module path and we will leave it for them to create the
> ModuleFinder.
>
> JlinkConfiguration constructor taking ModuleFinder does not need to
> take modulepaths and limitmods (I think getModulePaths and
> getLimitedmods are not used).   If we change AppRuntimeImageBuilder to
> pass ModuleFinder to JlinkConfiguration constructor, tests are the
> remaining one using the existing constructor and perhaps we should
> consider updating the test and drop the existing constructor.
>
> I agree that getDefaultModulePath should be moved JlinkTask.
>
> Mandy
>
> On 10/25/17 6:43 AM, Sundararajan Athijegannathan wrote:
>> Second constructor is used by packager (internal) api. I could move
>> getDefaultModulePath to JlinkTask..
>>
>> -Sundar
>>
>> On 25/10/17, 6:25 PM, Alan Bateman wrote:
>>> On 25/10/2017 11:23, Sundararajan Athijegannathan wrote:
>>>> Updated: http://cr.openjdk.java.net/~sundar/8189777/webrev.03/
>>> This looks better. A few comments/questions:
>>>
>>> Does the JlinkConfiguration constructor that takes the ModuleFinder
>>> still need the module path? I assume it shouldn't be needed now
>>> (getModulepaths() seems unused). Also is the second constructor
>>> needed? I ask because the second constructor as it callbacks back to
>>> JlinkTask which seems a bit odd.
>>>
>>> Is JlinkConfiguration the right place for getDefaultModulePath? It
>>> might be clearer to do that in JlinkTask.
>>>
>>> -Alan
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189777: jlink --module-path default value and automatic addition of $JAVA_HOME/jmods if java.base is missing

Alan Bateman
On 26/10/2017 18:04, Sundararajan Athijegannathan wrote:
> Updated webrev:
> http://cr.openjdk.java.net/~sundar/8189777/webrev.04/index.html
This is much cleaner and addresses the points that I brought up. So
looks good to me.

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

Re: RFR 8189777: jlink --module-path default value and automatic addition of $JAVA_HOME/jmods if java.base is missing

Mandy Chung
In reply to this post by Sundararajan Athijegannathan


On 10/26/17 10:04 AM, Sundararajan Athijegannathan wrote:
> Updated webrev:
> http://cr.openjdk.java.net/~sundar/8189777/webrev.04/index.html
>

Looks good.  Thanks for incorporating the comments.

Mandy

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8189777: jlink --module-path default value and automatic addition of $JAVA_HOME/jmods if java.base is missing

Sundararajan Athijegannathan
Thanks for your reviews!

-Sundar

On 27/10/17, 3:01 AM, mandy chung wrote:

>
>
> On 10/26/17 10:04 AM, Sundararajan Athijegannathan wrote:
>> Updated webrev:
>> http://cr.openjdk.java.net/~sundar/8189777/webrev.04/index.html
>>
>
> Looks good.  Thanks for incorporating the comments.
>
> Mandy
>