RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start
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
Hi Jaikiran, Thank you for tackling this feature request. I will finish going through the proposed patch and will sponsor once we have completed the overall review.
On Feb 1, 2020, at 2:38 AM, Jaikiran Pai <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).
Zip/JarInputStream rely on the LOC which is why just updating the CEN would not get you across the goal line.
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
Best Lance <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>
Thank you Lance. -Jaikiran On 04/02/20 5:12 am, Lance Andersen wrote:
Hi Jaikiran,
Thank you for tackling this feature request.
I will finish going through the proposed patch and will sponsor once we have completed the overall review.
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).
Zip/JarInputStream rely on the LOC which is why just updating the CEN would not get you across the goal line.
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
Best Lance
<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>
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> 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>
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>
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
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
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>
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
Hi Jaikiran, Lance, I'm a bit late to the game. It occurred to me, whether we could maybe use the streams API to process the inodes. Something like this: diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java @@ -56,6 +56,7 @@ import java.util.jar.Attributes; import java.util.jar.Manifest; import java.util.regex.Pattern; +import java.util.stream.Stream; import java.util.zip.CRC32; import java.util.zip.Deflater; import java.util.zip.DeflaterOutputStream; @@ -1730,7 +1731,17 @@ Entry e; // write loc - for (IndexNode inode : inodes.values()) { + + // write the manifest inode (if any) first in the loc so that the + // java.util.jar.JarInputStream can find it, since it expects it to be + // the first or second entry in the jar + final IndexNode manifestInode = getInode(getBytes("/META-INF/MANIFEST.MF")); + final Stream<IndexNode> inodeStream = manifestInode == null ? + inodes.values().stream() : + Stream.concat(Stream.of(manifestInode), + inodes.values().stream().filter(node -> !manifestInode.equals(node))); + + for (IndexNode inode : (Iterable<IndexNode>)inodeStream::iterator) { if (inode instanceof Entry) { // an updated inode e = (Entry)inode; try { What do you think? I didn't have time yet to have a closer look to the testcase. Will do tomorrow. Cheers Christoph
-----Original Message----- From: nio-dev <nio-dev-bounces@openjdk.java.net> On Behalf Of Jaikiran Pai Sent: Montag, 17. Februar 2020 03:56 To: Lance Andersen <lance.andersen@oracle.com> Cc: nio-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start
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
Hello Christoph, On 17/02/20 9:56 pm, Langer, Christoph wrote:
Hi Jaikiran, Lance,
I'm a bit late to the game. It occurred to me, whether we could maybe use the streams API to process the inodes. Something like this:
diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java @@ -56,6 +56,7 @@ import java.util.jar.Attributes; import java.util.jar.Manifest; import java.util.regex.Pattern; +import java.util.stream.Stream; import java.util.zip.CRC32; import java.util.zip.Deflater; import java.util.zip.DeflaterOutputStream; @@ -1730,7 +1731,17 @@ Entry e;
// write loc - for (IndexNode inode : inodes.values()) { + + // write the manifest inode (if any) first in the loc so that the + // java.util.jar.JarInputStream can find it, since it expects it to be + // the first or second entry in the jar + final IndexNode manifestInode = getInode(getBytes("/META-INF/MANIFEST.MF")); + final Stream<IndexNode> inodeStream = manifestInode == null ? + inodes.values().stream() : + Stream.concat(Stream.of(manifestInode), + inodes.values().stream().filter(node -> !manifestInode.equals(node))); + + for (IndexNode inode : (Iterable<IndexNode>)inodeStream::iterator) {
I don't have any practical experience with Stream and how it performs. Would using Stream here introduce any kind of performance penalty, especially when the number of entries in the jar is high? -Jaikiran
Hi Christoph
On Feb 17, 2020, at 11:26 AM, Langer, Christoph <christoph.langer@sap.com> wrote:
Hi Jaikiran, Lance,
I'm a bit late to the game. It occurred to me, whether we could maybe use the streams API to process the inodes. Something like this:
diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java @@ -56,6 +56,7 @@ import java.util.jar.Attributes; import java.util.jar.Manifest; import java.util.regex.Pattern; +import java.util.stream.Stream; import java.util.zip.CRC32; import java.util.zip.Deflater; import java.util.zip.DeflaterOutputStream; @@ -1730,7 +1731,17 @@ Entry e;
// write loc - for (IndexNode inode : inodes.values()) { + + // write the manifest inode (if any) first in the loc so that the + // java.util.jar.JarInputStream can find it, since it expects it to be + // the first or second entry in the jar + final IndexNode manifestInode = getInode(getBytes("/META-INF/MANIFEST.MF")); + final Stream<IndexNode> inodeStream = manifestInode == null ? + inodes.values().stream() : + Stream.concat(Stream.of(manifestInode), + inodes.values().stream().filter(node -> !manifestInode.equals(node))); + + for (IndexNode inode : (Iterable<IndexNode>)inodeStream::iterator) { if (inode instanceof Entry) { // an updated inode e = (Entry)inode; try {
What do you think?
I am not sure the use of Streams will help us too much here as in your proposed example I believe you would end out traversing the entire list of IndexNodes twice in the case of a Manifest existing. Once when building the Stream and the other when you are walking through the IndexNodes to write them out. Best Lance
I didn't have time yet to have a closer look to the testcase. Will do tomorrow.
Cheers Christoph
-----Original Message----- From: nio-dev <nio-dev-bounces@openjdk.java.net> On Behalf Of Jaikiran Pai Sent: Montag, 17. Februar 2020 03:56 To: Lance Andersen <lance.andersen@oracle.com> Cc: nio-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start
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
<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>
Hi Lance, Jaikiran, you’re probably right. Both, the gut feeling that using streams at all here will bring some performance penalties as well as the potential double traversal of the value map speak against the suggestion I made. I was actually not really convinced myself – however, I was looking for a way to write the coding in a more concise manner (without performance penalties, of course). So, going back to Jaikiran’s latest proposal, I only have a few minor performance tweaks. Please look at this webrev I generated: http://cr.openjdk.java.net/~clanger/webrevs/8211917.0/ First, the manifest node should be looked up by the statement “inodes.get(IndexNode.keyOf(getBytes("/META-INF/MANIFEST.MF")))”. Using “getInode(getBytes("/META-INF/MANIFEST.MF"))” would incur an unnecessary Object.requireNonNull() check and would also call the “entryLookup” function that is needed for mapped entries in MR jars. But even in MR jars, the manifest would not be mapped to some other versioned node. And then within the loop body I restructured the if statements such that in the case of no manifest (probably the most common case), just one “if (manifestInode == null)” check is performed and not another “if (inode == manifestInode)” after the call to inodeIterator.next(). So, just minor tweaks but probably still worthwile doing. 😊 Note that I also added a space after ‘while’ to be consistent with the rest of the file. Other than that I’m good with the change. The testcase looks comprising and zipfs tests are passing. Best regards Christoph From: Lance Andersen <lance.andersen@oracle.com> Sent: Mittwoch, 19. Februar 2020 02:42 To: Langer, Christoph <christoph.langer@sap.com> Cc: Jaikiran Pai <jai.forums2013@gmail.com>; nio-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start Hi Christoph On Feb 17, 2020, at 11:26 AM, Langer, Christoph <christoph.langer@sap.com<mailto:christoph.langer@sap.com>> wrote: Hi Jaikiran, Lance, I'm a bit late to the game. It occurred to me, whether we could maybe use the streams API to process the inodes. Something like this: diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java @@ -56,6 +56,7 @@ import java.util.jar.Attributes; import java.util.jar.Manifest; import java.util.regex.Pattern; +import java.util.stream.Stream; import java.util.zip.CRC32; import java.util.zip.Deflater; import java.util.zip.DeflaterOutputStream; @@ -1730,7 +1731,17 @@ Entry e; // write loc - for (IndexNode inode : inodes.values()) { + + // write the manifest inode (if any) first in the loc so that the + // java.util.jar.JarInputStream can find it, since it expects it to be + // the first or second entry in the jar + final IndexNode manifestInode = getInode(getBytes("/META-INF/MANIFEST.MF")); + final Stream<IndexNode> inodeStream = manifestInode == null ? + inodes.values().stream() : + Stream.concat(Stream.of(manifestInode), + inodes.values().stream().filter(node -> !manifestInode.equals(node))); + + for (IndexNode inode : (Iterable<IndexNode>)inodeStream::iterator) { if (inode instanceof Entry) { // an updated inode e = (Entry)inode; try { What do you think? I am not sure the use of Streams will help us too much here as in your proposed example I believe you would end out traversing the entire list of IndexNodes twice in the case of a Manifest existing. Once when building the Stream and the other when you are walking through the IndexNodes to write them out. Best Lance I didn't have time yet to have a closer look to the testcase. Will do tomorrow. Cheers Christoph -----Original Message----- From: nio-dev <nio-dev-bounces@openjdk.java.net<mailto:nio-dev-bounces@openjdk.java.net>> On Behalf Of Jaikiran Pai Sent: Montag, 17. Februar 2020 03:56 To: Lance Andersen <lance.andersen@oracle.com<mailto:lance.andersen@oracle.com>> Cc: nio-dev@openjdk.java.net<mailto:nio-dev@openjdk.java.net>; core-libs-dev@openjdk.java.net<mailto:core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start 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 [cid:image001.gif@01D5E713.0B83F590]<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>
Hi Christoph I think your changes to ZipFileSystem look fine. I am running it through mach5 now. I am still planning to clean up the test a bit and will create a web rev when I do.
On Feb 19, 2020, at 4:55 AM, Langer, Christoph <christoph.langer@sap.com> wrote:
Hi Lance, Jaikiran,
you’re probably right. Both, the gut feeling that using streams at all here will bring some performance penalties as well as the potential double traversal of the value map speak against the suggestion I made. I was actually not really convinced myself – however, I was looking for a way to write the coding in a more concise manner (without performance penalties, of course). Understand I did not come up with anything but if we do we can always update it later.
So, going back to Jaikiran’s latest proposal, I only have a few minor performance tweaks. Please look at this webrev I generated:http://cr.openjdk.java.net/~clanger/webrevs/8211917.0/ <http://cr.openjdk.java.net/~clanger/webrevs/8211917.0/>
First, the manifest node should be looked up by the statement “inodes.get(IndexNode.keyOf(getBytes("/META-INF/MANIFEST.MF")))”. Using “getInode(getBytes("/META-INF/MANIFEST.MF"))” would incur an unnecessary Object.requireNonNull() check and would also call the “entryLookup” function that is needed for mapped entries in MR jars. But even in MR jars, the manifest would not be mapped to some other versioned node.
And then within the loop body I restructured the if statements such that in the case of no manifest (probably the most common case), just one “if (manifestInode == null)” check is performed and not another “if (inode == manifestInode)” after the call to inodeIterator.next().
So, just minor tweaks but probably still worthwile doing. 😊
All good
Note that I also added a space after ‘while’ to be consistent with the rest of the file.
Other than that I’m good with the change. The testcase looks comprising and zipfs tests are passing.
Best Lance
Best regards Christoph
From: Lance Andersen <lance.andersen@oracle.com <mailto:lance.andersen@oracle.com>> Sent: Mittwoch, 19. Februar 2020 02:42 To: Langer, Christoph <christoph.langer@sap.com <mailto:christoph.langer@sap.com>> Cc: Jaikiran Pai <jai.forums2013@gmail.com <mailto:jai.forums2013@gmail.com>>; nio-dev@openjdk.java.net <mailto:nio-dev@openjdk.java.net>; core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start
Hi Christoph
On Feb 17, 2020, at 11:26 AM, Langer, Christoph <christoph.langer@sap.com <mailto:christoph.langer@sap.com>> wrote:
Hi Jaikiran, Lance,
I'm a bit late to the game. It occurred to me, whether we could maybe use the streams API to process the inodes. Something like this:
diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java @@ -56,6 +56,7 @@ import java.util.jar.Attributes; import java.util.jar.Manifest; import java.util.regex.Pattern; +import java.util.stream.Stream; import java.util.zip.CRC32; import java.util.zip.Deflater; import java.util.zip.DeflaterOutputStream; @@ -1730,7 +1731,17 @@ Entry e;
// write loc - for (IndexNode inode : inodes.values()) { + + // write the manifest inode (if any) first in the loc so that the + // java.util.jar.JarInputStream can find it, since it expects it to be + // the first or second entry in the jar + final IndexNode manifestInode = getInode(getBytes("/META-INF/MANIFEST.MF")); + final Stream<IndexNode> inodeStream = manifestInode == null ? + inodes.values().stream() : + Stream.concat(Stream.of(manifestInode), + inodes.values().stream().filter(node -> !manifestInode.equals(node))); + + for (IndexNode inode : (Iterable<IndexNode>)inodeStream::iterator) { if (inode instanceof Entry) { // an updated inode e = (Entry)inode; try {
What do you think?
I am not sure the use of Streams will help us too much here as in your proposed example I believe you would end out traversing the entire list of IndexNodes twice in the case of a Manifest existing. Once when building the Stream and the other when you are walking through the IndexNodes to write them out.
Best Lance
I didn't have time yet to have a closer look to the testcase. Will do tomorrow.
Cheers Christoph
-----Original Message----- From: nio-dev <nio-dev-bounces@openjdk.java.net <mailto:nio-dev-bounces@openjdk.java.net>> On Behalf Of Jaikiran Pai Sent: Montag, 17. Februar 2020 03:56 To: Lance Andersen <lance.andersen@oracle.com <mailto:lance.andersen@oracle.com>> Cc: nio-dev@openjdk.java.net <mailto:nio-dev@openjdk.java.net>; core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start
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
<image001.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>
<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>
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 <http://cr.openjdk.java.net/~lancea/8211917/webrev.00/index.html> Best Lance
On Feb 19, 2020, at 3:06 PM, Lance Andersen <lance.andersen@oracle.com <mailto:lance.andersen@oracle.com>> wrote:
Hi Christoph
I think your changes to ZipFileSystem look fine. I am running it through mach5 now. I am still planning to clean up the test a bit and will create a web rev when I do.
On Feb 19, 2020, at 4:55 AM, Langer, Christoph <christoph.langer@sap.com <mailto:christoph.langer@sap.com>> wrote:
Hi Lance, Jaikiran,
you’re probably right. Both, the gut feeling that using streams at all here will bring some performance penalties as well as the potential double traversal of the value map speak against the suggestion I made. I was actually not really convinced myself – however, I was looking for a way to write the coding in a more concise manner (without performance penalties, of course). Understand I did not come up with anything but if we do we can always update it later.
So, going back to Jaikiran’s latest proposal, I only have a few minor performance tweaks. Please look at this webrev I generated:http://cr.openjdk.java.net/~clanger/webrevs/8211917.0/ <http://cr.openjdk.java.net/~clanger/webrevs/8211917.0/>
First, the manifest node should be looked up by the statement “inodes.get(IndexNode.keyOf(getBytes("/META-INF/MANIFEST.MF")))”. Using “getInode(getBytes("/META-INF/MANIFEST.MF"))” would incur an unnecessary Object.requireNonNull() check and would also call the “entryLookup” function that is needed for mapped entries in MR jars. But even in MR jars, the manifest would not be mapped to some other versioned node.
And then within the loop body I restructured the if statements such that in the case of no manifest (probably the most common case), just one “if (manifestInode == null)” check is performed and not another “if (inode == manifestInode)” after the call to inodeIterator.next().
So, just minor tweaks but probably still worthwile doing. 😊
All good
Note that I also added a space after ‘while’ to be consistent with the rest of the file.
Other than that I’m good with the change. The testcase looks comprising and zipfs tests are passing.
Best Lance
Best regards Christoph
From: Lance Andersen <lance.andersen@oracle.com <mailto:lance.andersen@oracle.com>> Sent: Mittwoch, 19. Februar 2020 02:42 To: Langer, Christoph <christoph.langer@sap.com <mailto:christoph.langer@sap.com>> Cc: Jaikiran Pai <jai.forums2013@gmail.com <mailto:jai.forums2013@gmail.com>>; nio-dev@openjdk.java.net <mailto:nio-dev@openjdk.java.net>; core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start
Hi Christoph
On Feb 17, 2020, at 11:26 AM, Langer, Christoph <christoph.langer@sap.com <mailto:christoph.langer@sap.com>> wrote:
Hi Jaikiran, Lance,
I'm a bit late to the game. It occurred to me, whether we could maybe use the streams API to process the inodes. Something like this:
diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java @@ -56,6 +56,7 @@ import java.util.jar.Attributes; import java.util.jar.Manifest; import java.util.regex.Pattern; +import java.util.stream.Stream; import java.util.zip.CRC32; import java.util.zip.Deflater; import java.util.zip.DeflaterOutputStream; @@ -1730,7 +1731,17 @@ Entry e;
// write loc - for (IndexNode inode : inodes.values()) { + + // write the manifest inode (if any) first in the loc so that the + // java.util.jar.JarInputStream can find it, since it expects it to be + // the first or second entry in the jar + final IndexNode manifestInode = getInode(getBytes("/META-INF/MANIFEST.MF")); + final Stream<IndexNode> inodeStream = manifestInode == null ? + inodes.values().stream() : + Stream.concat(Stream.of(manifestInode), + inodes.values().stream().filter(node -> !manifestInode.equals(node))); + + for (IndexNode inode : (Iterable<IndexNode>)inodeStream::iterator) { if (inode instanceof Entry) { // an updated inode e = (Entry)inode; try {
What do you think?
I am not sure the use of Streams will help us too much here as in your proposed example I believe you would end out traversing the entire list of IndexNodes twice in the case of a Manifest existing. Once when building the Stream and the other when you are walking through the IndexNodes to write them out.
Best Lance
I didn't have time yet to have a closer look to the testcase. Will do tomorrow.
Cheers Christoph
-----Original Message----- From: nio-dev <nio-dev-bounces@openjdk.java.net <mailto:nio-dev-bounces@openjdk.java.net>> On Behalf Of Jaikiran Pai Sent: Montag, 17. Februar 2020 03:56 To: Lance Andersen <lance.andersen@oracle.com <mailto:lance.andersen@oracle.com>> Cc: nio-dev@openjdk.java.net <mailto:nio-dev@openjdk.java.net>; core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start
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
<image001.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>
<oracle_sig_logo.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> <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> <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>
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
Hi Jaikiran
On Mar 4, 2020, at 9:54 AM, Jaikiran Pai <jai.forums2013@gmail.com> 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 <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
<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>
Hi Lance, I eventually found the time to have a look at your latest webrev. So, overall this looks good. It’s a good start towards cleaning up the tests and use common utility methods. Thanks for doing this. 😊 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? Other than that, I only have a few nits: test/jdk/jdk/nio/zipfs/testng/test/ManifestOrderTest.java: - package declaration should be moved after the copyright header - line 245: copyed -> copied - line 335: Manfifest -> Manifest - line 369: when its has -> when it has - line 387: Manfiest -> Manifest- line 417: Parameter "final Map<?, ?> attributes" of ManifestOrderTest::verify should be renamed to "manifestAttributes" to make it easier to understand its purpose test/jdk/jdk/nio/zipfs/testng/util/ZipFsBaseTest.java: - rename to ZipFSBaseTest (Capital ‘S’)?? - package declaration should be moved after the copyright header - always use "protected" for methods/members where currently “public” is used - line 92: use private instead of public static String formatMap, since the method isn’t used outside of ZipFsBaseTest - 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 - line 176: public void zip: dito, this method doesn’t seem used. So I suggest to remove it for this change Thanks and Best regards Christoph From: Lance Andersen <lance.andersen@oracle.com> Sent: Donnerstag, 27. Februar 2020 22:12 To: Langer, Christoph <christoph.langer@sap.com> Cc: nio-dev@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start 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 Best Lance On Feb 19, 2020, at 3:06 PM, Lance Andersen <lance.andersen@oracle.com<mailto:lance.andersen@oracle.com>> wrote: Hi Christoph I think your changes to ZipFileSystem look fine. I am running it through mach5 now. I am still planning to clean up the test a bit and will create a web rev when I do. On Feb 19, 2020, at 4:55 AM, Langer, Christoph <christoph.langer@sap.com<mailto:christoph.langer@sap.com>> wrote: Hi Lance, Jaikiran, you’re probably right. Both, the gut feeling that using streams at all here will bring some performance penalties as well as the potential double traversal of the value map speak against the suggestion I made. I was actually not really convinced myself – however, I was looking for a way to write the coding in a more concise manner (without performance penalties, of course). Understand I did not come up with anything but if we do we can always update it later. So, going back to Jaikiran’s latest proposal, I only have a few minor performance tweaks. Please look at this webrev I generated:http://cr.openjdk.java.net/~clanger/webrevs/8211917.0/ First, the manifest node should be looked up by the statement “inodes.get(IndexNode.keyOf(getBytes("/META-INF/MANIFEST.MF")))”. Using “getInode(getBytes("/META-INF/MANIFEST.MF"))” would incur an unnecessary Object.requireNonNull() check and would also call the “entryLookup” function that is needed for mapped entries in MR jars. But even in MR jars, the manifest would not be mapped to some other versioned node. And then within the loop body I restructured the if statements such that in the case of no manifest (probably the most common case), just one “if (manifestInode == null)” check is performed and not another “if (inode == manifestInode)” after the call to inodeIterator.next(). So, just minor tweaks but probably still worthwile doing. 😊 All good Note that I also added a space after ‘while’ to be consistent with the rest of the file. Other than that I’m good with the change. The testcase looks comprising and zipfs tests are passing. Best Lance Best regards Christoph From: Lance Andersen <lance.andersen@oracle.com<mailto:lance.andersen@oracle.com>> Sent: Mittwoch, 19. Februar 2020 02:42 To: Langer, Christoph <christoph.langer@sap.com<mailto:christoph.langer@sap.com>> Cc: Jaikiran Pai <jai.forums2013@gmail.com<mailto:jai.forums2013@gmail.com>>; nio-dev@openjdk.java.net<mailto:nio-dev@openjdk.java.net>; core-libs-dev@openjdk.java.net<mailto:core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start Hi Christoph On Feb 17, 2020, at 11:26 AM, Langer, Christoph <christoph.langer@sap.com<mailto:christoph.langer@sap.com>> wrote: Hi Jaikiran, Lance, I'm a bit late to the game. It occurred to me, whether we could maybe use the streams API to process the inodes. Something like this: diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java @@ -56,6 +56,7 @@ import java.util.jar.Attributes; import java.util.jar.Manifest; import java.util.regex.Pattern; +import java.util.stream.Stream; import java.util.zip.CRC32; import java.util.zip.Deflater; import java.util.zip.DeflaterOutputStream; @@ -1730,7 +1731,17 @@ Entry e; // write loc - for (IndexNode inode : inodes.values()) { + + // write the manifest inode (if any) first in the loc so that the + // java.util.jar.JarInputStream can find it, since it expects it to be + // the first or second entry in the jar + final IndexNode manifestInode = getInode(getBytes("/META-INF/MANIFEST.MF")); + final Stream<IndexNode> inodeStream = manifestInode == null ? + inodes.values().stream() : + Stream.concat(Stream.of(manifestInode), + inodes.values().stream().filter(node -> !manifestInode.equals(node))); + + for (IndexNode inode : (Iterable<IndexNode>)inodeStream::iterator) { if (inode instanceof Entry) { // an updated inode e = (Entry)inode; try { What do you think? I am not sure the use of Streams will help us too much here as in your proposed example I believe you would end out traversing the entire list of IndexNodes twice in the case of a Manifest existing. Once when building the Stream and the other when you are walking through the IndexNodes to write them out. Best Lance I didn't have time yet to have a closer look to the testcase. Will do tomorrow. Cheers Christoph -----Original Message----- From: nio-dev <nio-dev-bounces@openjdk.java.net<mailto:nio-dev-bounces@openjdk.java.net>> On Behalf Of Jaikiran Pai Sent: Montag, 17. Februar 2020 03:56 To: Lance Andersen <lance.andersen@oracle.com<mailto:lance.andersen@oracle.com>> Cc: nio-dev@openjdk.java.net<mailto:nio-dev@openjdk.java.net>; core-libs-dev@openjdk.java.net<mailto:core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start 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 <image001.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> <oracle_sig_logo.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> [cid:image001.gif@01D5F26C.11092760]<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>
Hi Christoph, Thank you for the comments see below.
On Mar 4, 2020, at 3:30 PM, Langer, Christoph <christoph.langer@sap.com> wrote:
Hi Lance,
I eventually found the time to have a look at your latest webrev.
So, overall this looks good. It’s a good start towards cleaning up the tests and use common utility methods. Thanks for doing this. 😊
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
Other than that, I only have a few nits:
test/jdk/jdk/nio/zipfs/testng/test/ManifestOrderTest.java: - package declaration should be moved after the copyright header moved - line 245: copyed -> copied Not sure how I missed this but fixed - line 335: Manfifest -> Manifest done - line 369: when its has -> when it has done - line 387: Manfiest -> Manifest- 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
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 - package declaration should be moved after the copyright header moved - always use "protected" for methods/members where currently “public” is used I updated the methods to be protected - line 92: use private instead of public static String formatMap, since the method isn’t used outside of ZipFsBaseTest Fair point. I had some other tests that used this outside of the methods that are in this class but I will rework them - 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
The webrev for the above is http://cr.openjdk.java.net/~lancea/8211917/webrev.01/index.html Mach5 is re-running now Best Lance
Thanks and Best regards Christoph
From: Lance Andersen <lance.andersen@oracle.com <mailto:lance.andersen@oracle.com>> Sent: Donnerstag, 27. Februar 2020 22:12 To: Langer, Christoph <christoph.langer@sap.com <mailto:christoph.langer@sap.com>> Cc: nio-dev@openjdk.java.net <mailto:nio-dev@openjdk.java.net>; core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start
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 <http://cr.openjdk.java.net/~lancea/8211917/webrev.00/index.html>
Best Lance
On Feb 19, 2020, at 3:06 PM, Lance Andersen <lance.andersen@oracle.com <mailto:lance.andersen@oracle.com>> wrote:
Hi Christoph
I think your changes to ZipFileSystem look fine. I am running it through mach5 now. I am still planning to clean up the test a bit and will create a web rev when I do.
On Feb 19, 2020, at 4:55 AM, Langer, Christoph <christoph.langer@sap.com <mailto:christoph.langer@sap.com>> wrote:
Hi Lance, Jaikiran,
you’re probably right. Both, the gut feeling that using streams at all here will bring some performance penalties as well as the potential double traversal of the value map speak against the suggestion I made. I was actually not really convinced myself – however, I was looking for a way to write the coding in a more concise manner (without performance penalties, of course). Understand I did not come up with anything but if we do we can always update it later.
So, going back to Jaikiran’s latest proposal, I only have a few minor performance tweaks. Please look at this webrev I generated:http://cr.openjdk.java.net/~clanger/webrevs/8211917.0/ <http://cr.openjdk.java.net/~clanger/webrevs/8211917.0/>
First, the manifest node should be looked up by the statement “inodes.get(IndexNode.keyOf(getBytes("/META-INF/MANIFEST.MF")))”. Using “getInode(getBytes("/META-INF/MANIFEST.MF"))” would incur an unnecessary Object.requireNonNull() check and would also call the “entryLookup” function that is needed for mapped entries in MR jars. But even in MR jars, the manifest would not be mapped to some other versioned node.
And then within the loop body I restructured the if statements such that in the case of no manifest (probably the most common case), just one “if (manifestInode == null)” check is performed and not another “if (inode == manifestInode)” after the call to inodeIterator.next().
So, just minor tweaks but probably still worthwile doing. 😊
All good
Note that I also added a space after ‘while’ to be consistent with the rest of the file.
Other than that I’m good with the change. The testcase looks comprising and zipfs tests are passing.
Best Lance
Best regards Christoph
From: Lance Andersen <lance.andersen@oracle.com <mailto:lance.andersen@oracle.com>> Sent: Mittwoch, 19. Februar 2020 02:42 To: Langer, Christoph <christoph.langer@sap.com <mailto:christoph.langer@sap.com>> Cc: Jaikiran Pai <jai.forums2013@gmail.com <mailto:jai.forums2013@gmail.com>>; nio-dev@openjdk.java.net <mailto:nio-dev@openjdk.java.net>; core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start
Hi Christoph
On Feb 17, 2020, at 11:26 AM, Langer, Christoph <christoph.langer@sap.com <mailto:christoph.langer@sap.com>> wrote:
Hi Jaikiran, Lance,
I'm a bit late to the game. It occurred to me, whether we could maybe use the streams API to process the inodes. Something like this:
diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java @@ -56,6 +56,7 @@ import java.util.jar.Attributes; import java.util.jar.Manifest; import java.util.regex.Pattern; +import java.util.stream.Stream; import java.util.zip.CRC32; import java.util.zip.Deflater; import java.util.zip.DeflaterOutputStream; @@ -1730,7 +1731,17 @@ Entry e;
// write loc - for (IndexNode inode : inodes.values()) { + + // write the manifest inode (if any) first in the loc so that the + // java.util.jar.JarInputStream can find it, since it expects it to be + // the first or second entry in the jar + final IndexNode manifestInode = getInode(getBytes("/META-INF/MANIFEST.MF")); + final Stream<IndexNode> inodeStream = manifestInode == null ? + inodes.values().stream() : + Stream.concat(Stream.of(manifestInode), + inodes.values().stream().filter(node -> !manifestInode.equals(node))); + + for (IndexNode inode : (Iterable<IndexNode>)inodeStream::iterator) { if (inode instanceof Entry) { // an updated inode e = (Entry)inode; try {
What do you think?
I am not sure the use of Streams will help us too much here as in your proposed example I believe you would end out traversing the entire list of IndexNodes twice in the case of a Manifest existing. Once when building the Stream and the other when you are walking through the IndexNodes to write them out.
Best Lance
I didn't have time yet to have a closer look to the testcase. Will do tomorrow.
Cheers Christoph
-----Original Message----- From: nio-dev <nio-dev-bounces@openjdk.java.net <mailto:nio-dev-bounces@openjdk.java.net>> On Behalf Of Jaikiran Pai Sent: Montag, 17. Februar 2020 03:56 To: Lance Andersen <lance.andersen@oracle.com <mailto:lance.andersen@oracle.com>> Cc: nio-dev@openjdk.java.net <mailto:nio-dev@openjdk.java.net>; core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start
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
<image001.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>
<oracle_sig_logo.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>
<image001.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>
<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>
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
Hi Christoph
On Mar 5, 2020, at 4:03 AM, Langer, Christoph <christoph.langer@sap.com> 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 <http://cr.openjdk.java.net/~lancea/8211917/webrev.02/index.html> Best Lance
Thanks Christoph
<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>
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
participants (3)
-
Jaikiran Pai
-
Lance Andersen
-
Langer, Christoph