On Mon, 16 Sep 2024 13:55:49 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 one additional > commit since the last revision: > > comment improvements Thank you for the review David. > Okay I had expected (hoped for?) more simplifications but it seems a lot of > the complexity of the launch procedure remains. In general, I've several changes that are lined up for cleaning up the launcher code. Some deprecations, some clean up and some fixes. I plan to propose those changes in the coming days. This current PR was a small isolated step towards getting the launcher to a state where we can get rid of jar manifest processing within the C code (there are a few additional things needed as a separate PR to help us reach there). This code and the area is new to me, I welcome any additional inputs that you have in mind in updating/changing or cleaning up this launcher code. > I think basically this just gets rid of the data model and mJRE stuff and > moves the rest out of SelectVersion. Now that you mention it, one part of the changes in this PR, moves the checks and error reporting for the `-version:`, `-jre-restrict-search` and `-jre-no-restrict-search` options to the `ParseArguments` function. I think we can just get rid of that code and let those options be considered as just another unknown option to the launcher. I can open a CSR linked to this current PR to formalize that change. That would also mean that the `MultipleJRERemoved` test can also be deleted. ------------- PR Comment: https://git.openjdk.org/jdk/pull/20997#issuecomment-2355365863
