Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Attila Szegedi]

> On Nov 23, 2015, at 4:40 PM, Alan Bateman  wrote:
> 
> 
> 
> On 23/11/2015 15:27, Attila Szegedi wrote:
>> Folks,
>> 
>> I integrated the changes Mandy suggested; please review these (build 
>> related) changes:
>> jdk9 top level: 
>> 
>> jdk9/jdk: 
>> 
>> 
>> For the sake of completeness, the jdk/nashorn changes are here: 
>>  but they have 
>> already been reviewed by Hannes and Sundar; only the above two (jdk9 and 
>> jdk9/jdk) have been modified compared to the original review request.
>> 
>> Sundar was kind enough to verify that JDK9 builds fine with these changes.
>> 
> The jdk repo looks okay (just had to change your link to find it :-)

D’oh. I was doublechecking those links few times and still managed to bungle 
it… thanks for figuring it out.
> 
> In the top-level repo then you've moved jdk.scripting.nashorn from 
> PROVIDER_MODULES to MAIN_MODULES. The reason that we've had it in 
> PROVIDER_MODULES is because we treat it as a service provider module (it 
> provides an implementation of javax.script.ScriptEngineFactory).

Whichever is the stronger criteria for deciding whether to place it in MAIN or 
PROVIDER is fine with me. Intuitively “provider” seems like a weaker category 
(exposes a service provider but doesn’t have its own API), so (without knowing 
the particulars of the use of these *_MODULES variables) it seems to me Mandy’s 
suggestion is correct to reclassify Nashorn into a MAIN module.

Attila.

> 
> -Alan.
> 

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Sundararajan Athijegannathan]

But, in addition to providing service, jdk.scripting.nashorn module also 
"exports" nashorn specific APIs in jdk.nashorn.api.* packages.


-Sundar

On 11/23/2015 9:10 PM, Alan Bateman wrote:



On 23/11/2015 15:27, Attila Szegedi wrote:

Folks,

I integrated the changes Mandy suggested; please review these (build 
related) changes:
jdk9 top level: 

jdk9/jdk: 



For the sake of completeness, the jdk/nashorn changes are here: 
 but they 
have already been reviewed by Hannes and Sundar; only the above two 
(jdk9 and jdk9/jdk) have been modified compared to the original 
review request.


Sundar was kind enough to verify that JDK9 builds fine with these 
changes.



The jdk repo looks okay (just had to change your link to find it :-)

In the top-level repo then you've moved jdk.scripting.nashorn from 
PROVIDER_MODULES to MAIN_MODULES. The reason that we've had it in 
PROVIDER_MODULES is because we treat it as a service provider module 
(it provides an implementation of javax.script.ScriptEngineFactory).


-Alan.

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Alan Bateman]

On 23/11/2015 16:07, Attila Szegedi wrote:

:
Whichever is the stronger criteria for deciding whether to place it in MAIN or 
PROVIDER is fine with me. Intuitively “provider” seems like a weaker category 
(exposes a service provider but doesn’t have its own API), so (without knowing 
the particulars of the use of these *_MODULES variables) it seems to me Mandy’s 
suggestion is correct to reclassify Nashorn into a MAIN module.

We need to do a bit of clean-up in Images.gmk to make things clearer as 
this MAIN vs. PROVIDER topic has caused confusion on a few cases. If we 
can keep the lists separate to the list of modules for the compact 
profile builds then there is no reason why they can't be combined as 
JRE_MODULES.


In this case then jdk.scripting.nashorn.shell is already listed in 
MAIN_MODULES so this will ensure than Nashorn is linked into any 
run-time image that has the the jjs tool/shell. It doesn't matter if 
jdk.scripting.nashorn is listed or not.


-Alan.

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Alan Bateman]

On 23/11/2015 15:43, Sundararajan Athijegannathan wrote:
But, in addition to providing service, jdk.scripting.nashorn module 
also "exports" nashorn specific APIs in jdk.nashorn.api.* packages.

Sure, it could go in either but we mostly treat it as a service provider.

-Alan.

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Mandy Chung]

