Re: RFR JDK-8151913: Fix module dependencies in java/net tests

[John Jiang]

Hi Amy,
That's case to case.
If a test is using java.logging APIs directly, I declared the module for 
the test. Otherwise, I didn't.

I think that may be more clear.
Although a test is using jdk.httpserver, that doesn't mean it also 
dependents on java.logging.


Best regards,
John Jiang

On 2016/4/28 13:09, Amy Lu wrote:

On 4/28/16 12:50 PM, John Jiang wrote:

Hi,
Please review another webrev: 
http://cr.openjdk.java.net/~jjiang/8151913/webrev.02
The java.httpclient module declaration is removed from all of 
java/net/httpclient tests, even though some ones have to declare 
other modules.


+ * @modules jdk.httpserver
+ *  java.logging

Not necessary to declare java.logging as it’s a dependency of 
jdk.httpserver

(it does not hurt, though)

Please wait for reviewer's feedback...

Thanks,
Amy


Best regards,
John Jiang


On 2016/4/27 23:07, John Jiang wrote:

Hi Alan, Felix,
Thanks for your comments.
Please review the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8151913/webrev.01/


Best regards,
John Jiang


On 2016/4/27 15:08, John Jiang wrote:

Hi,
Please review the fix for explicitly declaring module dependencies 
for java net tests.


Issue: https://bugs.openjdk.java.net/browse/JDK-8151913
Webrev: http://cr.openjdk.java.net/~jjiang/8151913/webrev.00

Best regards,
John Jiang

Re: RFR JDK-8151913: Fix module dependencies in java/net tests

[Amy Lu]

On 4/28/16 12:50 PM, John Jiang wrote:

Hi,
Please review another webrev: 
http://cr.openjdk.java.net/~jjiang/8151913/webrev.02
The java.httpclient module declaration is removed from all of 
java/net/httpclient tests, even though some ones have to declare other 
modules.


+ * @modules jdk.httpserver
+ *  java.logging

Not necessary to declare java.logging as it’s a dependency of jdk.httpserver
(it does not hurt, though)

Please wait for reviewer's feedback...

Thanks,
Amy


Best regards,
John Jiang


On 2016/4/27 23:07, John Jiang wrote:

Hi Alan, Felix,
Thanks for your comments.
Please review the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8151913/webrev.01/


Best regards,
John Jiang


On 2016/4/27 15:08, John Jiang wrote:

Hi,
Please review the fix for explicitly declaring module dependencies 
for java net tests.


Issue: https://bugs.openjdk.java.net/browse/JDK-8151913
Webrev: http://cr.openjdk.java.net/~jjiang/8151913/webrev.00

Best regards,
John Jiang

Re: RFR JDK-8151913: Fix module dependencies in java/net tests

[John Jiang]

Hi,
Please review another webrev: 
http://cr.openjdk.java.net/~jjiang/8151913/webrev.02
The java.httpclient module declaration is removed from all of 
java/net/httpclient tests, even though some ones have to declare other 
modules.


Best regards,
John Jiang


On 2016/4/27 23:07, John Jiang wrote:

Hi Alan, Felix,
Thanks for your comments.
Please review the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8151913/webrev.01/


Best regards,
John Jiang


On 2016/4/27 15:08, John Jiang wrote:

Hi,
Please review the fix for explicitly declaring module dependencies 
for java net tests.


Issue: https://bugs.openjdk.java.net/browse/JDK-8151913
Webrev: http://cr.openjdk.java.net/~jjiang/8151913/webrev.00

Best regards,
John Jiang

Re: Ping - Re: RFR 8078812, Test RMI with client and servers as modules

[Stuart Marks]

Hi Felix,

I finally got a chance to take a good look at this. Again, sorry for the delays. 
I was able to reproduce the failure and find a fix for it. There are also some 
structural issues with the test that will require some improvements.


* * *

First is the way the test fixture is set up. The test now has a single copy of 
the source code for the client, server, and app, and they're compiled once. This 
is good. Unfortunately, the tests modify the filesystem in order to set up 
different cases for the classes' modular structure.


The problem is that makes the tests dependent on each other. The Test-NG 
directives enforce this execution order, but it makes them really hard to work 
with. For example, if I were to disable test #2, this would cause test #3 to 
fail since #3 depends on side effects from test #2. Also, if test #2 were to 
fail, it's hard to debug, since test #3 modifies the fixture after test #2 runs, 
potentially obscuring any problems that #2 might have run into.


Ideally the test should set up a single test fixture and not modify it, allowing 
the different cases to be run independently. Offhand I'm not sure of the best 
way to do this.


