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