> On Nov 23, 2015, at 7:40 AM, Alan Bateman  wrote:
> 
> 
> 
> On 23/11/2015 15:27, Attila Szegedi wrote:
>> Folks,
>> 
>> I integrated the changes Mandy suggested; please review these (build 
>> related) changes:
>> jdk9 top level: 
>> 
>> jdk9/jdk: 
>> 
>> 
>> For the sake of completeness, the jdk/nashorn changes are here: 
>>  but they have 
>> already been reviewed by Hannes and Sundar; only the above two (jdk9 and 
>> jdk9/jdk) have been modified compared to the original review request.
>> 
>> Sundar was kind enough to verify that JDK9 builds fine with these changes.
>> 
> The jdk repo looks okay (just had to change your link to find it :-)
> 
> In the top-level repo then you've moved jdk.scripting.nashorn from 
> PROVIDER_MODULES to MAIN_MODULES. The reason that we've had it in 
> PROVIDER_MODULES is because we treat it as a service provider module (it 
> provides an implementation of javax.script.ScriptEngineFactory).

jdk.scripting.nashorn now exports two API packages.  It is no longer a sole 
service provider and so I asked Attila to move it MAIN_MODULE.

Mandy

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Mandy Chung]

> On Nov 23, 2015, at 8:42 AM, Alan Bateman  wrote:
> 
> We need to do a bit of clean-up in Images.gmk to make things clearer as this 
> MAIN vs. PROVIDER topic has caused confusion on a few cases. If we can keep 
> the lists separate to the list of modules for the compact profile builds then 
> there is no reason why they can't be combined as JRE_MODULES.

Agree and we should clean that up to remove the confusion.

Mandy

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Hannes Wallnoefer]

+1 for the Nashorn/Dynalink changes.

The top level changes look good to me but I'd prefer to leave reviews to 
those who are more familiar with the build/modules infrastructure.


Hannes

Am 2015-11-20 um 00:15 schrieb Attila Szegedi:

Please review JDK-8141338 "Move jdk.internal.dynalink package to jdk.dynalink" for 
. This is basically the implementation 
step for integrating JEP 276. This changeset will introduce a new public API that has CCC 
approval (request 8075866), and is also the implementation step of JEP 276 which is now 
targeted for 9 and thus can be integrated.

The changes in this changeset fall into several categories:
- renaming of jdk.internal.dynalink.* package to jdk.dynalink.* package, with 
ripple effects in Nashorn classes that import from these packages
- changes to modules.xml and some build files to accommodate a new public 
module and a dependency of Nashorn on it
- new tests

  I’m sending this webrev to several lists with the following rationales:
- nashorn-dev as the primary users and expected reviewers (also, the Dynalink 
module code lives in jdk9/nashorn/src/jdk.dynalink). A lot of newly added test 
code was contributed by Sundar.
- jigsaw-dev because of modules.xml changes
- jdk9-dev for build changes (build file changes were graciously contributed by 
Erik Joelsson and Sundar)
- core-libs-dev since that’s the designated JEP 276 discussion list.

Nashorn changes: 
top-level jdk9 changes: 


Thanks,
   Attila.

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Attila Szegedi]

Folks,

I integrated the changes Mandy suggested; please review these (build related) 
changes:
jdk9 top level: 
 
jdk9/jdk: 
 

For the sake of completeness, the jdk/nashorn changes are here: 
 but they have already 
been reviewed by Hannes and Sundar; only the above two (jdk9 and jdk9/jdk) have 
been modified compared to the original review request.

Sundar was kind enough to verify that JDK9 builds fine with these changes.

Thanks,
  Attila.

> On Nov 20, 2015, at 4:41 PM, Attila Szegedi  wrote:
> 
> Thanks for pointing out these, Mandy; I will make the changes you suggested.
> 
> Attila.
> 
>> On Nov 20, 2015, at 6:10 AM, Mandy Chung  wrote:
>> 
>> jdk.scripting.nashorn is loaded by the extension class loader.  Is 
>> jdk.dynalink expected to be loaded by the ext. class loader?
>> 
>> You need to edit this file to include the new module:
>>  jdk/make/src/classes/build/tools/module/ext.modules
>> 
>> This is an interim file to map modules to class loader and we will fix it 
>> when the changeset propagates to jake.
>> Mandy
>> 
>>> On Nov 19, 2015, at 7:00 PM, Mandy Chung  wrote:
>>> 
>>> I reviewed the top repo change.
>>> 
>>> modules.xml looks fine.
>>> 
>>> jdk.dynalink  should be in MAIN_MODULES since it has exported APIs.  
>>> jdk.scripting.nashorn should be moved too.  They are not sole service 
>>> providers.  Since you are on this file, can you move jdk.scripting.nashorn 
>>> to MAIN_MODULES as well?
>>> 
>>> Mandy
>>> 
 On Nov 19, 2015, at 3:15 PM, Attila Szegedi  wrote:
 
 Please review JDK-8141338 "Move jdk.internal.dynalink package to 
 jdk.dynalink" for . This 
 is basically the implementation step for integrating JEP 276. This 
 changeset will introduce a new public API that has CCC approval (request 
 8075866), and is also the implementation step of JEP 276 which is now 
 targeted for 9 and thus can be integrated.
 
 The changes in this changeset fall into several categories:
 - renaming of jdk.internal.dynalink.* package to jdk.dynalink.* package, 
 with ripple effects in Nashorn classes that import from these packages
 - changes to modules.xml and some build files to accommodate a new public 
 module and a dependency of Nashorn on it
 - new tests
 
 I’m sending this webrev to several lists with the following rationales:
 - nashorn-dev as the primary users and expected reviewers (also, the 
 Dynalink module code lives in jdk9/nashorn/src/jdk.dynalink). A lot of 
 newly added test code was contributed by Sundar.
 - jigsaw-dev because of modules.xml changes
 - jdk9-dev for build changes (build file changes were graciously 
 contributed by Erik Joelsson and Sundar)
 - core-libs-dev since that’s the designated JEP 276 discussion list.
 
 Nashorn changes:  
 top-level jdk9 changes: 
 
 
 Thanks,
 Attila.
 
