Change 'any' to wildcard for variants (#19381)

This commit is contained in:
Greg Becker 2020-10-20 23:05:29 -07:00 committed by GitHub
parent 0e6b818c52
commit a5faf7d27a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 49 additions and 103 deletions

View File

@ -352,18 +352,13 @@ def concretize_variants(self, spec):
preferred_variants = PackagePrefs.preferred_variants(spec.name) preferred_variants = PackagePrefs.preferred_variants(spec.name)
pkg_cls = spec.package_class pkg_cls = spec.package_class
for name, variant in pkg_cls.variants.items(): for name, variant in pkg_cls.variants.items():
any_set = False
var = spec.variants.get(name, None) var = spec.variants.get(name, None)
if var and 'any' in var: if var and '*' in var:
# remove 'any' variant before concretizing # remove variant wildcard before concretizing
# 'any' cannot be combined with other variables in a # wildcard cannot be combined with other variables in a
# multivalue variant, a concrete variant cannot have the value # multivalue variant, a concrete variant cannot have the value
# 'any', and 'any' does not constrain a variant except to # wildcard, and a wildcard does not constrain a variant
# preclude the values 'none' and None. We track `any_set` to
# avoid replacing 'any' with None, and remove it to continue
# concretization.
spec.variants.pop(name) spec.variants.pop(name)
any_set = True
if name not in spec.variants: if name not in spec.variants:
changed = True changed = True
if name in preferred_variants: if name in preferred_variants:
@ -371,14 +366,6 @@ def concretize_variants(self, spec):
else: else:
spec.variants[name] = variant.make_default() spec.variants[name] = variant.make_default()
var = spec.variants[name]
if any_set and 'none' in var or None in var:
msg = "Attempted non-deterministic setting of variant"
msg += " '%s' set to 'any' and preference is." % name
msg += "'%s'. Set the variant to a non 'any'" % var.value
msg += " value or set a preference for variant '%s'." % name
raise NonDeterministicVariantError(msg)
return changed return changed
def concretize_compiler(self, spec): def concretize_compiler(self, spec):
@ -805,7 +792,3 @@ def __init__(self, spec):
msg = ("The spec\n '%s'\n is configured as not buildable, " msg = ("The spec\n '%s'\n is configured as not buildable, "
"and no matching external installs were found") "and no matching external installs were found")
super(NoBuildError, self).__init__(msg % spec) super(NoBuildError, self).__init__(msg % spec)
class NonDeterministicVariantError(spack.error.SpecError):
"""Raised when a spec variant is set to 'any' and concretizes to 'none'."""

View File

@ -1336,21 +1336,21 @@ def _spec_needs_overwrite(self, spec):
# if spec and all deps aren't dev builds, we don't need to overwrite it # if spec and all deps aren't dev builds, we don't need to overwrite it
if not any(spec.satisfies(c) if not any(spec.satisfies(c)
for c in ('dev_path=any', '^dev_path=any')): for c in ('dev_path=*', '^dev_path=*')):
return False return False
# if any dep needs overwrite, or any dep is missing and is a dev build # if any dep needs overwrite, or any dep is missing and is a dev build
# then overwrite this package # then overwrite this package
if any( if any(
self._spec_needs_overwrite(dep) or self._spec_needs_overwrite(dep) or
((not dep.package.installed) and dep.satisfies('dev_path=any')) ((not dep.package.installed) and dep.satisfies('dev_path=*'))
for dep in spec.traverse(root=False) for dep in spec.traverse(root=False)
): ):
return True return True
# if it's not a direct dev build and its dependencies haven't # if it's not a direct dev build and its dependencies haven't
# changed, it hasn't changed. # changed, it hasn't changed.
# We don't merely check satisfaction (spec.satisfies('dev_path=any') # We don't merely check satisfaction (spec.satisfies('dev_path=*')
# because we need the value of the variant in the next block of code # because we need the value of the variant in the next block of code
dev_path_var = spec.variants.get('dev_path', None) dev_path_var = spec.variants.get('dev_path', None)
if not dev_path_var: if not dev_path_var:
@ -1420,8 +1420,8 @@ def install_all(self, args=None):
for concretized_hash in self.concretized_order: for concretized_hash in self.concretized_order:
spec = self.specs_by_hash[concretized_hash] spec = self.specs_by_hash[concretized_hash]
if not spec.package.installed or ( if not spec.package.installed or (
spec.satisfies('dev_path=any') or spec.satisfies('dev_path=*') or
spec.satisfies('^dev_path=any') spec.satisfies('^dev_path=*')
): ):
# If it's a dev build it could need to be reinstalled # If it's a dev build it could need to be reinstalled
specs_to_install.append(spec) specs_to_install.append(spec)

View File

@ -333,7 +333,7 @@ def test_dev_build_env_dependency(tmpdir, mock_packages, install_mockery,
# Ensure variants set properly # Ensure variants set properly
for dep in (dep_spec, spec['dev-build-test-install']): for dep in (dep_spec, spec['dev-build-test-install']):
assert dep.satisfies('dev_path=%s' % build_dir) assert dep.satisfies('dev_path=%s' % build_dir)
assert spec.satisfies('^dev_path=any') assert spec.satisfies('^dev_path=*')
@pytest.mark.parametrize('test_spec', ['dev-build-test-install', @pytest.mark.parametrize('test_spec', ['dev-build-test-install',

View File

@ -35,8 +35,8 @@ def test_undevelop(tmpdir, mock_packages, mutable_mock_env_path):
after = spack.spec.Spec('mpich').concretized() after = spack.spec.Spec('mpich').concretized()
# Removing dev spec from environment changes concretization # Removing dev spec from environment changes concretization
assert before.satisfies('dev_path=any') assert before.satisfies('dev_path=*')
assert not after.satisfies('dev_path=any') assert not after.satisfies('dev_path=*')
def test_undevelop_nonexistent(tmpdir, mock_packages, mutable_mock_env_path): def test_undevelop_nonexistent(tmpdir, mock_packages, mutable_mock_env_path):

View File

@ -9,7 +9,6 @@
import spack.package_prefs import spack.package_prefs
import spack.repo import spack.repo
import spack.util.spack_yaml as syaml import spack.util.spack_yaml as syaml
from spack.concretize import NonDeterministicVariantError
from spack.config import ConfigScope, ConfigError from spack.config import ConfigScope, ConfigError
from spack.spec import Spec from spack.spec import Spec
from spack.version import Version from spack.version import Version
@ -85,23 +84,15 @@ def test_preferred_variants(self):
'mpileaks', debug=True, opt=True, shared=False, static=False 'mpileaks', debug=True, opt=True, shared=False, static=False
) )
def test_preferred_variants_from_any(self): def test_preferred_variants_from_wildcard(self):
""" """
Test that 'foo=any' concretizes to any non-none value Test that 'foo=*' concretizes to any value
Test that concretization of variants raises an error attempting
non-deterministic concretization from 'any' when preferred value is
'none'.
""" """
update_packages('multivalue-variant', 'variants', 'foo=bar') update_packages('multivalue-variant', 'variants', 'foo=bar')
assert_variant_values( assert_variant_values(
'multivalue-variant foo=any', foo=('bar',) 'multivalue-variant foo=*', foo=('bar',)
) )
update_packages('multivalue-variant', 'variants', 'foo=none')
with pytest.raises(NonDeterministicVariantError):
concretize('multivalue-variant foo=any')
def test_preferred_compilers(self): def test_preferred_compilers(self):
"""Test preferred compilers are applied correctly """Test preferred compilers are applied correctly
""" """

View File

@ -274,8 +274,8 @@ def test_satisfies_matching_variant(self):
check_satisfies('mpich foo=true', 'mpich+foo') check_satisfies('mpich foo=true', 'mpich+foo')
check_satisfies('mpich~foo', 'mpich foo=FALSE') check_satisfies('mpich~foo', 'mpich foo=FALSE')
check_satisfies('mpich foo=False', 'mpich~foo') check_satisfies('mpich foo=False', 'mpich~foo')
check_satisfies('mpich foo=any', 'mpich~foo') check_satisfies('mpich foo=*', 'mpich~foo')
check_satisfies('mpich +foo', 'mpich foo=any') check_satisfies('mpich +foo', 'mpich foo=*')
def test_satisfies_multi_value_variant(self): def test_satisfies_multi_value_variant(self):
# Check quoting # Check quoting
@ -288,9 +288,9 @@ def test_satisfies_multi_value_variant(self):
# A more constrained spec satisfies a less constrained one # A more constrained spec satisfies a less constrained one
check_satisfies('multivalue-variant foo="bar,baz"', check_satisfies('multivalue-variant foo="bar,baz"',
'multivalue-variant foo=any') 'multivalue-variant foo=*')
check_satisfies('multivalue-variant foo=any', check_satisfies('multivalue-variant foo=*',
'multivalue-variant foo="bar,baz"') 'multivalue-variant foo="bar,baz"')
check_satisfies('multivalue-variant foo="bar,baz"', check_satisfies('multivalue-variant foo="bar,baz"',
@ -317,7 +317,7 @@ def test_satisfies_single_valued_variant(self):
a.concretize() a.concretize()
assert a.satisfies('foobar=bar') assert a.satisfies('foobar=bar')
assert a.satisfies('foobar=any') assert a.satisfies('foobar=*')
# Assert that an autospec generated from a literal # Assert that an autospec generated from a literal
# gives the right result for a single valued variant # gives the right result for a single valued variant
@ -452,10 +452,6 @@ def test_unsatisfiable_variants(self):
check_unsatisfiable('mpich', 'mpich~foo', True) check_unsatisfiable('mpich', 'mpich~foo', True)
check_unsatisfiable('mpich', 'mpich foo=1', True) check_unsatisfiable('mpich', 'mpich foo=1', True)
# None and any do not satisfy each other
check_unsatisfiable('foo=none', 'foo=any')
check_unsatisfiable('foo=any', 'foo=none')
def test_unsatisfiable_variant_mismatch(self): def test_unsatisfiable_variant_mismatch(self):
# No matchi in specs # No matchi in specs
check_unsatisfiable('mpich~foo', 'mpich+foo') check_unsatisfiable('mpich~foo', 'mpich+foo')
@ -624,9 +620,9 @@ def test_constrain_multi_value_variant(self):
) )
check_constrain( check_constrain(
'libelf foo=bar,baz', 'libelf foo=bar,baz', 'libelf foo=any') 'libelf foo=bar,baz', 'libelf foo=bar,baz', 'libelf foo=*')
check_constrain( check_constrain(
'libelf foo=bar,baz', 'libelf foo=any', 'libelf foo=bar,baz') 'libelf foo=bar,baz', 'libelf foo=*', 'libelf foo=bar,baz')
def test_constrain_compiler_flags(self): def test_constrain_compiler_flags(self):
check_constrain( check_constrain(
@ -668,8 +664,6 @@ def test_invalid_constraint(self):
check_invalid_constraint('libelf+debug', 'libelf~debug') check_invalid_constraint('libelf+debug', 'libelf~debug')
check_invalid_constraint('libelf+debug~foo', 'libelf+debug+foo') check_invalid_constraint('libelf+debug~foo', 'libelf+debug+foo')
check_invalid_constraint('libelf debug=True', 'libelf debug=False') check_invalid_constraint('libelf debug=True', 'libelf debug=False')
check_invalid_constraint('libelf foo=none', 'libelf foo=any')
check_invalid_constraint('libelf foo=any', 'libelf foo=none')
check_invalid_constraint( check_invalid_constraint(
'libelf cppflags="-O3"', 'libelf cppflags="-O2"') 'libelf cppflags="-O3"', 'libelf cppflags="-O2"')
@ -684,7 +678,7 @@ def test_constrain_changed(self):
check_constrain_changed('libelf', '%gcc') check_constrain_changed('libelf', '%gcc')
check_constrain_changed('libelf%gcc', '%gcc@4.5') check_constrain_changed('libelf%gcc', '%gcc@4.5')
check_constrain_changed('libelf', '+debug') check_constrain_changed('libelf', '+debug')
check_constrain_changed('libelf', 'debug=any') check_constrain_changed('libelf', 'debug=*')
check_constrain_changed('libelf', '~debug') check_constrain_changed('libelf', '~debug')
check_constrain_changed('libelf', 'debug=2') check_constrain_changed('libelf', 'debug=2')
check_constrain_changed('libelf', 'cppflags="-O3"') check_constrain_changed('libelf', 'cppflags="-O3"')
@ -704,7 +698,7 @@ def test_constrain_not_changed(self):
check_constrain_not_changed('libelf+debug', '+debug') check_constrain_not_changed('libelf+debug', '+debug')
check_constrain_not_changed('libelf~debug', '~debug') check_constrain_not_changed('libelf~debug', '~debug')
check_constrain_not_changed('libelf debug=2', 'debug=2') check_constrain_not_changed('libelf debug=2', 'debug=2')
check_constrain_not_changed('libelf debug=2', 'debug=any') check_constrain_not_changed('libelf debug=2', 'debug=*')
check_constrain_not_changed( check_constrain_not_changed(
'libelf cppflags="-O3"', 'cppflags="-O3"') 'libelf cppflags="-O3"', 'cppflags="-O3"')
@ -918,14 +912,14 @@ def test_spec_flags_maintain_order(self):
for x in ('cflags', 'cxxflags', 'fflags') for x in ('cflags', 'cxxflags', 'fflags')
) )
def test_combination_of_any_or_none(self): def test_combination_of_wildcard_or_none(self):
# Test that using 'none' and another value raises # Test that using 'none' and another value raises
with pytest.raises(spack.variant.InvalidVariantValueCombinationError): with pytest.raises(spack.variant.InvalidVariantValueCombinationError):
Spec('multivalue-variant foo=none,bar') Spec('multivalue-variant foo=none,bar')
# Test that using 'any' and another value raises # Test that using wildcard and another value raises
with pytest.raises(spack.variant.InvalidVariantValueCombinationError): with pytest.raises(spack.variant.InvalidVariantValueCombinationError):
Spec('multivalue-variant foo=any,bar') Spec('multivalue-variant foo=*,bar')
@pytest.mark.skipif( @pytest.mark.skipif(
sys.version_info[0] == 2, reason='__wrapped__ requires python 3' sys.version_info[0] == 2, reason='__wrapped__ requires python 3'

View File

@ -25,7 +25,7 @@
except ImportError: except ImportError:
from collections import Sequence from collections import Sequence
special_variant_values = [None, 'none', 'any'] special_variant_values = [None, 'none', '*']
class Variant(object): class Variant(object):
@ -60,8 +60,8 @@ def __init__(
self.description = str(description) self.description = str(description)
self.values = None self.values = None
if values is any: if values == '*':
# 'any' is a special case to make it easy to say any value is ok # wildcard is a special case to make it easy to say any value is ok
self.single_value_validator = lambda x: True self.single_value_validator = lambda x: True
elif isinstance(values, type): elif isinstance(values, type):
@ -122,13 +122,13 @@ def validate_or_raise(self, vspec, pkg=None):
# Check and record the values that are not allowed # Check and record the values that are not allowed
not_allowed_values = [ not_allowed_values = [
x for x in value x for x in value
if x != 'any' and self.single_value_validator(x) is False if x != '*' and self.single_value_validator(x) is False
] ]
if not_allowed_values: if not_allowed_values:
raise InvalidVariantValueError(self, not_allowed_values, pkg) raise InvalidVariantValueError(self, not_allowed_values, pkg)
# Validate the group of values if needed # Validate the group of values if needed
if self.group_validator is not None and value != ('any',): if self.group_validator is not None and value != ('*',):
self.group_validator(pkg.name, self.name, value) self.group_validator(pkg.name, self.name, value)
@property @property
@ -270,8 +270,6 @@ def _value_setter(self, value):
# Tuple is necessary here instead of list because the # Tuple is necessary here instead of list because the
# values need to be hashed # values need to be hashed
value = re.split(r'\s*,\s*', str(value)) value = re.split(r'\s*,\s*', str(value))
value = list(map(lambda x: 'any' if str(x).upper() == 'ANY' else x,
value))
for val in special_variant_values: for val in special_variant_values:
if val in value and len(value) > 1: if val in value and len(value) > 1:
@ -313,15 +311,7 @@ def satisfies(self, other):
""" """
# If names are different then `self` does not satisfy `other` # If names are different then `self` does not satisfy `other`
# (`foo=bar` will never satisfy `baz=bar`) # (`foo=bar` will never satisfy `baz=bar`)
if other.name != self.name: return other.name == self.name
return False
# If the variant is already set to none, it can't satisfy any
if ('none' in self or None in self) and 'any' in other:
return False
# If the variant is set to any, it can't be constrained by none
if 'any' in self and ('none' in other or None in other):
return False
return True
@implicit_variant_conversion @implicit_variant_conversion
def compatible(self, other): def compatible(self, other):
@ -338,15 +328,7 @@ def compatible(self, other):
""" """
# If names are different then `self` is not compatible with `other` # If names are different then `self` is not compatible with `other`
# (`foo=bar` is incompatible with `baz=bar`) # (`foo=bar` is incompatible with `baz=bar`)
if other.name != self.name: return other.name == self.name
return False
# If the variant is already set to none, incompatible with any
if ('none' in self or None in self) and 'any' in other:
return False
# If the variant is set to any, it can't be compatible with none
if 'any' in self and ('none' in other or None in other):
return False
return True
@implicit_variant_conversion @implicit_variant_conversion
def constrain(self, other): def constrain(self, other):
@ -366,9 +348,9 @@ def constrain(self, other):
old_value = self.value old_value = self.value
values = list(sorted(set(self.value + other.value))) values = list(sorted(set(self.value + other.value)))
# If we constraint any by another value, just take value # If we constraint wildcard by another value, just take value
if 'any' in values and len(values) > 1: if '*' in values and len(values) > 1:
values.remove('any') values.remove('*')
self.value = ','.join(values) self.value = ','.join(values)
return old_value != self.value return old_value != self.value
@ -401,13 +383,11 @@ def satisfies(self, other):
Returns: Returns:
bool: True or False bool: True or False
""" """
# If it doesn't satisfy as an AbstractVariant, it doesn't satisfy as a
# MultiValuedVariant this handles conflicts between none and any
super_sat = super(MultiValuedVariant, self).satisfies(other) super_sat = super(MultiValuedVariant, self).satisfies(other)
# Otherwise we want all the values in `other` to be also in `self` # Otherwise we want all the values in `other` to be also in `self`
return super_sat and (all(v in self.value for v in other.value) or return super_sat and (all(v in self.value for v in other.value) or
'any' in other or 'any' in self) '*' in other or '*' in self)
class SingleValuedVariant(AbstractVariant): class SingleValuedVariant(AbstractVariant):
@ -427,12 +407,10 @@ def __str__(self):
@implicit_variant_conversion @implicit_variant_conversion
def satisfies(self, other): def satisfies(self, other):
# If it doesn't satisfy as an AbstractVariant, it doesn't satisfy as a
# SingleValuedVariant this handles conflicts between none and any
abstract_sat = super(SingleValuedVariant, self).satisfies(other) abstract_sat = super(SingleValuedVariant, self).satisfies(other)
return abstract_sat and (self.value == other.value or return abstract_sat and (self.value == other.value or
other.value == 'any' or self.value == 'any') other.value == '*' or self.value == '*')
def compatible(self, other): def compatible(self, other):
return self.satisfies(other) return self.satisfies(other)
@ -442,13 +420,13 @@ def constrain(self, other):
if self.name != other.name: if self.name != other.name:
raise ValueError('variants must have the same name') raise ValueError('variants must have the same name')
if self.value == 'any': if other.value == '*':
self.value = other.value
return self.value != other.value
if other.value == 'any' and self.value not in ('none', None):
return False return False
if self.value == '*':
self.value = other.value
return True
if self.value != other.value: if self.value != other.value:
raise UnsatisfiableVariantSpecError(other.value, self.value) raise UnsatisfiableVariantSpecError(other.value, self.value)
return False return False
@ -463,8 +441,8 @@ def yaml_entry(self):
class BoolValuedVariant(SingleValuedVariant): class BoolValuedVariant(SingleValuedVariant):
"""A variant that can hold either True or False. """A variant that can hold either True or False.
BoolValuedVariant can also hold the value 'any', for coerced BoolValuedVariant can also hold the value '*', for coerced
comparisons between ``foo=any`` and ``+foo`` or ``~foo``.""" comparisons between ``foo=*`` and ``+foo`` or ``~foo``."""
def _value_setter(self, value): def _value_setter(self, value):
# Check the string representation of the value and turn # Check the string representation of the value and turn
@ -475,9 +453,9 @@ def _value_setter(self, value):
elif str(value).upper() == 'FALSE': elif str(value).upper() == 'FALSE':
self._original_value = value self._original_value = value
self._value = False self._value = False
elif str(value).upper() == 'ANY': elif str(value) == '*':
self._original_value = value self._original_value = value
self._value = 'any' self._value = '*'
else: else:
msg = 'cannot construct a BoolValuedVariant for "{0}" from ' msg = 'cannot construct a BoolValuedVariant for "{0}" from '
msg += 'a value that does not represent a bool' msg += 'a value that does not represent a bool'
@ -874,7 +852,7 @@ def __init__(self, variant, pkg):
class InvalidVariantValueCombinationError(error.SpecError): class InvalidVariantValueCombinationError(error.SpecError):
"""Raised when a variant has values 'any' or 'none' with other values.""" """Raised when a variant has values '*' or 'none' with other values."""
class InvalidVariantValueError(error.SpecError): class InvalidVariantValueError(error.SpecError):