From 43114c2e06517c99dcab9e5c7dae16a0a8cb6a80 Mon Sep 17 00:00:00 2001 From: scheibelp Date: Wed, 30 May 2018 11:07:13 -0700 Subject: [PATCH] more-flexible user-specified dependency constraints (#8162) * allow user to constrain dependencies that are added conditionally * remove check for not-visited deps from normalize, move it to concretize. The check now runs after the concretization loop completes (so an error is only reported if the user-mentioned spec doesnt appear anywhere in the dag) * remove separate full_spec_deps variable; rename spec_deps to all_spec_deps to clarify that it merges user-specified dependencies with derived dependencies * add unit test to confirm new functionality --- lib/spack/spack/spec.py | 46 +++++++++++++++++++++++--------- lib/spack/spack/test/conftest.py | 6 ++++- lib/spack/spack/test/spec_dag.py | 45 ++++++++++++++++++++++++++++--- 3 files changed, 80 insertions(+), 17 deletions(-) diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index 8d9ecb320d..33ca196ad9 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -1833,13 +1833,26 @@ def concretize(self, tests=False): changed = True force = False + user_spec_deps = self.flat_dependencies(copy=False) + while changed: - changes = (self.normalize(force, tests), + changes = (self.normalize(force, tests=tests, + user_spec_deps=user_spec_deps), self._expand_virtual_packages(), self._concretize_helper()) changed = any(changes) force = True + visited_user_specs = set() + for dep in self.traverse(): + visited_user_specs.add(dep.name) + visited_user_specs.update(x.name for x in dep.package.provided) + + extra = set(user_spec_deps.keys()).difference(visited_user_specs) + if extra: + raise InvalidDependencyError( + self.name + " does not depend on " + comma_or(extra)) + for s in self.traverse(): # After concretizing, assign namespaces to anything left. # Note that this doesn't count as a "change". The repository @@ -2190,7 +2203,7 @@ def _normalize_helper(self, visited, spec_deps, provider_index, tests): return any_change - def normalize(self, force=False, tests=False): + def normalize(self, force=False, tests=False, user_spec_deps=None): """When specs are parsed, any dependencies specified are hanging off the root, and ONLY the ones that were explicitly provided are there. Normalization turns a partial flat spec into a DAG, where: @@ -2219,27 +2232,34 @@ def normalize(self, force=False, tests=False): # Ensure first that all packages & compilers in the DAG exist. self.validate_or_raise() - # Get all the dependencies into one DependencyMap - spec_deps = self.flat_dependencies(copy=False) + # Clear the DAG and collect all dependencies in the DAG, which will be + # reapplied as constraints. All dependencies collected this way will + # have been created by a previous execution of 'normalize'. + # A dependency extracted here will only be reintegrated if it is + # discovered to apply according to _normalize_helper, so + # user-specified dependencies are recorded separately in case they + # refer to specs which take several normalization passes to + # materialize. + all_spec_deps = self.flat_dependencies(copy=False) + + if user_spec_deps: + for name, spec in user_spec_deps.items(): + if name not in all_spec_deps: + all_spec_deps[name] = spec + else: + all_spec_deps[name].constrain(spec) # Initialize index of virtual dependency providers if # concretize didn't pass us one already provider_index = ProviderIndex( - [s for s in spec_deps.values()], restrict=True) + [s for s in all_spec_deps.values()], restrict=True) # traverse the package DAG and fill out dependencies according # to package files & their 'when' specs visited = set() any_change = self._normalize_helper( - visited, spec_deps, provider_index, tests) - - # If there are deps specified but not visited, they're not - # actually deps of this package. Raise an error. - extra = set(spec_deps.keys()).difference(visited) - if extra: - raise InvalidDependencyError( - self.name + " does not depend on " + comma_or(extra)) + visited, all_spec_deps, provider_index, tests) # Mark the spec as normal once done. self._normal = True diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py index a003df40c8..1072513c3c 100644 --- a/lib/spack/spack/test/conftest.py +++ b/lib/spack/spack/test/conftest.py @@ -611,7 +611,11 @@ def __init__(self, name, dependencies, dependency_types, conditions=None, if not conditions or dep.name not in conditions: self.dependencies[dep.name] = {Spec(name): d} else: - self.dependencies[dep.name] = {Spec(conditions[dep.name]): d} + dep_conditions = conditions[dep.name] + dep_conditions = dict( + (Spec(x), Dependency(self, Spec(y), type=dtype)) + for x, y in dep_conditions.items()) + self.dependencies[dep.name] = dep_conditions if versions: self.versions = versions diff --git a/lib/spack/spack/test/spec_dag.py b/lib/spack/spack/test/spec_dag.py index d6d4b34d55..a9abe278a3 100644 --- a/lib/spack/spack/test/spec_dag.py +++ b/lib/spack/spack/test/spec_dag.py @@ -102,6 +102,44 @@ def test_test_deptype(): assert ('z' not in spec) +@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 + initial normalization pass cannot determine whether the constraints are + met to add the dependency; this checks whether a user-specified constraint + on Y is applied properly. + """ + default = ('build', 'link') + + y = MockPackage('y', [], []) + x_on_y_conditions = { + y.name: { + 'x@2:': 'y' + } + } + x = MockPackage('x', [y], [default], conditions=x_on_y_conditions) + + mock_repo = MockPackageMultiRepo([x, y]) + with spack.repo.swap(mock_repo): + spec = Spec('x ^y@2') + spec.concretize() + + assert ('y@2' in spec) + + with spack.repo.swap(mock_repo): + spec = Spec('x@1') + spec.concretize() + + assert ('y' not in spec) + + with spack.repo.swap(mock_repo): + spec = Spec('x') + spec.concretize() + + assert ('y@3' in spec) + + @pytest.mark.usefixtures('mutable_mock_packages') class TestSpecDag(object): @@ -300,18 +338,19 @@ def test_unsatisfiable_architecture(self, set_dependency): with pytest.raises(spack.spec.UnsatisfiableArchitectureSpecError): spec.normalize() + @pytest.mark.usefixtures('config') def test_invalid_dep(self): spec = Spec('libelf ^mpich') with pytest.raises(spack.spec.InvalidDependencyError): - spec.normalize() + spec.concretize() spec = Spec('libelf ^libdwarf') with pytest.raises(spack.spec.InvalidDependencyError): - spec.normalize() + spec.concretize() spec = Spec('mpich ^dyninst ^libelf') with pytest.raises(spack.spec.InvalidDependencyError): - spec.normalize() + spec.concretize() def test_equal(self): # Different spec structures to test for equality