8302819: Remove JAR Index #13158

eirbjo · 2023-03-23T12:05:50Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change requires CSR request JDK-8305597 to be approved
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issues

JDK-8302819: Remove JAR Index
JDK-8305597: Remove JAR Index (CSR)

Reviewers

Mandy Chung (@mlchung - Reviewer) ⚠️ Review applies to 79b19e94
Alan Bateman (@AlanBateman - Reviewer) ⚠️ Review applies to 79b19e94
Lance Andersen (@LanceAndersen - Reviewer)
Jaikiran Pai (@jaikiran - Reviewer) ⚠️ Review applies to 79b19e94

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13158

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-03-23T12:07:07Z

👋 Welcome back eirbjo! 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.

…tool. Remove tests which depend on the jar index feature being considered during class loading.

openjdk · 2023-03-23T12:09:38Z

@eirbjo The following labels will be automatically applied to this pull request:

compiler
core-libs
security

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.

jaikiran · 2023-03-23T14:09:32Z

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.

jaikiran · 2023-03-23T14:18:31Z

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).

eirbjo · 2023-03-23T14:24:48Z

Perhaps now is the time to add a deprecation/warning message when jar -i option is used?

I'm not entirely sure I know what you mean or suggest here, so I'll try to be explicit with what I do (think I) understand and what my reasoning is:

I one of my early comments on JDK-8302819, I suggest / recommend that the deprecation and/or eventual removal of the jar -i options is handled as a follow-up to this PR. My reasoning was the usual concerns about PRs: Reducing the scope seems to make the review process simpler, faster, more efficient.

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 and Alan also agrees.

Do you think I understood your comment?

jaikiran · 2023-03-23T14:33:19Z

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.

eirbjo · 2023-03-23T14:35:16Z

Sorry my previous message wasn't clear.

Thanks for clarifying, Jaikiran. I think our understanding is in sync now, which is always good!

eirbjo · 2023-03-23T14:37:57Z

If we include the deprecation of jar -i in this PR, then the CSR for this PR would need to also cover that deprecation. Same is true for release notes. Not seeing any problem with that myself, but thought it would be good to take note here.

eirbjo · 2023-03-23T14:42:08Z

@jaikiran Do you agree with the move of JarIndex from java.base to jdk.jartool? Since it ends up only being used there, I thought it would be cleaner if we moved it. But the move is not strictly necessary. What's your thoughts?

eirbjo · 2023-03-23T14:58:31Z

Like you, I too would let Alan provide guidance on whether this should be done as part of this PR.

The alternative to including it in this PR would be to do it immediately following the integration of this PR. For users it maybe does not matter much as long as two changes are included in the same JDK/Java SE version?

jaikiran · 2023-03-23T15:10:39Z

@jaikiran Do you agree with the move of JarIndex from java.base to jdk.jartool? Since it ends up only being used there, I thought it would be cleaner if we moved it. But the move is not strictly necessary. What's your thoughts?

With the changes in this PR, the JarIndex (an internal) class would only be relevant/useful in the jar tool. So I think it makes sense to move it to the jdk.jartool module. Additionally you could even make it package-private there, instead of public. It would anyway need some updates to its javadoc and maybe remove any no longer used methods.

On a related note, I see that the module-info.java of java.base module has been updated to export sun.security.action package to the jdk.jartool. I think that is because the (now moved) JarIndex uses the sun.security.action.GetPropertyAction.privilegedGetProperty utility method to read a system property. I don't think we should be exporting that package. Instead, the JarIndex class can be updated to do AccessController.doPrivileged(...).

eirbjo · 2023-03-23T15:31:31Z

I don't think we should be exporting that package. Instead, the JarIndex class can be updated to do AccessController.doPrivileged(...).

Nice, I'll try that!

Do you know if the jar tool allows running with a SecurityManager? If not, we could perhaps simply use System.getProperty?

… use System.property instead since the 'jar' tool does not run with a SecurityManager

