Use key sorting instead of cmp()

- Get rid of pkgsort() usage for preferred variants.
- Concretization is now entirely based on key-based sorting.
- Remove PreferredPackages class and various spec cmp() methods.
- Replace with PackagePrefs class that implements a key function for
  sorting according to packages.yaml.
- Clear package pref caches on config test.
- Explicit compare methods instead of total_ordering in Version.
- Our total_ordering backport wasn't making Python 3 happy for some
  reason.
- Python 3's functools.total_ordering and spelling the operators out
  fixes the problem.
- Fix unicode issues with spec hashes, json, & YAML
- Try to use str everywhere and avoid unicode objects in python 2.

lib/spack/external/functools_backport.py

@@ -28,3 +28,20 @@ def total_ordering(cls):
             opfunc.__doc__ = getattr(int, opname).__doc__
             setattr(cls, opname, opfunc)
     return cls
+
+
+@total_ordering
+class reverse_order(object):
+    """Helper for creating key functions.
+
+       This is a wrapper that inverts the sense of the natural
+       comparisons on the object.
+    """
+    def __init__(self, value):
+        self.value = value
+
+    def __eq__(self, other):
+        return other.value == self.value
+
+    def __lt__(self, other):
+        return other.value < self.value

lib/spack/llnl/util/lang.py

@@ -33,6 +33,12 @@
 ignore_modules = [r'^\.#', '~$']
 
 
+class classproperty(property):
+    """classproperty decorator: like property but for classmethods."""
+    def __get__(self, cls, owner):
+        return self.fget.__get__(None, owner)()
+
+
 def index_by(objects, *funcs):
     """Create a hierarchy of dictionaries by splitting the supplied
        set of objects on unique values of the supplied functions.

lib/spack/spack/__init__.py

@@ -78,7 +78,6 @@
 import spack.config
 import spack.fetch_strategy
 from spack.file_cache import FileCache
-from spack.package_prefs import PreferredPackages
 from spack.abi import ABI
 from spack.concretize import DefaultConcretizer
 from spack.version import Version

lib/spack/spack/concretize.py

@@ -35,87 +35,77 @@
 """
 from __future__ import print_function
 from six import iteritems
+from spack.version import *
+from itertools import chain
+from ordereddict_backport import OrderedDict
+from functools_backport import reverse_order
 
 import spack
 import spack.spec
 import spack.compilers
 import spack.architecture
 import spack.error
-from spack.version import *
-from functools import partial
-from itertools import chain
 from spack.package_prefs import *
 
 
 class DefaultConcretizer(object):
-
     """This class doesn't have any state, it just provides some methods for
        concretization.  You can subclass it to override just some of the
        default concretization strategies, or you can override all of them.
     """
-
     def _valid_virtuals_and_externals(self, spec):
         """Returns a list of candidate virtual dep providers and external
-           packages that coiuld be used to concretize a spec."""
+           packages that coiuld be used to concretize a spec.
+
+           Preferred specs come first in the list.
+        """
         # First construct a list of concrete candidates to replace spec with.
         candidates = [spec]
+        pref_key = lambda spec: 0  # no-op pref key
+
         if spec.virtual:
-            providers = spack.repo.providers_for(spec)
+            candidates = spack.repo.providers_for(spec)
-            if not providers:
+            if not candidates:
-                raise UnsatisfiableProviderSpecError(providers[0], spec)
+                raise UnsatisfiableProviderSpecError(candidates[0], spec)
-            spec_w_preferred_providers = find_spec(
+
-                spec,
+            # Find nearest spec in the DAG (up then down) that has prefs.
-                lambda x: pkgsort().spec_has_preferred_provider(
+            spec_w_prefs = find_spec(
-                    x.name, spec.name))
+                spec, lambda p: PackagePrefs.has_preferred_providers(
-            if not spec_w_preferred_providers:
+                    p.name, spec.name),
-                spec_w_preferred_providers = spec
+                spec)  # default to spec itself.
-            provider_cmp = partial(pkgsort().provider_compare,
+
-                                   spec_w_preferred_providers.name,
+            # Create a key to sort candidates by the prefs we found
-                                   spec.name)
+            pref_key = PackagePrefs(spec_w_prefs.name, 'providers', spec.name)
-            candidates = sorted(providers, cmp=provider_cmp)
 
         # For each candidate package, if it has externals, add those
         # to the usable list.  if it's not buildable, then *only* add
         # the externals.
-        usable = []
+        #
+        # Use an OrderedDict to avoid duplicates (use it like a set)
+        usable = OrderedDict()
         for cspec in candidates:
             if is_spec_buildable(cspec):
-                usable.append(cspec)
+                usable[cspec] = True
+
             externals = spec_externals(cspec)
             for ext in externals:
                 if ext.satisfies(spec):
-                    usable.append(ext)
+                    usable[ext] = True
 
         # If nothing is in the usable list now, it's because we aren't
         # allowed to build anything.
         if not usable:
             raise NoBuildError(spec)
 
-        def cmp_externals(a, b):
+        # Use a sort key to order the results
-            if a.name != b.name and (not a.external or a.external_module and
+        return sorted(usable, key=lambda spec: (
-                                     not b.external and b.external_module):
+            not (spec.external or spec.external_module),  # prefer externals
-                # We're choosing between different providers, so
+            pref_key(spec),                               # respect prefs
-                # maintain order from provider sort
+            spec.name,                                    # group by name
-                index_of_a = next(i for i in range(0, len(candidates))
+            reverse_order(spec.versions),                 # latest version
-                                  if a.satisfies(candidates[i]))
+            spec                                          # natural order
-                index_of_b = next(i for i in range(0, len(candidates))
+        ))
-                                  if b.satisfies(candidates[i]))
-                return index_of_a - index_of_b
 
-            result = cmp_specs(a, b)
-            if result != 0:
-                return result
-
-            # prefer external packages to internal packages.
-            if a.external is None or b.external is None:
-                return -cmp(a.external, b.external)
-            else:
-                return cmp(a.external, b.external)
-
-        usable.sort(cmp=cmp_externals)
-        return usable
-
-    # XXX(deptypes): Look here.
     def choose_virtual_or_external(self, spec):
         """Given a list of candidate virtual and external packages, try to
            find one that is most ABI compatible.