>>> 
>> 
> 

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Alan Bateman]

On 23/11/2015 15:27, Attila Szegedi wrote:

Folks,

I integrated the changes Mandy suggested; please review these (build related) 
changes:
jdk9 top level: 

jdk9/jdk: 


For the sake of completeness, the jdk/nashorn changes are here: 
 but they have already 
been reviewed by Hannes and Sundar; only the above two (jdk9 and jdk9/jdk) have been 
modified compared to the original review request.

Sundar was kind enough to verify that JDK9 builds fine with these changes.


The jdk repo looks okay (just had to change your link to find it :-)

In the top-level repo then you've moved jdk.scripting.nashorn from 
PROVIDER_MODULES to MAIN_MODULES. The reason that we've had it in 
PROVIDER_MODULES is because we treat it as a service provider module (it 
provides an implementation of javax.script.ScriptEngineFactory).


-Alan.

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Attila Szegedi]

Thanks for pointing out these, Mandy; I will make the changes you suggested.

Attila.

> On Nov 20, 2015, at 6:10 AM, Mandy Chung  wrote:
> 
> jdk.scripting.nashorn is loaded by the extension class loader.  Is 
> jdk.dynalink expected to be loaded by the ext. class loader?
> 
> You need to edit this file to include the new module:
>   jdk/make/src/classes/build/tools/module/ext.modules
> 
> This is an interim file to map modules to class loader and we will fix it 
> when the changeset propagates to jake.
> Mandy
> 
>> On Nov 19, 2015, at 7:00 PM, Mandy Chung  wrote:
>> 
>> I reviewed the top repo change.
>> 
>> modules.xml looks fine.
>> 
>> jdk.dynalink  should be in MAIN_MODULES since it has exported APIs.  
>> jdk.scripting.nashorn should be moved too.  They are not sole service 
>> providers.  Since you are on this file, can you move jdk.scripting.nashorn 
>> to MAIN_MODULES as well?
>> 
>> Mandy
>> 
>>> On Nov 19, 2015, at 3:15 PM, Attila Szegedi  wrote:
>>> 
>>> Please review JDK-8141338 "Move jdk.internal.dynalink package to 
>>> jdk.dynalink" for . This 
>>> is basically the implementation step for integrating JEP 276. This 
>>> changeset will introduce a new public API that has CCC approval (request 
>>> 8075866), and is also the implementation step of JEP 276 which is now 
>>> targeted for 9 and thus can be integrated.
>>> 
>>> The changes in this changeset fall into several categories:
>>> - renaming of jdk.internal.dynalink.* package to jdk.dynalink.* package, 
>>> with ripple effects in Nashorn classes that import from these packages
>>> - changes to modules.xml and some build files to accommodate a new public 
>>> module and a dependency of Nashorn on it
>>> - new tests
>>> 
>>> I’m sending this webrev to several lists with the following rationales:
>>> - nashorn-dev as the primary users and expected reviewers (also, the 
>>> Dynalink module code lives in jdk9/nashorn/src/jdk.dynalink). A lot of 
>>> newly added test code was contributed by Sundar.
>>> - jigsaw-dev because of modules.xml changes
>>> - jdk9-dev for build changes (build file changes were graciously 
>>> contributed by Erik Joelsson and Sundar)
>>> - core-libs-dev since that’s the designated JEP 276 discussion list.
>>> 
>>> Nashorn changes:  
>>> top-level jdk9 changes: 
>>> 
>>> 
>>> Thanks,
>>> Attila.
>>> 
>> 
> 

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Attila Szegedi]

