On Thu, 12 Sep 2024 06:33:18 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Can I please get a review of this change which cleans up the code in the >> `java` launcher? The original motivation for the change was to prevent the >> `java` launcher's C code from parsing a jar's manifest when launching an >> application through `java -jar foo.jar`. The jar parsing code in C currently >> doesn't have the same level of robustness and features as what's available >> in the Java side implementation of `Zip/JarFile` API in `java.util.zip`. >> Among them, one is the lack of ZIP64 support in the launcher's jar parsing >> code. That issue was noticed in https://bugs.openjdk.org/browse/JDK-8328995 >> and a PR (https://github.com/openjdk/jdk/pull/18479) was proposed to enhance >> this C code in the launcher to support ZIP64 jar files. Even before the >> proposed changes in that PR, there already were a few concerns about long >> term maintainability of the jar parsing code in the launcher given that it >> duplicates what we already do in the Java side implementation, so adding new >> C code here isn't prefe rable. >> >> The change in this current PR removes the part where the launcher's C code >> was parsing the jar's manifest file to identify "Main-Class" and >> "SplashScreen-Image" attributes from the manifest of the jar that was being >> launched. This was being done in the C code to support a long outdated "JRE >> selection" feature (mJRE https://bugs.openjdk.org/browse/JDK-8058407) which >> required it to parse the jar's manifest. After that feature was removed, the >> sole reason the jar's manifest was continued to be parsed in this launcher >> code was for convenience. >> >> With the changes proposed in this PR, the launcher will use the jar parsing >> C code only if the jar has a "SplashScreen-Image" in its manifest. The >> number of such jars, based on an experiment against a large corpus of jar >> files, is very minimal (we found just 26 jars in around 900K odd jar files >> with a "SplashScreen-Image"). The updated code in this PR also delegates the >> identification of the "SplashScreen-Image" attribute to the java code (in >> `LauncherHelper`) thus removing the need to parse the manifest in C code. >> The only time the C code will now do the jar parsing is to display a splash >> screen image (if any is present) from the jar file. I think even that can be >> avoided, but I haven't experimented with it and I would like to pursue that >> separately in future, instead of this PR. >> >> Finally, a few of the utility functions that were introduced in the launcher >> C code in a recent JEP to support "Implicitly Declared Classes and... > > src/java.base/share/native/libjli/java.c line 1138: > >> 1136: has_arg = IsOptionWithArgument(argc, argv); >> 1137: if (JLI_StrCCmp(arg, "-version:") == 0) { >> 1138: JLI_ReportErrorMessage(SPC_ERROR1); > > Are these error message definitions, `SPC_ERROR1` etc, unused now? The `SPC_ERROR1` and similar error messages were being thrown when the launcher arguments `-version:`, `-jre-restrict-search` and `-jre-no-restrict-search` were being parsed. That parsing used to happen in this `SelectVersion()` function which has been removed in the PR. The parsing of these arguments has been moved to the existing `ParseArguments` function where it continues to throw this `SPC_ERROR1` and other related error messages. So these error message definitions are still in use. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20929#discussion_r1756252290
