On Sun, 18 Jan 2026 05:17:40 GMT, Alexander Matveev <[email protected]>
wrote:
>> - Version will be read from JDK's release file if not provided via
>> `--version` for runtime installer.
>> - Added test to cover added functionality.
>> - On macOS and Windows version from JDK's release file will be normalized if
>> it does not fit platform requirements.
>
> Alexander Matveev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8357404: jpackage should attempt to get a package version from the JDK's
> release file if the --version option is not specified [v6]
Changes requested by asemenyuk (Reviewer).
src/jdk.jpackage/share/classes/jdk/jpackage/internal/FromOptions.java line 191:
> 189: if (!APP_VERSION.containsIn(options)) {
> 190: // Version is not specified explicitly. Try to get it
> from the release file.
> 191: final Path releaseFile =
> RuntimeImageUtils.getReleaseFilePath(
The path to the predefined runtime directory (the directory with the standard
JDK structure) is stored in the `predefinedRuntimeDirectory` variable.
src/jdk.jpackage/share/classes/jdk/jpackage/internal/ModuleInfo.java line 64:
> 62: // of `release` file.
> 63:
> 64: final Path releaseFile =
> RuntimeImageUtils.getReleaseFilePath(cookedRuntime);
Assume macOS-specificity has been removed from
`RuntimeImageUtils.getReleaseFilePath()`, the code will be:
final Path releaseFile =
RuntimeImageUtils.getReleaseFilePath(MacBundle.fromPath(cookedRuntime).map(MacBundle::homeDir).orElse(cookedRuntime));
src/jdk.jpackage/share/classes/jdk/jpackage/internal/util/RuntimeImageUtils.java
line 47:
> 45: }
> 46:
> 47: return releaseFile;
Why do we need platform-specificity in this function?
Shouldn't it be as simple as:
public static Path getReleaseFilePath(Path runtimePath) {
return runtimePath.resolve("release");
}
src/jdk.jpackage/share/classes/jdk/jpackage/internal/util/RuntimeVersionReader.java
line 47:
> 45: String version = props.getProperty("JAVA_VERSION");
> 46: if (version != null) {
> 47: version = version.replaceAll("^\"|\"$", "");
Why does this function filter the value of the "JAVA_VERSION" property?
It should not do any filtering; it should just read the value as its name
suggests.
Filering is platform-specific and should be a separate method.
src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WinFromOptions.java line
139:
> 137: // When reading from release file it can be 1 or 3 or maybe more.
> 138: // We always normalize to 4 components.
> 139: DottedVersion ver = DottedVersion.greedy(version);
This should be `DottedVersion.lazy()`, not `DottedVersion.greedy()`, as the
latter will throw if the string has a trailing substring that is not a valid
version component.
src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WinFromOptions.java line
141:
> 139: DottedVersion ver = DottedVersion.greedy(version);
> 140: BigInteger[] components = ver.getComponents();
> 141: if (components.length == 1 || components.length == 3 ||
This condition is redundant if the DottedVersion class has `trim()` and `pad()`
methods as suggested above. The whole function can be a one-liner:
return DottedVersion.lazy(version).trim(4).pad(4).toComponentsString();
test/jdk/tools/jpackage/junit/share/jdk.jpackage/jdk/jpackage/internal/util/RuntimeVersionReaderTest.java
line 41:
> 39: import org.junit.jupiter.api.io.TempDir;
> 40:
> 41: public class RuntimeVersionReaderTest {
I doubt this unit test has ever been executed.
test/jdk/tools/jpackage/share/RuntimePackageTest.java line 87:
> 85: public class RuntimePackageTest {
> 86:
> 87: // @Test
Is this debugging leftovers?
test/jdk/tools/jpackage/share/RuntimePackageTest.java line 120:
> 118: @Test
> 119: // 27
> 120: @Parameter(value = {"27", ""}, ifOS = {LINUX, MACOS})
I think we need some "unusual" test cases to test the robustness of the
version-reading code. E.g.: "foo", "17.21.3+foo", "".
test/jdk/tools/jpackage/share/RuntimePackageTest.java line 160:
> 158: .cannedFormattedString("message.release-version",
> 159: version, runtimeImage.toString()));
> 160: if (!normalizedVersion.isEmpty()) {
Why would we not have a normalized version in some cases? Shouldn't it always
be available?
-------------
PR Review: https://git.openjdk.org/jdk/pull/29260#pullrequestreview-3741519906
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2756004587
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2756018116
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2756000367
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2755967624
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2756028862
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2756038982
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2756054341
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2756056271
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2756068456
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2756060100
