concretizer: remove ad-hoc rule for external packages (#20193)

fixes #20040

Matching compilers among nodes has been prioritized
in #20020. Selection of default variants has been
tuned in #20182. With this setup there is no need
to have an ad-hoc rule for external packages. On
the contrary it should be removed to prefer having
default variant values over more external nodes in
the DAG.

4 files changed, 42 insertions, 7 deletions

diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index d8b7c125be..2de7030ddf 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -515,13 +515,6 @@ root(Dependency, 1) :- not root(Dependency), node(Dependency).
     : provider_weight(Provider, Weight), root(Provider)
 }.
 
-% For external packages it's more important than for others
-% to match the compiler with their parent node
-#maximize{
-    Weight@12,Package
-    : compiler_version_match(Package, Weight), external(Package)
-}.
-
 % Try to use default variants or variants that have been set
 #minimize {
     Weight@11,Package,Variant,Value
diff --git a/lib/spack/spack/test/concretize_preferences.py b/lib/spack/spack/test/concretize_preferences.py
index d13748ad8b..9ec304e624 100644
--- a/lib/spack/spack/test/concretize_preferences.py
+++ b/lib/spack/spack/test/concretize_preferences.py
@@ -371,3 +371,13 @@ mpi:
         spec = Spec('callpath')
         with pytest.raises(ConfigError):
             spack.package_prefs.get_package_permissions(spec)
+
+    @pytest.mark.regression('20040')
+    def test_variant_not_flipped_to_pull_externals(self):
+        """Test that a package doesn't prefer pulling in an
+        external to using the default value of a variant.
+        """
+        s = Spec('vdefault-or-external-root').concretized()
+
+        assert '~external' in s['vdefault-or-external']
+        assert 'externaltool' not in s
diff --git a/var/spack/repos/builtin.mock/packages/vdefault-or-external-root/package.py b/var/spack/repos/builtin.mock/packages/vdefault-or-external-root/package.py
new file mode 100644
index 0000000000..49011fc458
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/vdefault-or-external-root/package.py
@@ -0,0 +1,15 @@
+# Copyright 2013-2020 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 VdefaultOrExternalRoot(Package):
+    """Test that we don't prefer adding an external to using
+    a default variant value.
+    """
+
+    homepage = 'https://www.example.org'
+    url = 'https://example.org/files/v3.4/cmake-3.4.3.tar.gz'
+
+    version('1.0', '4cb3ff35b2472aae70f542116d616e63')
+
+    depends_on('vdefault-or-external')
diff --git a/var/spack/repos/builtin.mock/packages/vdefault-or-external/package.py b/var/spack/repos/builtin.mock/packages/vdefault-or-external/package.py
new file mode 100644
index 0000000000..80c674917e
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/vdefault-or-external/package.py
@@ -0,0 +1,17 @@
+# Copyright 2013-2020 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 VdefaultOrExternal(Package):
+    """Test that we don't prefer adding an external to using
+    a default variant value.
+    """
+
+    homepage = 'https://www.example.org'
+    url = 'https://example.org/files/v3.4/cmake-3.4.3.tar.gz'
+
+    version('1.0', '4cb3ff35b2472aae70f542116d616e63')
+
+    variant('external', default=False, description='nope')
+
+    depends_on('externaltool', when='+external')