On Nov 20, 2015, at 4:10 PM, Alan Bateman  wrote:
> 
> On 19/11/2015 23:15, Attila Szegedi wrote:
>> Please review JDK-8141338 "Move jdk.internal.dynalink package to 
>> jdk.dynalink" for . This 
>> is basically the implementation step for integrating JEP 276. This changeset 
>> will introduce a new public API that has CCC approval (request 8075866), and 
>> is also the implementation step of JEP 276 which is now targeted for 9 and 
>> thus can be integrated.
>> 
>> The changes in this changeset fall into several categories:
>> - renaming of jdk.internal.dynalink.* package to jdk.dynalink.* package, 
>> with ripple effects in Nashorn classes that import from these packages
>> - changes to modules.xml and some build files to accommodate a new public 
>> module and a dependency of Nashorn on it
>> - new tests
>> 
>>  I’m sending this webrev to several lists
> Probably build-dev instead of jdk9-dev.
> 
> I'm curious if it's strictly necessary for module jdk.dynalink to be in the 
> nashorn repo now, I assume not but it's probably convenient when working 
> Nashorn.

Exactly; it isn’t necessary, but it has been beneficial for now seeing how 
changes in Dynalink usually had changes in Nashorn to go with them, so having 
them in a single repo for atomic update is a nice property. I guess someone 
might eventually give a bit of a thought to the issue of how modularization 
relates to the JDK forest organization…

> In any case, the module name and the changes to modules.xml look okay to me. 
> As Mandy noted, this isn't a service provider API so in Images.gmk then you 
> can add it to MAIN_MODULES rather than PROVIDER_MODULES.

Yep, will do that as Mandy suggested.

> Your webrevs don't have the changes to the jdk repo but I assume that 
> make/src/classes/build/tools/module/ext.modules has been updated to list 
> jdk.dynalink.

Because I haven’t updated the webrevs since Mandy pointed out that this file 
also has to be changed :-). Will do it soon.

> That is, I assume it needs to be defined to the ext loader because 
> jdk.scripting.nashorn uses it. The ext.modules file is temporary and goes 
> away when we bring in the module system.

I too believe that having it in ext loader is correct. It will hopefully become 
moot once the module system is in.

Thanks,
  Attila.

> -Alan.

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Alan Bateman]

On 19/11/2015 23:15, Attila Szegedi wrote:

Please review JDK-8141338 "Move jdk.internal.dynalink package to jdk.dynalink" for 
. This is basically the implementation 
step for integrating JEP 276. This changeset will introduce a new public API that has CCC 
approval (request 8075866), and is also the implementation step of JEP 276 which is now 
targeted for 9 and thus can be integrated.

The changes in this changeset fall into several categories:
- renaming of jdk.internal.dynalink.* package to jdk.dynalink.* package, with 
ripple effects in Nashorn classes that import from these packages
- changes to modules.xml and some build files to accommodate a new public 
module and a dependency of Nashorn on it
- new tests

  I’m sending this webrev to several lists

Probably build-dev instead of jdk9-dev.

I'm curious if it's strictly necessary for module jdk.dynalink to be in 
the nashorn repo now, I assume not but it's probably convenient when 
working Nashorn.


In any case, the module name and the changes to modules.xml look okay to 
me. As Mandy noted, this isn't a service provider API so in Images.gmk 
then you can add it to MAIN_MODULES rather than PROVIDER_MODULES.


Your webrevs don't have the changes to the jdk repo but I assume that 
make/src/classes/build/tools/module/ext.modules has been updated to list 
jdk.dynalink. That is, I assume it needs to be defined to the ext loader 
because jdk.scripting.nashorn uses it. The ext.modules file is temporary 
and goes away when we bring in the module system.


-Alan.

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Mandy Chung]

jdk.scripting.nashorn is loaded by the extension class loader.  Is jdk.dynalink 
expected to be loaded by the ext. class loader?

You need to edit this file to include the new module:
   jdk/make/src/classes/build/tools/module/ext.modules

This is an interim file to map modules to class loader and we will fix it when 
the changeset propagates to jake.
Mandy