@@ -126,25 +116,16 @@ def choose_virtual_or_external(self, spec):
 
         # Find the nearest spec in the dag that has a compiler.  We'll
         # use that spec to calibrate compiler compatibility.
-        abi_exemplar = find_spec(spec, lambda x: x.compiler)
+        abi_exemplar = find_spec(spec, lambda x: x.compiler, spec.root)
-        if not abi_exemplar:
-            abi_exemplar = spec.root
-
-        # Make a list including ABI compatibility of specs with the exemplar.
-        strict = [spack.abi.compatible(c, abi_exemplar) for c in candidates]
-        loose = [spack.abi.compatible(c, abi_exemplar, loose=True)
-                 for c in candidates]
-        keys = zip(strict, loose, candidates)
 
         # Sort candidates from most to least compatibility.
-        # Note:
+        #   We reverse because True > False.
-        #   1. We reverse because True > False.
+        #   Sort is stable, so candidates keep their order.
-        #   2. Sort is stable, so c's keep their order.
+        return sorted(candidates,
-        keys.sort(key=lambda k: k[:2], reverse=True)
+                      reverse=True,
-
+                      key=lambda spec: (
-        # Pull the candidates back out and return them in order
+                          spack.abi.compatible(spec, abi_exemplar, loose=True),
-        candidates = [c for s, l, c in keys]
+                          spack.abi.compatible(spec, abi_exemplar)))
-        return candidates
 
     def concretize_version(self, spec):
         """If the spec is already concrete, return.  Otherwise take
@@ -164,26 +145,12 @@ def concretize_version(self, spec):
         if spec.versions.concrete:
             return False
 
-        # If there are known available versions, return the most recent
-        # version that satisfies the spec
-        pkg = spec.package
-
-        # ---------- Produce prioritized list of versions
-        # Get list of preferences from packages.yaml
-        preferred = pkgsort()
-        # NOTE: pkgsort() == spack.package_prefs.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 = [
+        pkg = spec.package
-            v for v in pkg.versions
+        usable = [v for v in pkg.versions
-            if any(v.satisfies(sv) for sv in spec.versions)]
+                  if any(v.satisfies(sv) for sv in spec.versions)]
+
+        yaml_prefs = PackagePrefs(spec.name, 'version')
 
         # The keys below show the order of precedence of factors used
         # to select a version when concretizing.  The item with
@@ -191,12 +158,11 @@ def concretize_version(self, spec):
         #
         # NOTE: When COMPARING VERSIONS, the '@develop' version is always
         #       larger than other versions.  BUT when CONCRETIZING,
-        #       the largest NON-develop version is selected by
+        #       the largest NON-develop version is selected by default.
-        #       default.
+        keyfn = lambda v: (
-        keys = [(
             # ------- Special direction from the user
             # Respect order listed in packages.yaml
-            yaml_index.get(v, -1),
+            -yaml_prefs(v),
 
             # The preferred=True flag (packages or packages.yaml or both?)
             pkg.versions.get(Version(v)).get('preferred', False),
@@ -211,15 +177,11 @@ def concretize_version(self, spec):
             #    a) develop > everything (disabled by "not v.isdevelop() above)
             #    b) numeric > non-numeric
             #    c) Numeric or string comparison
-            v) for v in unsorted_versions]
+            v)
-        keys.sort(reverse=True)
+        usable.sort(key=keyfn, reverse=True)
 
-        # List of versions in complete sorted order
+        if usable:
-        valid_versions = [x[-1] for x in keys]
+            spec.versions = ver([usable[0]])
-        # --------------------------
-
-        if valid_versions:
-            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
@@ -278,16 +240,15 @@ def concretize_variants(self, spec):
            the package specification.
         """
         changed = False
-        preferred_variants = pkgsort().spec_preferred_variants(
+        preferred_variants = PackagePrefs.preferred_variants(spec.name)
-            spec.package_class.name)
         for name, variant in spec.package_class.variants.items():
             if name not in spec.variants:
                 changed = True
                 if name in preferred_variants:
                     spec.variants[name] = preferred_variants.get(name)
                 else:
-                    spec.variants[name] = \
+                    spec.variants[name] = spack.spec.VariantSpec(
-                        spack.spec.VariantSpec(name, variant.default)
+                        name, variant.default)
         return changed
 
     def concretize_compiler(self, spec):
@@ -329,12 +290,9 @@ def _proper_compiler_style(cspec, aspec):
                     spec.compiler, spec.architecture)
             return False
 
-        # Find the another spec that has a compiler, or the root if none do
+        # Find another spec that has a compiler, or the root if none do
         other_spec = spec if spec.compiler else find_spec(
-            spec, lambda x: x.compiler)
+            spec, lambda x: x.compiler, spec.root)
-
-        if not other_spec:
-            other_spec = spec.root
         other_compiler = other_spec.compiler
         assert(other_spec)
 
