From 949094544e479439e76581125a70c9c3c23cf997 Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Thu, 14 Oct 2021 12:33:10 +0200 Subject: Constrain abstract specs rather than concatenating strings in the "when" context manager (#26700) Using the Spec.constrain method doesn't work since it might trigger a repository lookup which could break our directives and triggers a circular import error. To fix that we introduce a function to merge abstract anonymous specs, based only on package names, which does not perform any lookup in the repository. --- lib/spack/docs/packaging_guide.rst | 2 +- lib/spack/spack/build_systems/cuda.py | 2 +- lib/spack/spack/directives.py | 13 ++++++++-- lib/spack/spack/spec.py | 28 ++++++++++++++++++++++ lib/spack/spack/test/directives.py | 23 ++++++++++++------ lib/spack/spack/test/spec_semantics.py | 16 +++++++++++++ .../packages/with-constraint-met/package.py | 5 +++- 7 files changed, 77 insertions(+), 12 deletions(-) diff --git a/lib/spack/docs/packaging_guide.rst b/lib/spack/docs/packaging_guide.rst index 2f42336d15..b9fbb18cf7 100644 --- a/lib/spack/docs/packaging_guide.rst +++ b/lib/spack/docs/packaging_guide.rst @@ -2824,7 +2824,7 @@ is equivalent to: depends_on('elpa+openmp', when='+openmp+elpa') -Constraints from nested context managers are also added together, but they are rarely +Constraints from nested context managers are also combined together, but they are rarely needed or recommended. .. _install-method: diff --git a/lib/spack/spack/build_systems/cuda.py b/lib/spack/spack/build_systems/cuda.py index 29ca46edff..1feeacd1c0 100644 --- a/lib/spack/spack/build_systems/cuda.py +++ b/lib/spack/spack/build_systems/cuda.py @@ -88,7 +88,7 @@ class CudaPackage(PackageBase): # Linux x86_64 compiler conflicts from here: # https://gist.github.com/ax3l/9489132 - with when('~allow-unsupported-compilers'): + with when('^cuda~allow-unsupported-compilers'): # GCC # According to # https://github.com/spack/spack/pull/25054#issuecomment-886531664 diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py index 91deedbb5a..863001a601 100644 --- a/lib/spack/spack/directives.py +++ b/lib/spack/spack/directives.py @@ -89,6 +89,9 @@ def make_when_spec(value): value indicating when a directive should be applied. """ + if isinstance(value, spack.spec.Spec): + return value + # Unsatisfiable conditions are discarded by the caller, and never # added to the package class if value is False: @@ -248,10 +251,16 @@ class DirectiveMeta(type): msg = msg.format(decorated_function.__name__) raise DirectiveError(msg) - when_spec_from_context = ' '.join( + when_constraints = [ + spack.spec.Spec(x) for x in DirectiveMeta._when_constraints_from_context + ] + if kwargs.get('when'): + when_constraints.append(spack.spec.Spec(kwargs['when'])) + when_spec = spack.spec.merge_abstract_anonymous_specs( + *when_constraints ) - when_spec = kwargs.get('when', '') + ' ' + when_spec_from_context + kwargs['when'] = when_spec # If any of the arguments are executors returned by a diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index ff71662a03..8b13374430 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -4433,6 +4433,34 @@ class Spec(object): return _spec_from_dict, (self.to_dict(hash=ht.build_hash),) +def merge_abstract_anonymous_specs(*abstract_specs): + """Merge the abstracts specs passed as input and return the result. + + The root specs must be anonymous, and it's duty of the caller to ensure that. + + This function merge the abstract specs based on package names. In particular + it doesn't try to resolve virtual dependencies. + + Args: + *abstract_specs (list of Specs): abstract specs to be merged + """ + merged_spec = spack.spec.Spec() + for current_spec_constraint in abstract_specs: + merged_spec.constrain(current_spec_constraint, deps=False) + + for name in merged_spec.common_dependencies(current_spec_constraint): + merged_spec[name].constrain( + current_spec_constraint[name], deps=False + ) + + # Update with additional constraints from other spec + for name in current_spec_constraint.dep_difference(merged_spec): + edge = current_spec_constraint.get_dependency(name) + merged_spec._add_dependency(edge.spec.copy(), edge.deptypes) + + return merged_spec + + def _spec_from_old_dict(data): """Construct a spec from JSON/YAML using the format version 1. Note: Version 1 format has no notion of a build_spec, and names are diff --git a/lib/spack/spack/test/directives.py b/lib/spack/spack/test/directives.py index a40eff6b71..31038ebb47 100644 --- a/lib/spack/spack/test/directives.py +++ b/lib/spack/spack/test/directives.py @@ -2,9 +2,10 @@ # Spack Project Developers. See the top-level COPYRIGHT file for details. # # SPDX-License-Identifier: (Apache-2.0 OR MIT) +import pytest import spack.repo -from spack.spec import Spec +import spack.spec def test_false_directives_do_not_exist(mock_packages): @@ -24,21 +25,29 @@ def test_true_directives_exist(mock_packages): cls = spack.repo.path.get_pkg_class('when-directives-true') assert cls.dependencies - assert Spec() in cls.dependencies['extendee'] - assert Spec() in cls.dependencies['b'] + assert spack.spec.Spec() in cls.dependencies['extendee'] + assert spack.spec.Spec() in cls.dependencies['b'] assert cls.resources - assert Spec() in cls.resources + assert spack.spec.Spec() in cls.resources assert cls.patches - assert Spec() in cls.patches + assert spack.spec.Spec() in cls.patches def test_constraints_from_context(mock_packages): pkg_cls = spack.repo.path.get_pkg_class('with-constraint-met') assert pkg_cls.dependencies - assert Spec('@1.0') in pkg_cls.dependencies['b'] + assert spack.spec.Spec('@1.0') in pkg_cls.dependencies['b'] assert pkg_cls.conflicts - assert (Spec('@1.0'), None) in pkg_cls.conflicts['%gcc'] + assert (spack.spec.Spec('+foo@1.0'), None) in pkg_cls.conflicts['%gcc'] + + +@pytest.mark.regression('26656') +def test_constraints_from_context_are_merged(mock_packages): + pkg_cls = spack.repo.path.get_pkg_class('with-constraint-met') + + assert pkg_cls.dependencies + assert spack.spec.Spec('@0.14:15 ^b@3.8:4.0') in pkg_cls.dependencies['c'] diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py index 97351b0ce2..a3952397cf 100644 --- a/lib/spack/spack/test/spec_semantics.py +++ b/lib/spack/spack/test/spec_semantics.py @@ -1212,3 +1212,19 @@ def test_spec_dict_hashless_dep(): } } ) + + +@pytest.mark.parametrize('specs,expected', [ + # Anonymous specs without dependencies + (['+baz', '+bar'], '+baz+bar'), + (['@2.0:', '@:5.1', '+bar'], '@2.0:5.1 +bar'), + # Anonymous specs with dependencies + (['^mpich@3.2', '^mpich@:4.0+foo'], '^mpich@3.2 +foo'), + # Mix a real package with a virtual one. This test + # should fail if we start using the repository + (['^mpich@3.2', '^mpi+foo'], '^mpich@3.2 ^mpi+foo'), +]) +def test_merge_abstract_anonymous_specs(specs, expected): + specs = [Spec(x) for x in specs] + result = spack.spec.merge_abstract_anonymous_specs(*specs) + assert result == Spec(expected) diff --git a/var/spack/repos/builtin.mock/packages/with-constraint-met/package.py b/var/spack/repos/builtin.mock/packages/with-constraint-met/package.py index 00c786f0b9..137e0be862 100644 --- a/var/spack/repos/builtin.mock/packages/with-constraint-met/package.py +++ b/var/spack/repos/builtin.mock/packages/with-constraint-met/package.py @@ -17,4 +17,7 @@ class WithConstraintMet(Package): with when('@1.0'): depends_on('b') - conflicts('%gcc') + conflicts('%gcc', when='+foo') + + with when('@0.14: ^b@:4.0'): + depends_on('c', when='@:15 ^b@3.8:') -- cgit v1.2.3-70-g09d2