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