database.py: remove process unsafe update_explicit (#47358)

Fixes an issue reported where `spack env depfile` + `make -j` would
non-deterministically refuse to mark all environment roots explicit.

`update_explicit` had the pattern

```python
rec = self._data[key]
with self.write_transaction():
    rec.explicit = explicit
```

but `write_transaction` may reinitialize `self._data`, meaning that
mutating `rec` won't mutate `self._data`, and the changes won't be
persisted.

Instead, use `mark` which has a correct implementation.

Also avoids the essentially incorrect early return in `update_explicit`
which is a pattern I don't think belongs in database.py: it branches on
possibly stale data to realize there is nothing to change, but in reality
it requires a write transaction to know that for a fact, but that would
defeat the purpose. So, leave this optimization to the call site.

lib/spack/spack/cmd/mark.py

@@ -98,8 +98,9 @@ def do_mark(specs, explicit):
         specs (list): list of specs to be marked
         explicit (bool): whether to mark specs as explicitly installed
     """
-    for spec in specs:
-        spack.store.STORE.db.update_explicit(spec, explicit)
+    with spack.store.STORE.db.write_transaction():
+        for spec in specs:
+            spack.store.STORE.db.mark(spec, "explicit", explicit)
 
 
 def mark_specs(args, specs):

lib/spack/spack/database.py

@@ -1336,7 +1336,7 @@ def _deprecate(self, spec: "spack.spec.Spec", deprecator: "spack.spec.Spec") ->
         self._data[spec_key] = spec_rec
 
     @_autospec
-    def mark(self, spec: "spack.spec.Spec", key, value) -> None:
+    def mark(self, spec: "spack.spec.Spec", key: str, value: Any) -> None:
         """Mark an arbitrary record on a spec."""
         with self.write_transaction():
             return self._mark(spec, key, value)
@@ -1771,24 +1771,6 @@ def root(key, record):
                 if id(rec.spec) not in needed and rec.installed
             ]
 
-    def update_explicit(self, spec, explicit):
-        """
-        Update the spec's explicit state in the database.
-
-        Args:
-            spec (spack.spec.Spec): the spec whose install record is being updated
-            explicit (bool): ``True`` if the package was requested explicitly
-                by the user, ``False`` if it was pulled in as a dependency of
-                an explicit package.
-        """
-        rec = self.get_record(spec)
-        if explicit != rec.explicit:
-            with self.write_transaction():
-                message = "{s.name}@{s.version} : marking the package {0}"
-                status = "explicit" if explicit else "implicit"
-                tty.debug(message.format(status, s=spec))
-                rec.explicit = explicit
-
 
 class NoUpstreamVisitor:
     """Gives edges to upstream specs, but does follow edges from upstream specs."""

lib/spack/spack/installer.py

@@ -412,7 +412,7 @@ def _process_external_package(pkg: "spack.package_base.PackageBase", explicit: b
         tty.debug(f"{pre} already registered in DB")
         record = spack.store.STORE.db.get_record(spec)
         if explicit and not record.explicit:
-            spack.store.STORE.db.update_explicit(spec, explicit)
+            spack.store.STORE.db.mark(spec, "explicit", True)
 
     except KeyError:
         # If not, register it and generate the module file.
@@ -1507,8 +1507,8 @@ def _prepare_for_install(self, task: Task) -> None:
             self._update_installed(task)
 
             # Only update the explicit entry once for the explicit package
-            if task.explicit:
-                spack.store.STORE.db.update_explicit(task.pkg.spec, True)
+            if task.explicit and not rec.explicit:
+                spack.store.STORE.db.mark(task.pkg.spec, "explicit", True)
 
     def _cleanup_all_tasks(self) -> None:
         """Cleanup all tasks to include releasing their locks."""