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

[Jaikiran Pai]

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;
> }
> —————————————————
>
Done.


> 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
>
Done.


>   * 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
https://cr.openjdk.java.net/~jpai/webrev/8211917/2/webrev/

-Jaikiran

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20200211/0d0f7aea/attachment.htm>