Hi Jaikiran, I think the changes to ZipFileSystem are OK.
The test overall is good. I am going to streamline it a bit and remove the long lines (we try to keep lines to around 80 characters). I will push a revised webrev out once I do this over the next few days Best Lance > On Feb 12, 2020, at 1:39 AM, Jaikiran Pai <jai.forums2...@gmail.com> wrote: > > 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/ > <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/ >> <https://cr.openjdk.java.net/~jpai/webrev/8211917/2/webrev/> >> -Jaikiran >> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
