Merge pull request #4017 from jon-turney/version-comparison-rewrite

Use rpmvercmp version comparison

6 files changed, 221 insertions, 91 deletions

diff --git a/docs/markdown/Reference-manual.md b/docs/markdown/Reference-manual.md
index 907ceda..b616d3e 100644
--- a/docs/markdown/Reference-manual.md
+++ b/docs/markdown/Reference-manual.md
@@ -1094,7 +1094,7 @@ Project supports the following keyword arguments.
     `meson.project_license()`.
 
   - `meson_version` takes a string describing which Meson version the
-    project requires. Usually something like `>0.28.0`.
+    project requires. Usually something like `>=0.28.0`.
 
   - `subproject_dir` specifies the top level directory name that holds
     Meson subprojects. This is only meant as a compatibility option
diff --git a/docs/markdown/snippets/version_comparison.md b/docs/markdown/snippets/version_comparison.md
new file mode 100644
index 0000000..861a3ee
--- /dev/null
+++ b/docs/markdown/snippets/version_comparison.md
@@ -0,0 +1,15 @@
+## Version comparison
+
+`dependency(version:)` and other version constraints now handle versions
+containing non-numeric characters better, comparing versions using the rpmvercmp
+algorithm (as using the `pkg-config` autoconf macro `PKG_CHECK_MODULES` does).
+
+This is a breaking change for exact comparison constraints which rely on the
+previous comparison behaviour of extending the compared versions with `'0'`
+elements, up to the same length of `'.'`-separated elements.
+
+For example, a version of `'0.11.0'` would previously match a version constraint
+of `'==0.11'`, but no longer does, being instead considered strictly greater.
+
+Instead, use a version constraint which exactly compares with the precise
+version required, e.g. `'==0.11.0'`.
diff --git a/mesonbuild/mesonlib.py b/mesonbuild/mesonlib.py
index 1b9cb42..8a2dc0c 100644
--- a/mesonbuild/mesonlib.py
+++ b/mesonbuild/mesonlib.py
@@ -14,6 +14,7 @@
 
 """A library of random helper functionality."""
 
+import functools
 import sys
 import stat
 import time
@@ -390,33 +391,59 @@ def detect_vcs(source_dir):
                 return vcs
     return None
 
-def grab_leading_numbers(vstr, strict=False):
-    result = []
-    for x in vstr.rstrip('.').split('.'):
-        try:
-            result.append(int(x))
-        except ValueError as e:
-            if strict:
-                msg = 'Invalid version to compare against: {!r}; only ' \
-                      'numeric digits separated by "." are allowed: ' + str(e)
-                raise MesonException(msg.format(vstr))
-            break
-    return result
+# a helper class which implements the same version ordering as RPM
+@functools.total_ordering
+class Version:
+    def __init__(self, s):
+        self._s = s
 
-def make_same_len(listA, listB):
-    maxlen = max(len(listA), len(listB))
-    for i in listA, listB:
-        for n in range(len(i), maxlen):
-            i.append(0)
+        # split into numeric, alphabetic and non-alphanumeric sequences
+        sequences = re.finditer(r'(\d+|[a-zA-Z]+|[^a-zA-Z\d]+)', s)
+        # non-alphanumeric separators are discarded
+        sequences = [m for m in sequences if not re.match(r'[^a-zA-Z\d]+', m.group(1))]
+        # numeric sequences have leading zeroes discarded
+        sequences = [re.sub(r'^0+(\d)', r'\1', m.group(1), 1) for m in sequences]
+
+        self._v = sequences
+
+    def __str__(self):
+        return '%s (V=%s)' % (self._s, str(self._v))
 
-numpart = re.compile('[0-9.]+')
+    def __lt__(self, other):
+        return self.__cmp__(other) == -1
 
