ASP-based solver: allow to reuse installed externals (#31558)

fixes #31484

Before this change if anything was matching an external
condition, it was considered "external" and thus something
to be "built".

This was happening in particular to external packages
that were re-read from the DB, which then couldn't be
reused, causing the problems shown in #31484.

This PR fixes the issue by excluding specs with a
"hash" from being considered "external"

* Test that users have a way to select a virtual

This ought to be solved by extending the "require"
attribute to virtual packages, so that one can:
```yaml
mpi:
  require: 'multi-provider-mpi'
```

* Prevent conflicts to be enforced on specs that can be reused.

* Rename the "external_only" fact to "buildable_false", to better reflect its origin

4 files changed, 146 insertions, 31 deletions

diff --git a/lib/spack/docs/build_settings.rst b/lib/spack/docs/build_settings.rst
index 654473de9c..3bdeb903bd 100644
--- a/lib/spack/docs/build_settings.rst
+++ b/lib/spack/docs/build_settings.rst
@@ -49,9 +49,8 @@ packages rather than building its own packages. This may be desirable
 if machines ship with system packages, such as a customized MPI
 that should be used instead of Spack building its own MPI.
 
-External packages are configured through the ``packages.yaml`` file found
-in a Spack installation's ``etc/spack/`` or a user's ``~/.spack/``
-directory. Here's an example of an external configuration:
+External packages are configured through the ``packages.yaml`` file.
+Here's an example of an external configuration:
 
 .. code-block:: yaml
 
@@ -97,11 +96,14 @@ Each package version and compiler listed in an external should
 have entries in Spack's packages and compiler configuration, even
 though the package and compiler may not ever be built.
 
-The packages configuration can tell Spack to use an external location
-for certain package versions, but it does not restrict Spack to using
-external packages.  In the above example, since newer versions of OpenMPI
-are available, Spack will choose to start building and linking with the
-latest version rather than continue using the pre-installed OpenMPI versions.
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+Prevent packages from being built from sources
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Adding an external spec in ``packages.yaml`` allows Spack to use an external location,
+but it does not prevent Spack from building packages from sources. In the above example,
+Spack might choose for many valid reasons to start building and linking with the
+latest version of OpenMPI rather than continue using the pre-installed OpenMPI versions.
 
 To prevent this, the ``packages.yaml`` configuration also allows packages
 to be flagged as non-buildable.  The previous example could be modified to
@@ -121,9 +123,15 @@ be:
        buildable: False
 
 The addition of the ``buildable`` flag tells Spack that it should never build
-its own version of OpenMPI, and it will instead always rely on a pre-built
-OpenMPI.  Similar to ``paths``, ``buildable`` is specified as a property under
-a package name.
+its own version of OpenMPI from sources, and it will instead always rely on a pre-built
+OpenMPI.
+
+.. note::
+
+   If ``concretizer:reuse`` is on (see :ref:`concretizer-options` for more information on that flag)
+   pre-built specs include specs already available from a local store, an upstream store, a registered
+   buildcache or specs marked as externals in ``packages.yaml``. If ``concretizer:reuse`` is off, only
+   external specs in ``packages.yaml`` are included in the list of pre-built specs.
 
 If an external module is specified as not buildable, then Spack will load the
 external module into the build environment which can be used for linking.
@@ -132,6 +140,10 @@ The ``buildable`` does not need to be paired with external packages.
 It could also be used alone to forbid packages that may be
 buggy or otherwise undesirable.
 
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+Non-buildable virtual packages
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
 Virtual packages in Spack can also be specified as not buildable, and
 external implementations can be provided. In the example above,
 OpenMPI is configured as not buildable, but Spack will often prefer
@@ -153,21 +165,37 @@ but more conveniently:
        - spec: "openmpi@1.6.5%intel@10.1 arch=linux-debian7-x86_64"
          prefix: /opt/openmpi-1.6.5-intel
 
-Implementations can also be listed immediately under the virtual they provide:
+Spack can then use any of the listed external implementations of MPI
+to satisfy a dependency, and will choose depending on the compiler and
+architecture.
+
+In cases where the concretizer is configured to reuse specs, and other ``mpi`` providers
+(available via stores or buildcaches) are not wanted, Spack can be configured to require
+specs matching only the available externals:
 
 .. code-block:: yaml
 
    packages:
      mpi:
        buildable: False
-         openmpi@1.4.3%gcc@4.4.7 arch=linux-debian7-x86_64: /opt/openmpi-1.4.3
-         openmpi@1.4.3%gcc@4.4.7 arch=linux-debian7-x86_64+debug: /opt/openmpi-1.4.3-debug
-         openmpi@1.6.5%intel@10.1 arch=linux-debian7-x86_64: /opt/openmpi-1.6.5-intel
-         mpich@3.3 %clang@9.0.0 arch=linux-debian7-x86_64: /opt/mpich-3.3-intel
+       require:
+       - one_of: [
+           "openmpi@1.4.3%gcc@4.4.7 arch=linux-debian7-x86_64",
+           "openmpi@1.4.3%gcc@4.4.7 arch=linux-debian7-x86_64+debug",
+           "openmpi@1.6.5%intel@10.1 arch=linux-debian7-x86_64"
+         ]
+     openmpi:
+       externals:
+       - spec: "openmpi@1.4.3%gcc@4.4.7 arch=linux-debian7-x86_64"
+         prefix: /opt/openmpi-1.4.3
+       - spec: "openmpi@1.4.3%gcc@4.4.7 arch=linux-debian7-x86_64+debug"
+         prefix: /opt/openmpi-1.4.3-debug
+       - spec: "openmpi@1.6.5%intel@10.1 arch=linux-debian7-x86_64"
+         prefix: /opt/openmpi-1.6.5-intel
 
-Spack can then use any of the listed external implementations of MPI
-to satisfy a dependency, and will choose depending on the compiler and
-architecture.
+This configuration prevents any spec using MPI and originating from stores or buildcaches to be reused,
+unless it matches the requirements under ``packages:mpi:require``. For more information on requirements see
+:ref:`package-requirements`.
 
 .. _cmd-spack-external-find:
 
@@ -194,11 +222,6 @@ Specific limitations include:
 * Packages are not discoverable by default: For a package to be
   discoverable with ``spack external find``, it needs to add special
   logic. See :ref:`here <make-package-findable>` for more details.
-* The current implementation only collects and examines executable files,
-  so it is typically only useful for build/run dependencies (in some cases
-  if a library package also provides an executable, it may be possible to
-  extract a meaningful Spec by running the executable - for example the
-  compiler wrappers in MPI implementations).
 * The logic does not search through module files, it can only detect
   packages with executables defined in ``PATH``; you can help Spack locate
   externals which use module files by loading any associated modules for
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 7e489f4537..aeef4044d5 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -1210,10 +1210,11 @@ class SpackSolverSetup(object):
 
             self.gen.h2("External package: {0}".format(pkg_name))
             # Check if the external package is buildable. If it is
-            # not then "external(<pkg>)" is a fact.
+            # not then "external(<pkg>)" is a fact, unless we can
+            # reuse an already installed spec.
             external_buildable = data.get("buildable", True)
             if not external_buildable:
-                self.gen.fact(fn.external_only(pkg_name))
+                self.gen.fact(fn.buildable_false(pkg_name))
 
             # Read a list of all the specs for this package
             externals = data.get("externals", [])
diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index fb58589b7a..8bef7f2c1e 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -276,7 +276,8 @@ error(0, Msg) :- node(Package),
    conflict(Package, TriggerID, ConstraintID, Msg),
    condition_holds(TriggerID),
    condition_holds(ConstraintID),
-   not external(Package).  % ignore conflicts for externals
+   not external(Package),  % ignore conflicts for externals
+   not hash(Package, _).  % ignore conflicts for installed packages
 
 #defined conflict/4.
 
@@ -436,7 +437,7 @@ attr("node_compiler_version_satisfies", Package, Compiler, Version)
 #defined external/1.
 #defined external_spec/2.
 #defined external_version_declared/4.
-#defined external_only/1.
+#defined buildable_false/1.
 #defined pkg_provider_preference/4.
 #defined default_provider_preference/3.
 #defined node_version_satisfies/2.
@@ -463,8 +464,10 @@ error(2, "Attempted to use external for '{0}' which does not satisfy any configu
 version_weight(Package, Weight) :- external_version(Package, Version, Weight).
 version(Package, Version) :- external_version(Package, Version, Weight).
 
-% if a package is not buildable (external_only), only externals are allowed
-external(Package) :- external_only(Package), node(Package).
+% if a package is not buildable, only externals or hashed specs are allowed
+external(Package) :- buildable_false(Package),
+                     node(Package),
+                     not hash(Package, _).
 
 % a package is a real_node if it is not external
 real_node(Package) :- node(Package), not external(Package).
@@ -483,7 +486,8 @@ external(Package) :- external_spec_selected(Package, _).
 % determine if an external spec has been selected
 external_spec_selected(Package, LocalIndex) :-
     external_conditions_hold(Package, LocalIndex),
-    node(Package).
+    node(Package),
+    not hash(Package, _).
 
 external_conditions_hold(Package, LocalIndex) :-
     possible_external(ID, Package, LocalIndex), condition_holds(ID).
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index dd6d0e8ab6..f9d8cced62 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -1767,3 +1767,90 @@ class TestConcretize(object):
             match="The reference version 'main' for package 'develop-branch-version'",
         ):
             s.concretized()
+
+    @pytest.mark.regression("31484")
+    def test_installed_externals_are_reused(self, mutable_database, repo_with_changing_recipe):
+        """Test that external specs that are in the DB can be reused."""
+        if spack.config.get("config:concretizer") == "original":
+            pytest.xfail("Use case not supported by the original concretizer")
+
+        # Configuration to be added to packages.yaml
+        external_conf = {
+            "changing": {
+                "buildable": False,
+                "externals": [{"spec": "changing@1.0", "prefix": "/usr"}],
+            }
+        }
+        spack.config.set("packages", external_conf)
+
+        # Install the external spec
+        external1 = Spec("changing@1.0").concretized()
+        external1.package.do_install(fake=True, explicit=True)
+        assert external1.external
+
+        # Modify the package.py file
+        repo_with_changing_recipe.change({"delete_variant": True})
+
+        # Try to concretize the external without reuse and confirm the hash changed
+        with spack.config.override("concretizer:reuse", False):
+            external2 = Spec("changing@1.0").concretized()
+        assert external2.dag_hash() != external1.dag_hash()
+
+        # ... while with reuse we have the same hash
+        with spack.config.override("concretizer:reuse", True):
+            external3 = Spec("changing@1.0").concretized()
+        assert external3.dag_hash() == external1.dag_hash()
+
+    @pytest.mark.regression("31484")
+    def test_user_can_select_externals_with_require(self, mutable_database):
+        """Test that users have means to select an external even in presence of reusable specs."""
+        if spack.config.get("config:concretizer") == "original":
+            pytest.xfail("Use case not supported by the original concretizer")
+
+        # Configuration to be added to packages.yaml
+        external_conf = {
+            "mpi": {"buildable": False},
+            "multi-provider-mpi": {
+                "externals": [{"spec": "multi-provider-mpi@2.0.0", "prefix": "/usr"}]
+            },
+        }
+        spack.config.set("packages", external_conf)
+
+        # mpich and others are installed, so check that
+        # fresh use the external, reuse does not
+        with spack.config.override("concretizer:reuse", False):
+            mpi_spec = Spec("mpi").concretized()
+            assert mpi_spec.name == "multi-provider-mpi"
+
+        with spack.config.override("concretizer:reuse", True):
+            mpi_spec = Spec("mpi").concretized()
+            assert mpi_spec.name != "multi-provider-mpi"
+
+        external_conf["mpi"]["require"] = "multi-provider-mpi"
+        spack.config.set("packages", external_conf)
+
+        with spack.config.override("concretizer:reuse", True):
+            mpi_spec = Spec("mpi").concretized()
+            assert mpi_spec.name == "multi-provider-mpi"
+
+    @pytest.mark.regression("31484")
+    def test_installed_specs_disregard_conflicts(self, mutable_database, monkeypatch):
+        """Test that installed specs do not trigger conflicts. This covers for the rare case
+        where a conflict is added on a package after a spec matching the conflict was installed.
+        """
+        if spack.config.get("config:concretizer") == "original":
+            pytest.xfail("Use case not supported by the original concretizer")
+
+        # Add a conflict to "mpich" that match an already installed "mpich~debug"
+        pkg_cls = spack.repo.path.get_pkg_class("mpich")
+        monkeypatch.setitem(pkg_cls.conflicts, "~debug", [(spack.spec.Spec(), None)])
+
+        # If we concretize with --fresh the conflict is taken into account
+        with spack.config.override("concretizer:reuse", False):
+            s = Spec("mpich").concretized()
+            assert s.satisfies("+debug")
+
+        # If we concretize with --reuse it is not, since "mpich~debug" was already installed
+        with spack.config.override("concretizer:reuse", True):
+            s = Spec("mpich").concretized()
+            assert s.satisfies("~debug")