database: fix reindex with uninstalled deps (#28764)

* Fix reindex with uninstalled deps

When a prefix of a dep is removed, and the db is reindexed, it is added
through the dependent, but until now it incorrectly listed the spec as
'installed'.

There was also some questionable behavior in the db when the same spec
was added multiple times, it would always be marked installed.

* Always reserve path

* Only add installed spec's prefixes to install prefixes set

* Improve warning, and ensure ensure only ensures

* test: reindex with every file system remnant removed except for the old index; it should give a database with nothing installed, including records with installed==False,external==False,ref_count==0,explicit=True, and these should be removable from the database

4 files changed, 94 insertions, 42 deletions

diff --git a/lib/spack/spack/database.py b/lib/spack/spack/database.py
index b7294f3ecb..2b99e33bb0 100644
--- a/lib/spack/spack/database.py
+++ b/lib/spack/spack/database.py
@@ -937,22 +937,15 @@ class Database(object):
                 tty.debug(
                     'RECONSTRUCTING FROM OLD DB: {0}'.format(entry.spec))
                 try:
-                    layout = spack.store.layout
-                    if entry.spec.external:
-                        layout = None
-                        install_check = True
-                    else:
-                        install_check = layout.check_installed(entry.spec)
-
-                    if install_check:
-                        kwargs = {
-                            'spec': entry.spec,
-                            'directory_layout': layout,
-                            'explicit': entry.explicit,
-                            'installation_time': entry.installation_time  # noqa: E501
-                        }
-                        self._add(**kwargs)
-                        processed_specs.add(entry.spec)
+                    layout = None if entry.spec.external else spack.store.layout
+                    kwargs = {
+                        'spec': entry.spec,
+                        'directory_layout': layout,
+                        'explicit': entry.explicit,
+                        'installation_time': entry.installation_time  # noqa: E501
+                    }
+                    self._add(**kwargs)
+                    processed_specs.add(entry.spec)
                 except Exception as e:
                     # Something went wrong, so the spec was not restored
                     # from old data
@@ -1096,24 +1089,28 @@ class Database(object):
                 }
                 self._add(dep, directory_layout, **extra_args)
 
-        if key not in self._data:
-            installed = bool(spec.external)
-            path = None
-            if not spec.external and directory_layout:
-                path = directory_layout.path_for_spec(spec)
-                if path in self._installed_prefixes:
-                    raise Exception("Install prefix collision.")
-                try:
-                    directory_layout.check_installed(spec)
-                    installed = True
-                except DirectoryLayoutError as e:
-                    tty.warn(
-                        'Dependency missing: may be deprecated or corrupted:',
-                        path, str(e))
+        # Make sure the directory layout agrees whether the spec is installed
+        if not spec.external and directory_layout:
+            path = directory_layout.path_for_spec(spec)
+            installed = False
+            try:
+                directory_layout.ensure_installed(spec)
+                installed = True
                 self._installed_prefixes.add(path)
-            elif spec.external_path:
-                path = spec.external_path
+            except DirectoryLayoutError as e:
+                msg = ("{0} is being {1} in the database with prefix {2}, "
+                       "but this directory does not contain an installation of "
+                       "the spec, due to: {3}")
+                action = "updated" if key in self._data else "registered"
+                tty.warn(msg.format(spec.short_spec, action, path, str(e)))
+        elif spec.external_path:
+            path = spec.external_path
+            installed = True
+        else:
+            path = None
+            installed = True
 