One approach that might be worth exploring is to create several different jar 
files from the class files; the jar files would have the classes in different 
modular configurations. Then, each test would invoke a JVM with arguments to put 
the different jar files on the classpath or the module path. I haven't 
investigated this approach, though, so I don't know if it's practical.


Other approaches may be viable as well. It's probably worth discussing and 
prototyping an approach first before you spend too much time implementing any 
particular scheme.


* * *

Moving on to the actual test failure, the message from testUnnamedApp() is as 
follows:


==
Command line: 
[/Users/src/jdk-dev/jdk9-dev/build/macosx-x86_64-normal-server-fastdebug/jdk/bin/java 
-ea -esa -mp mods 
-Djava.rmi.server.codebase=file:/Users/src/jdk-dev/jdk9-dev/jdk/testoutput/JTwork/java/rmi/module/ModuleTest/./mods/mserver/ 
-cp classes testpkg.DummyApp 65071 ]


Error: A JNI error has occurred, please check your installation and try again
Exception in thread "main" java.lang.NoClassDefFoundError: serverpkg/Hello
at java.lang.Class.getDeclaredMethods0(java.base/Native Method)
at java.lang.Class.privateGetDeclaredMethods(java.base/Class.java:2937)
at java.lang.Class.privateGetMethodRecursive(java.base/Class.java:3282)
at java.lang.Class.getMethod0(java.base/Class.java:3252)
at java.lang.Class.getMethod(java.base/Class.java:1961)
at 
sun.launcher.LauncherHelper.validateMainClass(java.base/LauncherHelper.java:648)
at 
sun.launcher.LauncherHelper.checkAndLoadMain(java.base/LauncherHelper.java:499)
Caused by: java.lang.ClassNotFoundException: serverpkg.Hello
	at 
jdk.internal.loader.BuiltinClassLoader.loadClass(java.base/BuiltinClassLoader.java:366)
	at 
jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(java.base/ClassLoaders.java:184)

at java.lang.ClassLoader.loadClass(java.base/ClassLoader.java:419)
... 7 more

test ModuleTest.testUnnamedApp(): failure
java.lang.AssertionError: expected [0] but found [1]
[remainder of stack trace elided]
==

The main class is testpkg.DummyApp, but the launcher fails with 
NoClassDefFoundError on serverpkg/Hello. This was a bit of a puzzle, but I 
finally figured out that this is occurring during the verification of the 
DummyApp class. DummyApp depends on Hello, which can't be found, so it fails 
verification before DummyApp.main() gets called.


It's also strange that it says "A JNI error has occurred" but that might be a 
red herring.


In any case the real issue is that serverpkg.Hello can't be found. The reason it 
can't be found is that it's in a module. That module is on the module path, but 
the module isn't resolved by default. Thus its classes aren't visible to classes 
in the unnamed module.


To add modules to the set of resolved modules, use the -addmods option. In this 
case, testUnnamedApp() has the client and server classes in modules, so it needs 
to have the arguments


-addmods mclient,mserver

added to its command line. Since the mclient module explicitly depends on the 
mserver module, this could instead just e


-addmods mclient

The testUnnamedAppandClient() method has only the server in a named module, so 
it needs to have


-addmods mserver

on its command line.

With these changes, all the tests pass.

* * *

There are a couple other things that could be improved as well. I'll just 
mention them here (so I don't forget) but we should work on these later:


1) The registry and codebase stuff is probably unnecessary and can be removed. 
These might be interesting test cases for the future. It's not clear to me that 
we need to 

Re: Ant JunitTask update to support JDK9 modules

[Alex Buckley]

On 4/27/2016 1:41 PM, Jan Lahoda wrote:

On 27.4.2016 21:30, Alex Buckley wrote:

