From 7f3f4930249b580bd27891299d330f8dd272ecd3 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Sat, 25 Mar 2017 22:49:46 -0700 Subject: Fix concretization bugs with virtuals and deptypes. 1. Fix #2807: Can't depend on virtual and non-virtual package - This is tested by test_my_dep_depends_on_provider_of_my_virtual_dep in the concretize.py test. - This was actually working in the test suite, but it depended on the order the dependencies were resolved in. Resolving non-virtual then virtual worked, but virtual, then non-virtual did not. - Problem was that an unnecessary copy was made of a spec that already had some dependencies set up, and the copy lost half of some of the dependency relationships. This caused the "can'd depend on X twice error". - Fix by eliminating unnecessary copy and ensuring that dep parameter of _merge_dependency is always safe to own -- i.e. it's a defensive copy from somewhere else. 2. Fix bug and simplify concretization of deptypes. - deptypes weren't being accumulated; they were being set on each DependencySpec. This could cause concretization to get into an infinite loop. - Fixed by accumulating deptypes in DependencySpec.update_deptypes() - Also simplified deptype normalization logic: deptypes are now merged in constrain() like everything else -- there is no need to merge them specially or to look at dpeendents in _merge_dependency(). - Add some docstrings to deptype tests. --- lib/spack/spack/spec.py | 52 +++++++++++++++++++++++----------------- lib/spack/spack/test/spec_dag.py | 12 +++++++--- 2 files changed, 39 insertions(+), 25 deletions(-) diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index b7a819cc46..fa88698ea9 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -581,8 +581,11 @@ class DependencySpec(object): self.deptypes = tuple(sorted(set(deptypes))) def update_deptypes(self, deptypes): - deptypes = tuple(sorted(set(deptypes))) + deptypes = set(deptypes) + deptypes.update(self.deptypes) + deptypes = tuple(sorted(deptypes)) changed = self.deptypes != deptypes + self.deptypes = deptypes return changed @@ -1801,6 +1804,8 @@ class Spec(object): dependency already in this spec. """ assert(vdep.virtual) + + # note that this defensively copies. providers = provider_index.providers_for(vdep) # If there is a provider for the vpkg, then use that instead of @@ -1830,6 +1835,10 @@ class Spec(object): provider_index): """Merge the dependency into this spec. + Caller should assume that this routine can owns the dep parameter + (i.e. it needs to be a copy of any internal structures like + dependencies on Package class objects). + This is the core of normalize(). There are some basic steps: * If dep is virtual, evaluate whether it corresponds to an @@ -1842,6 +1851,7 @@ class Spec(object): constraints into this spec. This method returns True if the spec was changed, False otherwise. + """ changed = False @@ -1854,7 +1864,8 @@ class Spec(object): dep = provider else: index = ProviderIndex([dep], restrict=True) - for vspec in (v for v in spec_deps.values() if v.virtual): + items = list(spec_deps.items()) + for name, vspec in items: if index.providers_for(vspec): vspec._replace_with(dep) del spec_deps[vspec.name] @@ -1865,29 +1876,23 @@ class Spec(object): raise UnsatisfiableProviderSpecError(required[0], dep) provider_index.update(dep) - # If the spec isn't already in the set of dependencies, clone - # it from the package description. + # If the spec isn't already in the set of dependencies, add it. + # Note: dep is always owned by this method. If it's from the + # caller, it's a copy from _evaluate_dependency_conditions. If it + # comes from a vdep, it's a defensive copy from _find_provider. if dep.name not in spec_deps: - spec_deps[dep.name] = dep.copy() + spec_deps[dep.name] = dep changed = True else: - dspec = spec_deps[dep.name] - if self.name not in dspec._dependents: - self._add_dependency(dspec, deptypes) - else: - dependent = dspec._dependents[self.name] - changed = dependent.update_deptypes(deptypes) - - # Constrain package information with spec info - try: - changed |= spec_deps[dep.name].constrain(dep) - - except UnsatisfiableSpecError as e: - e.message = "Invalid spec: '%s'. " - e.message += "Package %s requires %s %s, but spec asked for %s" - e.message %= (spec_deps[dep.name], dep.name, - e.constraint_type, e.required, e.provided) - raise e + # merge package/vdep information into spec + try: + changed |= spec_deps[dep.name].constrain(dep) + except UnsatisfiableSpecError as e: + e.message = "Invalid spec: '%s'. " + e.message += "Package %s requires %s %s, but spec asked for %s" + e.message %= (spec_deps[dep.name], dep.name, + e.constraint_type, e.required, e.provided) + raise e # Add merged spec to my deps and recurse dependency = spec_deps[dep.name] @@ -2097,6 +2102,9 @@ class Spec(object): changed = False for name in self.common_dependencies(other): changed |= self[name].constrain(other[name], deps=False) + if name in self._dependencies: + changed |= self._dependencies[name].update_deptypes( + other._dependencies[name].deptypes) # Update with additional constraints from other spec for name in other.dep_difference(self): diff --git a/lib/spack/spack/test/spec_dag.py b/lib/spack/spack/test/spec_dag.py index 31d6203f7f..af6a4efd95 100644 --- a/lib/spack/spack/test/spec_dag.py +++ b/lib/spack/spack/test/spec_dag.py @@ -63,8 +63,7 @@ def set_dependency(saved_deps): pkg = spack.repo.get(pkg_name) if pkg_name not in saved_deps: saved_deps[pkg_name] = (pkg, pkg.dependencies.copy()) - # Change dep spec - # XXX(deptype): handle deptypes. + pkg.dependencies[spec.name] = {Spec(pkg_name): spec} pkg.dependency_types[spec.name] = set(deptypes) return _mock @@ -609,6 +608,8 @@ class TestSpecDag(object): assert '^mpich2' in s2 def test_construct_spec_with_deptypes(self): + """Ensure that it is possible to construct a spec with explicit + dependency types.""" s = Spec('a', Spec('b', ['build'], Spec('c')), @@ -633,7 +634,12 @@ class TestSpecDag(object): assert s['f']._dependents['e'].deptypes == ('run',) def check_diamond_deptypes(self, spec): - """Validate deptypes in dt-diamond spec.""" + """Validate deptypes in dt-diamond spec. + + This ensures that concretization works properly when two packages + depend on the same dependency in different ways. + + """ assert spec['dt-diamond']._dependencies[ 'dt-diamond-left'].deptypes == ('build', 'link') -- cgit v1.2.3-70-g09d2