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