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

[Jaikiran Pai]

I realized that the verify() method in the testcase can include a couple
of more tests while dealing with the JarInputStream. So I've updated
that method and created a fresh webrev which is available at
https://cr.openjdk.java.net/~jpai/webrev/8211917/3/webrev/

-Jaikiran

On 11/02/20 10:03 pm, Jaikiran Pai wrote:
>
> 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/20200212/1e1f650b/attachment-0001.htm>