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

[Steve Drach]

 There is a new webrev at 
 http://cr.openjdk.java.net/~sdrach/8164805/webrev.01/ 
>>> 
>>> sun/tools/jar/Main.java
>>> 
>>> Thanks for refactoring and adding the findConcealedPackages method.  What I 
>>> actually meant was to move out this line:
>>>  concealedPackages = findConcealedPackages(rd);
>>> 
>>> to probably before calling addExtendedModuleAttributes(moduleInfos) above 
>>> line 342 and 1101.
>> 
>> I tried that and decided it was non-optimal because we’d have to construct 
>> the ModuleDescriptor from the modInfos twice in succession.  Let's 
>> compromise here, okay?
> 
> OK.  It wasn’t clear to me that you considered that.  It’s fine to leave it 
> for a future clean up.
> 
>> 
>>> 
>>> 2014 .filter(p -> !p.equals("”))
>> 
>> 
>>> 
>>> For a modular JAR, there should be no unnamed package.  I think the jar 
>>> tool should fail if it detects an unnamed package.  Your test does not have 
>>> any unnamed package - how did you find this?
>> 
>> the modularJar/Basic test found a bug.  Then when I was fixing the bug in 
>> toPackageNames I noticed it could return unnamed packages (“”).  And in fact 
>> there are some, a few, classes that aren’t in a package.  Jar tool shouldn’t 
>> fail with unnamed packages.
> 
> I meant for modular JAR.
> 
>> They could even exist in a modular multi-release jar when the module-info 
>> class is only in a versioned directory.  
> 
> module-info.class should be filtered before calling toPackageName.

It is, although not in a stream.  My inspection of usages led me to an unused 
method, findPackages, that I will remove (I did so and all tests passed).

> 
>> I guess it should fail if a class in an unnamed package is in a module, 
>> although I’m not even sure about that.
> 
> 
> Mandy

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

[Mandy Chung]

> On Oct 24, 2016, at 1:39 PM, Steve Drach  wrote:
> 
>>> There is a new webrev at 
>>> http://cr.openjdk.java.net/~sdrach/8164805/webrev.01/ 
>> 
>> sun/tools/jar/Main.java
>> 
>> Thanks for refactoring and adding the findConcealedPackages method.  What I 
>> actually meant was to move out this line:
>>   concealedPackages = findConcealedPackages(rd);
>> 
>> to probably before calling addExtendedModuleAttributes(moduleInfos) above 
>> line 342 and 1101.
> 
> I tried that and decided it was non-optimal because we’d have to construct 
> the ModuleDescriptor from the modInfos twice in succession.  Let's compromise 
> here, okay?

OK.  It wasn’t clear to me that you considered that.  It’s fine to leave it for 
a future clean up.

> 
>> 
>> 2014 .filter(p -> !p.equals("”))
> 
> 
>> 
>> For a modular JAR, there should be no unnamed package.  I think the jar tool 
>> should fail if it detects an unnamed package.  Your test does not have any 
>> unnamed package - how did you find this?
> 
> the modularJar/Basic test found a bug.  Then when I was fixing the bug in 
> toPackageNames I noticed it could return unnamed packages (“”).  And in fact 
> there are some, a few, classes that aren’t in a package.  Jar tool shouldn’t 
> fail with unnamed packages.

I meant for modular JAR.

> They could even exist in a modular multi-release jar when the module-info 
> class is only in a versioned directory.  

module-info.class should be filtered before calling toPackageName.

> I guess it should fail if a class in an unnamed package is in a module, 
> although I’m not even sure about that.


Mandy

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

[Steve Drach]

>> There is a new webrev at 
>> http://cr.openjdk.java.net/~sdrach/8164805/webrev.01/ 
> 
> sun/tools/jar/Main.java
> 
> Thanks for refactoring and adding the findConcealedPackages method.  What I 
> actually meant was to move out this line:
>concealedPackages = findConcealedPackages(rd);
> 
> to probably before calling addExtendedModuleAttributes(moduleInfos) above 
> line 342 and 1101.

I tried that and decided it was non-optimal because we’d have to construct the 
ModuleDescriptor from the modInfos twice in succession.  Let's compromise here, 
okay?

> 
> 2014 .filter(p -> !p.equals("”))


> 
> For a modular JAR, there should be no unnamed package.  I think the jar tool 
> should fail if it detects an unnamed package.  Your test does not have any 
> unnamed package - how did you find this?

the modularJar/Basic test found a bug.  Then when I was fixing the bug in 
toPackageNames I noticed it could return unnamed packages (“”).  And in fact 
there are some, a few, classes that aren’t in a package.  Jar tool shouldn’t 
fail with unnamed packages.  They could even exist in a modular multi-release 
jar when the module-info class is only in a versioned directory.  I guess it 
should fail if a class in an unnamed package is in a module, although I’m not 
even sure about that.

> 
> ConcealedPackage.java test
> 
> Thanks for improving the test.  It’d be good to name the @Test method with a 
> descriptive method name e.g. 
>   test1 -> testUpdateVersionedPublicClass
>   test2 -> testUpdatedVersionedPublicConcealedClass

It’s difficult to come up with names that aren’t sentences.  I figured the 
comments would explain the test adequately.

> 
> 117 @Test // updates a valid multi-release jar with a new public class in
> 118   // versioned section and fails
> 
> Nit: You can consider moving the comment above @Test.

Ok

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

[Mandy Chung]

> On Oct 24, 2016, at 10:28 AM, Steve Drach  wrote:
> 
> There is a new webrev at 
> http://cr.openjdk.java.net/~sdrach/8164805/webrev.01/ 

sun/tools/jar/Main.java

Thanks for refactoring and adding the findConcealedPackages method.  What I 
actually meant was to move out this line:
concealedPackages = findConcealedPackages(rd);

to probably before calling addExtendedModuleAttributes(moduleInfos) above line 
342 and 1101.

2014 .filter(p -> !p.equals("”))

For a modular JAR, there should be no unnamed package.  I think the jar tool 
should fail if it detects an unnamed package.  Your test does not have any 
unnamed package - how did you find this?

ConcealedPackage.java test

Thanks for improving the test.  It’d be good to name the @Test method with a 
descriptive method name e.g. 
   test1 -> testUpdateVersionedPublicClass
   test2 -> testUpdatedVersionedPublicConcealedClass

 117 @Test // updates a valid multi-release jar with a new public class in
 118   // versioned section and fails

Nit: You can consider moving the comment above @Test.

Mandy

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

[Steve Drach]

There is a new webrev at http://cr.openjdk.java.net/~sdrach/8164805/webrev.01/ 
 that addresses the 
issues raised by Mandy.  Comments inline.

>> 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.

Done, although I still don’t see an advantage to this.

> 
> 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.  

All done.

> 
> 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);

Done.

> 
> 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.

Done.

> 
> 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.

Doen, although I still clean up after tests.

> 
>  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.

Done.

> 
> 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).

Done, but perhaps not exactly the suggested way.  The test has been 
considerably beefed up.

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

I left it the way it was.

> 
> 
>  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.

Done.

Also fixed a bug I found Main::toPackageName

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

[Chris Hegarty]

On 19/10/16 19:34, Steve Drach wrote:

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 for fixing this Steve. The changes look ok to me.

-Chris.

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: 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: 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

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