Your changes skyrocket in complexity because your Ant setup involves
both ant-junit.jar and ant-junit4.jar. The JUnit task documentation
(https://ant.apache.org/manual/Tasks/junit.html) doesn't suggest this is
necessary or desirable. It's either pointless or dangerous to have both
JARs on the classpath, which is why the module system rejects them as


I think it would be useful if you could elaborate on why using both
ant-junit.jar and ant-junit4.jar is pointless or dangerous. What I see
in the jars, the org.apache.tools.ant.taskdefs.optional.junit package is
perfectly partitioned between the jars (there is no class that would be
in both jars). It seems ant-junit4.jar cannot work by itself, without
ant-junit.jar. I am not 100% sure what ant-junit4.jar does, but it seems
it aids in running single test method in a JUnit 4 way. There appears to
be some fallback to JUnit 3 style if the JUnit 4 style is not available,
but it is hard to say if that is sufficient. So dropping (the content
of) ant-junit4.jar would mean at least careful testing, but more
probably a loss of a significant feature.


Perfect partitioning of a split package is uncommon. The common case is 
some overlap of types, leading to obscure mix-and-match problems, so the 
module system rejects a split package unconditionally. Ant's 
redistribution of JUnit JARs (or perhaps its JUnit's own scheme for 
v3-to-v4 migration) is architected in a way that's at odds with the 
module system.


It might be possible to make a case for relaxing the anti-split-package 
provision for automatic modules, since they're coming from the classpath 
world where every artifact is "open" to combination with artifacts 
elsewhere on the path. But that would reduce reliability, since 
generally it's a _feature_ that the module system warns:


java.lang.module.ResolutionException: Modules ant.junit4 and ant.junit 
export package org.apache.tools.ant.taskdefs.optional.junit

to module hamcrest.core

Alex

Re: JDK-8150990: [packager] Research Refactoring Packager

[Kevin Rushforth]

Let's leave it then.

-- Kevin


Chris Bensen wrote:

To be honest I messed up. I noticed when I was building and testing just before 
sending out the review. I kinda liked it after I noticed but I’m good with 
either one.

Chris


  

On Apr 27, 2016, at 1:59 PM, Mandy Chung  wrote:




On Apr 27, 2016, at 1:44 PM, Kevin Rushforth  wrote:

Looks fine to me, too.

Btw, I thought Mandy had earlier suggested jdk.tools.jlink.internal.packager as 
a name, but it's currently jdk.tools.internal.packager (without the jlink). I 
don't care one way or the other and the current one is shorter.
  

I don’t have a preference on either one. This is just temporary and this may go 
away when jlink API is simplified and becomes stable (something to revisit 
later).

Mandy




  

Re: JDK-8150990: [packager] Research Refactoring Packager

[Chris Bensen]

To be honest I messed up. I noticed when I was building and testing just before 
sending out the review. I kinda liked it after I noticed but I’m good with 
either one.

Chris


> On Apr 27, 2016, at 1:59 PM, Mandy Chung  wrote:
> 
> 
>> On Apr 27, 2016, at 1:44 PM, Kevin Rushforth  
>> wrote:
>> 
>> Looks fine to me, too.
>> 
>> Btw, I thought Mandy had earlier suggested jdk.tools.jlink.internal.packager 
>> as a name, but it's currently jdk.tools.internal.packager (without the 
>> jlink). I don't care one way or the other and the current one is shorter.
> 
> I don’t have a preference on either one. This is just temporary and this may 
> go away when jlink API is simplified and becomes stable (something to revisit 
> later).
> 
> Mandy
> 

Re: JDK-8150990: [packager] Research Refactoring Packager

[Kevin Rushforth]

Looks fine to me, too.

Btw, I thought Mandy had earlier suggested 
jdk.tools.jlink.internal.packager as a name, but it's currently 
jdk.tools.internal.packager (without the jlink). I don't care one way or 
the other and the current one is shorter.


-- Kevin


Mandy Chung wrote:

On Apr 27, 2016, at 11:51 AM, Chris Bensen  wrote:

http://cr.openjdk.java.net/~cbensen/JDK-8150990/webrev.00/



This looks okay to me. This is an interim solution to greatly simplify FX coordination with jlink API change during jdk9 development.  


I can sponsor it and push it to jdk9-dev.

Mandy

Re: JDK-8150990: [packager] Research Refactoring Packager

[Mandy Chung]

> On Apr 27, 2016, at 11:51 AM, Chris Bensen  wrote:
> 
> http://cr.openjdk.java.net/~cbensen/JDK-8150990/webrev.00/

This looks okay to me. This is an interim solution to greatly simplify FX 
coordination with jlink API change during jdk9 development.  

I can sponsor it and push it to jdk9-dev.

Mandy

Re: Ant JunitTask update to support JDK9 modules

[Alex Buckley]

Hi Tomas,

