Externals are preferred even when they have non-default variant values

fixes #22596

Variants which are specified in an external spec are not
scored negatively if they encode a non-default value.

5 files changed, 64 insertions, 2 deletions

diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index b875c58527..9e66760695 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -360,23 +360,40 @@ variant_value(Package, Variant, Value)
     variant(Package, Variant),
     variant_set(Package, Variant, Value).
 
-% prefer default values.
+% The rules below allow us to prefer default values for variants
+% whenever possible. If a variant is set in a spec, or if it is
+% specified in an external, we score it as if it was a default value.
 variant_not_default(Package, Variant, Value, 1)
  :- variant_value(Package, Variant, Value),
     not variant_default_value(Package, Variant, Value),
     not variant_set(Package, Variant, Value),
+    not external_with_variant_set(Package, Variant, Value),
     node(Package).
 
+% We are using the default value for a variant
 variant_not_default(Package, Variant, Value, 0)
  :- variant_value(Package, Variant, Value),
     variant_default_value(Package, Variant, Value),
     node(Package).
 
+% The variant is set in the spec
 variant_not_default(Package, Variant, Value, 0)
  :- variant_value(Package, Variant, Value),
     variant_set(Package, Variant, Value),
     node(Package).
 
+% The variant is set in an external spec
+external_with_variant_set(Package, Variant, Value)
+ :- variant_value(Package, Variant, Value),
+    condition_requirement(ID, "variant_value", Package, Variant, Value),
+    possible_external(ID, Package, _),
+    external(Package),
+    node(Package).
+
+variant_not_default(Package, Variant, Value, 0)
+ :- variant_value(Package, Variant, Value),
+    external_with_variant_set(Package, Variant, Value),
+    node(Package).
 
 % The default value for a variant in a package is what is written
 % in the package.py file, unless some preference is set in packages.yaml
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index c118304e64..f5294ae596 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -1142,3 +1142,13 @@ class TestConcretize(object):
 
         s = Spec('mvapich2').concretized()
         assert set(s.variants['file_systems'].value) == set(['ufs', 'nfs'])
+
+    @pytest.mark.regression('22596')
+    def test_external_with_non_default_variant_as_dependency(self):
+        # This package depends on another that is registered as an external
+        # with 'buildable: true' and a variant with a non-default value set
+        s = Spec('trigger-external-non-default-variant').concretized()
+
+        assert '~foo' in s['external-non-default-variant']
+        assert '~bar' in s['external-non-default-variant']
+        assert s['external-non-default-variant'].external
diff --git a/lib/spack/spack/test/data/config/packages.yaml b/lib/spack/spack/test/data/config/packages.yaml
index 83f8cf1bb3..496c3623cc 100644
--- a/lib/spack/spack/test/data/config/packages.yaml
+++ b/lib/spack/spack/test/data/config/packages.yaml
@@ -35,4 +35,14 @@ packages:
       - spec: external-buildable-with-variant@1.1.special +baz
         prefix: /usr
       - spec: external-buildable-with-variant@0.9 +baz
-        prefix: /usr
\ No newline at end of file
+        prefix: /usr
+  'old-external':
+    buildable: True
+    externals:
+      - spec: old-external@1.0.0
+        prefix: /usr
+  'external-non-default-variant':
+    buildable: True
+    externals:
+      - spec: external-non-default-variant@3.8.7~foo~bar
+        prefix: /usr
diff --git a/var/spack/repos/builtin.mock/packages/external-non-default-variant/package.py b/var/spack/repos/builtin.mock/packages/external-non-default-variant/package.py
new file mode 100644
index 0000000000..f5a4e0f07d
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/external-non-default-variant/package.py
@@ -0,0 +1,13 @@
+# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+class ExternalNonDefaultVariant(Package):
+    """An external that is registered with a non-default value"""
+    homepage = "http://www.python.org"
+    url = "http://www.python.org/ftp/python/3.8.7/Python-3.8.7.tgz"
+
+    version('3.8.7', 'be78e48cdfc1a7ad90efff146dce6cfe')
+
+    variant('foo', default=True, description='just a variant')
+    variant('bar', default=True, description='just a variant')
diff --git a/var/spack/repos/builtin.mock/packages/trigger-external-non-default-variant/package.py b/var/spack/repos/builtin.mock/packages/trigger-external-non-default-variant/package.py
new file mode 100644
index 0000000000..8b2b2be70d
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/trigger-external-non-default-variant/package.py
@@ -0,0 +1,12 @@
+# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+class TriggerExternalNonDefaultVariant(Package):
+    """This ackage depends on an external with a non-default variant"""
+    homepage = "http://www.example.com"
+    url = "http://www.someurl.tar.gz"
+
+    version('1.0', 'foobarbaz')
+
+    depends_on('external-non-default-variant')