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.forums2013@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.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>