Re: RFR: 8336040: Missing closing anchor element in Docs.gmk
On Tue, 16 Jul 2024 14:12:54 GMT, Nizar Benalla wrote: > Can I please have a review for this bug fix to make file used to generate the > OpenJDK documentation. It adds the missing `` tag after `OTHER > SPECIFICATIONS`, this should help remove some HTML warnings in out generated > docs. > > Thanks in advance. Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20195#pullrequestreview-2181972668
Re: RFR: 8331552: Update to use jtreg 7.4
On Thu, 2 May 2024 09:48:51 GMT, Christian Stein wrote: > Please review the change to update to using `jtreg` **7.4**. > > The primary change is to the `jib-profiles.js` file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > Testing: _tier1-tier5 pending..._ Hello Christian, it would be better to merge latest master branch into this PR before integrating this. It looks like it currently uses `master` from more than a month back. - PR Comment: https://git.openjdk.org/jdk/pull/19052#issuecomment-2171466397
Re: RFR: 8331552: Update to use jtreg 7.4
On Thu, 2 May 2024 09:48:51 GMT, Christian Stein wrote: > Please review the change to update to using `jtreg` **7.4**. > > The primary change is to the `jib-profiles.js` file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > Testing: _tier1-tier5 pending..._ Looks OK to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19052#pullrequestreview-2121354181
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]
On Fri, 17 May 2024 13:38:25 GMT, Maurizio Cimadamore wrote: >> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting >> the use of JNI in the following ways: >> >> * `System::load` and `System::loadLibrary` are now restricted methods >> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods >> * binding a JNI `native` method declaration to a native implementation is >> now considered a restricted operation >> >> This PR slightly changes the way in which the JDK deals with restricted >> methods, even for FFM API calls. In Java 22, the single >> `--enable-native-access` was used both to specify a set of modules for which >> native access should be allowed *and* to specify whether illegal native >> access (that is, native access occurring from a module not specified by >> `--enable-native-access`) should be treated as an error or a warning. More >> specifically, an error is only issued if the `--enable-native-access flag` >> is used at least once. >> >> Here, a new flag is introduced, namely >> `illegal-native-access=allow/warn/deny`, which is used to specify what >> should happen when access to a restricted method and/or functionality is >> found outside the set of modules specified with `--enable-native-access`. >> The default policy is `warn`, but users can select `allow` to suppress the >> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This >> aligns the treatment of restricted methods with other mechanisms, such as >> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`. >> >> Some changes were required in the package-info javadoc for >> `java.lang.foreign`, to reflect the changes in the command line flags >> described above. > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2064736036
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]
On Thu, 16 May 2024 11:47:13 GMT, Maurizio Cimadamore wrote: >> src/java.base/share/classes/sun/launcher/resources/launcher.properties line >> 72: >> >>> 70: \ by code in modules for which native access is not >>> explicitly enabled.\n\ >>> 71: \ is one of "deny", "warn" or "allow".\n\ >>> 72: \ This option will be removed in a future release.\n\ >> >> Should this specify the current default value for this option if it isn't >> set? > > We already do, see > https://github.com/openjdk/jdk/pull/19213/files/1c45e5d56c429205ab8185481bc1044a86ab3bc6#diff-d05029afe6aed86f860a10901114402f1f6af4fe1e4b46d883141ab1d2a527b8R582 This is slightly different from what we do in the other PR for unsafe memory access where we specify the default in the launcher's help text too https://github.com/openjdk/jdk/pull/19174/files#diff-799093930b698e97b23ead98c6496261af1e2e33ec7aa9261584870cbee8a5eaR219. I don't have a strong opinion on this, I think either one is fine. - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603205279
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]
On Wed, 15 May 2024 16:08:17 GMT, Maurizio Cimadamore wrote: >> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting >> the use of JNI in the following ways: >> >> * `System::load` and `System::loadLibrary` are now restricted methods >> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods >> * binding a JNI `native` method declaration to a native implementation is >> now considered a restricted operation >> >> This PR slightly changes the way in which the JDK deals with restricted >> methods, even for FFM API calls. In Java 22, the single >> `--enable-native-access` was used both to specify a set of modules for which >> native access should be allowed *and* to specify whether illegal native >> access (that is, native access occurring from a module not specified by >> `--enable-native-access`) should be treated as an error or a warning. More >> specifically, an error is only issued if the `--enable-native-access flag` >> is used at least once. >> >> Here, a new flag is introduced, namely >> `illegal-native-access=allow/warn/deny`, which is used to specify what >> should happen when access to a restricted method and/or functionality is >> found outside the set of modules specified with `--enable-native-access`. >> The default policy is `warn`, but users can select `allow` to suppress the >> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This >> aligns the treatment of restricted methods with other mechanisms, such as >> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`. >> >> Some changes were required in the package-info javadoc for >> `java.lang.foreign`, to reflect the changes in the command line flags >> described above. > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comment Hello Maurizio, in the current mainline, we have code in `LauncherHelper` https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/launcher/LauncherHelper.java#L636 where we enable native access to all unnamed modules if an executable jar with `Enable-Native-Access: ALL-UNNAMED` manifest is being launched. For such executable jars, what is the expected semantics when the launch also explicitly has a `--enable-native-access=M1,M2` option. Something like: java --enable-native-access=M1,M2 -jar foo.jar where `foo.jar` has `Enable-Native-Access: ALL-UNNAMED` in its manifest. - PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2115005638
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]
On Wed, 15 May 2024 16:08:17 GMT, Maurizio Cimadamore wrote: >> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting >> the use of JNI in the following ways: >> >> * `System::load` and `System::loadLibrary` are now restricted methods >> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods >> * binding a JNI `native` method declaration to a native implementation is >> now considered a restricted operation >> >> This PR slightly changes the way in which the JDK deals with restricted >> methods, even for FFM API calls. In Java 22, the single >> `--enable-native-access` was used both to specify a set of modules for which >> native access should be allowed *and* to specify whether illegal native >> access (that is, native access occurring from a module not specified by >> `--enable-native-access`) should be treated as an error or a warning. More >> specifically, an error is only issued if the `--enable-native-access flag` >> is used at least once. >> >> Here, a new flag is introduced, namely >> `illegal-native-access=allow/warn/deny`, which is used to specify what >> should happen when access to a restricted method and/or functionality is >> found outside the set of modules specified with `--enable-native-access`. >> The default policy is `warn`, but users can select `allow` to suppress the >> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This >> aligns the treatment of restricted methods with other mechanisms, such as >> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`. >> >> Some changes were required in the package-info javadoc for >> `java.lang.foreign`, to reflect the changes in the command line flags >> described above. > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comment src/java.base/share/classes/sun/launcher/resources/launcher.properties line 72: > 70: \ by code in modules for which native access is not > explicitly enabled.\n\ > 71: \ is one of "deny", "warn" or "allow".\n\ > 72: \ This option will be removed in a future release.\n\ Should this specify the current default value for this option if it isn't set? - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603157916
Re: RFR: 8331952: --enable-compatible-cds-alignment should be enabled by default
On Fri, 10 May 2024 01:08:29 GMT, xiaotaonan wrote: >> --enable-compatible-cds-alignment should be enabled by default > > @lgxbslgx Hello @xiaotaonan, the JBS issue against which this PR has been opened has already been closed as a duplicate of https://bugs.openjdk.org/browse/JDK-8331942, which itself has already been resolved. - PR Comment: https://git.openjdk.org/jdk/pull/19150#issuecomment-2103921218
Re: RFR: 8331164: createJMHBundle.sh download jars fail when url needed to be redirected
On Fri, 26 Apr 2024 16:23:28 GMT, SendaoYan wrote:
>>> Hello @sendaoYan, who is adding the 302 redirect to that jar location and
>>> to which location is the URL being redirected to? What does `curl -v -O
>>> ` show in the logs (please paste the textual logs instead of an
>>> image).
>>
>> Hello @jaikiran
>>
>> - who is adding the 302 redirect to that jar location
>>
>> Is `server: Tengine` adding the 302 redirect? I am not sure.
>> - which location is the URL being redirected to
>> It's redirected to oss URL:
>> `https://archiva-maven-storage-prod.oss-cn-beijing.aliyuncs.com/repository/central/org/openjdk/jmh/jmh-core/1.37/jmh-core-1.37.jar?Expires=1714128070=LTAIfU51SusnnfCC=RulXVJOdnC09nHSkGmZmN5NIS98%3D`
>>
>>
>> This is the full testual logs
>> [curl.log](https://github.com/openjdk/jdk/files/15128464/curl.log).
>
>> Thank you for those details @sendaoYan.
>>
>> Adding `-L` (follow redirects) to unconditionally follow redirects doesn't
>> look right to me. I think, one would want to know, during the build process,
>> if any URLs that are in use (like this one) have changed their location and
>> then decide if the build script should be updated to point to the new URL.
>> I'll let the build team decide if this is OK to change. I don't know
>> anything about the server (Maven mirror?) you are using that's generating
>> this redirect, to suggest a workaround.
>
> @jaikiran Hello
> The `maven.aliyun.com` always redirects to a new OSS URL everytime, and the
> OSS URL will expired in 3600 secnods, as show below:
>
> yansendao@j66e07344:[tmp]> date +%s
> 1714148237
> yansendao@j66e07344:[tmp]> curl -OL -v --fail
> https://maven.aliyun.com/repository/public/org/openjdk/jmh/jmh-core/1.37/jmh-core-1.37.jar
> 2>&1 | grep '> GET'
>> GET /repository/public/org/openjdk/jmh/jmh-core/1.37/jmh-core-1.37.jar HTTP/2
>> GET
>> /repository/central/org/openjdk/jmh/jmh-core/1.37/jmh-core-1.37.jar?Expires=1714151841=LTAIfU51SusnnfCC=%2BU29N2ems10WCjO%2FhhLJm6tT6tU%3D
>> HTTP/1.1
> yansendao@j66e07344:[tmp]> curl -OL -v --fail
> https://maven.aliyun.com/repository/public/org/openjdk/jmh/jmh-core/1.37/jmh-core-1.37.jar
> 2>&1 | grep '> GET'
>> GET /repository/public/org/openjdk/jmh/jmh-core/1.37/jmh-core-1.37.jar HTTP/2
>> GET
>> /repository/central/org/openjdk/jmh/jmh-core/1.37/jmh-core-1.37.jar?Expires=1714151845=LTAIfU51SusnnfCC=iu2IfGjRq2p2PZJ6zMg8DtqWg%2Fg%3D
>> HTTP/1.1
>
> So we can't use the final URL in script, because it will expired in short
> time.
> By the way, the
> [make/devkit/createGraphvizBundle.sh](https://github.com/openjdk/jdk/blob/master/make/devkit/createGraphvizBundle.sh#L93)
> file already use `curl -L`
>
>> grep curl make/devkit/createGraphvizBundle.sh
> curl -L -o "${file}" "${url}"
Hello @sendaoYan, I hadn't checked whether we already have such usage in our
build scripts. Thank you for pointing me to it.
Erik's review is what matters here and he's explained that this change is OK
and has approved the PR. So you can go ahead with this proposed change.
-
PR Comment: https://git.openjdk.org/jdk/pull/18965#issuecomment-2080295263
Re: RFR: 8331164: createJMHBundle.sh download jars fail when url needed to be redirected
On Fri, 26 Apr 2024 09:48:51 GMT, SendaoYan wrote: >> Hello @sendaoYan, who is adding the 302 redirect to that jar location and to >> which location is the URL being redirected to? What does `curl -v -O >> ` show in the logs (please paste the textual logs instead of an >> image). > >> Hello @sendaoYan, who is adding the 302 redirect to that jar location and to >> which location is the URL being redirected to? What does `curl -v -O >> ` show in the logs (please paste the textual logs instead of an >> image). > > Hello @jaikiran > > - who is adding the 302 redirect to that jar location > > Is `server: Tengine` adding the 302 redirect? I am not sure. > - which location is the URL being redirected to > It's redirected to oss URL: > `https://archiva-maven-storage-prod.oss-cn-beijing.aliyuncs.com/repository/central/org/openjdk/jmh/jmh-core/1.37/jmh-core-1.37.jar?Expires=1714128070=LTAIfU51SusnnfCC=RulXVJOdnC09nHSkGmZmN5NIS98%3D` > > > This is the full testual logs > [curl.log](https://github.com/openjdk/jdk/files/15128464/curl.log). Thank you for those details @sendaoYan. Adding `-L` (follow redirects) to unconditionally follow redirects doesn't look right to me. I think, one would want to know, during the build process, if any URLs that are in use (like this one) have changed their location and then decide if the build script should be updated to point to the new URL. I'll let the build team decide if this is OK to change. I don't know anything about the server (Maven mirror?) you are using that's generating this redirect, to suggest a workaround. - PR Comment: https://git.openjdk.org/jdk/pull/18965#issuecomment-2079206615
Re: RFR: 8331164: createJMHBundle.sh download jars fail when url needed to be redirected
On Fri, 26 Apr 2024 03:32:25 GMT, SendaoYan wrote: > Hi, > > The curl command lack of "-L" option, cause download file fail, the size of > download file is empty. > >  > > Only change devkit shell script, the risk is low. Hello @sendaoYan, who is adding the 302 redirect to that jar location and to which location is the URL being redirected to? What does `curl -v -O ` show in the logs (please paste the textual logs instead of an image). - PR Comment: https://git.openjdk.org/jdk/pull/18965#issuecomment-2078940654
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 16:13:14 GMT, Jan Lahoda wrote: >> I see. I then misunderstood a part of the PR description. > > I believe we could technically reuse the list for boot/platform modules. But, > the intent here is to provide more control over the modules with native > access, not only being able to add to the list, but also remove from the > list. So, to me, it seemed better to have an explicit list, from/to which we > can remove/add modules easily. Hello Jan, my suggestion was based on a misunderstanding that NATIVE_ACCESS_MODULES is always a superset of boot and platform modules. What you currently have looks fine to me. - PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1513674172
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 14:01:55 GMT, Alan Bateman wrote:
>> make/conf/module-loader-map.conf line 95:
>>
>>> 93: #
>>> 94:
>>> 95: NATIVE_ACCESS_MODULES= \
>>
>> Hello Jan, does this make configuration allow for something like:
>>
>>
>> NATIVE_ACCESS_MODULES= ${BOOT_MODULES} \
>> ${PLATFORM_MODULES} \
>> foo.bar.additional.module
>>
>> (please ignore any syntax issues in that snippet)
>>
>> to make the intention clear as well as reduce the chances of missing some
>> boot or platform module in this NATIVE_ACCESS_MODULES?
>
>> to make the intention clear as well as reduce the chances of missing some
>> boot or platform module in this NATIVE_ACCESS_MODULES?
>
> The list to be be granted native access is a subset, it shouldn't be granted
> all modules mapped to the boot or platform class loaders.
I see. I then misunderstood a part of the PR description.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1512894628
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 12:44:10 GMT, Jan Lahoda wrote:
>> Currently, JDK modules load by the bootstrap and platform ClassLoaders are
>> automatically granted the native access. I am working on an upgrade of JLine
>> inside the `jdk.internal.le` module, and I would like to replace the current
>> native bindings with FFM-based bindings (which are now somewhat provided by
>> JLine). But, for that, native access is needed for the `jdk.internal.le`
>> module. We could possibly move the module to the platform ClassLoader, but
>> it seems it might be better to have more control over which modules have the
>> native access.
>>
>> This patch introduces an explicit list of modules that will automatically be
>> granted the native access. Note this patch is not yet intended to change the
>> end behavior - the list of modules granted native access is supposed to be
>> the same as modules in the boot and platform ClassLoaders.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Apply suggestions from code review
>
> Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> Co-authored-by: Maurizio Cimadamore
> <54672762+mcimadam...@users.noreply.github.com>
Some of the files updated in this PR need a copyright year update.
src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 817:
> 815: JLA.addEnableNativeAccessToAllUnnamed();
> 816: } else if (!JLA.addEnableNativeAccess(layer, name) &&
> shouldWarn) {
> 817: warnUnknownModule(ENABLE_NATIVE_ACCESS, name);
Should we instead warn irrespective of whether or not it's user configured
native access module name or a module name configured in JDK's build
configuration? Or are the build misconfiguration in the `NATIVE_ACCESS_MODULES`
expected to be caught much earlier in some other place?
-
PR Comment: https://git.openjdk.org/jdk/pull/18106#issuecomment-1978836356
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1512869279
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 12:44:10 GMT, Jan Lahoda wrote:
>> Currently, JDK modules load by the bootstrap and platform ClassLoaders are
>> automatically granted the native access. I am working on an upgrade of JLine
>> inside the `jdk.internal.le` module, and I would like to replace the current
>> native bindings with FFM-based bindings (which are now somewhat provided by
>> JLine). But, for that, native access is needed for the `jdk.internal.le`
>> module. We could possibly move the module to the platform ClassLoader, but
>> it seems it might be better to have more control over which modules have the
>> native access.
>>
>> This patch introduces an explicit list of modules that will automatically be
>> granted the native access. Note this patch is not yet intended to change the
>> end behavior - the list of modules granted native access is supposed to be
>> the same as modules in the boot and platform ClassLoaders.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Apply suggestions from code review
>
> Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> Co-authored-by: Maurizio Cimadamore
> <54672762+mcimadam...@users.noreply.github.com>
make/conf/module-loader-map.conf line 95:
> 93: #
> 94:
> 95: NATIVE_ACCESS_MODULES= \
Hello Jan, does this make configuration allow for something like:
NATIVE_ACCESS_MODULES= ${BOOT_MODULES} \
${PLATFORM_MODULES} \
foo.bar.additional.module
(please ignore any syntax issues in that snippet)
to make the intention clear as well as reduce the chances of missing some boot
or platform module in this NATIVE_ACCESS_MODULES?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1512845758
Re: RFR: 8324723: GHA: Upgrade some actions to avoid deprecated Node 16 [v2]
On Fri, 26 Jan 2024 19:33:49 GMT, Aleksey Shipilev wrote: >> Current GHA runs produce lots of warnings: >> >> Node.js 16 actions are deprecated. Please update the following actions to >> use Node.js 20: actions/cache@v3, actions/download-artifact@v3, >> actions/upload-artifact@v3. For more information see: >> https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/. >> >> We can upgrade to new actions to get the Node20. >> >> Release/migration notes: >> https://github.com/actions/cache#whats-new >> https://github.com/actions/upload-artifact/blob/main/docs/MIGRATION.md >> https://github.com/actions/download-artifact/blob/main/docs/MIGRATION.md >> https://github.com/actions/github-script#breaking-changes >> >> There is also msys2/setup-msys2, which was pinned by >> [JDK-8310259](https://bugs.openjdk.org/browse/JDK-8310259). We need at least >> 2.21.0 to get Node 20: >> https://github.com/msys2/setup-msys2/blob/main/CHANGELOG.md. I think we can >> unpin msys2 at this point. >> >> I don't think any of the migration problems outlined in those notes apply to >> our workflows. >> >> Additional testing: >> - [x] 3x GHA with cleaned caches >> - [x] 3x GHA with populated caches (default) > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Also update github-script Hello Severin, > Not really sure how to test the msys2 pinning issue... I think as long as the GitHub actions jobs complete successfully, that should be enough. However, it's not a guarantee that the issue won't happen again (previously, we have had a case where it worked for a few hours or maybe a day when Java 17 was used https://bugs.openjdk.org/browse/JDK-8309934 before it started failing again). If it does happen again, in a subsequent PR, we can always pin it back and spend some more time investigating what's going on. - PR Comment: https://git.openjdk.org/jdk/pull/17572#issuecomment-1914394646
Integrated: 8320931: [REDO] dsymutil command leaves around temporary directories
On Wed, 29 Nov 2023 06:45:17 GMT, Jaikiran Pai wrote: > Can I please get a review of this change will attempts to workaround an issue > in dsymutil? > > The previous attempt to use `--reproducer Off` has shown that it fails to > build on some other Xcode versions other than 14.3.1. Users have reported it > to fail on Xcode 15.0.1 and Xcode from a 12.4 devkit. The `--reproducer` > option isn't being recognized on those versions. > > This current PR proposes to use an alternate workaround, one which just sets > an environment variable that Xcode 14.3.1 recognizes and prevents the > creation of the leftover `dsymutil-*` temporary directories. Since this new > approach uses an environment variable to workaround the issue, the command > itself shouldn't ideally run into any usage issues. Once we agree upon this > change, before integrating, I'll ask for inputs from those who had run into > build issues to verify this alternate approach doesn't cause problems. > > I have locally verified that this new approach continues to work and doesn't > create those temporary directories. Additionally tier1, tier2, tier3 has > completed successfully in our CI environment with this change. > > Magnus, Erik, I looked around to see if there's any convention in setting > such environment variables that will be used when invoking external tools > during the build. I didn't find any similar usage. Although the current > change in this PR is working, if there's a different and better way to do > this, please do let me know. This pull request has now been integrated. Changeset: 6f7bb79a Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/6f7bb79a5b543ebd9ccd72d7b1b289b1f6e4cedb Stats: 11 lines in 1 file changed: 11 ins; 0 del; 0 mod 8320931: [REDO] dsymutil command leaves around temporary directories Reviewed-by: ihse, clanger - PR: https://git.openjdk.org/jdk/pull/16876
Re: RFR: 8320931: [REDO] dsymutil command leaves around temporary directories [v2]
On Thu, 30 Nov 2023 09:39:54 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change will attempts to workaround an >> issue in dsymutil? >> >> The previous attempt to use `--reproducer Off` has shown that it fails to >> build on some other Xcode versions other than 14.3.1. Users have reported it >> to fail on Xcode 15.0.1 and Xcode from a 12.4 devkit. The `--reproducer` >> option isn't being recognized on those versions. >> >> This current PR proposes to use an alternate workaround, one which just sets >> an environment variable that Xcode 14.3.1 recognizes and prevents the >> creation of the leftover `dsymutil-*` temporary directories. Since this new >> approach uses an environment variable to workaround the issue, the command >> itself shouldn't ideally run into any usage issues. Once we agree upon this >> change, before integrating, I'll ask for inputs from those who had run into >> build issues to verify this alternate approach doesn't cause problems. >> >> I have locally verified that this new approach continues to work and doesn't >> create those temporary directories. Additionally tier1, tier2, tier3 has >> completed successfully in our CI environment with this change. >> >> Magnus, Erik, I looked around to see if there's any convention in setting >> such environment variables that will be used when invoking external tools >> during the build. I didn't find any similar usage. Although the current >> change in this PR is working, if there's a different and better way to do >> this, please do let me know. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - check for the support of --reproducer option before using it for dsymutil > - merge latest from master branch > - 8320931: [REDO] dsymutil command leaves around temporary directories Thank you Magnus and others for helping review and test this change. - PR Comment: https://git.openjdk.org/jdk/pull/16876#issuecomment-1837049715
Re: RFR: 8320931: [REDO] dsymutil command leaves around temporary directories [v2]
On Thu, 30 Nov 2023 09:39:54 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change will attempts to workaround an >> issue in dsymutil? >> >> The previous attempt to use `--reproducer Off` has shown that it fails to >> build on some other Xcode versions other than 14.3.1. Users have reported it >> to fail on Xcode 15.0.1 and Xcode from a 12.4 devkit. The `--reproducer` >> option isn't being recognized on those versions. >> >> This current PR proposes to use an alternate workaround, one which just sets >> an environment variable that Xcode 14.3.1 recognizes and prevents the >> creation of the leftover `dsymutil-*` temporary directories. Since this new >> approach uses an environment variable to workaround the issue, the command >> itself shouldn't ideally run into any usage issues. Once we agree upon this >> change, before integrating, I'll ask for inputs from those who had run into >> build issues to verify this alternate approach doesn't cause problems. >> >> I have locally verified that this new approach continues to work and doesn't >> create those temporary directories. Additionally tier1, tier2, tier3 has >> completed successfully in our CI environment with this change. >> >> Magnus, Erik, I looked around to see if there's any convention in setting >> such environment variables that will be used when invoking external tools >> during the build. I didn't find any similar usage. Although the current >> change in this PR is working, if there's a different and better way to do >> this, please do let me know. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - check for the support of --reproducer option before using it for dsymutil > - merge latest from master branch > - 8320931: [REDO] dsymutil command leaves around temporary directories Thank you Christoph for running the tests. - PR Comment: https://git.openjdk.org/jdk/pull/16876#issuecomment-1836058345
Re: RFR: 8320931: [REDO] dsymutil command leaves around temporary directories [v2]
On Thu, 30 Nov 2023 09:39:54 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change will attempts to workaround an >> issue in dsymutil? >> >> The previous attempt to use `--reproducer Off` has shown that it fails to >> build on some other Xcode versions other than 14.3.1. Users have reported it >> to fail on Xcode 15.0.1 and Xcode from a 12.4 devkit. The `--reproducer` >> option isn't being recognized on those versions. >> >> This current PR proposes to use an alternate workaround, one which just sets >> an environment variable that Xcode 14.3.1 recognizes and prevents the >> creation of the leftover `dsymutil-*` temporary directories. Since this new >> approach uses an environment variable to workaround the issue, the command >> itself shouldn't ideally run into any usage issues. Once we agree upon this >> change, before integrating, I'll ask for inputs from those who had run into >> build issues to verify this alternate approach doesn't cause problems. >> >> I have locally verified that this new approach continues to work and doesn't >> create those temporary directories. Additionally tier1, tier2, tier3 has >> completed successfully in our CI environment with this change. >> >> Magnus, Erik, I looked around to see if there's any convention in setting >> such environment variables that will be used when invoking external tools >> during the build. I didn't find any similar usage. Although the current >> change in this PR is working, if there's a different and better way to do >> this, please do let me know. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - check for the support of --reproducer option before using it for dsymutil > - merge latest from master branch > - 8320931: [REDO] dsymutil command leaves around temporary directories @RealCLanger Hello Christoph, in https://bugs.openjdk.org/browse/JDK-8320863 you noted that you ran into a build issue with Xcode version 13.1. Would you be able to test this current proposed patch in this PR against that setup (and any other setups you might have access to) and see if it builds fine? - PR Comment: https://git.openjdk.org/jdk/pull/16876#issuecomment-1833449783
Re: RFR: 8320931: [REDO] dsymutil command leaves around temporary directories [v2]
On Thu, 30 Nov 2023 09:39:54 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change will attempts to workaround an >> issue in dsymutil? >> >> The previous attempt to use `--reproducer Off` has shown that it fails to >> build on some other Xcode versions other than 14.3.1. Users have reported it >> to fail on Xcode 15.0.1 and Xcode from a 12.4 devkit. The `--reproducer` >> option isn't being recognized on those versions. >> >> This current PR proposes to use an alternate workaround, one which just sets >> an environment variable that Xcode 14.3.1 recognizes and prevents the >> creation of the leftover `dsymutil-*` temporary directories. Since this new >> approach uses an environment variable to workaround the issue, the command >> itself shouldn't ideally run into any usage issues. Once we agree upon this >> change, before integrating, I'll ask for inputs from those who had run into >> build issues to verify this alternate approach doesn't cause problems. >> >> I have locally verified that this new approach continues to work and doesn't >> create those temporary directories. Additionally tier1, tier2, tier3 has >> completed successfully in our CI environment with this change. >> >> Magnus, Erik, I looked around to see if there's any convention in setting >> such environment variables that will be used when invoking external tools >> during the build. I didn't find any similar usage. Although the current >> change in this PR is working, if there's a different and better way to do >> this, please do let me know. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - check for the support of --reproducer option before using it for dsymutil > - merge latest from master branch > - 8320931: [REDO] dsymutil command leaves around temporary directories Hello Magnus, thank you for those inputs. Based on that I've now modified this PR to check if the `dsymutil` supports the `--reproducer` option and only if it supports it, I then use that option. I started off with using the `DSYMUTIL_REPRODUCER_PATH` environment variable usage and thought of checking the `dsymutil` version to decide whether or not to set that environment variable. However, it's extremely unclear which exact version of `dsymutil` should be considered as supporting it. In the upstream llvm project, the commit that introduced this auto generation of temporary directories (along with the `--reproducer` option) is this one https://github.com/llvm/llvm-project/commit/33b6891db24dd1e6702b4f04d2b08c1bf417dbee and going by the git tags associated with that commit, it would appear that even 15.0.x version of dsymutil would support the `--reproducer` option. However, we have had reports that `--reproducer` isn't supported on: Apple LLVM version 15.0.0 Optimized build. Default target: arm64-apple-darwin23.1.0 Host CPU: apple-m1 ``` I think we can't rely on the version numbers of dsymutil alone to decide whether or not to set the `DSYMUTIL_REPRODUCER_PATH` environment variable. So I decided to add a check in the build configuration to see if `--reproducer` option is supported by `dsymutil` and since we were checking for this option anyway, then I felt that when this option is supported, it makes sense to set `--reproducer Off` instead of the `DSYMUTIL_REPRODUCER_PATH` environment variable. I have tested this change locally with: dsymutil --version Apple LLVM version 14.0.0 (clang-1400.0.29.202) Optimized build. Default target: arm64-apple-darwin23.1.0 Host CPU: apple-a12 which doesn't support this option and with: Apple LLVM version 14.0.3 (clang-1403.0.22.14.1) Optimized build. Default target: arm64-apple-darwin23.1.0 Host CPU: apple-m1 which supports this option. I've verified that the build change correctly sets the `--reproducer Off` when this option is supported and the temporary directories don't get created. tier1, tier2 and tier3 testing with this change is currently in progress. - PR Comment: https://git.openjdk.org/jdk/pull/16876#issuecomment-1833422791
Re: RFR: 8320931: [REDO] dsymutil command leaves around temporary directories [v2]
> Can I please get a review of this change will attempts to workaround an issue > in dsymutil? > > The previous attempt to use `--reproducer Off` has shown that it fails to > build on some other Xcode versions other than 14.3.1. Users have reported it > to fail on Xcode 15.0.1 and Xcode from a 12.4 devkit. The `--reproducer` > option isn't being recognized on those versions. > > This current PR proposes to use an alternate workaround, one which just sets > an environment variable that Xcode 14.3.1 recognizes and prevents the > creation of the leftover `dsymutil-*` temporary directories. Since this new > approach uses an environment variable to workaround the issue, the command > itself shouldn't ideally run into any usage issues. Once we agree upon this > change, before integrating, I'll ask for inputs from those who had run into > build issues to verify this alternate approach doesn't cause problems. > > I have locally verified that this new approach continues to work and doesn't > create those temporary directories. Additionally tier1, tier2, tier3 has > completed successfully in our CI environment with this change. > > Magnus, Erik, I looked around to see if there's any convention in setting > such environment variables that will be used when invoking external tools > during the build. I didn't find any similar usage. Although the current > change in this PR is working, if there's a different and better way to do > this, please do let me know. Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - check for the support of --reproducer option before using it for dsymutil - merge latest from master branch - 8320931: [REDO] dsymutil command leaves around temporary directories - Changes: - all: https://git.openjdk.org/jdk/pull/16876/files - new: https://git.openjdk.org/jdk/pull/16876/files/6ba87593..0dc877eb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16876=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16876=00-01 Stats: 4598 lines in 168 files changed: 2944 ins; 1101 del; 553 mod Patch: https://git.openjdk.org/jdk/pull/16876.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16876/head:pull/16876 PR: https://git.openjdk.org/jdk/pull/16876
RFR: 8320931: [REDO] dsymutil command leaves around temporary directories
Can I please get a review of this change will attempts to workaround an issue in dsymutil? The previous attempt to use `--reproducer Off` has shown that it fails to build on some other Xcode versions other than 14.3.1. Users have reported it to fail on Xcode 15.0.1 and Xcode from a 12.4 devkit. The `--reproducer` option isn't being recognized on those versions. This current PR proposes to use an alternate workaround, one which just sets an environment variable that Xcode 14.3.1 recognizes and prevents the creation of the leftover `dsymutil-*` temporary directories. Since this new approach uses an environment variable to workaround the issue, the command itself shouldn't ideally run into any usage issues. Once we agree upon this change, before integrating, I'll ask for inputs from those who had run into build issues to verify this alternate approach doesn't cause problems. I have locally verified that this new approach continues to work and doesn't create those temporary directories. Additionally tier1, tier2, tier3 has completed successfully in our CI environment with this change. Magnus, Erik, I looked around to see if there's any convention in setting such environment variables that will be used when invoking external tools during the build. I didn't find any similar usage. Although the current change in this PR is working, if there's a different and better way to do this, please do let me know. - Commit messages: - 8320931: [REDO] dsymutil command leaves around temporary directories Changes: https://git.openjdk.org/jdk/pull/16876/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16876=00 Issue: https://bugs.openjdk.org/browse/JDK-8320931 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16876.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16876/head:pull/16876 PR: https://git.openjdk.org/jdk/pull/16876
Integrated: 8320863: dsymutil command leaves around temporary directories
On Tue, 28 Nov 2023 10:17:51 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to workaround a bug > in `dsymutil` tool on macos? > > As noted in https://bugs.openjdk.org/browse/JDK-8320863, the `dsymutil` tool > shipped in Xcode 14.3.1 has a bug which causes it to create a temporary > directories and leave them around, whenever that tool is used. JDK build uses > that tool in the build process to generate debuginfo files on macos. When I > run the JDK mainline build locally on my macos system, I see that every `make > clean images` build ends up creating and leaving 72 new `dsymutil-*` > temporary directories. > > The bug in dsymutil has been acknowledged and fixed in llvm upstream project > https://github.com/llvm/llvm-project/issues/61920. Until that fix makes it > into Xcode released versions, the JDK build would need a workaround. The > commit in this PR proposes to pass `--reproducer Off` option to this command. > Doing so prevents the tool from leaving around these temporary directories. > As noted in the linked llvm project's issue, these temporary directories are > used for generating any crash report reproducers so that they can then be > submitted as bug reports. Using `--reproducer Off` disables the reproducer > generation > https://github.com/llvm/llvm-project/issues/61920#issuecomment-1495392157: > >> The default is that if dsymutil crashes it will produce a reproducer for >> filing a bug report. So with this option you will not get that. > > Not generating these reproducers should be OK for the JDK builds. > > Before doing this change, I ran several `make clean images` build locally on > my macos and verified that every single run generates these dsymutil > temporary directories. After this proposed change, I reran the same command > again several times and I have verified that these directories are no longer > created. > > tier1,tier2, tier3 testing too has completed successfully with this change. This pull request has now been integrated. Changeset: 86bb8040 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/86bb8040297bef55a46f9089f11481433746a27d Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8320863: dsymutil command leaves around temporary directories Reviewed-by: erikj, ihse - PR: https://git.openjdk.org/jdk/pull/16843
Re: RFR: 8320863: dsymutil command leaves around temporary directories
On Tue, 28 Nov 2023 10:17:51 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to workaround a bug > in `dsymutil` tool on macos? > > As noted in https://bugs.openjdk.org/browse/JDK-8320863, the `dsymutil` tool > shipped in Xcode 14.3.1 has a bug which causes it to create a temporary > directories and leave them around, whenever that tool is used. JDK build uses > that tool in the build process to generate debuginfo files on macos. When I > run the JDK mainline build locally on my macos system, I see that every `make > clean images` build ends up creating and leaving 72 new `dsymutil-*` > temporary directories. > > The bug in dsymutil has been acknowledged and fixed in llvm upstream project > https://github.com/llvm/llvm-project/issues/61920. Until that fix makes it > into Xcode released versions, the JDK build would need a workaround. The > commit in this PR proposes to pass `--reproducer Off` option to this command. > Doing so prevents the tool from leaving around these temporary directories. > As noted in the linked llvm project's issue, these temporary directories are > used for generating any crash report reproducers so that they can then be > submitted as bug reports. Using `--reproducer Off` disables the reproducer > generation > https://github.com/llvm/llvm-project/issues/61920#issuecomment-1495392157: > >> The default is that if dsymutil crashes it will produce a reproducer for >> filing a bug report. So with this option you will not get that. > > Not generating these reproducers should be OK for the JDK builds. > > Before doing this change, I ran several `make clean images` build locally on > my macos and verified that every single run generates these dsymutil > temporary directories. After this proposed change, I reran the same command > again several times and I have verified that these directories are no longer > created. > > tier1,tier2, tier3 testing too has completed successfully with this change. Thank you Erik and Magnus for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/16843#issuecomment-1829997092
RFR: 8320863: dsymutil command leaves around temporary directories
Can I please get a review of this change which proposes to workaround a bug in `dsymutil` tool on macos? As noted in https://bugs.openjdk.org/browse/JDK-8320863, the `dsymutil` tool shipped in Xcode 14.3.1 has a bug which causes it to create a temporary directories and leave them around, whenever that tool is used. JDK build uses that tool in the build process to generate debuginfo files on macos. When I run the JDK mainline build locally on my macos system, I see that every `make clean images` build ends up creating and leaving 72 new `dsymutil-*` temporary directories. The bug in dsymutil has been acknowledged and fixed in llvm upstream project https://github.com/llvm/llvm-project/issues/61920. Until that fix makes it into Xcode released versions, the JDK build would need a workaround. The commit in this PR proposes to pass `--reproducer Off` option to this command. Doing so prevents the tool from leaving around these temporary directories. As noted in the linked llvm project's issue, these temporary directories are used for generating any crash report reproducers so that they can then be submitted as bug reports. Using `--reproducer Off` disables the reproducer generation https://github.com/llvm/llvm-project/issues/61920#issuecomment-1495392157: > The default is that if dsymutil crashes it will produce a reproducer for > filing a bug report. So with this option you will not get that. Not generating these reproducers should be OK for the JDK builds. Before doing this change, I ran several `make clean images` build locally on my macos and verified that every single run generates these dsymutil temporary directories. After this proposed change, I reran the same command again several times and I have verified that these directories are no longer created. tier1,tier2, tier3 testing too has completed successfully with this change. - Commit messages: - 8320863: dsymutil command leaves around temporary directories Changes: https://git.openjdk.org/jdk/pull/16843/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16843=00 Issue: https://bugs.openjdk.org/browse/JDK-8320863 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16843/head:pull/16843 PR: https://git.openjdk.org/jdk/pull/16843
Re: RFR: 8320691: Timeout handler on Windows takes 2 hours to complete [v2]
On Fri, 24 Nov 2023 09:58:17 GMT, Daniel Jeliński wrote: >> The recent cdb versions do not support `.dump /f`: >> >> * >> * .dump /f is not supported on a user mode process. * >> * * >> * .dump /ma creates a complete memory dump of a user mode process. * >> * >> >> and after printing that message, cdb ignores the rest of the command line >> and never quits. >> >> This PR updates the dump command to use the recommended `/ma` parameter. >> This allows the command to produce a dump and complete in a timely manner. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments The timeout change looks good to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16806#pullrequestreview-1747793376
Re: RFR: 8320691: Timeout handler on Windows takes 2 hours to complete
On Fri, 24 Nov 2023 09:05:15 GMT, Jaikiran Pai wrote: > Notice the absence of "params" part in that key. I wonder if that is playing > a role here and whether we should fix this key. Actually ignore that part. I had a look at the internal logs that you referenced. It appears that this form of specifying the timeout (through the use of `params` key) seems to work too. The reason why it took 2 hours is because it ran that command against 2 separate processes and each one timed out after one hour: [2023-11-23 21:45:40] [cdb.exe, -c, ".dump, /f, core.12345;qd", -p, 12345] timeout=360 ... 0:001> WARNING: tool timed out: killed process after 360 ms [2023-11-23 22:45:40] exit code: -2 time: 366 ms [2023-11-23 22:47:36] [cdb.exe, -c, ".dump, /f, core.6789;qd", -p, 6789] timeout=360 ... 0:063> WARNING: tool timed out: killed process after 360 ms [2023-11-23 23:47:36] exit code: -2 time: 356 ms - PR Review Comment: https://git.openjdk.org/jdk/pull/16806#discussion_r1404118909
Re: RFR: 8320691: Timeout handler on Windows takes 2 hours to complete
On Fri, 24 Nov 2023 07:58:18 GMT, Daniel Jeliński wrote: > The recent cdb versions do not support `.dump /f`: > > * > * .dump /f is not supported on a user mode process. * > * * > * .dump /ma creates a complete memory dump of a user mode process. * > * > > and after printing that message, cdb ignores the rest of the command line and > never quits. > > This PR updates the dump command to use the recommended `/ma` parameter. This > allows the command to produce a dump and complete in a timely manner. test/failure_handler/src/share/conf/windows.properties line 61: > 59: native.core.app=cdb > 60: native.core.args=-c ".dump /ma core.%p;qd" -p %p > 61: native.core.params.timeout=360 Hello Daniel, I found it surprising that this takes 2 hours to complete. The failure handler infrastructure has timeout handling built in, after which it kills the failure handler action (the process). Looking at the value specified here it translates to a timeout of 60 minutes (which is too high by the way). So I looked around in some other files and I think there might be a bug here. In other files (linux.properties and mac.properties), I notice the timeout is specified as: native.core.timeout=60 Notice the absence of "params" part in that key. I wonder if that is playing a role here and whether we should fix this key. While at it, perhaps we should also reduce this timeout to may be something lesser (1 hour seems to high). Linux and macosx use a value of `60` which is 10 minutes. If Windows requires a few more minutes then that's understandable but perhaps we should set it to a maximum of 30 minutes maybe? - PR Review Comment: https://git.openjdk.org/jdk/pull/16806#discussion_r1404106460
Re: building on macOS 14
I think it's this place in the build where the logs/warning written out to stdout gets put into the generated source file https://github.com/openjdk/jdk/blob/master/make/modules/java.desktop/gensrc/GensrcIcons.gmk#L85 (there are few more places in that same file which have similar calls). -Jaikiran On 06/11/23 6:33 pm, Magnus Ihse Bursie wrote: On 2023-11-03 21:09, Alan Snyder wrote: On Nov 3, 2023, at 11:54 AM, erik.joels...@oracle.com wrote: If you change out the toolchain through xcode-select, then you definitely need to reconfigure after. I would expect the configuration to be quite messed up if you don’t.. OK, but I think it is a bug when an error message is inserted into generated code. Is the message written to stdout instead of stderr? I agree, this sounds bad and should not happen. Can you give a filename for any such file where it happened? That will help us track down which part of the build that created these files. /Magnus
Integrated: 8317802: jmh tests fail with Unable to find the resource: /META-INF/BenchmarkList after JDK-8306819
On Tue, 10 Oct 2023 14:21:40 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to fix jmh test > launch failures noted in https://bugs.openjdk.org/browse/JDK-8317802? > > jmh apparently relies on annotation processors during compilation of a > benchmark. When annotation processing is disabled, the generated benchmark > jar is unusable when the benchmark is launched and fails with: > > > Exception in thread "main" java.lang.RuntimeException: ERROR: Unable to find > the resource: /META-INF/BenchmarkList > at > org.openjdk.jmh.runner.AbstractResourceReader.getReaders(AbstractResourceReader.java:98) > at org.openjdk.jmh.runner.BenchmarkList.find(BenchmarkList.java:124) > at org.openjdk.jmh.runner.Runner.internalRun(Runner.java:252) > at org.openjdk.jmh.runner.Runner.run(Runner.java:208) > at org.openjdk.jmh.Main.main(Main.java:71) > > After the integration of https://bugs.openjdk.org/browse/JDK-8306819 recently > in JDK mainline, `javac` no longer runs annotation processors by default. > These jmh tests that are compiled as part of the JDK build will now have to > enable annotation processing explicitly. > > The commit in this PR enables annotation processing by setting `-proc:full` > (implying run annotation processors as well as compile the files) when > compiling the benchmarks. > > I tested this change locally. Without this change, the following command > fails: > > > make test TEST="micro:java.lang.ArraysSort" > > ... > Test selection 'micro:java.lang.ArraysSort', will run: > * micro:java.lang.ArraysSort > > Running test 'micro:java.lang.ArraysSort' > Exception in thread "main" java.lang.RuntimeException: ERROR: Unable to find > the resource: /META-INF/BenchmarkList > at > org.openjdk.jmh.runner.AbstractResourceReader.getReaders(AbstractResourceReader.java:98) > at org.openjdk.jmh.runner.BenchmarkList.find(BenchmarkList.java:124) > at org.openjdk.jmh.runner.Runner.internalRun(Runner.java:252) > at org.openjdk.jmh.runner.Runner.run(Runner.java:208) > at org.openjdk.jmh.Main.main(Main.java:71) > Finished running test 'micro:java.lang.ArraysSort' > > > > After the change in this PR, the same command works fine and the jmh > benchmark is run. This pull request has now been integrated. Changeset: 54861df3 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/54861df3d9e29a86dcfcecc4eb5072cc3f006069 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod 8317802: jmh tests fail with Unable to find the resource: /META-INF/BenchmarkList after JDK-8306819 Reviewed-by: erikj, ihse - PR: https://git.openjdk.org/jdk/pull/16122
Re: RFR: 8317802: jmh tests fail with Unable to find the resource: /META-INF/BenchmarkList after JDK-8306819 [v3]
On Tue, 10 Oct 2023 17:09:51 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to fix jmh test >> launch failures noted in https://bugs.openjdk.org/browse/JDK-8317802? >> >> jmh apparently relies on annotation processors during compilation of a >> benchmark. When annotation processing is disabled, the generated benchmark >> jar is unusable when the benchmark is launched and fails with: >> >> >> Exception in thread "main" java.lang.RuntimeException: ERROR: Unable to find >> the resource: /META-INF/BenchmarkList >> at >> org.openjdk.jmh.runner.AbstractResourceReader.getReaders(AbstractResourceReader.java:98) >> at org.openjdk.jmh.runner.BenchmarkList.find(BenchmarkList.java:124) >> at org.openjdk.jmh.runner.Runner.internalRun(Runner.java:252) >> at org.openjdk.jmh.runner.Runner.run(Runner.java:208) >> at org.openjdk.jmh.Main.main(Main.java:71) >> >> After the integration of https://bugs.openjdk.org/browse/JDK-8306819 >> recently in JDK mainline, `javac` no longer runs annotation processors by >> default. These jmh tests that are compiled as part of the JDK build will now >> have to enable annotation processing explicitly. >> >> The commit in this PR enables annotation processing by setting `-proc:full` >> (implying run annotation processors as well as compile the files) when >> compiling the benchmarks. >> >> I tested this change locally. Without this change, the following command >> fails: >> >> >> make test TEST="micro:java.lang.ArraysSort" >> >> ... >> Test selection 'micro:java.lang.ArraysSort', will run: >> * micro:java.lang.ArraysSort >> >> Running test 'micro:java.lang.ArraysSort' >> Exception in thread "main" java.lang.RuntimeException: ERROR: Unable to find >> the resource: /META-INF/BenchmarkList >> at >> org.openjdk.jmh.runner.AbstractResourceReader.getReaders(AbstractResourceReader.java:98) >> at org.openjdk.jmh.runner.BenchmarkList.find(BenchmarkList.java:124) >> at org.openjdk.jmh.runner.Runner.internalRun(Runner.java:252) >> at org.openjdk.jmh.runner.Runner.run(Runner.java:208) >> at org.openjdk.jmh.Main.main(Main.java:71) >> Finished running test 'micro:java.lang.ArraysSort' >> >> >> >> After the change in this PR, the same command works fine and the jmh >> benchmark is run. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Erik's suggestion - move param to new line Hello Magnus, > @jaikiran Was your intention that someone else should integrate this patch, > and if so, who, and depending on what criteria? It was too late for me yesterday here, so I didn't want to rush this in and cause any unexpected CI failures when I am not around. But since this was already reviewed and if any committer in their workday was blocked on this and wanted to integrate it (and of course watch for unexpected failures), then I thought it would be good for them to go ahead and integrate. If that didn't happen then I planned to integrate it my morning time (now). Thank you Erik and Magnus for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/16122#issuecomment-1756545056 PR Comment: https://git.openjdk.org/jdk/pull/16122#issuecomment-1756546450
Re: RFR: 8317802: jmh tests fail with Unable to find the resource: /META-INF/BenchmarkList after JDK-8306819 [v2]
On Tue, 10 Oct 2023 14:42:37 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to fix jmh test >> launch failures noted in https://bugs.openjdk.org/browse/JDK-8317802? >> >> jmh apparently relies on annotation processors during compilation of a >> benchmark. When annotation processing is disabled, the generated benchmark >> jar is unusable when the benchmark is launched and fails with: >> >> >> Exception in thread "main" java.lang.RuntimeException: ERROR: Unable to find >> the resource: /META-INF/BenchmarkList >> at >> org.openjdk.jmh.runner.AbstractResourceReader.getReaders(AbstractResourceReader.java:98) >> at org.openjdk.jmh.runner.BenchmarkList.find(BenchmarkList.java:124) >> at org.openjdk.jmh.runner.Runner.internalRun(Runner.java:252) >> at org.openjdk.jmh.runner.Runner.run(Runner.java:208) >> at org.openjdk.jmh.Main.main(Main.java:71) >> >> After the integration of https://bugs.openjdk.org/browse/JDK-8306819 >> recently in JDK mainline, `javac` no longer runs annotation processors by >> default. These jmh tests that are compiled as part of the JDK build will now >> have to enable annotation processing explicitly. >> >> The commit in this PR enables annotation processing by setting `-proc:full` >> (implying run annotation processors as well as compile the files) when >> compiling the benchmarks. >> >> I tested this change locally. Without this change, the following command >> fails: >> >> >> make test TEST="micro:java.lang.ArraysSort" >> >> ... >> Test selection 'micro:java.lang.ArraysSort', will run: >> * micro:java.lang.ArraysSort >> >> Running test 'micro:java.lang.ArraysSort' >> Exception in thread "main" java.lang.RuntimeException: ERROR: Unable to find >> the resource: /META-INF/BenchmarkList >> at >> org.openjdk.jmh.runner.AbstractResourceReader.getReaders(AbstractResourceReader.java:98) >> at org.openjdk.jmh.runner.BenchmarkList.find(BenchmarkList.java:124) >> at org.openjdk.jmh.runner.Runner.internalRun(Runner.java:252) >> at org.openjdk.jmh.runner.Runner.run(Runner.java:208) >> at org.openjdk.jmh.Main.main(Main.java:71) >> Finished running test 'micro:java.lang.ArraysSort' >> >> >> >> After the change in this PR, the same command works fine and the jmh >> benchmark is run. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > use specific annotation processor instead of -proc:full Hello Erik, thank you for the review. I've updated the PR to move that new param to a new line as suggested. A clean rebuild and test run of a micro benchmark continues to pass with this latest change. - PR Comment: https://git.openjdk.org/jdk/pull/16122#issuecomment-175581
Re: RFR: 8317802: jmh tests fail with Unable to find the resource: /META-INF/BenchmarkList after JDK-8306819 [v3]
> Can I please get a review of this change which proposes to fix jmh test > launch failures noted in https://bugs.openjdk.org/browse/JDK-8317802? > > jmh apparently relies on annotation processors during compilation of a > benchmark. When annotation processing is disabled, the generated benchmark > jar is unusable when the benchmark is launched and fails with: > > > Exception in thread "main" java.lang.RuntimeException: ERROR: Unable to find > the resource: /META-INF/BenchmarkList > at > org.openjdk.jmh.runner.AbstractResourceReader.getReaders(AbstractResourceReader.java:98) > at org.openjdk.jmh.runner.BenchmarkList.find(BenchmarkList.java:124) > at org.openjdk.jmh.runner.Runner.internalRun(Runner.java:252) > at org.openjdk.jmh.runner.Runner.run(Runner.java:208) > at org.openjdk.jmh.Main.main(Main.java:71) > > After the integration of https://bugs.openjdk.org/browse/JDK-8306819 recently > in JDK mainline, `javac` no longer runs annotation processors by default. > These jmh tests that are compiled as part of the JDK build will now have to > enable annotation processing explicitly. > > The commit in this PR enables annotation processing by setting `-proc:full` > (implying run annotation processors as well as compile the files) when > compiling the benchmarks. > > I tested this change locally. Without this change, the following command > fails: > > > make test TEST="micro:java.lang.ArraysSort" > > ... > Test selection 'micro:java.lang.ArraysSort', will run: > * micro:java.lang.ArraysSort > > Running test 'micro:java.lang.ArraysSort' > Exception in thread "main" java.lang.RuntimeException: ERROR: Unable to find > the resource: /META-INF/BenchmarkList > at > org.openjdk.jmh.runner.AbstractResourceReader.getReaders(AbstractResourceReader.java:98) > at org.openjdk.jmh.runner.BenchmarkList.find(BenchmarkList.java:124) > at org.openjdk.jmh.runner.Runner.internalRun(Runner.java:252) > at org.openjdk.jmh.runner.Runner.run(Runner.java:208) > at org.openjdk.jmh.Main.main(Main.java:71) > Finished running test 'micro:java.lang.ArraysSort' > > > > After the change in this PR, the same command works fine and the jmh > benchmark is run. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Erik's suggestion - move param to new line - Changes: - all: https://git.openjdk.org/jdk/pull/16122/files - new: https://git.openjdk.org/jdk/pull/16122/files/24a05dff..d3fa29b9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16122=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16122=01-02 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16122.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16122/head:pull/16122 PR: https://git.openjdk.org/jdk/pull/16122
Re: RFR: 8317802: jmh tests fail with Unable to find the resource: /META-INF/BenchmarkList after JDK-8306819
On Tue, 10 Oct 2023 14:21:40 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to fix jmh test > launch failures noted in https://bugs.openjdk.org/browse/JDK-8317802? > > jmh apparently relies on annotation processors during compilation of a > benchmark. When annotation processing is disabled, the generated benchmark > jar is unusable when the benchmark is launched and fails with: > > > Exception in thread "main" java.lang.RuntimeException: ERROR: Unable to find > the resource: /META-INF/BenchmarkList > at > org.openjdk.jmh.runner.AbstractResourceReader.getReaders(AbstractResourceReader.java:98) > at org.openjdk.jmh.runner.BenchmarkList.find(BenchmarkList.java:124) > at org.openjdk.jmh.runner.Runner.internalRun(Runner.java:252) > at org.openjdk.jmh.runner.Runner.run(Runner.java:208) > at org.openjdk.jmh.Main.main(Main.java:71) > > After the integration of https://bugs.openjdk.org/browse/JDK-8306819 recently > in JDK mainline, `javac` no longer runs annotation processors by default. > These jmh tests that are compiled as part of the JDK build will now have to > enable annotation processing explicitly. > > The commit in this PR enables annotation processing by setting `-proc:full` > (implying run annotation processors as well as compile the files) when > compiling the benchmarks. > > I tested this change locally. Without this change, the following command > fails: > > > make test TEST="micro:java.lang.ArraysSort" > > ... > Test selection 'micro:java.lang.ArraysSort', will run: > * micro:java.lang.ArraysSort > > Running test 'micro:java.lang.ArraysSort' > Exception in thread "main" java.lang.RuntimeException: ERROR: Unable to find > the resource: /META-INF/BenchmarkList > at > org.openjdk.jmh.runner.AbstractResourceReader.getReaders(AbstractResourceReader.java:98) > at org.openjdk.jmh.runner.BenchmarkList.find(BenchmarkList.java:124) > at org.openjdk.jmh.runner.Runner.internalRun(Runner.java:252) > at org.openjdk.jmh.runner.Runner.run(Runner.java:208) > at org.openjdk.jmh.Main.main(Main.java:71) > Finished running test 'micro:java.lang.ArraysSort' > > > > After the change in this PR, the same command works fine and the jmh > benchmark is run. Magnus noted in the JBS issue that it would be good to enable only the specific JMH annotation processor instead of using `-proc:full`. I agree with Magnus. I looked at the annotation processor that jmh jar ships with (in `META-INF/services`) and I see that it's `org.openjdk.jmh.generators.BenchmarkProcessor`. I've now updated this PR to use `-processor` option instead of `-proc:full`. With this latest change, I cleaned up the local JDK build and rebuilt everything fresh and used the same command as previously to run the micro benchmark. It continues to pass with this new change. - PR Comment: https://git.openjdk.org/jdk/pull/16122#issuecomment-1755567425
Re: RFR: 8317802: jmh tests fail with Unable to find the resource: /META-INF/BenchmarkList after JDK-8306819 [v2]
> Can I please get a review of this change which proposes to fix jmh test > launch failures noted in https://bugs.openjdk.org/browse/JDK-8317802? > > jmh apparently relies on annotation processors during compilation of a > benchmark. When annotation processing is disabled, the generated benchmark > jar is unusable when the benchmark is launched and fails with: > > > Exception in thread "main" java.lang.RuntimeException: ERROR: Unable to find > the resource: /META-INF/BenchmarkList > at > org.openjdk.jmh.runner.AbstractResourceReader.getReaders(AbstractResourceReader.java:98) > at org.openjdk.jmh.runner.BenchmarkList.find(BenchmarkList.java:124) > at org.openjdk.jmh.runner.Runner.internalRun(Runner.java:252) > at org.openjdk.jmh.runner.Runner.run(Runner.java:208) > at org.openjdk.jmh.Main.main(Main.java:71) > > After the integration of https://bugs.openjdk.org/browse/JDK-8306819 recently > in JDK mainline, `javac` no longer runs annotation processors by default. > These jmh tests that are compiled as part of the JDK build will now have to > enable annotation processing explicitly. > > The commit in this PR enables annotation processing by setting `-proc:full` > (implying run annotation processors as well as compile the files) when > compiling the benchmarks. > > I tested this change locally. Without this change, the following command > fails: > > > make test TEST="micro:java.lang.ArraysSort" > > ... > Test selection 'micro:java.lang.ArraysSort', will run: > * micro:java.lang.ArraysSort > > Running test 'micro:java.lang.ArraysSort' > Exception in thread "main" java.lang.RuntimeException: ERROR: Unable to find > the resource: /META-INF/BenchmarkList > at > org.openjdk.jmh.runner.AbstractResourceReader.getReaders(AbstractResourceReader.java:98) > at org.openjdk.jmh.runner.BenchmarkList.find(BenchmarkList.java:124) > at org.openjdk.jmh.runner.Runner.internalRun(Runner.java:252) > at org.openjdk.jmh.runner.Runner.run(Runner.java:208) > at org.openjdk.jmh.Main.main(Main.java:71) > Finished running test 'micro:java.lang.ArraysSort' > > > > After the change in this PR, the same command works fine and the jmh > benchmark is run. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: use specific annotation processor instead of -proc:full - Changes: - all: https://git.openjdk.org/jdk/pull/16122/files - new: https://git.openjdk.org/jdk/pull/16122/files/d980215a..24a05dff Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16122=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16122=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16122.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16122/head:pull/16122 PR: https://git.openjdk.org/jdk/pull/16122
RFR: 8317802: jmh tests fail with Unable to find the resource: /META-INF/BenchmarkList after JDK-8306819
Can I please get a review of this change which proposes to fix jmh test launch failures noted in https://bugs.openjdk.org/browse/JDK-8317802? jmh apparently relies on annotation processors during compilation of a benchmark. When annotation processing is disabled, the generated benchmark jar is unusable when the benchmark is launched and fails with: Exception in thread "main" java.lang.RuntimeException: ERROR: Unable to find the resource: /META-INF/BenchmarkList at org.openjdk.jmh.runner.AbstractResourceReader.getReaders(AbstractResourceReader.java:98) at org.openjdk.jmh.runner.BenchmarkList.find(BenchmarkList.java:124) at org.openjdk.jmh.runner.Runner.internalRun(Runner.java:252) at org.openjdk.jmh.runner.Runner.run(Runner.java:208) at org.openjdk.jmh.Main.main(Main.java:71) After the integration of https://bugs.openjdk.org/browse/JDK-8306819 recently in JDK mainline, `javac` no longer runs annotation processors by default. These jmh tests that are compiled as part of the JDK build will now have to enable annotation processing explicitly. The commit in this PR enables annotation processing by setting `-proc:full` (implying run annotation processors as well as compile the files) when compiling the benchmarks. I tested this change locally. Without this change, the following command fails: make test TEST="micro:java.lang.ArraysSort" ... Test selection 'micro:java.lang.ArraysSort', will run: * micro:java.lang.ArraysSort Running test 'micro:java.lang.ArraysSort' Exception in thread "main" java.lang.RuntimeException: ERROR: Unable to find the resource: /META-INF/BenchmarkList at org.openjdk.jmh.runner.AbstractResourceReader.getReaders(AbstractResourceReader.java:98) at org.openjdk.jmh.runner.BenchmarkList.find(BenchmarkList.java:124) at org.openjdk.jmh.runner.Runner.internalRun(Runner.java:252) at org.openjdk.jmh.runner.Runner.run(Runner.java:208) at org.openjdk.jmh.Main.main(Main.java:71) Finished running test 'micro:java.lang.ArraysSort' After the change in this PR, the same command works fine and the jmh benchmark is run. - Commit messages: - 8317802: jmh tests fail with Unable to find the resource: /META-INF/BenchmarkList after JDK-8306819 Changes: https://git.openjdk.org/jdk/pull/16122/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16122=00 Issue: https://bugs.openjdk.org/browse/JDK-8317802 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16122.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16122/head:pull/16122 PR: https://git.openjdk.org/jdk/pull/16122
Re: RFR: 8314495: Update to use jtreg 7.3.1
On Thu, 17 Aug 2023 07:24:14 GMT, Christian Stein wrote: > Please review the change to update to using jtreg 7.3,1. > > The primary change is to the `jib-profiles.js` file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > This change set also un-problem-lists tests from > https://github.com/openjdk/jdk/commit/360f65d7b15b327e2f160c42f318945cc6548bda > > This change set also fixes: > - https://bugs.openjdk.org/browse/JDK-8313902 > - https://bugs.openjdk.org/browse/JDK-8313903 > > Testing: tier1-tier5 OK Looks OK to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15323#pullrequestreview-1585812406
Integrated: 8313274: [BACKOUT] Relax prerequisites for java.base-jmod target
On Wed, 2 Aug 2023 06:54:36 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to fix > https://bugs.openjdk.org/browse/JDK-8313274? > > The commit in this PR reverts the commit that was introduced in > https://github.com/openjdk/jdk/pull/14561. Martin, in the 8313274 JBS issue, > notes that the failure started happening with that change. > > The issue can be consistently reproduced using `--with-jobs=1` during `bash > configure` and then triggering a build using `make images`. After the revert > of that commit, the build now passes with both `--with-jobs=1` and without > the `--with-jobs` option (in which case it uses 8 jobs on my setup). > > The other PR where this commit was introduced noted that: > >> Fixing this won't impact the build much, but certainly won't hurt either. > > which I think meant that the change in that PR is mostly a good to have. > Given that it does cause these unexpected failures, the proposal in this PR > is to revert that change until we have a better way to do that change (if we > want to do it). > > tier testing is currently in progress with this change. This pull request has now been integrated. Changeset: 3c920f9c Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/3c920f9cc61566b7bd08d2bf8773d39a616082d3 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod 8313274: [BACKOUT] Relax prerequisites for java.base-jmod target Reviewed-by: dholmes - PR: https://git.openjdk.org/jdk/pull/15118
Re: RFR: 8313274: [BACKOUT] Relax prerequisites for java.base-jmod target
On Wed, 2 Aug 2023 06:54:36 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to fix > https://bugs.openjdk.org/browse/JDK-8313274? > > The commit in this PR reverts the commit that was introduced in > https://github.com/openjdk/jdk/pull/14561. Martin, in the 8313274 JBS issue, > notes that the failure started happening with that change. > > The issue can be consistently reproduced using `--with-jobs=1` during `bash > configure` and then triggering a build using `make images`. After the revert > of that commit, the build now passes with both `--with-jobs=1` and without > the `--with-jobs` option (in which case it uses 8 jobs on my setup). > > The other PR where this commit was introduced noted that: > >> Fixing this won't impact the build much, but certainly won't hurt either. > > which I think meant that the change in that PR is mostly a good to have. > Given that it does cause these unexpected failures, the proposal in this PR > is to revert that change until we have a better way to do that change (if we > want to do it). > > tier testing is currently in progress with this change. Thank you David for the review and the pointer to the backout instructions. I've followed those instructions and updated this PR title as well the relevant JBS issues to track this backout. I'll go ahead and integrate this PR in the next hour or so. - PR Comment: https://git.openjdk.org/jdk/pull/15118#issuecomment-1663369645
Re: RFR: 8313274: Failure building java.base-jmod target
On Wed, 2 Aug 2023 06:54:36 GMT, Jaikiran Pai wrote: > tier testing is currently in progress with this change. tier1, tier2 and tier3 tests have passed with this change. - PR Comment: https://git.openjdk.org/jdk/pull/15118#issuecomment-1661821594
RFR: 8313274: Failure building java.base-jmod target
Can I please get a review of this change which proposes to fix https://bugs.openjdk.org/browse/JDK-8313274? The commit in this PR reverts the commit that was introduced in https://github.com/openjdk/jdk/pull/14561. Martin, in the 8313274 JBS issue, notes that the failure started happening with that change. The issue can be consistently reproduced using `--with-jobs=1` during `bash configure` and then triggering a build using `make images`. After the revert of that commit, the build now passes with both `--with-jobs=1` and without the `--with-jobs` option (in which case it uses 8 jobs on my setup). The other PR where this commit was introduced noted that: > Fixing this won't impact the build much, but certainly won't hurt either. which I think meant that the change in that PR is mostly a good to have. Given that it does cause these unexpected failures, the proposal in this PR is to revert that change until we have a better way to do that change (if we want to do it). tier testing is currently in progress with this change. - Commit messages: - Revert "8310379: Relax prerequisites for java.base-jmod target" Changes: https://git.openjdk.org/jdk/pull/15118/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15118=00 Issue: https://bugs.openjdk.org/browse/JDK-8313274 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15118.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15118/head:pull/15118 PR: https://git.openjdk.org/jdk/pull/15118
Re: RFR: 8313274: Failure building java.base-jmod target
On Wed, 2 Aug 2023 05:24:28 GMT, David Holmes wrote: > @jaikiran I would concur - back out the change that caused the problem. Hello David, I have now opened a PR to revert that change https://github.com/openjdk/jdk/pull/15118 - PR Comment: https://git.openjdk.org/jdk/pull/15102#issuecomment-1661607103
Re: RFR: 8313274: Failure building java.base-jmod target
On Tue, 1 Aug 2023 10:30:18 GMT, Alan Bateman wrote: > It also opens the door to accidental mix 'n match of modules from different > JDK modules. So I don't think this the right change for this issue. - PR Comment: https://git.openjdk.org/jdk/pull/15102#issuecomment-1660035694
Withdrawn: 8313274: Failure building java.base-jmod target
On Tue, 1 Aug 2023 09:01:49 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to address the build > failure noted in https://bugs.openjdk.org/browse/JDK-8313274? > > The build failure is consistently reproducible with `--with-jobs=1`. Martin, > in that JBS issue, has narrowed down the commit to the change in > https://github.com/openjdk/jdk/pull/14561, starting which this failure is > reproducible. The change in that PR, from what I understand, was meant to not > require upgradable modules be a prerequisite for the `java.base-jmod` make > target: > >> ... upgradeable modules, those shouldn't be on the prerequisites list for >> java.base-jmod. > > The implementation of that change uses the `FindAllUpgradeableModules` > function which as commented in the make files does: > >> #Upgradeable modules are those that are either defined as upgradeable or that >> #require an upradeable module. > > The implementation of `FindAllUpgradeableModules` uses the > `UPGRADEABLE_PLATFORM_MODULES` make variable which similarly comments: > >> #Modules that directly or indirectly requiring upgradeable modules >> #should carefully be considered if it should be upgradeable or not. > > However, that set currently doesn't include the "indirectly requiring > upgradable modules" and thus appears to be missing some of the modules that > are considered upgradable. > > As a result, what seems to be happening is that the `java.base-jmod` make > target now can (and does) end up requiring a particular target as a > prerequisite, say `jdk.jdeps` (which uses `java.compiler` upgradable module), > but doesn't add the `java.compiler-jmod` target as a prerequisite and thus > ends up with that build failure: > > > Creating java.base.jmod > Error: Resolution failed: Module java.compiler not found, required by > jdk.jdeps > > > The commit in this PR proposes to fix this by updating the static set of > `UPGRADEABLE_PLATFORM_MODULES` to include the indirect modules that require > the upgradable modules. How these additional modules were derived is > explained as a separate comment in this PR. > > The build succeeds with this change, both with `--with-jobs=1` and without > the `--with-jobs` option (in which case on my setup it uses 8 jobs). > > I have triggered tier testing in the CI to make sure this doesn't cause any > unexpected regressions. > > This change will require reviews from both the build team as well as the Java > modules team - my knowledge of these areas is limited and I'm unsure if there > are any additional considerations to take into account. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/15102
Re: RFR: 8313274: Failure building java.base-jmod target
On Tue, 1 Aug 2023 09:01:49 GMT, Jaikiran Pai wrote:
> Can I please get a review of this change which proposes to address the build
> failure noted in https://bugs.openjdk.org/browse/JDK-8313274?
>
> The build failure is consistently reproducible with `--with-jobs=1`. Martin,
> in that JBS issue, has narrowed down the commit to the change in
> https://github.com/openjdk/jdk/pull/14561, starting which this failure is
> reproducible. The change in that PR, from what I understand, was meant to not
> require upgradable modules be a prerequisite for the `java.base-jmod` make
> target:
>
>> ... upgradeable modules, those shouldn't be on the prerequisites list for
>> java.base-jmod.
>
> The implementation of that change uses the `FindAllUpgradeableModules`
> function which as commented in the make files does:
>
>> #Upgradeable modules are those that are either defined as upgradeable or that
>> #require an upradeable module.
>
> The implementation of `FindAllUpgradeableModules` uses the
> `UPGRADEABLE_PLATFORM_MODULES` make variable which similarly comments:
>
>> #Modules that directly or indirectly requiring upgradeable modules
>> #should carefully be considered if it should be upgradeable or not.
>
> However, that set currently doesn't include the "indirectly requiring
> upgradable modules" and thus appears to be missing some of the modules that
> are considered upgradable.
>
> As a result, what seems to be happening is that the `java.base-jmod` make
> target now can (and does) end up requiring a particular target as a
> prerequisite, say `jdk.jdeps` (which uses `java.compiler` upgradable module),
> but doesn't add the `java.compiler-jmod` target as a prerequisite and thus
> ends up with that build failure:
>
>
> Creating java.base.jmod
> Error: Resolution failed: Module java.compiler not found, required by
> jdk.jdeps
>
>
> The commit in this PR proposes to fix this by updating the static set of
> `UPGRADEABLE_PLATFORM_MODULES` to include the indirect modules that require
> the upgradable modules. How these additional modules were derived is
> explained as a separate comment in this PR.
>
> The build succeeds with this change, both with `--with-jobs=1` and without
> the `--with-jobs` option (in which case on my setup it uses 8 jobs).
>
> I have triggered tier testing in the CI to make sure this doesn't cause any
> unexpected regressions.
>
> This change will require reviews from both the build team as well as the Java
> modules team - my knowledge of these areas is limited and I'm unsure if there
> are any additional considerations to take into account.
The additional set of modules included in the `UPGRADEABLE_PLATFORM_MODULES`
was determined programatically using the following code, which starts with the
"well known" upgradable modules (defined in JEP 261
https://openjdk.org/jeps/261) and then finds the dependents of these upgradable
modules:
import java.lang.module.ModuleDescriptor;
import java.util.HashSet;
import java.util.Set;
import java.lang.module.ModuleFinder;
import java.lang.module.ModuleReference;
import java.util.Collections;
import java.util.TreeSet;
import java.util.stream.Collectors;
public class UpgradableModules {
public static void main(final String[] args) throws Exception {
// upgradable as specified by the JEP https://openjdk.org/jeps/261
final Set specedUpgradable = new TreeSet<>(Set.of(
// "java.activation", // no longer present in mainline
"java.compiler",
// "java.corba", // no longer present in mainline
// "java.transaction", // no longer present in mainline
// "java.xml.bind", // no longer present in mainline
// "java.xml.ws" // no longer present in mainline
//"java.xml.ws.annotation", // no longer present in mainline
"jdk.internal.vm.compiler"
// "jdk.xml.bind" // no longer present in mainline
// "jdk.xml.ws" // no longer present in mainline
));
// all other modules that "require" these JEP specified upgradable
modules
final Set dependentUpgradable = new TreeSet<>();
final ModuleFinder sysModuleFinder = ModuleFinder.ofSystem();
final Set allModules = sysModuleFinder.findAll();
System.out.println("Using following modules as universe for finding
upgradable modules: "
+ allModules.stream()
.map((mr) -> mr.descriptor().name())
.collect(Collectors.toList()));
for (final String upgradableModName : specedUpgradable)
RFR: 8313274: Failure building java.base-jmod target
Can I please get a review of this change which proposes to address the build failure noted in https://bugs.openjdk.org/browse/JDK-8313274? The build failure is consistently reproducible with `--with-jobs=1`. Martin, in that JBS issue, has narrowed down the commit to the change in https://github.com/openjdk/jdk/pull/14561, starting which this failure is reproducible. The change in that PR, from what I understand, was meant to not require upgradable modules be a prerequisite for the `java.base-jmod` make target: > ... upgradeable modules, those shouldn't be on the prerequisites list for > java.base-jmod. The implementation of that change uses the `FindAllUpgradeableModules` function which as commented in the make files does: > #Upgradeable modules are those that are either defined as upgradeable or that > #require an upradeable module. The implementation of `FindAllUpgradeableModules` uses the `UPGRADEABLE_PLATFORM_MODULES` make variable which similarly comments: > #Modules that directly or indirectly requiring upgradeable modules > #should carefully be considered if it should be upgradeable or not. However, that set currently doesn't include the "indirectly requiring upgradable modules" and thus appears to be missing some of the modules that are considered upgradable. As a result, what seems to be happening is that the `java.base-jmod` make target now can (and does) end up requiring a particular target as a prerequisite, say `jdk.jdeps` (which uses `java.compiler` upgradable module), but doesn't add the `java.compiler-jmod` target as a prerequisite and thus ends up with that build failure: Creating java.base.jmod Error: Resolution failed: Module java.compiler not found, required by jdk.jdeps The commit in this PR proposes to fix this by updating the static set of `UPGRADEABLE_PLATFORM_MODULES` to include the indirect modules that require the upgradable modules. How these additional modules were derived is explained as a separate comment in this PR. The build succeeds with this change, both with `--with-jobs=1` and without the `--with-jobs` option (in which case on my setup it uses 8 jobs). I have triggered tier testing in the CI to make sure this doesn't cause any unexpected regressions. This change will require reviews from both the build team as well as the Java modules team - my knowledge of these areas is limited and I'm unsure if there are any additional considerations to take into account. - Commit messages: - update the UPGRADEABLE_PLATFORM_MODULES list Changes: https://git.openjdk.org/jdk/pull/15102/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15102=00 Issue: https://bugs.openjdk.org/browse/JDK-8313274 Stats: 8 lines in 1 file changed: 8 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15102.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15102/head:pull/15102 PR: https://git.openjdk.org/jdk/pull/15102
Re: RFR: 8312882: Update the CONTRIBUTING.md with pointers to lifecycle of a PR
On Thu, 29 Jun 2023 18:59:45 GMT, Jesse Glick wrote: > @jaikiran in > https://github.com/openjdk/jdk/pull/12871#issuecomment-1612310113 pointed me > to a resource I had not previously found. https://openjdk.org/contribute is > fine at a high level but still says things like > >> When your change is ready, send a message to the appropriate development >> list… > > which clearly predates Skara, and does not apparently link to anything more > current. Whereas this > https://github.com/openjdk/jdk/blob/master/doc/building.md is thorough and > discoverable, finding accurate guidelines for proposing pull requests was > tough. Hello Jesse, I've opened https://bugs.openjdk.org/browse/JDK-8312882 to track this change. Could you update the title of this PR to `8312882: Update the CONTRIBUTING.md with pointers to lifecycle of a PR` so that it triggers the official review process? - PR Comment: https://git.openjdk.org/jdk/pull/14716#issuecomment-1649626326
Integrated: 8227229: Deprecate the launcher -Xdebug/-debug flags that have not done anything since Java 6
On Tue, 18 Jul 2023 06:59:52 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to deprecate for > removal the `-Xdebug` option and `-debug` option of the `java` command? This > addresses https://bugs.openjdk.org/browse/JDK-8227229. > > As noted in the JBS issue this option is currently a no-op and has been there > only for backward compatible since even Java 8 days. This pull request has now been integrated. Changeset: 842d6329 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/842d6329cf5a3da8df7eddb195b5fcb7baadbdc3 Stats: 41 lines in 25 files changed: 0 ins; 9 del; 32 mod 8227229: Deprecate the launcher -Xdebug/-debug flags that have not done anything since Java 6 Reviewed-by: alanb, cjplummer, dholmes - PR: https://git.openjdk.org/jdk/pull/14918
Re: RFR: 8227229: Deprecate the launcher -Xdebug/-debug flags that have not done anything since Java 6 [v6]
> Can I please get a review of this change which proposes to deprecate for > removal the `-Xdebug` option and `-debug` option of the `java` command? This > addresses https://bugs.openjdk.org/browse/JDK-8227229. > > As noted in the JBS issue this option is currently a no-op and has been there > only for backward compatible since even Java 8 days. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: remove -Xdebug usage from SourceDebugExtension test - Changes: - all: https://git.openjdk.org/jdk/pull/14918/files - new: https://git.openjdk.org/jdk/pull/14918/files/b6c867b0..79c48cb0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14918=05 - incr: https://webrevs.openjdk.org/?repo=jdk=14918=04-05 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14918.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14918/head:pull/14918 PR: https://git.openjdk.org/jdk/pull/14918
Re: RFR: 8227229: Deprecate the launcher -Xdebug/-debug flags that have not done anything since Java 6 [v4]
On Tue, 18 Jul 2023 12:41:32 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to deprecate for >> removal the `-Xdebug` option and `-debug` option of the `java` command? >> This addresses https://bugs.openjdk.org/browse/JDK-8227229. >> >> As noted in the JBS issue this option is currently a no-op and has been >> there only for backward compatible since even Java 8 days. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Alan's suggestion for -Xdebug output Hello Alan, > I think ParseArguments (in libjli/java.c) could be changed so that it doesn't > translate -debug to -Xdebug, instead it can print a warning, like it does for > -Xfuture. The reason is -debug is a java launcher option, it's not known to > the VM and means that Arguments::parse_each_vm_init_arg doesn't need to > mention -debug when it warns about -Xdebug. I had initially considered that but had noticed that there's a small difference between the generic warning message "Warning: %s option is deprecated and may be removed in a future release." printed by the launcher and the one printed by the VM "OpenJDK 64-Bit Server VM warning: Option -Xdebug was deprecated in JDK 22 and will likely be removed in a future release." But I think that small difference in the warning messages is OK when considered against your stated reasoning that `-debug` isn't known to the VM. I've now updated the PR to implement your suggestion. - PR Comment: https://git.openjdk.org/jdk/pull/14918#issuecomment-1641532218
Re: RFR: 8227229: Deprecate the launcher -Xdebug/-debug flags that have not done anything since Java 6 [v5]
On Wed, 19 Jul 2023 07:05:51 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to deprecate for >> removal the `-Xdebug` option and `-debug` option of the `java` command? >> This addresses https://bugs.openjdk.org/browse/JDK-8227229. >> >> As noted in the JBS issue this option is currently a no-op and has been >> there only for backward compatible since even Java 8 days. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Alan's suggestion - report -debug usage as a warning right in the java.c > code Adding one of my previous comments again, because it's not easily visible among the bot generated comments in the GitHub UI: > I would like inputs on the > test/hotspot/jtreg/runtime/6294277/SourceDebugExtension.java test. That test > launches java passing it the -Xdebug option and was introduced as part of > https://bugs.openjdk.org/browse/JDK-6294277. -Xdebug has been a no-op for > many releases now. Is this test still relevant and if not, should I go ahead > and remove it as part of this change? - PR Comment: https://git.openjdk.org/jdk/pull/14918#issuecomment-1641534305
Re: RFR: 8227229: Deprecate the launcher -Xdebug/-debug flags that have not done anything since Java 6 [v5]
> Can I please get a review of this change which proposes to deprecate for > removal the `-Xdebug` option and `-debug` option of the `java` command? This > addresses https://bugs.openjdk.org/browse/JDK-8227229. > > As noted in the JBS issue this option is currently a no-op and has been there > only for backward compatible since even Java 8 days. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Alan's suggestion - report -debug usage as a warning right in the java.c code - Changes: - all: https://git.openjdk.org/jdk/pull/14918/files - new: https://git.openjdk.org/jdk/pull/14918/files/78498068..b6c867b0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14918=04 - incr: https://webrevs.openjdk.org/?repo=jdk=14918=03-04 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14918.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14918/head:pull/14918 PR: https://git.openjdk.org/jdk/pull/14918
Re: RFR: 8227229: Deprecate the launcher -Xdebug/-debug flags that have not done anything since Java 6 [v4]
> Can I please get a review of this change which proposes to deprecate for > removal the `-Xdebug` option and `-debug` option of the `java` command? This > addresses https://bugs.openjdk.org/browse/JDK-8227229. > > As noted in the JBS issue this option is currently a no-op and has been there > only for backward compatible since even Java 8 days. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Alan's suggestion for -Xdebug output - Changes: - all: https://git.openjdk.org/jdk/pull/14918/files - new: https://git.openjdk.org/jdk/pull/14918/files/e5472701..78498068 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14918=03 - incr: https://webrevs.openjdk.org/?repo=jdk=14918=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14918.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14918/head:pull/14918 PR: https://git.openjdk.org/jdk/pull/14918
Re: RFR: 8227229: Deprecate the launcher -Xdebug/-debug flags that have not done anything since Java 6 [v3]
On Tue, 18 Jul 2023 11:01:33 GMT, Alan Bateman wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> include -debug (alias) option in the deprecated log message > > src/java.base/share/classes/sun/launcher/resources/launcher.properties line > 137: > >> 135: \-Xcheck:jni perform additional checks for JNI functions\n\ >> 136: \-Xcompforces compilation of methods on first >> invocation\n\ >> 137: \-Xdebug deprecated and may be removed in a future >> release.\n\ > > This drops "does nothing" so no longer clear that the option doesn't do > anything. It could be changed to "does nothing; deprecated for removal in a > future release". Alternatively maybe it would be better to just remove it > from the -X output. Hello Alan, I updated the PR to use your suggested wording. I thought it's of no harm to let it be present in the -X output until we actually remove it (soon). - PR Review Comment: https://git.openjdk.org/jdk/pull/14918#discussion_r1266705837
Re: RFR: 8227229: Deprecate the launcher -Xdebug/-debug flags that have not done anything since Java 6 [v3]
On Tue, 18 Jul 2023 07:58:58 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to deprecate for >> removal the `-Xdebug` option and `-debug` option of the `java` command? >> This addresses https://bugs.openjdk.org/browse/JDK-8227229. >> >> As noted in the JBS issue this option is currently a no-op and has been >> there only for backward compatible since even Java 8 days. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > include -debug (alias) option in the deprecated log message Given that https://bugs.openjdk.org/browse/JDK-8227229 had more historical details about `-Xdebug` and `-debug`, I've updated this PR to link to that issue and also drafted CSR afresh https://bugs.openjdk.org/browse/JDK-8312220 - PR Comment: https://git.openjdk.org/jdk/pull/14918#issuecomment-1639725244
Re: RFR: 8227229: Deprecate the launcher -Xdebug/-debug flags that have not done anything since Java 6 [v3]
> Can I please get a review of this change which proposes to deprecate for > removal the `-Xdebug` option and `-debug` option of the `java` command? This > addresses https://bugs.openjdk.org/browse/JDK-8227229. > > As noted in the JBS issue this option is currently a no-op and has been there > only for backward compatible since even Java 8 days. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: include -debug (alias) option in the deprecated log message - Changes: - all: https://git.openjdk.org/jdk/pull/14918/files - new: https://git.openjdk.org/jdk/pull/14918/files/a8d472ad..e5472701 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14918=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14918=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14918.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14918/head:pull/14918 PR: https://git.openjdk.org/jdk/pull/14918
Re: RFR: 8312151: Deprecate for removal the -Xdebug option [v2]
> Can I please get a review of this change which proposes to deprecate for > removal the `-Xdebug` option of the `java` command? This addresses > https://bugs.openjdk.org/browse/JDK-8312151? > > As noted in the JBS issue this option is currently a no-op and has been there > only for backward compatible since even Java 8 days. > > A CSR has been drafted for this change > https://bugs.openjdk.org/browse/JDK-8312216 Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: update java man page for -Xdebug deprecation - Changes: - all: https://git.openjdk.org/jdk/pull/14918/files - new: https://git.openjdk.org/jdk/pull/14918/files/e165dc24..a8d472ad Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14918=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14918=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14918.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14918/head:pull/14918 PR: https://git.openjdk.org/jdk/pull/14918
RFR: 8312151: Deprecate for removal the -Xdebug option
Can I please get a review of this change which proposes to deprecate for removal the `-Xdebug` option of the `java` command? This addresses https://bugs.openjdk.org/browse/JDK-8312151? As noted in the JBS issue this option is currently a no-op and has been there only for backward compatible since even Java 8 days. A CSR has been drafted for this change https://bugs.openjdk.org/browse/JDK-8312216 - Commit messages: - 8312151: Deprecate for removal the -Xdebug option Changes: https://git.openjdk.org/jdk/pull/14918/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14918=00 Issue: https://bugs.openjdk.org/browse/JDK-8312151 Stats: 35 lines in 22 files changed: 0 ins; 8 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/14918.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14918/head:pull/14918 PR: https://git.openjdk.org/jdk/pull/14918
Re: RFR: 8312151: Deprecate for removal the -Xdebug option
On Tue, 18 Jul 2023 06:59:52 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to deprecate for > removal the `-Xdebug` option of the `java` command? This addresses > https://bugs.openjdk.org/browse/JDK-8312151? > > As noted in the JBS issue this option is currently a no-op and has been there > only for backward compatible since even Java 8 days. > > A CSR has been drafted for this change > https://bugs.openjdk.org/browse/JDK-8312216 I would like inputs on the `test/hotspot/jtreg/runtime/6294277/SourceDebugExtension.java` test. That test launches `java` passing it the `-Xdebug` option and was introduced as part of https://bugs.openjdk.org/browse/JDK-6294277. `-Xdebug` has been a no-op for many releases now. Is this test still relevant and if not, should I go ahead and remove it as part of this change? - PR Comment: https://git.openjdk.org/jdk/pull/14918#issuecomment-1639615535
Integrated: 8312072: Deprecate for removal the -Xnoagent option
On Fri, 14 Jul 2023 07:34:03 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to deprecate > `-Xnoagent` option of the `java` command? This addresses > https://bugs.openjdk.org/browse/JDK-8312072. > > As noted in the JBS issue, this option currently is a no-op and provides no > functionality. The commit in this PR logs a deprecation warning if this > option is used when launching `java`. > > Additionally, this PR also cleans up one such usage that I found in the JDK > code. > > `tier1`, `tier2` and `tier3` tests passed with this change. A CSR has been > proposed for this deprecation https://bugs.openjdk.org/browse/JDK-8312073. This pull request has now been integrated. Changeset: 8ec136e6 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/8ec136e6f0fa684255274181d09c86251ef5428f Stats: 23 lines in 12 files changed: 0 ins; 0 del; 23 mod 8312072: Deprecate for removal the -Xnoagent option Reviewed-by: alanb, dholmes, cjplummer - PR: https://git.openjdk.org/jdk/pull/14882
Re: RFR: 8312072: Deprecate for removal the -Xnoagent option [v3]
On Sun, 16 Jul 2023 06:34:46 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to deprecate >> `-Xnoagent` option of the `java` command? This addresses >> https://bugs.openjdk.org/browse/JDK-8312072. >> >> As noted in the JBS issue, this option currently is a no-op and provides no >> functionality. The commit in this PR logs a deprecation warning if this >> option is used when launching `java`. >> >> Additionally, this PR also cleans up one such usage that I found in the JDK >> code. >> >> `tier1`, `tier2` and `tier3` tests passed with this change. A CSR has been >> proposed for this deprecation https://bugs.openjdk.org/browse/JDK-8312073. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Remove -Djava.compiler=none Thank you everyone for the reviews. I think the current Reviews cover the necessary areas of hotspot-runtime and serviceability. So I'll go ahead with the integration later in the day today (3-4 hours from now). - PR Comment: https://git.openjdk.org/jdk/pull/14882#issuecomment-1639146443
Re: RFR: 8312072: Deprecate for removal the -Xnoagent option [v3]
On Sun, 16 Jul 2023 06:34:46 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to deprecate >> `-Xnoagent` option of the `java` command? This addresses >> https://bugs.openjdk.org/browse/JDK-8312072. >> >> As noted in the JBS issue, this option currently is a no-op and provides no >> functionality. The commit in this PR logs a deprecation warning if this >> option is used when launching `java`. >> >> Additionally, this PR also cleans up one such usage that I found in the JDK >> code. >> >> `tier1`, `tier2` and `tier3` tests passed with this change. A CSR has been >> proposed for this deprecation https://bugs.openjdk.org/browse/JDK-8312073. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Remove -Djava.compiler=none Hello Chris, > I think `-Xdebug` is also a no-op now. Yes, I'm preparing a separate JBS and PR to address that one. - PR Comment: https://git.openjdk.org/jdk/pull/14882#issuecomment-1637265830
Re: RFR: 8312072: Deprecate for removal the -Xnoagent option [v3]
> Can I please get a review of this change which proposes to deprecate > `-Xnoagent` option of the `java` command? This addresses > https://bugs.openjdk.org/browse/JDK-8312072. > > As noted in the JBS issue, this option currently is a no-op and provides no > functionality. The commit in this PR logs a deprecation warning if this > option is used when launching `java`. > > Additionally, this PR also cleans up one such usage that I found in the JDK > code. > > `tier1`, `tier2` and `tier3` tests passed with this change. A CSR has been > proposed for this deprecation https://bugs.openjdk.org/browse/JDK-8312073. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Remove -Djava.compiler=none - Changes: - all: https://git.openjdk.org/jdk/pull/14882/files - new: https://git.openjdk.org/jdk/pull/14882/files/71df3079..8254d8d8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14882=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14882=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14882.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14882/head:pull/14882 PR: https://git.openjdk.org/jdk/pull/14882
Re: RFR: 8312072: Deprecate for removal the -Xnoagent option
On Fri, 14 Jul 2023 08:52:06 GMT, Jaikiran Pai wrote: > We should be able to remove it from our tests too, looks like there are some > left overs in test/hotspot/jtreg/vmTestbase/nsk/jdi. I've now updated the PR to remove references to `-Xnoagent` from these tests. There's a `TestNullTerminatedFlags` which uses this option. I haven't changed that test given the nature of that test and the fact that this `-Xnoagent` is currently still an accepted command line option. - PR Comment: https://git.openjdk.org/jdk/pull/14882#issuecomment-1636596306
Re: RFR: 8312072: Deprecate for removal the -Xnoagent option [v2]
> Can I please get a review of this change which proposes to deprecate > `-Xnoagent` option of the `java` command? This addresses > https://bugs.openjdk.org/browse/JDK-8312072. > > As noted in the JBS issue, this option currently is a no-op and provides no > functionality. The commit in this PR logs a deprecation warning if this > option is used when launching `java`. > > Additionally, this PR also cleans up one such usage that I found in the JDK > code. > > `tier1`, `tier2` and `tier3` tests passed with this change. A CSR has been > proposed for this deprecation https://bugs.openjdk.org/browse/JDK-8312073. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Remove references to -Xnoagent from tests - Changes: - all: https://git.openjdk.org/jdk/pull/14882/files - new: https://git.openjdk.org/jdk/pull/14882/files/499e7061..71df3079 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14882=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14882=00-01 Stats: 20 lines in 10 files changed: 0 ins; 0 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/14882.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14882/head:pull/14882 PR: https://git.openjdk.org/jdk/pull/14882
Re: RFR: 8312072: Deprecate for removal the -Xnoagent option
On Fri, 14 Jul 2023 08:19:54 GMT, Alan Bateman wrote:
> We should be able to remove it from our tests too, looks like there are some
> left overs in test/hotspot/jtreg/vmTestbase/nsk/jdi.
I had noticed those but wasn't sure if they should be included in this change.
I'll remove them and run some tests and see how it goes.
> The changes makes me wonder if we should do the same for -Xdebug.
I had a quick look now and it looks like it's just being set and then ignored
if `-Xdebug` is passed. There's an accessor method to get the set value but I
don't see it being used anywhere. Plus, the comment where `-Xdebug` is handled
states:
> } else if (match_option(option, "-Xdebug")) {
> // note this flag has been used, then ignore
Would you want me to include the deprecation (for removal) for that option in
this same PR or should we do that separately?
-
PR Comment: https://git.openjdk.org/jdk/pull/14882#issuecomment-1635527599
RFR: 8312072: Deprecate for removal the -Xnoagent option
Can I please get a review of this change which proposes to deprecate `-Xnoagent` option of the `java` command? This addresses https://bugs.openjdk.org/browse/JDK-8312072. As noted in the JBS issue, this option currently is a no-op and provides no functionality. The commit in this PR logs a deprecation warning if this option is used when launching `java`. Additionally, this PR also cleans up one such usage that I found in the JDK code. `tier1`, `tier2` and `tier3` tests passed with this change. A CSR has been proposed for this deprecation https://bugs.openjdk.org/browse/JDK-8312073. - Commit messages: - update copyright year - Remove reference from netbeans build.xml - deprecate for removal -Xnoagent Changes: https://git.openjdk.org/jdk/pull/14882/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14882=00 Issue: https://bugs.openjdk.org/browse/JDK-8312072 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14882.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14882/head:pull/14882 PR: https://git.openjdk.org/jdk/pull/14882
Re: Run failed: OpenJDK GHA Sanity Checks - 8306040
Hello Vyom, The title of the PR needs to be updated. It should be: 8306040: HttpResponseInputStream.available() returns 1 on empty stream -Jaikiran On 10/07/23 4:38 pm, Vyom Tiwari wrote: Hi, I am trying to submit a PR(https://github.com/openjdk/jdk/pull/14810). I am getting the following notifications from github Run failed: OpenJDK GHA Sanity Checks - 8306040 (5e6cb9c) My PR is getting stuck at this point not moving to ready state so that it can be reviewed. Can you please help me to resolve this issue? Thanks, Vyom
Re: Terminating failing tests on Windows
Hello Chen, I don't use Windows and am not familiar with cygwin either, but I know of others who use Windows for their builds and jtreg testing. Perhaps they can help if you add some more details like what exact command you use to launch the tests and which specific tests are you running. Also when you say directories are being locked, what kind of lock do you mean and which directories (are those under the JTwork)? Is it the usual "this file is being used by some other process" kind of error that you notice when this happens? -Jaikiran On 02/07/23 7:23 am, - wrote: Hello, I am using the JDK's make build system to run JDK's tests on cygwin; however, the tests often lock the directories of failing runs, blocking Intellij Idea IDE and subsequent runs, and I cannot figure out what processes are doing so. What processes should I shut down in Windows's task manager besides bash.exe so I can proceed with reruns as quickly as possible after fixing problems? Chen Liang
Re: [jdk21] RFR: 8310259: Pin msys2/setup-msys2 github action to a specific commit
On Mon, 19 Jun 2023 11:39:21 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which backports the fixes for > https://bugs.openjdk.org/browse/JDK-8310259? > > This wasn't a clean backport because this also brings in > https://bugs.openjdk.org/browse/JDK-8309934 which in theory isn't a must-have > change, but I think it's good to have these changes match with what's in > mainline. > > With this commit in this PR, the windows github actions jobs that are failing > in this jdk21 repo, should now continue to pass. Thank you Christian and Christoph for the reviews. - PR Comment: https://git.openjdk.org/jdk21/pull/31#issuecomment-1597927095
[jdk21] Integrated: 8310259: Pin msys2/setup-msys2 github action to a specific commit
On Mon, 19 Jun 2023 11:39:21 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which backports the fixes for > https://bugs.openjdk.org/browse/JDK-8310259? > > This wasn't a clean backport because this also brings in > https://bugs.openjdk.org/browse/JDK-8309934 which in theory isn't a must-have > change, but I think it's good to have these changes match with what's in > mainline. > > With this commit in this PR, the windows github actions jobs that are failing > in this jdk21 repo, should now continue to pass. This pull request has now been integrated. Changeset: 08965e64 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk21/commit/08965e646e3aa8caabb5802331c637fed4db1197 Stats: 4 lines in 2 files changed: 1 ins; 0 del; 3 mod 8310259: Pin msys2/setup-msys2 github action to a specific commit 8309934: Update GitHub Actions to use JDK 17 for building jtreg Reviewed-by: cstein, clanger Backport-of: 959a61fdd483c9523764b9ba0972f59ca06db0ee - PR: https://git.openjdk.org/jdk21/pull/31
RFR: 8310306: Pin msys2/setup-msys2 github action to a specific commit
Can I please get a review of this change which backports the fixes for https://bugs.openjdk.org/browse/JDK-8310259? This wasn't a clean backport because this also brings in https://bugs.openjdk.org/browse/JDK-8309934 which in theory isn't a must-have change, but I think it's good to have these changes match with what's in mainline. With this commit in this PR, the windows github actions jobs that are failing in this jdk21 repo, should now continue to pass. - Commit messages: - 8310306: Pin msys2/setup-msys2 github action to a specific commit Changes: https://git.openjdk.org/jdk21/pull/31/files Webrev: https://webrevs.openjdk.org/?repo=jdk21=31=00 Issue: https://bugs.openjdk.org/browse/JDK-8310306 Stats: 4 lines in 2 files changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk21/pull/31.diff Fetch: git fetch https://git.openjdk.org/jdk21.git pull/31/head:pull/31 PR: https://git.openjdk.org/jdk21/pull/31
Re: RFR: 8310259: Pin msys2/setup-msys2 github action to a specific commit
On Fri, 16 Jun 2023 11:08:41 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to address the recent > github actions failures? > > As noted in https://bugs.openjdk.org/browse/JDK-8310259 the commit in this PR > pins the `msys2/setup-msys2` to a previous release (that works) and also > reverts the change that was done in > https://github.com/openjdk/jdk/pull/14507, for reasons discussed in that PR. Thank you Christian and Thomas for the reviews. P.S: The github jobs that have failed in this current PR are unrelated test failures. - PR Comment: https://git.openjdk.org/jdk/pull/14516#issuecomment-1595637977
Integrated: 8310259: Pin msys2/setup-msys2 github action to a specific commit
On Fri, 16 Jun 2023 11:08:41 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to address the recent > github actions failures? > > As noted in https://bugs.openjdk.org/browse/JDK-8310259 the commit in this PR > pins the `msys2/setup-msys2` to a previous release (that works) and also > reverts the change that was done in > https://github.com/openjdk/jdk/pull/14507, for reasons discussed in that PR. This pull request has now been integrated. Changeset: 959a61fd Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/959a61fdd483c9523764b9ba0972f59ca06db0ee Stats: 3 lines in 2 files changed: 1 ins; 0 del; 2 mod 8310259: Pin msys2/setup-msys2 github action to a specific commit Reviewed-by: cstein, stuefe - PR: https://git.openjdk.org/jdk/pull/14516
RFR: 8310259: Pin msys2/setup-msys2 github action to a specific commit
Can I please get a review of this change which proposes to address the recent github actions failures? As noted in https://bugs.openjdk.org/browse/JDK-8310259 the commit in this PR pins the `msys2/setup-msys2` to a previous release (that works) and also reverts the change that was done in https://github.com/openjdk/jdk/pull/14507, for reasons discussed in that PR. - Commit messages: - add comment - use specific (working) commit of msys2 github action - Revert "8310183: Update GitHub Actions to use boot JDK for building jtreg" Changes: https://git.openjdk.org/jdk/pull/14516/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14516=00 Issue: https://bugs.openjdk.org/browse/JDK-8310259 Stats: 3 lines in 2 files changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14516.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14516/head:pull/14516 PR: https://git.openjdk.org/jdk/pull/14516
Re: RFR: 8310183: Update GitHub Actions to use boot JDK for building jtreg
On Fri, 16 Jun 2023 14:51:21 GMT, Jonathan Gibbons wrote: > To those investigating the msys issue, do we know why and where `CLASSPATH` > is being used? In general, that's a deprecated style of use. The make files in jtreg build have a bunch of `CLASSPATH` usages while launching `javac`. For example here: https://github.com/openjdk/jtreg/blob/master/make/jtreg.gmk#L46 https://github.com/openjdk/jtreg/blob/master/make/jtdiff.gmk#L36 I'll experiment today to move away from `CLASSPATH` environment variable and use javac's classpath option instead. - PR Comment: https://git.openjdk.org/jdk/pull/14507#issuecomment-1595570158
Re: RFR: 8310183: Update GitHub Actions to use boot JDK for building jtreg
On Fri, 16 Jun 2023 06:05:16 GMT, Christian Stein wrote: > Please review this change to use the boot JDK for building jtreg when running > on GitHub Actions. > > This is a best-effort follow-up change to > - #14448 > which didn't have the desired results - the `Bad address` error does still > appear with using the pre-installed JDKs 11 and 17. > > Tests using the boot JDK for building jtreg seem to be successful and more > stable. I had a bit more look at this today. This most certainly is some issue in the `msys2` package that gets installed on these VMs. And this appears to be some recent regression. I went back and looked at some of my personal jobs which were passing without issues and I can see that those were on `7efe20baefed56359985e327d329042cde2434ff` commit of `msys2/setup-msys2@v2` github action. It appears that there has been a recent release of this action around 3 weeks back and the GitHub actions jobs are now bringing in `cf96e00c0aab3788743aaf63b64146f0d383cee9` https://github.com/msys2/setup-msys2/commit/cf96e00c0aab3788743aaf63b64146f0d383cee9 The place where this fails when building jtreg, is when launching `javac` with a `CLASSPATH` environment variable set. There's no obvious issues in that variable value - a few commands before, similar values for that variable have worked fine with `javac`. This will need a bit more investigation to understand why `make` fails to launch `javac` with this version of `msys2`. For now though, I pinned the `msys2/setup-msys2` action in our JDK's github action's workflow file to the commit that was working and that got my builds past this issue. Without that change I can consistently reproduce the issue every time. I have created a branch which that change and reverted the commit in this PR to see if the jobs run fine https://github.com/openjdk/jdk/pull/14516. It's currently in progress https://github.com/jaikiran/jdk/actions/runs/5288890247. If that works, then perhaps we should just use that pinned version of `msys2/setup-msys2`, until we report and have this issue fixed in that package? - PR Comment: https://git.openjdk.org/jdk/pull/14507#issuecomment-1594515114
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v8]
On Mon, 1 May 2023 13:06:24 GMT, Jim Laskey wrote:
>> Add flexible main methods and anonymous main classes to the Java language.
>
> Jim Laskey has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Anonymous main classes renamed to unnamed classes
> - Add test
test/jdk/tools/launcher/InstanceMainTest.java line 33:
> 31: public class InstanceMainTest extends TestHelper {
> 32:
> 33: @Test
Does this compile? I don't see imports for this annotation and this is launched
as a `@run main `
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1185930904
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v8]
On Mon, 1 May 2023 13:06:24 GMT, Jim Laskey wrote:
>> Add flexible main methods and anonymous main classes to the Java language.
>
> Jim Laskey has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Anonymous main classes renamed to unnamed classes
> - Add test
src/jdk.compiler/share/classes/com/sun/tools/javac/launcher/Main.java line 457:
> 455: if (isStatic) {
> 456: if (noArgs) {
> 457: mainMethod.invoke(appClass);
Given that `Method.invoke(...)` on a static method ignores the `obj` param
that's passed to it, perhaps pass `null`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1185924903
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v8]
On Mon, 1 May 2023 13:06:24 GMT, Jim Laskey wrote:
>> Add flexible main methods and anonymous main classes to the Java language.
>
> Jim Laskey has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Anonymous main classes renamed to unnamed classes
> - Add test
src/jdk.compiler/share/classes/com/sun/tools/javac/launcher/Main.java line 419:
> 417: private static boolean isStatic(Method method) {
> 418: return method != null &&
> Modifier.isStatic(method.getModifiers());
> 419: }
It looks like these new methods aren't being used.
src/jdk.compiler/share/classes/com/sun/tools/javac/launcher/Main.java line 440:
> 438: int mods = mainMethod.getModifiers();
> 439: boolean isStatic = Modifier.isStatic(mods);
> 440: boolean isPublic = Modifier.isStatic(mods);
This looks like a typo. This should have been `Modifier.isPublic(mods);`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1185918190
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1185917271
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v8]
On Mon, 1 May 2023 13:06:24 GMT, Jim Laskey wrote: >> Add flexible main methods and anonymous main classes to the Java language. > > Jim Laskey has updated the pull request incrementally with two additional > commits since the last revision: > > - Anonymous main classes renamed to unnamed classes > - Add test src/java.base/share/classes/sun/launcher/LauncherHelper.java line 893: > 891: * findMainMethod (above) will choose the correct method, based > 892: * on its name and parameter type, however, we still have to > 893: * ensure that the method is static (non-preview) and returns a > void. I think it's probably going to be easier to read and maintain if we moved these checks for `static` and `void` return type into the `MainMethodFinder.findMainMethod(...)` itself. What I mean is once we return from the `findMainMethod(...)`, I think the callers, like this code here, shouldn't have to do additional checks to know if this main method is valid and can be used. - PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1185908021
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v8]
On Mon, 1 May 2023 13:06:24 GMT, Jim Laskey wrote:
>> Add flexible main methods and anonymous main classes to the Java language.
>
> Jim Laskey has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Anonymous main classes renamed to unnamed classes
> - Add test
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 872:
> 870: Method mainMethod = null;
> 871: try {
> 872: mainMethod = MainMethodFinder.findMainMethod(mainClass);
The `MainMethodFinder.findMainMethod(...)` throws a `NoSuchMethodException`
when it can't find the regular `public static void main(String[])` or, when
preview is enabled, any other eligible main methods. That will now mean that
the next line here which catches a `NoSuchMethodException` can potentially end
up aborting with a (very confusing) error message about JavaFX application. We
should perhaps change that catch block to something like:
} catch (NoSuchMethodException nsme) {
if (!PreviewFeatures.isEnabled()) {
// invalid main or not FX application, abort with an error
abort(null, "java.launcher.cls.error4", mainClass.getName(),
JAVAFX_APPLICATION_CLASS_NAME);
} else {
// abort with something more clear error?
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1185905158
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v8]
On Mon, 1 May 2023 13:06:24 GMT, Jim Laskey wrote:
>> Add flexible main methods and anonymous main classes to the Java language.
>
> Jim Laskey has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Anonymous main classes renamed to unnamed classes
> - Add test
src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 70:
> 68: correctArgs(method) &&
> 69: // Only statics in the declaring class
> 70: (!isStatic(method) || declc == refc)
Should this also exclude `abstract` and `native` methods named `main`?
src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 134:
> 132:
> 133: /**
> 134: * {@return priority main method or null if none found}
I think this javadoc is perhaps outdated? The current implementation of this
method throws a `NoSuchMethodException` when it can't find any eligible main
method.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1185896198
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1185898281
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v8]
On Mon, 1 May 2023 13:06:24 GMT, Jim Laskey wrote: >> Add flexible main methods and anonymous main classes to the Java language. > > Jim Laskey has updated the pull request incrementally with two additional > commits since the last revision: > > - Anonymous main classes renamed to unnamed classes > - Add test src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 152: > 150: > 151: List mains = new ArrayList<>(); > 152: gatherMains(mainClass, mainClass, mains); The `try` block above is there to find `public static void main(String[])` in the launched class. When it's not found, then we gather the potential main methods (as listed in the JEP). The implementation of `gatherMains(...)`, in its current form starts gathering the main methods from `refc.getSuperclass()`, where `refc` in this case is the `mainClass`, which is the launched class. So if I'm reading this correctly, then I think it's going to completely skip the launched class for looking any potential main methods and instead start looking for them in the launched class' super hierarchy. src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 158: > 156: } > 157: > 158: mains.sort(MainMethodFinder::compareMethods); Perhaps skip the call to `sort` and just return the found main method if `mains.size() == 1`? - PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1185890136 PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1185892782
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v8]
On Mon, 1 May 2023 13:06:24 GMT, Jim Laskey wrote:
>> Add flexible main methods and anonymous main classes to the Java language.
>
> Jim Laskey has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Anonymous main classes renamed to unnamed classes
> - Add test
src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 139:
> 137: public static Method findMainMethod(Class mainClass) throws
> NoSuchMethodException {
> 138: try {
> 139: Method mainMethod = mainClass.getMethod("main",
> String[].class);
Hello Jim, I think this specific line is trying to find a `public static void
main(String[])` method from the launched class. In the current form of this
implementation, this has the potential of returning a non-static `public void
main(String[])` from here. I think a `isStatic(mainMethod)` would be needed
here before returning this method as the found method.
src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 142:
> 140:
> 141: if (mainMethod.getDeclaringClass() != mainClass) {
> 142: System.err.println("WARNING: static main in super class
> will be deprecated.");
Similarly, this warning would have to be logged only if the method is `static`.
Furthermore, do you think we should include the declaring class in the log
message to provide some context on what's causing this warning? Something like:
> WARNING: static main(String[]) in super class foo.bar.Parent will be
> deprecated.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1185882851
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1185885497
Re: RFR: 8303703: Add support of execution tests using virtual thread factory jtreg plugin [v4]
On Fri, 21 Apr 2023 02:05:39 GMT, Leonid Mesnik wrote: >> The plugin which support execution of test's main method in separate virtual >> thread is added. >> The plugin is built as a part of test image and might be used in testing by >> adding JTREG_TEST_THREAD_FACTORY=Virtual option. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > doc updated. Thank you Leonid. Looks good to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13432#pullrequestreview-1395018990
Re: RFR: 8303703: Add support of execution tests using virtual thread factory jtreg plugin
On Tue, 18 Apr 2023 05:58:08 GMT, Jaikiran Pai wrote: >> The plugin which support execution of test's main method in separate virtual >> thread is added. >> The plugin is built as a part of test image and might be used in testing by >> adding JTREG_TEST_THREAD_FACTORY=Virtual option. > > I use `jtreg` standalone (outside of `make`) a lot of times to run tests. If > I understand the build changes correctly, `make images` will build the > necessary jars for the virtual thread factory class (does it require > `--with-jtreg` option to have been used during `configure`?). And then if I > want to use this `Virtual` thread factory, I can just do: > > > jtreg -testThreadFactoryPath: -testThreadFactory:... > > Is that right? > @jaikiran, Have I answered to all your questions? Thank you Lenoid for those details and updates to this PR. The latest changes look OK to me. I've added an inline comment about some typos in the `testing.md` documentation. Other than that this looks fine to me. - PR Comment: https://git.openjdk.org/jdk/pull/13432#issuecomment-1517144209
Re: RFR: 8303703: Add support of execution tests using virtual thread factory jtreg plugin [v3]
On Tue, 18 Apr 2023 15:19:29 GMT, Leonid Mesnik wrote: >> The plugin which support execution of test's main method in separate virtual >> thread is added. >> The plugin is built as a part of test image and might be used in testing by >> adding JTREG_TEST_THREAD_FACTORY=Virtual option. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > fixed doc doc/testing.md line 385: > 383: Sets the `-testThreadFactory` for JTReg. Is should be the name of class > implementing ThreadFactory > 384: and located in `test/jtreg_test_thread_factory/'. The plugins are built > as a part of test image. > 385: Currently, the only `Virtual` factory which executes test method main in > virtual thread is implmented. Hello Leonid, there are some typos in this section. Perhaps consider changing it to: > > Sets the `-testThreadFactory` for JTReg. It should be the fully qualified > classname of a class which implements `java.util.concurrent.ThreadFactory`. One such implementation class, named `Virtual`, is currently part of the JDK build in the `test/jtreg_test_thread_factory/` directory. This class gets compiled during the test image build. The implementation of the `Virtual` class creates a new virtual thread for executing each test class. Does that sound accurate? - PR Review Comment: https://git.openjdk.org/jdk/pull/13432#discussion_r1173228976
Re: RFR: 8303703: Add support of execution tests using virtual thread factory jtreg plugin
On Tue, 11 Apr 2023 18:17:06 GMT, Leonid Mesnik wrote: > The plugin which support execution of test's main method in separate virtual > thread is added. > The plugin is built as a part of test image and might be used in testing by > adding JTREG_TEST_THREAD_FACTORY=Virtual option. I use `jtreg` standalone (outside of `make`) a lot of times to run tests. If I understand the build changes correctly, `make images` will build the necessary jars for the virtual thread factory class (does it require `--with-jtreg` option to have been used during `configure`?). And then if I want to use this `Virtual` thread factory, I can just do: jtreg -testThreadFactoryPath: -testThreadFactory:... Is that right? - PR Comment: https://git.openjdk.org/jdk/pull/13432#issuecomment-1512479588
Re: RFR: 8303703: Add support of execution tests using virtual thread factory jtreg plugin
On Tue, 11 Apr 2023 18:17:06 GMT, Leonid Mesnik wrote: > The plugin which support execution of test's main method in separate virtual > thread is added. > The plugin is built as a part of test image and might be used in testing by > adding JTREG_TEST_THREAD_FACTORY=Virtual option. >From what I understand, this `Virtual` thread factory is similar to >`java.util.concurrent.Executors.newVirtualThreadPerTaskExecutor()` which >creates a new virtual thread whenever a thread needs to be created for a task. >We use this `Virtual` class to plug into jtreg. In context of jtreg, as far as I can see, these threads will only get used when running the tests in agent vm mode. So the agent VM which is running as a separate process would be the one which will be launching these virtual threads. Did I understand it correctly? Furthermore, is this applicable for `othervm` mode in any way? - PR Comment: https://git.openjdk.org/jdk/pull/13432#issuecomment-1512468733
Re: RFR: 8303703: Add support of execution tests using virtual thread factory jtreg plugin
On Tue, 11 Apr 2023 18:17:06 GMT, Leonid Mesnik wrote: > The plugin which support execution of test's main method in separate virtual > thread is added. > The plugin is built as a part of test image and might be used in testing by > adding JTREG_TEST_THREAD_FACTORY=Virtual option. doc/testing.md line 384: > 382: > 383: Sets the `-testThreadFactory` for JTReg. The source code of all plugins > is located > 384: in `test/jtreg_test_thread_factory/'. The plugins are built as a part of > test image. I think this should probably even include a brief note about what possible values, in context of the JDK build, are applicable. For example the value `Virtual` and what that value means. - PR Review Comment: https://git.openjdk.org/jdk/pull/13432#discussion_r1169510591
Re: RFR: 8303703: Add support of execution tests using virtual thread factory jtreg plugin
On Tue, 11 Apr 2023 18:17:06 GMT, Leonid Mesnik wrote:
> The plugin which support execution of test's main method in separate virtual
> thread is added.
> The plugin is built as a part of test image and might be used in testing by
> adding JTREG_TEST_THREAD_FACTORY=Virtual option.
test/jtreg_test_thread_factory/src/share/classes/Virtual.java line 8:
> 6: * under the terms of the GNU General Public License version 2 only, as
> 7: * published by the Free Software Foundation. Oracle designates this
> 8: * particular file as subject to the "Classpath" exception as provided
Hello Leonid, is this file expected to have a Classpath exception or should it
have the same license header that we use for test files?
test/jtreg_test_thread_factory/src/share/classes/Virtual.java line 35:
> 33: System.setProperty("main.wrapper", "Virtual");
> 34: } catch (Throwable t) {
> 35: // might be thrown by security manager
Since tests can be launched in presence of a security manager, would that mean
that when security manager is enabled, the `main.wrapper` will not get set?
Would that have any impact on the functioning of the test?
test/jtreg_test_thread_factory/src/share/classes/Virtual.java line 46:
> 44: public Thread newThread(Runnable task) {
> 45: try {
> 46: return Thread.ofVirtual().factory().newThread(task);
As far as I can see, none of these method calls throw a checked exception.
Should we perhaps remove this try/catch block and let any runtime exception
that gets thrown be propagated as-is instead of wrapping it in a
`RuntimException`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13432#discussion_r1169503122
PR Review Comment: https://git.openjdk.org/jdk/pull/13432#discussion_r1169504400
PR Review Comment: https://git.openjdk.org/jdk/pull/13432#discussion_r1169505814
Re: RFR: 8305089: Implement missing socket options on AIX [v5]
On Fri, 14 Apr 2023 12:32:32 GMT, Varada M wrote: >> Breaking this into two parts : >> >> 1. Implementing socket options for AIX >> 2. DontFragmentTest failure >> >> - Implementing socket options for AIX : >> >> Unlike the linux, windows and macOS, AIX uses the default implementation for >> socket options such as ipDontFragmentSupported(), >> keepAliveOptionsSupported(), setTcpkeepAliveProbes, getTcpkeepAliveProbes, >> setQuickAck, getQuickAck and more, where it either returns false or >> exception. These options can be implemented on AIX with the supported flags >> like SO_PEERID, TCP_NODELAYACK is the equivalent AIX option for TCP_QUICKACK >> and IPPROTO_TCP, IP_DONTFRAG. >> >> - DontFragment test failure : >> >> DontFragmentTest.java fails with a runtime exception : “IP_DONTFRAGMENT >> should be supported” because the supportOptions doesn’t contain >> IP_DONTFRAGMENT flag. >> >> Reported Issue : [JDK-8305089](https://bugs.openjdk.org/browse/JDK-8305089) > > Varada M has updated the pull request incrementally with one additional > commit since the last revision: > > added SO_PEERID failed These changes shouldn't cause any regressions, but just to be sure, I've triggered a CI run with these changes. I can sponsor this change once the results are available. - PR Comment: https://git.openjdk.org/jdk/pull/13240#issuecomment-1510722415
Re: RFR: 8305089: Implement missing socket options on AIX [v5]
On Fri, 14 Apr 2023 12:32:32 GMT, Varada M wrote: >> Breaking this into two parts : >> >> 1. Implementing socket options for AIX >> 2. DontFragmentTest failure >> >> - Implementing socket options for AIX : >> >> Unlike the linux, windows and macOS, AIX uses the default implementation for >> socket options such as ipDontFragmentSupported(), >> keepAliveOptionsSupported(), setTcpkeepAliveProbes, getTcpkeepAliveProbes, >> setQuickAck, getQuickAck and more, where it either returns false or >> exception. These options can be implemented on AIX with the supported flags >> like SO_PEERID, TCP_NODELAYACK is the equivalent AIX option for TCP_QUICKACK >> and IPPROTO_TCP, IP_DONTFRAG. >> >> - DontFragment test failure : >> >> DontFragmentTest.java fails with a runtime exception : “IP_DONTFRAGMENT >> should be supported” because the supportOptions doesn’t contain >> IP_DONTFRAGMENT flag. >> >> Reported Issue : [JDK-8305089](https://bugs.openjdk.org/browse/JDK-8305089) > > Varada M has updated the pull request incrementally with one additional > commit since the last revision: > > added SO_PEERID failed Hello Varada, you can go ahead and issue the /integrate command. - PR Comment: https://git.openjdk.org/jdk/pull/13240#issuecomment-1510714473
Re: RFR: 8305089: Implement missing socket options on AIX [v4]
On Fri, 14 Apr 2023 11:33:13 GMT, Varada M wrote: >> I'm curious, what is the rationale? >> By comparison the Linux version has: >> >> handleError(env, rv, "get SO_PEERCRED failed"); > > On linux it is implemented from > [attachListener_linux.cpp](https://github.com/openjdk/jdk/blob/master/src/hotspot/os/linux/attachListener_linux.cpp#L361) > and on aix it is from > [attachListener_aix.cpp](https://github.com/openjdk/jdk/blob/master/src/hotspot/os/aix/attachListener_aix.cpp#L392) > In both the cases when rv == -1 , it fails to get this socket option. Hello Varada, so the Linux version uses `SO_PEERCRED` socket option and when it fails, the error message says "get SO_PEERCRED failed". The AIX version uses `SO_PEERID` and if it fails, the current proposed form in this PR will report the error message as "get failed" which wouldn't be too informative. I'm guessing that the reason you didn't include the socket option in the error message is perhaps because the native socket option (on AIX) is `SO_PEERID` but this method is named `getSoPeerCred`. So it might be confusing to report `SO_PEERID` in the error message. Maybe you could change the message to just "getSoPeerCred failed" instead? - PR Review Comment: https://git.openjdk.org/jdk/pull/13240#discussion_r1166743891
Re: RFR: 8305089: Implement missing socket options on AIX [v4]
On Fri, 14 Apr 2023 07:59:49 GMT, Varada M wrote: >> Breaking this into two parts : >> >> 1. Implementing socket options for AIX >> 2. DontFragmentTest failure >> >> - Implementing socket options for AIX : >> >> Unlike the linux, windows and macOS, AIX uses the default implementation for >> socket options such as ipDontFragmentSupported(), >> keepAliveOptionsSupported(), setTcpkeepAliveProbes, getTcpkeepAliveProbes, >> setQuickAck, getQuickAck and more, where it either returns false or >> exception. These options can be implemented on AIX with the supported flags >> like SO_PEERID, TCP_NODELAYACK is the equivalent AIX option for TCP_QUICKACK >> and IPPROTO_TCP, IP_DONTFRAG. >> >> - DontFragment test failure : >> >> DontFragmentTest.java fails with a runtime exception : “IP_DONTFRAGMENT >> should be supported” because the supportOptions doesn’t contain >> IP_DONTFRAGMENT flag. >> >> Reported Issue : [JDK-8305089](https://bugs.openjdk.org/browse/JDK-8305089) > > Varada M has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains six commits: > > - Merge branch 'master' into aix_socket_options > - Updated copyright year > - Update to copyright year > - AIX socket options > - aix socket options > - AIX Socket Options Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13240#pullrequestreview-1385016595
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Fri, 7 Apr 2023 10:24:20 GMT, Thomas Stuefe wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove unneeded qualified export from java.base to jdk.attach > > src/java.base/share/classes/jdk/internal/util/Architecture.java line 41: > >> 39: AARCH64(), >> 40: RISCV64(), >> 41: S390(), > > Why getting rid of the X in s390x? There has not been an s390 linux kernel in > almost ten years. Hello Thomas, that change was based on the review comment here https://github.com/openjdk/jdk/pull/13357#discussion_r1159810942 - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1160623813
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v4]
On Wed, 5 Apr 2023 19:20:08 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, >> PPC64LE` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The values of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Correct spelling of isAARCH64 in WIndows AttachProviderImpl src/java.base/share/classes/jdk/internal/util/Architecture.java line 42: > 40: AARCH64(), > 41: RISCV64(), > 42: S390X(), Hello Roger, in a recent unrelated discussion, @RealLucy noted here https://github.com/openjdk/jdk/pull/13228#issuecomment-1488650865 that `s390x` denotes the arch and operating system combination and `s390` on the other hand represents the architecture. So should this enum value instead be `S390`? - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1159810942
Re: RFR: 8305089: Implement missing socket options on AIX [v2]
On Thu, 30 Mar 2023 16:05:08 GMT, Varada M wrote: >> Breaking this into two parts : >> >> 1. Implementing socket options for AIX >> 2. DontFragmentTest failure >> >> - Implementing socket options for AIX : >> >> Unlike the linux, windows and macOS, AIX uses the default implementation for >> socket options such as ipDontFragmentSupported(), >> keepAliveOptionsSupported(), setTcpkeepAliveProbes, getTcpkeepAliveProbes, >> setQuickAck, getQuickAck and more, where it either returns false or >> exception. These options can be implemented on AIX with the supported flags >> like SO_PEERID, TCP_NODELAYACK is the equivalent AIX option for TCP_QUICKACK >> and IPPROTO_TCP, IP_DONTFRAG. >> >> - DontFragment test failure : >> >> DontFragmentTest.java fails with a runtime exception : “IP_DONTFRAGMENT >> should be supported” because the supportOptions doesn’t contain >> IP_DONTFRAGMENT flag. >> >> Reported Issue : [JDK-8305089](https://bugs.openjdk.org/browse/JDK-8305089) > > Varada M has updated the pull request incrementally with one additional > commit since the last revision: > > Update to copyright year Hello Varada, overall the changes look OK to me. I missed it previously, but the `jdk/net/ExtendedSocketOptions.java` file will need an update to the copyright year. Before integrating, please wait for another review from someone more experienced in this area. I lack experience in the native/C area, so I would like another Reviewer to take a look at this. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13240#pullrequestreview-1370236461
