On Fri, 13 Sep 2024 12:29:26 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
> Can I please get a review of this change which proposes to remove the > (internal) `SelectVersion()` function from the java launcher and also update > the code comments in the launcher to match the current implementation? > > As noted in https://bugs.openjdk.org/browse/JDK-8340114, the > `SelectVersion()` function in the launcher used to be relevant when JRE > selection was a feature. That feature has been removed since Java 9 > https://bugs.openjdk.org/browse/JDK-8058407. The SelectVersion() in its > current form isn't relevant anymore and can be removed. > > While at it, it was noticed that the current "flowchart" code comments in the > launcher which attempts to explain the flow in the launcher code are > outdated. The commit in this PR updates those comments for macosx and unix > implementation. The windows variant doesn't have a "flowchart", but it too > deserves a high level comment explaining this flow. I haven't updated the > windows variant in this PR because that does a few additional things, which I > need to review and understand better. I plan to take that up in a future > change. > > An existing > `test/jdk/tools/launcher/MultipleJRERemoved.java/MultipleJRERemoved` test had > to be updated due to the changes in this PR. That test was launching `java` > (once) with 3 unsupported JRE selection options and was expecting 3 error > messages (one each for the unsupported option) for that single launch. With > the change in this PR, we don't accumulate and throw all those 3 errors and > instead we fail fast for any of these 3 unsupported JRE selection options. > The fail fast implementation matches what we do with other similar > unsupported options. The test had to be updated to not expect all 3 errors > message in a single launch and instead expect to find one of those error > messages. Given what this test is for, and the fact that JRE version > selection options (rightly) continue to raise an error after this change, I > think, an update to that test should be OK. > > No new tests have been introduced in this PR and tier testing is currently in > progress. Good to see a clean up here but I'm still puzzled by some aspects. And it is a bit hard to track all the changes especially in relation to splashscreen. Thanks src/java.base/macosx/native/libjli/java_md_macosx.m line 83: > 81: * point of creating a new thread in CreateExecutionEnvironment, the > CreateExecutionEnvironment will check for > 82: * the state flag to see if a new thread has already been spawned and > upon noticing that it has, it will skip > 83: * spawning any more threads and will return back from > CreateExecutionEnvironment. My understanding was that this recursive invocation was only needed when switching modes/models. I don't see why it would be needed now. src/java.base/share/native/libjli/java.c line 38: > 36: * One job of the launcher is to remove command line options which the > 37: * vm does not understand and will not process. These options include > 38: * options which select which style of vm is run (e.g. -client and Aren't these the only options now? src/java.base/share/native/libjli/java.c line 1345: > 1343: if (JLI_StrCCmp(arg, "-Djava.class.path=") == 0) { > 1344: _have_classpath = JNI_TRUE; > 1345: } else if (JLI_StrCmp(arg, "-Djava.awt.headless=true") == > 0) { Was this processing moved from somewhere else? src/java.base/share/native/libjli/java.c line 1347: > 1345: } else if (JLI_StrCmp(arg, "-Djava.awt.headless=true") == > 0) { > 1346: /* > 1347: * Checking for headless toolkit option in the some way > as AWT does: Suggestion: * Checking for headless toolkit option in the same way as AWT does: src/java.base/share/native/libjli/java.c line 1409: > 1407: } > 1408: // not in headless mode. we now set a couple of env variables > that > 1409: // will be used later by ShowSplashScreen() Suggestion: // Not in headless mode. We now set a couple of env variables that // will be used later by ShowSplashScreen(). src/java.base/share/native/libjli/java.c line 1421: > 1419: ) { > 1420: > 1421: // command line specified "-splash:" takes priority over manifest > one. General comment on comment style. Comments should either be written like sentences and start with a Capital and end with a period; or have neither. src/java.base/unix/native/libjli/java_md.c line 60: > 58: * - JLI_Launch function, which is the entry point to the launcher, > calls CreateExecutionEnvironment > 59: * > 60: * - CreateExecutionEnvironment does the following (not necessarily in > that order): Suggestion: * - CreateExecutionEnvironment does the following (not necessarily in this order): ------------- PR Review: https://git.openjdk.org/jdk/pull/20997#pullrequestreview-2305661381 PR Review Comment: https://git.openjdk.org/jdk/pull/20997#discussion_r1760429832 PR Review Comment: https://git.openjdk.org/jdk/pull/20997#discussion_r1760430531 PR Review Comment: https://git.openjdk.org/jdk/pull/20997#discussion_r1760432600 PR Review Comment: https://git.openjdk.org/jdk/pull/20997#discussion_r1760431555 PR Review Comment: https://git.openjdk.org/jdk/pull/20997#discussion_r1760433596 PR Review Comment: https://git.openjdk.org/jdk/pull/20997#discussion_r1760434219 PR Review Comment: https://git.openjdk.org/jdk/pull/20997#discussion_r1760434658
