On Mon, 23 Sep 2024 04:53:15 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 three additional > commits since the last revision: > > - David's review for code comment > > Co-authored-by: David Holmes > <62092539+dholmes-...@users.noreply.github.com> > - David's review for code comment > > Co-authored-by: David Holmes > <62092539+dholmes-...@users.noreply.github.com> > - David's review > > Co-authored-by: David Holmes > <62092539+dholmes-...@users.noreply.github.com> Thank you all for the reviews and the help with this cleanup. I'll go ahead with the integration now. ------------- PR Comment: https://git.openjdk.org/jdk/pull/20997#issuecomment-2369943423
