RFR: JDK-8232879: (zipfs) Writing out data with ZipFileSystem leads to a CRC failure in the generated jar file
Jaikiran Pai
jai.forums2013 at gmail.com
Thu Oct 24 02:46:10 UTC 2019
Hello Christoph,
Thank you for the very detailed review. Comments inline.
On 24/10/19 3:00 AM, Langer, Christoph wrote:
>
> For the path to the test file, you could simply do: final Path jarPath = Paths.get("zipfs-crc-test.jar");
> The test is run in the scratch directory of jtreg, so no reason to go to java.io.tmpdir.
Thank you. I wasn't aware of jtreg scratch dir. Does it get used for
every test or is there something specific to the location/type of this
test? The updated webrev now uses the above construct.
> You can also skip deletion of this test file in the finally block, as the jtreg scratch directories will be wiped by jtreg eventually.
>
> For the existence check of the test file in line 62, you can simply use Files.exists.
Indeed. Updated in new webrev.
> As for creating the zipfs Filesystem (line 68), you can simply use FileSystems.newFileSystem(Path, Map), no need to mess around with URIs.
The FileSystems.newFileSystem(Path, Map) doesn't exist in Java 11[1]. Of
course, this specific test will be run against latest upstream, where
this new method exists. I actually copied over that usage from my Java
11 reproducer. But given that this issue isn't about creating the
FileSystem itself, I have taken your advice and changed this line in the
new webrev.
>
> Line 96, where construct the input stream and then in 97, the ZipInputStream, I suggest you either put both into the try statement or you use ZipInputStream zis = new ZipInputStream(Files.newInputStream(jarPath)) and then rely on ZipInputStream cascading the close.
Done - updated in new webrev.
>
> And my last suggestion: you could statically import Assert.assertEquals to shorten lines 105 and 106.
Done - updated in new webrev.
The updated webrev is here
https://cr.openjdk.java.net/~jpai/webrev/8232879/2/webrev/
[1]
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/nio/file/FileSystems.html
-Jaikiran
More information about the core-libs-dev
mailing list