Re: RFR: JDK-8310550: Adjust references to rt.jar [v5]
On Fri, 30 Jun 2023 09:57:04 GMT, Matthias Baesken wrote: > Hi Alan, I adjusted the comment in DriverManager.java . Thanks, the update looks okay. - PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1257728322
Re: RFR: JDK-8310550: Adjust references to rt.jar [v4]
On Wed, 5 Jul 2023 15:07:15 GMT, Matthias Baesken wrote: >> There are a few references to rt.jar in comments and in the codebase itself. >> Some of them might be removed or adjusted. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Adjust comments Hi Christoph, thanks for the review ! I added the '.' as suggested. Any objections to the latest revision? - PR Comment: https://git.openjdk.org/jdk/pull/14593#issuecomment-1623132227
Re: RFR: JDK-8310550: Adjust references to rt.jar [v5]
On Thu, 22 Jun 2023 09:23:05 GMT, Matthias Baesken wrote: >> test/langtools/tools/javap/4798312/JavapShouldLoadClassesFromRTJarTest.java >> line 1: >> >>> 1: /* >> >> The name of this test includes RTJar. It needs to be changed too I think. >> Does this test actually still test something? > > It seems to start a javap. So I think it tests something, how important this > is and what other tests might cover similar stuff, I do not know > unfortunately . This is a trivial test for a trivial issue. javap will be tested much more thoroughly by other tests. I think this test can be deleted without any loss of coverage. Or it can just be left. - PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1254047816
Re: RFR: JDK-8310550: Adjust references to rt.jar [v5]
> There are a few references to rt.jar in comments and in the codebase itself. > Some of them might be removed or adjusted. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: Adjust comment - Changes: - all: https://git.openjdk.org/jdk/pull/14593/files - new: https://git.openjdk.org/jdk/pull/14593/files/3a7b057a..f29c4019 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14593&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14593&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14593.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14593/head:pull/14593 PR: https://git.openjdk.org/jdk/pull/14593
Re: RFR: JDK-8310550: Adjust references to rt.jar [v3]
On Fri, 30 Jun 2023 11:37:10 GMT, Matthias Baesken wrote: >> There are a few references to rt.jar in comments and in the codebase itself. >> Some of them might be removed or adjusted. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > remove import Hi Christoph, thanks for the suggestions, I added some changes. - PR Comment: https://git.openjdk.org/jdk/pull/14593#issuecomment-1621939153
Re: RFR: JDK-8310550: Adjust references to rt.jar [v4]
On Wed, 5 Jul 2023 15:01:52 GMT, Matthias Baesken wrote: >> There are a few references to rt.jar in comments and in the codebase itself. >> Some of them might be removed or adjusted. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Adjust comments Fine from my end now. Just one minor nit left. 😄 src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java line 196: > 194: > 195: /** > 196: * Set whether or not to use ct.sym as an alternate to the current > runtime You should bring back the period at the end of the sentence. - Marked as reviewed by clanger (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14593#pullrequestreview-1514740197 PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1253244166
Re: RFR: JDK-8310550: Adjust references to rt.jar [v4]
> There are a few references to rt.jar in comments and in the codebase itself. > Some of them might be removed or adjusted. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: Adjust comments - Changes: - all: https://git.openjdk.org/jdk/pull/14593/files - new: https://git.openjdk.org/jdk/pull/14593/files/9b2232a7..3a7b057a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14593&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14593&range=02-03 Stats: 4 lines in 4 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/14593.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14593/head:pull/14593 PR: https://git.openjdk.org/jdk/pull/14593
Re: RFR: JDK-8310550: Adjust references to rt.jar [v3]
On Fri, 30 Jun 2023 11:37:10 GMT, Matthias Baesken wrote:
>> There are a few references to rt.jar in comments and in the codebase itself.
>> Some of them might be removed or adjusted.
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> remove import
Looks good overall. I made a few suggestions.
test/hotspot/jtreg/vmTestbase/nsk/jvmti/AttachOnDemand/attach024/TestDescription.java
line 40:
> 38: * Agent's JAR file contains modified class
> java.util.TooManyListenersException (it is assumed
> 39: * that this class isn't loaded before agent is loaded), agent
> instantiates TooManyListenersException
> 40: * and checks that non-modified version of this class was loaded from
> jdk image (not from agent's JAR).
"from the jdk image"
test/jdk/com/sun/tools/attach/ProviderTest.java line 110:
> 108: public static void main(String args[]) throws Exception {
> 109: // deal with internal builds where classes are loaded from
> the
> 110: // 'classes' directory rather than the image modules file
"... rather than the runtime image"
test/langtools/tools/javap/4798312/JavapShouldLoadClassesFromRTJarTest.java
line 27:
> 25: * @test
> 26: * @bug 4798312
> 27: * @summary In Windows, javap doesn't load classes from image
"... from the runtime image"
-
Changes requested by clanger (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14593#pullrequestreview-1514576016
PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1253140142
PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1253141204
PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1253142105
Re: RFR: JDK-8310550: Adjust references to rt.jar [v3]
On Thu, 22 Jun 2023 09:21:29 GMT, Matthias Baesken wrote: >> src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java >> line 196: >> >>> 194: >>> 195: /** >>> 196: * Set whether or not to use ct.sym as an alternate >> >> As an alternate to what? This needs something else. > > should "to the image modules files" be used instead ? maybe "... to the current runtime."? - PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1253139297
Re: RFR: JDK-8310550: Adjust references to rt.jar [v3]
> There are a few references to rt.jar in comments and in the codebase itself. > Some of them might be removed or adjusted. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: remove import - Changes: - all: https://git.openjdk.org/jdk/pull/14593/files - new: https://git.openjdk.org/jdk/pull/14593/files/6665f60b..9b2232a7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14593&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14593&range=01-02 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14593.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14593/head:pull/14593 PR: https://git.openjdk.org/jdk/pull/14593
Re: RFR: JDK-8310550: Adjust references to rt.jar [v2]
> There are a few references to rt.jar in comments and in the codebase itself. > Some of them might be removed or adjusted. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: Adjust comment in src/java.sql/share/classes/java/sql/DriverManager.java - Changes: - all: https://git.openjdk.org/jdk/pull/14593/files - new: https://git.openjdk.org/jdk/pull/14593/files/5d52b4cb..6665f60b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14593&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14593&range=00-01 Stats: 6 lines in 1 file changed: 1 ins; 2 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14593.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14593/head:pull/14593 PR: https://git.openjdk.org/jdk/pull/14593
Re: RFR: JDK-8310550: Adjust references to rt.jar [v2]
On Wed, 28 Jun 2023 13:22:20 GMT, Matthias Baesken wrote:
>>> Hi Alan, regarding usage of class VM I get 'package jdk.internal.misc is
>>> declared in module java.base, which does not export it to module java.sql'
>>> Is there any concern to export it as well to module java.sql ? And btw did
>>> you mean to use it like this, in the if ?
>>>
>>> `if (callerCL == null || VM.isSystemDomainLoader(callerCL)) { callerCL =
>>> Thread.currentThread().getContextClassLoader(); }`
>>
>> It was just a passing comment, I didn't meant to suggest changing it as part
>> of this PR. We have always think twice before adding qualified exports from
>> java.base and this is case where java.sql is very "non-core", we don't want
>> to give it any access to java.base internals.
>
> Hi Alan, thanks for clarifying. So I should only adjust the comment, correct
> ?
Hi Alan, I adjusted the comment in DriverManager.java .
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1247686721
Re: RFR: JDK-8310550: Adjust references to rt.jar
On Wed, 28 Jun 2023 13:16:30 GMT, Alan Bateman wrote:
>> Hi Alan, regarding usage of class VM I get
>> 'package jdk.internal.misc is declared in module java.base, which does not
>> export it to module java.sql'
>> Is there any concern to export it as well to module java.sql ?
>> And btw did you mean to use it like this, in the if ?
>>
>> `
>> if (callerCL == null || VM.isSystemDomainLoader(callerCL)) {
>> callerCL = Thread.currentThread().getContextClassLoader();
>> }
>> `
>
>> Hi Alan, regarding usage of class VM I get 'package jdk.internal.misc is
>> declared in module java.base, which does not export it to module java.sql'
>> Is there any concern to export it as well to module java.sql ? And btw did
>> you mean to use it like this, in the if ?
>>
>> `if (callerCL == null || VM.isSystemDomainLoader(callerCL)) { callerCL =
>> Thread.currentThread().getContextClassLoader(); }`
>
> It was just a passing comment, I didn't meant to suggest changing it as part
> of this PR. We have always think twice before adding qualified exports from
> java.base and this is case where java.sql is very "non-core", we don't want
> to give it any access to java.base internals.
Hi Alan, thanks for clarifying. So I should only adjust the comment, correct ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1245204791
Re: RFR: JDK-8310550: Adjust references to rt.jar
On Wed, 28 Jun 2023 12:54:10 GMT, Matthias Baesken wrote:
> Hi Alan, regarding usage of class VM I get 'package jdk.internal.misc is
> declared in module java.base, which does not export it to module java.sql' Is
> there any concern to export it as well to module java.sql ? And btw did you
> mean to use it like this, in the if ?
>
> `if (callerCL == null || VM.isSystemDomainLoader(callerCL)) { callerCL =
> Thread.currentThread().getContextClassLoader(); }`
It was just a passing comment, I didn't meant to suggest changing it as part of
this PR. We have always think twice before adding qualified exports from
java.base and this is case where java.sql is very "non-core", we don't want to
give it any access to java.base internals.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1245197137
Re: RFR: JDK-8310550: Adjust references to rt.jar
On Thu, 22 Jun 2023 14:20:30 GMT, Alan Bateman wrote:
>> There are a few references to rt.jar in comments and in the codebase itself.
>> Some of them might be removed or adjusted.
>
> src/java.sql/share/classes/java/sql/DriverManager.java line 658:
>
>> 656: * (which is invoking this class indirectly)
>> 657: * classloader, so that the JDBC driver class outside the image
>> 658: * can be loaded from here.
>
> This code should probably be changed to use VM.isSystemDomainLoader(callerCL).
>
> I think the comment should be replaced because it doesn't match what it
> actually does and it's nothing to do with the whether the JDBC driver is in
> the run-time image or not. How about:
>
> "If the caller is defined to the bootstrap or platform class loader then use
> the Thread CCL as the initiating class loader so that a JDBC on the class
> path, or bundled with an application, is found."
Hi Alan, regarding usage of class VM I get
'package jdk.internal.misc is declared in module java.base, which does not
export it to module java.sql'
Is there any concern to export it as well to module java.sql ?
And btw did you mean to use it like this, in the if ?
`if (callerCL == null || VM.isSystemDomainLoader(callerCL)) {
callerCL = Thread.currentThread().getContextClassLoader();
}`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1245164604
Re: RFR: JDK-8310550: Adjust references to rt.jar
On Wed, 21 Jun 2023 15:18:19 GMT, Matthias Baesken wrote: > There are a few references to rt.jar in comments and in the codebase itself. > Some of them might be removed or adjusted. src/java.sql/share/classes/java/sql/DriverManager.java line 658: > 656: * (which is invoking this class indirectly) > 657: * classloader, so that the JDBC driver class outside the image > 658: * can be loaded from here. This code should probably be changed to use VM.isSystemDomainLoader(callerCL). I think the comment should be replaced because it doesn't match what it actually does and it's nothing to do with the whether the JDBC driver is in the run-time image or not. How about: "If the caller is defined to the bootstrap or platform class loader then use the Thread CCL as the initiating class loader so that a JDBC on the class path, or bundled with an application, is found." - PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1238604300
Re: RFR: JDK-8310550: Adjust references to rt.jar
On Wed, 21 Jun 2023 21:46:03 GMT, David Holmes wrote: >> There are a few references to rt.jar in comments and in the codebase itself. >> Some of them might be removed or adjusted. > > src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java > line 196: > >> 194: >> 195: /** >> 196: * Set whether or not to use ct.sym as an alternate > > As an alternate to what? This needs something else. should "to the image modules files" be used instead ? > test/langtools/tools/javap/4798312/JavapShouldLoadClassesFromRTJarTest.java > line 1: > >> 1: /* > > The name of this test includes RTJar. It needs to be changed too I think. > Does this test actually still test something? It seems to start a javap. So I think it tests something, how important this is and what other tests might cover similar stuff, I do not know unfortunately . - PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1238252196 PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1238254149
Re: RFR: JDK-8310550: Adjust references to rt.jar
On Wed, 21 Jun 2023 15:18:19 GMT, Matthias Baesken wrote: > There are a few references to rt.jar in comments and in the codebase itself. > Some of them might be removed or adjusted. Mostly seems okay - a couple of things need further adjusting I think. Thanks. src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java line 196: > 194: > 195: /** > 196: * Set whether or not to use ct.sym as an alternate As an alternate to what? This needs something else. test/langtools/tools/javap/4798312/JavapShouldLoadClassesFromRTJarTest.java line 1: > 1: /* The name of this test includes RTJar. It needs to be changed too I think. Does this test actually still test something? - Changes requested by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14593#pullrequestreview-1491961660 PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1237747922 PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1237749197
Re: RFR: JDK-8310550: Adjust references to rt.jar
On Wed, 21 Jun 2023 15:18:19 GMT, Matthias Baesken wrote: > There are a few references to rt.jar in comments and in the codebase itself. > Some of them might be removed or adjusted. The update to Java.gmk is good. - Marked as reviewed by erikj (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14593#pullrequestreview-1491263836
RFR: JDK-8310550: Adjust references to rt.jar
There are a few references to rt.jar in comments and in the codebase itself. Some of them might be removed or adjusted. - Commit messages: - JDK-8310550 Changes: https://git.openjdk.org/jdk/pull/14593/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14593&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8310550 Stats: 14 lines in 12 files changed: 0 ins; 7 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/14593.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14593/head:pull/14593 PR: https://git.openjdk.org/jdk/pull/14593
