From f31711b84e4ce9c34b845c2061f23829154b7069 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Sat, 8 Jun 2019 11:43:26 -0700 Subject: 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) --- lib/spack/spack/spec.py | 20 ++++++++-- lib/spack/spack/test/directory_layout.py | 2 +- lib/spack/spack/test/spec_dag.py | 68 ++++++++++++++++++++++++++++++++ 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 @@ -83,6 +83,74 @@ w->y deptypes are (link, build), w->x and y->z deptypes are (test) assert ('z' not in spec) +@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 -- cgit v1.2.3-60-g2f50