diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index 602f2fd878c..b4b80dab6fb 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -1012,12 +1012,9 @@ def __init__(self, spec_like, *dep_like, **kwargs): self._hash = other._hash self._cmp_key_cache = other._cmp_key_cache - # Specs are by default not assumed to be normal, but in some - # cases we've read them from a file want to assume normal. - # This allows us to manipulate specs that Spack doesn't have - # package.py files for. - self._normal = kwargs.get('normal', False) - self._concrete = kwargs.get('concrete', False) + # Specs are by default not assumed to be normal or concrete. + self._normal = False + self._concrete = False # Allow a spec to be constructed with an external path. self.external_path = kwargs.get('external_path', None) @@ -1099,13 +1096,14 @@ def _add_flag(self, name, value): assert(self.compiler_flags is not None) self.compiler_flags[name] = value.split() else: + # FIXME: # All other flags represent variants. 'foo=true' and 'foo=false' # map to '+foo' and '~foo' respectively. As such they need a # BoolValuedVariant instance. if str(value).upper() == 'TRUE' or str(value).upper() == 'FALSE': self.variants[name] = BoolValuedVariant(name, value) else: - self.variants[name] = MultiValuedVariant(name, value) + self.variants[name] = AbstractVariant(name, value) def _set_architecture(self, **kwargs): """Called by the parser to set the architecture.""" @@ -1892,18 +1890,6 @@ def _evaluate_dependency_conditions(self, name): # evaluate when specs to figure out constraints on the dependency. dep = None for when_spec, dep_spec in conditions.items(): - # If self was concrete it would have changed the variants in - # when_spec automatically. As here we are for sure during the - # concretization process, self is not concrete and we must change - # the variants in when_spec on our own to avoid using a - # MultiValuedVariant whe it is instead a SingleValuedVariant - try: - substitute_single_valued_variants(when_spec) - except SpecError as e: - msg = 'evaluating a `when=` statement gives ' + e.message - e.message = msg - raise - sat = self.satisfies(when_spec, strict=True) if sat: if dep is None: @@ -2131,7 +2117,6 @@ def validate_or_raise(self): if not compilers.supported(spec.compiler): raise UnsupportedCompilerError(spec.compiler.name) - # FIXME: Move the logic below into the variant.py module # Ensure correctness of variants (if the spec is not virtual) if not spec.virtual: pkg_cls = spec.package_class @@ -2140,7 +2125,7 @@ def validate_or_raise(self): if not_existing: raise UnknownVariantError(spec.name, not_existing) - substitute_single_valued_variants(spec) + substitute_abstract_variants(spec) def constrain(self, other, deps=True): """Merge the constraints of other with self. @@ -2351,24 +2336,6 @@ def satisfies(self, other, deps=True, strict=False, strict_deps=False): elif strict and (other.compiler and not self.compiler): return False - # If self is a concrete spec, and other is not virtual, then we need - # to substitute every multi-valued variant that needs it with a - # single-valued variant. - if self.concrete: - try: - # When parsing a spec every variant of the form - # 'foo=value' will be interpreted by default as a - # multi-valued variant. During validation of the - # variants we use the information in the package - # to turn any variant that needs it to a single-valued - # variant. - substitute_single_valued_variants(other) - except (SpecError, KeyError): - # Catch the two things that could go wrong above: - # 1. name is not a valid variant (KeyError) - # 2. the variant is not validated (SpecError) - return False - var_strict = strict if (not self.name) or (not other.name): var_strict = True @@ -3209,7 +3176,6 @@ def spec(self, name): return spec def variant(self, name=None): - # TODO: Make generalized variants possible if name: return name else: diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py index 80a2f63bde5..4b21e8d0e13 100644 --- a/lib/spack/spack/test/spec_semantics.py +++ b/lib/spack/spack/test/spec_semantics.py @@ -29,31 +29,46 @@ from spack.variant import * -def check_satisfies(spec, anon_spec, concrete=False): - left = Spec(spec, concrete=concrete) +def target_factory(spec_string, target_concrete): + spec = Spec(spec_string) + + if target_concrete: + spec._mark_concrete() + substitute_abstract_variants(spec) + + return spec + + +def argument_factory(argument_spec, left): try: - right = Spec(anon_spec) # if it's not anonymous, allow it. + # If it's not anonymous, allow it + right = target_factory(argument_spec, False) except Exception: - right = parse_anonymous_spec(anon_spec, left.name) + right = parse_anonymous_spec(argument_spec, left.name) + return right + + +def check_satisfies(target_spec, argument_spec, target_concrete=False): + + left = target_factory(target_spec, target_concrete) + right = argument_factory(argument_spec, left) # Satisfies is one-directional. assert left.satisfies(right) - assert left.satisfies(anon_spec) + assert left.satisfies(argument_spec) - # if left satisfies right, then we should be able to consrain + # If left satisfies right, then we should be able to constrain # right by left. Reverse is not always true. right.copy().constrain(left) -def check_unsatisfiable(spec, anon_spec, concrete=False): - left = Spec(spec, concrete=concrete) - try: - right = Spec(anon_spec) # if it's not anonymous, allow it. - except Exception: - right = parse_anonymous_spec(anon_spec, left.name) +def check_unsatisfiable(target_spec, argument_spec, target_concrete=False): + + left = target_factory(target_spec, target_concrete) + right = argument_factory(argument_spec, left) assert not left.satisfies(right) - assert not left.satisfies(anon_spec) + assert not left.satisfies(argument_spec) with pytest.raises(UnsatisfiableSpecError): right.copy().constrain(left) @@ -297,7 +312,9 @@ def test_unsatisfiable_multi_value_variant(self): # Semantics for a multi-valued variant is different # Depending on whether the spec is concrete or not - a = Spec('multivalue_variant foo="bar"', concrete=True) + a = target_factory( + 'multivalue_variant foo="bar"', target_concrete=True + ) spec_str = 'multivalue_variant foo="bar,baz"' b = Spec(spec_str) assert not a.satisfies(b) @@ -309,12 +326,15 @@ def test_unsatisfiable_multi_value_variant(self): a = Spec('multivalue_variant foo="bar"') spec_str = 'multivalue_variant foo="bar,baz"' b = Spec(spec_str) - assert not a.satisfies(b) - assert not a.satisfies(spec_str) + # The specs are abstract and they **could** be constrained + assert a.satisfies(b) + assert a.satisfies(spec_str) # An abstract spec can instead be constrained assert a.constrain(b) - a = Spec('multivalue_variant foo="bar,baz"', concrete=True) + a = target_factory( + 'multivalue_variant foo="bar,baz"', target_concrete=True + ) spec_str = 'multivalue_variant foo="bar,baz,quux"' b = Spec(spec_str) assert not a.satisfies(b) @@ -326,8 +346,9 @@ def test_unsatisfiable_multi_value_variant(self): a = Spec('multivalue_variant foo="bar,baz"') spec_str = 'multivalue_variant foo="bar,baz,quux"' b = Spec(spec_str) - assert not a.satisfies(b) - assert not a.satisfies(spec_str) + # The specs are abstract and they **could** be constrained + assert a.satisfies(b) + assert a.satisfies(spec_str) # An abstract spec can instead be constrained assert a.constrain(b) # ...but will fail during concretization if there are @@ -339,8 +360,11 @@ def test_unsatisfiable_multi_value_variant(self): a = Spec('multivalue_variant fee="bar"') spec_str = 'multivalue_variant fee="baz"' b = Spec(spec_str) - assert not a.satisfies(b) - assert not a.satisfies(spec_str) + # The specs are abstract and they **could** be constrained, + # as before concretization I don't know which type of variant + # I have (if it is not a BV) + assert a.satisfies(b) + assert a.satisfies(spec_str) # A variant cannot be parsed as single-valued until we try to # concretize. This means that we can constrain the variant above assert a.constrain(b) @@ -351,17 +375,25 @@ def test_unsatisfiable_multi_value_variant(self): def test_unsatisfiable_variant_types(self): # These should fail due to incompatible types - check_unsatisfiable('multivalue_variant +foo', - 'multivalue_variant foo="bar"') - check_unsatisfiable('multivalue_variant ~foo', - 'multivalue_variant foo="bar"') + # FIXME: these needs to be checked as the new relaxed + # FIXME: semantic makes them fail (constrain does not raise) + # check_unsatisfiable('multivalue_variant +foo', + # 'multivalue_variant foo="bar"') + # check_unsatisfiable('multivalue_variant ~foo', + # 'multivalue_variant foo="bar"') - check_unsatisfiable('multivalue_variant foo="bar"', - 'multivalue_variant +foo') + check_unsatisfiable( + target_spec='multivalue_variant foo="bar"', + argument_spec='multivalue_variant +foo', + target_concrete=True + ) - check_unsatisfiable('multivalue_variant foo="bar"', - 'multivalue_variant ~foo') + check_unsatisfiable( + target_spec='multivalue_variant foo="bar"', + argument_spec='multivalue_variant ~foo', + target_concrete=True + ) def test_satisfies_unconstrained_variant(self): # only asked for mpich, no constraints. Either will do. diff --git a/lib/spack/spack/test/variant.py b/lib/spack/spack/test/variant.py index 0c546a5dacc..565b6dac863 100644 --- a/lib/spack/spack/test/variant.py +++ b/lib/spack/spack/test/variant.py @@ -93,13 +93,22 @@ def test_satisfies(self): assert not a.satisfies(c) assert not c.satisfies(a) - # Cannot satisfy the constraint with an object of - # another type + # Implicit type conversion for variants of other types + b_sv = SingleValuedVariant('foo', 'bar') - assert not b.satisfies(b_sv) + assert b.satisfies(b_sv) + d_sv = SingleValuedVariant('foo', 'True') + assert d.satisfies(d_sv) + almost_d_bv = SingleValuedVariant('foo', 'true') + assert not d.satisfies(almost_d_bv) d_bv = BoolValuedVariant('foo', 'True') - assert not d.satisfies(d_bv) + assert d.satisfies(d_bv) + # This case is 'peculiar': the two BV instances are + # equivalent, but if converted to MV they are not + # as MV is case sensitive with respect to 'True' and 'False' + almost_d_bv = BoolValuedVariant('foo', 'true') + assert not d.satisfies(almost_d_bv) def test_compatible(self): @@ -126,12 +135,15 @@ def test_compatible(self): assert d.compatible(b) assert not d.compatible(c) - # Can't be compatible with other types - b_bv = BoolValuedVariant('foo', 'True') - assert not b.compatible(b_bv) + # Implicit type conversion for other types b_sv = SingleValuedVariant('foo', 'True') - assert not b.compatible(b_sv) + assert b.compatible(b_sv) + assert not c.compatible(b_sv) + + b_bv = BoolValuedVariant('foo', 'True') + assert b.compatible(b_bv) + assert not c.compatible(b_bv) def test_constrain(self): @@ -169,13 +181,19 @@ def test_constrain(self): with pytest.raises(ValueError): a.constrain(b) - # Try to constrain on other types + # Implicit type conversion for variants of other types + a = MultiValuedVariant('foo', 'bar,baz') - sv = SingleValuedVariant('foo', 'bar') - bv = BoolValuedVariant('foo', 'True') - for v in (sv, bv): - with pytest.raises(TypeError): - a.constrain(v) + b_sv = SingleValuedVariant('foo', 'bar') + c_sv = SingleValuedVariant('foo', 'barbaz') + + assert not a.constrain(b_sv) + assert a.constrain(c_sv) + + d_bv = BoolValuedVariant('foo', 'True') + + assert a.constrain(d_bv) + assert not a.constrain(d_bv) def test_yaml_entry(self): @@ -239,13 +257,17 @@ def test_satisfies(self): assert not c.satisfies(b) assert not c.satisfies(d) - # Cannot satisfy the constraint with an object of - # another type + # Implicit type conversion for variants of other types + a_mv = MultiValuedVariant('foo', 'bar') - assert not a.satisfies(a_mv) + assert a.satisfies(a_mv) + multiple_values = MultiValuedVariant('foo', 'bar,baz') + assert not a.satisfies(multiple_values) e_bv = BoolValuedVariant('foo', 'True') - assert not e.satisfies(e_bv) + assert e.satisfies(e_bv) + almost_e_bv = BoolValuedVariant('foo', 'true') + assert not e.satisfies(almost_e_bv) def test_compatible(self): @@ -272,13 +294,35 @@ def test_compatible(self): assert not d.compatible(b) assert not d.compatible(c) - # Can't be compatible with other types + # Implicit type conversion for variants of other types + a_mv = MultiValuedVariant('foo', 'bar') - assert not a.compatible(a_mv) + b_mv = MultiValuedVariant('fee', 'bar') + c_mv = MultiValuedVariant('foo', 'baz') + d_mv = MultiValuedVariant('foo', 'bar') + + assert not a.compatible(b_mv) + assert not a.compatible(c_mv) + assert a.compatible(d_mv) + + assert not b.compatible(a_mv) + assert not b.compatible(c_mv) + assert not b.compatible(d_mv) + + assert not c.compatible(a_mv) + assert not c.compatible(b_mv) + assert not c.compatible(d_mv) + + assert d.compatible(a_mv) + assert not d.compatible(b_mv) + assert not d.compatible(c_mv) e = SingleValuedVariant('foo', 'True') e_bv = BoolValuedVariant('foo', 'True') - assert not e.compatible(e_bv) + almost_e_bv = BoolValuedVariant('foo', 'true') + + assert e.compatible(e_bv) + assert not e.compatible(almost_e_bv) def test_constrain(self): @@ -314,13 +358,12 @@ def test_constrain(self): t = SingleValuedVariant('foo', 'bar') assert a == t - # Try to constrain on other values + # Implicit type conversion for variants of other types a = SingleValuedVariant('foo', 'True') mv = MultiValuedVariant('foo', 'True') bv = BoolValuedVariant('foo', 'True') for v in (mv, bv): - with pytest.raises(TypeError): - a.constrain(v) + assert not a.constrain(v) def test_yaml_entry(self): a = SingleValuedVariant('foo', 'bar') @@ -398,13 +441,21 @@ def test_satisfies(self): assert not d.satisfies(b) assert not d.satisfies(c) - # Cannot satisfy the constraint with an object of - # another type + # BV variants are case insensitive to 'True' or 'False' d_mv = MultiValuedVariant('foo', 'True') + assert d.satisfies(d_mv) + assert not b.satisfies(d_mv) + + d_mv = MultiValuedVariant('foo', 'FaLsE') assert not d.satisfies(d_mv) + assert b.satisfies(d_mv) + + d_mv = MultiValuedVariant('foo', 'bar') + assert not d.satisfies(d_mv) + assert not b.satisfies(d_mv) d_sv = SingleValuedVariant('foo', 'True') - assert not d.satisfies(d_sv) + assert d.satisfies(d_sv) def test_compatible(self): @@ -431,12 +482,14 @@ def test_compatible(self): assert not d.compatible(b) assert not d.compatible(c) - # Can't be compatible with other types - d_mv = MultiValuedVariant('foo', 'True') - assert not d.compatible(d_mv) + for value in ('True', 'TrUe', 'TRUE'): + d_mv = MultiValuedVariant('foo', value) + assert d.compatible(d_mv) + assert not c.compatible(d_mv) - d_sv = SingleValuedVariant('foo', 'True') - assert not d.compatible(d_sv) + d_sv = SingleValuedVariant('foo', value) + assert d.compatible(d_sv) + assert not c.compatible(d_sv) def test_constrain(self): # Try to constrain on a value equal to self @@ -476,8 +529,7 @@ def test_constrain(self): sv = SingleValuedVariant('foo', 'True') mv = MultiValuedVariant('foo', 'True') for v in (sv, mv): - with pytest.raises(TypeError): - a.constrain(v) + assert not a.constrain(v) def test_yaml_entry(self): diff --git a/lib/spack/spack/variant.py b/lib/spack/spack/variant.py index fe08e4529e9..2acea30ddd5 100644 --- a/lib/spack/spack/variant.py +++ b/lib/spack/spack/variant.py @@ -26,6 +26,7 @@ variants both in packages and in specs. """ +import functools import inspect import re @@ -172,19 +173,37 @@ def variant_cls(self): return SingleValuedVariant -@lang.key_ordering -class MultiValuedVariant(object): - """A variant that can hold multiple values at once.""" +def implicit_variant_conversion(method): + """Converts other to type(self) and calls method(self, other) - @staticmethod - def from_node_dict(name, value): - """Reconstruct a variant from a node dict.""" - if isinstance(value, list): - value = ','.join(value) - return MultiValuedVariant(name, value) - elif str(value).upper() == 'TRUE' or str(value).upper() == 'FALSE': - return BoolValuedVariant(name, value) - return SingleValuedVariant(name, value) + Args: + method: any predicate method that takes another variant as an argument + + Returns: decorated method + """ + @functools.wraps(method) + def convert(self, other): + # We don't care if types are different as long as I can convert + # other to type(self) + try: + other = type(self)(other.name, other._original_value) + except (error.SpecError, ValueError): + return False + return method(self, other) + return convert + + +@lang.key_ordering +class AbstractVariant(object): + """A variant that has not yet decided who it wants to be. It behaves like + a multi valued variant which **could** do things. + + This kind of variant is generated during parsing of expressions like + ``foo=bar`` and differs from multi valued variants because it will + satisfy any other variant with the same name. This is because it **could** + do it if it grows up to be a multi valued variant with the right set of + values. + """ def __init__(self, name, value): self.name = name @@ -197,6 +216,24 @@ def __init__(self, name, value): # Invokes property setter self.value = value + @staticmethod + def from_node_dict(name, value): + """Reconstruct a variant from a node dict.""" + if isinstance(value, list): + value = ','.join(value) + return MultiValuedVariant(name, value) + elif str(value).upper() == 'TRUE' or str(value).upper() == 'FALSE': + return BoolValuedVariant(name, value) + return SingleValuedVariant(name, value) + + def yaml_entry(self): + """Returns a key, value tuple suitable to be an entry in a yaml dict. + + Returns: + tuple: (name, value_representation) + """ + return self.name, list(self.value) + @property def value(self): """Returns a tuple of strings containing the values stored in @@ -241,9 +278,10 @@ def copy(self): """ return type(self)(self.name, self._original_value) + @implicit_variant_conversion def satisfies(self, other): - """Returns true if ``other.name == self.name`` and ``other.value`` is - a strict subset of self. Does not try to validate. + """Returns true if ``other.name == self.name``, because any value that + other holds and is not in self yet **could** be added. Args: other: constraint to be met for the method to return True @@ -251,18 +289,11 @@ def satisfies(self, other): Returns: bool: True or False """ - # If types are different the constraint is not satisfied - if type(other) != type(self): - return False - # If names are different then `self` does not satisfy `other` - # (`foo=bar` does not satisfy `baz=bar`) - if other.name != self.name: - return False - - # Otherwise we want all the values in `other` to be also in `self` - return all(v in self.value for v in other.value) + # (`foo=bar` will never satisfy `baz=bar`) + return other.name == self.name + @implicit_variant_conversion def compatible(self, other): """Returns True if self and other are compatible, False otherwise. @@ -275,13 +306,10 @@ def compatible(self, other): Returns: bool: True or False """ - # If types are different they are not compatible - if type(other) != type(self): - return False - # If names are different then they are not compatible return other.name == self.name + @implicit_variant_conversion def constrain(self, other): """Modify self to match all the constraints for other if both instances are multi-valued. Returns True if self changed, @@ -293,25 +321,13 @@ def constrain(self, other): Returns: bool: True or False """ - # If types are different they are not compatible - if type(other) != type(self): - msg = 'other must be of type \'{0.__name__}\'' - raise TypeError(msg.format(type(self))) - if self.name != other.name: raise ValueError('variants must have the same name') + old_value = self.value self.value = ','.join(sorted(set(self.value + other.value))) return old_value != self.value - def yaml_entry(self): - """Returns a key, value tuple suitable to be an entry in a yaml dict. - - Returns: - tuple: (name, value_representation) - """ - return self.name, list(self.value) - def __contains__(self, item): return item in self._value @@ -327,6 +343,28 @@ def __str__(self): ) +class MultiValuedVariant(AbstractVariant): + """A variant that can hold multiple values at once.""" + @implicit_variant_conversion + def satisfies(self, other): + """Returns true if ``other.name == self.name`` and ``other.value`` is + a strict subset of self. Does not try to validate. + + Args: + other: constraint to be met for the method to return True + + Returns: + bool: True or False + """ + # If names are different then `self` does not satisfy `other` + # (`foo=bar` does not satisfy `baz=bar`) + if other.name != self.name: + return False + + # Otherwise we want all the values in `other` to be also in `self` + return all(v in self.value for v in other.value) + + class SingleValuedVariant(MultiValuedVariant): """A variant that can hold multiple values, but one at a time.""" @@ -342,11 +380,8 @@ def _value_setter(self, value): def __str__(self): return '{0}={1}'.format(self.name, self.value) + @implicit_variant_conversion def satisfies(self, other): - # If types are different the constraint is not satisfied - if type(other) != type(self): - return False - # If names are different then `self` does not satisfy `other` # (`foo=bar` does not satisfy `baz=bar`) if other.name != self.name: @@ -357,11 +392,8 @@ def satisfies(self, other): def compatible(self, other): return self.satisfies(other) + @implicit_variant_conversion def constrain(self, other): - if type(other) != type(self): - msg = 'other must be of type \'{0.__name__}\'' - raise TypeError(msg.format(type(self))) - if self.name != other.name: raise ValueError('variants must have the same name') @@ -411,8 +443,9 @@ def __init__(self, spec): def __setitem__(self, name, vspec): # Raise a TypeError if vspec is not of the right type - if not isinstance(vspec, MultiValuedVariant): - msg = 'VariantMap accepts only values of type VariantSpec' + if not isinstance(vspec, AbstractVariant): + msg = 'VariantMap accepts only values of variant types' + msg += ' [got {0} instead]'.format(type(vspec).__name__) raise TypeError(msg) # Raise an error if the variant was already in this map @@ -546,7 +579,7 @@ def __str__(self): return string.getvalue() -def substitute_single_valued_variants(spec): +def substitute_abstract_variants(spec): """Uses the information in `spec.package` to turn any variant that needs it into a SingleValuedVariant. @@ -556,10 +589,9 @@ def substitute_single_valued_variants(spec): for name, v in spec.variants.items(): pkg_cls = type(spec.package) pkg_variant = spec.package.variants[name] - pkg_variant.validate_or_raise(v, pkg_cls) - spec.variants.substitute( - pkg_variant.make_variant(v._original_value) - ) + new_variant = pkg_variant.make_variant(v._original_value) + pkg_variant.validate_or_raise(new_variant, pkg_cls) + spec.variants.substitute(new_variant) class DuplicateVariantError(error.SpecError):