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

[Langer, Christoph]

Hi Lance,

The revised webrev is at 
http://cr.openjdk.java.net/~lancea/8211917/webrev.02/index.html

This looks good to me now. Ship it 

Cheers
Christoph

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

[Lance Andersen]

Hi Christoph

> On Mar 5, 2020, at 4:03 AM, Langer, Christoph  
> wrote:
> 
> Hi Lance,
> 
> Thanks for addressing my points. Here my answer to those things which weren't 
> in full agreement yet:

Please see below
> 
>> I dislike a bit the fact that we introduce a new testng subfolder in
>> test/jdk/nio/zipfs. Wouldn’t it be possible to consolidate in the current 
>> test
>> folder?
>> 
>> I am trying to add some organization separating the non-testng  tests from
>> the  testng tests and other testng tests will be moved here.  I did the same
>> for JDBC a few years back
> 
> ok, maybe it's a good idea to do this here and gradually move all testng 
> tests over.
> 
>> - line 387: Manfiest -> Manifest
> 
> I think you missed that one

Fixed must have not saved it but it is there now
> 
> 
>> - line 417: Parameter "final Map
>> attributes" of ManifestOrderTest::verify should be renamed to
>> "manifestAttributes" to make it easier to understand its purpose
>> 
>> 
>> Prefer to leave as is as it gets wordy and I believe the description is 
>> clear in
>> the comments
> 
> Hm, I needed to look twice to grasp it. So, I'd still prefer something with 
> "manifest" in the variable name. But I leave it to you since it's probably a 
> personal taste thing 

I left the variable name as to your point it sometimes is a personal preference 
;-) 
> 
> However, two more things here:
> 
> The Javadoc of verify says (line 412):
> * @param attributes  Is there a Manifest to check
> 
> You should rather go with the Javadoc of validateManifest here as well:
> * @param attributes A Map containing the attributes expected in the Manifest;
> *   otherwise empty

Updated…  Thought I had done that previously as that @param was originally a 
boolean ….
> 
> Also, I spotted in the Javadoc, line 413:
> * @param entries Entries to validate are in the JAR
> 
> I guess the "are" is wrong here.

Fixed

> 
>> test/jdk/jdk/nio/zipfs/testng/util/ZipFsBaseTest.java:
>> - rename to ZipFSBaseTest (Capital ‘S’)??
>> hmm  I had that originally but did not like it…  but don’t have a strong
>> preference
> 
> Ok, leave it as you have it 

:-)
> 
>> - line 120: public static void verify - > this method is not used by
>> ManifestOrderTest. I think we should only add it when having a test that
>> really uses this verify method. But I generally agree that the verify method 
>> is
>> a candidate for communalization
>> 
>> This will be used by some tests I have created and some I will be moving so
>> rather add it now on the initial push.  This is used by several tests that 
>> will be
>> migrated
>> 
>> - line 176: public void zip: dito, this method doesn’t seem used. So I 
>> suggest
>> to remove it for this change
>> Same comment as above
> 
> OK.
> 
>> The webrev for the above
>> is http://cr.openjdk.java.net/~lancea/8211917/webrev.01/index.html
> 
> Looks good, apart from my comments above.

Thank you again for the review

The revised webrev is at 
http://cr.openjdk.java.net/~lancea/8211917/webrev.02/index.html 


Best
Lance
> 
> Thanks
> Christoph
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

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

[Langer, Christoph]

Hi Lance,

Thanks for addressing my points. Here my answer to those things which weren't 
in full agreement yet:

> I dislike a bit the fact that we introduce a new testng subfolder in
> test/jdk/nio/zipfs. Wouldn’t it be possible to consolidate in the current test
> folder?
> 
> I am trying to add some organization separating the non-testng  tests from
> the  testng tests and other testng tests will be moved here.  I did the same
> for JDBC a few years back

ok, maybe it's a good idea to do this here and gradually move all testng tests 
over.

> - line 387: Manfiest -> Manifest

I think you missed that one


> - line 417: Parameter "final Map
> attributes" of ManifestOrderTest::verify should be renamed to
> "manifestAttributes" to make it easier to understand its purpose
> 
> 
> Prefer to leave as is as it gets wordy and I believe the description is clear 
> in
> the comments

Hm, I needed to look twice to grasp it. So, I'd still prefer something with 
"manifest" in the variable name. But I leave it to you since it's probably a 
personal taste thing 

However, two more things here:

The Javadoc of verify says (line 412):
* @param attributes  Is there a Manifest to check

You should rather go with the Javadoc of validateManifest here as well:
* @param attributes A Map containing the attributes expected in the Manifest;
*   otherwise empty

Also, I spotted in the Javadoc, line 413:
* @param entries Entries to validate are in the JAR

I guess the "are" is wrong here.