-def version_compare(vstr1, vstr2, strict=False):
-    match = numpart.match(vstr1.strip())
-    if match is None:
-        msg = 'Uncomparable version string {!r}.'
-        raise MesonException(msg.format(vstr1))
-    vstr1 = match.group(0)
+    def __eq__(self, other):
+        return self.__cmp__(other) == 0
+
+    def __cmp__(self, other):
+        def cmp(a, b):
+            return (a > b) - (a < b)
+
+        # compare each sequence in order
+        for i in range(0, min(len(self._v), len(other._v))):
+            # sort a non-digit sequence before a digit sequence
+            if self._v[i].isdigit() != other._v[i].isdigit():
+                return 1 if self._v[i].isdigit() else -1
+
+            # compare as numbers
+            if self._v[i].isdigit():
+                # because leading zeros have already been removed, if one number
+                # has more digits, it is greater
+                c = cmp(len(self._v[i]), len(other._v[i]))
+                if c != 0:
+                    return c
+                # fallthrough
+
+            # compare lexicographically
+            c = cmp(self._v[i], other._v[i])
+            if c != 0:
+                return c
+
+        # if equal length, all components have matched, so equal
+        # otherwise, the version with a suffix remaining is greater
+        return cmp(len(self._v), len(other._v))
+
+def _version_extract_cmpop(vstr2):
     if vstr2.startswith('>='):
         cmpop = operator.ge
         vstr2 = vstr2[2:]
@@ -440,10 +467,12 @@ def version_compare(vstr1, vstr2, strict=False):
         vstr2 = vstr2[1:]
     else:
         cmpop = operator.eq
-    varr1 = grab_leading_numbers(vstr1, strict)
-    varr2 = grab_leading_numbers(vstr2, strict)
-    make_same_len(varr1, varr2)
-    return cmpop(varr1, varr2)
+
+    return (cmpop, vstr2)
+
+def version_compare(vstr1, vstr2):
+    (cmpop, vstr2) = _version_extract_cmpop(vstr2)
+    return cmpop(Version(vstr1), Version(vstr2))
 
 def version_compare_many(vstr1, conditions):
     if not isinstance(conditions, (list, tuple, frozenset)):
@@ -451,28 +480,22 @@ def version_compare_many(vstr1, conditions):
     found = []
     not_found = []
     for req in conditions:
-        if not version_compare(vstr1, req, strict=True):
+        if not version_compare(vstr1, req):
             not_found.append(req)
         else:
             found.append(req)
     return not_found == [], not_found, found
 
-
+# determine if the minimum version satisfying the condition |condition| exceeds
+# the minimum version for a feature |minimum|
 def version_compare_condition_with_min(condition, minimum):
-    match = numpart.match(minimum.strip())
-    if match is None:
-        msg = 'Uncomparable version string {!r}.'
-        raise MesonException(msg.format(minimum))
-    minimum = match.group(0)
     if condition.startswith('>='):
         cmpop = operator.le
         condition = condition[2:]
     elif condition.startswith('<='):
-        return True
-        condition = condition[2:]
+        return False
     elif condition.startswith('!='):
-        return True
-        condition = condition[2:]
+        return False
     elif condition.startswith('=='):
         cmpop = operator.le
         condition = condition[2:]
@@ -483,49 +506,24 @@ def version_compare_condition_with_min(condition, minimum):
         cmpop = operator.lt
         condition = condition[1:]
     elif condition.startswith('<'):
-        return True
-        condition = condition[2:]
-    else:
-        cmpop = operator.le
-    varr1 = grab_leading_numbers(minimum, True)
-    varr2 = grab_leading_numbers(condition, True)
-    make_same_len(varr1, varr2)
-    return cmpop(varr1, varr2)
-
-def version_compare_condition_with_max(condition, maximum):
-    match = numpart.match(maximum.strip())
-    if match is None:
-        msg = 'Uncomparable version string {!r}.'
-        raise MesonException(msg.format(maximum))
-    maximum = match.group(0)
-    if condition.startswith('>='):
-        return False
-        condition = condition[2:]
-    elif condition.startswith('<='):
-        cmpop = operator.ge
-        condition = condition[2:]
-    elif condition.startswith('!='):
         return False
