external find --not-buildable: mark virtuals (#45159)

This change makes `spack external find --not-buildable` mark virtuals
provided by detected packages as non-buildable, so that it's sufficient
for users to let spack detect say mpich and have the concretizer pick it
up as mpi provider even when openmpi is "more preferred".

4 files changed, 74 insertions, 7 deletions

diff --git a/lib/spack/spack/cmd/external.py b/lib/spack/spack/cmd/external.py
index 8b6e750932..421685d42a 100644
--- a/lib/spack/spack/cmd/external.py
+++ b/lib/spack/spack/cmd/external.py
@@ -7,7 +7,7 @@ import errno
 import os
 import re
 import sys
-from typing import List, Optional
+from typing import List, Optional, Set
 
 import llnl.util.tty as tty
 import llnl.util.tty.colify as colify
@@ -19,6 +19,7 @@ import spack.cray_manifest as cray_manifest
 import spack.detection
 import spack.error
 import spack.repo
+import spack.spec
 import spack.util.environment
 from spack.cmd.common import arguments
 
@@ -138,14 +139,26 @@ def external_find(args):
         candidate_packages, path_hints=args.path, max_workers=args.jobs
     )
 
-    new_entries = spack.detection.update_configuration(
+    new_specs = spack.detection.update_configuration(
         detected_packages, scope=args.scope, buildable=not args.not_buildable
     )
-    if new_entries:
+
+    # If the user runs `spack external find --not-buildable mpich` we also mark `mpi` non-buildable
+    # to avoid that the concretizer picks a different mpi provider.
+    if new_specs and args.not_buildable:
+        virtuals: Set[str] = {
+            virtual.name
+            for new_spec in new_specs
+            for virtual_specs in spack.repo.PATH.get_pkg_class(new_spec.name).provided.values()
+            for virtual in virtual_specs
+        }
+        new_virtuals = spack.detection.set_virtuals_nonbuildable(virtuals, scope=args.scope)
+        new_specs.extend(spack.spec.Spec(name) for name in new_virtuals)
+
+    if new_specs:
         path = spack.config.CONFIG.get_config_filename(args.scope, "packages")
-        msg = "The following specs have been detected on this system and added to {0}"
-        tty.msg(msg.format(path))
-        spack.cmd.display_specs(new_entries)
+        tty.msg(f"The following specs have been detected on this system and added to {path}")
+        spack.cmd.display_specs(new_specs)
     else:
         tty.msg("No new external packages detected")
 
diff --git a/lib/spack/spack/detection/__init__.py b/lib/spack/spack/detection/__init__.py
index 44225ef062..1cff789c26 100644
--- a/lib/spack/spack/detection/__init__.py
+++ b/lib/spack/spack/detection/__init__.py
@@ -2,7 +2,12 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-from .common import DetectedPackage, executable_prefix, update_configuration
+from .common import (
+    DetectedPackage,
+    executable_prefix,
+    set_virtuals_nonbuildable,
+    update_configuration,
+)
 from .path import by_path, executables_in_path
 from .test import detection_tests
 
@@ -12,5 +17,6 @@ __all__ = [
     "executables_in_path",
     "executable_prefix",
     "update_configuration",
+    "set_virtuals_nonbuildable",
     "detection_tests",
 ]
diff --git a/lib/spack/spack/detection/common.py b/lib/spack/spack/detection/common.py
index 516bd11c8d..ae3c1927d2 100644
--- a/lib/spack/spack/detection/common.py
+++ b/lib/spack/spack/detection/common.py
@@ -252,6 +252,27 @@ def update_configuration(
     return all_new_specs
 
 
+def set_virtuals_nonbuildable(virtuals: Set[str], scope: Optional[str] = None) -> List[str]:
+    """Update packages:virtual:buildable:False for the provided virtual packages, if the property
+    is not set by the user. Returns the list of virtual packages that have been updated."""
+    packages = spack.config.get("packages")
+    new_config = {}
+    for virtual in virtuals:
+        # If the user has set the buildable prop do not override it
+        if virtual in packages and "buildable" in packages[virtual]:
+            continue
+        new_config[virtual] = {"buildable": False}
+
+    # Update the provided scope
+    spack.config.set(
+        "packages",
+        spack.config.merge_yaml(spack.config.get("packages", scope=scope), new_config),
+        scope=scope,
+    )
+
+    return list(new_config.keys())
+
+
 def _windows_drive() -> str:
     """Return Windows drive string extracted from the PROGRAMFILES environment variable,
     which is guaranteed to be defined for all logins.
diff --git a/lib/spack/spack/test/cmd/external.py b/lib/spack/spack/test/cmd/external.py
index e5bb29c6ea..2de6f18b50 100644
--- a/lib/spack/spack/test/cmd/external.py
+++ b/lib/spack/spack/test/cmd/external.py
@@ -11,6 +11,7 @@ import pytest
 from llnl.util.filesystem import getuid, touch
 
 import spack
+import spack.cmd.external
 import spack.detection
 import spack.detection.path
 from spack.main import SpackCommand
@@ -311,3 +312,29 @@ def test_failures_in_scanning_do_not_result_in_an_error(
     assert "cmake" in output
     assert "3.23.3" in output
     assert "3.19.1" not in output
+
+
+def test_detect_virtuals(mock_executable, mutable_config, monkeypatch):
+    """Test whether external find --not-buildable sets virtuals as non-buildable (unless user
+    config sets them to buildable)"""
+    mpich = mock_executable("mpichversion", output="echo MPICH Version:    4.0.2")
+    prefix = os.path.dirname(mpich)
+    external("find", "--path", prefix, "--not-buildable", "mpich")
+
+    # Check that mpich was correctly detected
+    mpich = mutable_config.get("packages:mpich")
+    assert mpich["buildable"] is False
+    assert Spec(mpich["externals"][0]["spec"]).satisfies("mpich@4.0.2")
+
+    # Check that the virtual package mpi was marked as non-buildable
+    assert mutable_config.get("packages:mpi:buildable") is False
+
+    # Delete the mpich entry, and set mpi explicitly to buildable
+    mutable_config.set("packages:mpich", {})
+    mutable_config.set("packages:mpi:buildable", True)
+
+    # Run the detection again
+    external("find", "--path", prefix, "--not-buildable", "mpich")
+
+    # Check that the mpi:buildable entry was not overwritten
+    assert mutable_config.get("packages:mpi:buildable") is True