From 22c815f3d426ef8c909bedc20c2bd27f9fae527e Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Mon, 5 Aug 2024 18:24:57 +0200 Subject: [PATCH] Do not halt concretization on unknown variants in externals (#45326) * Do not halt concretization on unkwnown variants in externals --- lib/spack/spack/solver/asp.py | 57 +++++++++++++++++------------- lib/spack/spack/test/concretize.py | 49 +++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 24 deletions(-) diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py index f26a83f9e2..950ab8ce04 100644 --- a/lib/spack/spack/solver/asp.py +++ b/lib/spack/spack/solver/asp.py @@ -1433,16 +1433,14 @@ def condition( # caller, we won't emit partial facts. condition_id = next(self._id_counter) - self.gen.fact(fn.pkg_fact(required_spec.name, fn.condition(condition_id))) - self.gen.fact(fn.condition_reason(condition_id, msg)) - trigger_id = self._get_condition_id( required_spec, cache=self._trigger_cache, body=True, transform=transform_required ) + self.gen.fact(fn.pkg_fact(required_spec.name, fn.condition(condition_id))) + self.gen.fact(fn.condition_reason(condition_id, msg)) self.gen.fact( fn.pkg_fact(required_spec.name, fn.condition_trigger(condition_id, trigger_id)) ) - if not imposed_spec: return condition_id @@ -1691,19 +1689,43 @@ def external_packages(self): spack.spec.parse_with_version_concrete(x["spec"]) for x in externals ] - external_specs = [] + selected_externals = set() if spec_filters: for current_filter in spec_filters: current_filter.factory = lambda: candidate_specs - external_specs.extend(current_filter.selected_specs()) - else: - external_specs.extend(candidate_specs) + selected_externals.update(current_filter.selected_specs()) + + # Emit facts for externals specs. Note that "local_idx" is the index of the spec + # in packages::externals. This means: + # + # packages::externals[local_idx].spec == spec + external_versions = [] + for local_idx, spec in enumerate(candidate_specs): + msg = f"{spec.name} available as external when satisfying {spec}" + + if spec_filters and spec not in selected_externals: + continue + + if not spec.versions.concrete: + warnings.warn(f"cannot use the external spec {spec}: needs a concrete version") + continue + + def external_imposition(input_spec, requirements): + return requirements + [ + fn.attr("external_conditions_hold", input_spec.name, local_idx) + ] + + try: + self.condition(spec, spec, msg=msg, transform_imposed=external_imposition) + except (spack.error.SpecError, RuntimeError) as e: + warnings.warn(f"while setting up external spec {spec}: {e}") + continue + external_versions.append((spec.version, local_idx)) + self.possible_versions[spec.name].add(spec.version) + self.gen.newline() # Order the external versions to prefer more recent versions # even if specs in packages.yaml are not ordered that way - external_versions = [ - (x.version, external_id) for external_id, x in enumerate(external_specs) - ] external_versions = [ (v, idx, external_id) for idx, (v, external_id) in enumerate(sorted(external_versions, reverse=True)) @@ -1713,19 +1735,6 @@ def external_packages(self): DeclaredVersion(version=version, idx=idx, origin=Provenance.EXTERNAL) ) - # Declare external conditions with a local index into packages.yaml - for local_idx, spec in enumerate(external_specs): - msg = "%s available as external when satisfying %s" % (spec.name, spec) - - def external_imposition(input_spec, requirements): - return requirements + [ - fn.attr("external_conditions_hold", input_spec.name, local_idx) - ] - - self.condition(spec, spec, msg=msg, transform_imposed=external_imposition) - self.possible_versions[spec.name].add(spec.version) - self.gen.newline() - self.trigger_rules() self.effect_rules() diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py index 1114feabe2..697c7bbf14 100644 --- a/lib/spack/spack/test/concretize.py +++ b/lib/spack/spack/test/concretize.py @@ -2574,6 +2574,55 @@ def test_can_reuse_concrete_externals_for_dependents(self, mutable_config, tmp_p sombrero = result.specs[0] assert sombrero["externaltool"].dag_hash() == external_spec.dag_hash() + @pytest.mark.regression("45321") + @pytest.mark.parametrize( + "corrupted_str", + [ + "cmake@3.4.3 foo=bar", # cmake has no variant "foo" + "mvdefaults@1.0 foo=a,d", # variant "foo" has no value "d" + "cmake %gcc", # spec has no version + ], + ) + def test_corrupted_external_does_not_halt_concretization(self, corrupted_str, mutable_config): + """Tests that having a wrong variant in an external spec doesn't stop concretization""" + corrupted_spec = Spec(corrupted_str) + packages_yaml = { + f"{corrupted_spec.name}": { + "externals": [{"spec": corrupted_str, "prefix": "/dev/null"}] + } + } + mutable_config.set("packages", packages_yaml) + # Assert we don't raise due to the corrupted external entry above + s = Spec("pkg-a").concretized() + assert s.concrete + + @pytest.mark.regression("44828") + @pytest.mark.not_on_windows("Tests use linux paths") + def test_correct_external_is_selected_from_packages_yaml(self, mutable_config): + """Tests that when filtering external specs, the correct external is selected to + reconstruct the prefix, and other external attributes. + """ + packages_yaml = { + "cmake": { + "externals": [ + {"spec": "cmake@3.23.1 %gcc", "prefix": "/tmp/prefix1"}, + {"spec": "cmake@3.23.1 %clang", "prefix": "/tmp/prefix2"}, + ] + } + } + concretizer_yaml = { + "reuse": {"roots": True, "from": [{"type": "external", "exclude": ["%gcc"]}]} + } + mutable_config.set("packages", packages_yaml) + mutable_config.set("concretizer", concretizer_yaml) + + s = Spec("cmake").concretized() + + # Check that we got the properties from the right external + assert s.external + assert s.satisfies("%clang") + assert s.prefix == "/tmp/prefix2" + @pytest.fixture() def duplicates_test_repository():