From 8ef5f1027a2b98ea72f70922dd7f068d50a9103c Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Fri, 14 Feb 2025 08:45:50 +0100 Subject: [PATCH] Spec.__getitem__: restrict to direct deps + transitive runtime deps (#49016) With this change spec["pkg"] searches only direct dependencies and transitive link/run dependencies, ordered by depth. This avoids situations where we pick up unwanted deps of build/test deps. To reach those, you need to do spec["build_dep"]["pkg"] explicitly. Co-authored-by: Massimiliano Culpo --- lib/spack/spack/spec.py | 16 +++++++--------- lib/spack/spack/test/spec_dag.py | 17 ++++++++++------- lib/spack/spack/test/spec_semantics.py | 6 +++--- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index dc5a4f5a09e..5f16d9e7ad5 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -3751,18 +3751,16 @@ def __getitem__(self, name: str): csv = query_parameters.pop().strip() query_parameters = re.split(r"\s*,\s*", csv) - order = lambda: itertools.chain( - self.traverse_edges(deptype=dt.LINK, order="breadth", cover="edges"), - self.edges_to_dependencies(depflag=dt.BUILD | dt.RUN | dt.TEST), - self.traverse_edges(deptype=dt.ALL, order="breadth", cover="edges"), + # Consider all direct dependencies and transitive runtime dependencies + order = itertools.chain( + self.edges_to_dependencies(depflag=dt.ALL), + self.traverse_edges(deptype=dt.LINK | dt.RUN, order="breadth", cover="edges"), ) - # Consider runtime dependencies and direct build/test deps before transitive dependencies, - # and prefer matches closest to the root. try: - edge = next((e for e in order() if e.spec.name == name or name in e.virtuals)) - except StopIteration: - raise KeyError(f"No spec with name {name} in {self}") + edge = next((e for e in order if e.spec.name == name or name in e.virtuals)) + except StopIteration as e: + raise KeyError(f"No spec with name {name} in {self}") from e if self._concrete: return SpecBuildInterface( diff --git a/lib/spack/spack/test/spec_dag.py b/lib/spack/spack/test/spec_dag.py index 1741cccc8f0..7ee0f0a44fc 100644 --- a/lib/spack/spack/test/spec_dag.py +++ b/lib/spack/spack/test/spec_dag.py @@ -585,8 +585,9 @@ def test_construct_spec_with_deptypes(self): assert s["b"].edges_to_dependencies(name="c")[0].depflag == dt.BUILD assert s["d"].edges_to_dependencies(name="e")[0].depflag == dt.BUILD | dt.LINK assert s["e"].edges_to_dependencies(name="f")[0].depflag == dt.RUN - - assert s["c"].edges_from_dependents(name="b")[0].depflag == dt.BUILD + # The subscript follows link/run transitive deps or direct build/test deps, therefore + # we need an extra step to get to "c" + assert s["b"]["c"].edges_from_dependents(name="b")[0].depflag == dt.BUILD assert s["e"].edges_from_dependents(name="d")[0].depflag == dt.BUILD | dt.LINK assert s["f"].edges_from_dependents(name="e")[0].depflag == dt.RUN @@ -898,8 +899,9 @@ def test_adding_same_deptype_with_the_same_name_raises( @pytest.mark.regression("33499") def test_indexing_prefers_direct_or_transitive_link_deps(): - # Test whether spec indexing prefers direct/transitive link type deps over deps of - # build/run/test deps, and whether it does fall back to a full dag search. + """Tests whether spec indexing prefers direct/transitive link/run type deps over deps of + build/test deps. + """ root = Spec("root") # Use a and z to since we typically traverse by edges sorted alphabetically. @@ -912,7 +914,7 @@ def test_indexing_prefers_direct_or_transitive_link_deps(): z3_flavor_1 = Spec("z3 +through_a1") z3_flavor_2 = Spec("z3 +through_z1") - root.add_dependency_edge(a1, depflag=dt.BUILD | dt.RUN | dt.TEST, virtuals=()) + root.add_dependency_edge(a1, depflag=dt.BUILD | dt.TEST, virtuals=()) # unique package as a dep of a build/run/test type dep. a1.add_dependency_edge(a2, depflag=dt.ALL, virtuals=()) @@ -927,8 +929,9 @@ def test_indexing_prefers_direct_or_transitive_link_deps(): assert "through_z1" in root["z3"].variants assert "through_a1" in a1["z3"].variants - # Ensure that the full DAG is still searched - assert root["a2"] + # Ensure that only the runtime sub-DAG can be searched + with pytest.raises(KeyError): + root["a2"] def test_getitem_sticks_to_subdag(): diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py index a4b046fb443..ca31f66fce2 100644 --- a/lib/spack/spack/test/spec_semantics.py +++ b/lib/spack/spack/test/spec_semantics.py @@ -700,9 +700,9 @@ def test_unsatisfiable_multi_value_variant(self, default_mock_concretization): def test_copy_satisfies_transitive(self): spec = spack.concretize.concretize_one("dttop") copy = spec.copy() - for s in spec.traverse(): - assert s.satisfies(copy[s.name]) - assert copy[s.name].satisfies(s) + for s, t in zip(spec.traverse(), copy.traverse()): + assert s.satisfies(t) + assert t.satisfies(s) def test_intersects_virtual(self): assert Spec("mpich").intersects(Spec("mpi"))