Read only ZIP file system. #24506

david-beaumont · 2025-04-08T12:10:46Z

Draft for pre-review comments.

This is NOT intended to be a single PR that's submitted (I'm including at least 2 things together here so things like internal helper function API changes can be seen in context).

Please do not review this unless you've reached out to me first for proper context.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24506/head:pull/24506
$ git checkout pull/24506

Update a local copy of the PR:
$ git checkout pull/24506
$ git pull https://git.openjdk.org/jdk.git pull/24506/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24506

View PR using the GUI difftool:
$ git pr show -t 24506

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24506.diff

bridgekeeper · 2025-04-08T12:11:45Z

👋 Welcome back david-beaumont! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-04-08T12:12:10Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-04-08T12:12:51Z

@david-beaumont The following labels will be automatically applied to this pull request:

core-libs
nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

david-beaumont

Pre-loading to give context for some of the changes.

david-beaumont · 2025-04-08T12:25:37Z

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java

@@ -180,7 +251,26 @@ class ZipFileSystem extends FileSystem {
        this.provider = provider;
        this.zfpath = zfpath;

-        initializeReleaseVersion(env);
+        // Determining a release version uses 'this' instance to read paths etc.


Moving all the mutable instance state to the end of the constructor, so we can more easily reason about what state its in when the entry lookup remapping is built.

david-beaumont · 2025-04-08T12:30:30Z

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java

            }
-            createVersionedLinks(version < 0 ? 0 : version);


This is now done in the caller (constructor) because it was a hidden side-effecting method.

david-beaumont · 2025-04-08T12:31:30Z

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java

@@ -1435,11 +1531,11 @@ private boolean isMultiReleaseJar() throws IOException {
     * Then wrap the map in a function that getEntry can use to override root
     * entry lookup for entries that have corresponding versioned entries.
     */
-    private void createVersionedLinks(int version) {


Changed to return the map to the constructor. This could also be made to return Optional<...> to maybe simplify caller.

david-beaumont · 2025-04-09T12:52:55Z

src/jdk.compiler/share/classes/com/sun/tools/javac/file/FSInfo.java

@@ -163,4 +161,36 @@ public synchronized FileSystemProvider getJarFSProvider() {
        return null;
    }

+    // Should match the properties/values in ZipFileSystem.java.


I added a helper here (it seemed like the right place). Happy to move or inline stuff.

david-beaumont · 2025-04-09T12:54:57Z

src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java

                } catch (ZipException ze) {
                    throw new IOException("ZipException opening \"" + archivePath.getFileName() + "\": " + ze.getMessage(), ze);
                }
            } else {
-                this.fileSystem = FileSystems.newFileSystem(archivePath, (ClassLoader)null);


Existing code is passing null, but that constructor path is identical to the ones not passing a classloader at all.
I'm not sure if the old code was calling the version for additional clarity-of-intent, but the new code has the same effect wrt class loaders.

david-beaumont · 2025-04-09T12:56:57Z

src/jdk.compiler/share/classes/com/sun/tools/javac/main/JavaCompiler.java

+                     | UncheckedIOException
+                     | FileSystemNotFoundException
+                     | InvalidPathException
+                     | ReadOnlyFileSystemException ex) {


This change is just about adding slightly more useful information to the (common) case when no exception message exists. It helped me when debugging.

david-beaumont · 2025-04-09T13:27:03Z

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java

    private final Set<PosixFilePermission> defaultPermissions;

    private final Set<String> supportedFileAttributeViews;

+    // If it's decided to try and make access mode common to other file systems,


I'd like to think it might be possible to make a common access mode for any file system, but it's probably a long way off. Right now I'm reticent to make this enum public (because if it were a common property later we'd need to move the type or support both). Happy to rethink depending on what others think though.

david-beaumont · 2025-04-09T13:28:12Z

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java

+        static AccessMode from(Object value) {
+            switch (value) {
+                case null -> {
+                    return null;


By using null rather than a "default" instance it's more in tune with what we'd expose in terms of a user facing enum, and I wanted to just align what's here with that (even if we aren't doing it yet).

LanceAndersen

Dave,

Thank you for taking on this enhancement to zipfs.

It would be best to:

Have separate PRs for the javac and zipfs changes
Keep the zipfs changes to what is required to support "read-only" file systems
-- Additional cleanup can be done under a separate PR in order to keep the changes more narrowly focused
As part of this change, there should be additional test(s) added to the zipfs area which test the proposed behavior as well as what happens when conflicting values are provided

david-beaumont · 2025-04-09T15:07:22Z

This a draft review not intended to be reviewed yet. I'm just having the discussion around the changes privately for now. Please don't bother looking at it until it's out of draft status. I do hope you weren't assigned as a reviewer automatically on a draft PR ?!

* Reverting and simplifying some things after discussion. * Fix bad test. * Repatching javac changes. * Minor fix for env. * Making Javac only ever create read-only ZIP/JAR filesystems.

* Rewording module info.

…tually exclusive.

openjdk bot added nio [email protected] core-libs [email protected] labels Apr 8, 2025

david-beaumont commented Apr 9, 2025

View reviewed changes

LanceAndersen reviewed Apr 9, 2025

View reviewed changes

david-beaumont force-pushed the jdk_8350880 branch from 3e44b27 to 0935cb8 Compare April 15, 2025 14:23

david-beaumont added 5 commits April 16, 2025 13:29

Read only ZIP file system.

8c2bf36

Include javac changes for read-only ZIP files.

dec46fb

* Reverting and simplifying some things after discussion. * Fix bad test. * Repatching javac changes. * Minor fix for env. * Making Javac only ever create read-only ZIP/JAR filesystems.

Rewording module info.

4302846

* Rewording module info.

More simplification.

ce10ed5

Tweaking spec and code to clarify how create and readOnly mode are mu…

e8aff8f

…tually exclusive.

david-beaumont force-pushed the jdk_8350880 branch from 0935cb8 to e8aff8f Compare April 16, 2025 22:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read only ZIP file system. #24506

Read only ZIP file system. #24506

david-beaumont commented Apr 8, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 8, 2025

openjdk bot commented Apr 8, 2025

openjdk bot commented Apr 8, 2025

david-beaumont left a comment

david-beaumont Apr 8, 2025

david-beaumont Apr 8, 2025

david-beaumont Apr 8, 2025

david-beaumont Apr 9, 2025

david-beaumont Apr 9, 2025

david-beaumont Apr 9, 2025

david-beaumont Apr 9, 2025

david-beaumont Apr 9, 2025

LanceAndersen left a comment

david-beaumont commented Apr 9, 2025

Read only ZIP file system. #24506

Are you sure you want to change the base?

Read only ZIP file system. #24506

Conversation

david-beaumont commented Apr 8, 2025 • edited by openjdk bot Loading

Progress

Reviewing

bridgekeeper bot commented Apr 8, 2025

openjdk bot commented Apr 8, 2025

openjdk bot commented Apr 8, 2025

david-beaumont left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LanceAndersen left a comment

Choose a reason for hiding this comment

david-beaumont commented Apr 9, 2025

david-beaumont commented Apr 8, 2025 •

edited by openjdk bot

Loading