eirbjo · 2023-03-23T18:50:27Z

Do you know if the jar tool allows running with a SecurityManager? If not, we could perhaps simply use System.getProperty?

I ended up feeling opimistic and replaced the current code with just System.getProperty. If that is any concern doing that, let me know and I can use AccessController instead.

eirbjo · 2023-03-23T18:53:13Z

Regarding JarIndex:

Additionally you could even make it package-private there, instead of public. It would anyway need some updates to its javadoc and maybe remove any no longer used methods.

Making it package-private did not work because of the JarIndexMergeTest test which directly depends on JarIndex. Changing package of the test did not work because of split packages.

…geTest test. Removing this test, the JarIndex.merge methods and a number of other JarIndex methods not used by the 'jar' tool

eirbjo · 2023-03-23T19:34:43Z

@jaikiran made the good observation that the JarIndex class has various cruft left which is not needed for the jar -i implementation.

Since we at the moment do not know when jar -i will be removed (if ever), we would risk leaving a lot of unused code around if we chose to postpone this cleanup.

Becuse of this, I suggest we extend the scope of this PR to clean up IndexJar for use by jdk.jartool only.

jaikiran · 2023-03-24T05:20:34Z

I don't think we should be exporting that package. Instead, the JarIndex class can be updated to do AccessController.doPrivileged(...).

Nice, I'll try that!

Do you know if the jar tool allows running with a SecurityManager? If not, we could perhaps simply use System.getProperty?

I had a look at the existing code in the jar tool. It appears that it doesn't run with a SecurityManager - there's direct calls to System.getProperty() in some places in the code. So I think just calling System.getProperty() should be fine. When this PR goes into a published state for review, I'm sure others who have more knowledge about this area will help decide.

eirbjo · 2023-03-24T10:56:11Z

@jaikiran

Looking into how the deprecation warning could be implemented, it seems jar has some existing infrastructure for deprecation warnings, so the deprecation could be implemented with the following small patch:

