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
>
