RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start
Lance Andersen
lance.andersen at oracle.com
Wed Mar 4 23:00:43 UTC 2020
Hi Christoph,
Thank you for the comments see below.
> On Mar 4, 2020, at 3:30 PM, Langer, Christoph <christoph.langer at 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 at oracle.com <mailto:lance.andersen at oracle.com>>
> Sent: Donnerstag, 27. Februar 2020 22:12
> To: Langer, Christoph <christoph.langer at sap.com <mailto:christoph.langer at sap.com>>
> Cc: nio-dev at openjdk.java.net <mailto:nio-dev at openjdk.java.net>; core-libs-dev at openjdk.java.net <mailto:core-libs-dev at 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 at oracle.com <mailto:lance.andersen at 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 at sap.com <mailto:christoph.langer at 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 at oracle.com <mailto:lance.andersen at oracle.com>>
> Sent: Mittwoch, 19. Februar 2020 02:42
> To: Langer, Christoph <christoph.langer at sap.com <mailto:christoph.langer at sap.com>>
> Cc: Jaikiran Pai <jai.forums2013 at gmail.com <mailto:jai.forums2013 at gmail.com>>; nio-dev at openjdk.java.net <mailto:nio-dev at openjdk.java.net>; core-libs-dev at openjdk.java.net <mailto:core-libs-dev at 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 at sap.com <mailto:christoph.langer at 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 at openjdk.java.net <mailto:nio-dev-bounces at openjdk.java.net>> On Behalf Of Jaikiran
> Pai
> Sent: Montag, 17. Februar 2020 03:56
> To: Lance Andersen <lance.andersen at oracle.com <mailto:lance.andersen at oracle.com>>
> Cc: nio-dev at openjdk.java.net <mailto:nio-dev at openjdk.java.net>; core-libs-dev at openjdk.java.net <mailto:core-libs-dev at 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 at oracle.com <mailto:Lance.Andersen at 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 at oracle.com <mailto:Lance.Andersen at 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 at oracle.com <mailto:Lance.Andersen at 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 at oracle.com <mailto:Lance.Andersen at oracle.com>
More information about the core-libs-dev
mailing list