> test/jdk/jdk/nio/zipfs/testng/util/ZipFsBaseTest.java:
> - rename to ZipFSBaseTest (Capital ‘S’)??
> hmm  I had that originally but did not like it…  but don’t have a strong
> preference

Ok, leave it as you have it 

> - line 120: public static void verify - > this method is not used by
> ManifestOrderTest. I think we should only add it when having a test that
> really uses this verify method. But I generally agree that the verify method 
> is
> a candidate for communalization
> 
> This will be used by some tests I have created and some I will be moving so
> rather add it now on the initial push.  This is used by several tests that 
> will be
> migrated
> 
> - line 176: public void zip: dito, this method doesn’t seem used. So I suggest
> to remove it for this change
> Same comment as above

OK.

> The webrev for the above
> is http://cr.openjdk.java.net/~lancea/8211917/webrev.01/index.html

Looks good, apart from my comments above.

Thanks
Christoph

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

[Lance Andersen]

Hi Jaikiran

> On Mar 4, 2020, at 9:54 AM, Jaikiran Pai  wrote:
> 
> Hello Lance,
> 
> On 28/02/20 2:41 am, Lance Andersen wrote:
>> Hi Christoph,
>> 
>> I have cleaned up, re-vamped and added more coverage to the test.  As part 
>> of this I started to lay the foundation for removing some of the duplicate 
>> code as part of continued clean up and enhanced coverage going forward
>> 
>> The revised webrev can be found at: 
>> http://cr.openjdk.java.net/~lancea/8211917/webrev.00/index.html 
>> This all 
>> looks good to me (of course, I'm not an official reviewer). The new base 
>> testcase for zipfs related testing is definitely going to help in future 
>> fixes/enhancements.
> 
> I just had one question in there:
> 
> +++ new/test/jdk/jdk/nio/zipfs/testng/util/ZipFsBaseTest.java 
> ...
> +{Map.of("create", "true", "noCompression", "true"),
> +ZipEntry.STORED},
> +{Map.of("create", "true", "noCompression", "false"),
> +ZipEntry.DEFLATED}
> 
> From what I had read in the javadoc of the private method 
> getDefaultCompressionMethod in jdk.nio.zipfs.ZipFileSystem,
> the "noCompression" property is only there for backward compatibility reasons 
> and the new way
> of configuring this semantic is the use of "compressionMethod" property (with 
> value of either STORED
> or DEFLATED). Is the use of "noCompression" in this base test class 
> intentional or is it just a personal preference?
> I'm fine either way, but wanted to know if I should stick to this form in any 
> future test cases.

Yes when I added formal support for some of the existing Zip FS properties, we 
decided to  add compressionMethod as the formal property in the event  
additional compression methods were added in the future.  So noCompression will 
always exist and this DataProvider is used in several tests  so I left it as 
is.  At some point I might change it, but really does not matter as there are 
other tests which specifically test the compressionMethod property.

Best
Lance
> 
> -Jaikiran
> 
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

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

[Jaikiran Pai]

Hello Lance,

On 28/02/20 2:41 am, Lance Andersen wrote:
> Hi Christoph,
>
> I have cleaned up, re-vamped and added more coverage to the test.  As
> part of this I started to lay the foundation for removing some of the
> duplicate code as part of continued clean up and enhanced coverage
> going forward
>
> The revised webrev can be found
> at: http://cr.openjdk.java.net/~lancea/8211917/webrev.00/index.html

This all looks good to me (of course, I'm not an official reviewer). The
new base testcase for zipfs related testing is definitely going to help
in future fixes/enhancements.

I just had one question in there:

+++ new/test/jdk/jdk/nio/zipfs/testng/util/ZipFsBaseTest.java   
...

+{Map.of("create", "true", "noCompression", "true"),
+ZipEntry.STORED},
+{Map.of("create", "true", "noCompression", "false"),
+ZipEntry.DEFLATED}

From what I had read in the javadoc of the private method 
getDefaultCompressionMethod in jdk.nio.zipfs.ZipFileSystem,
the "noCompression" property is only there for backward compatibility reasons 
and the new way
of configuring this semantic is the use of "compressionMethod" property (with 
value of either STORED
or DEFLATED). Is the use of "noCompression" in this base test class intentional 
or is it just a personal preference?
I'm fine either way, but wanted to know if I should stick to this form in any 
future test cases.

-Jaikiran

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

[Jaikiran Pai]

Hello Lance,

On 15/02/20 2:27 am, Lance Andersen wrote:
> 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'll keep that in mind for future changes. Thank you for taking care of
this.

-Jaikiran

Re: 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  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/ 
> 
> -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/ 
>> 
>> -Jaikiran
>> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

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

[Jaikiran Pai]

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/

-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/
>
> -Jaikiran
>

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

[Jaikiran Pai]

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/

-Jaikiran