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

[Lance Andersen]

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



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20200214/8a30296b/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: oracle_sig_logo.gif
Type: image/gif
Size: 658 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20200214/8a30296b/oracle_sig_logo.gif>