On Thu, 23 Mar 2023 12:05:50 GMT, Eirik Bjorsnos <d...@openjdk.org> wrote:
> This PR removes the JAR index feature from the runtime: > > - `URLClassPath` is updated to remove the `enableJarIndex` system property > and any code which would be called when this property was `true` > - The `JarIndex` implementation class is moved into `jdk.jartool` module. > - The `InvalidJarIndexError` exception class is removed because it falls out > of use > - The test `test/jdk/sun/misc/JarIndex/metaInfFileNames/Basic.java` is > removed because it depends on the JarIndex feature being present > - The test `test/jdk/sun/misc/JarIndex/JarIndexMergeForClassLoaderTest.java` > is removed because it depends on the JarIndex feature being present > - The test `test/jdk/sun/misc/JarIndex/JarIndexMergeTest.java` is removed > because it end up being the only caller of the JarIndex.merge feature > - All `JarIndex` methods/constructors which are not used by the `jar -i` > implementation are removed. > - `JarIndex` is given package-private access. > > Outstanding code work: > > - Create tests for `JarFile` and `JarInputStream` accepting dusty INDEX jars. > > Outstanding work: > > - CSR for the removal > - Release notes for the removal > - Coordination of the update of the Jar File Specification Hello Eirik, > Note that this PR does not aim to remove the ability for the jar tool to > produce JAR files with indexes. Such a removal could be handled separately > from this PR. My understanding of https://bugs.openjdk.org/browse/JDK-8302819 is that we are removing the JAR Index since it is no longer usable in recent versions. The work, that by default disabled support for jar indexes in URLClassLoader https://bugs.openjdk.org/browse/JDK-8273473 did not deprecate the `jar -i` option and explicitly noted that it would continue to work the way it used to. Perhaps now is the time to add a deprecation/warning message when `jar -i` option is used? JDK-8302819 does note this as a possibility as part of this change: > The jar -i (--generate-index=FILE) option can continue to generate the JAR > index or the option can be changed to just emit a warning. > > The `InvalidJarIndexError` exception class is removed because it falls out > of use Since `Throwable` classes are serializable, I looked up the serialization spec to see if this removal would have any compatibility issues that need to be accounted for. The specification https://docs.oracle.com/en/java/javase/20/docs/specs/serialization/version.html#compatible-changes states that removing classes is a compatible change, so this should be fine. Plus, this exception appears to be thrown only when index file is being processed (as the name suggests). Sorry my previous message wasn't clear. > Hooking in a simple deprecation warning in this PR would propably not require > a lot of extra review cycles, perhaps the opposite. So I'm happy to do that > if that is what you meant Yes, that's what I meant - I think we should add a warning message when that option is used, as part of these changes. I don't think we can completely remove that option in this release, since it wasn't deprecated (for removal) before, so it would be a good idea to start warning. Like you, I too would let Alan provide guidance on whether this should be done as part of this PR. Hello Eirik, thank you for the updates to the PR. I see that Alan suggested that we take up the `jar -i` deprecation in a separate PR. I think that simplifies the work in this PR. I'll run some tests with these changes and do another review soon, but you don't have to wait for it, if you want to move this PR out of draft status. Hello Eirik, I ran our tier tests against the current state of this PR and they came back fine. With the removal of support for `META-INF/INDEX.LIST`, this PR rightly removes the related test cases. I think we should introduce a new test class with some test methods which verifies that the `java.util.jar.JarFile` and `java.util.jar.JarInputStream` APIs continue to work fine when they are presented with a jar which has a `META-INF/INDEX.LIST`. Some such tests would be to use these APIs to load: - A jar which has a META-INF/MANIFEST.MF and a META-INF/INDEX.LIST - A jar which has a META-INF/INDEX.LIST but no manifest file - A signed jar which has a META-INF/INDEX.LIST Adding these tests I think will help us verify that these APIs and also exercise the code in these classes where we have checks for `META-INF/INDEX.LIST` entry. Is that something you could consider adding as part of this PR? ------------- PR Comment: https://git.openjdk.org/jdk/pull/13158#issuecomment-1481267431 PR Comment: https://git.openjdk.org/jdk/pull/13158#issuecomment-1481283448 PR Comment: https://git.openjdk.org/jdk/pull/13158#issuecomment-1481306676 PR Comment: https://git.openjdk.org/jdk/pull/13158#issuecomment-1483076228 PR Comment: https://git.openjdk.org/jdk/pull/13158#issuecomment-1485073282
