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

6 files changed, 161 insertions, 72 deletions

diff --git a/lib/spack/docs/packaging_guide.rst b/lib/spack/docs/packaging_guide.rst
index 641bb74fea..32fc484eb5 100644
--- a/lib/spack/docs/packaging_guide.rst
+++ b/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
-vs. non-numeric versions:
+Versions are compared as follows. First, a version string is split into
+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
-   sorted numerically.
+#. Numbers are all less than the chosen develop-like strings above,
+   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:")``
 
 ^^^^^^^^^^^^^^^^^
diff --git a/lib/spack/spack/test/concretize_preferences.py b/lib/spack/spack/test/concretize_preferences.py
index eb441e006e..e7c8727371 100644
--- a/lib/spack/spack/test/concretize_preferences.py
+++ b/lib/spack/spack/test/concretize_preferences.py
@@ -124,11 +124,39 @@ class TestConcretizePreferences(object):
         spec = concretize('mpileaks')
         assert 'zmpi' in spec
 
+    def test_preferred(self):
+        """"Test packages with some version marked as preferred=True"""
+        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 version"""
-        spec = Spec('builtin.mock.develop-test')
+        """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."""
diff --git a/lib/spack/spack/test/versions.py b/lib/spack/spack/test/versions.py
index bf060c2f83..11da83df8c 100644
--- a/lib/spack/spack/test/versions.py
+++ b/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():
diff --git a/lib/spack/spack/version.py b/lib/spack/spack/version.py
index a73b61d347..5abe6b84e2 100644
--- a/lib/spack/spack/version.py
+++ b/lib/spack/spack/version.py
@@ -38,6 +38,9 @@ __all__ = ['Version', 'VersionRange', 'VersionList', 'ver']
 # 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 @@ class Version(object):
     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."""
-        return self.string == 'develop'
+        """Triggers on the special case of the `@develop-like` version."""
+        for inf in infinity_versions:
+            for v in self.version:
+                if v == inf:
+                    return True
+
+        return False
 
     @coerced
     def satisfies(self, other):
@@ -272,13 +263,36 @@ class Version(object):
     def concrete(self):
         return self
 
-    def _numeric_lt(self, other):
-        """Compares two versions, knowing they're both numeric"""
+    @coerced
+    def __lt__(self, other):
+        """Version comparison is designed for consistency with the way RPM
+           does things.  If you need more complicated versions in installed
+           packages, you should override your package's version string to
+           express it more sensibly.
+        """
+        if other is None:
+            return False
+
+        # Coerce if other is not a Version
+        # simple equality test first.
+        if self.version == other.version:
+            return False
+
         # Standard comparison of two numeric versions
         for a, b in zip(self.version, other.version):
             if a == b:
                 continue
             else:
+                if a in infinity_versions:
+                    if b in infinity_versions:
+                        return (infinity_versions.index(a) >
+                                infinity_versions.index(b))
+                    else:
+                        return False
+                if b in infinity_versions:
+                    return True
+
+                # Neither a nor b is infinity
                 # Numbers are always "newer" than letters.
                 # This is for consistency with RPM.  See patch
                 # #60884 (and details) from bugzilla #50977 in
@@ -289,54 +303,12 @@ class Version(object):
                     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
-           does things.  If you need more complicated versions in installed
-           packages, you should override your package's version string to
-           express it more sensibly.
-        """
-        if other is None:
-            return False
-
-        # Coerce if other is not a Version
-        # simple equality test first.
-        if self.version == other.version:
-            return False
-
-        # First priority: anything < develop
-        sdev = self.isdevelop()
-        if sdev:
-            return False    # source = develop, it can't be < anything
-
-        # 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
-                # non-numeric < numeric (always)
-                return True
-            else:  # Both non-numeric
-                # Maybe consider other ways to compare here...
-                return self.string < other.string
-
-    @coerced
     def __eq__(self, other):
         return (other is not None and
                 type(other) == Version and self.version == other.version)
diff --git a/var/spack/repos/builtin.mock/packages/develop-test2/package.py b/var/spack/repos/builtin.mock/packages/develop-test2/package.py
new file mode 100644
index 0000000000..762a82f838
--- /dev/null
+++ b/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
diff --git a/var/spack/repos/builtin.mock/packages/preferred-test/package.py b/var/spack/repos/builtin.mock/packages/preferred-test/package.py
new file mode 100644
index 0000000000..29da42242d
--- /dev/null
+++ b/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