From d352b71df05976b019bcc6d5a309e96c26f0b916 Mon Sep 17 00:00:00 2001
From: Massimiliano Culpo <massimiliano.culpo@gmail.com>
Date: Wed, 12 Mar 2025 08:33:42 +0100
Subject: [PATCH] Error when an anonymous spec is required for a virtual
 package (#49385)

When requiring a constraint on a virtual package, it makes little
sense to use anonymous specs, and our documentation shows no example
of requirements on virtual packages starting with `^`.

Right now, due to how `^` is implemented in the solver, writing:
```yaml
mpi:
  require: "^openmpi"
```
is equivalent to the more correct form:
```yaml
mpi:
  require: "openmpi"
```
but the situation will change when `%` will shift its meaning to be a
direct dependency.

To avoid later errors that are both unclear, and quite slow to get to the user,
this commit makes anonymous specs under virtual requirements an error,
and shows a clear error message pointing to the file and line where the
spec needs to be changed.

Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
---
 lib/spack/spack/solver/requirements.py        | 29 ++++++++++---
 .../spack/test/concretization/requirements.py | 43 +++++++++++++++++++
 .../stacks/data-vis-sdk/spack.yaml            |  2 +-
 3 files changed, 67 insertions(+), 7 deletions(-)

diff --git a/lib/spack/spack/solver/requirements.py b/lib/spack/spack/solver/requirements.py
index 0fd67a7250a..6fa107e9fd0 100644
--- a/lib/spack/spack/solver/requirements.py
+++ b/lib/spack/spack/solver/requirements.py
@@ -165,7 +165,8 @@ def _rules_from_requirements(
 
                 # validate specs from YAML first, and fail with line numbers if parsing fails.
                 constraints = [
-                    parse_spec_from_yaml_string(constraint) for constraint in constraints
+                    parse_spec_from_yaml_string(constraint, named=kind == RequirementKind.VIRTUAL)
+                    for constraint in constraints
                 ]
                 when_str = requirement.get("when")
                 when = parse_spec_from_yaml_string(when_str) if when_str else spack.spec.Spec()
@@ -203,7 +204,7 @@ def reject_requirement_constraint(
                 s.validate_or_raise()
             except spack.error.SpackError as e:
                 tty.debug(
-                    f"[SETUP] Rejecting the default '{constraint}' requirement "
+                    f"[{__name__}] Rejecting the default '{constraint}' requirement "
                     f"on '{pkg_name}': {str(e)}",
                     level=2,
                 )
@@ -211,21 +212,37 @@ def reject_requirement_constraint(
         return False
 
 
-def parse_spec_from_yaml_string(string: str) -> spack.spec.Spec:
+def parse_spec_from_yaml_string(string: str, *, named: bool = False) -> spack.spec.Spec:
     """Parse a spec from YAML and add file/line info to errors, if it's available.
 
     Parse a ``Spec`` from the supplied string, but also intercept any syntax errors and
     add file/line information for debugging using file/line annotations from the string.
 
-    Arguments:
+    Args:
         string: a string representing a ``Spec`` from config YAML.
-
+        named: if True, the spec must have a name
     """
     try:
-        return spack.spec.Spec(string)
+        result = spack.spec.Spec(string)
     except spack.error.SpecSyntaxError as e:
         mark = get_mark_from_yaml_data(string)
         if mark:
             msg = f"{mark.name}:{mark.line + 1}: {str(e)}"
             raise spack.error.SpecSyntaxError(msg) from e
         raise e
+
+    if named is True and not result.name:
+        msg = f"expected a named spec, but got '{string}' instead"
+        mark = get_mark_from_yaml_data(string)
+
+        # Add a hint in case it's dependencies
+        deps = result.dependencies()
+        if len(deps) == 1:
+            msg = f"{msg}. Did you mean '{deps[0]}'?"
+
+        if mark:
+            msg = f"{mark.name}:{mark.line + 1}: {msg}"
+
+        raise spack.error.SpackError(msg)
+
+    return result
diff --git a/lib/spack/spack/test/concretization/requirements.py b/lib/spack/spack/test/concretization/requirements.py
index c2e5d4cadaf..54bc1e15255 100644
--- a/lib/spack/spack/test/concretization/requirements.py
+++ b/lib/spack/spack/test/concretization/requirements.py
@@ -1125,3 +1125,46 @@ def test_strong_preferences_higher_priority_than_reuse(concretize_scope, mock_pa
         )
         ascent = result.specs[0]
     assert ascent["adios2"].dag_hash() == reused_spec.dag_hash(), ascent
+
+
+@pytest.mark.parametrize(
+    "packages_yaml,err_match",
+    [
+        (
+            """
+packages:
+  mpi:
+    require:
+    - "+bzip2"
+""",
+            "expected a named spec",
+        ),
+        (
+            """
+packages:
+  mpi:
+    require:
+    - one_of: ["+bzip2", openmpi]
+""",
+            "expected a named spec",
+        ),
+        (
+            """
+packages:
+  mpi:
+    require:
+    - "^mpich"
+""",
+            "Did you mean",
+        ),
+    ],
+)
+def test_anonymous_spec_cannot_be_used_in_virtual_requirements(
+    packages_yaml, err_match, concretize_scope, mock_packages
+):
+    """Tests that using anonymous specs in requirements for virtual packages raises an
+    appropriate error message.
+    """
+    update_packages_config(packages_yaml)
+    with pytest.raises(spack.error.SpackError, match=err_match):
+        spack.concretize.concretize_one("mpileaks")
diff --git a/share/spack/gitlab/cloud_pipelines/stacks/data-vis-sdk/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/data-vis-sdk/spack.yaml
index 02594f3b611..a85e004104e 100644
--- a/share/spack/gitlab/cloud_pipelines/stacks/data-vis-sdk/spack.yaml
+++ b/share/spack/gitlab/cloud_pipelines/stacks/data-vis-sdk/spack.yaml
@@ -25,7 +25,7 @@ spack:
       # Minimize LLVM
       variants: ~lldb~lld~libomptarget~polly~gold libunwind=none compiler-rt=none
     libllvm:
-      require: ["^llvm"]
+      require: ["llvm"]
     visit:
       require: ["@3.4.1"]