RFR: 8350880: (zipfs) Add support for read-only zip file systems [v6]

[David Beaumont]

On Mon, 19 May 2025 12:58:00 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> David Beaumont has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fixed test.
>
> test/jdk/jdk/nio/zipfs/NewFileSystemTests.java line 239:
> 
>> 237:             assertTrue(fs.isReadOnly());
>> 238:             if (!"First version".equals(Files.readString(fs.getPath("file.txt"), UTF_8))) {
>> 239:                 fail("unexpected file content");
> 
> Nit, for this and the other new `fail` statements, it might be good to include the unexpected file content in the fail message. If at all the test fails for whatever reason, that additional detail usually help to quickly debug the failures. Or maybe just replace the `if` followed by a `fail` with an `assertEquals()` call.

Urgh, my bad. That's the hangover from these tests being first implemented in the ZipFSTester class without access to sensible assert methods.

> test/jdk/jdk/nio/zipfs/TestPosix.java line 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 2019, 2025, SAP SE. All rights reserved.
> 
> For non-Oracle copyright lines like these, we don't edit them and instead we introduce a newline for the Oracle copyright year. So we should revert the change to this line and introduce the following new line just after this current one:
> 
> * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.

Thanks! Done.

> test/jdk/jdk/nio/zipfs/Utils.java line 48:
> 
>> 46:      *
>> 47:      * @param name the file name of the jar file to create in the working directory.
>> 48:      * @param entries a list of JAR entries to be populated with random bytes.
> 
> Nit - maybe reword this to:
> 
>> @param entries JAR file entry names, whose content will be populated with random bytes.

Done.

> test/jdk/jdk/nio/zipfs/Utils.java line 52:
> 
>> 50:      */
>> 51:     static Path createJarFile(String name, String... entries) throws IOException {
>> 52:         Path jarFile = Paths.get(name);
> 
> In recent times we have replaced `Paths.get(...)` call in the JDK code with `Path.of(...)`. We should do the same here and the other line in this utility class.

Good spot, thanks.
Sadly I'm currently working on other code in which Path.of() is not permitted, so I won't naturally spot these things.

> test/jdk/jdk/nio/zipfs/Utils.java line 78:
> 
>> 76:      *
>> 77:      * @param name the file name of the jar file to create in the working directory.
>> 78:      * @param entries a map of relative file name path strings to file content
> 
> Nit - I think we should replace this description with something like:
> 
>> @param entries a map of JAR file entry names to entry content

Done.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095730823
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095740846
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095740347
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095735890
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095738222