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 <steve.drach at oracle.com> 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
>