Splice differing virtual packages (#27919)

Co-authored-by: Nathan Hanford <hanford1@llnl.gov>

6 files changed, 148 insertions, 21 deletions

diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index e933984a06..bf9bbaa789 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -4603,8 +4603,8 @@ class Spec(object):
         | \
         Z<-H
         In this example, Spec T depends on H and Z, and H also depends on Z.
-        Suppose, however, that we wish to use a differently-built H, known as
-        H'. This function will splice in the new H' in one of two ways:
+        Suppose, however, that we wish to use a different H, known as H'. This
+        function will splice in the new H' in one of two ways:
         1. transitively, where H' depends on the Z' it was built with, and the
         new T* also directly depends on this new Z', or
         2. intransitively, where the new T* and H' both depend on the original
@@ -4617,10 +4617,34 @@ class Spec(object):
         """
         assert self.concrete
         assert other.concrete
-        assert other.name in self
 
-        # Check, for the time being, that we don't have DAG with multiple
-        # specs from the same package
+        virtuals_to_replace = [v.name for v in other.package.virtuals_provided
+                               if v in self]
+        if virtuals_to_replace:
+            deps_to_replace = dict((self[v], other) for v in virtuals_to_replace)
+            # deps_to_replace = [self[v] for v in virtuals_to_replace]
+        else:
+            # TODO: sanity check and error raise here for other.name not in self
+            deps_to_replace = {self[other.name]: other}
+            # deps_to_replace = [self[other.name]]
+
+        for d in deps_to_replace:
+            if not all(v in other.package.virtuals_provided or v not in self
+                       for v in d.package.virtuals_provided):
+                # There was something provided by the original that we don't
+                # get from its replacement.
+                raise SpliceError(("Splice between {0} and {1} will not provide "
+                                   "the same virtuals.").format(self.name, other.name))
+            for n in d.traverse(root=False):
+                if not all(any(v in other_n.package.virtuals_provided
+                               for other_n in other.traverse(root=False))
+                           or v not in self for v in n.package.virtuals_provided):
+                    raise SpliceError(("Splice between {0} and {1} will not provide "
+                                       "the same virtuals."
+                                       ).format(self.name, other.name))
+
+        # For now, check that we don't have DAG with multiple specs from the
+        # same package
         def multiple_specs(root):
             counter = collections.Counter([node.name for node in root.traverse()])
             _, max_number = counter.most_common()[0]
@@ -4638,29 +4662,43 @@ class Spec(object):
         # Keep all cached hashes because we will invalidate the ones that need
         # invalidating later, and we don't want to invalidate unnecessarily
 
+        def from_self(name, transitive):
+            if transitive:
+                if name in other:
+                    return False
+                if any(v in other for v in self[name].package.virtuals_provided):
+                    return False
+                return True
+            else:
+                if name == other.name:
+                    return False
+                if any(v in other.package.virtuals_provided
+                       for v in self[name].package.virtuals_provided):
+                    return False
+                return True
+
+        self_nodes = dict((s.name, s.copy(deps=False, caches=True))
+                          for s in self.traverse(root=True)
+                          if from_self(s.name, transitive))
+
         if transitive:
-            self_nodes = dict((s.name, s.copy(deps=False, caches=True))
-                              for s in self.traverse(root=True)
-                              if s.name not in other)
             other_nodes = dict((s.name, s.copy(deps=False, caches=True))
                                for s in other.traverse(root=True))
         else:
-            # If we're not doing a transitive splice, then we only want the
-            # root of other.
-            self_nodes = dict((s.name, s.copy(deps=False, caches=True))
-                              for s in self.traverse(root=True)
-                              if s.name != other.name)
-            other_nodes = {other.name: other.copy(deps=False, caches=True)}
+            # NOTE: Does not fully validate providers; loader races possible
+            other_nodes = dict((s.name, s.copy(deps=False, caches=True))
+                               for s in other.traverse(root=True)
+                               if s is other or s.name not in self)
 
         nodes = other_nodes.copy()
         nodes.update(self_nodes)
 
         for name in nodes:
-            # TODO: check if splice semantics is respected
             if name in self_nodes:
                 for edge in self[name].edges_to_dependencies():
+                    dep_name = deps_to_replace.get(edge.spec, edge.spec).name
                     nodes[name].add_dependency_edge(
-                        nodes[edge.spec.name], edge.deptypes
+                        nodes[dep_name], edge.deptypes
                     )
                 if any(dep not in self_nodes
                        for dep in self[name]._dependencies):
@@ -5506,3 +5544,8 @@ class SpecDeprecatedError(spack.error.SpecError):
 
 class InvalidSpecDetected(spack.error.SpecError):
     """Raised when a detected spec doesn't pass validation checks."""
+
+
+class SpliceError(spack.error.SpecError):
+    """Raised when a splice is not possible due to dependency or provider
+    satisfaction mismatch. The resulting splice would be unusable."""
diff --git a/lib/spack/spack/test/rewiring.py b/lib/spack/spack/test/rewiring.py
index f467c8e239..e15be34fdc 100644
--- a/lib/spack/spack/test/rewiring.py
+++ b/lib/spack/spack/test/rewiring.py
@@ -24,6 +24,7 @@ else:
 @pytest.mark.requires_executables(*args)
 @pytest.mark.parametrize('transitive', [True, False])
 def test_rewire(mock_fetch, install_mockery, transitive):
+    """Tests basic rewiring without binary executables."""
     spec = Spec('splice-t^splice-h~foo').concretized()
     dep = Spec('splice-h+foo').concretized()
     spec.package.do_install()
@@ -53,6 +54,7 @@ def test_rewire(mock_fetch, install_mockery, transitive):
 @pytest.mark.requires_executables(*args)
 @pytest.mark.parametrize('transitive', [True, False])
 def test_rewire_bin(mock_fetch, install_mockery, transitive):
+    """Tests basic rewiring with binary executables."""
     spec = Spec('quux').concretized()
     dep = Spec('garply cflags=-g').concretized()
     spec.package.do_install()
@@ -82,8 +84,8 @@ def test_rewire_bin(mock_fetch, install_mockery, transitive):
 
 @pytest.mark.requires_executables(*args)
 def test_rewire_writes_new_metadata(mock_fetch, install_mockery):
-    # check for spec.json and install_manifest.json and that they are new
-    # for a simple case.
+    """Tests that new metadata was written during a rewire.
+    Accuracy of metadata is left to other tests."""
     spec = Spec('quux').concretized()
     dep = Spec('garply cflags=-g').concretized()
     spec.package.do_install()
@@ -120,7 +122,7 @@ def test_rewire_writes_new_metadata(mock_fetch, install_mockery):
 @pytest.mark.requires_executables(*args)
 @pytest.mark.parametrize('transitive', [True, False])
 def test_uninstall_rewired_spec(mock_fetch, install_mockery, transitive):
-    # Test that rewired packages can be uninstalled as normal.
+    """Test that rewired packages can be uninstalled as normal."""
     spec = Spec('quux').concretized()
     dep = Spec('garply cflags=-g').concretized()
     spec.package.do_install()
@@ -134,6 +136,8 @@ def test_uninstall_rewired_spec(mock_fetch, install_mockery, transitive):
 
 @pytest.mark.requires_executables(*args)
 def test_rewire_not_installed_fails(mock_fetch, install_mockery):
+    """Tests error when an attempt is made to rewire a package that was not
+    previously installed."""
     spec = Spec('quux').concretized()
     dep = Spec('garply cflags=-g').concretized()
     spliced_spec = spec.splice(dep, False)
diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py
index fe8e07a54f..621815494d 100644
--- a/lib/spack/spack/test/spec_semantics.py
+++ b/lib/spack/spack/test/spec_semantics.py
@@ -995,8 +995,6 @@ class TestSpecSematics(object):
     def test_splice(self, transitive):
         # Tests the new splice function in Spec using a somewhat simple case
         # with a variant with a conditional dependency.
-        # TODO: Test being able to splice in different provider for a virtual.
-        # Example: mvapich for mpich.
         spec = Spec('splice-t')
         dep = Spec('splice-h+foo')
         spec.concretize()
@@ -1171,6 +1169,26 @@ class TestSpecSematics(object):
         s._add_dependency(d, ())
         assert s.satisfies('mpileaks ^zmpi ^fake', strict=True)
 
+    @pytest.mark.parametrize('transitive', [True, False])
+    def test_splice_swap_names(self, transitive):
+        spec = Spec('splice-t')
+        dep = Spec('splice-a+foo')
+        spec.concretize()
+        dep.concretize()
+        out = spec.splice(dep, transitive)
+        assert dep.name in out
+        assert transitive == ('+foo' in out['splice-z'])
+
+    @pytest.mark.parametrize('transitive', [True, False])
+    def test_splice_swap_names_mismatch_virtuals(self, transitive):
+        spec = Spec('splice-t')
+        dep = Spec('splice-vh+foo')
+        spec.concretize()
+        dep.concretize()
+        with pytest.raises(spack.spec.SpliceError,
+                           match='will not provide the same virtuals.'):
+            spec.splice(dep, transitive)
+
 
 @pytest.mark.regression('3887')
 @pytest.mark.parametrize('spec_str', [
diff --git a/var/spack/repos/builtin.mock/packages/splice-a/package.py b/var/spack/repos/builtin.mock/packages/splice-a/package.py
new file mode 100644
index 0000000000..ebd37777dd
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/splice-a/package.py
@@ -0,0 +1,30 @@
+# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack import *
+
+
+class SpliceA(Package):
+    """Simple package with one optional dependency"""
+
+    homepage = "http://www.example.com"
+    url      = "http://www.example.com/splice-a-1.0.tar.gz"
+
+    version('1.0', '0123456789abcdef0123456789efghij')
+
+    variant('foo', default=False, description='nope')
+    variant('bar', default=False, description='nope')
+    variant('baz', default=False, description='nope')
+
+    depends_on('splice-z')
+    depends_on('splice-z+foo', when='+foo')
+
+    provides('something')
+    provides('somethingelse')
+
+    def install(self, spec, prefix):
+        with open(prefix.join('splice-a'), 'w') as f:
+            f.write('splice-a: {0}'.format(prefix))
+            f.write('splice-z: {0}'.format(spec['splice-z'].prefix))
diff --git a/var/spack/repos/builtin.mock/packages/splice-h/package.py b/var/spack/repos/builtin.mock/packages/splice-h/package.py
index 8236b11766..b09c3b667a 100644
--- a/var/spack/repos/builtin.mock/packages/splice-h/package.py
+++ b/var/spack/repos/builtin.mock/packages/splice-h/package.py
@@ -21,6 +21,9 @@ class SpliceH(Package):
     depends_on('splice-z')
     depends_on('splice-z+foo', when='+foo')
 
+    provides('something')
+    provides('somethingelse')
+
     def install(self, spec, prefix):
         with open(prefix.join('splice-h'), 'w') as f:
             f.write('splice-h: {0}'.format(prefix))
diff --git a/var/spack/repos/builtin.mock/packages/splice-vh/package.py b/var/spack/repos/builtin.mock/packages/splice-vh/package.py
new file mode 100644
index 0000000000..e4310f0b42
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/splice-vh/package.py
@@ -0,0 +1,29 @@
+# Copyright 2013-2022 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack import *
+
+
+class SpliceVh(Package):
+    """Simple package with one optional dependency"""
+
+    homepage = "http://www.example.com"
+    url      = "http://www.example.com/splice-vh-1.0.tar.gz"
+
+    version('1.0', '0123456789abcdef0123456789abcdef')
+
+    variant('foo', default=False, description='nope')
+    variant('bar', default=False, description='nope')
+    variant('baz', default=False, description='nope')
+
+    depends_on('splice-z')
+    depends_on('splice-z+foo', when='+foo')
+
+    provides('something')
+
+    def install(self, spec, prefix):
+        with open(prefix.join('splice-vh'), 'w') as f:
+            f.write('splice-vh: {0}'.format(prefix))
+            f.write('splice-z: {0}'.format(spec['splice-z'].prefix))