RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start

[Lance Andersen]

Hi Jaikiran,

Thank you again for tackling this feature request.

Overall, I think this looks OK.

In ZipFileSystem.java, I would reverse the if statement given a MANiFEST.MF present is going to be the exception vs the norm.  Perhaps something similar to:

————————————
if(manifestInode == null || manifestProcessed) {
    inode = inodeIterator.next();
    if(inode == manifestInode)
        continue;
} else {
    inode = manifestInode;
    manifestProcessed = true;
}
—————————————————

A few comments/suggestions on the test:

I would prefer  that the tests use the newer FileSystems:: newFileSystem methods for the patch to the current openjdk repository
Please use Map.of() vs Collections.xxxMap
We should test with STORED and DEFLATED specified for compression.  
I would look at the CopyMoveTest and leverage the Entry class and verify method in this test.  This will keep the tests looking somewhat similar
Please add a test which copies the Manifest from an OS file to the JAR
I would consider adding a test which creates a JAR with a Manifest and then uses Files::copy to create a another JAR from the original JAR
I would consider a test which creates the JAR via  the jar tool(using the ToolProvider API) and then updates the JAR via ZipFS
You may want to consider executing the JAR if you are setting the main class attribute see the LargeEntriesTest as an example

Best,
Lance

> On Feb 1, 2020, at 2:38 AM, Jaikiran Pai <jai.forums2013 at gmail.com> wrote:
> 
> Can I please get a review and a sponsor for a patch for
> https://bugs.openjdk.java.net/browse/JDK-8211917?
> 
> The webrev containing the patch is available at
> https://cr.openjdk.java.net/~jpai/webrev/8211917/1/webrev/
> 
> The commit in the patch updates the jdk.nio.zipfs.ZipFileSystem to write
> out the manifest (if any) as the first entry in the LOC. That then
> allows the java.util.jar.JarInputStream to find the manifest and return
> it through JarInputStream.getManifest().
> 
> In an initial attempt at this patch, I had tried to just reorder the CEN
> to add the manifest entry first instead of forcing the manifest entry
> first in the LOC. But that didn't work and the JarInputStream still
> couldn't find the manifest. So I used this approach to force the
> manifest as the first entry in the LOC (and the existing code
> effectively also ensures that it's also the first entry in the CEN).
> 
> The patch also includes a test case to reproduce this issue and verify
> the change. In the test case I intentionally used the verbose version of
> the FileSystem.newFileSystem() API, instead of the newer simpler ones.
> This is to allow cleaner/easier backporting of this patch to Java
> version 8 if and when that happens. Functionally, which API variant of
> the FileSystem.newFileSystem() API is used shouldn't matter in the
> context of this test case. However, if someone feels that I should
> switch to the newer available API, then please do let me know and I'll
> update the patch.
> 
> -Jaikiran
> 
> 

 <http://oracle.com/us/design/oracle-email-sig-198324.gif>
 <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
 <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>