Re: RFR(L) : 8176176 : fix @modules in jdk_svc tests

[Alexandre (Shura) Iline]

Igor,

I have looked through a bunch of tests where @modules is changed - that looks 
good.

One minor thing I noticed is that, in 
test/java/lang/management/PlatformLoggingMXBean/TEST.properties you are 
explicitly calling out java.management. You do not have to do that because 
“modules” property is concatenated through the hierarchy.  java.management is 
already listed in test/java/lang/management/TEST.properties.

Shura

> On Mar 13, 2017, at 9:03 PM, Igor Ignatyev <igor.ignat...@oracle.com> wrote:
> 
> Shura,
> 
> Thank you for review, I agree that having separate bugs is more convenient. 
> [1] is new webrev w/ changes only in the files w/ incorrect module dependency 
> declarations.  
> 
> [1] http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/index.html
> 
> Thanks,
> — Igor
> 
>> On Mar 9, 2017, at 5:02 PM, Alexandre (Shura) Iline 
>> <alexandre.il...@oracle.com> wrote:
>> 
>> Igor,
>> 
>> I have reviewed some number tests which change the @modules - they are fine. 
>> 
>> You, however, fix more things with this than missing module dependency 
>> declaration. There is a redesign of line-number-sensitive tests [1] and 
>> other multiple improvements such as in [2]. Would it be more convenient to 
>> have that as separate bugs?
>> 
>> Shura
>> 
>> [1] 
>> http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArgumentValuesTest.java.sdiff.html
>> [2] 
>> http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArrayLengthDumpTest.sh.sdiff.html
>> 
>>> On Mar 7, 2017, at 1:07 PM, Igor Ignatyev <igor.ignat...@oracle.com> wrote:
>>> 
>>> http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html
>>>> 2586 lines changed: 669 ins; 484 del; 1433 mod;
>>> 
>>> Hi all,
>>> 
>>> Could you please review this changeset which fix @modules dependency 
>>> declaration in jdk_svc tests?
>>> there are a couple issues w/ modules in jdk_svc tests: 
>>> - some tests do not specify modules which they depend on 
>>> - modules in TEST.properties is not used in cases there all tests (should) 
>>> have the same @modules directive 
>>> - @modules directive isn't placed according to current convention (before 
>>> the 1st run directive)
>>> 
>>> Since this fix has already touched lots of tests, I have decided to use 
>>> this opportunity and reordered some of jtreg tags as well, so there won’t 
>>> be two massive updates in the tests.
>>> 
>>> Some of our tests are line number sensitive, and then I fixed jtreg 
>>> declaration, they started to fail. It was really hard to find our all line 
>>> number sensitive tests, so I have unified the way we declare that as a part 
>>> of this fix. Please let me know if you prefer to have it done separately.
>>> 
>>> There are two one-liners which, I hope, can simplify review:
>>> [1] shows only the changes which are not in comments. Besides obvious new 
>>> added TEST.properties, there are changes in the following line number 
>>> sensitive tests (which I mentioned before):
>>> test/com/sun/jdi/ArgumentValuesTest.java
>>> test/com/sun/jdi/BreakpointTest.java  
>>> test/com/sun/jdi/FetchLocals.java
>>> test/com/sun/jdi/GetLocalVariables.java
>>> test/com/sun/jdi/GetSetLocalTest.java 
>>> test/com/sun/jdi/LambdaBreakpointTest.java 
>>> test/com/sun/jdi/LineNumberOnBraceTest.java 
>>> test/com/sun/jdi/PopAndStepTest.java
>>> 
>>> [2] shows changes in jtreg tags, it can help to see that almost all changes 
>>> in jtreg tags are either moving of tags which does not affect execution 
>>> order or @modules changes.
>>> 
>>> webrev: http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8176176
>>> Testing: 
>>> - jdk_svc on linux, windows, mac
>>> - checked that all tests which could be executed with full jdk before still 
>>> can be executed with full jdk
>>> 
>>> Thanks,
>>> — Igor
>>> 
>>> [1] $ hg diff -w | grep "^[+-]" | grep -v "^[+-]\s*[*#]"
>>> [2] $ hg diff -w | grep -e "^\(+++ b\)\|\(--- a\)" -e "^[+-]\s*[*#]\s*@"
>> 
> 

RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

[Alexandre (Shura) Iline]

Hi,

Could you please review the suggested fox for: 
https://bugs.openjdk.java.net/browse/JDK-8159523

There has been a bit of discussion on jigsaw-dev, but it perhaps make sense to 
include core-libs-dev.

There have been some fixes since the review was published, so we are now at 
revision #4:
http://cr.openjdk.java.net/~shurailine/8159523/webrev.04/

The background for this fix is sufficiently captured in the original e-mail:
> 1. An attempt was made to enhance jdk.testlibrary.ProcessTools class to 
> support some filtering java and VM options. That implementation came out 
> really overloaded (check executeModularTest(...) in [1]).
> 2. It was suggested that a builder API could be introduced instead. “toolbox” 
> classes from langtools test library were suggested as a start [2].
> 3. Further on, it was agreed that the classes need to be copied into the JDK 
> test library rather than updated in place or somehow else shared between 
> langtools and jdk test libraries.

Quite recently the suggested improvement was discussed with a few folks from 
hotspot team where the same or a similar implementation could be reused. I am 
CCing Igor and Dima to comment.

Thank you

Shura


> Hi,
> 
> Please review a fix for https://bugs.openjdk.java.net/browse/JDK-8159523
> 
> To recap what has happened in the past.
> 
> 1. An attempt was made to enhance jdk.testlibrary.ProcessTools class to 
> support some filtering java and VM options. That implementation came out 
> really overloaded (check executeModularTest(...) in [1]).
> 2. It was suggested that a builder API could be introduced instead. “toolbox” 
> classes from langtools test library were suggested as a start [2].
> 3. Further on, it was agreed that the classes need to be copied into the JDK 
> test library rather than updated in place or somehow else shared between 
> langtools and jdk test libraries.
> 
> I have started porting a few tasks (JavaTask, JavacTask and JarTask) only to 
> realize that there are many questions which may cause design discussions. So, 
> instead, I have rolled back to one class (JavaTask) and the superclasses. If 
> the design is acceptable, more tasks could be added as needed.
> 
> The webrev: http://cr.openjdk.java.net/~shurailine/8159523/webrev.01
> 
> Thank you.
> 
> Shura
> 
> [1] 
> http://cr.openjdk.java.net/~shurailine/8159523/webrev.00/test/lib/testlibrary/jdk/testlibrary/ProcessTools.java.sdiff.html
> 
> [2] 
> http://hg.openjdk.java.net/jdk9/jdk9/langtools/file/8e9e1a2373a4/test/tools/lib/toolbox

Re: [9] RFR: 8167240: Write new tests to cover functionality of existing 'jimage' options

[Alexandre (Shura) Iline]

Denis,

I can see that you have switched to the top level test library with this 
change. With that you are getting more module dependencies than  just 
java.base. First of all, it would probably make sense to build only the classes 
you needed (which would be  jdk.test.lib.process.ProcessTools, I assume), but 
even if you only build that, jdk.test.lib.process.ProcessTools has dependencies 
outside java.base module.

You either have to declare @modules in your test or go back to the 
jdk/test/lib/testlibrary. Then, of course, unneeded module dependencies are 
questionable.

Shura


> On Nov 3, 2016, at 6:29 AM, Denis Kononenko <denis.konone...@oracle.com> 
> wrote:
> 
> Hi,
> 
> I've done some rework accordingly to Alan's and Shura's comments:
> 
> 1) removed overlapped tests from JImageToolTest.java;
> 
> 2) added new tests JImageVerifyTest.java for jimage verify;
> 
> 3) reorganized jtreg's tags;
> 
> The new WEBREV can be found here: 
> http://cr.openjdk.java.net/~dkononenko/8167240/webrev.01/
> 
> Thank you,
> Denis.
> 
> On 06.10.2016 19:37, Denis Kononenko wrote:
>> Hi,
>> 
>> Could someone please review these new tests for jimage utility.
>> 
>> There're 5 new files containing tests to cover use cases for 'info', 'list', 
>> 'extract' and other options. No new tests for 'verify'.
>> 
>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8167240
>> WEBREV: http://cr.openjdk.java.net/~dkononenko/8167240/webrev.00/
>> 
>> 
>> Thank you,
>> Denis.
> 

Re: RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

[Alexandre (Shura) Iline]

Alan,

For me, having module and package parameters separate makes it a lot easier:
public JavaTask addExports(String module, String package, String… targetModules)

Consider this:
.addExports(JAVA_BASE, JDK_MISC, ALL_UNNAMED)
where, needless to say, JAVA_BASE and JDK_MISC  are constants defined in the 
test, or, perhaps, library class and presumably used in other cases.

To me, this is better for obvious reasons, such as code completion in IDE and 
compile-time control over the mistakes. I also will be able to avoid 
concatenation all over the code: 
.addExports(JAVA_BASE + “/“ + JDK_MISC, ALL_UNNAMED)

Similarly, for the same reasons, I prefer
.addExports(JAVA_BASE, JDK_MISC, M1, M2)



What do others think? 

To get everybody on the same page, we are discussing this webrev, which is 
introducing new task builder API to the JDK test library: 
http://cr.openjdk.java.net/~shurailine/8159523/webrev.04

This is the review request:
http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-October/009627.html

Shura




