spec.py: fix virtual reconstruction for old specs (#49103)

Co-authored-by: Harmen Stoppels <me@harmenstoppels.nl>

lib/spack/spack/spec.py

@@ -798,7 +798,7 @@ def update_deptypes(self, depflag: dt.DepFlag) -> bool:
         self.depflag = new
         return True
 
-    def update_virtuals(self, virtuals: Tuple[str, ...]) -> bool:
+    def update_virtuals(self, virtuals: Iterable[str]) -> bool:
         """Update the list of provided virtuals"""
         old = self.virtuals
         self.virtuals = tuple(sorted(set(virtuals).union(self.virtuals)))
@@ -4752,33 +4752,51 @@ def merge_abstract_anonymous_specs(*abstract_specs: Spec):
     return merged_spec
 
 
-def reconstruct_virtuals_on_edges(spec):
-    """Reconstruct virtuals on edges. Used to read from old DB and reindex.
+def reconstruct_virtuals_on_edges(spec: Spec) -> None:
+    """Reconstruct virtuals on edges. Used to read from old DB and reindex."""
+    virtuals_needed: Dict[str, Set[str]] = {}
+    virtuals_provided: Dict[str, Set[str]] = {}
+    for edge in spec.traverse_edges(cover="edges", root=False):
+        parent_key = edge.parent.dag_hash()
+        if parent_key not in virtuals_needed:
+            # Construct which virtuals are needed by parent
+            virtuals_needed[parent_key] = set()
+            try:
+                parent_pkg = edge.parent.package
+            except Exception as e:
+                warnings.warn(
+                    f"cannot reconstruct virtual dependencies on {edge.parent.name}: {e}"
+                )
+                continue
 
-    Args:
-        spec: spec on which we want to reconstruct virtuals
-    """
-    # Collect all possible virtuals
-    possible_virtuals = set()
-    for node in spec.traverse():
-        try:
-            possible_virtuals.update(
-                {x for x in node.package.dependencies if spack.repo.PATH.is_virtual(x)}
+            virtuals_needed[parent_key].update(
+                name
+                for name, when_deps in parent_pkg.dependencies_by_name(when=True).items()
+                if spack.repo.PATH.is_virtual(name)
+                and any(edge.parent.satisfies(x) for x in when_deps)
             )
-        except Exception as e:
-            warnings.warn(f"cannot reconstruct virtual dependencies on package {node.name}: {e}")
+
+        if not virtuals_needed[parent_key]:
             continue
 
-    # Assume all incoming edges to provider are marked with virtuals=
-    for vspec in possible_virtuals:
-        try:
-            provider = spec[vspec]
-        except KeyError:
-            # Virtual not in the DAG
+        child_key = edge.spec.dag_hash()
+        if child_key not in virtuals_provided:
+            virtuals_provided[child_key] = set()
+            try:
+                child_pkg = edge.spec.package
+            except Exception as e:
+                warnings.warn(
+                    f"cannot reconstruct virtual dependencies on {edge.parent.name}: {e}"
+                )
+                continue
+            virtuals_provided[child_key].update(x.name for x in child_pkg.virtuals_provided)
+
+        if not virtuals_provided[child_key]:
             continue
 
-        for edge in provider.edges_from_dependents():
-            edge.update_virtuals([vspec])
+        virtuals_to_add = virtuals_needed[parent_key] & virtuals_provided[child_key]
+        if virtuals_to_add:
+            edge.update_virtuals(virtuals_to_add)
 
 
 class SpecfileReaderBase:

lib/spack/spack/test/spec_yaml.py

@@ -427,6 +427,9 @@ def test_load_json_specfiles(specfile, expected_hash, reader_cls):
     openmpi_edges = s2.edges_to_dependencies(name="openmpi")
     assert len(openmpi_edges) == 1
 
+    # Check that virtuals have been reconstructed
+    assert "mpi" in openmpi_edges[0].virtuals
+
     # The virtuals attribute must be a tuple, when read from a
     # JSON or YAML file, not a list
     for edge in s2.traverse_edges():