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

Mandy Chung mandy.chung at oracle.com
Thu Oct 20 00:32:20 UTC 2016


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

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



More information about the core-libs-dev mailing list