Fixes for various hash issues (#2626)

* Better output for disambiguate_specs()

* Fix wrong exception name.

* Fix satsifies(): concrete specs require matching by hash.

- Fixes uninstall by hash and other places where we need to match a
  specific spec.

- Fix an error in provider_index (satisfies() call was backwards)

- Fix an error in satisfies_dependencies(): checks were too shallow.

* Fix default args in Spec.tree()

* Move installed_dependents() to DB to avoid unknown package error.

* Make `spack find` and `sapck.store.db.query()` faster for hashes.

* Add a test to ensure satisfies() respects concrete Specs' hashes.

10 files changed, 84 insertions, 59 deletions

diff --git a/lib/spack/spack/cmd/__init__.py b/lib/spack/spack/cmd/__init__.py
index 8000c9f1f2..e712ba8e1d 100644
--- a/lib/spack/spack/cmd/__init__.py
+++ b/lib/spack/spack/cmd/__init__.py
@@ -144,7 +144,9 @@ def disambiguate_spec(spec):
     elif len(matching_specs) > 1:
         args = ["%s matches multiple packages." % spec,
                 "Matching packages:"]
-        args += ["  " + str(s) for s in matching_specs]
+        color = sys.stdout.isatty()
+        args += [colorize("  @K{%s} " % s.dag_hash(7), color=color) +
+                 s.format('$_$@$%@$=', color=color) for s in matching_specs]
         args += ["Use a more specific spec."]
         tty.die(*args)
 
diff --git a/lib/spack/spack/cmd/common/arguments.py b/lib/spack/spack/cmd/common/arguments.py
index 1839132218..53e75a4f2c 100644
--- a/lib/spack/spack/cmd/common/arguments.py
+++ b/lib/spack/spack/cmd/common/arguments.py
@@ -25,6 +25,7 @@
 
 import argparse
 
+import spack.cmd
 import spack.store
 import spack.modules
 from spack.util.pattern import Args
@@ -59,11 +60,18 @@ class ConstraintAction(argparse.Action):
         namespace.specs = self._specs
 
     def _specs(self, **kwargs):
-        specs = [s for s in spack.store.db.query(**kwargs)]
-        values = ' '.join(self.values)
-        if values:
-            specs = [x for x in specs if x.satisfies(values, strict=True)]
-        return specs
+        qspecs = spack.cmd.parse_specs(self.values)
+
+        # return everything for an empty query.
+        if not qspecs:
+            return spack.store.db.query()
+
+        # Return only matching stuff otherwise.
+        specs = set()
+        for spec in qspecs:
+            for s in spack.store.db.query(spec, **kwargs):
+                specs.add(s)
+        return sorted(specs)
 
 
 _arguments['constraint'] = Args(
diff --git a/lib/spack/spack/cmd/dependents.py b/lib/spack/spack/cmd/dependents.py
index 7729105e62..dc2ee658ac 100644
--- a/lib/spack/spack/cmd/dependents.py
+++ b/lib/spack/spack/cmd/dependents.py
@@ -27,6 +27,7 @@ import argparse
 import llnl.util.tty as tty
 
 import spack
+import spack.store
 import spack.cmd
 
 description = "Show installed packages that depend on another."
@@ -39,11 +40,14 @@ def setup_parser(subparser):
 
 
 def dependents(parser, args):
-    specs = spack.cmd.parse_specs(args.spec, concretize=True)
+    specs = spack.cmd.parse_specs(args.spec)
     if len(specs) != 1:
         tty.die("spack dependents takes only one spec.")
-
-    fmt = '$_$@$%@$+$=$#'
-    deps = [d.format(fmt, color=True)
-            for d in specs[0].package.installed_dependents]
-    tty.msg("Dependents of %s" % specs[0].format(fmt, color=True), *deps)
+    spec = spack.cmd.disambiguate_spec(specs[0])
+
+    tty.msg("Dependents of %s" % spec.format('$_$@$%@$#', color=True))
+    deps = spack.store.db.installed_dependents(spec)
+    if deps:
+        spack.cmd.display_specs(deps)
+    else:
+        print "No dependents"
diff --git a/lib/spack/spack/cmd/find.py b/lib/spack/spack/cmd/find.py
index 9f1759afc0..ab0b29c30e 100644
--- a/lib/spack/spack/cmd/find.py
+++ b/lib/spack/spack/cmd/find.py
@@ -114,14 +114,17 @@ def query_arguments(args):
 def find(parser, args):
     q_args = query_arguments(args)
     query_specs = args.specs(**q_args)
+
     # Exit early if no package matches the constraint
     if not query_specs and args.constraint:
         msg = "No package matches the query: {0}".format(args.constraint)
         tty.msg(msg)
         return
+
     # Display the result
     if sys.stdout.isatty():
         tty.msg("%d installed packages." % len(query_specs))
+
     display_specs(query_specs,
                   mode=args.mode,
                   long=args.long,
diff --git a/lib/spack/spack/cmd/uninstall.py b/lib/spack/spack/cmd/uninstall.py
index 2f7e15ba55..0fc22ce538 100644
--- a/lib/spack/spack/cmd/uninstall.py
+++ b/lib/spack/spack/cmd/uninstall.py
@@ -124,7 +124,8 @@ def installed_dependents(specs):
     """
     dependents = {}
     for item in specs:
-        lst = [x for x in item.package.installed_dependents if x not in specs]
+        lst = [x for x in spack.store.db.installed_dependents(item)
+               if x not in specs]
         if lst:
             lst = list(set(lst))
             dependents[item] = lst
@@ -152,7 +153,7 @@ def do_uninstall(specs, force):
     # Sort packages to be uninstalled by the number of installed dependents
     # This ensures we do things in the right order
     def num_installed_deps(pkg):
-        return len(pkg.installed_dependents)
+        return len(spack.store.db.installed_dependents(pkg.spec))
 
     packages.sort(key=num_installed_deps)
     for item in packages:
@@ -163,11 +164,14 @@ def get_uninstall_list(args):
     specs = [any]
     if args.packages:
         specs = spack.cmd.parse_specs(args.packages)
+
     # Gets the list of installed specs that match the ones give via cli
     # takes care of '-a' is given in the cli
     uninstall_list = concretize_specs(specs, args.all, args.force)
+
     # Takes care of '-d'
     dependent_list = installed_dependents(uninstall_list)
+
     # Process dependent_list and update uninstall_list
     has_error = False
     if dependent_list and not args.dependents and not args.force:
diff --git a/lib/spack/spack/database.py b/lib/spack/spack/database.py
index a01795ca0f..3a0c028d5b 100644
--- a/lib/spack/spack/database.py
+++ b/lib/spack/spack/database.py
@@ -605,6 +605,15 @@ class Database(object):
             return self._remove(spec)
 
     @_autospec
+    def installed_dependents(self, spec):
+        """List the installed specs that depend on this one."""
+        dependents = set()
+        for spec in self.query(spec):
+            for dependent in spec.traverse(direction='parents', root=False):
+                dependents.add(dependent)
+        return dependents
+
+    @_autospec
     def installed_extensions_for(self, extendee_spec):
         """
         Return the specs of all packages that extend
@@ -655,6 +664,18 @@ class Database(object):
 
         """
         with self.read_transaction():
+            # Just look up concrete specs with hashes; no fancy search.
+            if (isinstance(query_spec, spack.spec.Spec) and
+                query_spec._concrete):
+
+                hash_key = query_spec.dag_hash()
+                if hash_key in self._data:
+                    return [self._data[hash_key].spec]
+                else:
+                    return []
+
+            # Abstract specs require more work -- currently we test
+            # against everything.
             results = []
             for key, rec in self._data.items():
                 if installed is not any and rec.installed != installed:
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index 3502eb2931..0c924cfcbc 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -856,24 +856,6 @@ class PackageBase(object):
         return os.path.isdir(self.prefix)
 
     @property
-    def installed_dependents(self):
-        """Return a list of the specs of all installed packages that depend
-           on this one.
-
-        TODO: move this method to database.py?
-        """
-        dependents = []
-        for spec in spack.store.db.query():
-            if self.name == spec.name:
-                continue
-            # XXX(deptype): Should build dependencies not count here?
-            # for dep in spec.traverse(deptype=('run')):
-            for dep in spec.traverse(deptype=spack.alldeps):
-                if self.spec == dep:
-                    dependents.append(spec)
-        return dependents
-
-    @property
     def prefix_lock(self):
         """Prefix lock is a byte range lock on the nth byte of a file.
 
@@ -1528,7 +1510,7 @@ class PackageBase(object):
                 raise InstallError(str(self.spec) + " is not installed.")
 
         if not force:
-            dependents = self.installed_dependents
+            dependents = spack.store.db.installed_dependents(self.spec)
             if dependents:
                 raise PackageStillNeededError(self.spec, dependents)
 
diff --git a/lib/spack/spack/provider_index.py b/lib/spack/spack/provider_index.py
index 6bcf98009e..3911947e4a 100644
--- a/lib/spack/spack/provider_index.py
+++ b/lib/spack/spack/provider_index.py
@@ -100,7 +100,8 @@ class ProviderIndex(object):
         for provided_spec, provider_spec in pkg.provided.iteritems():
             # We want satisfaction other than flags
             provider_spec.compiler_flags = spec.compiler_flags.copy()
-            if provider_spec.satisfies(spec, deps=False):
+
+            if spec.satisfies(provider_spec, deps=False):
                 provided_name = provided_spec.name
 
                 provider_map = self.providers.setdefault(provided_name, {})
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 238eb07e1f..a8377e324f 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -818,7 +818,7 @@ class Spec(object):
         dep = self._dependencies.get(name)
         if dep is not None:
             return dep
-        raise InvalidDependencyException(
+        raise InvalidDependencyError(
             self.name + " does not depend on " + comma_or(name))
 
     def _find_deps(self, where, deptype):
@@ -1395,26 +1395,6 @@ class Spec(object):
                 dependent._add_dependency(concrete, deptypes,
                                           dep_spec.default_deptypes)
 
-    def _replace_node(self, replacement):
-        """Replace this spec with another.
-
-        Connects all dependents of this spec to its replacement, and
-        disconnects this spec from any dependencies it has. New spec
-        will have any dependencies the replacement had, and may need
-        to be normalized.
-
-        """
-        for name, dep_spec in self._dependents.items():
-            dependent = dep_spec.spec
-            deptypes = dep_spec.deptypes
-            del dependent._dependencies[self.name]
-            dependent._add_dependency(
-                replacement, deptypes, dep_spec.default_deptypes)
-
-        for name, dep_spec in self._dependencies.items():
-            del dep_spec.spec.dependents[self.name]
-            del self._dependencies[dep.name]
-
     def _expand_virtual_packages(self):
         """Find virtual packages in this spec, replace them with providers,
            and normalize again to include the provider's (potentially virtual)
@@ -2043,6 +2023,10 @@ class Spec(object):
         """
         other = self._autospec(other)
 
+        # The only way to satisfy a concrete spec is to match its hash exactly.
+        if other._concrete:
+            return self._concrete and self.dag_hash() == other.dag_hash()
+
         # A concrete provider can satisfy a virtual dependency.
         if not self.virtual and other.virtual:
             pkg = spack.repo.get(self.fullname)
@@ -2113,8 +2097,9 @@ class Spec(object):
             if other._dependencies and not self._dependencies:
                 return False
 
-            if not all(dep in self._dependencies
-                       for dep in other._dependencies):
+            alldeps = set(d.name for d in self.traverse(root=False))
+            if not all(dep.name in alldeps
+                       for dep in other.traverse(root=False)):
                 return False
 
         elif not self._dependencies or not other._dependencies:
@@ -2620,9 +2605,9 @@ class Spec(object):
            with indentation."""
         color = kwargs.pop('color', False)
         depth = kwargs.pop('depth', False)
-        hashes = kwargs.pop('hashes', True)
+        hashes = kwargs.pop('hashes', False)
         hlen = kwargs.pop('hashlen', None)
-        install_status = kwargs.pop('install_status', True)
+        install_status = kwargs.pop('install_status', False)
         cover = kwargs.pop('cover', 'nodes')
         indent = kwargs.pop('indent', 0)
         fmt = kwargs.pop('format', '$_$@$%@+$+$=')
diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py
index c165934948..16d6121dea 100644
--- a/lib/spack/spack/test/spec_semantics.py
+++ b/lib/spack/spack/test/spec_semantics.py
@@ -312,6 +312,21 @@ class SpecSematicsTest(MockPackagesTest):
             Spec('netlib-lapack ^netlib-blas').satisfies(
                 'netlib-lapack ^netlib-blas'))
 
+    def test_satisfies_same_spec_with_different_hash(self):
+        """Ensure that concrete specs are matched *exactly* by hash."""
+        s1 = Spec('mpileaks').concretized()
+        s2 = s1.copy()
+
+        self.assertTrue(s1.satisfies(s2))
+        self.assertTrue(s2.satisfies(s1))
+
+        # Simulate specs that were installed before and after a change to
+        # Spack's hashing algorithm.  This just reverses s2's hash.
+        s2._hash = s1.dag_hash()[-1::-1]
+
+        self.assertFalse(s1.satisfies(s2))
+        self.assertFalse(s2.satisfies(s1))
+
     # ========================================================================
     # Indexing specs
     # ========================================================================