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=5524=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5524=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