+        if key not in self._data:
             # Create a new install record with no deps initially.
             new_spec = spec.copy(deps=False)
             extra_args = {
@@ -1141,9 +1138,8 @@ class Database(object):
             new_spec._full_hash = spec._full_hash
 
         else:
-            # If it is already there, mark it as installed and update
-            # installation time
-            self._data[key].installed = True
+            # It is already in the database
+            self._data[key].installed = installed
             self._data[key].installation_time = _now()
 
         self._data[key].explicit = explicit
@@ -1210,7 +1206,7 @@ class Database(object):
 
         # This install prefix is now free for other specs to use, even if the
         # spec is only marked uninstalled.
-        if not rec.spec.external:
+        if not rec.spec.external and rec.installed:
             self._installed_prefixes.remove(rec.path)
 
         if rec.ref_count > 0:
diff --git a/lib/spack/spack/directory_layout.py b/lib/spack/spack/directory_layout.py
index 762b591827..8193b328b9 100644
--- a/lib/spack/spack/directory_layout.py
+++ b/lib/spack/spack/directory_layout.py
@@ -233,13 +233,20 @@ class DirectoryLayout(object):
 
         self.write_spec(spec, self.spec_file_path(spec))
 
-    def check_installed(self, spec):
+    def ensure_installed(self, spec):
+        """
+        Throws DirectoryLayoutError if:
+        1. spec prefix does not exist
+        2. spec prefix does not contain a spec file
+        3. the spec file does not correspond to the spec
+        """
         _check_concrete(spec)
         path = self.path_for_spec(spec)
         spec_file_path = self.spec_file_path(spec)
 
         if not os.path.isdir(path):
-            return None
+            raise InconsistentInstallDirectoryError(
+                "Install prefix {0} does not exist.".format(path))
 
         if not os.path.isfile(spec_file_path):
             raise InconsistentInstallDirectoryError(
@@ -248,7 +255,7 @@ class DirectoryLayout(object):
 
         installed_spec = self.read_spec(spec_file_path)
         if installed_spec == spec:
-            return path
+            return
 
         # DAG hashes currently do not include build dependencies.
         #
@@ -261,7 +268,7 @@ class DirectoryLayout(object):
             # may be installed. This means for example that for two instances
             # that differ only in CMake version used to build, only one will
             # be installed.
-            return path
+            return
 
         if spec.dag_hash() == installed_spec.dag_hash():
             raise SpecHashCollisionError(spec, installed_spec)
diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py
index 9a6dc0b13b..00ab62e020 100644
--- a/lib/spack/spack/test/conftest.py
+++ b/lib/spack/spack/test/conftest.py
@@ -876,8 +876,8 @@ class MockLayout(object):
     def path_for_spec(self, spec):
         return '/'.join([self.root, spec.name + '-' + spec.dag_hash()])
 
-    def check_installed(self, spec):
-        return True
+    def ensure_installed(self, spec):
+        pass
 
 
 @pytest.fixture()
diff --git a/lib/spack/spack/test/database.py b/lib/spack/spack/test/database.py
index 57a03a5db9..f8f4b5de82 100644
--- a/lib/spack/spack/test/database.py
+++ b/lib/spack/spack/test/database.py
@@ -11,6 +11,7 @@ import datetime
 import functools
 import json
 import os
+import shutil
 
 import pytest
 
@@ -909,3 +910,51 @@ def test_database_works_with_empty_dir(tmpdir):
         db.query()
     # Check that reading an empty directory didn't create a new index.json
     assert not os.path.exists(db._index_path)
+
+
+def test_reindex_removed_prefix_is_not_installed(mutable_database, mock_store, capfd):
+    """When a prefix of a dependency is removed and the database is reindexed,
+    the spec should still be added through the dependent, but should be listed as
+    not installed."""
+
+    # Remove libelf from the filesystem
+    prefix = mutable_database.query_one('libelf').prefix
+    assert prefix.startswith(str(mock_store))
+    shutil.rmtree(prefix)
+
+    # Reindex should pick up libelf as a dependency of libdwarf
+    spack.store.store.reindex()
+
+    # Reindexing should warn about libelf not being found on the filesystem
+    err = capfd.readouterr()[1]
+    assert 'this directory does not contain an installation of the spec' in err
+
+    # And we should still have libelf in the database, but not installed.
+    assert not mutable_database.query_one('libelf', installed=True)
+    assert mutable_database.query_one('libelf', installed=False)
+
+
+def test_reindex_when_all_prefixes_are_removed(mutable_database, mock_store):
+    # Remove all non-external installations from the filesystem
+    for spec in spack.store.db.query_local():
+        if not spec.external:
+            assert spec.prefix.startswith(str(mock_store))
+            shutil.rmtree(spec.prefix)
+
+    # Make sure we have some explicitly installed specs
+    num = len(mutable_database.query_local(installed=True, explicit=True))
+    assert num > 0
+
+    # Reindex uses the current index to repopulate itself
+    spack.store.store.reindex()
+
+    # Make sure all explicit specs are still there, but are now uninstalled.
+    specs = mutable_database.query_local(installed=False, explicit=True)
+    assert len(specs) == num
+
+    # And make sure they can be removed from the database (covers the case where
+    # `ref_count == 0 and not installed`, which hits some obscure branches.
+    for s in specs:
+        mutable_database.remove(s)
+
+    assert len(mutable_database.query_local(installed=False, explicit=True)) == 0