Review request: jar tool to transform a plain jar file to a modular jar

[Mandy Chung]

  Sherman, Alan,

Thanks for the review.  I did some further cleanup and here is an 
updated webrev:
    http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/modularize-jar.02/

My comments inlined below.

On 09/12/11 6:06, Alan Bateman wrote:
>
> I'm looking at additions to java.util.jar.JarFile and I'm wondering if 
> the class or method descriptions should having wording to explain that 
> a JAR file can have a legacy manifest and also a module-info. I also 
> wonder if we will need to update the JAR specification too. 

Yes, the JAR spec needs to be updated but the JAR spec is in the pubs 
repo.  I prefer to defer to update the docs later before we integrate to 
JDK 8 and track it in the bug database for the time being.

> Minor comment is that the new methods should have @since 1.8.

Fixed.
>
> A passing comment on SimpleLibrary lines 980-988 is that they could be 
> replaced with Files.write(is, md.toPath().resolve("info"));
>

Thanks for the suggestion.  I think you meant Files.copy.

> The sun.tools.jar.ModuleInfo class looks very useful and I'm sure we 
> will have other places that need to generate module-info classes. 
> Would it make sense to put it in org.openjdk.jigsaw? If so then it 
> would be good to have it support requires optional too.

org.openjdk.jigsaw.** is included in the base module while 
sun.tools.jar.ModuleInfo is for tools to use and it depends on 
com.sun.tools.classfile library.  Sorry I initially also thought that 
org.openjdk.jigsaw might be an appropriate package to put this class but 
it's not.

I propose to leave it in sun.tools.jar package for now and move it to an 
appropriate package later.  I have modified this class to support 
require optional and other modifiers.

On 08/31/11 12:37, Xueming Shen wrote:
> Comments on the sun.tools.jar.Main
>
> (1) #197-#201
>      Shouldn't the "equals" be the "ignore case equals"?

module-info.class is a jar entry whose name is case-sensitive.  On the 
other hand, there is a bug in L628 that should use equals instead of 
equalsIgnoreCase method.  I fixed that in the new webrev.

>
> (2) #202-#214
>      The code might be better(?) organized as
>       if (moduled != null) {
>           if (mif != nuill) {
>               ...
>               return false;
>           }
>           ....
>       }
>

Did some additional cleanup too (see the webrev).

> (3) It might be better to move the addModuleRequires() functionality into
>       ModuleInfo implementation? in which it can simply take a 
> Manifest object
>       and take whatever info it might need from the manifest to create 
> the
>       new ModuleInfo object. The sun.tools.jar.Main might not want to 
> know the
>       details of how to transfer the necessary from a Manifest to a 
> ModuleInfo.
>       For example, later you might find you would need an additional 
> attribute
>       from a existing Manifest, ideally you should be able to simply 
> update the
>       ModuleInfo implementation, without touching the 
> sun.tools.jar.Main (the
>       minfo.write(zos) will take care of the writing...)
>

In fact, I'd like to keep sun.tools.jar.ModuleInfo not to couple with 
Manifest.  It's a helper class to generate module-info.class with the 
given module metadata.

> (4) With the migration of new try-resource in getJarPath(), the code 
> might look
>      much better by simply do
>
>      if (man == null)
>          return files;
>      ...
>      if (value == null)
>           return files
>

I tried to retain the existing structure except those which bug me :)
I folded the three if-statements into 1.

>       And we probably don't need check if (rt != null)
>       new JarFile() should never return null, it will through an 
> IOException if it
>       fails.
>

Removed the check.

Thanks
Mandy