@@ -353,9 +311,9 @@ def _proper_compiler_style(cspec, aspec):
         if not compiler_list:
             # No compiler with a satisfactory spec was found
             raise UnavailableCompilerVersionError(other_compiler)
-        cmp_compilers = partial(
+
-            pkgsort().compiler_compare, other_spec.name)
+        ppk = PackagePrefs(other_spec.name, 'compiler')
-        matches = sorted(compiler_list, cmp=cmp_compilers)
+        matches = sorted(compiler_list, key=ppk)
 
         # copy concrete version into other_compiler
         try:
@@ -420,7 +378,7 @@ def concretize_compiler_flags(self, spec):
         return ret
 
 
-def find_spec(spec, condition):
+def find_spec(spec, condition, default=None):
     """Searches the dag from spec in an intelligent order and looks
        for a spec that matches a condition"""
     # First search parents, then search children
@@ -447,7 +405,7 @@ def find_spec(spec, condition):
     if condition(spec):
         return spec
 
-    return None   # Nothing matched the condition.
+    return default   # Nothing matched the condition; return default.
 
 
 def _compiler_concretization_failure(compiler_spec, arch):
@@ -466,7 +424,7 @@ def _compiler_concretization_failure(compiler_spec, arch):
 class NoCompilersForArchError(spack.error.SpackError):
     def __init__(self, arch, available_os_targets):
         err_msg = ("No compilers found"
-                   " for operating system %s and target %s." 
+                   " for operating system %s and target %s."
                    "\nIf previous installations have succeeded, the"
                    " operating system may have been updated." %
                    (arch.platform_os, arch.target))
@@ -485,7 +443,6 @@ def __init__(self, arch, available_os_targets):
 
 
 class UnavailableCompilerVersionError(spack.error.SpackError):
-
     """Raised when there is no available compiler that satisfies a
        compiler spec."""
 
@@ -500,7 +457,6 @@ def __init__(self, compiler_spec, arch=None):
 
 
 class NoValidVersionError(spack.error.SpackError):
-
     """Raised when there is no way to have a concrete version for a
        particular spec."""
 

lib/spack/spack/fetch_strategy.py

@@ -90,7 +90,6 @@ class FetchStrategy(with_metaclass(FSMeta, object)):
     enabled = False  # Non-abstract subclasses should be enabled.
     required_attributes = None  # Attributes required in version() args.
 
-
     def __init__(self):
         # The stage is initialized late, so that fetch strategies can be
         # constructed at package construction time.  This is where things

lib/spack/spack/package_prefs.py

@@ -25,11 +25,22 @@
 from six import string_types
 from six import iteritems
 
+from llnl.util.lang import classproperty
+
 import spack
 import spack.error
 from spack.version import *
 
 
+_lesser_spec_types = {'compiler': spack.spec.CompilerSpec,
+                      'version': VersionList}
+
+
+def _spec_type(component):
+    """Map from component name to spec type for package prefs."""
+    return _lesser_spec_types.get(component, spack.spec.Spec)
+
+
 def get_packages_config():
     """Wrapper around get_packages_config() to validate semantics."""
     config = spack.config.get_config('packages')
@@ -51,177 +62,141 @@ def get_packages_config():
     return config
 
 
-class PreferredPackages(object):
+class PackagePrefs(object):
-    def __init__(self):
+    """Defines the sort order for a set of specs.
-        self.preferred = get_packages_config()
-        self._spec_for_pkgname_cache = {}
 
-    # Given a package name, sort component (e.g, version, compiler, ...), and
+    Spack's package preference implementation uses PackagePrefss to
-    # a second_key (used by providers), return the list
+    define sort order. The PackagePrefs class looks at Spack's
-    def _order_for_package(self, pkgname, component, second_key,
+    packages.yaml configuration and, when called on a spec, returns a key
-                           test_all=True):
+    that can be used to sort that spec in order of the user's
+    preferences.
+
+    You can use it like this:
+
+       # key function sorts CompilerSpecs for `mpich` in order of preference
+       kf = PackagePrefs('mpich', 'compiler')
+       compiler_list.sort(key=kf)
+
+    Or like this:
+
+       # key function to sort VersionLists for OpenMPI in order of preference.
+       kf = PackagePrefs('openmpi', 'version')
+       version_list.sort(key=kf)
+
+    Optionally, you can sort in order of preferred virtual dependency
+    providers.  To do that, provide 'providers' and a third argument
+    denoting the virtual package (e.g., ``mpi``):
+
+       kf = PackagePrefs('trilinos', 'providers', 'mpi')
+       provider_spec_list.sort(key=kf)
+
+    """
+    _packages_config_cache = None
+    _spec_cache = {}
+
+    def __init__(self, pkgname, component, vpkg=None):
+        self.pkgname = pkgname
+        self.component = component
+        self.vpkg = vpkg
+
+    def __call__(self, spec):
+        """Return a key object (an index) that can be used to sort spec.
+
+           Sort is done in package order. We don't cache the result of
+           this function as Python's sort functions already ensure that the
+           key function is called at most once per sorted element.
+        """
+        spec_order = self._specs_for_pkg(
+            self.pkgname, self.component, self.vpkg)
+
+        # integer is the index of the first spec in order that satisfies
+        # spec, or it's a number larger than any position in the order.
+        return next(
+            (i for i, s in enumerate(spec_order) if spec.satisfies(s)),
+            len(spec_order))
+
+    @classproperty
+    @classmethod
+    def _packages_config(cls):
+        if cls._packages_config_cache is None:
+            cls._packages_config_cache = get_packages_config()
+        return cls._packages_config_cache
+
+    @classmethod
+    def _order_for_package(cls, pkgname, component, vpkg=None, all=True):
+        """Given a package name, sort component (e.g, version, compiler, ...),
+           and an optional vpkg, return the list from the packages config.
+        """
         pkglist = [pkgname]
