On Mon, Mar 2, 2020 at 7:15 AM Volker Simonis <volker.simo...@gmail.com> wrote:
> On Fri, Feb 28, 2020 at 8:02 PM Martin Buchholz <marti...@google.com> > wrote: > > > > Welcome to the troublesome world of zip implementations! > > > > Well, not many remember that the zip format was designed to work > efficiently with floppy discs :) > > > It looks like by creating a new JarEntry you are discarding possible > information from the old one, e.g. you might be discarding whether the > input entry was compressed. If "jar u" does that, I would consider it a > bug! > > > > You're right. My fix was a little to simple, and can be improved: > > http://cr.openjdk.java.net/~simonis/webrevs/2020/8240235.01 > > Preserving the time, comment, extra information, compression method > and size/CRC if the compression method was "stored" is the best we can > do. And this is actually exactly what "jar -" does (see [1]). > > [1] > http://hg.openjdk.java.net/jdk/jdk/file/e04746cec89c/src/jdk.jartool/share/classes/sun/tools/jar/Main.java#l934 > > This fix Looks Good To Me. > > Arguably there is a missing API that would allow you to copy zip entries > from an input to an output completely intact, without having to decompress > and recompress. (might be hard to design!) > > > > Yes, I did think about this as well. It might not be that hard at all. > We could simply add readRawEntry()/wrtiteRawEntry() methods to > ZipInputStream/ZipOutputStream. We'd have to tweak > ZipOutputStream.closeEntry() such that it can handle such "raw" writes > (e.g. it wouldn't be able to check the CRC any more) and various > methods like "skip()" in ZipInputStream. I think this might be > interesting not only because it would allow updating zip-files without > changing untouched entries, but also because it will considerably > improve the speed of updating (i.e. we would save the time for > repeatedly inflating and deflating). I'm not sure though how frequent > the use-case of updating a zip-file really is? > > But that would definitely be a larger enhancement with SE scope. Do > you think it's worth it? > > The platform "should" have something like this, and I've had similar thoughts myself in the past, and my employer would benefit, since there's a lot of zip munging that happens when building large applications. But as always, we have trouble finding the time. > > Probably there should be no such utility method as updateJarFile at > all. Perhaps instead the jar/zip code could be refactored so that tests > can reuse the same code used by "jar u". > > > > I'll leave that as an extra improvement for others :) > > > On Fri, Feb 28, 2020 at 10:50 AM Volker Simonis < > volker.simo...@gmail.com> wrote: > >> > >> Hi, > >> > >> can I please have a review for this small test fix: > >> > >> http://cr.openjdk.java.net/~simonis/webrevs/2020/8240235/ > >> https://bugs.openjdk.java.net/browse/JDK-8240235 > >> > >> JarUtils.updateJar() and JarUtils.updateJarFile() update jar files by > >> reading JarEntry-s from a source jar file and writing these exact > >> JarEntry-s to the destination jar file followed be the inflated and > >> re-deflated data belonging to this entry. > >> > >> This approach is not correct, because JarEntry contains both, the > >> compressed and uncompressed size of the corresponding entry. But the > >> original Defalter which initially compressed that entry can be either > >> different from the current one or it might have used a different > >> compression level compared to the current Deflater which re-deflates > >> the entry data. > >> > >> When the newly created entry is closed, the new compressed size will > >> be checked against the original compressed size if that was recorded > >> in the JarEntry and an exception will be thrown, when they differ: > >> > >> java.util.zip.ZipException: invalid entry compressed size (expected > >> 237 but got 238 bytes) > >> at > java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:267) > >> at > java.base/java.util.zip.ZipOutputStream.putNextEntry(ZipOutputStream.java:192) > >> at > java.base/java.util.jar.JarOutputStream.putNextEntry(JarOutputStream.java:108) > >> at jdk.test.lib.util.JarUtils.updateJar(JarUtils.java:294) > >> at jdk.test.lib.util.JarUtils.updateJar(JarUtils.java:252) > >> at HasUnsignedEntryTest.start(HasUnsignedEntryTest.java:77) > >> at HasUnsignedEntryTest.main(HasUnsignedEntryTest.java:44) > >> at > java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native > >> Method) > >> at > java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) > >> at > java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) > >> at java.base/java.lang.reflect.Method.invoke(Method.java:564) > >> at > com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) > >> at java.base/java.lang.Thread.run(Thread.java:832) > >> Suppressed: java.util.zip.ZipException: invalid entry > >> compressed size (expected 237 but got 238 bytes) > >> at > >> > java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:267) > >> at > >> java.base/java.util.zip.ZipOutputStream.finish(ZipOutputStream.java:360) > >> at > >> > java.base/java.util.zip.DeflaterOutputStream.close(DeflaterOutputStream.java:237) > >> at > >> java.base/java.util.zip.ZipOutputStream.close(ZipOutputStream.java:377) > >> at > jdk.test.lib.util.JarUtils.updateJar(JarUtils.java:280) > >> > >> The fix is trivial. Instead of copying the JarEntry-s verbatim to the > >> output stream, simply write newly created JarEntry-s to the output > >> stream which only contain the entry name. This is also the way how > >> this is handled by the jar tool, when it updates jar files. > >> > >> Thank you and best regards, > >> Volker >