-        condition = condition[2:]
-    elif condition.startswith('=='):
-        cmpop = operator.ge
-        condition = condition[2:]
-    elif condition.startswith('='):
-        cmpop = operator.ge
-        condition = condition[1:]
-    elif condition.startswith('>'):
-        return False
-        condition = condition[1:]
-    elif condition.startswith('<'):
-        cmpop = operator.gt
-        condition = condition[2:]
     else:
-        cmpop = operator.ge
-    varr1 = grab_leading_numbers(maximum, True)
-    varr2 = grab_leading_numbers(condition, True)
-    make_same_len(varr1, varr2)
-    return cmpop(varr1, varr2)
+        cmpop = operator.le
+
+    # Declaring a project(meson_version: '>=0.46') and then using features in
+    # 0.46.0 is valid, because (knowing the meson versioning scheme) '0.46.0' is
+    # the lowest version which satisfies the constraint '>=0.46'.
+    #
+    # But this will fail here, because the minimum version required by the
+    # version constraint ('0.46') is strictly less (in our version comparison)
+    # than the minimum version needed for the feature ('0.46.0').
+    #
+    # Map versions in the constraint of the form '0.46' to '0.46.0', to embed
+    # this knowledge of the meson versioning scheme.
+    if re.match('^\d+.\d+$', condition):
+        condition += '.0'
 
+    return cmpop(Version(minimum), Version(condition))
 
 def default_libdir():
     if is_debianlike():
diff --git a/run_unittests.py b/run_unittests.py
index 79bafcd..96802cc 100755
--- a/run_unittests.py
+++ b/run_unittests.py
@@ -39,7 +39,7 @@ from mesonbuild.interpreter import Interpreter, ObjectHolder
 from mesonbuild.mesonlib import (
     is_windows, is_osx, is_cygwin, is_dragonflybsd, is_openbsd,
     windows_proof_rmtree, python_command, version_compare,
-    grab_leading_numbers, BuildDirLock
+    BuildDirLock, Version
 )
 from mesonbuild.environment import detect_ninja
 from mesonbuild.mesonlib import MesonException, EnvironmentException
@@ -691,6 +691,114 @@ class InternalTests(unittest.TestCase):
             PkgConfigDependency.pkgbin_cache = {}
             PkgConfigDependency.class_pkgbin = None
 