> On Nov 1, 2016, at 3:26 AM, Alan Bateman <alan.bate...@oracle.com> wrote:
> 
> 
> 
> On 01/11/2016 10:23, Alan Bateman wrote:
>> :
>> 
>> .addExports("java.base", "jdk.internal.misc=ALL-UNNAMED")
>> .addOpens("java.base", "jdk.internal.misc=m1,m2")
> Oops, a typo here, I meant this of course:
> 
> .addExports("java.base/jdk.internal.misc", "ALL-UNNAMED")
> .addOpens("java.base/jdk.internal.misc", m1,m2")
> 
> -Alan.

Re: RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

[Alexandre (Shura) Iline]

Alan,

> On Oct 26, 2016, at 12:53 PM, Alexandre (Shura) Iline 
> <alexandre.il...@oracle.com> wrote:
> 
> 
>> On Oct 26, 2016, at 12:00 PM, Alan Bateman <alan.bate...@oracle.com> wrote:
>> 
>> On 26/10/2016 19:33, Alexandre (Shura) Iline wrote:
>> 
>>> :
>>> Like this:
>>> http://cr.openjdk.java.net/~shurailine/8159523/webrev.04/
>>> 
>> It's all subjective but I prefer:
>> 
>> .addExports("java.base/jdk.internal.misc=ALL-UNNAMED")
>> .addExports("java.base/jdk.internal.reflect=ALL-UNNAMED")
>> 
>> to:
>> 
>> .addExports("java.base", "jdk.internal.misc")
>> .addExports("java.base", "jdk.internal.reflect")
>> 
>> only because I can immediately see what the packages are being exported to 
>> all unnamed modules.
> 
> Would this be better then?
> 
> .addExports("java.base", “jdk.internal.misc”, ALL_UNNAMED)
> .addExports("java.base", "jdk.internal.reflect”, ALL_UNNAMED)
> ?

Which syntax would be better?

1. Requiring explicit ALL-UNNAMED
.addExports("java.base", “jdk.internal.misc”, ALL_UNNAMED)
.addExports("java.base", "jdk.internal.reflect”, ALL_UNNAMED) 
2. Treating ALL-UNNAMED as a default
.addExports("java.base", “jdk.internal.misc”)
.addExports("java.base", "jdk.internal.reflect”) 

Same for addReads(…) and, I assume, addOpens(…).

With any of the two, I am in favor of removing addExports(String), 
addReads(String) and addOpens(String) methods. If a complete string parameter 
needs to be passed (such as for negative tests), it could be passed through the 
vmOptions(String …) method.

What do you think?

Shura
> 

> This is what I had in the previous version.
> 
> Shura
> 
>> 
>> -Alan
>> 
>> 
>> 
> 

Re: RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

[Alexandre (Shura) Iline]

> On Oct 26, 2016, at 12:00 PM, Alan Bateman <alan.bate...@oracle.com> wrote:
> 
> On 26/10/2016 19:33, Alexandre (Shura) Iline wrote:
> 
>> :
>> Like this:
>> http://cr.openjdk.java.net/~shurailine/8159523/webrev.04/
>> 
> It's all subjective but I prefer:
> 
> .addExports("java.base/jdk.internal.misc=ALL-UNNAMED")
> .addExports("java.base/jdk.internal.reflect=ALL-UNNAMED")
> 
> to:
> 
> .addExports("java.base", "jdk.internal.misc")
> .addExports("java.base", "jdk.internal.reflect")
> 
> only because I can immediately see what the packages are being exported to 
> all unnamed modules.

Would this be better then?

.addExports("java.base", “jdk.internal.misc”, ALL_UNNAMED)
.addExports("java.base", "jdk.internal.reflect”, ALL_UNNAMED)
?

This is what I had in the previous version.

Shura

> 
> -Alan
> 
> 
> 

Re: RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

[Alexandre (Shura) Iline]

> On Oct 22, 2016, at 3:06 AM, Alan Bateman <alan.bate...@oracle.com> wrote:
> 
> On 21/10/2016 22:07, Alexandre (Shura) Iline wrote:
> 
>> Alan,
>> 
>> This is a newer version:
>> http://cr.openjdk.java.net/~shurailine/8159523/webrev.03/
>> 
> The usages looks good in this version, would be interesting to get other 
> opinions (although this isn't specifically module options, this is really 
> more about replacing usages of ProcessTools).
> 
> One small comment is that I'm not sure about Task defining ALL-UNNAMED. It 
> might be simpler to leave addExports taking two options so that the LHS is 
> more obvious in the tests that use.

Like this:
http://cr.openjdk.java.net/~shurailine/8159523/webrev.04/

Shura

> 
> -Alan
> 

Re: RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

[Alexandre (Shura) Iline]

> On Oct 22, 2016, at 3:06 AM, Alan Bateman <alan.bate...@oracle.com> wrote:
> 
> On 21/10/2016 22:07, Alexandre (Shura) Iline wrote:
> 
>> Alan,
>> 
>> This is a newer version:
>> http://cr.openjdk.java.net/~shurailine/8159523/webrev.03/
>> 
> The usages looks good in this version, would be interesting to get other 
> opinions (although this isn't specifically module options, this is really 
> more about replacing usages of ProcessTools).
> 
> One small comment is that I'm not sure about Task defining ALL-UNNAMED. It 
> might be simpler to leave addExports taking two options so that the LHS is 
> more obvious in the tests that use.

Then, would it be more consistent to change addReads(String) should to take 
source module as the parameter and not the whole string?

Where now there is 
addReads(“module1=module2”)
it would not have to be 
addReads(“module1”, "module2”)

where there it is now 
addReads(“module1=ALL_UNNAMED”) or addReads(“module1”, "ALL_UNNAMED”)
there could be
addReads(“module1”)


If to go this path, addExports(String) should be dropped altogether.

One other things which needs to be changed with that is data providers, where 
they return “module1=module2”, they will need to be returning “module1”, 
“module2” separately. And that is also positive change IMO. Such example is 
present in tools/launcher/modules/addreads/AddReadsTest.java test.

Shura

> 
> -Alan
> 

Re: RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

[Alexandre (Shura) Iline]

Alan,

This is a newer version:
http://cr.openjdk.java.net/~shurailine/8159523/webrev.03/

Shura

> On Oct 17, 2016, at 12:16 AM, Alan Bateman <alan.bate...@oracle.com> wrote:
> 
> On 14/10/2016 23:26, Alexandre (Shura) Iline wrote:
> 
>> Could you please take another look?
>> 
>> I have added more options, and fixed other things you have pointed out. I 
>> have also picked up a couple more tests to cover the newly added methods.
>> 
>> http://cr.openjdk.java.net/~shurailine/8159523/webrev.02/
>> 
>> 
> The usages are easy to read so it looks to me that this effort is coming 
> along well.
> 
> On repeating options again then this version is an improvement but it still 
> doesn't add to the list for usages like .addExports(...).addExports(...). So 
> rather than taking a String[] then I think it would be simpler to take a 
> String for one value and just add to the builder's list.
> 
> I see shouldContain requires specifying the OutputKind when sometimes the 
> test doesn't care if the output is sent to stdout or stderr, maybe there can 
> be a shouldContain overload that searches both streams (we have this in 
> ProcessTools already).
> 
> For the updated tests then I assume `throws Exception` can be removed as 
> there are no unchecked exceptions now. Related is whether TaskError should be 
> TaskException extends RuntimeException instead.
> 
> -Alan

Re: RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

[Alexandre (Shura) Iline]

Could you please take another look?

I have added more options, and fixed other things you have pointed out. I have 
also picked up a couple more tests to cover the newly added methods.

http://cr.openjdk.java.net/~shurailine/8159523/webrev.02/

Shura

> On Oct 10, 2016, at 2:54 AM, Alan Bateman <alan.bate...@oracle.com> wrote:
> 
> On 07/10/2016 21:24, Alexandre (Shura) Iline wrote:
> 
>> Hi,
>> 
>> Please review a fix for https://bugs.openjdk.java.net/browse/JDK-8159523
>> 
>> To recap what has happened in the past.
>> 
>> 1. An attempt was made to enhance jdk.testlibrary.ProcessTools class to 
>> support some filtering java and VM options. That implementation came out 
>> really overloaded (check executeModularTest(...) in [1]).
>> 2. It was suggested that a builder API could be introduced instead. 
>> “toolbox” classes from langtools test library were suggested as a start [2].
>> 3. Further on, it was agreed that the classes need to be copied into the JDK 
>> test library rather than updated in place or somehow else shared between 
>> langtools and jdk test libraries.
>> 
>> I have started porting a few tasks (JavaTask, JavacTask and JarTask) only to 
>> realize that there are many questions which may cause design discussions. 
>> So, instead, I have rolled back to one class (JavaTask) and the 
>> superclasses. If the design is acceptable, more tasks could be added as 
>> needed.
>> 
>> The webrev: http://cr.openjdk.java.net/~shurailine/8159523/webrev.01/
>> 
> At a high-level then the approach looks reasonable, just lots of details to 
> get agreement on before doing a big conversion.
> 
> One thing that jumps out is that JavaTask doesn't seem to handle repeated 
> options, I'll assume that will be fixed.
> 
> In the examples then I suspect that "mainClass" be be clearer than 
> "className". Also "module" might work better than "moduleName". One thing you 
> could look at for the module method is an overload that takes 2 parameters, 
> the module name and main class. Another suggestion is methods such as 
> modulepath (modulePath?) should have an overload that takes a Path for the 
> common cases where tests just specify one path element - if have those then a 
> lot of calls to toString go away.
> 
> One discussion point is whether it should support all options. I note the 
> usage in one of the examples:
> 
> .vmOptions("--upgrade-module-path", UPGRADE_MODS_DIRS.toString())
> .modulepath(MODS_DIR.toString())
> 
> where
> 
> .upgradeModulePath(UPGRADE_MODS)
> .modulePath(MODS_DIRS)
> 
> would be clearer.
> 
> Anyway, these are just some passing comments, I do agree that the tests using 
> ProcessTools could be more readable.
> 
> -Alan.

Re: RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

[Alexandre (Shura) Iline]

> On Oct 11, 2016, at 1:35 PM, Alexandre (Shura) Iline 
> <alexandre.il...@oracle.com> wrote:
> 
> 
>> On Oct 10, 2016, at 2:54 AM, Alan Bateman <alan.bate...@oracle.com> wrote:
>> 
>> On 07/10/2016 21:24, Alexandre (Shura) Iline wrote:
>> 
>>> Hi,
>>> 
>>> Please review a fix for https://bugs.openjdk.java.net/browse/JDK-8159523
>>> 
>>> To recap what has happened in the past.
>>> 
>>> 1. An attempt was made to enhance jdk.testlibrary.ProcessTools class to 
>>> support some filtering java and VM options. That implementation came out 
>>> really overloaded (check executeModularTest(...) in [1]).
>>> 2. It was suggested that a builder API could be introduced instead. 
>>> “toolbox” classes from langtools test library were suggested as a start [2].
>>> 3. Further on, it was agreed that the classes need to be copied into the 
>>> JDK test library rather than updated in place or somehow else shared 
>>> between langtools and jdk test libraries.
>>> 
>>> I have started porting a few tasks (JavaTask, JavacTask and JarTask) only 
>>> to realize that there are many questions which may cause design 
>>> discussions. So, instead, I have rolled back to one class (JavaTask) and 
>>> the superclasses. If the design is acceptable, more tasks could be added as 
>>> needed.
>>> 
>>> The webrev: http://cr.openjdk.java.net/~shurailine/8159523/webrev.01/
>>> 
>> At a high-level then the approach looks reasonable, just lots of details to 
>> get agreement on before doing a big conversion.
>> 
>> One thing that jumps out is that JavaTask doesn't seem to handle repeated 
>> options, I'll assume that will be fixed.
> 
> Do you think there are many cases in the test code with the repeated options? 
> the current implementation allows lists for --add-modules, —add-exports, I 
> would also do it where it makes sense for other options. For negative testing 
> then there is vmOptions, which could contain anything.

Actually —add-exports is implemented incorrectly. I see the point now. 

Thanks.

Shura

> 
> Not that I had anything against supporting multiple options. Just trying to 
> better understand the use case.
> 
>> 
>> In the examples then I suspect that "mainClass" be be clearer than 
>> "className". Also "module" might work better than "moduleName". One thing 
>> you could look at for the module method is an overload that takes 2 
>> parameters, the module name and main class. Another suggestion is methods 
>> such as modulepath (modulePath?)
> 
> Then classPath also, I think. It was “classpath” in the original source, so I 
> added “modulepath”.
> 
>> should have an overload that takes a Path for the common cases where tests 
>> just specify one path element - if have those then a lot of calls to 
>> toString go away.
> 
> The other idea I had is to use varargs for the paths: modulePath(String... 
> modulePath) and modulePath(Path... modulePath). That would allow to avoid the 
> concatenation in the test code.
> 
>> 
>> One discussion point is whether it should support all options. I note the 
>> usage in one of the examples:
>> 
>> .vmOptions("--upgrade-module-path", UPGRADE_MODS_DIRS.toString())
>> .modulepath(MODS_DIR.toString())
>> 
>> where
>> 
>> .upgradeModulePath(UPGRADE_MODS)
>> .modulePath(MODS_DIRS)
>> 
>> would be clearer.
> 
> I will add all the missing options.
> 
> Shura
> 
>> 
>> Anyway, these are just some passing comments, I do agree that the tests 
>> using ProcessTools could be more readable.
>> 
>> -Alan.
> 

Re: RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

[Alexandre (Shura) Iline]

> On Oct 10, 2016, at 2:54 AM, Alan Bateman <alan.bate...@oracle.com> wrote:
> 
> On 07/10/2016 21:24, Alexandre (Shura) Iline wrote:
> 
>> Hi,
>> 
>> Please review a fix for https://bugs.openjdk.java.net/browse/JDK-8159523
>> 
>> To recap what has happened in the past.
>> 
>> 1. An attempt was made to enhance jdk.testlibrary.ProcessTools class to 
>> support some filtering java and VM options. That implementation came out 
>> really overloaded (check executeModularTest(...) in [1]).
>> 2. It was suggested that a builder API could be introduced instead. 
>> “toolbox” classes from langtools test library were suggested as a start [2].
>> 3. Further on, it was agreed that the classes need to be copied into the JDK 
>> test library rather than updated in place or somehow else shared between 
>> langtools and jdk test libraries.
>> 
>> I have started porting a few tasks (JavaTask, JavacTask and JarTask) only to 
>> realize that there are many questions which may cause design discussions. 
>> So, instead, I have rolled back to one class (JavaTask) and the 
>> superclasses. If the design is acceptable, more tasks could be added as 
>> needed.
>> 
>> The webrev: http://cr.openjdk.java.net/~shurailine/8159523/webrev.01/
>> 
> At a high-level then the approach looks reasonable, just lots of details to 
> get agreement on before doing a big conversion.
> 
> One thing that jumps out is that JavaTask doesn't seem to handle repeated 
> options, I'll assume that will be fixed.

Do you think there are many cases in the test code with the repeated options? 
the current implementation allows lists for --add-modules, —add-exports, I 
would also do it where it makes sense for other options. For negative testing 
then there is vmOptions, which could contain anything.

Not that I had anything against supporting multiple options. Just trying to 
better understand the use case.

> 
> In the examples then I suspect that "mainClass" be be clearer than 
> "className". Also "module" might work better than "moduleName". One thing you 
> could look at for the module method is an overload that takes 2 parameters, 
> the module name and main class. Another suggestion is methods such as 
> modulepath (modulePath?)

Then classPath also, I think. It was “classpath” in the original source, so I 
added “modulepath”.

> should have an overload that takes a Path for the common cases where tests 
> just specify one path element - if have those then a lot of calls to toString 
> go away.

The other idea I had is to use varargs for the paths: modulePath(String... 
modulePath) and modulePath(Path... modulePath). That would allow to avoid the 
concatenation in the test code.

> 
> One discussion point is whether it should support all options. I note the 
> usage in one of the examples:
> 
> .vmOptions("--upgrade-module-path", UPGRADE_MODS_DIRS.toString())
> .modulepath(MODS_DIR.toString())
> 
> where
> 
> .upgradeModulePath(UPGRADE_MODS)
> .modulePath(MODS_DIRS)
> 
> would be clearer.

I will add all the missing options.

Shura

> 
> Anyway, these are just some passing comments, I do agree that the tests using 
> ProcessTools could be more readable.
> 
> -Alan.

RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

[Alexandre (Shura) Iline]

Hi,

Please review a fix for https://bugs.openjdk.java.net/browse/JDK-8159523

To recap what has happened in the past.

1. An attempt was made to enhance jdk.testlibrary.ProcessTools class to support 
some filtering java and VM options. That implementation came out really 
overloaded (check executeModularTest(...) in [1]).
2. It was suggested that a builder API could be introduced instead. “toolbox” 
classes from langtools test library were suggested as a start [2].
3. Further on, it was agreed that the classes need to be copied into the JDK 
test library rather than updated in place or somehow else shared between 
langtools and jdk test libraries.

I have started porting a few tasks (JavaTask, JavacTask and JarTask) only to 
realize that there are many questions which may cause design discussions. So, 
instead, I have rolled back to one class (JavaTask) and the superclasses. If 
the design is acceptable, more tasks could be added as needed.

The webrev: http://cr.openjdk.java.net/~shurailine/8159523/webrev.01/

Thank you.

Shura

[1] 
http://cr.openjdk.java.net/~shurailine/8159523/webrev.00/test/lib/testlibrary/jdk/testlibrary/ProcessTools.java.sdiff.html
[2] 
http://hg.openjdk.java.net/jdk9/jdk9/langtools/file/8e9e1a2373a4/test/tools/lib/toolbox

Re: [9] RFR: 8167240: Write new tests to cover functionality of existing 'jimage' options

[Alexandre (Shura) Iline]

Hi, Denis,

By an agreement, @modules should go before any action tag. You use it after 
@build.

Shura

> On Oct 6, 2016, at 9:37 AM, Denis Kononenko <denis.konone...@oracle.com> 
> wrote:
> 
> Hi,
> 
> Could someone please review these new tests for jimage utility.
> 
> There're 5 new files containing tests to cover use cases for 'info', 'list', 
> 'extract' and other options. No new tests for 'verify'.
> 
> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8167240
> WEBREV: http://cr.openjdk.java.net/~dkononenko/8167240/webrev.00/
> 
> 
> Thank you,
> Denis.

Re: RFR: 8163126 Wrong @modules in some of jdk/* tests

[Alexandre (Shura) Iline]

Thank you!

Fixed in place:
http://cr.openjdk.java.net/~shurailine/8163126/webrev.00/test/jdk/security/jarsigner/Spec.java.sdiff.html

Shura

> On Aug 16, 2016, at 6:48 PM, Wang Weijun <weijun.w...@oracle.com> wrote:
> 
> 
>> On Aug 17, 2016, at 9:26 AM, Alexandre (Shura) Iline 
>> <alexandre.il...@oracle.com> wrote:
>> 
>> Before the suggested fix, the test in question would fail on a system with 
>> no jdk.crypto.pkcs11. That could be emulated by:
>> $ jtreg ... -javaoptions:"-limitmods jdk.jartool" 
>> jdk/security/jarsigner/Spec.java
>> ...
>> FAILED: jdk/security/jarsigner/Spec.java
>> ...
>> $ jtreg ... -javaoptions:"-limitmods jdk.jartool,jdk.crypto.pkcs11" 
>> jdk/security/jarsigner/Spec.java
>> ...
>> Passed: jdk/security/jarsigner/Spec.java
>> ...
>> $ 
> 
> Thanks very much for the detailed answer.
> 
> Just a small suggestion: Can you try using jdk.crypto.ec module instead? It 
> also provides all EC-related algorithms and it is available and loaded 
> out-of-box on all platforms?
> 
> --Max
> 

Re: RFR: 8163126 Wrong @modules in some of jdk/* tests

[Alexandre (Shura) Iline]

Hi, Max.

Excerpt from JTReg documentation:
=
@modules [/]+
Express a dependence on a modules in the system being tested, and optionally, 
on selected internal packages in some or all of those modules.

If no dependencies are specified explicitly, a default set of dependencies will 
be read from the modules entry in test suite configuration files.

A test will not be run if the system being tested does not contain all of the 
specified modules.

Note: Specifying an entry that includes a dependence on an internal package is 
essentially equivalent to using -XaddExports:/ALL-UNNAMED when 
the test is compiled and run.
=


Before the suggested fix, the test in question would fail on a system with no 
jdk.crypto.pkcs11. That could be emulated by:
$ jtreg ... -javaoptions:"-limitmods jdk.jartool" 
jdk/security/jarsigner/Spec.java
...
FAILED: jdk/security/jarsigner/Spec.java
...
$ jtreg ... -javaoptions:"-limitmods jdk.jartool,jdk.crypto.pkcs11" 
jdk/security/jarsigner/Spec.java
...
Passed: jdk/security/jarsigner/Spec.java
...
$ 

With my fix, the test will not be selected for execution when there is no 
jdk.crypto.pkcs11:
$ jtreg ... -javaoptions:"-limitmods jdk.jartool" 
jdk/security/jarsigner/Spec.java
...
Test results: no tests selected
…
$

Shura



> On Aug 16, 2016, at 5:30 PM, Weijun Wang <weijun.w...@oracle.com> wrote:
> 
> Hi Shura
> 
> I am looking at test/jdk/security/jarsigner/Spec.java.
> 
> IMHO, even on a Solaris, without the SunPKCS11 provider at runtime, this test 
> should be able to find Signature and MessageDigest implementations from the 
> SunRsaSign and SUN provider.
> 
> Is the new @modules dependency necessary? In fact, I am curious how you note 
> this test. Does it fail with some special settings?
> 
> Thanks
> Max
> 
> On 8/17/2016 1:17, Alexandre (Shura) Iline wrote:
>> Hi.
>> 
>> Please review fixes related to module dependencies in a few jdk tests:
>> http://cr.openjdk.java.net/~shurailine/8163126/webrev.00/index.html
>> 
>> The review contains a few cases where jdk.zipfs is added to the module list. 
>> This is happening because all TestNG tests which use compiler API require 
>> jdk.zipfs to be able to read the testng jar. This is unfortunate but there 
>> is no easy way around that.
>> 
>> Shura
>> 
>> 
>> 

RFR: 8163126 Wrong @modules in some of jdk/* tests

[Alexandre (Shura) Iline]

Hi.

Please review fixes related to module dependencies in a few jdk tests:
http://cr.openjdk.java.net/~shurailine/8163126/webrev.00/index.html

The review contains a few cases where jdk.zipfs is added to the module list. 
This is happening because all TestNG tests which use compiler API require 
jdk.zipfs to be able to read the testng jar. This is unfortunate but there is 
no easy way around that. 

Shura

Re: Review request 8136930: Simplify use of module-system options by custom launchers

[Alexandre (Shura) Iline]

> On Aug 11, 2016, at 1:06 PM, Jonathan Gibbons <jonathan.gibb...@oracle.com> 
> wrote:
> 
> Shura,
> 
> Such is the nature of things, I would expect there are some instances of the 
> old style options inbound from separate changesets under development or in 
> review. I would expect that we will do a separate additional pass over tests 
> before we remove support for the old style options.

Of course.

And also, to be completely sure that the tests using new options everywhere, a 
test run needs to be performed for all tests and failures need to be looked on. 
Which we will do once the promotion build is out.

Shura

> 
> -- Jon
> 
> On 08/11/2016 09:16 AM, Alexandre (Shura) Iline wrote:
>>> On Aug 11, 2016, at 7:57 AM, Mandy Chung <mandy.ch...@oracle.com> wrote:
>>> 
>>> Tests in jdk, hotspot, langtools, jaxp and nashorn repos have been 
>>> converted to use the new options when this fix was pushed.
>>> 
>>> Any test changes in jdk9/client or jdk9/hs that are not yet in jdk9/dev 
>>> would require changes.
>> Just to be absolutely clear, you have scanned the source files for 
>> appearances of the options which are to go away, correct? You have searched 
>> java and shell files, I assume.
>> 
>> I hope it covers all the cases. If there is anything else we should be able 
>> to quickly identify it after running the tests.
>> 
>> Shura
>> 
>> 
>>> Mandy
>>> 
>>>> On Aug 11, 2016, at 7:47 AM, Alexandre (Shura) Iline 
>>>> <alexandre.il...@oracle.com> wrote:
>>>> 
>>>> Hi, Mandy.
>>>> 
>>>> Could you help to identify what tests in the JTreg suite(s) require more 
>>>> work to switch to the new options?
>>>> 
>>>> Or the other way around: which tests have been fixed with this commit?
>>>> 
>>>> Thank you.
>>>> 
>>>> 
>>>>> On Aug 5, 2016, at 1:11 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
>>>>> 
>>>>> This patch renames the module-system options to GNU-style as specified
>>>>> in JEP 293 [1] (see below for the new proposed option names).  This
>>>>> addresses the problems discussed in [2] that the launcher will pass
>>>>> the module-system options down to the VM in the form of =.
>>>>> This provides a consistent way to configure the module system and
>>>>> simplify use of module-system options by custom launcher.  This patch
>>>>> also updates several JDK tools including jlink, jmod, jimage, jar,
>>>>> javac, javap, javadoc, javah, jdeps such that the GNU-style
>>>>> module-system options are consistent across all tools.
>>>>> 
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8136930/gnu-options/webrev.00/
>>>>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8136930/gnu-options/webrev-langtools.00/
>>>>> 
>>>>> Harold has posted a separate code review for the hotspot change [3].
>>>>> webrev.00 includes changes in all repos except hotspot.  
>>>>> webrev-langtools.00
>>>>> includes the changes in langtools repo (this may be convenient for
>>>>> those who wants to review langtools change only).
>>>>> 
>>>>> Note that existing -cp and -classpath have no change and continue to
>>>>> be supported.  New long form option `--class-path` and `--help` are
>>>>> added to java, javac, and other tools where applicable.
>>>>> 
>>>>> For transition, all old options except -listmods continue to be
>>>>> supported by the java launcher, javac, javadoc, javap, javah.
>>>>> I propose to remove the old options in two weeks after this patch
>>>>> is promoted (i.e. two promoted builds to go through the transition).
>>>>> 
>>>>> Mandy
>>>>> 
>>>>> [1] http://openjdk.java.net/jeps/293
>>>>> [2] 
>>>>> http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-June/008079.html
>>>>> [3] 
>>>>> http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-July/008715.html
>>>>> 
>>>>> Existing OptionsNew Options
>>>>> 
>>>>> -addmods--add-modules
>>>>> -classpath | -cp | --classpath  -classpath | -cp | --class-path
>>>>> -limitmods  --limit-modules
>>>>> -listmods   --list-modules
>>>>> -m  --module | -m
>>>>> -modulepath | -mp | --modulepath--module-path | -p
>>>>> -modulesourcepath   --module-source-path
>>>>> --plugin-module-path(no change)
>>>>> -processormodulepath--processor-module-path
>>>>> -upgrademodulepath  --upgrade-module-path
>>>>> -XaddExports--add-exports
>>>>> -XaddReads  --add-reads
>>>>> -Xpatch --patch-module
>>>>> -bootclasspath  --boot-class-path | -bootclasspath
>>>>> -processorpath  --processor-path  | -processor-path
>>>>> -sourcepath --source-path | -sourcepath
>>>>> -system --system
>>>>> -release--release
> 

Re: Review request 8136930: Simplify use of module-system options by custom launchers

[Alexandre (Shura) Iline]

> On Aug 11, 2016, at 7:57 AM, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> Tests in jdk, hotspot, langtools, jaxp and nashorn repos have been converted 
> to use the new options when this fix was pushed.
> 
> Any test changes in jdk9/client or jdk9/hs that are not yet in jdk9/dev would 
> require changes.

Just to be absolutely clear, you have scanned the source files for appearances 
of the options which are to go away, correct? You have searched java and shell 
files, I assume.

I hope it covers all the cases. If there is anything else we should be able to 
quickly identify it after running the tests.

Shura


> 
> Mandy
> 
>> On Aug 11, 2016, at 7:47 AM, Alexandre (Shura) Iline 
>> <alexandre.il...@oracle.com> wrote:
>> 
>> Hi, Mandy.
>> 
>> Could you help to identify what tests in the JTreg suite(s) require more 
>> work to switch to the new options?
>> 
>> Or the other way around: which tests have been fixed with this commit?
>> 
>> Thank you.
>> 
>> 
>>> On Aug 5, 2016, at 1:11 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
>>> 
>>> This patch renames the module-system options to GNU-style as specified
>>> in JEP 293 [1] (see below for the new proposed option names).  This
>>> addresses the problems discussed in [2] that the launcher will pass 
>>> the module-system options down to the VM in the form of =.
>>> This provides a consistent way to configure the module system and
>>> simplify use of module-system options by custom launcher.  This patch
>>> also updates several JDK tools including jlink, jmod, jimage, jar,
>>> javac, javap, javadoc, javah, jdeps such that the GNU-style 
>>> module-system options are consistent across all tools.
>>> 
>>> Webrev:
>>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8136930/gnu-options/webrev.00/
>>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8136930/gnu-options/webrev-langtools.00/
>>> 
>>> Harold has posted a separate code review for the hotspot change [3].
>>> webrev.00 includes changes in all repos except hotspot.  webrev-langtools.00
>>> includes the changes in langtools repo (this may be convenient for
>>> those who wants to review langtools change only).
>>> 
>>> Note that existing -cp and -classpath have no change and continue to
>>> be supported.  New long form option `--class-path` and `--help` are 
>>> added to java, javac, and other tools where applicable.
>>> 
>>> For transition, all old options except -listmods continue to be 
>>> supported by the java launcher, javac, javadoc, javap, javah.  
>>> I propose to remove the old options in two weeks after this patch
>>> is promoted (i.e. two promoted builds to go through the transition).
>>> 
>>> Mandy
>>> 
>>> [1] http://openjdk.java.net/jeps/293
>>> [2] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-June/008079.html
>>> [3] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-July/008715.html
>>> 
>>> Existing OptionsNew Options
>>> 
>>> -addmods--add-modules
>>> -classpath | -cp | --classpath  -classpath | -cp | --class-path
>>> -limitmods  --limit-modules
>>> -listmods   --list-modules
>>> -m  --module | -m
>>> -modulepath | -mp | --modulepath--module-path | -p
>>> -modulesourcepath   --module-source-path
>>> --plugin-module-path(no change)
>>> -processormodulepath--processor-module-path
>>> -upgrademodulepath  --upgrade-module-path
>>> -XaddExports--add-exports
>>> -XaddReads  --add-reads
>>> -Xpatch --patch-module
>>> -bootclasspath  --boot-class-path | -bootclasspath
>>> -processorpath  --processor-path  | -processor-path
>>> -sourcepath --source-path | -sourcepath
>>> -system --system
>>> -release--release
>> 
> 

Re: Review request 8136930: Simplify use of module-system options by custom launchers

[Alexandre (Shura) Iline]

Hi, Mandy.

Could you help to identify what tests in the JTreg suite(s) require more work 
to switch to the new options?

Or the other way around: which tests have been fixed with this commit?

Thank you.


> On Aug 5, 2016, at 1:11 PM, Mandy Chung  wrote:
> 
> This patch renames the module-system options to GNU-style as specified
> in JEP 293 [1] (see below for the new proposed option names).  This
> addresses the problems discussed in [2] that the launcher will pass 
> the module-system options down to the VM in the form of =.
> This provides a consistent way to configure the module system and
> simplify use of module-system options by custom launcher.  This patch
> also updates several JDK tools including jlink, jmod, jimage, jar,
> javac, javap, javadoc, javah, jdeps such that the GNU-style 
> module-system options are consistent across all tools.
> 
> Webrev:
> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8136930/gnu-options/webrev.00/
> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8136930/gnu-options/webrev-langtools.00/
> 
> Harold has posted a separate code review for the hotspot change [3].
> webrev.00 includes changes in all repos except hotspot.  webrev-langtools.00
> includes the changes in langtools repo (this may be convenient for
> those who wants to review langtools change only).
> 
> Note that existing -cp and -classpath have no change and continue to
> be supported.  New long form option `--class-path` and `--help` are 
> added to java, javac, and other tools where applicable.
> 
> For transition, all old options except -listmods continue to be 
> supported by the java launcher, javac, javadoc, javap, javah.  
> I propose to remove the old options in two weeks after this patch
> is promoted (i.e. two promoted builds to go through the transition).
> 
> Mandy
> 
> [1] http://openjdk.java.net/jeps/293
> [2] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-June/008079.html
> [3] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-July/008715.html
> 
> Existing OptionsNew Options
> 
> -addmods--add-modules
> -classpath | -cp | --classpath  -classpath | -cp | --class-path
> -limitmods  --limit-modules
> -listmods   --list-modules
> -m  --module | -m
> -modulepath | -mp | --modulepath--module-path | -p
> -modulesourcepath   --module-source-path
> --plugin-module-path(no change)
> -processormodulepath--processor-module-path
> -upgrademodulepath  --upgrade-module-path
> -XaddExports--add-exports
> -XaddReads  --add-reads
> -Xpatch --patch-module
> -bootclasspath  --boot-class-path | -bootclasspath
> -processorpath  --processor-path  | -processor-path
> -sourcepath --source-path | -sourcepath
> -system --system
> -release--release

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Alexandre (Shura) Iline]

Valerie, could you sponsor the patch for me?

Shura

> On Jul 6, 2016, at 10:08 AM, Valerie Peng <valerie.p...@oracle.com> wrote:
> 
> 
> Changes look fine to me.
> Thanks,
> Valerie
> 
> On 7/5/2016 2:31 PM, Mandy Chung wrote:
>>> On Jul 5, 2016, at 1:53 PM, Alexandre (Shura) Iline 
>>> <alexandre.il...@oracle.com> wrote:
>>> 
>>> 
>>>> On Jul 5, 2016, at 1:36 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
>>>> 
>>>> 
>>>>> On Jul 5, 2016, at 12:42 PM, Alexandre (Shura) Iline 
>>>>> <alexandre.il...@oracle.com> wrote:
>>>>> 
>>>>> This made sense, than you, Mandy.
>>>>> 
>>>>> Please review new version:
>>>>> http://cr.openjdk.java.net/~shurailine/8158670/webrev.02/
>>>> You can use Layer::findModule instead of Configuration::findModule.
>>> That is correct. Changed in place.
>>> 
>>>> You can also use List::equals.
>>> I am assuming you are suggesting to use List::equals in the bottom part of 
>>> the test where the expected result is compared with the actual list of 
>>> providers. The whole reason I redid that section to provide more 
>>> information in the jtr file, both for a case of a failure and to find out 
>>> what providers were actually expected for given configuration. I do not see 
>>> how List:equals help me with that. Information on size mismatch is useful, 
>>> and also the information on unexpected provider name.
>> What you have is fine.  The information is useful to help diagnosis.  The 
>> alternative I was thinking is to check List::equals and if not equals, do 
>> line-108-117.  It’s a minor thing and up to you.
>> 
>> Mandy
> 

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Alexandre (Shura) Iline]

> On Jul 5, 2016, at 1:36 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> 
>> On Jul 5, 2016, at 12:42 PM, Alexandre (Shura) Iline 
>> <alexandre.il...@oracle.com> wrote:
>> 
>> This made sense, than you, Mandy.
>> 
>> Please review new version:
>> http://cr.openjdk.java.net/~shurailine/8158670/webrev.02/
> 
> You can use Layer::findModule instead of Configuration::findModule.

That is correct. Changed in place.

> 
> You can also use List::equals.

I am assuming you are suggesting to use List::equals in the bottom part of the 
test where the expected result is compared with the actual list of providers. 
The whole reason I redid that section to provide more information in the jtr 
file, both for a case of a failure and to find out what providers were actually 
expected for given configuration. I do not see how List:equals help me with 
that. Information on size mismatch is useful, and also the information on 
unexpected provider name.

Shura 

> 
> Mandy

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Alexandre (Shura) Iline]

Jon,

> On Jul 1, 2016, at 6:44 PM, Jonathan Gibbons <jonathan.gibb...@oracle.com> 
> wrote:
> 
> The test can also report which providers it is testing and/or skipping, so 
> that anyone checking the .jtr file can verify the behavior.

I have added some debug output, pls take a look.
http://cr.openjdk.java.net/~shurailine/8158670/webrev.02/

> 
> Maybe it fails if expected modules or providers are not found.

In the current state the test would work with any module availabilities - it 
simply checks that order and presence of security providers is consistent with 
the module availabilities.

Shura

> 
> -- Jon
> 
> 
> On 06/29/2016 10:50 AM, Mandy Chung wrote:
>> Valerie’s suggestion is a good one.  The test will require a minimum image 
>> with cross-platform security providers to run the test while it can still 
>> verify other platform-specific providers.
>> 
>> Mandy
>> 
>>> On Jun 29, 2016, at 10:41 AM, Valerie Peng <valerie.p...@oracle.com> wrote:
>>> 
>>> 
>>> It's not like the test silently passes as the test still covers the 
>>> cross-platform modules.
>>> The way I view this is that the platform=specific modules are "optional" 
>>> and we update the expected result by detecting their presence (or the not). 
>>> It's not a hack or workaround, but rather an enhancement for the test to 
>>> handle different images.
>>> 
>>> Just my .02,
>>> Valerie
>>> 
>>> On 6/29/2016 10:22 AM, Alexandre (Shura) Iline wrote:
>>>>> On Jun 28, 2016, at 5:22 PM, Valerie Peng <valerie.p...@oracle.com> wrote:
>>>>> 
>>>>> 
>>>>> One of the purpose of this test is to test the ordering (see the initial 
>>>>> bug which this test is for: JDK-6997010).
>>>>> 
>>>>> The original test already detects the OS and will skip certain providers 
>>>>> accordingly.
>>>>> Instead of splitting the test into multiple platform-specific tests, 
>>>>> maybe we can keep the original test but add module-presence checking? Is 
>>>>> there API available to query if certain module are present?
>>>> ModuleFinder.ofSystem().find(String).
>>>> 
>>>> We can have only the cross-platform modules listed in @modules and make 
>>>> the test to pass silently if the required platform-specific modules are 
>>>> not present.
>>>> 
>>>> So, for example, on windows, if the test would be executed against an 
>>>> image which have no jdk.crypto.mscapi, the test will not run any checks 
>>>> and report pass.
>>>> 
>>>> This would help to avoid the additional test creation, but it will add 
>>>> another silently passing test, which is less clean.
>>>> 
>>>> Mandy?
>>>> 
>>>> Shura
>>>> 
>>>>> If yes, then we can leave out the platform-specific providers from the 
>>>>> @modules line and skip the providers if either the OS does not match or 
>>>>> the module is not present.
>>>>> 
>>>>> If we can't query what modules are available, then we may have to think 
>>>>> of something else.
>>>>> Valerie
>>>>> 
>>>>> On 6/27/2016 12:27 PM, Mandy Chung wrote:
>>>>>> I’m including security-dev which would be a better list to review this 
>>>>>> test fix.
>>>>>> 
>>>>>> Valerie,
>>>>>>Does this test have to be order-sensitive?  I think this test would 
>>>>>> be cleaner to make it order-insensitive and simply test the security 
>>>>>> provider initialization.
>>>>>> 
>>>>>> See my comments below.
>>>>>> 
>>>>>>> On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline 
>>>>>>> <alexandre.il...@oracle.com> wrote:
>>>>>>> 
>>>>>>> Hi.
>>>>>>> 
>>>>>>> Please take a look on a suggested for for the 
>>>>>>> java/lang/SecurityManager/CheckSecurityProvider.java test.
>>>>>>> 
>>>>>>> The test in question depend on a list of modules, some of them are 
>>>>>>> platform-specific. Listing all the dependencies in one test is causing 
>>>>>>> the test to be skipped on every platform. In an offline conversation it 
>>>>>>> was decided that it is better to split this tests into a few tests to 
>>>>>>> declare the per-platform module dependencies.
>>>>>>> 
>>>>>>> The bug: https://bugs.openjdk.java.net/browse/JDK-8158670
>>>>>>> The suggested fix: 
>>>>>>> http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/
>>>>>> The copyright header start year of the new tests should be 2016.
>>>>>> 
>>>>>> I would suggest to make CheckSecurityProvide a platform-neutral test, 
>>>>>> i.e.,
>>>>>> - drop @requires
>>>>>> - make line 94-97 to ignore the platform-dependent provider if it’s 
>>>>>> present in the white list
>>>>>> 
>>>>>> If we could make this test order-insensitive, it’d be cleaner to 
>>>>>> maintain a platform-neutral list of security providers and one list for 
>>>>>> the platform-dependent security providers for each platform.  Just an 
>>>>>> idea.
>>>>>> 
>>>>>> Mandy
>>>>>> 
> 

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Alexandre (Shura) Iline]

This made sense, than you, Mandy.

Please review new version:
http://cr.openjdk.java.net/~shurailine/8158670/webrev.02/

Shura

> On Jul 2, 2016, at 3:26 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> 
>> On Jul 1, 2016, at 6:20 PM, Alexandre (Shura) Iline 
>> <alexandre.il...@oracle.com> wrote:
>> 
>> Please review the new version of the fix.
>> http://cr.openjdk.java.net/~shurailine/8158670/webrev.01/
> 
> This looks much better.  Small comment: you can use Layer::findModule and 
> also Optional::ifPresent, like this:
> 
> boot.findModule("jdk.crypto.ucrypto”)
>.ifPresent(m -> 
> expected.add("com.oracle.security.ucrypto.UcryptoProvider”));
> 
> Mandy
> 

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Alexandre (Shura) Iline]

> On Jul 1, 2016, at 6:20 PM, Alexandre (Shura) Iline 
> <alexandre.il...@oracle.com> wrote:
> 
> Pleas review the new version of the fix.
> http://cr.openjdk.java.net/~shurailine/8158670/webrev.01/
> 
> I have executed the changed test successfully on linux, windows, mac os x and 
> solaris.

I have also executed it manually on my laptop with most relevant values for 
-limitmods. 

Shura

> 
> Shura
> 
>> On Jun 29, 2016, at 10:43 AM, Alexandre (Shura) Iline 
>> <alexandre.il...@oracle.com> wrote:
>> 
>> 
>>> On Jun 29, 2016, at 10:41 AM, Valerie Peng <valerie.p...@oracle.com> wrote:
>>> 
>>> 
>>> It's not like the test silently passes as the test still covers the 
>>> cross-platform modules.
>>> The way I view this is that the platform=specific modules are "optional" 
>>> and we update the expected result by detecting their presence (or the not). 
>>> It's not a hack or workaround, but rather an enhancement for the test to 
>>> handle different images.
>> 
>> Oh … that makes more sense. I mis-understood it originally.
>> 
>> Let me fix it like this.
>> 
>> Shura
>> 
>>> 
>>> Just my .02,
>>> Valerie
>>> 
>>> On 6/29/2016 10:22 AM, Alexandre (Shura) Iline wrote:
>>>>> On Jun 28, 2016, at 5:22 PM, Valerie Peng <valerie.p...@oracle.com> wrote:
>>>>> 
>>>>> 
>>>>> One of the purpose of this test is to test the ordering (see the initial 
>>>>> bug which this test is for: JDK-6997010).
>>>>> 
>>>>> The original test already detects the OS and will skip certain providers 
>>>>> accordingly.
>>>>> Instead of splitting the test into multiple platform-specific tests, 
>>>>> maybe we can keep the original test but add module-presence checking? Is 
>>>>> there API available to query if certain module are present?
>>>> ModuleFinder.ofSystem().find(String).
>>>> 
>>>> We can have only the cross-platform modules listed in @modules and make 
>>>> the test to pass silently if the required platform-specific modules are 
>>>> not present.
>>>> 
>>>> So, for example, on windows, if the test would be executed against an 
>>>> image which have no jdk.crypto.mscapi, the test will not run any checks 
>>>> and report pass.
>>>> 
>>>> This would help to avoid the additional test creation, but it will add 
>>>> another silently passing test, which is less clean.
>>>> 
>>>> Mandy?
>>>> 
>>>> Shura
>>>> 
>>>>> If yes, then we can leave out the platform-specific providers from the 
>>>>> @modules line and skip the providers if either the OS does not match or 
>>>>> the module is not present.
>>>>> 
>>>>> If we can't query what modules are available, then we may have to think 
>>>>> of something else.
>>>>> Valerie
>>>>> 
>>>>> On 6/27/2016 12:27 PM, Mandy Chung wrote:
>>>>>> I’m including security-dev which would be a better list to review this 
>>>>>> test fix.
>>>>>> 
>>>>>> Valerie,
>>>>>>  Does this test have to be order-sensitive?  I think this test would be 
>>>>>> cleaner to make it order-insensitive and simply test the security 
>>>>>> provider initialization.
>>>>>> 
>>>>>> See my comments below.
>>>>>> 
>>>>>>> On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline 
>>>>>>> <alexandre.il...@oracle.com> wrote:
>>>>>>> 
>>>>>>> Hi.
>>>>>>> 
>>>>>>> Please take a look on a suggested for for the 
>>>>>>> java/lang/SecurityManager/CheckSecurityProvider.java test.
>>>>>>> 
>>>>>>> The test in question depend on a list of modules, some of them are 
>>>>>>> platform-specific. Listing all the dependencies in one test is causing 
>>>>>>> the test to be skipped on every platform. In an offline conversation it 
>>>>>>> was decided that it is better to split this tests into a few tests to 
>>>>>>> declare the per-platform module dependencies.
>>>>>>> 
>>>>>>> The bug: https://bugs.openjdk.java.net/browse/JDK-8158670
>>>>>>> The suggested fix: 
>>>>>>> http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/
>>>>>> The copyright header start year of the new tests should be 2016.
>>>>>> 
>>>>>> I would suggest to make CheckSecurityProvide a platform-neutral test, 
>>>>>> i.e.,
>>>>>> - drop @requires
>>>>>> - make line 94-97 to ignore the platform-dependent provider if it’s 
>>>>>> present in the white list
>>>>>> 
>>>>>> If we could make this test order-insensitive, it’d be cleaner to 
>>>>>> maintain a platform-neutral list of security providers and one list for 
>>>>>> the platform-dependent security providers for each platform.  Just an 
>>>>>> idea.
>>>>>> 
>>>>>> Mandy
>>>>>> 
>>> 
>> 
> 

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Alexandre (Shura) Iline]

Pleas review the new version of the fix.
http://cr.openjdk.java.net/~shurailine/8158670/webrev.01/

I have executed the changed test successfully on linux, windows, mac os x and 
solaris.

Shura

> On Jun 29, 2016, at 10:43 AM, Alexandre (Shura) Iline 
> <alexandre.il...@oracle.com> wrote:
> 
> 
>> On Jun 29, 2016, at 10:41 AM, Valerie Peng <valerie.p...@oracle.com> wrote:
>> 
>> 
>> It's not like the test silently passes as the test still covers the 
>> cross-platform modules.
>> The way I view this is that the platform=specific modules are "optional" and 
>> we update the expected result by detecting their presence (or the not). It's 
>> not a hack or workaround, but rather an enhancement for the test to handle 
>> different images.
> 
> Oh … that makes more sense. I mis-understood it originally.
> 
> Let me fix it like this.
> 
> Shura
> 
>> 
>> Just my .02,
>> Valerie
>> 
>> On 6/29/2016 10:22 AM, Alexandre (Shura) Iline wrote:
>>>> On Jun 28, 2016, at 5:22 PM, Valerie Peng <valerie.p...@oracle.com> wrote:
>>>> 
>>>> 
>>>> One of the purpose of this test is to test the ordering (see the initial 
>>>> bug which this test is for: JDK-6997010).
>>>> 
>>>> The original test already detects the OS and will skip certain providers 
>>>> accordingly.
>>>> Instead of splitting the test into multiple platform-specific tests, maybe 
>>>> we can keep the original test but add module-presence checking? Is there 
>>>> API available to query if certain module are present?
>>> ModuleFinder.ofSystem().find(String).
>>> 
>>> We can have only the cross-platform modules listed in @modules and make the 
>>> test to pass silently if the required platform-specific modules are not 
>>> present.
>>> 
>>> So, for example, on windows, if the test would be executed against an image 
>>> which have no jdk.crypto.mscapi, the test will not run any checks and 
>>> report pass.
>>> 
>>> This would help to avoid the additional test creation, but it will add 
>>> another silently passing test, which is less clean.
>>> 
>>> Mandy?
>>> 
>>> Shura
>>> 
>>>> If yes, then we can leave out the platform-specific providers from the 
>>>> @modules line and skip the providers if either the OS does not match or 
>>>> the module is not present.
>>>> 
>>>> If we can't query what modules are available, then we may have to think of 
>>>> something else.
>>>> Valerie
>>>> 
>>>> On 6/27/2016 12:27 PM, Mandy Chung wrote:
>>>>> I’m including security-dev which would be a better list to review this 
>>>>> test fix.
>>>>> 
>>>>> Valerie,
>>>>>   Does this test have to be order-sensitive?  I think this test would be 
>>>>> cleaner to make it order-insensitive and simply test the security 
>>>>> provider initialization.
>>>>> 
>>>>> See my comments below.
>>>>> 
>>>>>> On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline 
>>>>>> <alexandre.il...@oracle.com> wrote:
>>>>>> 
>>>>>> Hi.
>>>>>> 
>>>>>> Please take a look on a suggested for for the 
>>>>>> java/lang/SecurityManager/CheckSecurityProvider.java test.
>>>>>> 
>>>>>> The test in question depend on a list of modules, some of them are 
>>>>>> platform-specific. Listing all the dependencies in one test is causing 
>>>>>> the test to be skipped on every platform. In an offline conversation it 
>>>>>> was decided that it is better to split this tests into a few tests to 
>>>>>> declare the per-platform module dependencies.
>>>>>> 
>>>>>> The bug: https://bugs.openjdk.java.net/browse/JDK-8158670
>>>>>> The suggested fix: 
>>>>>> http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/
>>>>> The copyright header start year of the new tests should be 2016.
>>>>> 
>>>>> I would suggest to make CheckSecurityProvide a platform-neutral test, 
>>>>> i.e.,
>>>>> - drop @requires
>>>>> - make line 94-97 to ignore the platform-dependent provider if it’s 
>>>>> present in the white list
>>>>> 
>>>>> If we could make this test order-insensitive, it’d be cleaner to maintain 
>>>>> a platform-neutral list of security providers and one list for the 
>>>>> platform-dependent security providers for each platform.  Just an idea.
>>>>> 
>>>>> Mandy
>>>>> 
>> 
> 

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Alexandre (Shura) Iline]

> On Jun 29, 2016, at 10:41 AM, Valerie Peng <valerie.p...@oracle.com> wrote:
> 
> 
> It's not like the test silently passes as the test still covers the 
> cross-platform modules.
> The way I view this is that the platform=specific modules are "optional" and 
> we update the expected result by detecting their presence (or the not). It's 
> not a hack or workaround, but rather an enhancement for the test to handle 
> different images.

Oh … that makes more sense. I mis-understood it originally.

Let me fix it like this.

Shura

> 
> Just my .02,
> Valerie
> 
> On 6/29/2016 10:22 AM, Alexandre (Shura) Iline wrote:
>>> On Jun 28, 2016, at 5:22 PM, Valerie Peng <valerie.p...@oracle.com> wrote:
>>> 
>>> 
>>> One of the purpose of this test is to test the ordering (see the initial 
>>> bug which this test is for: JDK-6997010).
>>> 
>>> The original test already detects the OS and will skip certain providers 
>>> accordingly.
>>> Instead of splitting the test into multiple platform-specific tests, maybe 
>>> we can keep the original test but add module-presence checking? Is there 
>>> API available to query if certain module are present?
>> ModuleFinder.ofSystem().find(String).
>> 
>> We can have only the cross-platform modules listed in @modules and make the 
>> test to pass silently if the required platform-specific modules are not 
>> present.
>> 
>> So, for example, on windows, if the test would be executed against an image 
>> which have no jdk.crypto.mscapi, the test will not run any checks and report 
>> pass.
>> 
>> This would help to avoid the additional test creation, but it will add 
>> another silently passing test, which is less clean.
>> 
>> Mandy?
>> 
>> Shura
>> 
>>> If yes, then we can leave out the platform-specific providers from the 
>>> @modules line and skip the providers if either the OS does not match or the 
>>> module is not present.
>>> 
>>> If we can't query what modules are available, then we may have to think of 
>>> something else.
>>> Valerie
>>> 
>>> On 6/27/2016 12:27 PM, Mandy Chung wrote:
>>>> I’m including security-dev which would be a better list to review this 
>>>> test fix.
>>>> 
>>>> Valerie,
>>>>Does this test have to be order-sensitive?  I think this test would be 
>>>> cleaner to make it order-insensitive and simply test the security provider 
>>>> initialization.
>>>> 
>>>> See my comments below.
>>>> 
>>>>> On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline 
>>>>> <alexandre.il...@oracle.com> wrote:
>>>>> 
>>>>> Hi.
>>>>> 
>>>>> Please take a look on a suggested for for the 
>>>>> java/lang/SecurityManager/CheckSecurityProvider.java test.
>>>>> 
>>>>> The test in question depend on a list of modules, some of them are 
>>>>> platform-specific. Listing all the dependencies in one test is causing 
>>>>> the test to be skipped on every platform. In an offline conversation it 
>>>>> was decided that it is better to split this tests into a few tests to 
>>>>> declare the per-platform module dependencies.
>>>>> 
>>>>> The bug: https://bugs.openjdk.java.net/browse/JDK-8158670
>>>>> The suggested fix: 
>>>>> http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/
>>>> The copyright header start year of the new tests should be 2016.
>>>> 
>>>> I would suggest to make CheckSecurityProvide a platform-neutral test, i.e.,
>>>> - drop @requires
>>>> - make line 94-97 to ignore the platform-dependent provider if it’s 
>>>> present in the white list
>>>> 
>>>> If we could make this test order-insensitive, it’d be cleaner to maintain 
>>>> a platform-neutral list of security providers and one list for the 
>>>> platform-dependent security providers for each platform.  Just an idea.
>>>> 
>>>> Mandy
>>>> 
> 

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Alexandre (Shura) Iline]

Copyrights fixed in place.

Thank you, Mandy.

Shura

> On Jun 27, 2016, at 12:27 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> I’m including security-dev which would be a better list to review this test 
> fix.
> 
> Valerie,
>   Does this test have to be order-sensitive?  I think this test would be 
> cleaner to make it order-insensitive and simply test the security provider 
> initialization.
> 
> See my comments below.
> 
>> On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline 
>> <alexandre.il...@oracle.com> wrote:
>> 
>> Hi.
>> 
>> Please take a look on a suggested for for the 
>> java/lang/SecurityManager/CheckSecurityProvider.java test. 
>> 
>> The test in question depend on a list of modules, some of them are 
>> platform-specific. Listing all the dependencies in one test is causing the 
>> test to be skipped on every platform. In an offline conversation it was 
>> decided that it is better to split this tests into a few tests to declare 
>> the per-platform module dependencies.
>> 
>> The bug: https://bugs.openjdk.java.net/browse/JDK-8158670
>> The suggested fix: http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/
> 
> The copyright header start year of the new tests should be 2016.
> 
> I would suggest to make CheckSecurityProvide a platform-neutral test, i.e.,
> - drop @requires
> - make line 94-97 to ignore the platform-dependent provider if it’s present 
> in the white list
> 
> If we could make this test order-insensitive, it’d be cleaner to maintain a 
> platform-neutral list of security providers and one list for the 
> platform-dependent security providers for each platform.  Just an idea.
> 
> Mandy
> 

Re: RFR 8158855: Fix remaining module dependences in java/lang

[Alexandre (Shura) Iline]

Hi.

After some offline conversations it was decided to leave the tests which depend 
on absence of “-limitmods” in VM options for later. I have created a separate 
bug ti track that:
https://bugs.openjdk.java.net/browse/JDK-8159523

What left for this review is this, then:
http://cr.openjdk.java.net/~shurailine/8158855/webrev.01/

Please take a look on it.

Next problems are left unfixed in the java/lang tests:
java/lang/invoke/modules/ModuleAccessControlTest.java, 
java/lang/reflect/Proxy/ProxyClassAccessTest.java 
,java/lang/reflect/Proxy/ProxyTest.java JDK-8159523
java/lang/SecurityManager/CheckSecurityProvider.java JDK-8158670
java/lang/String/concat/WithSecurityManager.java JDK-8158851
java/lang/management/MemoryMXBean/Pending.java JDK-8158837
java/lang/reflect/OldenCompilingWithDefaults.java CODETOOLS-7901694
java/lang/StackWalker/TestBCI.java, java/lang/invoke/lambda/LambdaAsm.java 
CODETOOLS-7901678

Shura


> On Jun 8, 2016, at 5:50 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> Hi Shura,
> 
> test/java/lang/Class/GetModuleTest.java
>   This can use java.base/jdk.internal.misc.Unsafe instead.
> 
> Other @modules looks okay.
> 
> Regarding the executeTestJava change, if you drop the VM options, this test 
> will only exercise the default launcher setting.  The hotspot nightlies 
> depend on VM options to exercise different configurations.  I think many 
> tests that launch a modular test will run into the same issue.  It’s not 
> ideal to drop the VM options then.  One alternative is to have a new 
> ProcessTool::executeModularTest that will take test.vm.opts and 
> test.java.opts but dropping -limitmods, if any.
> 
> Mandy
> 

Re: RFR 8158855: Fix remaining module dependences in java/lang

[Alexandre (Shura) Iline]

> On Jun 6, 2016, at 8:59 PM, Amy Lu <amy...@oracle.com> wrote:
> 
> I'm not an official reviewer but just a minor comment ...
> 
> I noticed that "executeTestJava" changed to "executeProcess" in 
> ProxyTest.java, ProxyClassAccessTest.java and ModuleAccessControlTest.java, 
> are these changes expected in this patch?

Yes, the changes are expected.

These tests did not work with -limitmods java.base before because 
executeTestJava(String…) adds the test java options to the executed java, 
including the -limitmods.

This could be fixed differently, I have selected the easiest way: doing the 
same as what executeTestJava(String …) does only without what 
Utils.addTestJavaOpts(String…).


> If yes, should test/java/lang/reflect/Module/access/AccessTest.java also take 
> similar change? AccessTest.java also uses "executeTestJava”.

I have not reviewed all the tests, but the rest of tests is passing when 
executed with declared dependencies. AcceptTest passes with 
-javaoptions:"-limitmods jdk.compiler”, which is why I did not change it.

It may make sense to do something different about all the module tests, such as 
to create another library call which passes all VM options with the exception 
of module-specific options. I will wait for Alan or Mandy to comment.

Thank you.

Shura

> 
> Thanks,
> Amy
> 
> 
> On 6/7/16 6:21 AM, Alexandre (Shura) Iline wrote:
>> Hi,
>> 
>> Please review the change with fixes the remaining @module declaration in 
>> java/lang tests:
>> 
>> http://cr.openjdk.java.net/~shurailine/8158855/webrev.00
>> 
>> 
>> This addresses everything in the java.lang, except next few tests which 
>> either require more work or blocked by other issues:
>> java/lang/SecurityManager/CheckSecurityProvider.java JDK-8158670
>> java/lang/String/concat/WithSecurityManager.java JDK-8158851
>> java/lang/management/MemoryMXBean/Pending.java JDK-8158837
>> java/lang/reflect/OldenCompilingWithDefaults.java CODETOOLS-7901694
>> java/lang/StackWalker/TestBCI.java, java/lang/invoke/lambda/LambdaAsm.java 
>> CODETOOLS-7901678
>> 
>> 
>> Shura
>> 
>> 
> 

RFR 8158855: Fix remaining module dependences in java/lang

[Alexandre (Shura) Iline]

Hi,

Please review the change with fixes the remaining @module declaration in 
java/lang tests:
http://cr.openjdk.java.net/~shurailine/8158855/webrev.00

This addresses everything in the java.lang, except next few tests which either 
require more work or blocked by other issues:
java/lang/SecurityManager/CheckSecurityProvider.java JDK-8158670
java/lang/String/concat/WithSecurityManager.java JDK-8158851
java/lang/management/MemoryMXBean/Pending.java JDK-8158837
java/lang/reflect/OldenCompilingWithDefaults.java CODETOOLS-7901694
java/lang/StackWalker/TestBCI.java, java/lang/invoke/lambda/LambdaAsm.java 
CODETOOLS-7901678


Shura

Re: RFR 8156497: Add jar tool support for Multi-release modular JARs

[Alexandre (Shura) Iline]

Chris,

Does the test need to use @modules for jdk.jartool and java.compiler?

Shura

> On May 20, 2016, at 8:55 AM, Chris Hegarty <chris.hega...@oracle.com> wrote:
> 
> What do you get if you mix JEP 261 [1] with JEP 238 [2]?
> A Multi-release modular JAR.
> 
> This issue proposes to add support to the jar tool for creating and
> updating modular JAR files with an optional module-info.class in the
> versioned section.
> 
> MRJARs are intended to target multiple releases of the Java platform, so
> it seems reasonable for a Multi-release modular JAR ( MRMJAR ) to be
> able to declare a different set of dependencies on modules that are part
> of the Java platform. The reasoning here is that these are
> implementation details rather than parts of a module's API surface, and
> that one may well want to change them as the JDK itself evolves. The
> runtime already has support for loading from the versioned section.
> 
> Specifically, a versioned module descriptor must be identical to the
> root module descriptor, with two exceptions:
> 
> - A versioned descriptor can have different non-public `requires`
> clauses of platform ( `java.*` and `jdk.*` ) modules, and
> 
> - A versioned descriptor can have different `uses` clauses, even of
> service types defined outside of `java.*` and `jdk.*` modules.
> 
> http://cr.openjdk.java.net/~chegar/8156497.00/
> 
> Note: while there are some new test scenarios added, which give
> reasonable coverage, further tests will be added later. Steve has some
> additional jar tools support coming for easier creation of MRJARS. 
> 
> -Chris.
> 
> [1] http://openjdk.java.net/jeps/261
> [2] http://openjdk.java.net/jeps/238

Re: RFR 8156972: java/lang/reflect/Layer/LayerAndLoadersTest.java test requires jdk.compiler

[Alexandre (Shura) Iline]

You are right, Alan. Thank you.

http://cr.openjdk.java.net/~shurailine/8156972/webrev.01/

I have also tried to think of a better message, since it is UOE not and not ISE 
… but decided it is descriptive enough as it is. 

Shura

> On May 14, 2016, at 8:37 AM, Alan Bateman <alan.bate...@oracle.com> wrote:
> 
> On 13/05/2016 23:54, Alexandre (Shura) Iline wrote:
>> Hi.
>> 
>> Could you please take a look on the fix:
>> http://cr.openjdk.java.net/~shurailine/8156972/webrev.00/
>> 
>> Besides adding @modules, I have also added a couple of lines to the library 
>> class, because I have found the NPE which I was getting originally 
>> non-informative.
>> 
> The @modules looks good. For CompileUtils then it might be better to throw 
> UOE instead of ISE for this case. Also would be good to add this exception to 
> the @throws list in the javadoc.
> 
> -Alan.

RFR 8156972: java/lang/reflect/Layer/LayerAndLoadersTest.java test requires jdk.compiler

[Alexandre (Shura) Iline]

Hi.

Could you please take a look on the fix:
http://cr.openjdk.java.net/~shurailine/8156972/webrev.00/

Besides adding @modules, I have also added a couple of lines to the library 
class, because I have found the NPE which I was getting originally 
non-informative.

Shura

Re: RFR: 8155993: Some tests in java/lang/management fail with NCDFE: com/sun/management/internal/GarbageCollectorExtImpl

[Alexandre (Shura) Iline]

> On May 11, 2016, at 10:57 AM, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> 
>> On May 10, 2016, at 12:49 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
>> 
>> 
>>> On May 10, 2016, at 12:00 PM, Alexandre (Shura) Iline 
>>> <alexandre.il...@oracle.com> wrote:
>>> 
>>> Hi.
>>> 
>>> Please review the fix:
>>> http://cr.openjdk.java.net/~shurailine/8155993/webrev.00/
>>> 
> 
> Pushed.

Thank you.

> 
>> 
>> In fact JDK-8155993 reveals a potential bug in the VM support for 
>> java.lang.management when jdk.management module is not present.
> 
> 
> I see another open issue for this VM issue (sorry for not seeing it earlier)
>   https://bugs.openjdk.java.net/browse/JDK-8151099

And both reported by me. I did forget about the old one.

Shura

> 
> I closed JDK-8155993 as a dup of JDK-8151099.
> 
> Mandy

Re: RFR: 8155993: Some tests in java/lang/management fail with NCDFE: com/sun/management/internal/GarbageCollectorExtImpl

[Alexandre (Shura) Iline]

OK, I will do that, Mandy.

Should I create a separate bug (subtask) for this test fixes?

Shura

> On May 10, 2016, at 12:49 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> 
>> On May 10, 2016, at 12:00 PM, Alexandre (Shura) Iline 
>> <alexandre.il...@oracle.com> wrote:
>> 
>> Hi.
>> 
>> Please review the fix:
>> http://cr.openjdk.java.net/~shurailine/8155993/webrev.00/
>> 
>> In the failing tests I have replaced java.management with jdk.management in 
>> @modules.
>> 
>> Besides the failed tests I have scanned all the tests which call 
>> ManagementFactory.getPlatformMBeanServer(…). Those which were not failing 
>> were already declaring dependency on jdk.management, sometimes together with 
>> java.management. Declaring java.management in such case is not needed, 
>> because java.management is a dependency of jdk.management.
> 
> In fact JDK-8155993 reveals a potential bug in the VM support for 
> java.lang.management when jdk.management module is not present.
> 
> I have no objection to change these tests to run with jdk.management and this 
> patch looks fine.  But we should keep JDK-8155993  open and provide a small 
> test to reproduce NCDFE when running with an image with only java.management 
> but no jdk.management module.
> 
> Mandy
> 

RFR: 8155993: Some tests in java/lang/management fail with NCDFE: com/sun/management/internal/GarbageCollectorExtImpl

[Alexandre (Shura) Iline]

Hi.

Please review the fix:
http://cr.openjdk.java.net/~shurailine/8155993/webrev.00/

In the failing tests I have replaced java.management with jdk.management in 
@modules.

Besides the failed tests I have scanned all the tests which call 
ManagementFactory.getPlatformMBeanServer(…). Those which were not failing were 
already declaring dependency on jdk.management, sometimes together with 
java.management. Declaring java.management in such case is not needed, because 
java.management is a dependency of jdk.management.

Shura

Re: RFR: JDK-8151914 java/util/jar/JarFile/MultiReleaseJar* tests do not declare module dependences

[Alexandre (Shura) Iline]

> On May 5, 2016, at 12:12 PM, Jonathan Gibbons <jonathan.gibb...@oracle.com> 
> wrote:
> 
> There is potentially a separate discussion here, as to whether javac should 
> "force" the provision of a jar-fs provider.
> 
> Strictly speaking, javac does not inherently require it.  You can use javac 
> just fine, with files in the system image, and source and class files in the 
> default file system.  But I suspect most folk would be suprised if they ca 
> across an image for which javac could not read jar files.

What would definitely help is a error message which specifies the jar name. At 
least in my case that would, as I would not miss the message.

Shura

> 
> -- Jon
> 
> On 05/05/2016 12:01 PM, Alexandre (Shura) Iline wrote:
>>> On May 5, 2016, at 11:42 AM, Alexandre (Shura) Iline 
>>> <alexandre.il...@oracle.com> wrote:
>>> Whether the java.tools API behavior is correct is a separate matter. I am 
>>> planning to create a standalone test case and take it with javac ppl.
>> I take this ^^ back, as the error was there all along: 
>> "java.nio.file.ProviderNotFoundException: no provider found for .jar files”
>> 
>> The fix is valid, then, is and waiting for a review.
>> 
>> Shura
>> 
>>> Thank you.
>>> 
>>> Shura
>>> 
>>>> On May 4, 2016, at 8:30 AM, Chris Hegarty <chris.hega...@oracle.com> wrote:
>>>> 
>>>> On 4 May 2016, at 14:32, Alan Bateman <alan.bate...@oracle.com> wrote:
>>>>> On 04/05/2016 11:24, Chris Hegarty wrote:
>>>>>> :
>>>>>> The tests cause compilation of test library classes, but only some tests
>>>>>> actually use the methods that provoke compilation. Similar to above, 
>>>>>> tests
>>>>>> that don’t actually compile anything could depend on just java.compiler.
>>>>>> 
>>>>>> This is all to fragile and may cause problems with future refactoring. I
>>>>>> think we should add the same set of @moduels to all these tests, rather
>>>>>> than an individual set determined by intimate knowledge of the inner
>>>>>> workings of the test.
>>>>>> 
>>>>>> @modules java.compiler
>>>>>>  jdk.compiler
>>>>>>  jdk.zipfs
>>>>>>  jdk.jartool
>>>>>> 
>>>>>> with the addition of jdk.httpserver for MultiReleaseJarHttpProperties.
>>>>>> 
>>>>> or we could move the tests into their own MultiRelease sub-directory and 
>>>>> create a TEST.properties with a module key. That would allow these tests 
>>>>> to drop @modules, except the test that uses the HTTP server.
>>>> I think that would be better.
>>>> 
>>>> -Chris.
> 

Re: RFR 8154182: Fix java/lang/Class/getDeclaredField/FieldSetAccessibleTest.java to only use available modules

[Alexandre (Shura) Iline]

http://cr.openjdk.java.net/~shurailine/8154182/webrev.01/

I was storing the module list in a collection before to get the things more 
“effective”. It did not make it more effective, you are right about that.

Thanks for the tip on “/modules”.

Please take a look.

Shura

> On May 4, 2016, at 1:07 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> 
>> On Apr 13, 2016, at 10:29 AM, Alexandre (Shura) Iline 
>> <alexandre.il...@oracle.com> wrote:
>> 
>> Hi,
>> 
>> Could you be so kind to take a look on this fix:
>> http://cr.openjdk.java.net/~shurailine/8154182/webrev.00/
> 
> Looks okay.  Some suggestion:
> 
> You only need to walk one root:
>   root = fs.getPath("/modules”)
> 
> and instead of caching the available modules, you could
>   .filter(x -> Layer.boot().findModule(x.getName(1).toString())).isPresent()
> 
> I can sponsor your patch - please send me the changeset.
> 
> Mandy

Re: RFR: JDK-8151914 java/util/jar/JarFile/MultiReleaseJar* tests do not declare module dependences

[Alexandre (Shura) Iline]

> On May 5, 2016, at 11:42 AM, Alexandre (Shura) Iline 
> <alexandre.il...@oracle.com> wrote:
> Whether the java.tools API behavior is correct is a separate matter. I am 
> planning to create a standalone test case and take it with javac ppl.
I take this ^^ back, as the error was there all along: 
"java.nio.file.ProviderNotFoundException: no provider found for .jar files”

The fix is valid, then, is and waiting for a review.

Shura

> 
> Thank you.
> 
> Shura
> 
>> On May 4, 2016, at 8:30 AM, Chris Hegarty <chris.hega...@oracle.com> wrote:
>> 
>> On 4 May 2016, at 14:32, Alan Bateman <alan.bate...@oracle.com> wrote:
>>> 
>>> On 04/05/2016 11:24, Chris Hegarty wrote:
>>>> :
>>>> The tests cause compilation of test library classes, but only some tests
>>>> actually use the methods that provoke compilation. Similar to above, tests
>>>> that don’t actually compile anything could depend on just java.compiler.
>>>> 
>>>> This is all to fragile and may cause problems with future refactoring. I
>>>> think we should add the same set of @moduels to all these tests, rather
>>>> than an individual set determined by intimate knowledge of the inner
>>>> workings of the test.
>>>> 
>>>> @modules java.compiler
>>>>  jdk.compiler
>>>>  jdk.zipfs
>>>>  jdk.jartool
>>>> 
>>>> with the addition of jdk.httpserver for MultiReleaseJarHttpProperties.
>>>> 
>>> or we could move the tests into their own MultiRelease sub-directory and 
>>> create a TEST.properties with a module key. That would allow these tests to 
>>> drop @modules, except the test that uses the HTTP server.
>> 
>> I think that would be better.
>> 
>> -Chris.
> 

Re: RFR: JDK-8151914 java/util/jar/JarFile/MultiReleaseJar* tests do not declare module dependences

[Alexandre (Shura) Iline]

Chris, could you please take another look:
http://cr.openjdk.java.net/~shurailine/8151914/webrev.02/

What I have discovered is that jdk.zipfs was used to access jars on the 
classpath, which were JTreg jars: jtreg.jar, testing.jar, etc. Cleaning the 
class path through the environment removed dependency on the zipfs. 

Whether the java.tools API behavior is correct is a separate matter. I am 
planning to create a standalone test case and take it with javac ppl.

Thank you.

Shura

> On May 4, 2016, at 8:30 AM, Chris Hegarty <chris.hega...@oracle.com> wrote:
> 
> On 4 May 2016, at 14:32, Alan Bateman <alan.bate...@oracle.com> wrote:
>> 
>> On 04/05/2016 11:24, Chris Hegarty wrote:
>>> :
>>> The tests cause compilation of test library classes, but only some tests
>>> actually use the methods that provoke compilation. Similar to above, tests
>>> that don’t actually compile anything could depend on just java.compiler.
>>> 
>>> This is all to fragile and may cause problems with future refactoring. I
>>> think we should add the same set of @moduels to all these tests, rather
>>> than an individual set determined by intimate knowledge of the inner
>>> workings of the test.
>>> 
>>> @modules java.compiler
>>>   jdk.compiler
>>>   jdk.zipfs
>>>   jdk.jartool
>>> 
>>> with the addition of jdk.httpserver for MultiReleaseJarHttpProperties.
>>> 
>> or we could move the tests into their own MultiRelease sub-directory and 
>> create a TEST.properties with a module key. That would allow these tests to 
>> drop @modules, except the test that uses the HTTP server.
> 
> I think that would be better.
> 
> -Chris.

Re: RFR: JDK-8151914 java/util/jar/JarFile/MultiReleaseJar* tests do not declare module dependences

[Alexandre (Shura) Iline]

This makes sense - I will move the tests into a subduer, put the dependencies 
into a TEST.properties file.

jdk.zipfs has the code needed for access jars - the tests are failing without 
that dependency.

Shura
> On May 4, 2016, at 8:30 AM, Chris Hegarty <chris.hega...@oracle.com> wrote:
> 
> On 4 May 2016, at 14:32, Alan Bateman <alan.bate...@oracle.com> wrote:
>> 
>> On 04/05/2016 11:24, Chris Hegarty wrote:
>>> :
>>> The tests cause compilation of test library classes, but only some tests
>>> actually use the methods that provoke compilation. Similar to above, tests
>>> that don’t actually compile anything could depend on just java.compiler.
>>> 
>>> This is all to fragile and may cause problems with future refactoring. I
>>> think we should add the same set of @moduels to all these tests, rather
>>> than an individual set determined by intimate knowledge of the inner
>>> workings of the test.
>>> 
>>> @modules java.compiler
>>>   jdk.compiler
>>>   jdk.zipfs
>>>   jdk.jartool
>>> 
>>> with the addition of jdk.httpserver for MultiReleaseJarHttpProperties.
>>> 
>> or we could move the tests into their own MultiRelease sub-directory and 
>> create a TEST.properties with a module key. That would allow these tests to 
>> drop @modules, except the test that uses the HTTP server.
> 
> I think that would be better.
> 
> -Chris.

Re: RFR 8154182: Fix java/lang/Class/getDeclaredField/FieldSetAccessibleTest.java to only use available modules

[Shura]

> On Apr 14, 2016, at 7:27 AM, Alan Bateman <alan.bate...@oracle.com> wrote:
> 
> On 13/04/2016 18:29, Alexandre (Shura) Iline wrote:
>> Hi,
>> 
>> Could you be so kind to take a look on this fix:
>> http://cr.openjdk.java.net/~shurailine/8154182/webrev.00/
>> 
> What would you think of having this test run with `-addmods ALL-SYSTEM` 
> instead? Your patch is okay but it will mean that the test won't exercise 
> getDeclaredFields on the EE module, at least not when we bring the changes in 
> jake to JDK 9. This option can be used with `-limitmods`.
> 
> Another test like this is VerifyModuleDelegation. In both cases the tests 
> have been changed to run with `-addmods ALL-SYSTEM` in jake.

Alan,

I am not sure what you are suggesting.

Right now the test is failing without my changes, if “-limitmods java.base” is 
supplied to the jtreg command line:
$ jtreg -jdk:... -noreport "-javaoptions:-limitmods java.base" 
java/lang/Class/getDeclaredField/FieldSetAccessibleTest.java
with 
Test failed for the following classes: …

Right now the test has:
 * @run main/othervm -Djdk.launcher.addmods=ALL-SYSTEM FieldSetAccessibleTest 
UNSECURE
Changing it to 
 * @run main/othervm -addmods ALL-SYSTEM FieldSetAccessibleTest UNSECURE
yields the same results.

I was thinking that the Jake integration would change something, but I see the 
same behavior as before.

Shura

> 
> -Alan

RFR 8154182: Fix java/lang/Class/getDeclaredField/FieldSetAccessibleTest.java to only use available modules

[Alexandre (Shura) Iline]

Hi,

Could you be so kind to take a look on this fix:
http://cr.openjdk.java.net/~shurailine/8154182/webrev.00/

Thank you.

Shura

Re: RFR: JDK-8150998: Fix module dependences in java/lang tests

[Alexandre (Shura) Iline]

Hi.

Could you please have another look on the webrev.

http://cr.openjdk.java.net/~shurailine/8150998/webrev.jake.03/
http://cr.openjdk.java.net/~shurailine/8150998/webrev.jdk9.03/

I have reverted changes to java/lang/management tests for now, until resolution 
of https://bugs.openjdk.java.net/browse/JDK-8151099.

I have addressed all the comments except moving the fix of 
test/java/lang/management/MemoryMXBean/PendingAllGC.sh to JDK9, as this test is 
already different in JDK9 and Jake, so a fix in JDK9 will be overridden.

Shura


> On Mar 2, 2016, at 9:48 AM, Alexandre (Shura) Iline 
> <alexandre.il...@oracle.com> wrote:
> 
> Hi.
> 
> Could you please take a look on a fix to add missing module dependencies for 
> tests in java/lang. 
> 
> JDK9 changes: http://cr.openjdk.java.net/~shurailine/8150998/webrev.jdk9.01
> Jake changes: http://cr.openjdk.java.net/~shurailine/8150998/webrev.jake.01
> 
> Shura.

Re: RFR: JDK-8150998: Fix module dependences in java/lang tests

[Alexandre (Shura) Iline]

> On Mar 2, 2016, at 2:56 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> 
>> On Mar 2, 2016, at 2:13 PM, Alexandre (Shura) Iline 
>> <alexandre.il...@oracle.com> wrote:
>> 
>> 
>>> On Mar 2, 2016, at 2:03 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
>>> 
>>> 
>>>> On Mar 2, 2016, at 12:38 PM, Alexandre (Shura) Iline 
>>>> <alexandre.il...@oracle.com> wrote:
>>>> 
>>>> http://cr.openjdk.java.net/~shurailine/8150998/webrev.jdk9.02/
>>> 
>>> 
>>> test/java/lang/instrument/MakeJAR2.sh
>>> -XaddExports should not be brought to jdk9.
>>> 
>>> test/java/lang/management/ManagementFactory/TEST.properties
>>> Should it be moved to test/java/lang/management since all tests under it 
>>> are testing java/lang/management?
>>> 
>>> should this require java.management instead?  I expect most tests only need 
>>> java.management.
>> 
>> 10 of 11 tests in java/lang/management/ManagementFactory throw 
>> java.lang.NoClassDefFoundError: 
>> com/sun/management/internal/GarbageCollectorExtImpl 
>> when jdk.management is not available.
>> 
> 
> Thanks for the stack trace you sent offline.
> 
> I think it’s a bug.  
> java.lang.management.ManagementFactory.getPlatformMXBeans() should work even 
> if jdk.management is not present.  Can you help file an issue?
> 
>> java/lang/management/ManagementFactory/GetObjectName.java indeed works with 
>> java.management. Are you suggesting to get back to declaring dependencies in 
>> every test?
> 
> I agree with Jon that declaring the module dependences in each test makes it 
> explicitly but either way is fine with me.
> 
>> That would include java/lang/management/ManagementFactory/GetObjectName.java 
>> into a run when java.management is present but not jdk.management. 
> 
> This reveals a bug.

I will redo the fix with this new info.

Shura

> 
> Mandy

Re: RFR: JDK-8150998: Fix module dependences in java/lang tests

[Alexandre (Shura) Iline]

> On Mar 2, 2016, at 10:06 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:
> 
> On 02/03/16 18:48, Alexandre (Shura) Iline wrote:
>> Hi.
>> 
>> Could you please take a look on a fix to add missing module dependencies for 
>> tests in java/lang.
>> 
>> JDK9 changes: http://cr.openjdk.java.net/~shurailine/8150998/webrev.jdk9.01
>> Jake changes: http://cr.openjdk.java.net/~shurailine/8150998/webrev.jake.01
>> 
>> Shura.
>> 
> 
> Hi Shura,
> 
> I'm not sure I understand exactly the logic behind
> your proposed changes.
> 
> I see for instance that you added @modules java.logging to tests
> that import java.util.logging APIs, but then removed
> @modules java.management to tests that are importing
> java.lang.management APIs?

Most of those tests were declaring dependencies to java.management, which is a 
compile-time dependency. What is actually needed for those tests to work is 
jdk.management module.

So, the dependency declaration needed to be changed anyway. Instead of fixing 
every test, I have declared the dependency in a newly added TEST.properties 
file, and removed @modules from tests altogether.

Module dependencies for a test are inherited from TEST.properties files above 
in the hierarchy, unless the test overrides @modules explicitly.

> 
> In what concern changes to the logging tests, there's a small
> inconsistency: I see that most of the test are using a
> single @modules clause, except for this one which has two @modules:
> 
> http://cr.openjdk.java.net/~shurailine/8150998/webrev.jdk9.01/test/java/lang/System/LoggerFinder/internal/PlatformLoggerBridgeTest/PlatformLoggerBridgeTest.java.frames.html

Indeed! Thank you!

Fixed:
http://cr.openjdk.java.net/~shurailine/8150998/webrev.jdk9.02/

Shura

> 
> best regards,
> 
> -- daniel

RFR: JDK-8150998: Fix module dependences in java/lang tests

[Shura]

Hi.

Could you please take a look on a fix to add missing module dependencies for 
tests in java/lang. 

JDK9 changes: http://cr.openjdk.java.net/~shurailine/8150998/webrev.jdk9.01
Jake changes: http://cr.openjdk.java.net/~shurailine/8150998/webrev.jake.01

Shura.

Re: RFR 8149391: Fix module dependencies in java/util

[Alexandre (Shura) Iline]

> On Feb 8, 2016, at 3:58 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> 
>> On Feb 8, 2016, at 3:36 PM, Alexandre (Shura) Iline 
>> <alexandre.il...@oracle.com> wrote:
>> 
>>> 
>>> On Feb 8, 2016, at 1:23 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
>>> 
>>> 
>>>> On Feb 8, 2016, at 11:43 AM, Alexandre (Shura) Iline 
>>>> <alexandre.il...@oracle.com> wrote:
>>>> 
>>>> Hi.
>>>> 
>>>> Could you please take a look on additional module dependencies to be added 
>>>> for tests in java/util. There is more work to do for :jdk_core test group, 
>>>> which is to come separately.
>>>> 
>>>> JDK9 changes: 
>>>> http://cr.openjdk.java.net/~shurailine/8149391/webrev.jdk9.00/
>>>> Jake changes: 
>>>> http://cr.openjdk.java.net/~shurailine/8149391/webrev.jake.00/
>>> 
>>> Looks okay.  I can sponsor and push the patches for you.
>>> 
>>> One question:  test/java/util/logging/modules/GetResourceBundleTest.java - 
>>> what uses jdk.zipfs?
>> 
>> This dependency is required by this line:
>>   assertTrue(CompilerUtils.compile(PKG_SRC_DIR, PKG_DEST_DIR,
>>   "-modulepath", MOD_DEST_DIR.toString()));
>> 
>> It is currently believed to be a bug in javac. A separate bug will be 
>> created on that.
> 
> Ah that’s right.  Thanks.
> 
> I’ll push your change sets.

Thank you.

Shura

> 
> Mandy
> 

Re: RFR 8149391: Fix module dependencies in java/util

[Alexandre (Shura) Iline]

> On Feb 8, 2016, at 1:23 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> 
>> On Feb 8, 2016, at 11:43 AM, Alexandre (Shura) Iline 
>> <alexandre.il...@oracle.com> wrote:
>> 
>> Hi.
>> 
>> Could you please take a look on additional module dependencies to be added 
>> for tests in java/util. There is more work to do for :jdk_core test group, 
>> which is to come separately.
>> 
>> JDK9 changes: http://cr.openjdk.java.net/~shurailine/8149391/webrev.jdk9.00/
>> Jake changes: http://cr.openjdk.java.net/~shurailine/8149391/webrev.jake.00/
> 
> Looks okay.  I can sponsor and push the patches for you.
> 
> One question:  test/java/util/logging/modules/GetResourceBundleTest.java - 
> what uses jdk.zipfs?

This dependency is required by this line:
assertTrue(CompilerUtils.compile(PKG_SRC_DIR, PKG_DEST_DIR,
"-modulepath", MOD_DEST_DIR.toString()));

It is currently believed to be a bug in javac. A separate bug will be created 
on that.

Shura

> 
> Mandy

Re: RFR: 8139430

[Alexandre (Shura) Iline]

Yes, sorry.

Since the methods do not have any work to complete before interrupting and also 
methods are not used anywhere currently, I assume re-throwing the 
InterruptedException is a better choice.

http://cr.openjdk.java.net/~shurailine/8139430/webrev.07/test/lib/testlibrary/jdk/testlibrary/management/ThreadMXBeanTool.java.html

Shura

> On Nov 16, 2015, at 8:33 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> 
>> On Nov 16, 2015, at 5:38 AM, Alan Bateman <alan.bate...@oracle.com> wrote:
>> 
>> 
>> 
>> On 16/11/2015 13:01, Alexandre (Shura) Iline wrote:
>>> V6:
>>> http://cr.openjdk.java.net/~shurailine/8139430/webrev.06/
>>> 
>>> 
>> This looks okay to me. For completeness then I assume the 
>> THreadMXBeanTool.waitUntilXXX methods should re-assert the interrupt status 
>> if interrupted when waiting.
> 
> +1
> 
> Mandy
> 

Re: RFR: 8139430

[Alexandre (Shura) Iline]

V6:
http://cr.openjdk.java.net/~shurailine/8139430/webrev.06/

Thank you.

Shura

> On Nov 11, 2015, at 7:41 PM, Alexandre (Shura) Iline 
> <alexandre.il...@oracle.com> wrote:
> 
> 
>> On Nov 10, 2015, at 11:42 PM, Alan Bateman <alan.bate...@oracle.com> wrote:
>> 
>> 
>> 
>> On 09/11/2015 19:12, Alexandre (Shura) Iline wrote:
>>> Hi
>>> 
>>> I have just realized that an NPE could also be possible in 
>>> test/lib/testlibrary/jdk/testlibrary/Platform.java so it should be updated 
>>> also:
>>> http://cr.openjdk.java.net/~shurailine/8139430/webrev.04/
>>> 
>>> Shura
>>> 
>> I skimmed through the webrev and it looks okay. Assuming InputArguments is 
>> not renamed then you could rename containsPrefix to something like 
>> hasArgStartingWith or something that makes it clear what this method does.
>> 
>> Would it break many tests if getProcessId were changed to return long to 
>> match ProcessHandle::getPid?
> 
> Yes, there are a few places only, actually. Let me also fix those.
> 
> Shura
> 
>> 
>> When this is pushed then can we drop @modules java.management from any tests 
>> or were those changes held back assuming the dependency would be dropped?
>> 
>> -Alan.
>> 
>> 
>> 
> 

Re: RFR: 8139430

[Alexandre (Shura) Iline]

> On Nov 10, 2015, at 11:42 PM, Alan Bateman <alan.bate...@oracle.com> wrote:
> 
> 
> 
> On 09/11/2015 19:12, Alexandre (Shura) Iline wrote:
>> Hi
>> 
>> I have just realized that an NPE could also be possible in 
>> test/lib/testlibrary/jdk/testlibrary/Platform.java so it should be updated 
>> also:
>> http://cr.openjdk.java.net/~shurailine/8139430/webrev.04/
>> 
>> Shura
>> 
> I skimmed through the webrev and it looks okay. Assuming InputArguments is 
> not renamed then you could rename containsPrefix to something like 
> hasArgStartingWith or something that makes it clear what this method does.
> 
> Would it break many tests if getProcessId were changed to return long to 
> match ProcessHandle::getPid?

Yes, there are a few places only, actually. Let me also fix those.

Shura

> 
> When this is pushed then can we drop @modules java.management from any tests 
> or were those changes held back assuming the dependency would be dropped?
> 
> -Alan.
> 
> 
> 

Re: RFR: 8139430

[Alexandre (Shura) Iline]

> On Nov 10, 2015, at 9:04 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> Hi Shura,
> 
> Thanks for doing it and it’s good to see the unnecessary dependency to 
> java.management eliminated.
> 
> The new jdk.testlibrary.management package name is fine.  It’s okay to keep 
> the class name InputArguments as Jaroslav suggests and it’s easier to tell 
> what this class is about.

The reason I have refactored this is because there is more useful methods in 
RuntimeMXBean than just the getInputArguments(). If we are ever in a need in 
another library method dealing with RuntimeMXBean, then, a class 
RuntimeMXBeanTool, or similar, is a natural enough place to add them. Since 
also there is the ThreadMXBeanTool, it looks homogenous.

We can, of course, have the InputArguments class the way it is and still have 
other classes wrapping RuntimeMXBean calls.

http://cr.openjdk.java.net/~shurailine/8139430/webrev.05/

Shura

> 
> There is a copy of ProcessTools and InputArguments in the hotspot repository 
> under
>   hotspot/test/testlibrary/jdk/test/lib/
> 
> Are we planning to remove this duplicated copy?  If not, same patch should be 
> applied to those copy.  It’s fine to separate this and push the hotspot test 
> library change via hotspot-rt repo.  Christian can probably sponsor the patch 
> for you.
> 
> I’ll sponsor the jdk change and push it to jdk9/dev once you send me an 
> updated patch.
> 
> Mandy
> 
> 
>> On Nov 9, 2015, at 10:12 AM, Alexandre (Shura) Iline 
>> <alexandre.il...@oracle.com> wrote:
>> 
>> Hi
>> 
>> I have just realized that an NPE could also be possible in 
>> test/lib/testlibrary/jdk/testlibrary/Platform.java so it should be updated 
>> also:
>> http://cr.openjdk.java.net/~shurailine/8139430/webrev.04/
>> 
>> Shura
>> 
>>> On Nov 9, 2015, at 8:54 PM, Alexandre (Shura) Iline 
>>> <alexandre.il...@oracle.com> wrote:
>>> 
>>> Hi,
>>> 
>>> This is one of the ways to fix 8139430: 
>>> http://cr.openjdk.java.net/~shurailine/8139430/webrev.03
>>> 
>>> Could you please take a look on it?
>>> 
>>> A few comments. 
>>> 
>>> The biggest dependency on java.management was in 
>>> jdk.testlibrary.ProcessTools.getProcessId() method. I have changed the 
>>> method to use the new process API, which helped to avoid updating a lot of 
>>> code.
>>> 
>>> I am moving the rest library code which depended on java.management and 
>>> jdk.management  to a new “management" subpackage of the jdk library: 
>>> jdk.testlibrary.management. Another possibility I have considered is “ext” 
>>> or “extended” subpackage, which would have all the classes which depend on 
>>> anything but java.base. With “ext” there would be no way to keep the 
>>> desired granularity with the test modules dependencies.
>>> 
>>> The methods which worth moving fit well into two classes: 
>>> jdk.testlibrary.management.ThreadMXBeanTool - to include a couple of method 
>>> which previously belonged to the TestThread utility methods.
>>> jdk.testlibrary.management.RuntimeMXBeanTool - previously named as 
>>> jdk.testlibrary.InputArguments
>>> 
>>> None of moved/renamed test library methods were used anywhere in tests, so 
>>> no test updates needed:
>>> jdk.testlibrary.InputArguments is not used, instead every test which needs 
>>> that functionality directly calls RuntimeMXBean.getInputArguments()
>>> jdk.testlibrary.TestThread.waitUntilBlockingOnObject(Thread, Thread.State, 
>>> Object) and jdk.testlibrary.TestThread.waitUntilInNative(Thread) also were 
>>> not used as there is a similar class outside of the test library:
>>> ./closed/com/oracle/jfr/common/jrockit/TestThread.java
>>> 
>>> Please let me know what you think.
>>> 
>>> Shura
>> 
> 

Re: RFR: 8139430

[Alexandre (Shura) Iline]

Hi

I have just realized that an NPE could also be possible in 
test/lib/testlibrary/jdk/testlibrary/Platform.java so it should be updated also:
http://cr.openjdk.java.net/~shurailine/8139430/webrev.04/

Shura

> On Nov 9, 2015, at 8:54 PM, Alexandre (Shura) Iline 
> <alexandre.il...@oracle.com> wrote:
> 
> Hi,
> 
> This is one of the ways to fix 8139430: 
> http://cr.openjdk.java.net/~shurailine/8139430/webrev.03
> 
> Could you please take a look on it?
> 
> A few comments. 
> 
> The biggest dependency on java.management was in 
> jdk.testlibrary.ProcessTools.getProcessId() method. I have changed the method 
> to use the new process API, which helped to avoid updating a lot of code.
> 
> I am moving the rest library code which depended on java.management and 
> jdk.management  to a new “management" subpackage of the jdk library: 
> jdk.testlibrary.management. Another possibility I have considered is “ext” or 
> “extended” subpackage, which would have all the classes which depend on 
> anything but java.base. With “ext” there would be no way to keep the desired 
> granularity with the test modules dependencies.
> 
> The methods which worth moving fit well into two classes: 
> jdk.testlibrary.management.ThreadMXBeanTool - to include a couple of method 
> which previously belonged to the TestThread utility methods.
> jdk.testlibrary.management.RuntimeMXBeanTool - previously named as 
> jdk.testlibrary.InputArguments
> 
> None of moved/renamed test library methods were used anywhere in tests, so no 
> test updates needed:
> jdk.testlibrary.InputArguments is not used, instead every test which needs 
> that functionality directly calls RuntimeMXBean.getInputArguments()
> jdk.testlibrary.TestThread.waitUntilBlockingOnObject(Thread, Thread.State, 
> Object) and jdk.testlibrary.TestThread.waitUntilInNative(Thread) also were 
> not used as there is a similar class outside of the test library:
> ./closed/com/oracle/jfr/common/jrockit/TestThread.java
> 
> Please let me know what you think.
> 
> Shura

Re: RFR: 8140336: Add @modules for exported dependencies to jdk_core tests

[Alexandre (Shura) Iline]

> On Oct 27, 2015, at 12:17 AM, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> 
>> On Oct 26, 2015, at 10:13 AM, Alexandre (Shura) Iline 
>> <alexandre.il...@oracle.com> wrote:
>> 
>> Hi.
>> 
>> Could you please take a look on the suggested change in tests belonging to 
>> jdk_core test group.
>> 
>> Pls notice that the dependencies which would later be removed by fixing 
>> JDK-8139430 we not added in this change.
>> 
>> http://cr.openjdk.java.net/~shurailine/8140336/webrev.01/
> 
> Thanks for the patch.
> 
> This change is against jake/jdk repo.  I assume you intend to integrate this 
> change to jdk9/dev/jdk.  You can separate into two changesets, one for 
> jdk9/dev/jdk and the other for test/jdk/jigsaw tests that I can push your 
> change to test/jdk/jigsaw tests to jake.

I have created two separate reviews:
http://cr.openjdk.java.net/~shurailine/8140336/webrev.jdk9.02/
http://cr.openjdk.java.net/~shurailine/8140336/webrev.jake.02/

> 
> test/java/lang/ProcessHandle/TEST.properties
> - which test references types in jdk.management?   If there is only a couple 
> of tests depending on jdk.management, better to use @modules in those 
> individual tests instead.

The dependency is coming through java/lang/ProcessHandle/JavaChild.java. Three 
of five tests in the dir use it: InfoTest.java, OnExitTest.java, TreeTest.java.

Yes, I could update just the three tests, but since JTReg will compile all the 
classes in the dir, the compilation for the rest of the tests will fail if no 
jdk.management present. Instead I would rather have the fix as it is right now 
and have a separate bug to fix this later. Which is what I was going to do.

This is, effectively, the opposite of what I was suggesting with regards to 
JDK-8139430. The difference is that in the case of java/lang/ProcessHandle 
tests, I will only need to update two tests twice and in the case of 
JDK-8139430 it’s a _lot_ of tests.

> 
> Is java.management module pulled in due to the test library in the following 
> tests?
> test/java/lang/instrument/RedefineBigClass.sh
> test/java/lang/instrument/RedefineMethodInBacktrace.sh
> test/java/lang/instrument/RetransformBigClass.sh
> test/java/lang/instrument/NativeMethodPrefixAgent.java

The dependency is coming through test/java/lang/instrument/NMTHelper.java, 
test/java/lang/instrument/RedefineMethodInBacktraceApp.java, 
test/java/lang/instrument/NativeMethodPrefixApp.java

> 
> It’s okay to remove java.management dependency later by JDK-8139430.   At the 
> same time, would it be easier not to update the tests that require 
> java.management due to the test library and let JDK-8139430 fixes it?

See above.

I would rather push this and get on fixing JDK-8139430.

Shura


> 
> Mandy
> 
> 

RFR: 8140336: Add @modules for exported dependencies to jdk_core tests

[Alexandre (Shura) Iline]

Hi.

Could you please take a look on the suggested change in tests belonging to 
jdk_core test group.

Pls notice that the dependencies which would later be removed by fixing 
JDK-8139430 we not added in this change.

http://cr.openjdk.java.net/~shurailine/8140336/webrev.01/

Thank you.

Shura

Re: RFR: Additional tests for overlapping packages.

[Alexandre (Shura) Iline]

On Oct 1, 2015, at 6:23 PM, Alan Bateman <alan.bate...@oracle.com> wrote:

> On 23/09/2015 15:45, Alexandre (Shura) Iline wrote:
>> Hi.
>> 
>> This change adds a few more cases to existing cases for overlapping packages:
>> http://cr.openjdk.java.net/~shurailine/webrev.overlapping.packages.2/
>> 
>> 
> 

While I am working on the rest, let me double check this one.

>  2. testOverlapUpgradePath assumes that the upgrade module path can be 
> used as an alternative to the application module path. There are several 
> discussion points around the upgrade module path but I don't expect it can be 
> used to do anything other than upgrade modules that are on the system module 
> path (or linked into the runtime image). So maybe this one should be dropped 
> for now.
> 

If something is possible, it is bound to happen. Unless it is impossible to 
place a user code on the upgrade module path, somebody will.  For this, the 
test makes sense, even if it is assuming behavior beyond what is promised by 
the spec. 

Further on, if the behavior is currently undefined, then the more sense there 
is to keep the test, IMO. When the behavior changes, it is better to have the 
failing rather then to forget adding it later.

The only cases where it is better not to have this test are:
 - the test is invalid. For example there is no way to place any arbitrary code 
on the upgrade module path. 
 - the behavior is undefined so the error of package name overlap may or may 
not be reported.

Are you referring to any of these?

Shura


> 
> -Alan.