From a3520d14bdf8f78b1a903560e18627ee00d24b0c Mon Sep 17 00:00:00 2001 From: Nathan Hanford <8302958+nhanford@users.noreply.github.com> Date: Tue, 12 Apr 2022 13:31:39 -0700 Subject: Splice differing virtual packages (#27919) Co-authored-by: Nathan Hanford --- lib/spack/spack/spec.py | 75 ++++++++++++++++++++++++++-------- lib/spack/spack/test/rewiring.py | 10 +++-- lib/spack/spack/test/spec_semantics.py | 22 +++++++++- 3 files changed, 86 insertions(+), 21 deletions(-) (limited to 'lib') 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', [ -- cgit v1.2.3-60-g2f50