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.

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')