Re: RFR: 8273401: Disable JarIndex support in URLClassPath [v2]
On Thu, 16 Sep 2021 01:29:41 GMT, wxiang wrote: >> wxiang has updated the pull request incrementally with one additional commit >> since the last revision: >> >> add isEmpty check > > I have changed > > ENABLE_JAR_INDEX = p != null ? p.equals("true") : false; > > to > > ENABLE_JAR_INDEX = p != null ? p.equals("true") || p.isEmpty() : false; > > > Furthemore, in order to maintain consistency in URLClassPath, the property > name is "jdk.net.URLClassPath.enableJarIndex" . > @shiyuexw Just a reminder that you need to finalize the CSR. Thanks a lot. We have finalized the CSR. - PR: https://git.openjdk.java.net/jdk/pull/5524
Re: RFR: 8273401: Disable JarIndex support in URLClassPath [v2]
On Thu, 16 Sep 2021 01:29:41 GMT, wxiang wrote: >> wxiang has updated the pull request incrementally with one additional commit >> since the last revision: >> >> add isEmpty check > > I have changed > > ENABLE_JAR_INDEX = p != null ? p.equals("true") : false; > > to > > ENABLE_JAR_INDEX = p != null ? p.equals("true") || p.isEmpty() : false; > > > Furthemore, in order to maintain consistency in URLClassPath, the property > name is "jdk.net.URLClassPath.enableJarIndex" . @shiyuexw Just a reminder that you need to finalize the CSR. - PR: https://git.openjdk.java.net/jdk/pull/5524
Re: RFR: 8273401: Disable JarIndex support in URLClassPath [v2]
On Thu, 16 Sep 2021 11:08:17 GMT, Alan Bateman wrote: >> wxiang has updated the pull request incrementally with one additional commit >> since the last revision: >> >> add isEmpty check > > src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 949: > >> 947: return checkResource(name, check, entry); >> 948: >> 949: if (index == null || !ENABLE_JAR_INDEX) > > Is this needed? When ENABLE_JAR_INDEX is false then I assume index will > always be null. I'm only asking is that it would be nice if ENABLE_JAR_INDEX > was checked in one place rather than two. Yes, the check is redundant, and I removed it. - PR: https://git.openjdk.java.net/jdk/pull/5524
Re: RFR: 8273401: Disable JarIndex support in URLClassPath [v2]
On Thu, 16 Sep 2021 01:29:12 GMT, wxiang wrote: >> There is a bug for URLClassPath.findResources with JarIndex. >> Currently, 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 JarIndex support in URLClassPath. >> >> The PR includes: >> Disable JarIndex support in URLClassPath by default. >> Add system property jdk.net.URLClassPath.enableJarIndex to re-enable >> JarIndex support. > > wxiang has updated the pull request incrementally with one additional commit > since the last revision: > > add isEmpty check src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 949: > 947: return checkResource(name, check, entry); > 948: > 949: if (index == null || !ENABLE_JAR_INDEX) Is this needed? When ENABLE_JAR_INDEX is false then I assume index will always be null. I'm only asking is that it would be nice if ENABLE_JAR_INDEX was checked in one place rather than two. - PR: https://git.openjdk.java.net/jdk/pull/5524
Re: RFR: 8273401: Disable JarIndex support in URLClassPath [v2]
On Thu, 16 Sep 2021 01:29:12 GMT, wxiang wrote: >> There is a bug for URLClassPath.findResources with JarIndex. >> Currently, 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 JarIndex support in URLClassPath. >> >> The PR includes: >> Disable JarIndex support in URLClassPath by default. >> Add system property jdk.net.URLClassPath.enableJarIndex to re-enable >> JarIndex support. > > wxiang has updated the pull request incrementally with one additional commit > since the last revision: > > add isEmpty check Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5524
Re: RFR: 8273401: Disable JarIndex support in URLClassPath [v2]
On Thu, 16 Sep 2021 09:33:44 GMT, wxiang wrote: >> Given that there is a precedence in this file: >> >> >> p = >> props.getProperty("jdk.net.URLClassPath.showIgnoredClassPathEntries"); >> DEBUG_CP_URL_CHECK = p != null ? p.equals("true") || p.isEmpty() : >> false; >> >> >> I would tend to let the new property follow the same pattern, especially >> since it's temporary. > > Yes,I changed the code to follow the same pattern. > Given that there is a precedence in this file: > > ``` > p = > props.getProperty("jdk.net.URLClassPath.showIgnoredClassPathEntries"); > DEBUG_CP_URL_CHECK = p != null ? p.equals("true") || p.isEmpty() : > false; > ``` > > I would tend to let the new property follow the same pattern, especially > since it's temporary. fair enough :-) - PR: https://git.openjdk.java.net/jdk/pull/5524
Re: RFR: 8273401: Disable JarIndex support in URLClassPath [v2]
On Thu, 16 Sep 2021 09:15:34 GMT, Daniel Fuchs wrote: >> I can appreciate that and that is a fair point. I guess where I was >> coming from was that the property is temporary and has to be explicitly >> specified so ignoring the value seemed worth raising/discussing. >> >> Assuming the current check is kept(which we should based on your input), it >> should probably be changed to ignore case. > > Given that there is a precedence in this file: > > > p = > props.getProperty("jdk.net.URLClassPath.showIgnoredClassPathEntries"); > DEBUG_CP_URL_CHECK = p != null ? p.equals("true") || p.isEmpty() : > false; > > > I would tend to let the new property follow the same pattern, especially > since it's temporary. Yes,I changed the code to follow the same pattern. - PR: https://git.openjdk.java.net/jdk/pull/5524
Re: RFR: 8273401: Disable JarIndex support in URLClassPath [v2]
On Wed, 15 Sep 2021 19:41:11 GMT, Lance Andersen wrote: >> I would dislike it if `-Djdk.net.URLClassPath.enableJarIndex=false` meant >> true... > > I can appreciate that and that is a fair point. I guess where I was coming > from was that the property is temporary and has to be explicitly specified so > ignoring the value seemed worth raising/discussing. > > Assuming the current check is kept(which we should based on your input), it > should probably be changed to ignore case. Given that there is a precedence in this file: p = props.getProperty("jdk.net.URLClassPath.showIgnoredClassPathEntries"); DEBUG_CP_URL_CHECK = p != null ? p.equals("true") || p.isEmpty() : false; I would tend to let the new property follow the same pattern, especially since it's temporary. - PR: https://git.openjdk.java.net/jdk/pull/5524
Re: RFR: 8273401: Disable JarIndex support in URLClassPath [v2]
On Thu, 16 Sep 2021 01:29:12 GMT, wxiang wrote: >> There is a bug for URLClassPath.findResources with JarIndex. >> Currently, 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 JarIndex support in URLClassPath. >> >> The PR includes: >> Disable JarIndex support in URLClassPath by default. >> Add system property jdk.net.URLClassPath.enableJarIndex to re-enable >> JarIndex support. > > wxiang has updated the pull request incrementally with one additional commit > since the last revision: > > add isEmpty check Marked as reviewed by dfuchs (Reviewer). Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5524
Re: RFR: 8273401: Disable JarIndex support in URLClassPath [v2]
On Thu, 16 Sep 2021 01:29:12 GMT, wxiang wrote: >> There is a bug for URLClassPath.findResources with JarIndex. >> Currently, 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 JarIndex support in URLClassPath. >> >> The PR includes: >> Disable JarIndex support in URLClassPath by default. >> Add system property jdk.net.URLClassPath.enableJarIndex to re-enable >> JarIndex support. > > wxiang has updated the pull request incrementally with one additional commit > since the last revision: > > add isEmpty check I have changed ENABLE_JAR_INDEX = p != null ? p.equals("true") : false; to ENABLE_JAR_INDEX = p != null ? p.equals("true") || p.isEmpty() : false; Furthemore, in order to maintain consistency in URLClassPath, the property name is "jdk.net.URLClassPath.enableJarIndex" . - PR: https://git.openjdk.java.net/jdk/pull/5524
Re: RFR: 8273401: Disable JarIndex support in URLClassPath [v2]
> There is a bug for URLClassPath.findResources with JarIndex. > Currently, 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 JarIndex support in URLClassPath. > > The PR includes: > Disable JarIndex support in URLClassPath by default. > Add system property jdk.net.URLClassPath.enableJarIndex to re-enable JarIndex > support. wxiang has updated the pull request incrementally with one additional commit since the last revision: add isEmpty check - Changes: - all: https://git.openjdk.java.net/jdk/pull/5524/files - new: https://git.openjdk.java.net/jdk/pull/5524/files/6a6e7b15..8dd17484 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5524&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5524&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5524.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5524/head:pull/5524 PR: https://git.openjdk.java.net/jdk/pull/5524