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