clean up concreteness detection

- Fixes bugs where concretization would fail due to an erroneously cached
  _concrete attribute.

- Ripped out a bunch of code in spec.py that isn't needed/valid anymore:
  - The various concrete() methods on different types of Specs would
    attempt to statically compute whether the Spec was concrete.
  - This dates back to when DAGs were simpler and there were no optional
    dependencies.  It's actually NOT possible to compute statically
    whether a Spec is concrete now.  The ONLY way you know is if it goes
    through concretization and is marked concrete once that completes.
  - This commit removes all simple concreteness checks and relies only on
    the _concrete attribute.  This should make thinking about
    concreteness simpler.

- Fixed a couple places where Specs need to be marked concrete explicitly.
  - Specs read from files and Specs that are destructively copied from
    concrete Specs now need to be marked concrete explicitly.
  - These spots may previously have "worked", but they were brittle and
    should be explcitly marked anyway.
This commit is contained in:
Todd Gamblin 2017-09-11 11:43:41 -07:00
parent 8c42aed9d5
commit f8f1c308c9
3 changed files with 27 additions and 99 deletions

View File

@ -447,7 +447,12 @@ def get_specs():
except fs.FetchError: except fs.FetchError:
continue continue
with open(stage.save_filename, 'r') as f: with open(stage.save_filename, 'r') as f:
# read the spec from the build cache file. All specs
# in build caches are concrete (as they aer built) so
# we need to mark this spec concrete on read-in.
spec = spack.spec.Spec.from_yaml(f) spec = spack.spec.Spec.from_yaml(f)
spec._mark_concrete()
specs.add(spec) specs.add(spec)
durls[spec].append(link) durls[spec].append(link)
return specs, durls return specs, durls

View File

