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

Jaikiran Pai jai.forums2013 at gmail.com
Tue Feb 11 16:33:49 UTC 2020

Hello Lance,

On 05/02/20 3:13 am, Lance Andersen wrote:
> 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
Done - updated the testcase to use the newer available APIs.

>   * 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
Done - the testcase now tests the default (== DEFLATED), STORED and
(explicit) DEFLATED compression methods.

>   * Please add a test which copies the Manifest from an OS file to the JAR
Done. New testManifestCopiedFromOSFile test method added.

>   * 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
Done. New testDuplicateJar test method added.

>   * I would consider a test which creates the JAR via  the jar
>     tool(using the ToolProvider API) and then updates the JAR via ZipFS
Done. New testJarToolGeneratedJarWithManifest added.

>   * You may want to consider executing the JAR if you are setting the
>     main class attribute see the LargeEntriesTest as an example
In my initial version, I included the Main-Class manifest attribute only
to make sure the manifest file that is being verified is indeed the one
we had added in the tests. I did not have any intention of testing the
"Main-Class" semantics. In this newer updated version of the test case,
I decided to just remove the "Main-Class" and instead use a dummy
manifest attribute that I just check for equality. I decided to remove
the "Main-Class" attribute since I didn't want this test to do too many
things. Let me know if you prefer that the Main-Class to stay and be
verified that it can be launched.

All these changes are now part of the new webrev which is at


More information about the core-libs-dev mailing list