RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start

[Jaikiran Pai]

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