> On Nov 19, 2015, at 7:00 PM, Mandy Chung  wrote:
> 
> I reviewed the top repo change.
> 
> modules.xml looks fine.
> 
> jdk.dynalink  should be in MAIN_MODULES since it has exported APIs.  
> jdk.scripting.nashorn should be moved too.  They are not sole service 
> providers.  Since you are on this file, can you move jdk.scripting.nashorn to 
> MAIN_MODULES as well?
> 
> Mandy
> 
>> On Nov 19, 2015, at 3:15 PM, Attila Szegedi  wrote:
>> 
>> Please review JDK-8141338 "Move jdk.internal.dynalink package to 
>> jdk.dynalink" for . This 
>> is basically the implementation step for integrating JEP 276. This changeset 
>> will introduce a new public API that has CCC approval (request 8075866), and 
>> is also the implementation step of JEP 276 which is now targeted for 9 and 
>> thus can be integrated.
>> 
>> The changes in this changeset fall into several categories:
>> - renaming of jdk.internal.dynalink.* package to jdk.dynalink.* package, 
>> with ripple effects in Nashorn classes that import from these packages
>> - changes to modules.xml and some build files to accommodate a new public 
>> module and a dependency of Nashorn on it
>> - new tests
>> 
>> I’m sending this webrev to several lists with the following rationales:
>> - nashorn-dev as the primary users and expected reviewers (also, the 
>> Dynalink module code lives in jdk9/nashorn/src/jdk.dynalink). A lot of newly 
>> added test code was contributed by Sundar.
>> - jigsaw-dev because of modules.xml changes
>> - jdk9-dev for build changes (build file changes were graciously contributed 
>> by Erik Joelsson and Sundar)
>> - core-libs-dev since that’s the designated JEP 276 discussion list.
>> 
>> Nashorn changes:  
>> top-level jdk9 changes: 
>> 
>> 
>> Thanks,
>> Attila.
>> 
> 

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Mandy Chung]

I reviewed the top repo change.

modules.xml looks fine.

jdk.dynalink  should be in MAIN_MODULES since it has exported APIs.  
jdk.scripting.nashorn should be moved too.  They are not sole service 
providers.  Since you are on this file, can you move jdk.scripting.nashorn to 
MAIN_MODULES as well?

Mandy

> On Nov 19, 2015, at 3:15 PM, Attila Szegedi  wrote:
> 
> Please review JDK-8141338 "Move jdk.internal.dynalink package to 
> jdk.dynalink" for . This is 
> basically the implementation step for integrating JEP 276. This changeset 
> will introduce a new public API that has CCC approval (request 8075866), and 
> is also the implementation step of JEP 276 which is now targeted for 9 and 
> thus can be integrated.
> 
> The changes in this changeset fall into several categories:
> - renaming of jdk.internal.dynalink.* package to jdk.dynalink.* package, with 
> ripple effects in Nashorn classes that import from these packages
> - changes to modules.xml and some build files to accommodate a new public 
> module and a dependency of Nashorn on it
> - new tests
> 
> I’m sending this webrev to several lists with the following rationales:
> - nashorn-dev as the primary users and expected reviewers (also, the Dynalink 
> module code lives in jdk9/nashorn/src/jdk.dynalink). A lot of newly added 
> test code was contributed by Sundar.
> - jigsaw-dev because of modules.xml changes
> - jdk9-dev for build changes (build file changes were graciously contributed 
> by Erik Joelsson and Sundar)
> - core-libs-dev since that’s the designated JEP 276 discussion list.
> 
> Nashorn changes:  
> top-level jdk9 changes: 
> 
> 
> Thanks,
>  Attila.
> 

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Sundararajan Athijegannathan]

+1 on all changes.

-Sundar

On 11/20/2015 12:15 AM, Attila Szegedi wrote:

Please review JDK-8141338 "Move jdk.internal.dynalink package to jdk.dynalink" for 
. This is basically the implementation 
step for integrating JEP 276. This changeset will introduce a new public API that has CCC 
approval (request 8075866), and is also the implementation step of JEP 276 which is now 
targeted for 9 and thus can be integrated.

The changes in this changeset fall into several categories:
- renaming of jdk.internal.dynalink.* package to jdk.dynalink.* package, with 
ripple effects in Nashorn classes that import from these packages
- changes to modules.xml and some build files to accommodate a new public 
module and a dependency of Nashorn on it
- new tests

  I’m sending this webrev to several lists with the following rationales:
- nashorn-dev as the primary users and expected reviewers (also, the Dynalink 
module code lives in jdk9/nashorn/src/jdk.dynalink). A lot of newly added test 
code was contributed by Sundar.
- jigsaw-dev because of modules.xml changes
- jdk9-dev for build changes (build file changes were graciously contributed by 
Erik Joelsson and Sundar)
- core-libs-dev since that’s the designated JEP 276 discussion list.

Nashorn changes: 
top-level jdk9 changes: 


Thanks,
   Attila.