+    def test_version_compare(self):
+        comparefunc = mesonbuild.mesonlib.version_compare_many
+        for (a, b, result) in [
+                ('0.99.beta19', '>= 0.99.beta14', True),
+        ]:
+            self.assertEqual(comparefunc(a, b)[0], result)
+
+        for (a, b, result) in [
+                # examples from https://fedoraproject.org/wiki/Archive:Tools/RPM/VersionComparison
+                ("1.0010", "1.9", 1),
+                ("1.05", "1.5", 0),
+                ("1.0", "1", 1),
+                ("2.50", "2.5", 1),
+                ("fc4", "fc.4", 0),
+                ("FC5", "fc4", -1),
+                ("2a", "2.0", -1),
+                ("1.0", "1.fc4", 1),
+                ("3.0.0_fc", "3.0.0.fc", 0),
+                # from RPM tests
+                ("1.0", "1.0", 0),
+                ("1.0", "2.0", -1),
+                ("2.0", "1.0", 1),
+                ("2.0.1", "2.0.1", 0),
+                ("2.0", "2.0.1", -1),
+                ("2.0.1", "2.0", 1),
+                ("2.0.1a", "2.0.1a", 0),
+                ("2.0.1a", "2.0.1", 1),
+                ("2.0.1", "2.0.1a", -1),
+                ("5.5p1", "5.5p1", 0),
+                ("5.5p1", "5.5p2", -1),
+                ("5.5p2", "5.5p1", 1),
+                ("5.5p10", "5.5p10", 0),
+                ("5.5p1", "5.5p10", -1),
+                ("5.5p10", "5.5p1", 1),
+                ("10xyz", "10.1xyz", -1),
+                ("10.1xyz", "10xyz", 1),
+                ("xyz10", "xyz10", 0),
+                ("xyz10", "xyz10.1", -1),
+                ("xyz10.1", "xyz10", 1),
+                ("xyz.4", "xyz.4", 0),
+                ("xyz.4", "8", -1),
+                ("8", "xyz.4", 1),
+                ("xyz.4", "2", -1),
+                ("2", "xyz.4", 1),
+                ("5.5p2", "5.6p1", -1),
+                ("5.6p1", "5.5p2", 1),
+                ("5.6p1", "6.5p1", -1),
+                ("6.5p1", "5.6p1", 1),
+                ("6.0.rc1", "6.0", 1),
+                ("6.0", "6.0.rc1", -1),
+                ("10b2", "10a1", 1),
+                ("10a2", "10b2", -1),
+                ("1.0aa", "1.0aa", 0),
+                ("1.0a", "1.0aa", -1),
+                ("1.0aa", "1.0a", 1),
+                ("10.0001", "10.0001", 0),
+                ("10.0001", "10.1", 0),
+                ("10.1", "10.0001", 0),
+                ("10.0001", "10.0039", -1),
+                ("10.0039", "10.0001", 1),
+                ("4.999.9", "5.0", -1),
+                ("5.0", "4.999.9", 1),
+                ("20101121", "20101121", 0),
+                ("20101121", "20101122", -1),
+                ("20101122", "20101121", 1),
+                ("2_0", "2_0", 0),
+                ("2.0", "2_0", 0),
+                ("2_0", "2.0", 0),
+                ("a", "a", 0),
+                ("a+", "a+", 0),
+                ("a+", "a_", 0),
+                ("a_", "a+", 0),
+                ("+a", "+a", 0),
+                ("+a", "_a", 0),
+                ("_a", "+a", 0),
+                ("+_", "+_", 0),
+                ("_+", "+_", 0),
+                ("_+", "_+", 0),
+                ("+", "_", 0),
+                ("_", "+", 0),
+                # other tests
+                ('0.99.beta19', '0.99.beta14', 1),
+                ("1.0.0", "2.0.0", -1),
+                (".0.0", "2.0.0", -1),
+                ("alpha", "beta", -1),
+                ("1.0", "1.0.0", -1),
+                ("2.456", "2.1000", -1),
+                ("2.1000", "3.111", -1),
+                ("2.001", "2.1", 0),
+                ("2.34", "2.34", 0),
+                ("6.1.2", "6.3.8", -1),
+                ("1.7.3.0", "2.0.0", -1),
+                ("2.24.51", "2.25", -1),
+                ("2.1.5+20120813+gitdcbe778", "2.1.5", 1),
+                ("3.4.1", "3.4b1", 1),
+                ("041206", "200090325", -1),
+                ("0.6.2+git20130413", "0.6.2", 1),
+                ("2.6.0+bzr6602", "2.6.0", 1),
+                ("2.6.0", "2.6b2", 1),
+                ("2.6.0+bzr6602", "2.6b2x", 1),
+                ("0.6.7+20150214+git3a710f9", "0.6.7", 1),
+                ("15.8b", "15.8.0.1", -1),
+                ("1.2rc1", "1.2.0", -1),
+        ]:
+            ver_a = Version(a)
+            ver_b = Version(b)
+            self.assertEqual(ver_a.__cmp__(ver_b), result)
+            self.assertEqual(ver_b.__cmp__(ver_a), -result)
 
 @unittest.skipIf(is_tarball(), 'Skipping because this is a tarball release')
 class DataTests(unittest.TestCase):