-        if test_all:
+        if all:
             pkglist.append('all')
+
         for pkg in pkglist:
-            order = self.preferred.get(pkg, {}).get(component, {})
+            pkg_entry = cls._packages_config.get(pkg)
-            if isinstance(order, dict) and second_key:
+            if not pkg_entry:
-                order = order.get(second_key, {})
+                continue
+
+            order = pkg_entry.get(component)
             if not order:
                 continue
-            return [str(s).strip() for s in order]
+
+            # vpkg is one more level
+            if vpkg is not None:
+                order = order.get(vpkg)
+
+            if order:
+                return [str(s).strip() for s in order]
+
         return []
 
-    # A generic sorting function. Given a package name and sort
+    @classmethod
-    # component, return less-than-0, 0, or greater-than-0 if
+    def _specs_for_pkg(cls, pkgname, component, vpkg=None):
-    # a is respectively less-than, equal to, or greater than b.
+        """Given a sort order specified by the pkgname/component/second_key,
-    def _component_compare(self, pkgname, component, a, b,
+           return a list of CompilerSpecs, VersionLists, or Specs for
-                           reverse_natural_compare, second_key):
+           that sorting list.
-        if a is None:
+        """
-            return -1
+        key = (pkgname, component, vpkg)
-        if b is None:
-            return 1
-        orderlist = self._order_for_package(pkgname, component, second_key)
-        a_in_list = str(a) in orderlist
-        b_in_list = str(b) in orderlist
-        if a_in_list and not b_in_list:
-            return -1
-        elif b_in_list and not a_in_list:
-            return 1
 
-        cmp_a = None
+        specs = cls._spec_cache.get(key)
-        cmp_b = None
+        if specs is None:
-        reverse = None
+            pkglist = cls._order_for_package(pkgname, component, vpkg)
-        if not a_in_list and not b_in_list:
+            spec_type = _spec_type(component)
-            cmp_a = a
+            specs = [spec_type(s) for s in pkglist]
-            cmp_b = b
+            cls._spec_cache[key] = specs
-            reverse = -1 if reverse_natural_compare else 1
-        else:
-            cmp_a = orderlist.index(str(a))
-            cmp_b = orderlist.index(str(b))
-            reverse = 1
 
-        if cmp_a < cmp_b:
+        return specs
-            return -1 * reverse
-        elif cmp_a > cmp_b:
-            return 1 * reverse
-        else:
-            return 0
 
-    # A sorting function for specs.  Similar to component_compare, but
+    @classmethod
-    # a and b are considered to match entries in the sorting list if they
+    def clear_caches(cls):
-    # satisfy the list component.
+        cls._packages_config_cache = None
-    def _spec_compare(self, pkgname, component, a, b,
+        cls._spec_cache = {}
-                      reverse_natural_compare, second_key):
-        if not a or (not a.concrete and not second_key):
-            return -1
-        if not b or (not b.concrete and not second_key):
-            return 1
-        specs = self._spec_for_pkgname(pkgname, component, second_key)
-        a_index = None
-        b_index = None
-        reverse = -1 if reverse_natural_compare else 1
-        for i, cspec in enumerate(specs):
-            if a_index is None and (cspec.satisfies(a) or a.satisfies(cspec)):
-                a_index = i
-                if b_index:
-                    break
-            if b_index is None and (cspec.satisfies(b) or b.satisfies(cspec)):
-                b_index = i
-                if a_index:
-                    break
 
-        if a_index is not None and b_index is None:
+    @classmethod
-            return -1
+    def has_preferred_providers(cls, pkgname, vpkg):
-        elif a_index is None and b_index is not None:
+        """Whether specific package has a preferred vpkg providers."""
-            return 1
+        return bool(cls._order_for_package(pkgname, 'providers', vpkg, False))
-        elif a_index is not None and b_index == a_index:
-            return -1 * cmp(a, b)
-        elif (a_index is not None and b_index is not None and
-              a_index != b_index):
-            return cmp(a_index, b_index)
-        else:
-            return cmp(a, b) * reverse
 
-    # Given a sort order specified by the pkgname/component/second_key, return
+    @classmethod
-    # a list of CompilerSpecs, VersionLists, or Specs for that sorting list.
+    def preferred_variants(cls, pkg_name):
-    def _spec_for_pkgname(self, pkgname, component, second_key):
+        """Return a VariantMap of preferred variants/values for a spec."""
-        key = (pkgname, component, second_key)
+        for pkg in (pkg_name, 'all'):
-        if key not in self._spec_for_pkgname_cache:
+            variants = cls._packages_config.get(pkg, {}).get('variants', '')
-            pkglist = self._order_for_package(pkgname, component, second_key)
-            if component == 'compiler':
-                self._spec_for_pkgname_cache[key] = \
-                    [spack.spec.CompilerSpec(s) for s in pkglist]
-            elif component == 'version':
-                self._spec_for_pkgname_cache[key] = \
-                    [VersionList(s) for s in pkglist]
-            else:
-                self._spec_for_pkgname_cache[key] = \
-                    [spack.spec.Spec(s) for s in pkglist]
-        return self._spec_for_pkgname_cache[key]
-
-    def provider_compare(self, pkgname, provider_str, a, b):
-        """Return less-than-0, 0, or greater than 0 if a is respecively
-           less-than, equal-to, or greater-than b. A and b are possible
-           implementations of provider_str. One provider is less-than another
-           if it is preferred over the other. For example,
-           provider_compare('scorep', 'mpi', 'mvapich', 'openmpi') would
-           return -1 if mvapich should be preferred over openmpi for scorep."""
-        return self._spec_compare(pkgname, 'providers', a, b, False,
-                                  provider_str)
-
-    def spec_has_preferred_provider(self, pkgname, provider_str):
-        """Return True iff the named package has a list of preferred
-           providers"""
-        return bool(self._order_for_package(pkgname, 'providers',
-                                            provider_str, False))
-
-    def spec_preferred_variants(self, pkgname):
-        """Return a VariantMap of preferred variants and their values"""
-        for pkg in (pkgname, 'all'):
-            variants = self.preferred.get(pkg, {}).get('variants', '')
             if variants:
                 break