@ -635,10 +635,6 @@ def constrain(self, other):
def valid_compiler_flags(): def valid_compiler_flags():
return _valid_compiler_flags return _valid_compiler_flags
@property
def concrete(self):
return all(flag in self for flag in _valid_compiler_flags)
def copy(self): def copy(self):
clone = FlagMap(None) clone = FlagMap(None)
for name, value in self.items(): for name, value in self.items():
@ -661,73 +657,6 @@ class DependencyMap(HashableMap):
"""Each spec has a DependencyMap containing specs for its dependencies. """Each spec has a DependencyMap containing specs for its dependencies.
The DependencyMap is keyed by name. """ The DependencyMap is keyed by name. """
def __init__(self, owner):
super(DependencyMap, self).__init__()
# Owner Spec for the current map
self.owner = owner
@property
def concrete(self):
# Check if the dependencies are concrete and of the correct type
dependencies_are_concrete = all(
(d.spec.concrete and d.deptypes) for d in self.values()
)
if not dependencies_are_concrete:
return False
# If we are dealing with an external spec, it clearly has no
# dependencies managed by spack
if self.owner.external:
return True
# Now check we have every dependency we need
pkg = self.owner.package
for dependency in pkg.dependencies.values():
# Check that for every condition we satisfy we satisfy also
# the associated constraint
for condition, constraint in dependency.items():
# WARNING: This part relies on the fact that dependencies are
# the last item to be checked when computing if a spec is
# concrete. This means that we are ensured that all variants,
# versions, compilers, etc. are there with their final value
# when we call 'Spec.satisfies(..., strict=True)'
# FIXME: at the moment check only for non conditional
# FIXME: dependencies, to avoid infinite recursion
# satisfy_condition = self.owner.satisfies(
# condition, strict=True
# )
satisfy_condition = str(condition) == self.owner.name
if not satisfy_condition:
continue
dependency_name = constraint.name
# We don't want to check build dependencies
if pkg.dependency_types[dependency_name] == set(['build']):
continue
try:
dependency = self.owner[dependency_name]
satisfy_constraint = dependency.satisfies(
constraint, strict=True
)
except KeyError:
# If the dependency is not there we don't
# satisfy the constraint
satisfy_constraint = False
if satisfy_condition and not satisfy_constraint:
return False
return True
def __str__(self): def __str__(self):
return "{deps: %s}" % ', '.join(str(d) for d in sorted(self.values())) return "{deps: %s}" % ', '.join(str(d) for d in sorted(self.values()))
@ -1315,26 +1244,16 @@ def is_virtual(name):
@property @property
def concrete(self): def concrete(self):
"""A spec is concrete if it can describe only ONE build of a package. """A spec is concrete if it describes a single build of a package.
If any of the name, version, architecture, compiler,
variants, or depdenencies are ambiguous,then it is not concrete.
"""
# If we have cached that the spec is concrete, then it's concrete
if self._concrete:
return True
# Otherwise check if the various parts of the spec are concrete. More formally, a spec is concrete if concretize() has been called
# It's crucial to have the check on dependencies last, as it relies on it and it has been marked `_concrete`.
# on all the other parts to be already checked for concreteness.
self._concrete = bool(not self.virtual and Concrete specs either can be or have been built. All constraints
self.namespace is not None and have been resolved, optional dependencies have been added or
self.versions.concrete and removed, a compiler has been chosen, and all variants have
self.variants.concrete and values.
self.architecture and """
self.architecture.concrete and
self.compiler and self.compiler.concrete and
self.compiler_flags.concrete and
self._dependencies.concrete)
return self._concrete return self._concrete
def traverse(self, **kwargs): def traverse(self, **kwargs):
@ -1825,8 +1744,8 @@ def _expand_virtual_packages(self):
if replacement.external: if replacement.external:
if (spec._dependencies): if (spec._dependencies):
changed = True changed = True
spec._dependencies = DependencyMap(spec) spec._dependencies = DependencyMap()
replacement._dependencies = DependencyMap(replacement) replacement._dependencies = DependencyMap()
replacement.architecture = self.architecture replacement.architecture = self.architecture
# TODO: could this and the stuff in _dup be cleaned up? # TODO: could this and the stuff in _dup be cleaned up?
@ -1986,7 +1905,7 @@ def flat_dependencies(self, **kwargs):
def index(self, deptype=None): def index(self, deptype=None):
"""Return DependencyMap that points to all the dependencies in this """Return DependencyMap that points to all the dependencies in this
spec.""" spec."""
dm = DependencyMap(None) dm = DependencyMap()
for spec in self.traverse(deptype=deptype): for spec in self.traverse(deptype=deptype):
dm[spec.name] = spec dm[spec.name] = spec
return dm return dm
@ -2588,8 +2507,8 @@ def _dup(self, other, deps=True, cleardeps=True, caches=None):
else None else None
self.compiler = other.compiler.copy() if other.compiler else None self.compiler = other.compiler.copy() if other.compiler else None
if cleardeps: if cleardeps:
self._dependents = DependencyMap(self) self._dependents = DependencyMap()
self._dependencies = DependencyMap(self) self._dependencies = DependencyMap()
self.compiler_flags = other.compiler_flags.copy() self.compiler_flags = other.compiler_flags.copy()
self.compiler_flags.spec = self self.compiler_flags.spec = self
self.variants = other.variants.copy() self.variants = other.variants.copy()
@ -3296,8 +3215,8 @@ def spec(self, name):
spec.external_path = None spec.external_path = None
spec.external_module = None spec.external_module = None
spec.compiler_flags = FlagMap(spec) spec.compiler_flags = FlagMap(spec)
spec._dependents = DependencyMap(spec) spec._dependents = DependencyMap()
spec._dependencies = DependencyMap(spec) spec._dependencies = DependencyMap()
spec.namespace = spec_namespace spec.namespace = spec_namespace
spec._hash = None spec._hash = None
spec._cmp_key_cache = None spec._cmp_key_cache = None

View File

@ -55,8 +55,10 @@ def test_yaml_directory_layout_parameters(
# Ensure default layout matches expected spec format # Ensure default layout matches expected spec format
layout_default = YamlDirectoryLayout(str(tmpdir)) layout_default = YamlDirectoryLayout(str(tmpdir))
path_default = layout_default.relative_path_for_spec(spec) path_default = layout_default.relative_path_for_spec(spec)
assert(path_default == assert(path_default == spec.format(
spec.format("${ARCHITECTURE}/${COMPILERNAME}-${COMPILERVER}/${PACKAGE}-${VERSION}-${HASH}")) # NOQA: ignore=E501 "${ARCHITECTURE}/"
"${COMPILERNAME}-${COMPILERVER}/"
"${PACKAGE}-${VERSION}-${HASH}"))
# Test hash_length parameter works correctly # Test hash_length parameter works correctly
layout_10 = YamlDirectoryLayout(str(tmpdir), hash_len=10) layout_10 = YamlDirectoryLayout(str(tmpdir), hash_len=10)
@ -133,6 +135,8 @@ def test_read_and_write_spec(
# TODO: increase reuse of build dependencies. # TODO: increase reuse of build dependencies.
stored_deptypes = ('link', 'run') stored_deptypes = ('link', 'run')
expected = spec.copy(deps=stored_deptypes) expected = spec.copy(deps=stored_deptypes)
expected._mark_concrete()
assert expected.concrete assert expected.concrete
assert expected == spec_from_file assert expected == spec_from_file
assert expected.eq_dag(spec_from_file) assert expected.eq_dag(spec_from_file)