Thank you very much for the review Lance. I'll come up with an updated patch containing the suggested changes. -Jaikiran 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; } —————————————————
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 * Please use Map.of() vs Collections.xxxMap * 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 * Please add a test which copies the Manifest from an OS file to the JAR * 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 * I would consider a test which creates the JAR via the jar tool(using the ToolProvider API) and then updates the JAR via ZipFS * You may want to consider executing the JAR if you are setting the main class attribute see the LargeEntriesTest as an example
Best, Lance
On Feb 1, 2020, at 2:38 AM, Jaikiran Pai <jai.forums2013@gmail.com <mailto:jai.forums2013@gmail.com>> wrote:
Can I please get a review and a sponsor for a patch for https://bugs.openjdk.java.net/browse/JDK-8211917?
The webrev containing the patch is available at https://cr.openjdk.java.net/~jpai/webrev/8211917/1/webrev/
The commit in the patch updates the jdk.nio.zipfs.ZipFileSystem to write out the manifest (if any) as the first entry in the LOC. That then allows the java.util.jar.JarInputStream to find the manifest and return it through JarInputStream.getManifest().
In an initial attempt at this patch, I had tried to just reorder the CEN to add the manifest entry first instead of forcing the manifest entry first in the LOC. But that didn't work and the JarInputStream still couldn't find the manifest. So I used this approach to force the manifest as the first entry in the LOC (and the existing code effectively also ensures that it's also the first entry in the CEN).
The patch also includes a test case to reproduce this issue and verify the change. In the test case I intentionally used the verbose version of the FileSystem.newFileSystem() API, instead of the newer simpler ones. This is to allow cleaner/easier backporting of this patch to Java version 8 if and when that happens. Functionally, which API variant of the FileSystem.newFileSystem() API is used shouldn't matter in the context of this test case. However, if someone feels that I should switch to the newer available API, then please do let me know and I'll update the patch.
-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>