+
+        # allow variants to be list or string
         if not isinstance(variants, string_types):
             variants = " ".join(variants)
-        pkg = spack.repo.get(pkgname)
+
-        spec = spack.spec.Spec("%s %s" % (pkgname, variants))
         # Only return variants that are actually supported by the package
+        pkg = spack.repo.get(pkg_name)
+        spec = spack.spec.Spec("%s %s" % (pkg_name, variants))
         return dict((name, variant) for name, variant in spec.variants.items()
                     if name in pkg.variants)
 
-    def version_compare(self, pkgname, a, b):
-        """Return less-than-0, 0, or greater than 0 if version a of pkgname is
-           respectively less-than, equal-to, or greater-than version b of
-           pkgname. One version is less-than another if it is preferred over
-           the other."""
-        return self._spec_compare(pkgname, 'version', a, b, True, None)
-
-    def variant_compare(self, pkgname, a, b):
-        """Return less-than-0, 0, or greater than 0 if variant a of pkgname is
-           respectively less-than, equal-to, or greater-than variant b of
-           pkgname. One variant is less-than another if it is preferred over
-           the other."""
-        return self._component_compare(pkgname, 'variant', a, b, False, None)
-
-    def architecture_compare(self, pkgname, a, b):
-        """Return less-than-0, 0, or greater than 0 if architecture a of pkgname
-           is respectively less-than, equal-to, or greater-than architecture b
-           of pkgname. One architecture is less-than another if it is preferred
-           over the other."""
-        return self._component_compare(pkgname, 'architecture', a, b,
-                                       False, None)
-
-    def compiler_compare(self, pkgname, a, b):
-        """Return less-than-0, 0, or greater than 0 if compiler a of pkgname is
-           respecively less-than, equal-to, or greater-than compiler b of
-           pkgname. One compiler is less-than another if it is preferred over
-           the other."""
-        return self._spec_compare(pkgname, 'compiler', a, b, False, None)
-
 
 def spec_externals(spec):
-    """Return a list of external specs (with external directory path filled in),
+    """Return a list of external specs (w/external directory path filled in),
        one for each known external installation."""
     # break circular import.
     from spack.build_environment import get_path_from_module
@@ -255,7 +230,8 @@ def spec_externals(spec):
         if external_spec.satisfies(spec):
             external_specs.append(external_spec)
 
-    return external_specs
+    # defensively copy returned specs
+    return [s.copy() for s in external_specs]
 
 
 def is_spec_buildable(spec):
@@ -268,50 +244,5 @@ def is_spec_buildable(spec):
     return allpkgs[spec.name]['buildable']
 
 
-def cmp_specs(lhs, rhs):
-    # Package name sort order is not configurable, always goes alphabetical
-    if lhs.name != rhs.name:
-        return cmp(lhs.name, rhs.name)
-
-    # Package version is second in compare order
-    pkgname = lhs.name
-    if lhs.versions != rhs.versions:
-        return pkgsort().version_compare(
-            pkgname, lhs.versions, rhs.versions)
-
-    # Compiler is third
-    if lhs.compiler != rhs.compiler:
-        return pkgsort().compiler_compare(
-            pkgname, lhs.compiler, rhs.compiler)
-
-    # Variants
-    if lhs.variants != rhs.variants:
-        return pkgsort().variant_compare(
-            pkgname, lhs.variants, rhs.variants)
-
-    # Architecture
-    if lhs.architecture != rhs.architecture:
-        return pkgsort().architecture_compare(
-            pkgname, lhs.architecture, rhs.architecture)
-
-    # Dependency is not configurable
-    lhash, rhash = hash(lhs), hash(rhs)
-    if lhash != rhash:
-        return -1 if lhash < rhash else 1
-
-    # Equal specs
-    return 0
-
-
-_pkgsort = None
-
-
-def pkgsort():
-    global _pkgsort
-    if _pkgsort is None:
-        _pkgsort = PreferredPackages()
-    return _pkgsort
-
-
 class VirtualInPackagesYAMLError(spack.error.SpackError):
     """Raised when a disallowed virtual is found in packages.yaml"""

lib/spack/spack/parse.py

@@ -48,9 +48,8 @@ def __str__(self):
     def is_a(self, type):
         return self.type == type
 
-    def __cmp__(self, other):
+    def __eq__(self, other):
-        return cmp((self.type, self.value),
+        return (self.type == other.type) and (self.value == other.value)
-                   (other.type, other.value))
 
 
 class Lexer(object):

lib/spack/spack/provider_index.py

@@ -146,8 +146,8 @@ def providers_for(self, *vpkg_specs):
                     if p_spec.satisfies(vspec, deps=False):
                         providers.update(spec_set)
 
-        # Return providers in order
+        # Return providers in order. Defensively copy.
-        return sorted(providers)
+        return sorted(s.copy() for s in providers)
 
     # TODO: this is pretty darned nasty, and inefficient, but there
     # are not that many vdeps in most specs.

lib/spack/spack/spec.py

@@ -96,6 +96,7 @@
 expansion when it is the first character in an id typed on the command line.
 """
 import base64
