Re: RFR: jsr166 jdk9 integration wave 12

[Martin Buchholz]

On Wed, Oct 19, 2016 at 4:40 PM, Jonathan Bluett-Duncan <
jbluettdun...@gmail.com> wrote:

> Hi Martin,
>
> By collections infrastructure, do you mean something like the collection
> testers in guava-testlib?
>
> If so, I agree that JUnit 5's dynamic tests look promising for
> implementing such an infrastructure. I admit I don't have all the context
> here, but would using guava-testlib in the meantime be a viable medium- or
> short-term solution? Or would its dependence on JUnit 3/4 make switching
> impractical? Or, perhaps, has development into CollectionTest gone so far
> that, for that reason instead, it would be impractical until switch, at
> least until something superior using e.g. JUnit 5's dynamic tests is made?
>

I'm embarrassed to say I'm not familiar enough with guava's testlib.  Guava
does have generic collection oriented tests, and even runs them on jdk
classes. (Someone on the jdk quality team should be running the guava tests
using development jdks!).  I'm not familiar enough with the tools to work
on this myself, but I encourage someone who is to do that.

I see that guava testlib does have:

TestsForQueuesInJavaUtil.java
TestsForSetsInJavaUtil.java
TestsForListsInJavaUtil.java
TestsForMapsInJavaUtil.java

and that the infrastructure there is vaguely similar to what I ended up
doing.  JDK version skew is a problem, because openjdk development is
focused on jdk9, while guava is still trying to digest jdk8.

RFR 8168127: FilePermissionCollection merges incorrectly

[Wang Weijun]

Please review the code change at

   http://cr.openjdk.java.net/~weijun/8168127/webrev.00/

Two changes:

1. npath2 is considered in equals and hashCode of FilePermission, so 2 objects 
with different npath2 can be added to a map and different entries.

2. special name for newPermUsingAltPath and newPermPlusAltPath results, so 
FilePermissionCollection::add will not merge one with the original.

Thanks
Max

Re: RFR: jsr166 jdk9 integration wave 12

[Martin Buchholz]

On Wed, Oct 19, 2016 at 3:44 PM, Paul Sandoz  wrote:

>
> On 18 Oct 2016, at 13:58, Martin Buchholz  wrote:
>
> Latest webrev defers potential new methods:
>
>/* TODO: public */ private void trimToSize()
>
> Sorry to push back, i know it’s a hassle, but i think such features should
> be retained in a separate proposed patch.
>

OK.  These methods can still be found in jsr166 CVS, but are now filtered
out during upstreaming.

Re: RFR: 8164805: Fail to create a MR modular JAR with a versioned entry in base-versioned empty package

[Steve Drach]

>> On Oct 19, 2016, at 5:05 PM, Steve Drach  wrote:
>> 
 issue: https://bugs.openjdk.java.net/browse/JDK-8164805
 webrev: http://cr.openjdk.java.net/~sdrach/8164805/webrev.00/ 
>>> 
>>> Issue a warning is good in the case public classes are added in a concealed 
>>> package.  Some comments:
>>> 
>>> Main.java
>>> 
>>> addExtendedModuleAttributes does not seem to be the appropriate method to 
>>> initialize concealedPackages.  It would be better to set concealedPackages 
>>> in a separate method that will be called for each modular JAR.
>> 
>> I made a simple change to existing code, I wasn’t intending to make 
>> significant changes to jar tool.  Of course as time goes on, jar tool 
>> continues to grow into a bigger hair ball.  It would definitely benefit from 
>> major cosmetic surgery.  Perhaps I don’t understand the point you are trying 
>> to make.
> 
> It made it hard for review and future maintainability.   It was not obvious 
> to me when I reviewed the code whether this misses any case.
> 
> Refactoring it is a small change.

Just to be clear, you’d like me to take lines 1988-1995 and put them in a 
separate method that gets called by addExtendedModuleAttributes?


