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

[David Holmes]

On Mon, 23 Sep 2024 04:33:50 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.
>
> Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - move the code comment to a function comment
>  - clarify who sets the _JAVA_VERSION_SET environment variable

src/java.base/share/native/libjli/java.c line 1405:

> 1403:  * when launching java. It may be null, which implies the "-splash:" option wasn't used.
> 1404:  * The jar_path is the value that was provided to the "-jar" option when launching java.
> 1405:  * It too may be null, which implies the "-jar" option wasn't used.

But presumably we only call this function if (at least?) one of them is not null?

src/java.base/share/native/libjli/java.h line 58:

> 56:  * java runtime (older, newer or same version JRE installed at a different location) than
> 57:  * the one the launcher belongs to.
> 58:  * That support was discontinued starting JDK 9. However, JDK 1.8 launcher can still

Suggestion:

 * That support was discontinued starting JDK 9. However, the JDK 8 launcher can still

src/java.base/share/native/libjli/java.h line 59:

> 57:  * the one the launcher belongs to.
> 58:  * That support was discontinued starting JDK 9. However, JDK 1.8 launcher can still
> 59:  * be launched with JRE version selection options to launch Java runtimes greater than Java 8.

Suggestion:

 * be started with JRE version selection options to launch Java runtimes greater than JDK 8.

src/java.base/share/native/libjli/java.h line 60:

> 58:  * That support was discontinued starting JDK 9. However, JDK 1.8 launcher can still
> 59:  * be launched with JRE version selection options to launch Java runtimes greater than Java 8.
> 60:  * In such cases, the JDK 1.8 launcher when exec()ing the JDK N launcher, will set and propagate

Suggestion:

 * In such cases, the JDK 8 launcher when exec()ing the JDK N launcher, will set and propagate

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20997#discussion_r1770764935
PR Review Comment: https://git.openjdk.org/jdk/pull/20997#discussion_r1770765330
PR Review Comment: https://git.openjdk.org/jdk/pull/20997#discussion_r1770765205
PR Review Comment: https://git.openjdk.org/jdk/pull/20997#discussion_r1770765735