On Mon, 23 Sep 2024 04:33:50 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. > > 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
