summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorTodd Gamblin <tgamblin@llnl.gov>2017-03-25 22:49:46 -0700
committerTodd Gamblin <tgamblin@llnl.gov>2017-03-31 13:40:41 -0700
commit7f3f4930249b580bd27891299d330f8dd272ecd3 (patch)
treec29cd13b287128f82bbc54589035e746fc590d53
parentfe6f39b66287a4b3ecade2d776348d44920ec651 (diff)
downloadspack-7f3f4930249b580bd27891299d330f8dd272ecd3.tar.gz
spack-7f3f4930249b580bd27891299d330f8dd272ecd3.tar.bz2
spack-7f3f4930249b580bd27891299d330f8dd272ecd3.tar.xz
spack-7f3f4930249b580bd27891299d330f8dd272ecd3.zip
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.
-rw-r--r--lib/spack/spack/spec.py52
-rw-r--r--lib/spack/spack/test/spec_dag.py12
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')