> 
>> 
>>> 
>>> ConcealedPackage.java test
>>> 
>>> 60 Path destination = userdir.resolve("classes”);
>>> 
>>> I suggest to use Paths.get(“classes”) rather than ${user.dir}.
>> 
>> Is there a performance advantage or some other reason for doing this?  The 
>> way I do it seems reasonable.
> 
> I just want to point out that jtreg will do the clean up if you use the 
> scratch directory.
> 
>> 
>>> jtreg will clean up the JTwork/scratch directory after the test run.
>> 
>> That’s what the docs say but I’ve seen problems in the past with windows 
>> machines, so I just got in the habit
>> of doing my own clean up.
>> 
> 
> Up to you.
> 
>>> 
>>> 63 // add an empty directory
>>> 64 
>>> Files.createDirectory(destination.resolve("p").resolve("internal"));
>>> 
>>> I suggest to take this out.  The test verifies if "jar tf mmr.jar” succeeds.
>> 
>> Ok.  Just trying to make it exactly the same as the jar structure in the bug 
>> report.
> 
> I updated the JBS issue to clarify that per the recent discussion.
> 
>> 
>>> 
>>> 92 private int jar(String cmd) {
>>> Nit: this can simply take a varargs and no need to split:
>>> jar(String… options)
>> 
>> I like the String because it’s more readable and I suspect the split isn’t 
>> that costly.
>> 
> 
> I just point out that it’s a kind of silly to concat all args and then split 
> it. 
> 
> Mandy
> 

Re: RFR: 8164805: Fail to create a MR modular JAR with a versioned entry in base-versioned empty package

[Mandy Chung]

> On Oct 19, 2016, at 5:22 PM, Steve Drach  wrote:
> 
>> 76 private String files(Path start) throws IOException {
>> Perhaps return Stream.  That can replace the unnecessary concat in a 
>> String and then later split to pass to javac.
> 
> Is this better?
> 
>private void javac(Path source, Path destination) throws IOException {
>Stream prefix = Stream.of("-d", destination.toString());
>Stream suffix = Files.isDirectory(source)
>? Files.walk(source)
>   .map(Path::toString)
>   .filter(s -> s.endsWith(".java"))
>: Stream.of(source.toString());
>String[] args = Stream.concat(prefix, suffix).toArray(String[]::new);
>JAVAC_TOOL.run(System.out, System.err, args);
>}


This is okay.

Mandy

Re: RFR: 8164805: Fail to create a MR modular JAR with a versioned entry in base-versioned empty package

[Mandy Chung]

> On Oct 19, 2016, at 5:05 PM, Steve Drach  wrote:
> 
>>> issue: https://bugs.openjdk.java.net/browse/JDK-8164805
>>> webrev: http://cr.openjdk.java.net/~sdrach/8164805/webrev.00/ 
>> 
>> Issue a warning is good in the case public classes are added in a concealed 
>> package.  Some comments:
>> 
>> Main.java
>> 
>> addExtendedModuleAttributes does not seem to be the appropriate method to 
>> initialize concealedPackages.  It would be better to set concealedPackages 
>> in a separate method that will be called for each modular JAR.
> 
> I made a simple change to existing code, I wasn’t intending to make 
> significant changes to jar tool.  Of course as time goes on, jar tool 
> continues to grow into a bigger hair ball.  It would definitely benefit from 
> major cosmetic surgery.  Perhaps I don’t understand the point you are trying 
> to make.

It made it hard for review and future maintainability.   It was not obvious to 
me when I reviewed the code whether this misses any case.

Refactoring it is a small change.

> 
>> 
>> ConcealedPackage.java test
>> 
>> 60 Path destination = userdir.resolve("classes”);
>> 
>> I suggest to use Paths.get(“classes”) rather than ${user.dir}.
> 
> Is there a performance advantage or some other reason for doing this?  The 
> way I do it seems reasonable.

I just want to point out that jtreg will do the clean up if you use the scratch 
directory.

> 
>> jtreg will clean up the JTwork/scratch directory after the test run.
> 
> That’s what the docs say but I’ve seen problems in the past with windows 
> machines, so I just got in the habit
> of doing my own clean up.
> 

Up to you.

>> 
>> 63 // add an empty directory
>> 64 
>> Files.createDirectory(destination.resolve("p").resolve("internal"));
>> 
>> I suggest to take this out.  The test verifies if "jar tf mmr.jar” succeeds.
> 
> Ok.  Just trying to make it exactly the same as the jar structure in the bug 
> report.

I updated the JBS issue to clarify that per the recent discussion.

> 
>> 
>> 92 private int jar(String cmd) {
>> Nit: this can simply take a varargs and no need to split:
>> jar(String… options)
> 
> I like the String because it’s more readable and I suspect the split isn’t 
> that costly.
> 

I just point out that it’s a kind of silly to concat all args and then split 
it. 

Mandy

Re: Review Request: JDK-8167057 jdeps to list the modules and internal APIs to help find @modules for tests

[Mandy Chung]

> On Oct 19, 2016, at 4:22 PM, Alexandre (Shura) Iline 
>  wrote:
> 
> Mandy,
> 
> I have a question around line 228 of 
> src/jdk.jdeps/share/classes/com/sun/tools/jdeps/JdepsTask.java:
> 224 new Option(false, "--list-deps", "--list-reduced-deps") {
> 225 void process(JdepsTask task, String opt, String arg) {
> 226 task.options.showModulesAddExports = true;
> 227 task.options.reduced = opt.equals("--list-reduced-deps");
> 228 task.options.verbose = PACKAGE;
> 229 }
> 230 },
> 
> What is the expected behavior of jdeps if I use ‘—list-jdeps’ at the same 
> time as, say, ‘-v’? 
> 

-v (or other verbose options) specifies the granularity of analysis.  
-—list-deps only does package-level analysis and so verbose option has 
essentially no effect.


> Should there be more checks similar to these?
> 483 if ((options.findJDKInternals) && (options.hasFilter() || 
> options.showSummary)) {
> 484 showHelp();
> 485 return EXIT_CMDERR;
> 486 }
> 487 if (options.showSummary && options.verbose != SUMMARY) {
> 488 showHelp();
> 489 return EXIT_CMDERR;
> 490 }
> 
> Looking on already existing options, with the current implementation, for 
> ‘-s’ and -v’ options:
> ‘-v' ‘-s’ causes ‘-v' to be ignored

> ‘-s' ‘-v’ is not allowed.
> 

-s and -v are conflicting options and both cases should not be allowed.  It’s a 
bug.

> If used with ‘-jdkinternals', ‘-s’ is not allowed, while ‘-v’ is ignored.

-jdkinternals and -v and -s are conflicting options.  Can you file an issue for 
this bug?  I will need to take a pass on all options and should also consider 
subcommands.

Mandy

Re: RFR: 8164805: Fail to create a MR modular JAR with a versioned entry in base-versioned empty package

[Steve Drach]

>  76 private String files(Path start) throws IOException {
> Perhaps return Stream.  That can replace the unnecessary concat in a 
> String and then later split to pass to javac.

Is this better?

private void javac(Path source, Path destination) throws IOException {
Stream prefix = Stream.of("-d", destination.toString());
Stream suffix = Files.isDirectory(source)
? Files.walk(source)
   .map(Path::toString)
   .filter(s -> s.endsWith(".java"))
: Stream.of(source.toString());
String[] args = Stream.concat(prefix, suffix).toArray(String[]::new);
JAVAC_TOOL.run(System.out, System.err, args);
}

Re: RFR: 8164805: Fail to create a MR modular JAR with a versioned entry in base-versioned empty package

[Steve Drach]

>> issue: https://bugs.openjdk.java.net/browse/JDK-8164805
>> webrev: http://cr.openjdk.java.net/~sdrach/8164805/webrev.00/ 
> 
> Issue a warning is good in the case public classes are added in a concealed 
> package.  Some comments:
> 
> Main.java
> 
> addExtendedModuleAttributes does not seem to be the appropriate method to 
> initialize concealedPackages.  It would be better to set concealedPackages in 
> a separate method that will be called for each modular JAR.

I made a simple change to existing code, I wasn’t intending to make significant 
changes to jar tool.  Of course as time goes on, jar tool continues to grow 
into a bigger hair ball.  It would definitely benefit from major cosmetic 
surgery.  Perhaps I don’t understand the point you are trying to make.

> 
> Validator.java
> 160 
> main.error(Main.formatMsg("error.validator.concealed.public.class", 
> entryName));
> 
> This is a warning.  You should add a new warn method to make it clear.

Ok.

> Similiarly,  “error.validator.concealed.public.class” should be renamed to 
> “warn.validator.concealed.public.class”.  I notice there are several keys 
> with “error.validator” prefix that are meant for warnings.  It’d be good to  
> change them too.  
> 
> 247 if (idx > -1) {
> 248 String pkgName = internalName.substring(0, idx).replace('/', 
> '.');
> 249 if (main.concealedPackages.contains(pkgName)) {
> 250 return true;
> 251 }
> 252 }
> 
> Alternative impl for the above lines:
> 
> String pkgName = idx != -1 ? internalName.substring(0, idx).replace('/', '.’)
>  : “”;
> return main.concealedPackages.contains(pkgName);

Ok.

> 
> jar.properties
> 
> 112 error.validator.concealed.public.class=\
> 113  warning - entry {0} is a public class in a concealed package, 
> placing this \
> 114  jar on the class path will result in incompatible public 
> interfaces
> 
> line 113 needs new line ‘\n’.  You may break it into 3 lines
> since the entry name will take up some characters.

Ok.

> 
> ConcealedPackage.java test
> 
>  60 Path destination = userdir.resolve("classes”);
> 
> I suggest to use Paths.get(“classes”) rather than ${user.dir}.

Is there a performance advantage or some other reason for doing this?  The way 
I do it seems reasonable.

>  jtreg will clean up the JTwork/scratch directory after the test run.

That’s what the docs say but I’ve seen problems in the past with windows 
machines, so I just got in the habit
of doing my own clean up.

> 
>  63 // add an empty directory
>  64 
> Files.createDirectory(destination.resolve("p").resolve("internal"));
> 
> I suggest to take this out.  The test verifies if "jar tf mmr.jar” succeeds.

Ok.  Just trying to make it exactly the same as the jar structure in the bug 
report.

> 
> The test should test both “cf” and “uf” and verify that the ConcealedPackage 
> attributes in module-info.class is updated correctly (one single 
> module-info.class case in the root entry and two module-info.class - root and 
> versioned).

Ok.

> 
>  92 private int jar(String cmd) {
> Nit: this can simply take a varargs and no need to split:
>  jar(String… options)

I like the String because it’s more readable and I suspect the split isn’t that 
costly.

> 
> 
>  76 private String files(Path start) throws IOException {
> Perhaps return Stream.  That can replace the unnecessary concat in a 
> String and then later split to pass to javac.

What we probably want is stream.toArray(String[]::new), but then we’d have to 
copy that array into a new array that is 2 elements larger to accommodate the 
destination parameter.  I wonder if that’s any better.  I need to think about 
it.

> 
> Mandy

Re: RFR: jsr166 jdk9 integration wave 12

[Jonathan Bluett-Duncan]

Hi Martin,

By collections infrastructure, do you mean something like the collection
testers in guava-testlib?

If so, I agree that JUnit 5's dynamic tests look promising for implementing
such an infrastructure. I admit I don't have all the context here, but
would using guava-testlib in the meantime be a viable medium- or short-term
solution? Or would its dependence on JUnit 3/4 make switching impractical?
Or, perhaps, has development into CollectionTest gone so far that, for that
reason instead, it would be impractical until switch, at least until
something superior using e.g. JUnit 5's dynamic tests is made?

Kind regards,
Jonathan

On 19 October 2016 at 23:21, Martin Buchholz  wrote:

> These tests are maintained outside of openjdk and have only recently been
> made available within openjdk.
>
> There may be a future where all the consumers of these tests are willing to
> accept a good test framework.  Junit 5 dynamic tests look promising
> http://junit.org/junit5/docs/current/user-guide/#writing-
> tests-dynamic-tests
> but it may be a decade before we can use it.
>
> Probably code bases that include interfaces (e.g. Collection) should
> provide infrastructure to test those interfaces for any implementation, but
> openjdk does not try to do so.  CollectionTest is an experimental step in
> that direction, but it looks we could do better with Junit 5 some day.  The
> openjdk and jtreg projects can try to help by supporting junit 5 earlier
> rather than later.
>
> On Wed, Oct 19, 2016 at 12:19 PM, Stuart Marks 
> wrote:
>
> >
> >
> > On 10/18/16 11:00 AM, Martin Buchholz wrote:
> >
> >> There's already a certain amount of mixing, e.g. stream tests sometimes
> >> test
> >> j.u.c. and Queue tests sometimes test all the Queue implementations.
> >> Unlike
> >> non-test code, some redundancy and divergence of testing frameworks and
> >> styles
> >> is fine.  I still haven't found a way of writing shared tests for
> >> implementations of an interface that I really like.
> >>
> >
> > It's probably the case that divergence of testing frameworks is
> > unavoidable, or at least it's impractical. I doubt that it's worthwhile
> to
> > rewrite all the straight jtreg tests into TestNG, or JUnit, or whatever.
> > But I'd draw the line before saying this is "fine." [1]
> >
> > Maintaining the tests' organization is pretty important. Most of the
> > collections tests are in jdk/test/java/util though they're spread around
> a
> > bit uncomfortably even here. But now it's, oh, ArrayDeque and
> > ArrayList.removeIf tests are over *here* instead. Having things spread
> > around increases the likelihood of cases being missed or being tested
> > redundantly.
> >
> > The test groups have to be maintained as well. I know I've been bitten by
> > problems (not in collections) where I *thought* I had run the right set
> of
> > tests, but it ended up that I hadn't, resulting in me breaking the build.
> > As it turns out, jdk_collections_core doesn't include any ArrayDeque
> tests.
> > Hmmm. Well, maybe jdk_collections_core isn't useful. It would have been
> > nice to know this when I added it last year. :-/ [2]
> >
> > As things stand, I'm basically OK with this changeset going in. But it
> > does seem to highlight some areas that need some future cleanup,
> > particularly in the tests and the test group declarations.
> >
> > s'marks
> >
> > [1] http://knowyourmeme.com/memes/this-is-fine
> >
> > [2] http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/7a0c06013ae6
> >
> >
>

Re: [9] RfR: 8165271: Fix use of reflection to gain access to private fields

[Mandy Chung]

> On Oct 19, 2016, at 3:50 PM, David DeHaven  wrote:
> 
> 
> Please review the new patch version:
> http://cr.openjdk.java.net/~ddehaven/8165271/jdk.1/
> 

Looks okay but I think the new interface should be named 
“JavaNetURLClassLoaderAccess” (matching “URLClassLoader")

> This was updated for the recent changes due to JDK-8168073. Note that the 
> patch is against jdk9-client but I can apply and push to 9-dev. Test runs 
> with both client and jake were successful.

Pushing to jdk9/dev would avoid any merge if any change to related area is made 
before the next jdk9/client integration to jdk9/dev.

Mandy

Re: RFR: 8164805: Fail to create a MR modular JAR with a versioned entry in base-versioned empty package

[Mandy Chung]

> On Oct 19, 2016, at 11:34 AM, Steve Drach  wrote:
> 
> issue: https://bugs.openjdk.java.net/browse/JDK-8164805
> webrev: http://cr.openjdk.java.net/~sdrach/8164805/webrev.00/ 

Issue a warning is good in the case public classes are added in a concealed 
package.  Some comments:

Main.java

addExtendedModuleAttributes does not seem to be the appropriate method to 
initialize concealedPackages.  It would be better to set concealedPackages in a 
separate method that will be called for each modular JAR.

Validator.java
 160 
main.error(Main.formatMsg("error.validator.concealed.public.class", entryName));

This is a warning.  You should add a new warn method to make it clear.
Similiarly,  “error.validator.concealed.public.class” should be renamed to 
“warn.validator.concealed.public.class”.  I notice there are several keys with 
“error.validator” prefix that are meant for warnings.  It’d be good to  change 
them too.  

 247 if (idx > -1) {
 248 String pkgName = internalName.substring(0, idx).replace('/', 
'.');
 249 if (main.concealedPackages.contains(pkgName)) {
 250 return true;
 251 }
 252 }

Alternative impl for the above lines:

String pkgName = idx != -1 ? internalName.substring(0, idx).replace('/', '.’)
   : “”;
return main.concealedPackages.contains(pkgName);

jar.properties

112 error.validator.concealed.public.class=\
 113  warning - entry {0} is a public class in a concealed package, 
placing this \
 114  jar on the class path will result in incompatible public 
interfaces

line 113 needs new line ‘\n’.  You may break it into 3 lines
since the entry name will take up some characters.

ConcealedPackage.java test

  60 Path destination = userdir.resolve("classes”);

I suggest to use Paths.get(“classes”) rather than ${user.dir}.  jtreg will 
clean up the JTwork/scratch directory after the test run.

  63 // add an empty directory
  64 
Files.createDirectory(destination.resolve("p").resolve("internal"));

I suggest to take this out.  The test verifies if "jar tf mmr.jar” succeeds.

The test should test both “cf” and “uf” and verify that the ConcealedPackage 
attributes in module-info.class is updated correctly (one single 
module-info.class case in the root entry and two module-info.class - root and 
versioned).

  92 private int jar(String cmd) {
Nit: this can simply take a varargs and no need to split:
  jar(String… options)


  76 private String files(Path start) throws IOException {
Perhaps return Stream.  That can replace the unnecessary concat in a 
String and then later split to pass to javac.

Mandy

Re: Review Request: JDK-8167057 jdeps to list the modules and internal APIs to help find @modules for tests

[Alexandre (Shura) Iline]

Mandy,

I have a question around line 228 of 
src/jdk.jdeps/share/classes/com/sun/tools/jdeps/JdepsTask.java:
 224 new Option(false, "--list-deps", "--list-reduced-deps") {
 225 void process(JdepsTask task, String opt, String arg) {
 226 task.options.showModulesAddExports = true;
 227 task.options.reduced = opt.equals("--list-reduced-deps");
 228 task.options.verbose = PACKAGE;
 229 }
 230 },

What is the expected behavior of jdeps if I use ‘—list-jdeps’ at the same time 
as, say, ‘-v’? 

Should there be more checks similar to these?
 483 if ((options.findJDKInternals) && (options.hasFilter() || 
options.showSummary)) {
 484 showHelp();
 485 return EXIT_CMDERR;
 486 }
 487 if (options.showSummary && options.verbose != SUMMARY) {
 488 showHelp();
 489 return EXIT_CMDERR;
 490 }

Looking on already existing options, with the current implementation, for ‘-s’ 
and -v’ options:
‘-v' ‘-s’ causes ‘-v' to be ignored
‘-s' ‘-v’ is not allowed.

If used with ‘-jdkinternals', ‘-s’ is not allowed, while ‘-v’ is ignored.

Is this the intended behavior?

Shura


> On Oct 19, 2016, at 3:19 PM, Mandy Chung  wrote:
> 
> Webrev at:
>  http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8167057/webrev.00/
> 
> This patch enhances jdeps to print the dependences in the format : 
> $MODULE[/$PACKAGE].
> 
> This is intended for analyzing the regression tests we develop and add make 
> it easy to add the proper @modules.
> 
> Mandy

Re: [9] RfR: 8165271: Fix use of reflection to gain access to private fields

[David DeHaven]

Please review the new patch version:
http://cr.openjdk.java.net/~ddehaven/8165271/jdk.1/

This was updated for the recent changes due to JDK-8168073. Note that the patch 
is against jdk9-client but I can apply and push to 9-dev. Test runs with both 
client and jake were successful.

-DrD-

> On Oct 13, 2016, at 5:24 PM, Mandy Chung  wrote:
> 
> 
>> On Oct 13, 2016, at 4:08 PM, David DeHaven  wrote:
>> 
>> 
>> JBS Issue:
>> https://bugs.openjdk.java.net/browse/JDK-8165271
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~ddehaven/8165271/jdk.0/
> 
> This change looks fine.  This hack should have been replaced long ago and 
> happy to see them gone.
> 
> Mandy
> 

Re: RFR: jsr166 jdk9 integration wave 12

[Paul Sandoz]

> On 18 Oct 2016, at 13:58, Martin Buchholz  wrote:
> 
> Latest webrev defers potential new methods:
>/* TODO: public */ private void trimToSize()

Sorry to push back, i know it’s a hassle, but i think such features should be 
retained in a separate proposed patch.

Paul.

Re: RFR: jsr166 jdk9 integration wave 12

[Martin Buchholz]

These tests are maintained outside of openjdk and have only recently been
made available within openjdk.

There may be a future where all the consumers of these tests are willing to
accept a good test framework.  Junit 5 dynamic tests look promising
http://junit.org/junit5/docs/current/user-guide/#writing-tests-dynamic-tests
but it may be a decade before we can use it.

Probably code bases that include interfaces (e.g. Collection) should
provide infrastructure to test those interfaces for any implementation, but
openjdk does not try to do so.  CollectionTest is an experimental step in
that direction, but it looks we could do better with Junit 5 some day.  The
openjdk and jtreg projects can try to help by supporting junit 5 earlier
rather than later.

On Wed, Oct 19, 2016 at 12:19 PM, Stuart Marks 
wrote:

>
>
> On 10/18/16 11:00 AM, Martin Buchholz wrote:
>
>> There's already a certain amount of mixing, e.g. stream tests sometimes
>> test
>> j.u.c. and Queue tests sometimes test all the Queue implementations.
>> Unlike
>> non-test code, some redundancy and divergence of testing frameworks and
>> styles
>> is fine.  I still haven't found a way of writing shared tests for
>> implementations of an interface that I really like.
>>
>
> It's probably the case that divergence of testing frameworks is
> unavoidable, or at least it's impractical. I doubt that it's worthwhile to
> rewrite all the straight jtreg tests into TestNG, or JUnit, or whatever.
> But I'd draw the line before saying this is "fine." [1]
>
> Maintaining the tests' organization is pretty important. Most of the
> collections tests are in jdk/test/java/util though they're spread around a
> bit uncomfortably even here. But now it's, oh, ArrayDeque and
> ArrayList.removeIf tests are over *here* instead. Having things spread
> around increases the likelihood of cases being missed or being tested
> redundantly.
>
> The test groups have to be maintained as well. I know I've been bitten by
> problems (not in collections) where I *thought* I had run the right set of
> tests, but it ended up that I hadn't, resulting in me breaking the build.
> As it turns out, jdk_collections_core doesn't include any ArrayDeque tests.
> Hmmm. Well, maybe jdk_collections_core isn't useful. It would have been
> nice to know this when I added it last year. :-/ [2]
>
> As things stand, I'm basically OK with this changeset going in. But it
> does seem to highlight some areas that need some future cleanup,
> particularly in the tests and the test group declarations.
>
> s'marks
>
> [1] http://knowyourmeme.com/memes/this-is-fine
>
> [2] http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/7a0c06013ae6
>
>

Review Request: JDK-8167057 jdeps to list the modules and internal APIs to help find @modules for tests

[Mandy Chung]

Webrev at:
  http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8167057/webrev.00/

This patch enhances jdeps to print the dependences in the format : 
$MODULE[/$PACKAGE].

This is intended for analyzing the regression tests we develop and add make it 
easy to add the proper @modules.

Mandy

Re: RFR (JAXP): 8167179: Make XSL generated namespace prefixes local to transformation process

[Joe Wang]

Hi Aleksej,

The change looks good.

Thanks,
Joe

On 10/19/16, 7:19 AM, Aleks Efimov wrote:

Hi,
Please help to review the JDK9 fix for JDK-8167179 [1]:
http://cr.openjdk.java.net/~aefimov/8167179/9/00/:
The fix changes how the xsl:element namespace prefix values are 
generated during the transformation process.


JDK9 build that includes this fix was tested with JDK XML related 
tests and with JCK-runtime/devtools suites. No failures were observed 
among these tests.


With Best Regards,
Aleksej

[1] https://bugs.openjdk.java.net/browse/JDK-8167179

Re: JDK 9 org.omg.CORBA.ORBSingletonClass loading will not use context class loader

[Tom Hood]

I haven't tried JDK 9 yet.  I was assuming the 7u55 change in JDK 9 would
impact our application the same way.  I hope to try it next week as I'm
currently stuck testing on windows XP which limits me to 1.7 or earlier.
So maybe Approach #3 will be:  "It should just work"  [*I think I'm
required to add a quarter to some jar whenever I use that phrase*]:-)

That's good  to know about the java.corba module.  The vb jar does carry a
bunch of org.omg.* classes, so it sounds like with JDK 9 everything corba
related will be taken from the vb jar (assuming I don't explicitly use
--add-modules).

The realization I'm now having is that JDK 9 will use more code from the vb
jar than earlier versions did which I think will be a good thing.  The
scary thought is that right now we must be running in production with a
Frankenstein mix of some org.omg.* classes from the vb jar and some from
the JDK which could in theory vary across JDK updates, making the whole
thing more fragile in my mind.  I believe the parent-first model is used by
the JNLPClassLoader will choose the JDK version of a class over one with
the same name from the vb jar.  So the mixing of different ORB
implementations I was so concerned about is probably already the way we're
running?  Yikes!

That's also good to know about JDK 9 application class loader not being an
instance of URLClassLoader.  I'll probably use approach #1 when jre is
older than 9 so our app will work with the select versions of 6, 7, and 8
with the ORB.init() change.

Any thoughts on question 2 of approach #1 (i.e. disabling security manager
vs. granting the application AllPermission via policy file)?  It seems like
they should be equivalent.  I'm told it is near impossible to reliably get
IT at customer facilities to make desired modifications to the jre
installation, such as a policy file.

Thanks for all your help.  Looking forward to trying JDK 9 soon.


On Wed, Oct 19, 2016 at 12:48 AM, Alan Bateman 
wrote:

> On 19/10/2016 03:28, Tom Hood wrote:
>
> It's unclear if there really is an interop issue.   The application will
> launch with 7u55 if I don't set the ORBSingletonClass property, although
> that isn't how visibroker 5.2.1 was intended to run, so not setting it
> worries me.
>
> Our application does call ORB.init(args,props) once at startup and use
> that instance throughout the code we have written.
>
> However, visibroker calls ORB.init() all over the idl generated code and
> the vb jar itself.
>
> The javap of the vb jar does show some downcasts to its
> com.inprise.vbroker.orb.ORBSingleton implementation in a few classes, but
> I haven't tracked down if that code could execute in our application or if
> the object it tries to downcast originated from ORB.init().  The simple
> test of a basic launch and performing some activities within the
> application seem to work.  Sadly we don't have any automated tests that
> exercise our use of CORBA (java client to/from c++ server).
>
> The idea of mixing ORB implementations worries me, even if only for the
> singleton ORB.  I'm new to CORBA and the client side of our application and
> still a newbie to java as I've been primarily working on our c++ server.
>
> Do the reasons for the JDK change to ORB.init() apply in our use case?
> There's only one application/context and ORB vendor implementation running
> in the jvm in production.  Do you think approach #1 might be a reasonably
> safe workaround without the risk of hidden interop issues?
>
> I can't tell from this thread if you have tried with JDK 9 or not (you've
> mentioned 7u55 a few times).
>
> I ask because in JDK 9 then the java.corba module is not resolved by
> default. If your application starts there then it suggests to me that
> Visibroker may be carrying its own copy of the org.omg.** (and RMI-IIOP)
> classes, in which case the JDK ORB (even singleton ORB) is not in the
> picture. Or maybe you've got past this and your JNLP contains:
>   
>
> which is the equivalent to the --add-modules command line option.
>
> Another comment is that if there is code calling the no-arg ORB.init and
> blinding casting the return to com.inprise.vbroker.orb.ORBSingleton then
> it will never work in the scenario where the system-wide singleton ORB has
> been initialized by before Visibroker is used. I completely agree that it
> would be unusual to mix different ORB implementations in the same VM but
> the API has always allowed for that.
>
> On approach #1 then one thing to be aware of is that the application class
> loader is not an instance of URLClassLoader in JDK 9. That is an
> implementation change that is known to trip up code that has been hacking
> into it.
>
> -Alan
>

RFR 9: 8164908: ReflectionFactory support for IIOP and custom serialization

[Roger Riggs]

The support in sun.reflect.ReflectionFactory for custom serialization, 
such as IIOP input
and output streams, is being expanded beyond the necessary constructor 
of a serializable
class to include access to the private methods readObject, writeObject, 
readResolve,

writeReplace, etc.

The IIOP implementation is updated to use a combination of 
ReflectionFactory and
Unsafe to serialize and deserialize objects and no longer rely on 
setAccessible.

Tests are included for ReflectionFactory and the affected IIOP classes.

Please review and comment,

jdk repo webrev:
http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-8164908/
corba repo webrev :
http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-iiop-8164908/

Issue:
https://bugs.openjdk.java.net/browse/JDK-8164908

Thanks, Roger

Re: RFR: jsr166 jdk9 integration wave 12

[Stuart Marks]

On 10/18/16 11:00 AM, Martin Buchholz wrote:

There's already a certain amount of mixing, e.g. stream tests sometimes test
j.u.c. and Queue tests sometimes test all the Queue implementations. Unlike
non-test code, some redundancy and divergence of testing frameworks and styles
is fine.  I still haven't found a way of writing shared tests for
implementations of an interface that I really like.


It's probably the case that divergence of testing frameworks is unavoidable, or 
at least it's impractical. I doubt that it's worthwhile to rewrite all the 
straight jtreg tests into TestNG, or JUnit, or whatever. But I'd draw the line 
before saying this is "fine." [1]


Maintaining the tests' organization is pretty important. Most of the collections 
tests are in jdk/test/java/util though they're spread around a bit uncomfortably 
even here. But now it's, oh, ArrayDeque and ArrayList.removeIf tests are over 
*here* instead. Having things spread around increases the likelihood of cases 
being missed or being tested redundantly.


The test groups have to be maintained as well. I know I've been bitten by 
problems (not in collections) where I *thought* I had run the right set of 
tests, but it ended up that I hadn't, resulting in me breaking the build. As it 
turns out, jdk_collections_core doesn't include any ArrayDeque tests. Hmmm. 
Well, maybe jdk_collections_core isn't useful. It would have been nice to know 
this when I added it last year. :-/ [2]


As things stand, I'm basically OK with this changeset going in. But it does seem 
to highlight some areas that need some future cleanup, particularly in the tests 
and the test group declarations.


s'marks

[1] http://knowyourmeme.com/memes/this-is-fine

[2] http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/7a0c06013ae6

RFR: 8164805: Fail to create a MR modular JAR with a versioned entry in base-versioned empty package

[Steve Drach]

Hi,

Please review the following changeset.  This fix allows jar tool to add a new 
public class in a versioned directory in a modular multi-release jar file if 
the class is in a concealed package. When this event takes place, a warning 
message is emitted saying that placing this jar on a class path will result in 
an incompatible public interface, the jar file does not meet the single api 
rule of a multi-release jar file.  As before, adding a new public class in a 
versioned directory of a multi-release jar file (not a modular jar file) will 
result in an error.

issue: https://bugs.openjdk.java.net/browse/JDK-8164805 


webrev: http://cr.openjdk.java.net/~sdrach/8164805/webrev.00/ 


Thanks,
Steve

RFR (JAXP): 8167179: Make XSL generated namespace prefixes local to transformation process

[Aleks Efimov]

Hi,
Please help to review the JDK9 fix for JDK-8167179 [1]:
http://cr.openjdk.java.net/~aefimov/8167179/9/00/:
The fix changes how the xsl:element namespace prefix values are 
generated during the transformation process.


JDK9 build that includes this fix was tested with JDK XML related tests 
and with JCK-runtime/devtools suites. No failures were observed among 
these tests.


With Best Regards,
Aleksej

[1] https://bugs.openjdk.java.net/browse/JDK-8167179

Re: RFR 8071588: The spec for javax.script.ScriptEngineFactory.getProgram() should specify NPEs thrown

[Sundararajan Athijegannathan]

Hi Alan,

Thanks for your review. Yes, spec. change is being tracked. I'll remove 
that whitespace after doc comment of getProgram method and push it.


PS. Updated webrev for the record: 
http://cr.openjdk.java.net/~sundar/8071588/jdk/webrev.01/


Thanks,
-Sundar

On 19/10/16, 12:42 PM, Alan Bateman wrote:

On 19/10/2016 06:06, Sundararajan Athijegannathan wrote:


Please review.

Bug: https://bugs.openjdk.java.net/browse/JDK-8071588

jdk webrev: http://cr.openjdk.java.net/~sundar/8071588/jdk/webrev.00/

nashorn webrev:
http://cr.openjdk.java.net/~sundar/8071588/nashorn/webrev.00/

This looks okay (except the spurious blank line after the class 
comment). It's a spec change that will need to be tracked.


-Alan

Re: JDK 9 org.omg.CORBA.ORBSingletonClass loading will not use context class loader

[Alan Bateman]

On 19/10/2016 03:28, Tom Hood wrote:

It's unclear if there really is an interop issue.   The application 
will launch with 7u55 if I don't set the ORBSingletonClass property, 
although that isn't how visibroker 5.2.1 was intended to run, so not 
setting it worries me.


Our application does call ORB.init(args,props) once at startup and use 
that instance throughout the code we have written.


However, visibroker calls ORB.init() all over the idl generated code 
and the vb jar itself.


The javap of the vb jar does show some downcasts to its 
com.inprise.vbroker.orb.ORBSingleton implementation in a few classes, 
but I haven't tracked down if that code could execute in our 
application or if the object it tries to downcast originated from 
ORB.init().  The simple test of a basic launch and performing some 
activities within the application seem to work.  Sadly we don't have 
any automated tests that exercise our use of CORBA (java client 
to/from c++ server).


The idea of mixing ORB implementations worries me, even if only for 
the singleton ORB.  I'm new to CORBA and the client side of our 
application and still a newbie to java as I've been primarily working 
on our c++ server.


Do the reasons for the JDK change to ORB.init() apply in our use 
case?  There's only one application/context and ORB vendor 
implementation running in the jvm in production.  Do you think 
approach #1 might be a reasonably safe workaround without the risk of 
hidden interop issues?


I can't tell from this thread if you have tried with JDK 9 or not 
(you've mentioned 7u55 a few times).


I ask because in JDK 9 then the java.corba module is not resolved by 
default. If your application starts there then it suggests to me that 
Visibroker may be carrying its own copy of the org.omg.** (and RMI-IIOP) 
classes, in which case the JDK ORB (even singleton ORB) is not in the 
picture. Or maybe you've got past this and your JNLP contains:

  

which is the equivalent to the --add-modules command line option.

Another comment is that if there is code calling the no-arg ORB.init and 
blinding casting the return to com.inprise.vbroker.orb.ORBSingleton then 
it will never work in the scenario where the system-wide singleton ORB 
has been initialized by before Visibroker is used. I completely agree 
that it would be unusual to mix different ORB implementations in the 
same VM but the API has always allowed for that.


On approach #1 then one thing to be aware of is that the application 
class loader is not an instance of URLClassLoader in JDK 9. That is an 
implementation change that is known to trip up code that has been 
hacking into it.


-Alan

Re: RFR 8071588: The spec for javax.script.ScriptEngineFactory.getProgram() should specify NPEs thrown

[Alan Bateman]

On 19/10/2016 06:06, Sundararajan Athijegannathan wrote:


Please review.

Bug: https://bugs.openjdk.java.net/browse/JDK-8071588

jdk webrev: http://cr.openjdk.java.net/~sundar/8071588/jdk/webrev.00/

nashorn webrev:
http://cr.openjdk.java.net/~sundar/8071588/nashorn/webrev.00/

This looks okay (except the spurious blank line after the class 
comment). It's a spec change that will need to be tracked.


-Alan

Re: RFR 8071588: The spec for javax.script.ScriptEngineFactory.getProgram() should specify NPEs thrown

[Hannes Wallnöfer]

+1

Hannes

> Am 19.10.2016 um 07:06 schrieb Sundararajan Athijegannathan 
> :
> 
> Please review.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8071588
> 
> jdk webrev: http://cr.openjdk.java.net/~sundar/8071588/jdk/webrev.00/
> 
> nashorn webrev:
> http://cr.openjdk.java.net/~sundar/8071588/nashorn/webrev.00/
> 
> Thanks
> 
> -Sundar
> 

Re: RFR: 8163984: Fix license and copyright headers in jdk9 under test/lib

[Stanislav Smirnov]

David, thank you

Best regards,
Stanislav Smirnov





> On 19 Oct 2016, at 04:27, David Holmes  wrote:
> 
> Hi Stanislav,
> 
> On 19/10/2016 1:06 AM, Stanislav Smirnov wrote:
>> Hi,
>> 
>> I'm still looking for volunteers to review
> 
> This is trivial. Reviewed. I will push for you, to jdk9/dev.
> 
> Thanks,
> David
> 
>> Best regards,
>> Stanislav Smirnov
>> 
>> 
>> 
>> 
>> 
>>> On 05 Oct 2016, at 19:44, Stanislav Smirnov  
>>> wrote:
>>> 
>>> Hi,
>>> 
>>> Please review this fix for JDK-8163984 
>>> .
>>> This one is similar to another one, I have sent earlier.
>>> Legal notices and Oracle copyrights were updated (white and blank space, 
>>> commas) in tests files under test/lib for uniformity to meet Oracle 
>>> requirements.
>>> 
>>> JBS bug: https://bugs.openjdk.java.net/browse/JDK-8163984 
>>> 
>>> Webrev: http://cr.openjdk.java.net/~stsmirno/8163984/webrev.00/ 
>>> 
>>> Best regards,
>>> Stanislav Smirnov
>>> 
>>> 
>>> 
>>> 
>>> 
>>