RFR: 8302819: Remove JAR Index

Tue Mar 28 17:57:39 UTC 2023

On Thu, 23 Mar 2023 12:05:50 GMT, Eirik Bjorsnos <duke at 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