Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Wed, 8 Sep 2021 06:22:38 GMT, wxiang wrote: >> There is a bug for URLClassPath.findResources with JarIndex. >> With some discussions about the bug, the current priority is to remove the >> JAR index support in URLClassPath, >> and don’t need to do anything to the jar tool in the short term, except just >> to move JarIndex to the jdk.jartool module. >> >> The PR includes: >> 1. remove the JarIndex support in URLClassPath >> 2. move JarIndex into jdk.jartool module. > > wxiang has updated the pull request incrementally with one additional commit > since the last revision: > > Some minor changes > > Include: > 1. remove public for INDEX_NAME in JarFile > 2. recover the definition for INDEX_NAME in JarIndex > 3. mv the test case JarIndexMergeTest.java into test/jdk/java/util/jar Firstly, disable JarIndex support in URLClassPath. So close the PR. - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Wed, 15 Sep 2021 05:59:13 GMT, Alan Bateman wrote: > > Yes, I will take care of it. Need I create a new PR? > > Up to you, it's okay to continue with this one if you want, we would just > need to change the description here and in JBS. I created a new PR https://github.com/openjdk/jdk/pull/5524 to disable JarIndex and update the description in JBS. - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Wed, 15 Sep 2021 01:45:49 GMT, wxiang wrote: > Yes, I will take care of it. Need I create a new PR? Up to you, it's okay to continue with this one if you want, we would just need to change the description here and in JBS. - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Wed, 8 Sep 2021 06:30:34 GMT, wxiang wrote: >> wxiang has updated the pull request incrementally with one additional commit >> since the last revision: >> >> Some minor changes >> >> Include: >> 1. remove public for INDEX_NAME in JarFile >> 2. recover the definition for INDEX_NAME in JarIndex >> 3. mv the test case JarIndexMergeTest.java into test/jdk/java/util/jar > > Create CSR: https://bugs.openjdk.java.net/browse/JDK-8273473 > @shiyuexw I discussed this issue with other maintainers in this area. There > was agreement on dropping the support from the URLClassLoader implementation > but it was suggested that it should be disabled for a release or two before > the code is removed. A system property can be used to re-enable. I've updated > the CSR to reflect this proposal. Are you okay to continue with this new > proposal? It would mean that we wouldn't remove the tests but instead change > them to run with the system property. Yes, I will take care of it. Need I create a new PR? - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Wed, 8 Sep 2021 06:30:34 GMT, wxiang wrote: >> wxiang has updated the pull request incrementally with one additional commit >> since the last revision: >> >> Some minor changes >> >> Include: >> 1. remove public for INDEX_NAME in JarFile >> 2. recover the definition for INDEX_NAME in JarIndex >> 3. mv the test case JarIndexMergeTest.java into test/jdk/java/util/jar > > Create CSR: https://bugs.openjdk.java.net/browse/JDK-8273473 @shiyuexw I discussed this issue with other maintainers in this area. There was agreement on dropping the support from the URLClassLoader implementation but it was suggested that it should be disabled for a release or two before the code is removed. A system property can be used to re-enable. I've updated the CSR to reflect this proposal. Are you okay to continue with this new proposal? It would mean that we wouldn't remove the tests but instead change them to run with the system property. - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Wed, 8 Sep 2021 06:30:34 GMT, wxiang wrote: > Create CSR: https://bugs.openjdk.java.net/browse/JDK-8273473 Thanks. I've updated the CSR to make it clearer what this change is about. There is still some TDB for the JAR file spec. - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Wed, 8 Sep 2021 06:22:38 GMT, wxiang wrote: >> There is a bug for URLClassPath.findResources with JarIndex. >> With some discussions about the bug, the current priority is to remove the >> JAR index support in URLClassPath, >> and don’t need to do anything to the jar tool in the short term, except just >> to move JarIndex to the jdk.jartool module. >> >> The PR includes: >> 1. remove the JarIndex support in URLClassPath >> 2. move JarIndex into jdk.jartool module. > > wxiang has updated the pull request incrementally with one additional commit > since the last revision: > > Some minor changes > > Include: > 1. remove public for INDEX_NAME in JarFile > 2. recover the definition for INDEX_NAME in JarIndex > 3. mv the test case JarIndexMergeTest.java into test/jdk/java/util/jar Hi all, Just wondering why we remove JarIndex other than fixing it? Is there any strong motive that drives us to do this? Judging from our internal performance testing and user feedback, JarIndex can significantly reduce the time for class/resource lookup. Although JarIndex is an ancient technology, it is still helpful for many modern scenarios such as serverless applications. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Wed, 8 Sep 2021 06:22:38 GMT, wxiang wrote: >> There is a bug for URLClassPath.findResources with JarIndex. >> With some discussions about the bug, the current priority is to remove the >> JAR index support in URLClassPath, >> and don’t need to do anything to the jar tool in the short term, except just >> to move JarIndex to the jdk.jartool module. >> >> The PR includes: >> 1. remove the JarIndex support in URLClassPath >> 2. move JarIndex into jdk.jartool module. > > wxiang has updated the pull request incrementally with one additional commit > since the last revision: > > Some minor changes > > Include: > 1. remove public for INDEX_NAME in JarFile > 2. recover the definition for INDEX_NAME in JarIndex > 3. mv the test case JarIndexMergeTest.java into test/jdk/java/util/jar Create CSR: https://bugs.openjdk.java.net/browse/JDK-8273473 - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Tue, 7 Sep 2021 17:39:20 GMT, Sean Mullan wrote:
>> src/java.base/share/classes/java/util/jar/JarVerifier.java line 147:
>>
>>> 145:
>>> 146: if (uname.equals(JarFile.MANIFEST_NAME) ||
>>> 147: uname.equals(JarFile.INDEX_NAME)) {
>>
>> It would be useful if someone from security-libs could comment on this. The
>> interaction between signed JAR and JAR index isn't very clear. The change
>> you have is safe but it might be that we can drop the checking for
>> INDEX.LIST here.
>
> I am thinking this line should not be removed for compatibility with existing
> JARs that have indexes.
still keep the code
-
PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Tue, 7 Sep 2021 10:39:01 GMT, Lance Andersen wrote: >> src/java.base/share/classes/java/util/jar/JarFile.java line 220: >> >>> 218: * The index file name. >>> 219: */ >>> 220: public static final String INDEX_NAME = "META-INF/INDEX.LIST"; >> >> Adding this as a public field means it becomes part of the API, so it >> shouldn't be public here. > > Agree remove public，but recover the same definition in JarIndex - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
> There is a bug for URLClassPath.findResources with JarIndex. > With some discussions about the bug, the current priority is to remove the > JAR index support in URLClassPath, > and don’t need to do anything to the jar tool in the short term, except just > to move JarIndex to the jdk.jartool module. > > The PR includes: > 1. remove the JarIndex support in URLClassPath > 2. move JarIndex into jdk.jartool module. wxiang has updated the pull request incrementally with one additional commit since the last revision: Some minor changes Include: 1. remove public for INDEX_NAME in JarFile 2. recover the definition for INDEX_NAME in JarIndex 3. mv the test case JarIndexMergeTest.java into test/jdk/java/util/jar - Changes: - all: https://git.openjdk.java.net/jdk/pull/5383/files - new: https://git.openjdk.java.net/jdk/pull/5383/files/7e11c607..cba9047d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5383=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5383=00-01 Stats: 12 lines in 4 files changed: 5 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/5383.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5383/head:pull/5383 PR: https://git.openjdk.java.net/jdk/pull/5383
