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

@@ -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())
+                    selected_externals.update(current_filter.selected_specs())
-            else:
+
-                external_specs.extend(candidate_specs)
+            # Emit facts for externals specs. Note that "local_idx" is the index of the spec
+            # in packages:<pkg_name>:externals. This means:
+            #
+            # packages:<pkg_name>: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()
 

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():