+import sys
 import collections
 import ctypes
 import hashlib
@@ -732,8 +733,7 @@ def _cmp_key(self):
         return tuple((k, tuple(v)) for k, v in sorted(iteritems(self)))
 
     def __str__(self):
-        sorted_keys = filter(
+        sorted_keys = [k for k in sorted(self.keys()) if self[k] != []]
-            lambda flag: self[flag] != [], sorted(self.keys()))
         cond_symbol = ' ' if len(sorted_keys) > 0 else ''
         return cond_symbol + ' '.join(
             str(key) + '=\"' + ' '.join(
@@ -1316,7 +1316,11 @@ def dag_hash(self, length=None):
             yaml_text = syaml.dump(
                 self.to_node_dict(), default_flow_style=True, width=maxint)
             sha = hashlib.sha1(yaml_text.encode('utf-8'))
+
             b32_hash = base64.b32encode(sha.digest()).lower()
+            if sys.version_info[0] >= 3:
+                b32_hash = b32_hash.decode('utf-8')
+
             if self.concrete:
                 self._hash = b32_hash
             return b32_hash[:length]
@@ -1567,14 +1571,12 @@ def _expand_virtual_packages(self):
               a problem.
         """
         # Make an index of stuff this spec already provides
-        # XXX(deptype): 'link' and 'run'?
         self_index = ProviderIndex(self.traverse(), restrict=True)
         changed = False
         done = False
 
         while not done:
             done = True
-            # XXX(deptype): 'link' and 'run'?
             for spec in list(self.traverse()):
                 replacement = None
                 if spec.virtual:
@@ -1600,7 +1602,7 @@ def _expand_virtual_packages(self):
 
                         # Replace spec with the candidate and normalize
                         copy = self.copy()
-                        copy[spec.name]._dup(replacement.copy(deps=False))
+                        copy[spec.name]._dup(replacement, deps=False)
 
                         try:
                             # If there are duplicate providers or duplicate
@@ -2327,9 +2329,6 @@ def _dup(self, other, deps=True, cleardeps=True):
         self.external_module = other.external_module
         self.namespace = other.namespace
 
-        self.external = other.external
-        self.external_module = other.external_module
-
         # If we copy dependencies, preserve DAG structure in the new spec
         if deps:
             deptypes = alldeps  # by default copy all deptypes
@@ -2343,6 +2342,7 @@ def _dup(self, other, deps=True, cleardeps=True):
         # These fields are all cached results of expensive operations.
         # If we preserved the original structure, we can copy them
         # safely. If not, they need to be recomputed.
+        # TODO: dependency hashes can be copied more aggressively.
         if deps is True or deps == alldeps:
             self._hash = other._hash
             self._cmp_key_cache = other._cmp_key_cache
@@ -2725,41 +2725,6 @@ def write(s, c):
     def dep_string(self):
         return ''.join("^" + dep.format() for dep in self.sorted_deps())
 
-    def __cmp__(self, other):
-        from package_prefs import pkgsort
-
-        # Package name sort order is not configurable, always goes alphabetical
-        if self.name != other.name:
-            return cmp(self.name, other.name)
-
-        # Package version is second in compare order
-        pkgname = self.name
-        if self.versions != other.versions:
-            return pkgsort().version_compare(
-                pkgname, self.versions, other.versions)
-
-        # Compiler is third
-        if self.compiler != other.compiler:
-            return pkgsort().compiler_compare(
-                pkgname, self.compiler, other.compiler)
-
-        # Variants
-        if self.variants != other.variants:
-            return pkgsort().variant_compare(
-                pkgname, self.variants, other.variants)
-
-        # Target
-        if self.architecture != other.architecture:
-            return pkgsort().architecture_compare(
-                pkgname, self.architecture, other.architecture)
-
-        # Dependency is not configurable
-        if self._dependencies != other._dependencies:
-            return -1 if self._dependencies < other._dependencies else 1
-
-        # Equal specs
-        return 0
-
     def __str__(self):
         ret = self.format() + self.dep_string()
         return ret.strip()

lib/spack/spack/stage.py

@@ -690,5 +690,6 @@ class RestageError(StageError):
 class ChdirError(StageError):
     """Raised when Spack can't change directories."""
 
+
 # Keep this in namespace for convenience
 FailedDownloadError = fs.FailedDownloadError

lib/spack/spack/test/concretize_preferences.py

@@ -27,7 +27,7 @@
 import spack
 import spack.util.spack_yaml as syaml
 from spack.spec import Spec
-from spack.package_prefs import PreferredPackages
+import spack.package_prefs
 
 
 @pytest.fixture()
@@ -41,7 +41,7 @@ def concretize_scope(config, tmpdir):
     # This is kind of weird, but that's how config scopes are
     # set in ConfigScope.__init__
     spack.config.config_scopes.pop('concretize')
-    spack.package_prefs._pkgsort = PreferredPackages()
+    spack.package_prefs.PackagePrefs.clear_caches()
 
     # reset provider index each time, too
     spack.repo._provider_index = None
@@ -55,7 +55,7 @@ def update_packages(pkgname, section, value):
     """Update config and reread package list"""
     conf = {pkgname: {section: value}}
     spack.config.update_config('packages', conf, 'concretize')
-    spack.package_prefs._pkgsort = PreferredPackages()
+    spack.package_prefs.PackagePrefs.clear_caches()
 
 
 def assert_variant_values(spec, **variants):
@@ -146,7 +146,7 @@ def test_all_is_not_a_virtual(self):
         spack.config.update_config('packages', conf, 'concretize')
 
         # should be no error for 'all':
-        spack.package_prefs._pkgsort = PreferredPackages()
+        spack.package_prefs.PackagePrefs.clear_caches()
         spack.package_prefs.get_packages_config()
 
     def test_external_mpi(self):

lib/spack/spack/test/conftest.py

@@ -168,16 +168,19 @@ def configuration_dir(tmpdir_factory, linux_os):
 def config(configuration_dir):
     """Hooks the mock configuration files into spack.config"""
     # Set up a mock config scope
+    spack.package_prefs.PackagePrefs.clear_caches()
     spack.config.clear_config_caches()
     real_scope = spack.config.config_scopes
     spack.config.config_scopes = ordereddict_backport.OrderedDict()
     spack.config.ConfigScope('site', str(configuration_dir.join('site')))
     spack.config.ConfigScope('user', str(configuration_dir.join('user')))
     Config = collections.namedtuple('Config', ['real', 'mock'])
+
     yield Config(real=real_scope, mock=spack.config.config_scopes)
+
     spack.config.config_scopes = real_scope
     spack.config.clear_config_caches()
-
+    spack.package_prefs.PackagePrefs.clear_caches()
 
 
 @pytest.fixture(scope='module')

lib/spack/spack/test/directory_layout.py

@@ -92,23 +92,25 @@ def test_read_and_write_spec(
         # TODO: increase reuse of build dependencies.
         stored_deptypes = ('link', 'run')
         expected = spec.copy(deps=stored_deptypes)
+        assert expected.concrete
         assert expected == spec_from_file
-        assert expected.eq_dag  # msg , spec_from_file
+        assert expected.eq_dag(spec_from_file)
         assert spec_from_file.concrete
 
         # Ensure that specs that come out "normal" are really normal.
         with open(spec_path) as spec_file:
             read_separately = Spec.from_yaml(spec_file.read())
 
-            # TODO: revise this when build deps are in dag_hash
+        # TODO: revise this when build deps are in dag_hash
-            norm = read_separately.normalized().copy(deps=stored_deptypes)
+        norm = read_separately.normalized().copy(deps=stored_deptypes)
-            assert norm == spec_from_file
+        assert norm == spec_from_file
+        assert norm.eq_dag(spec_from_file)
 
-            # TODO: revise this when build deps are in dag_hash
+        # TODO: revise this when build deps are in dag_hash
-            conc = read_separately.concretized().copy(deps=stored_deptypes)
+        conc = read_separately.concretized().copy(deps=stored_deptypes)
-            assert conc == spec_from_file
+        assert conc == spec_from_file
+        assert conc.eq_dag(spec_from_file)
 
-        # Make sure the hash of the read-in spec is the same
         assert expected.dag_hash() == spec_from_file.dag_hash()
 
         # Ensure directories are properly removed

lib/spack/spack/test/spec_semantics.py

@@ -293,7 +293,7 @@ def test_copy_satisfies_transitive(self):
         copy = spec.copy()
         for s in spec.traverse():
             assert s.satisfies(copy[s.name])
-            assert copy[s.name].satisfies(s) 
+            assert copy[s.name].satisfies(s)
 
     def test_unsatisfiable_compiler_flag_mismatch(self):
         # No matchi in specs

lib/spack/spack/test/spec_yaml.py

@@ -27,6 +27,8 @@
 YAML format preserves DAG informatoin in the spec.
 
 """
+from collections import Iterable, Mapping
+
 import spack.util.spack_json as sjson
 import spack.util.spack_yaml as syaml
 from spack.spec import Spec
@@ -78,8 +80,6 @@ def test_using_ordered_dict(builtin_mock):
     versions and processes.
     """
     def descend_and_check(iterable, level=0):
-        from spack.util.spack_yaml import syaml_dict
-        from collections import Iterable, Mapping
         if isinstance(iterable, Mapping):
             assert isinstance(iterable, syaml_dict)
             return descend_and_check(iterable.values(), level=level + 1)
@@ -95,7 +95,12 @@ def descend_and_check(iterable, level=0):
     for spec in specs:
         dag = Spec(spec)
         dag.normalize()
+        from pprint import pprint
+        pprint(dag.to_node_dict())
+        break
+
         level = descend_and_check(dag.to_node_dict())
+
         # level just makes sure we are doing something here
         assert level >= 5
 

lib/spack/spack/util/spack_json.py

@@ -23,6 +23,7 @@
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 ##############################################################################
 """Simple wrapper around JSON to guarantee consistent use of load/dump. """
+import sys
 import json
 from six import string_types
 from six import iteritems
@@ -40,11 +41,11 @@
 def load(stream):
     """Spack JSON needs to be ordered to support specs."""
     if isinstance(stream, string_types):
-        return _byteify(json.loads(stream, object_hook=_byteify),
+        load = json.loads
-                        ignore_dicts=True)
     else:
-        return _byteify(json.load(stream, object_hook=_byteify),
+        load = json.load
-                        ignore_dicts=True)
+
+    return _strify(load(stream, object_hook=_strify), ignore_dicts=True)
 
 
 def dump(data, stream=None):
@@ -55,18 +56,21 @@ def dump(data, stream=None):
         return json.dump(data, stream, **_json_dump_args)
 
 
-def _byteify(data, ignore_dicts=False):
+def _strify(data, ignore_dicts=False):
-    # if this is a unicode string, return its string representation
+    # if this is a unicode string in python 2, return its string representation
-    if isinstance(data, unicode):
+    if sys.version_info[0] < 3:
-        return data.encode('utf-8')
+        if isinstance(data, unicode):
+            return data.encode('utf-8')
+
     # if this is a list of values, return list of byteified values
     if isinstance(data, list):
-        return [_byteify(item, ignore_dicts=True) for item in data]
+        return [_strify(item, ignore_dicts=True) for item in data]
+
     # if this is a dictionary, return dictionary of byteified keys and values
     # but only if we haven't already byteified it
     if isinstance(data, dict) and not ignore_dicts:
-        return dict((_byteify(key, ignore_dicts=True),
+        return dict((_strify(key, ignore_dicts=True),
-                     _byteify(value, ignore_dicts=True)) for key, value in
+                     _strify(value, ignore_dicts=True)) for key, value in
                     iteritems(data))
 
     # if it's anything else, return it in its original form
@@ -76,5 +80,5 @@ def _byteify(data, ignore_dicts=False):
 class SpackJSONError(spack.error.SpackError):
     """Raised when there are issues with JSON parsing."""
 
-    def __init__(self, msg, yaml_error):
+    def __init__(self, msg, json_error):
-        super(SpackJSONError, self).__init__(msg, str(yaml_error))
+        super(SpackJSONError, self).__init__(msg, str(json_error))

lib/spack/spack/util/spack_yaml.py

@@ -86,7 +86,6 @@ class OrderedLineLoader(Loader):
     def construct_yaml_str(self, node):
         value = self.construct_scalar(node)
         value = syaml_str(value)
-
         mark(value, node)
         return value
 
@@ -149,11 +148,11 @@ def construct_mapping(self, node, deep=False):
 
 # register above new constructors
 OrderedLineLoader.add_constructor(
-    u'tag:yaml.org,2002:map', OrderedLineLoader.construct_yaml_map)
+    'tag:yaml.org,2002:map', OrderedLineLoader.construct_yaml_map)
 OrderedLineLoader.add_constructor(
-    u'tag:yaml.org,2002:seq', OrderedLineLoader.construct_yaml_seq)
+    'tag:yaml.org,2002:seq', OrderedLineLoader.construct_yaml_seq)
 OrderedLineLoader.add_constructor(
-    u'tag:yaml.org,2002:str', OrderedLineLoader.construct_yaml_str)
+    'tag:yaml.org,2002:str', OrderedLineLoader.construct_yaml_str)
 
 
 class OrderedLineDumper(Dumper):

lib/spack/spack/util/web.py

@@ -36,6 +36,7 @@
 except ImportError:
     # In Python 3, things moved to html.parser
     from html.parser import HTMLParser
+
     # Also, HTMLParseError is deprecated and never raised.
     class HTMLParseError:
         pass

lib/spack/spack/version.py

@@ -49,7 +49,6 @@
 from functools import wraps
 from six import string_types
 
-from functools_backport import total_ordering
 from spack.util.spack_yaml import syaml_dict
 
 __all__ = ['Version', 'VersionRange', 'VersionList', 'ver']
@@ -112,7 +111,6 @@ def _numeric_lt(self0, other):
     """Compares two versions, knowing they're both numeric"""
 
 
-@total_ordering
 class Version(object):
     """Class to represent versions"""
 
@@ -330,9 +328,22 @@ def __eq__(self, other):
         return (other is not None and
                 type(other) == Version and self.version == other.version)
 
+    @coerced
     def __ne__(self, other):
         return not (self == other)
 
+    @coerced
+    def __le__(self, other):
+        return self == other or self < other
+
+    @coerced
+    def __ge__(self, other):
+        return not (self < other)
+
+    @coerced
+    def __gt__(self, other):
+        return not (self == other) and not (self < other)
+
     def __hash__(self):
         return hash(self.version)
 
@@ -378,7 +389,6 @@ def intersection(self, other):
             return VersionList()
 
 
-@total_ordering
 class VersionRange(object):
 
     def __init__(self, start, end):
@@ -421,9 +431,22 @@ def __eq__(self, other):
                 type(other) == VersionRange and
                 self.start == other.start and self.end == other.end)
 
+    @coerced
     def __ne__(self, other):
         return not (self == other)
 
+    @coerced
+    def __le__(self, other):
+        return self == other or self < other
+
+    @coerced
+    def __ge__(self, other):
+        return not (self < other)
+
+    @coerced
+    def __gt__(self, other):
+        return not (self == other) and not (self < other)
+
     @property
     def concrete(self):
         return self.start if self.start == self.end else None
@@ -568,7 +591,6 @@ def __str__(self):
         return out
 
 
-@total_ordering
 class VersionList(object):
     """Sorted, non-redundant list of Versions and VersionRanges."""
 
@@ -761,6 +783,7 @@ def __len__(self):
     def __eq__(self, other):
         return other is not None and self.versions == other.versions
 
+    @coerced
     def __ne__(self, other):
         return not (self == other)
 
@@ -768,6 +791,18 @@ def __ne__(self, other):
     def __lt__(self, other):
         return other is not None and self.versions < other.versions
 
+    @coerced
+    def __le__(self, other):
+        return self == other or self < other
+
+    @coerced
+    def __ge__(self, other):
+        return not (self < other)
+
+    @coerced
+    def __gt__(self, other):
+        return not (self == other) and not (self < other)
+
     def __hash__(self):
         return hash(tuple(self.versions))