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

Steve Drach steve.drach at oracle.com
Mon Oct 24 20:39:58 UTC 2016

>> There is a new webrev at http://cr.openjdk.java.net/~sdrach/8164805/webrev.01/ 
> sun/tools/jar/Main.java
> Thanks for refactoring and adding the findConcealedPackages method.  What I actually meant was to move out this line:
>    concealedPackages = findConcealedPackages(rd);
> to probably before calling addExtendedModuleAttributes(moduleInfos) above line 342 and 1101.

I tried that and decided it was non-optimal because we’d have to construct the ModuleDescriptor from the modInfos twice in succession.  Let's compromise here, okay?

> 2014                 .filter(p -> !p.equals("”))

> For a modular JAR, there should be no unnamed package.  I think the jar tool should fail if it detects an unnamed package.  Your test does not have any unnamed package - how did you find this?

the modularJar/Basic test found a bug.  Then when I was fixing the bug in toPackageNames I noticed it could return unnamed packages (“”).  And in fact there are some, a few, classes that aren’t in a package.  Jar tool shouldn’t fail with unnamed packages.  They could even exist in a modular multi-release jar when the module-info class is only in a versioned directory.  I guess it should fail if a class in an unnamed package is in a module, although I’m not even sure about that.

> ConcealedPackage.java test
> Thanks for improving the test.  It’d be good to name the @Test method with a descriptive method name e.g. 
>   test1 -> testUpdateVersionedPublicClass
>   test2 -> testUpdatedVersionedPublicConcealedClass

It’s difficult to come up with names that aren’t sentences.  I figured the comments would explain the test adequately.

> 117     @Test // updates a valid multi-release jar with a new public class in
> 118           // versioned section and fails
> Nit: You can consider moving the comment above @Test.


More information about the core-libs-dev mailing list