extend Version class so that 2.0 > 1.develop > 1.1 and develop > master > head > trunk > 9999 (#1983)

* extend Version class so that 2.0 > 1.develop > 1.1

* add concretization tests, with preferences and preferred version.

* add master, head, trunk as develop-like versions, develop > master > head > trunk

* update documentation on version comparison

lib/spack/docs/packaging_guide.rst

@@ -590,13 +590,15 @@ with `RPM <https://bugzilla.redhat.com/show_bug.cgi?id=50977>`_.
 
 Spack versions may also be arbitrary non-numeric strings; any string
 here will suffice; for example, ``@develop``, ``@master``, ``@local``.
-The following rules determine the sort order of numeric
+Versions are compared as follows. First, a version string is split into
-vs. non-numeric versions:
+multiple fields based on delimiters such as ``.``, ``-`` etc. Then
+matching fields are compared using the rules below:
 
-#. The non-numeric version ``@develop`` is considered greatest (newest).
+#. The following develop-like strings are greater (newer) than all
+   numbers and are ordered as ``develop > master > head > trunk``.
 
-#. Numeric versions are all less than ``@develop`` version, and are
+#. Numbers are all less than the chosen develop-like strings above,
-   sorted numerically.
+   and are sorted numerically.
 
 #. All other non-numeric versions are less than numeric versions, and
    are sorted alphabetically.
@@ -610,7 +612,7 @@ The logic behind this sort order is two-fold:
 
 #. The most-recent development version of a package will usually be
    newer than any released numeric versions.  This allows the
-   ``develop`` version to satisfy dependencies like ``depends_on(abc,
+   ``@develop`` version to satisfy dependencies like ``depends_on(abc,
    when="@x.y.z:")``
 
 ^^^^^^^^^^^^^^^^^

lib/spack/spack/test/concretize_preferences.py

@@ -124,12 +124,40 @@ def test_preferred_providers(self):
         spec = concretize('mpileaks')
         assert 'zmpi' in spec
 
-    def test_develop(self):
+    def test_preferred(self):
-        """Test concretization with develop version"""
+        """"Test packages with some version marked as preferred=True"""
-        spec = Spec('builtin.mock.develop-test')
+        spec = Spec('preferred-test')
         spec.concretize()
         assert spec.version == spack.spec.Version('0.2.15')
 
+        # now add packages.yaml with versions other than preferred
+        # ensure that once config is in place, non-preferred version is used
+        update_packages('preferred-test', 'version', ['0.2.16'])
+        spec = Spec('preferred-test')
+        spec.concretize()
+        assert spec.version == spack.spec.Version('0.2.16')
+
+    def test_develop(self):
+        """Test concretization with develop-like versions"""
+        spec = Spec('develop-test')
+        spec.concretize()
+        assert spec.version == spack.spec.Version('0.2.15')
+        spec = Spec('develop-test2')
+        spec.concretize()
+        assert spec.version == spack.spec.Version('0.2.15')
+
+        # now add packages.yaml with develop-like versions
+        # ensure that once config is in place, develop-like version is used
+        update_packages('develop-test', 'version', ['develop'])
+        spec = Spec('develop-test')
+        spec.concretize()
+        assert spec.version == spack.spec.Version('develop')
+
+        update_packages('develop-test2', 'version', ['0.2.15.develop'])
+        spec = Spec('develop-test2')
+        spec.concretize()
+        assert spec.version == spack.spec.Version('0.2.15.develop')
+
     def test_no_virtuals_in_packages_yaml(self):
         """Verify that virtuals are not allowed in packages.yaml."""
 

lib/spack/spack/test/versions.py

@@ -90,13 +90,62 @@ def check_union(expected, a, b):
     assert ver(expected) == ver(a).union(ver(b))
 
 
+def test_string_prefix():
+    assert_ver_eq('xsdk-0.2.0', 'xsdk-0.2.0')
+    assert_ver_lt('xsdk-0.2.0', 'xsdk-0.3')
+    assert_ver_gt('xsdk-0.3', 'xsdk-0.2.0')
+
+
 def test_two_segments():
     assert_ver_eq('1.0', '1.0')
     assert_ver_lt('1.0', '2.0')
     assert_ver_gt('2.0', '1.0')
+
+
+def test_develop():
     assert_ver_eq('develop', 'develop')
+    assert_ver_eq('develop.local', 'develop.local')
     assert_ver_lt('1.0', 'develop')
     assert_ver_gt('develop', '1.0')
+    assert_ver_eq('1.develop', '1.develop')
+    assert_ver_lt('1.1', '1.develop')
+    assert_ver_gt('1.develop', '1.0')
+    assert_ver_gt('0.5.develop', '0.5')
+    assert_ver_lt('0.5', '0.5.develop')
+    assert_ver_lt('1.develop', '2.1')
+    assert_ver_gt('2.1', '1.develop')
+    assert_ver_lt('1.develop.1', '1.develop.2')
+    assert_ver_gt('1.develop.2', '1.develop.1')
+    assert_ver_lt('develop.1', 'develop.2')
+    assert_ver_gt('develop.2', 'develop.1')
+    # other +infinity versions
+    assert_ver_gt('master', '9.0')
+    assert_ver_gt('head', '9.0')
+    assert_ver_gt('trunk', '9.0')
+    assert_ver_gt('develop', '9.0')
+    # hierarchical develop-like versions
+    assert_ver_gt('develop', 'master')
+    assert_ver_gt('master', 'head')
+    assert_ver_gt('head', 'trunk')
+    assert_ver_gt('9.0', 'system')
+    # not develop
+    assert_ver_lt('mydevelopmentnightmare', '1.1')
+    assert_ver_lt('1.mydevelopmentnightmare', '1.1')
+    assert_ver_gt('1.1', '1.mydevelopmentnightmare')
+
+
+def test_isdevelop():
+    assert ver('develop').isdevelop()
+    assert ver('develop.1').isdevelop()
+    assert ver('develop.local').isdevelop()
+    assert ver('master').isdevelop()
+    assert ver('head').isdevelop()
+    assert ver('trunk').isdevelop()
+    assert ver('1.develop').isdevelop()
+    assert ver('1.develop.2').isdevelop()
+    assert not ver('1.1').isdevelop()
+    assert not ver('1.mydevelopmentnightmare.3').isdevelop()
+    assert not ver('mydevelopmentnightmare.3').isdevelop()
 
 
 def test_three_segments():

lib/spack/spack/version.py

@@ -38,6 +38,9 @@
 # Valid version characters
 VALID_VERSION = r'[A-Za-z0-9_.-]'
 
+# Infinity-like versions. The order in the list implies the comparision rules
+infinity_versions = ['develop', 'master', 'head', 'trunk']
+
 
 def int_if_int(string):
     """Convert a string to int if possible.  Otherwise, return a string."""
@@ -199,26 +202,14 @@ def lowest(self):
     def highest(self):
         return self
 
-    def isnumeric(self):
-        """Tells if this version is numeric (vs. a non-numeric version).  A
-        version will be numeric as long as the first section of it is,
-        even if it contains non-numerica portions.
-
-        Some numeric versions:
-            1
-            1.1
-            1.1a
-            1.a.1b
-        Some non-numeric versions:
-            develop
-            system
-            myfavoritebranch
-        """
-        return isinstance(self.version[0], numbers.Integral)
-
     def isdevelop(self):
-        """Triggers on the special case of the `@develop` version."""
+        """Triggers on the special case of the `@develop-like` version."""
-        return self.string == 'develop'
+        for inf in infinity_versions:
+            for v in self.version:
+                if v == inf:
+                    return True
+
+        return False
 
     @coerced
     def satisfies(self, other):
@@ -272,27 +263,6 @@ def __format__(self, format_spec):
     def concrete(self):
         return self
 
-    def _numeric_lt(self, other):
-        """Compares two versions, knowing they're both numeric"""
-        # Standard comparison of two numeric versions
-        for a, b in zip(self.version, other.version):
-            if a == b:
-                continue
-            else:
-                # Numbers are always "newer" than letters.
-                # This is for consistency with RPM.  See patch
-                # #60884 (and details) from bugzilla #50977 in
-                # the RPM project at rpm.org.  Or look at
-                # rpmvercmp.c if you want to see how this is
-                # implemented there.
-                if type(a) != type(b):
-                    return type(b) == int
-                else:
-                    return a < b
-        # If the common prefix is equal, the one
-        # with more segments is bigger.
-        return len(self.version) < len(other.version)
-
     @coerced
     def __lt__(self, other):
         """Version comparison is designed for consistency with the way RPM
@@ -308,33 +278,35 @@ def __lt__(self, other):
         if self.version == other.version:
             return False
 
-        # First priority: anything < develop
+        # Standard comparison of two numeric versions
-        sdev = self.isdevelop()
+        for a, b in zip(self.version, other.version):
-        if sdev:
+            if a == b:
-            return False    # source = develop, it can't be < anything
+                continue
-
-        # Now we know !sdev
-        odev = other.isdevelop()
-        if odev:
-            return True    # src < dst
-
-        # now we know neither self nor other isdevelop().
-
-        # Principle: Non-numeric is less than numeric
-        # (so numeric will always be preferred by default)
-        if self.isnumeric():
-            if other.isnumeric():
-                return self._numeric_lt(other)
-            else:    # self = numeric; other = non-numeric
-                # Numeric > Non-numeric (always)
-                return False
             else:
-            if other.isnumeric():  # self = non-numeric, other = numeric
+                if a in infinity_versions:
-                # non-numeric < numeric (always)
+                    if b in infinity_versions:
+                        return (infinity_versions.index(a) >
+                                infinity_versions.index(b))
+                    else:
+                        return False
+                if b in infinity_versions:
                     return True
-            else:  # Both non-numeric
+
-                # Maybe consider other ways to compare here...
+                # Neither a nor b is infinity
-                return self.string < other.string
+                # Numbers are always "newer" than letters.
+                # This is for consistency with RPM.  See patch
+                # #60884 (and details) from bugzilla #50977 in
+                # the RPM project at rpm.org.  Or look at
+                # rpmvercmp.c if you want to see how this is
+                # implemented there.
+                if type(a) != type(b):
+                    return type(b) == int
+                else:
+                    return a < b
+
+        # If the common prefix is equal, the one
+        # with more segments is bigger.
+        return len(self.version) < len(other.version)
 
     @coerced
     def __eq__(self, other):

var/spack/repos/builtin.mock/packages/develop-test2/package.py

@@ -0,0 +1,18 @@
+# Copyright 2013-2019 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack import *
+
+
+class DevelopTest2(Package):
+    """Dummy package with develop version"""
+    homepage = "http://www.openblas.net"
+    url      = "http://github.com/xianyi/OpenBLAS/archive/v0.2.15.tar.gz"
+
+    version('0.2.15.develop', git='https://github.com/dummy/repo.git')
+    version('0.2.15', 'b1190f3d3471685f17cfd1ec1d252ac9')
+
+    def install(self, spec, prefix):
+        pass

var/spack/repos/builtin.mock/packages/preferred-test/package.py

@@ -0,0 +1,20 @@
+# Copyright 2013-2019 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack import *
+
+
+class PreferredTest(Package):
+    """Dummy package with develop version and preffered version"""
+    homepage = "http://www.openblas.net"
+    url      = "http://github.com/xianyi/OpenBLAS/archive/v0.2.15.tar.gz"
+
+    version('develop', git='https://github.com/dummy/repo.git')
+    version('0.2.16', 'b1190f3d3471685f17cfd1ec1d252ac9')
+    version('0.2.15', 'b1190f3d3471685f17cfd1ec1d252ac9', preferred=True)
+    version('0.2.14', 'b1190f3d3471685f17cfd1ec1d252ac9')
+
+    def install(self, spec, prefix):
+        pass