@@ -2552,13 +2660,14 @@ recommended as it is not supported on some platforms''')
         out = self.init(testdir)
         # Parent project warns correctly
         self.assertRegex(out, "WARNING: Project targetting '>=0.45'.*'0.47.0': dict")
-        # Subproject warns correctly
-        self.assertRegex(out, "|WARNING: Project targetting '>=0.40'.*'0.44.0': disabler")
+        # Subprojects warn correctly
+        self.assertRegex(out, r"\|WARNING: Project targetting '>=0.40'.*'0.44.0': disabler")
+        self.assertRegex(out, r"\|WARNING: Project targetting '!=0.40'.*'0.44.0': disabler")
         # Subproject has a new-enough meson_version, no warning
         self.assertNotRegex(out, "WARNING: Project targetting.*Python")
         # Ensure a summary is printed in the subproject and the outer project
-        self.assertRegex(out, "|WARNING: Project specifies a minimum meson_version '>=0.40'")
-        self.assertRegex(out, "| * 0.44.0: {'disabler'}")
+        self.assertRegex(out, r"\|WARNING: Project specifies a minimum meson_version '>=0.40'")
+        self.assertRegex(out, r"\| \* 0.44.0: {'disabler'}")
         self.assertRegex(out, "WARNING: Project specifies a minimum meson_version '>=0.45'")
         self.assertRegex(out, " * 0.47.0: {'dict'}")
 
@@ -2656,7 +2765,7 @@ class FailureTests(BasePlatformTests):
         super().tearDown()
         windows_proof_rmtree(self.srcdir)
 
-    def assertMesonRaises(self, contents, match, extra_args=None, langs=None):
+    def assertMesonRaises(self, contents, match, extra_args=None, langs=None, meson_version=None):
         '''
         Assert that running meson configure on the specified @contents raises
         a error message matching regex @match.
@@ -2664,7 +2773,10 @@ class FailureTests(BasePlatformTests):
         if langs is None:
             langs = []
         with open(self.mbuild, 'w') as f:
-            f.write("project('failure test', 'c', 'cpp')\n")
+            f.write("project('failure test', 'c', 'cpp'")
+            if meson_version:
+                f.write(", meson_version: '{}'".format(meson_version))
+            f.write(")\n")
             for lang in langs:
                 f.write("add_languages('{}', required : false)\n".format(lang))
             f.write(contents)
@@ -2674,13 +2786,14 @@ class FailureTests(BasePlatformTests):
             # Must run in-process or we'll get a generic CalledProcessError
             self.init(self.srcdir, extra_args=extra_args, inprocess=True)
 
-    def obtainMesonOutput(self, contents, match, extra_args, langs, meson_version):
+    def obtainMesonOutput(self, contents, match, extra_args, langs, meson_version=None):
         if langs is None:
             langs = []
         with open(self.mbuild, 'w') as f:
-            core_version = '.'.join([str(component) for component in grab_leading_numbers(mesonbuild.coredata.version)])
-            meson_version = meson_version or core_version
-            f.write("project('output test', 'c', 'cpp', meson_version: '{}')\n".format(meson_version))
+            f.write("project('output test', 'c', 'cpp'")
+            if meson_version:
+                f.write(", meson_version: '{}'".format(meson_version))
+            f.write(")\n")
             for lang in langs:
                 f.write("add_languages('{}', required : false)\n".format(lang))
             f.write(contents)
diff --git a/test cases/unit/41 featurenew subprojects/meson.build b/test cases/unit/41 featurenew subprojects/meson.build
index 27898cd..d136bed 100644
--- a/test cases/unit/41 featurenew subprojects/meson.build
+++ b/test cases/unit/41 featurenew subprojects/meson.build
@@ -4,3 +4,4 @@ foo = {}
 
 subproject('foo')
 subproject('bar')
+subproject('baz')
diff --git a/test cases/unit/41 featurenew subprojects/subprojects/baz/meson.build b/test cases/unit/41 featurenew subprojects/subprojects/baz/meson.build
new file mode 100644
index 0000000..811e7aa
--- /dev/null
+++ b/test cases/unit/41 featurenew subprojects/subprojects/baz/meson.build
@@ -0,0 +1,3 @@
+project('baz subproject', meson_version: '!=0.40')
+
+disabler()