Your changes skyrocket in complexity because your Ant setup involves 
both ant-junit.jar and ant-junit4.jar. The JUnit task documentation 
(https://ant.apache.org/manual/Tasks/junit.html) doesn't suggest this is 
necessary or desirable. It's either pointless or dangerous to have both 
JARs on the classpath, which is why the module system rejects them as 
automatic modules when both are on the modulepath. Can you purify your 
Ant setup to have just one of these JARs?


Alex

On 4/27/2016 9:17 AM, Tomas Zezula wrote:

As seen in the modulepath and classpath mixture thread there are many 
possibilities how to execute unit tests.
I've sumarised them on the ant-dev mailing list in the context of Apache Ant in 
http://mail-archives.apache.org/mod_mbox/ant-dev/201604.mbox/%3cafe6c849-0622-44d1-9ff7-3a6ca4832...@oracle.com%3E
As a result of the discussion I've created a pull request for Apache Ant 
JUnitTask to support JDK9 modules:
https://github.com/apache/ant/pull/18

Thanks for comments!
I will link them back to the pull request.

-- Tomas

Re: JDK-8150990: [packager] Research Refactoring Packager

[Chris Bensen]

> On Apr 27, 2016, at 11:09 AM, Alan Bateman  wrote:
> 
> 
> 
> On 27/04/2016 19:00, Chris Bensen wrote:
>> Because the Java Packager lives in the JavaFX repository and jlink lives in 
>> the JDK repository, changes to the jlink API have caused a lot of headaches. 
>> This change introduces a new class for jdk.packager in the jdk.jlink module 
>> to coordinate the jlink API changes. This is an interim solution and will go 
>> away at at some point in the future when the jdk.packager can be brought 
>> into the JDK.
>> 
>> Review: https://java.se.oracle.com/code/cru/CR-JDK9DEV-1361 
>> 
>> JIRA: https://bugs.openjdk.java.net/browse/JDK-8150990 
>> 
>> 
>> 
> Chris - this seems to be an Oracle internal link so better to post the patch 
> on cr.openjdk.java.net.
> 
> -Alan.

OK, I’ll do that right after lunch.

Chris

Re: JDK-8150990: [packager] Research Refactoring Packager

[Alan Bateman]

On 27/04/2016 19:00, Chris Bensen wrote:

Because the Java Packager lives in the JavaFX repository and jlink lives in the 
JDK repository, changes to the jlink API have caused a lot of headaches. This 
change introduces a new class for jdk.packager in the jdk.jlink module to 
coordinate the jlink API changes. This is an interim solution and will go away 
at at some point in the future when the jdk.packager can be brought into the 
JDK.

Review: https://java.se.oracle.com/code/cru/CR-JDK9DEV-1361 

JIRA: https://bugs.openjdk.java.net/browse/JDK-8150990 



Chris - this seems to be an Oracle internal link so better to post the 
patch on cr.openjdk.java.net.


-Alan.

JDK-8150990: [packager] Research Refactoring Packager

[Chris Bensen]

Because the Java Packager lives in the JavaFX repository and jlink lives in the 
JDK repository, changes to the jlink API have caused a lot of headaches. This 
change introduces a new class for jdk.packager in the jdk.jlink module to 
coordinate the jlink API changes. This is an interim solution and will go away 
at at some point in the future when the jdk.packager can be brought into the 
JDK.

Review: https://java.se.oracle.com/code/cru/CR-JDK9DEV-1361 

JIRA: https://bugs.openjdk.java.net/browse/JDK-8150990 


Thanks,
Chris

Re: RFR JDK-8151913: Fix module dependencies in java/net tests

[Felix Yang]

Hi John,

 I think it is necessary to update the years at the beginning of 
copyright.


-Felix
On 2016/4/27 15:08, John Jiang wrote:

Hi,
Please review the fix for explicitly declaring module dependencies for 
java net tests.


Issue: https://bugs.openjdk.java.net/browse/JDK-8151913
Webrev: http://cr.openjdk.java.net/~jjiang/8151913/webrev.00

Best regards,
John Jiang

Re: RFR JDK-8151913: Fix module dependencies in java/net tests

[Alan Bateman]

On 27/04/2016 08:08, John Jiang wrote:

Hi,
Please review the fix for explicitly declaring module dependencies for 
java net tests.


Issue: https://bugs.openjdk.java.net/browse/JDK-8151913
Webrev: http://cr.openjdk.java.net/~jjiang/8151913/webrev.00
Since every test in test/java/net/httpclient/** will require the 
java.httpclient module then we could put a TEST.properties in the 
httpclient directly so that @modules doesn't need to be added to every test.


-Alan

Re: Review request 8154905: Rename jdk.jvmstat.rmi to jdk.jstatd

[Alan Bateman]

On 26/04/2016 21:29, Mandy Chung wrote:

http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8154905/webrev.00/

jdk.jvmstat.rmi does not have any API but instead is a tool module. This patch 
proposes to rename jdk.jvmstat.rmi to jdk.statd to follow JDK module naming 
convention.



This looks okay to me.

-Alan.

Re: RFR JDK-8151913: Fix module dependencies in java/net tests

[Chris Hegarty]

On 27 Apr 2016, at 08:08, John Jiang  wrote:

> Hi,
> Please review the fix for explicitly declaring module dependencies for java 
> net tests.
> 
> Issue: https://bugs.openjdk.java.net/browse/JDK-8151913
> Webrev: http://cr.openjdk.java.net/~jjiang/8151913/webrev.00

This looks ok to me.  Thanks,

-Chris.