On Mon, 24 Mar 2025 23:15:06 GMT, Jason Zaugg <jza...@openjdk.org> wrote:
>> I wonder if there's some suspicion of a particular problem in the other >> branch, or other reason to not add the flag there as well. I would expect >> the default should be to add it to both branches, unless there's a fairly >> strong reason not to. >> >> There are some more new jar FileSystems created in `Locations`, but those >> only typically read one file, so probably not that big deal w.r.t. this flag. > >> There are some more new jar FileSystems created in Locations, but those only >> typically read one file, so probably not that big deal w.r.t. this flag. > > Agreed. I initially changed those as well but when I realized they only read > a single file I reverted. > >> I wonder if there's some suspicion of a particular problem in the other >> branch, > > The `then` branch, which I've modified, was introduced in > e652402ed239e171a6f87f5ef88e07d505ad3fe8 (JDK-8149757: Implement > Multi-Release JAR aware JavacFileManager for javac). It became necessary to > ensure exact usage of `ZipFileSystemProvider` to be able to pass in the `env` > map with the `multiRelease` config. > > I reviewed the discussion of that change. There was a mention of the > introduction of this branch, but it didn't go as far to suggest removal of > the `FileSystems.newFileSystem` call, it only justified the addition of the > `jarFsProvider.newFileSystem` call. > > https://mail.openjdk.org/pipermail/compiler-dev/2016-April/thread.html > > >> -why is JavacFileManager.getJarFSProvider() needed? Shouldn't >> FileSystems.newFileSystem(realPath, env); >> be enough? (Calling an SPI directly from an API client seems >> suspicious to me.) > > If I recall correctly, the NIO2 API design forced this one. The method > you are referring to does not exist. There is only one that takes a URI, > and that has very different semantics. So we had to go with the method > on the provider, hence the use of getJarFSProvider. > > > The tests introduced in e652402ed239e171a6f87f5ef88e07d505ad3fe8 don't appear > to exercise the else branch. > > It appears there are two ways to get there: > 1. Direct, programatic use of the a file manager in which the > initialization sequence does not populate `multiReleaseValue` > 2. Use of javac with a `.zip` as a classpath entry (I had to check, but > this does work.) Side question -- what is the expected behaviour with a > classpath entry with a non-lowercase extension, `.JAR`? It is allowed by > `javac` but would not respect the `multi-release` setting. I think that's all true, but I am not sure if it quite explains why not do the same for the same for the else section. There is `FileSystems.newFileSystem(Path path, Map<String,?> env, ClassLoader loader)` since JDK 13, so there shouldn't be a big problem to call that one instead of `FileSystems.newFileSystem(Path path, ClassLoader loader)` (the latter delegates to the former, so there shouldn't be any real change in semantics). I guess one concern could be that, in theory, this method works for other FS providers/types as well. But, presumably, those would ignore env keys they don't know, and would ignore it. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24176#discussion_r2012355003
