concretization: don't apply build-dep constraints for installed packages (#11594)

Spack currently tries to unify everything in the DAG, but this is too strict for build dependencies, where it is fine to build a dependency with a tool that conflicts with a version fo that tool for a dependent's build.

To enable a workaround for conflicts among build dependencies, so that users can install in multiple steps to avoid these conflicts, make the following changes:

* Dont apply package dependency constraints for build deps of installed packages
* Avoid applying constraints for installed packages vs. concrete packages
* Mark all dependencies of installed packages as visited in normalization method
* don't remove dependency links for concrete specs in flat_dependencies

Also add tests:
* Update test to ensure that link dependencies of installed packages have constraints applied
* Add test to check for proper handling of transitive dependencies (which is currently not the case)

3 files changed, 85 insertions, 5 deletions

diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index c08d074415..cc1613549f 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -2056,7 +2056,11 @@ class Spec(object):
            without modifying the spec it's called on.
 
            If copy is False, clears this spec's dependencies and
-           returns them.
+           returns them. This disconnects all dependency links including
+           transitive dependencies, except for concrete specs: if a spec
+           is concrete it will not be disconnected from its dependencies
+           (although a non-concrete spec with concrete dependencies will
+           be disconnected from those dependencies).
         """
         copy = kwargs.get('copy', True)
 
@@ -2074,8 +2078,9 @@ class Spec(object):
 
             if not copy:
                 for spec in flat_deps.values():
-                    spec._dependencies.clear()
-                    spec._dependents.clear()
+                    if not spec.concrete:
+                        spec._dependencies.clear()
+                        spec._dependents.clear()
                 self._dependencies.clear()
 
             return flat_deps
@@ -2274,11 +2279,17 @@ class Spec(object):
             return False
         visited.add(self.name)
 
-        # if we descend into a virtual spec, there's nothing more
+        # If we descend into a virtual spec, there's nothing more
         # to normalize.  Concretize will finish resolving it later.
         if self.virtual or self.external:
             return False
 
+        # Avoid recursively adding constraints for already-installed packages:
+        # these may include build dependencies which are not needed for this
+        # install (since this package is already installed).
+        if self.concrete and self.package.installed:
+            return False
+
         # Combine constraints from package deps with constraints from
         # the spec, until nothing changes.
         any_change = False
@@ -3730,6 +3741,7 @@ class SpecParser(spack.parse.Parser):
                             # We're finding a dependency by hash for an
                             # anonymous spec
                             dep = self.spec_by_hash()
+                            dep = dep.copy(deps=('link', 'run'))
                         else:
                             # We're adding a dependency to the last spec
                             self.expect(ID)
diff --git a/lib/spack/spack/test/directory_layout.py b/lib/spack/spack/test/directory_layout.py
index 0b6b31bef0..10e77143a7 100644
--- a/lib/spack/spack/test/directory_layout.py
+++ b/lib/spack/spack/test/directory_layout.py
@@ -145,7 +145,7 @@ def test_read_and_write_spec(
             read_separately = Spec.from_yaml(spec_file.read())
 
         # TODO: revise this when build deps are in dag_hash
-        norm = read_separately.normalized().copy(deps=stored_deptypes)
+        norm = read_separately.copy(deps=stored_deptypes)
         assert norm == spec_from_file
         assert norm.eq_dag(spec_from_file)
 
diff --git a/lib/spack/spack/test/spec_dag.py b/lib/spack/spack/test/spec_dag.py
index 5f26631148..773faeb1fd 100644
--- a/lib/spack/spack/test/spec_dag.py
+++ b/lib/spack/spack/test/spec_dag.py
@@ -84,6 +84,74 @@ w->y deptypes are (link, build), w->x and y->z deptypes are (test)
 
 
 @pytest.mark.usefixtures('config')
+def test_installed_deps():
+    """Preinstall a package P with a constrained build dependency D, then
+    concretize a dependent package which also depends on P and D, specifying
+    that the installed instance of P should be used. In this case, D should
+    not be constrained by P since P is already built.
+    """
+    default = ('build', 'link')
+    build_only = ('build',)
+
+    e = MockPackage('e', [], [])
+    d = MockPackage('d', [], [])
+    c_conditions = {
+        d.name: {
+            'c': 'd@2'
+        },
+        e.name: {
+            'c': 'e@2'
+        }
+    }
+    c = MockPackage('c', [d, e], [build_only, default],
+                    conditions=c_conditions)
+    b = MockPackage('b', [d, e], [default, default])
+    a = MockPackage('a', [b, c], [default, default])
+    mock_repo = MockPackageMultiRepo([a, b, c, d, e])
+
+    with spack.repo.swap(mock_repo):
+        c_spec = Spec('c')
+        c_spec.concretize()
+        assert c_spec['d'].version == spack.version.Version('2')
+
+        c_installed = spack.spec.Spec.from_dict(c_spec.to_dict(all_deps=False))
+        for spec in c_installed.traverse():
+            setattr(spec.package, 'installed', True)
+
+        a_spec = Spec('a')
+        a_spec._add_dependency(c_installed, default)
+        a_spec.concretize()
+
+        assert a_spec['d'].version == spack.version.Version('3')
+        assert a_spec['e'].version == spack.version.Version('2')
+
+
+@pytest.mark.usefixtures('config')
+def test_specify_preinstalled_dep():
+    """Specify the use of a preinstalled package during concretization with a
+    transitive dependency that is only supplied by the preinstalled package.
+    """
+    default = ('build', 'link')
+
+    c = MockPackage('c', [], [])
+    b = MockPackage('b', [c], [default])
+    a = MockPackage('a', [b], [default])
+    mock_repo = MockPackageMultiRepo([a, b, c])
+
+    with spack.repo.swap(mock_repo):
+        b_spec = Spec('b')
+        b_spec.concretize()
+        for spec in b_spec.traverse():
+            setattr(spec.package, 'installed', True)
+
+        a_spec = Spec('a')
+        a_spec._add_dependency(b_spec, default)
+        a_spec.concretize()
+
+        assert set(x.name for x in a_spec.traverse()) == set(['a', 'b', 'c'])
+
+
+@pytest.mark.usefixtures('config')
 def test_conditional_dep_with_user_constraints():
     """This sets up packages X->Y such that X depends on Y conditionally. It
     then constructs a Spec with X but with no constraints on X, so that the