Subject: [PATCH] Remove JarIndex default constructor
---
Index: src/jdk.jartool/share/classes/sun/tools/jar/Main.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/jdk.jartool/share/classes/sun/tools/jar/Main.java b/src/jdk.jartool/share/classes/sun/tools/jar/Main.java
--- a/src/jdk.jartool/share/classes/sun/tools/jar/Main.java	(revision fee0da27be832a7413886ddd2cfd5586bfd2e37a)
+++ b/src/jdk.jartool/share/classes/sun/tools/jar/Main.java	(date 1679654673667)
@@ -396,6 +396,9 @@
                     }
                 }
             } else if (iflag) {
+                if (!suppressDeprecateMsg) {
+                    warn(formatMsg("warn.flag.is.deprecated", "-i"));
+                }
                 String[] files = filesMap.get(BASE_VERSION);  // base entries only, can be null
                 genIndex(rootjar, files);
             } else if (dflag) {

This gives the following warning message on use:

% build/macosx-x86_64-server-release/images/jdk/bin/jar --generate-index file.jar
Warning: The -i option is deprecated, and is planned for removal in a future JDK release

Which may be overridden by using -XDsuppress-tool-removal-message:

% build/macosx-x86_64-server-release/images/jdk/bin/jar --generate-index file.jar -XDsuppress-tool-removal-message

eirbjo · 2023-04-04T19:00:13Z

I added a draft release note https://bugs.openjdk.org/browse/JDK-8305603 as a sub-task of this PR's issue https://bugs.openjdk.org/browse/JDK-8303410

jaikiran

Hello Eirik, the changes look good to me. The new test that was planned for testing JarFile and JarInputStream when dealing with a jar containing a META-INF/INDEX.LIST, is that something that you want to be done as a separate PR/task? It's fine with me if you want that to be done separately.

AlanBateman · 2023-04-05T06:12:24Z

Thanks a lot Alan, as this CSR has deeper waters than what I've experienced before. Did you intend to remove the line break in the example warning output, collapsing it into one line?

I used the inline code formatting (3 backquotes) to make the output clear, but maybe it looks differently for you or maybe your comment was while I was editing? When you are ready, press "Finalize" and that will put in the CSR queue to review.

eirbjo · 2023-04-05T06:18:51Z

I used the inline code formatting (3 backquotes) to make the output clear, but maybe it looks differently for you or maybe your comment was while I was editing?

It looks good now, so my comment was probably made during your edits.

When you are ready, press "Finalize" and that will put in the CSR queue to review.

Done.

eirbjo · 2023-04-05T07:05:11Z

The new test that was planned for testing JarFile and JarInputStream when dealing with a jar containing a META-INF/INDEX.LIST, is that something that you want to be done as a separate PR/task? It's fine with me if you want that to be done separately.

@jaikiran

I was looking into creating such a test yesterday. For JarInputStream, I was able to write a tests which expects a certain order of entries. This could made broken by disabling the META-INF/INDEX.LIST check in JarInputStream.getNextEntry.

With JarVerifier, I'm struggling to create a scenario which excercises the META-INF/INDEX.LIST check. It seems to me this code is simply unreachable, even after moving the META-INF/INDEX.LIST file first in the JAR.

If the code is properly unreachable, then maybe we should consider removing it?

In any case, since I feel the current test is somewhat dubious, I checked it into another branch here:

https://github.com/eirbjo/jdk/blob/remove-jar-index-test/test/jdk/java/util/jar/JarFile/JarIndexLegacyJars.java

This is perhaps better than having no tests. I'm not feeling sure about this, please let me know what you think. Perhaps @AlanBateman also has an opinion on this?

LanceAndersen · 2023-04-05T10:25:01Z

The new test that was planned for testing JarFile and JarInputStream when dealing with a jar containing a META-INF/INDEX.LIST, is that something that you want to be done as a separate PR/task? It's fine with me if you want that to be done separately.

@jaikiran

I was looking into creating such a test yesterday. For JarInputStream, I was able to write a tests which expects a certain order of entries. This could made broken by disabling the META-INF/INDEX.LIST check in JarInputStream.getNextEntry.

With JarVerifier, I'm struggling to create a scenario which excercises the META-INF/INDEX.LIST check. It seems to me this code is simply unreachable, even after moving the META-INF/INDEX.LIST file first in the JAR.

If the code is properly unreachable, then maybe we should consider removing it?

In any case, since I feel the current test is somewhat dubious, I checked it into another branch here:

https://github.com/eirbjo/jdk/blob/remove-jar-index-test/test/jdk/java/util/jar/JarFile/JarIndexLegacyJars.java

This is perhaps better than having no tests. I'm not feeling sure about this, please let me know what you think. Perhaps @AlanBateman also has an opinion on this?

I am not convinced we need a test but to your point Jai, Eirik, lets handle this as a separate PR/Issue and work through what we are hoping to achieve/validate

eirbjo · 2023-04-05T11:06:25Z

I am not convinced we need a test but to your point Jai, Eirik, lets handle this as a separate PR/Issue and work through what we are hoping to achieve/validate

Good. Let's agree to handle testing of dusty 'index jars' separately from this PR.

AlanBateman · 2023-04-06T07:32:12Z

One additional thing we could do is update the usage output for -i/--generate-index to say that the option is deprecated. Looking at jdeps -help output to see this for the deprecated -P/--profile option.

…recated and may be removed in a future release.

eirbjo · 2023-04-07T07:43:17Z

One additional thing we could do is update the usage output for -i/--generate-index to say that the option is deprecated. Looking at jdeps -help output to see this for the deprecated -P/--profile option.

Implemented as suggested. The --help output now looks like:

  -i, --generate-index=FILE  Generate index information for the specified jar
                             archives. This option is deprecated and may be 
                             removed in a future release.

AlanBateman · 2023-04-07T16:29:31Z

I've updated the CSR to align it with the proposed solution. Please review again and if you happy, press "Finalize".

eirbjo · 2023-04-07T16:33:44Z

I've updated the CSR to align it with the proposed solution. Please review again and if you happy, press "Finalize".

Thanks, Alan, this looks good to me. I have pressed "Finalize".

The CSR makes promises about the Release Note. Do you think the current Release Note delivers on this promise?

https://bugs.openjdk.org/browse/JDK-8305603

openjdk · 2023-04-08T01:49:49Z

@eirbjo This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8302819: Remove JAR Index

Reviewed-by: mchung, alanb, lancea, jpai

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 149 new commits pushed to the master branch:

0243da2: 8305481: gtest is_first_C_frame failing on ARM
a8e3a2d: 8205129: Remove java.lang.Compiler
db174a1: 4825182: DefaultBoundedRangeModel.setMinimum() changes extent unnecessarily
969a6b9: 8304825: MacOS metal pipeline - window isn't painted if created during display sleep
9727685: 8305310: Calculate PublicKey from PrivateKey
50d7335: 8305696: (zipfs) Avoid redundant LinkedHashMap.containsKey call ZipFileSystem.makeParentDirs
ec9d816: 6441827: Documentation mentions nonexistent NullReferenceException
9e20382: 8305721: add make compile-commands artifacts to .gitignore
f45b01f: 8305766: ProblemList runtime/CompressedOops/CompressedClassPointers.java
a8871f5: 8305407: ExternalSpecsWriter should ignore white-space differences in spec titles
... and 139 more: https://git.openjdk.org/jdk/compare/1683a63a7df6eb3bd71cd9d0a7ab7081b92107c4...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mlchung, @AlanBateman, @LanceAndersen, @jaikiran) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

eirbjo · 2023-04-09T09:52:23Z

It seems the stars are getting aligned for this PR to be integrated.

None of the current approvals apply to the latest commit d04df25, it would be good to have that jar --help update approved as well.

Are there any other concerns left before we can integrate?

LanceAndersen · 2023-04-10T10:09:04Z

It seems the stars are getting aligned for this PR to be integrated.

None of the current approvals apply to the latest commit d04df25, it would be good to have that jar --help update approved as well.

Are there any other concerns left before we can integrate?

I think we are good to go.

eirbjo · 2023-04-10T10:19:43Z

/integrate

openjdk · 2023-04-10T10:21:06Z

@eirbjo
Your change (at version d04df25) is now ready to be sponsored by a Committer.

eirbjo · 2023-04-10T10:42:54Z

I think we are good to go.

Thanks, the PR is now ready to be sponsored by a Committer.

LanceAndersen · 2023-04-10T11:21:43Z

/sponsor

openjdk · 2023-04-10T11:23:33Z

Going to push as commit 0d45a52.
Since your change was applied there have been 149 commits pushed to the master branch:

0243da2: 8305481: gtest is_first_C_frame failing on ARM
a8e3a2d: 8205129: Remove java.lang.Compiler
db174a1: 4825182: DefaultBoundedRangeModel.setMinimum() changes extent unnecessarily
969a6b9: 8304825: MacOS metal pipeline - window isn't painted if created during display sleep
9727685: 8305310: Calculate PublicKey from PrivateKey
50d7335: 8305696: (zipfs) Avoid redundant LinkedHashMap.containsKey call ZipFileSystem.makeParentDirs
ec9d816: 6441827: Documentation mentions nonexistent NullReferenceException
9e20382: 8305721: add make compile-commands artifacts to .gitignore
f45b01f: 8305766: ProblemList runtime/CompressedOops/CompressedClassPointers.java
a8871f5: 8305407: ExternalSpecsWriter should ignore white-space differences in spec titles
... and 139 more: https://git.openjdk.org/jdk/compare/1683a63a7df6eb3bd71cd9d0a7ab7081b92107c4...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-04-10T11:23:49Z

@LanceAndersen @eirbjo Pushed as commit 0d45a52.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Remove jar index feature from the runtime. Move JarIndex into jdk.jar…

37c409e

…tool. Remove tests which depend on the jar index feature being considered during class loading.

eirbjo force-pushed the remove-jar-index branch from 4894a91 to 37c409e Compare March 23, 2023 12:08

openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org compiler compiler-dev@openjdk.org labels Mar 23, 2023

eirbjo added 2 commits March 23, 2023 13:24

Revert noisy whitespace changes

0c850c8

Revert noisy whitespace changes

ac09ea3

Revert the export of sun.security.action to jdk.jartool. JarIndex can…

9ccc592

… use System.property instead since the 'jar' tool does not run with a SecurityManager

eirbjo added 2 commits March 23, 2023 20:24

The JarIndex.merge method ended up being used only by the JarIndexMer…

3072c50

…geTest test. Removing this test, the JarIndex.merge methods and a number of other JarIndex methods not used by the 'jar' tool

Make JarIndex package private

33b1547

Remove outdated reference to URLClassLoader using JarIndex

b073244

eirbjo added 2 commits March 24, 2023 10:57

Make JarIndex.INDEX_NAME package private

046483a

Remove JarIndex default constructor

fee0da2

jaikiran approved these changes Apr 5, 2023

View reviewed changes

Add a note in the --help for --generate-index that this option is dep…

d04df25

…recated and may be removed in a future release.

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Apr 8, 2023

LanceAndersen approved these changes Apr 10, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 10, 2023

openjdk bot added the integrated Pull request has been integrated label Apr 10, 2023

openjdk bot closed this Apr 10, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 10, 2023

8302819: Remove JAR Index #13158

8302819: Remove JAR Index #13158

Conversation

eirbjo commented Mar 23, 2023 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Mar 23, 2023

openjdk bot commented Mar 23, 2023

jaikiran commented Mar 23, 2023

jaikiran commented Mar 23, 2023

eirbjo commented Mar 23, 2023

jaikiran commented Mar 23, 2023

eirbjo commented Mar 23, 2023

eirbjo commented Mar 23, 2023

eirbjo commented Mar 23, 2023 • edited Loading

eirbjo commented Mar 23, 2023 • edited Loading

jaikiran commented Mar 23, 2023 • edited Loading

eirbjo commented Mar 23, 2023 • edited Loading

eirbjo commented Mar 23, 2023

eirbjo commented Mar 23, 2023 • edited Loading

eirbjo commented Mar 23, 2023

jaikiran commented Mar 24, 2023

eirbjo commented Mar 24, 2023

eirbjo commented Apr 4, 2023

jaikiran left a comment

Choose a reason for hiding this comment

AlanBateman commented Apr 5, 2023

eirbjo commented Apr 5, 2023

eirbjo commented Apr 5, 2023

LanceAndersen commented Apr 5, 2023

eirbjo commented Apr 5, 2023

AlanBateman commented Apr 6, 2023

eirbjo commented Apr 7, 2023

AlanBateman commented Apr 7, 2023

eirbjo commented Apr 7, 2023

openjdk bot commented Apr 8, 2023 • edited Loading

eirbjo commented Apr 9, 2023

LanceAndersen commented Apr 10, 2023

eirbjo commented Apr 10, 2023

openjdk bot commented Apr 10, 2023

eirbjo commented Apr 10, 2023

LanceAndersen commented Apr 10, 2023

openjdk bot commented Apr 10, 2023

openjdk bot commented Apr 10, 2023

eirbjo commented Mar 23, 2023 •

edited by openjdk bot

Loading

eirbjo commented Mar 23, 2023 •

edited

Loading

eirbjo commented Mar 23, 2023 •

edited

Loading

jaikiran commented Mar 23, 2023 •

edited

Loading

eirbjo commented Mar 23, 2023 •

edited

Loading

eirbjo commented Mar 23, 2023 •

edited

Loading

openjdk bot commented Apr 8, 2023 •

edited

Loading