Fix Issues with non-numeric versions, as well as preferred=True (#1561)

* Fix bug in handling of precedence of preferred=True vs. versions given in packages.yaml (#1556)

* Standardized comparison of versions: numeric versions are always greater than non-numeric versions; and non-numeric versions are sorted alphabetically.

This is
   a) simple
   b) ensures that non-numeric versions (such as 'develop') in package.py are not chosen ahead of numeric versions, when nothing is specified in packages.yaml

Fixes Issue #1557

* Removed debugging output

* Fix variable shadowing bug

* Ensure develop < numeric version.

* Bug fix.

* Passes all unit tests in versions.py

* flake8 fixes

* flake8 fixes

* Changed type test to be more correct.

See http://stackoverflow.com/questions/8203336/difference-between-int-and-numbers-integral-in-python

3 files changed, 113 insertions, 39 deletions

diff --git a/lib/spack/spack/concretize.py b/lib/spack/spack/concretize.py
index 726dee62e3..cf8cf2965c 100644
--- a/lib/spack/spack/concretize.py
+++ b/lib/spack/spack/concretize.py
@@ -33,6 +33,7 @@ or user preferences.
 TODO: make this customizable and allow users to configure
       concretization  policies.
 """
+from __future__ import print_function
 import spack
 import spack.spec
 import spack.compilers
@@ -42,6 +43,7 @@ from spack.version import *
 from functools import partial
 from itertools import chain
 from spack.config import *
+import spack.preferred_packages
 
 
 class DefaultConcretizer(object):
@@ -160,23 +162,43 @@ class DefaultConcretizer(object):
         # If there are known available versions, return the most recent
         # version that satisfies the spec
         pkg = spec.package
-        cmp_versions = partial(spack.pkgsort.version_compare, spec.name)
-        valid_versions = sorted(
-            [v for v in pkg.versions
-             if any(v.satisfies(sv) for sv in spec.versions)],
-            cmp=cmp_versions)
 
-        def prefer_key(v):
-            return pkg.versions.get(Version(v)).get('preferred', False)
-        valid_versions.sort(key=prefer_key, reverse=True)
+        # ---------- Produce prioritized list of versions
+        # Get list of preferences from packages.yaml
+        preferred = spack.pkgsort
+        # NOTE: spack.pkgsort == spack.preferred_packages.PreferredPackages()
+
+        yaml_specs = [
+            x[0] for x in
+            preferred._spec_for_pkgname(spec.name, 'version', None)]
+        n = len(yaml_specs)
+        yaml_index = dict(
+            [(spc, n - index) for index, spc in enumerate(yaml_specs)])
+
+        # List of versions we could consider, in sorted order
+        unsorted_versions = [
+            v for v in pkg.versions
+            if any(v.satisfies(sv) for sv in spec.versions)]
+
+        keys = [(
+            # Respect order listed in packages.yaml
+            yaml_index.get(v, -1),
+            # The preferred=True flag (packages or packages.yaml or both?)
+            pkg.versions.get(Version(v)).get('preferred', False),
+            # @develop special case
+            v.isdevelop(),
+            # Numeric versions more preferred than non-numeric
+            v.isnumeric(),
+            # Compare the version itself
+            v) for v in unsorted_versions]
+        keys.sort(reverse=True)
+
+        # List of versions in complete sorted order
+        valid_versions = [x[-1] for x in keys]
+        # --------------------------
 
         if valid_versions:
-            # Disregard @develop and take the next valid version
-            if ver(valid_versions[0]) == ver('develop') and \
-                    len(valid_versions) > 1:
-                spec.versions = ver([valid_versions[1]])
-            else:
-                spec.versions = ver([valid_versions[0]])
+            spec.versions = ver([valid_versions[0]])
         else:
             # We don't know of any SAFE versions that match the given
             # spec.  Grab the spec's versions and grab the highest
@@ -255,7 +277,7 @@ class DefaultConcretizer(object):
             spec.architecture = spack.architecture.Arch()
             return True
 
-            # Concretize the operating_system and target based of the spec
+        # Concretize the operating_system and target based of the spec
         ret = any((self._concretize_platform(spec),
                    self._concretize_operating_system(spec),
                    self._concretize_target(spec)))
diff --git a/lib/spack/spack/test/versions.py b/lib/spack/spack/test/versions.py
index 41d72e7c34..9b4dc29f35 100644
--- a/lib/spack/spack/test/versions.py
+++ b/lib/spack/spack/test/versions.py
@@ -428,3 +428,6 @@ class VersionsTest(unittest.TestCase):
         self.assertEqual(str(b), '1_2-3')
         # Raise TypeError on tuples
         self.assertRaises(TypeError, b.__getitem__, 1, 2)
+
+if __name__ == '__main__':
+    unittest.main()
diff --git a/lib/spack/spack/version.py b/lib/spack/spack/version.py
index e3efa6c87a..67a22f4660 100644
--- a/lib/spack/spack/version.py
+++ b/lib/spack/spack/version.py
@@ -107,6 +107,10 @@ def coerced(method):
     return coercing_method
 
 
+def _numeric_lt(self0, other):
+    """Compares two versions, knowing they're both numeric"""
+
+
 @total_ordering
 class Version(object):
     """Class to represent versions"""
@@ -154,6 +158,27 @@ 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'
+
     @coerced
     def satisfies(self, other):
         """A Version 'satisfies' another if it is at least as specific and has
@@ -225,6 +250,27 @@ class Version(object):
     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
@@ -240,30 +286,33 @@ class Version(object):
         if self.version == other.version:
             return False
 
-        # dev is __gt__ than anything but itself.
-        if other.string == 'develop':
-            return True
-
-        # If lhs is dev then it can't be < than anything
-        if self.string == 'develop':
-            return False
-
-        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)
+        # 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):