On Thu, 5 Feb 2026 02:52:59 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 [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
