From f8f1c308c994512622c952d289388f9ab7f19709 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Mon, 11 Sep 2017 11:43:41 -0700 Subject: 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. --- lib/spack/spack/binary_distribution.py | 5 ++ lib/spack/spack/spec.py | 113 +++++-------------------------- lib/spack/spack/test/directory_layout.py | 8 ++- 3 files changed, 27 insertions(+), 99 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py index 9693a75c39..f915e3a0d0 100644 --- a/lib/spack/spack/binary_distribution.py +++ b/lib/spack/spack/binary_distribution.py @@ -447,7 +447,12 @@ def get_specs(): except fs.FetchError: continue 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._mark_concrete() + specs.add(spec) durls[spec].append(link) return specs, durls diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index f2836e8eb8..1b5f41d4c4 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -635,10 +635,6 @@ class FlagMap(HashableMap): def 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): clone = FlagMap(None) for name, value in self.items(): @@ -661,73 +657,6 @@ class DependencyMap(HashableMap): """Each spec has a DependencyMap containing specs for its dependencies. 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): return "{deps: %s}" % ', '.join(str(d) for d in sorted(self.values())) @@ -1315,26 +1244,16 @@ class Spec(object): @property def concrete(self): - """A spec is concrete if it can describe only ONE 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 + """A spec is concrete if it describes a single build of a package. - # Otherwise check if the various parts of the spec are concrete. - # It's crucial to have the check on dependencies last, as it relies - # on all the other parts to be already checked for concreteness. - self._concrete = bool(not self.virtual and - self.namespace is not None and - self.versions.concrete and - self.variants.concrete and - self.architecture and - self.architecture.concrete and - self.compiler and self.compiler.concrete and - self.compiler_flags.concrete and - self._dependencies.concrete) + More formally, a spec is concrete if concretize() has been called + on it and it has been marked `_concrete`. + + Concrete specs either can be or have been built. All constraints + have been resolved, optional dependencies have been added or + removed, a compiler has been chosen, and all variants have + values. + """ return self._concrete def traverse(self, **kwargs): @@ -1825,8 +1744,8 @@ class Spec(object): if replacement.external: if (spec._dependencies): changed = True - spec._dependencies = DependencyMap(spec) - replacement._dependencies = DependencyMap(replacement) + spec._dependencies = DependencyMap() + replacement._dependencies = DependencyMap() replacement.architecture = self.architecture # TODO: could this and the stuff in _dup be cleaned up? @@ -1986,7 +1905,7 @@ class Spec(object): def index(self, deptype=None): """Return DependencyMap that points to all the dependencies in this spec.""" - dm = DependencyMap(None) + dm = DependencyMap() for spec in self.traverse(deptype=deptype): dm[spec.name] = spec return dm @@ -2588,8 +2507,8 @@ class Spec(object): else None self.compiler = other.compiler.copy() if other.compiler else None if cleardeps: - self._dependents = DependencyMap(self) - self._dependencies = DependencyMap(self) + self._dependents = DependencyMap() + self._dependencies = DependencyMap() self.compiler_flags = other.compiler_flags.copy() self.compiler_flags.spec = self self.variants = other.variants.copy() @@ -3296,8 +3215,8 @@ class SpecParser(spack.parse.Parser): spec.external_path = None spec.external_module = None spec.compiler_flags = FlagMap(spec) - spec._dependents = DependencyMap(spec) - spec._dependencies = DependencyMap(spec) + spec._dependents = DependencyMap() + spec._dependencies = DependencyMap() spec.namespace = spec_namespace spec._hash = None spec._cmp_key_cache = None diff --git a/lib/spack/spack/test/directory_layout.py b/lib/spack/spack/test/directory_layout.py index a2609c8162..0cfd796e06 100644 --- a/lib/spack/spack/test/directory_layout.py +++ b/lib/spack/spack/test/directory_layout.py @@ -55,8 +55,10 @@ def test_yaml_directory_layout_parameters( # Ensure default layout matches expected spec format layout_default = YamlDirectoryLayout(str(tmpdir)) path_default = layout_default.relative_path_for_spec(spec) - assert(path_default == - spec.format("${ARCHITECTURE}/${COMPILERNAME}-${COMPILERVER}/${PACKAGE}-${VERSION}-${HASH}")) # NOQA: ignore=E501 + assert(path_default == spec.format( + "${ARCHITECTURE}/" + "${COMPILERNAME}-${COMPILERVER}/" + "${PACKAGE}-${VERSION}-${HASH}")) # Test hash_length parameter works correctly layout_10 = YamlDirectoryLayout(str(tmpdir), hash_len=10) @@ -133,6 +135,8 @@ def test_read_and_write_spec( # TODO: increase reuse of build dependencies. stored_deptypes = ('link', 'run') expected = spec.copy(deps=stored_deptypes) + expected._mark_concrete() + assert expected.concrete assert expected == spec_from_file assert expected.eq_dag(spec_from_file) -- cgit v1.2.3-60-g2f50