# HG changeset patch
# User herrick
# Date 1572618315 14400
# Node ID 6539ad1d90aaaf55b38e994138040d5514754de8
# Parent  f04c0704a00660a1976978cb6e3c536835be3c77
8233333 : Incorrect comparison of number version strings in ToolValidator
Submitted-by: asemenyuk
Reviewed-by: aherrick, almatvee

diff -r f04c0704a006 -r 6539ad1d90aa src/jdk.jpackage/linux/classes/jdk/jpackage/internal/LinuxRpmBundler.java
--- a/src/jdk.jpackage/linux/classes/jdk/jpackage/internal/LinuxRpmBundler.java	Fri Nov 01 10:22:49 2019 -0400
+++ b/src/jdk.jpackage/linux/classes/jdk/jpackage/internal/LinuxRpmBundler.java	Fri Nov 01 10:25:15 2019 -0400
@@ -107,7 +107,8 @@
 
     public final static String TOOL_RPM = "rpm";
     public final static String TOOL_RPMBUILD = "rpmbuild";
-    public final static String TOOL_RPMBUILD_MIN_VERSION = "4.0";
+    public final static DottedVersion TOOL_RPMBUILD_MIN_VERSION = DottedVersion.lazy(
+            "4.0");
 
     public LinuxRpmBundler() {
         super(PACKAGE_NAME);
diff -r f04c0704a006 -r 6539ad1d90aa src/jdk.jpackage/share/classes/jdk/jpackage/internal/DottedVersion.java
--- a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/DottedVersion.java	Fri Nov 01 10:22:49 2019 -0400
+++ b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/DottedVersion.java	Fri Nov 01 10:25:15 2019 -0400
@@ -28,6 +28,7 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Objects;
+import java.util.regex.Pattern;
 
 /**
  * Dotted numeric version string.
@@ -36,14 +37,29 @@
 class DottedVersion implements Comparable<String> {
 
     public DottedVersion(String version) {
-        components = parseVersionString(version);
+        greedy = true;
+        components = parseVersionString(version, greedy);
+        value = version;
+    }
+
+    private DottedVersion(String version, boolean greedy) {
+        this.greedy = greedy;
+        components = parseVersionString(version, greedy);
         value = version;
     }
 
+    public static DottedVersion greedy(String version) {
+        return new DottedVersion(version);
+    }
+
+    public static DottedVersion lazy(String version) {
+        return new DottedVersion(version, false);
+    }
+
     @Override
     public int compareTo(String o) {
         int result = 0;
-        int[] otherComponents = parseVersionString(o);
+        int[] otherComponents = parseVersionString(o, greedy);
         for (int i = 0; i < Math.min(components.length, otherComponents.length)
                 && result == 0; ++i) {
             result = components[i] - otherComponents[i];
@@ -56,9 +72,12 @@
         return result;
     }
 
-    private static int[] parseVersionString(String version) {
+    private static int[] parseVersionString(String version, boolean greedy) {
         Objects.requireNonNull(version);
         if (version.isEmpty()) {
+            if (!greedy) {
+                return new int[] {0};
+            }
             throw new IllegalArgumentException("Version may not be empty string");
         }
 
@@ -66,17 +85,36 @@
         List<Integer> components = new ArrayList<>();
         for (var component : version.split("\\.", -1)) {
             if (component.isEmpty()) {
+                if (!greedy) {
+                    break;
+                }
+
                 throw new IllegalArgumentException(String.format(
                         "Version [%s] contains a zero lenght component", version));
             }
 
-            int num = Integer.parseInt(component);
-            if (num < 0) {
+            if (!DIGITS.matcher(component).matches()) {
+                // Catch "+N" and "-N"  cases.
+                if (!greedy) {
+                    break;
+                }
+
                 throw new IllegalArgumentException(String.format(
                         "Version [%s] contains invalid component [%s]", version,
                         component));
             }
 
+            final int num;
+            try {
+                num = Integer.parseInt(component);
+            } catch (NumberFormatException ex) {
+                if (!greedy) {
+                    break;
+                }
+
+                throw ex;
+            }
+
             if (num != 0) {
                 lastNotZeroIdx = components.size();
             }
@@ -88,6 +126,9 @@
             components = components.subList(0, lastNotZeroIdx + 1);
         }
 
+        if (components.isEmpty()) {
+            components.add(0);
+        }
         return components.stream().mapToInt(Integer::intValue).toArray();
     }
 
@@ -98,4 +139,7 @@
 
     final private int[] components;
     final private String value;
+    final private boolean greedy;
+
+    private static final Pattern DIGITS = Pattern.compile("\\d+");
 }
diff -r f04c0704a006 -r 6539ad1d90aa src/jdk.jpackage/share/classes/jdk/jpackage/internal/ToolValidator.java
--- a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/ToolValidator.java	Fri Nov 01 10:22:49 2019 -0400
+++ b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/ToolValidator.java	Fri Nov 01 10:25:15 2019 -0400
@@ -58,7 +58,7 @@
         return this;
     }
 
-    ToolValidator setMinimalVersion(String v) {
+    ToolValidator setMinimalVersion(Comparable<String> v) {
         minimalVersion = v;
         return this;
     }
@@ -129,7 +129,7 @@
 
     private final Path toolPath;
     private List<String> args;
-    private String minimalVersion;
+    private Comparable<String> minimalVersion;
     private Function<Stream<String>, String> versionParser;
     private BiFunction<String, IOException, ConfigException> toolNotFoundErrorHandler;
     private BiFunction<String, String, ConfigException> toolOldVersionErrorHandler;
diff -r f04c0704a006 -r 6539ad1d90aa src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WixTool.java
--- a/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WixTool.java	Fri Nov 01 10:22:49 2019 -0400
+++ b/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WixTool.java	Fri Nov 01 10:25:15 2019 -0400
@@ -114,7 +114,7 @@
                 })::validate;
     }
 
-    private final static String MINIMAL_VERSION = "3.0";
+    private final static DottedVersion MINIMAL_VERSION = DottedVersion.lazy("3.0");
 
     static Path getSystemDir(String envVar, String knownDir) {
         return Optional
@@ -132,7 +132,6 @@
             } catch (InvalidPathException ex) {
                 Log.error(MessageFormat.format(I18N.getString(
                         "error.invalid-envvar"), envVar));
-                return null;
             }
         }
         return null;
@@ -142,9 +141,9 @@
         PathMatcher wixInstallDirMatcher = FileSystems.getDefault().getPathMatcher(
                 "glob:WiX Toolset v*");
 
-        Path programFiles = getSystemDir("ProgramFiles", "Program Files");
-        Path programFilesX86 = getSystemDir("ProgramFiles(X86)",
-                "Program Files (x86)");
+        Path programFiles = getSystemDir("ProgramFiles", "\\Program Files");
+        Path programFilesX86 = getSystemDir("ProgramFiles(x86)",
+                "\\Program Files (x86)");
 
         // Returns list of WiX install directories ordered by WiX version number.
         // Newer versions go first.
diff -r f04c0704a006 -r 6539ad1d90aa test/jdk/tools/jpackage/TEST.properties
--- a/test/jdk/tools/jpackage/TEST.properties	Fri Nov 01 10:22:49 2019 -0400
+++ b/test/jdk/tools/jpackage/TEST.properties	Fri Nov 01 10:25:15 2019 -0400
@@ -1,2 +1,3 @@
 keys=jpackagePlatformPackage
 requires.properties=jpackage.test.SQETest
+maxOutputSize=2000000
diff -r f04c0704a006 -r 6539ad1d90aa test/jdk/tools/jpackage/junit/jdk/jpackage/internal/CompareDottedVersionTest.java
--- a/test/jdk/tools/jpackage/junit/jdk/jpackage/internal/CompareDottedVersionTest.java	Fri Nov 01 10:22:49 2019 -0400
+++ b/test/jdk/tools/jpackage/junit/jdk/jpackage/internal/CompareDottedVersionTest.java	Fri Nov 01 10:25:15 2019 -0400
@@ -22,10 +22,9 @@
  */
 package jdk.jpackage.internal;
 
+import java.util.ArrayList;
 import java.util.List;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
-import junit.framework.*;
+import java.util.function.Function;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
@@ -35,24 +34,46 @@
 @RunWith(Parameterized.class)
 public class CompareDottedVersionTest {
 
-    public CompareDottedVersionTest(String version1, String version2, int result) {
+    public CompareDottedVersionTest(boolean greedy, String version1,
+            String version2, int result) {
         this.version1 = version1;
         this.version2 = version2;
         this.expectedResult = result;
+
+        if (greedy) {
+            createTestee = DottedVersion::greedy;
+        } else {
+            createTestee = DottedVersion::lazy;
+        }
     }
 
     @Parameters
     public static List<Object[]> data() {
-        return List.of(new Object[][] {
-            { "00.0.0", "0", 0 },
-            { "0.035", "0.0035", 0 },
-            { "1", "1", 0 },
-            { "2", "2.0", 0 },
-            { "2.00", "2.0", 0 },
-            { "1.2.3.4", "1.2.3.4.5", -1 },
-            { "34", "33", 1 },
-            { "34.0.78", "34.1.78", -1 }
-        });
+        List<Object[]> data = new ArrayList<>();
+        for (var greedy : List.of(true, false)) {
+            data.addAll(List.of(new Object[][] {
+                { greedy, "00.0.0", "0", 0 },
+                { greedy, "0.035", "0.0035", 0 },
+                { greedy, "1", "1", 0 },
+                { greedy, "2", "2.0", 0 },
+                { greedy, "2.00", "2.0", 0 },
+                { greedy, "1.2.3.4", "1.2.3.4.5", -1 },
+                { greedy, "34", "33", 1 },
+                { greedy, "34.0.78", "34.1.78", -1 }
+            }));
+        }
+
+        data.addAll(List.of(new Object[][] {
+            { false, "", "1", -1 },
+            { false, "1.2.4-R4", "1.2.4-R5", 0 },
+            { false, "1.2.4.-R4", "1.2.4.R5", 0 },
+            { false, "7+1", "7+4", 0 },
+            { false, "2+14", "2-14", 0 },
+            { false, "23.4.RC4", "23.3.RC10", 1 },
+            { false, "77.0", "77.99999999999999999999999999999999999999999999999", 0 },
+        }));
+
+        return data;
     }
 
     @Test
@@ -64,8 +85,8 @@
         assertEquals(actualResult, -1 * actualNegateResult);
     }
 
-    private static int compare(String x, String y) {
-        int result = new DottedVersion(x).compareTo(y);
+    private int compare(String x, String y) {
+        int result = createTestee.apply(x).compareTo(y);
 
         if (result < 0) {
             return -1;
@@ -81,4 +102,5 @@
     private final String version1;
     private final String version2;
     private final int expectedResult;
+    private final Function<String, DottedVersion> createTestee;
 }
diff -r f04c0704a006 -r 6539ad1d90aa test/jdk/tools/jpackage/junit/jdk/jpackage/internal/DottedVersionTest.java
--- a/test/jdk/tools/jpackage/junit/jdk/jpackage/internal/DottedVersionTest.java	Fri Nov 01 10:22:49 2019 -0400
+++ b/test/jdk/tools/jpackage/junit/jdk/jpackage/internal/DottedVersionTest.java	Fri Nov 01 10:25:15 2019 -0400
@@ -22,29 +22,73 @@
  */
 package jdk.jpackage.internal;
 
+import java.util.Collections;
+import java.util.List;
+import java.util.function.Function;
 import java.util.stream.Stream;
-import org.junit.Assert;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import static org.junit.Assert.*;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 
+@RunWith(Parameterized.class)
 public class DottedVersionTest {
 
+    public DottedVersionTest(boolean greedy) {
+        this.greedy = greedy;
+        if (greedy) {
+            createTestee = DottedVersion::greedy;
+        } else {
+            createTestee = DottedVersion::lazy;
+        }
+    }
+
+    @Parameterized.Parameters
+    public static List<Object[]> data() {
+        return List.of(new Object[] { true }, new Object[] { false });
+    }
+
     @Rule
     public ExpectedException exceptionRule = ExpectedException.none();
 
     @Test
     public void testValid() {
-        Stream.of(
+        final List<String> validStrings = List.of(
             "1.0",
             "1",
             "2.234.045",
             "2.234.0",
             "0",
             "0.1"
-        ).forEach(value -> {
-            DottedVersion version = new DottedVersion(value);
+        );
+
+        final List<String> validLazyStrings;
+        if (greedy) {
+            validLazyStrings = Collections.emptyList();
+        } else {
+            validLazyStrings = List.of(
+                "1.-1",
+                "5.",
+                "4.2.",
+                "3..2",
+                "2.a",
+                "0a",
+                ".",
+                " ",
+                " 1",
+                "1. 2",
+                "+1",
+                "-1",
+                "-0",
+                "1234567890123456789012345678901234567890"
+            );
+        }
+
+        Stream.concat(validStrings.stream(), validLazyStrings.stream())
+        .forEach(value -> {
+            DottedVersion version = createTestee.apply(value);
             assertEquals(version.toString(), value);
         });
     }
@@ -52,13 +96,21 @@
     @Test
     public void testNull() {
         exceptionRule.expect(NullPointerException.class);
-        new DottedVersion(null);
+        createTestee.apply(null);
     }
 
     @Test
     public void testEmpty() {
-        exceptionRule.expect(IllegalArgumentException.class);
-        exceptionRule.expectMessage("Version may not be empty string");
-        new DottedVersion("");
+        if (greedy) {
+            exceptionRule.expect(IllegalArgumentException.class);
+            exceptionRule.expectMessage("Version may not be empty string");
+            createTestee.apply("");
+        } else {
+            assertTrue(0 == createTestee.apply("").compareTo(""));
+            assertTrue(0 == createTestee.apply("").compareTo("0"));
+        }
     }
+
+    private final boolean greedy;
+    private final Function<String, DottedVersion> createTestee;
 }
diff -r f04c0704a006 -r 6539ad1d90aa test/jdk/tools/jpackage/junit/jdk/jpackage/internal/InvalidDottedVersionTest.java
--- a/test/jdk/tools/jpackage/junit/jdk/jpackage/internal/InvalidDottedVersionTest.java	Fri Nov 01 10:22:49 2019 -0400
+++ b/test/jdk/tools/jpackage/junit/jdk/jpackage/internal/InvalidDottedVersionTest.java	Fri Nov 01 10:25:15 2019 -0400
@@ -51,7 +51,11 @@
             ".",
             " ",
             " 1",
-            "1. 2"
+            "1. 2",
+            "+1",
+            "-1",
+            "-0",
+            "1234567890123456789012345678901234567890"
         ).map(version -> new Object[] { version }).collect(Collectors.toList());
     }
 
diff -r f04c0704a006 -r 6539ad1d90aa test/jdk/tools/jpackage/junit/jdk/jpackage/internal/ToolValidatorTest.java
--- a/test/jdk/tools/jpackage/junit/jdk/jpackage/internal/ToolValidatorTest.java	Fri Nov 01 10:22:49 2019 -0400
+++ b/test/jdk/tools/jpackage/junit/jdk/jpackage/internal/ToolValidatorTest.java	Fri Nov 01 10:25:15 2019 -0400
@@ -51,13 +51,21 @@
         }).validate();
 
         // Minimal version is 1, actual is 10. Should be OK.
-        assertNull(
-                new ToolValidator(TOOL_JAVA).setMinimalVersion(
-                        "1").setVersionParser(unused -> "10").validate());
+        assertNull(new ToolValidator(TOOL_JAVA).setMinimalVersion(
+                new DottedVersion("1")).setVersionParser(unused -> "10").validate());
 
         // Minimal version is 5, actual is 4.99.37. Error expected.
         assertValidationFailure(new ToolValidator(TOOL_JAVA).setMinimalVersion(
-                "5").setVersionParser(unused -> "4.99.37").validate(), false);
+                new DottedVersion("5")).setVersionParser(unused -> "4.99.37").validate(),
+                false);
+
+        // Minimal version is 8, actual is 10, lexicographical comparison is used. Error expected.
+        assertValidationFailure(new ToolValidator(TOOL_JAVA).setMinimalVersion(
+                "8").setVersionParser(unused -> "10").validate(), false);
+
+        // Minimal version is 8, actual is 10, Use DottedVersion class for comparison. Should be OK.
+        assertNull(new ToolValidator(TOOL_JAVA).setMinimalVersion(
+                new DottedVersion("8")).setVersionParser(unused -> "10").validate());
     }
 
     private static void assertValidationFailure(ConfigException v,