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

lib/spack/spack/database.py

@@ -937,22 +937,15 @@ def _construct_from_directory_layout(self, directory_layout, old_data):
                 tty.debug(
                     'RECONSTRUCTING FROM OLD DB: {0}'.format(entry.spec))
                 try:
-                    layout = spack.store.layout
+                    layout = None if entry.spec.external else spack.store.layout
-                    if entry.spec.external:
+                    kwargs = {
-                        layout = None
+                        'spec': entry.spec,
-                        install_check = True
+                        'directory_layout': layout,
-                    else:
+                        'explicit': entry.explicit,
-                        install_check = layout.check_installed(entry.spec)
+                        'installation_time': entry.installation_time  # noqa: E501
-
+                    }
-                    if install_check:
+                    self._add(**kwargs)
-                        kwargs = {
+                    processed_specs.add(entry.spec)
-                            '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 @@ def _add(
                 }
                 self._add(dep, directory_layout, **extra_args)
 
-        if key not in self._data:
+        # Make sure the directory layout agrees whether the spec is installed
-            installed = bool(spec.external)
+        if not spec.external and directory_layout:
-            path = None
+            path = directory_layout.path_for_spec(spec)
-            if not spec.external and directory_layout:
+            installed = False
-                path = directory_layout.path_for_spec(spec)
+            try:
-                if path in self._installed_prefixes:
+                directory_layout.ensure_installed(spec)
-                    raise Exception("Install prefix collision.")
+                installed = True
-                try:
-                    directory_layout.check_installed(spec)
-                    installed = True
-                except DirectoryLayoutError as e:
-                    tty.warn(
-                        'Dependency missing: may be deprecated or corrupted:',
-                        path, str(e))
                 self._installed_prefixes.add(path)
-            elif spec.external_path:
+            except DirectoryLayoutError as e:
-                path = spec.external_path
+                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 @@ def _add(
             new_spec._full_hash = spec._full_hash
 
         else:
-            # If it is already there, mark it as installed and update
+            # It is already in the database
-            # installation time
+            self._data[key].installed = installed
-            self._data[key].installed = True
             self._data[key].installation_time = _now()
 
         self._data[key].explicit = explicit
@@ -1210,7 +1206,7 @@ def _remove(self, spec):
 
         # 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:

lib/spack/spack/directory_layout.py

@@ -233,13 +233,20 @@ def create_install_directory(self, spec):
 
         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 @@ def check_installed(self, spec):
 
         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 @@ def check_installed(self, spec):
             # 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)

lib/spack/spack/test/conftest.py

@@ -876,8 +876,8 @@ def __init__(self, root):
     def path_for_spec(self, spec):
         return '/'.join([self.root, spec.name + '-' + spec.dag_hash()])
 
-    def check_installed(self, spec):
+    def ensure_installed(self, spec):
-        return True
+        pass
 
 
 @pytest.fixture()

lib/spack/spack/test/database.py

@@ -11,6 +11,7 @@
 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