RFR: 8357404: jpackage should attempt to get a package version from the JDK's release file if the --version option is not specified [v9]
Alexey Semenyuk
asemenyuk at openjdk.org
Thu Feb 5 17:26:14 UTC 2026
On Thu, 5 Feb 2026 02:52:59 GMT, Alexander Matveev <almatvee at openjdk.org> 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 [v8]
src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacFromOptions.java line 237:
> 235: return normalizeVersion(version);
> 236: };
> 237: app = ApplicationBuilder.normalizeVersion(app, app.version(), versionNormalizer);
Let javac do the work:
ApplicationBuilder.normalizeVersion(app, app.version(), MacFromOptions::normalizeVersion);
src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacFromOptions.java line 367:
> 365: return ver.toComponentsString();
> 366: }
> 367: }
Why branching?
It can be as simple as:
DottedVersion.lazy(version).trim(3).pad(3).toComponentsString()
The same comment applies to other locations where `trim()` and `pad()` are used.
src/jdk.jpackage/share/classes/jdk/jpackage/internal/model/DottedVersion.java line 253:
> 251:
> 252: return this;
> 253: }
These methods should not modify the instance. They should create and return a copy instead. Besides, the implementation doesn't update the `value` field, so the `toString()` method would return the same string as before any of these methods is called.
The `value` field was OK until the new methods were added; now it is a problem. I suggest getting rid of it.
Attached refactored variant of the class with `trim()` and `pad()` methods implemented so that they don't modify the instance. Passes all tests.
[DottedVersion.patch](https://github.com/user-attachments/files/25105160/DottedVersion.patch)
src/jdk.jpackage/share/classes/jdk/jpackage/internal/model/DottedVersion.java line 263:
> 261: }
> 262:
> 263: private BigInteger[] components;
The "final" was on purpose. It made the instances R/O. This change makes them mutable and not thread-safe.
test/jdk/tools/jpackage/junit/share/jdk.jpackage/jdk/jpackage/internal/model/DottedVersionTest.java line 42:
> 40: public class DottedVersionTest {
> 41:
> 42: public record TestConfig(String input,
`TestConfig` is used to test the `DottedVersion.greedy()` and `DottedVersion.lazy()` methods, which construct a DottedVersion instance from a string.
`DottedVersion.trim()` and `DottedVersion.pad()` are instance methods. That said, `TestConfig` doesn't fit to test them.
test/jdk/tools/jpackage/junit/share/jdk.jpackage/jdk/jpackage/internal/model/DottedVersionTest.java line 163:
> 161: ));
> 162: return data;
> 163: }
Missing coverage:
- how the suffix is carried
- toString()
The below fills goverage gaps for the `trim()` method (with the assumpion the `trim()` method doesn't modify the instance, but instead returns a modified copy):
@ParameterizedTest
@MethodSource
public void test_trim(DottedVersion ver, String expectedStr, int limit) {
var expected = DottedVersion.lazy(expectedStr);
var actual = ver.trim(limit);
assertEquals(expected, actual);
if (limit >= ver.getComponents().length) {
assertSame(ver, actual);
} else {
assertNotSame(ver, actual);
}
assertEquals(expectedStr, actual.toString());
}
@ParameterizedTest
@MethodSource
public void test_trim_negative(DottedVersion ver, int limit) {
assertThrowsExactly(IllegalArgumentException.class, () -> {
ver.trim(limit);
});
}
private static Stream<Arguments> test_trim() {
var testCases = new ArrayList<Arguments>();
for (var suffix : List.of("", ".foo")) {
testCases.addAll(List.of(
Arguments.of("1.02.3" + suffix, "" + suffix, 0),
Arguments.of("1.02.3" + suffix, "1" + suffix, 1),
Arguments.of("1.02.3" + suffix, "1.02" + suffix, 2),
Arguments.of("1.02.3" + suffix, "1.02.3" + suffix, 3),
Arguments.of("1.02.3" + suffix, "1.02.3" + suffix, 4)
));
}
return testCases.stream().map(DottedVersionTest::mapFirstStringToDottedVersion);
}
private static Stream<Arguments> test_trim_negative() {
return Stream.of(
Arguments.of("10.5.foo", -1)
).map(DottedVersionTest::mapFirstStringToDottedVersion);
}
private static Arguments mapFirstStringToDottedVersion(Arguments v) {
var objs = v.get();
objs[0] = DottedVersion.lazy((String)objs[0]);
return Arguments.of(objs);
}
test/jdk/tools/jpackage/junit/share/jdk.jpackage/jdk/jpackage/internal/util/RuntimeVersionReaderTest.java line 50:
> 48: "27.1.2, false",
> 49: "27.1.2-ea, true",
> 50: "27.1.2-ea, false"
Can we remove redundant `quoteVesrion` parameter and simplify it to:
@CsvSource({
""27.1.2"",
"27.1.2",
""27.1.2-ea"",
"27.1.2-ea"
})
?
test/jdk/tools/jpackage/share/NoMPathRuntimeTest.java line 99:
> 97: Files.createDirectories(workDir.resolve("Contents/MacOS"));
> 98: }
> 99: }
I don't understand the comment. Why is this change?
test/jdk/tools/jpackage/share/RuntimePackageTest.java line 148:
> 146: @Parameter(value = {"17.21.3-ea", "17.21.3"}, ifOS = MACOS)
> 147: @Parameter(value = {"17.21.3-ea", "17.21.3.0"}, ifOS = WINDOWS)
> 148: public static void testReleaseFileVersion(String version, String normalizedVersion) {
We need a test case(s) when the "release" has an invalid JDK version to verify jpackage will ignore it. Something like:
@Parameter(value = {"foo", "1.0"})
@Parameter(value = {"", "1.0"})
@Parameter(value = {"17.21.3-foo", "1.0"})
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2770290107
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2770281987
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2770246352
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2770225139
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2770201577
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2770221902
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2769832525
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2769800430
PR Review Comment: https://git.openjdk.org/jdk/pull/29260#discussion_r2769819521
More information about the core-libs-dev
mailing list