RFR: 8340114: Remove outdated SelectVersion() function from the launcher and update the code comments explaining the code flow

[David Holmes]

On Fri, 13 Sep 2024 12:29:26 GMT, Jaikiran Pai <jpai at 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