bugfix: make compiler preferences slightly saner (#17590)

* bugfix: make compiler preferences slightly saner

This fixes two issues with the way we currently select compilers.

If multiple compilers have the same "id" (os/arch/compiler/version), we
currently prefer them by picking this one with the most supported
languages.  This can have some surprising effects:

* If you have no `gfortran` but you have `gfortran-8`, you can detect
  `clang` that has no configured C compiler -- just `f77` and `f90`. This
  happens frequently on macOS with homebrew. The bug is due to some
  kludginess about the way we detect mixed `clang`/`gfortran`.

* We can prefer suffixed versions of compilers to non-suffixed versions,
  which means we may select `clang-gpu` over `clang` at LLNL. But,
  `clang-gpu` is not actually clang, and it can break builds. We should
  prefer `clang` if it's available.

- [x] prefer compilers that have C compilers and prefer no name variation
  to variation.

* tests: add test for which()

5 files changed, 191 insertions, 19 deletions

diff --git a/lib/spack/spack/compiler.py b/lib/spack/spack/compiler.py
index 9f3c4dc1c7..c59c654803 100644
--- a/lib/spack/spack/compiler.py
+++ b/lib/spack/spack/compiler.py
@@ -28,7 +28,7 @@ __all__ = ['Compiler']
 
 
 @llnl.util.lang.memoized
-def get_compiler_version_output(compiler_path, version_arg, ignore_errors=()):
+def _get_compiler_version_output(compiler_path, version_arg, ignore_errors=()):
     """Invokes the compiler at a given path passing a single
     version argument and returns the output.
 
@@ -42,6 +42,18 @@ def get_compiler_version_output(compiler_path, version_arg, ignore_errors=()):
     return output
 
 
+def get_compiler_version_output(compiler_path, *args, **kwargs):
+    """Wrapper for _get_compiler_version_output()."""
+    # This ensures that we memoize compiler output by *absolute path*,
+    # not just executable name. If we don't do this, and the path changes
+    # (e.g., during testing), we can get incorrect results.
+    if not os.path.isabs(compiler_path):
+        compiler_path = spack.util.executable.which_string(
+            compiler_path, required=True)
+
+    return _get_compiler_version_output(compiler_path, *args, **kwargs)
+
+
 def tokenize_flags(flags_str):
     """Given a compiler flag specification as a string, this returns a list
        where the entries are the flags. For compiler options which set values
diff --git a/lib/spack/spack/compilers/__init__.py b/lib/spack/spack/compilers/__init__.py
index dfa750cc4d..0f0fb94c1e 100644
--- a/lib/spack/spack/compilers/__init__.py
+++ b/lib/spack/spack/compilers/__init__.py
@@ -650,23 +650,18 @@ def make_compiler_list(detected_versions):
     Returns:
         list of Compiler objects
     """
-    # We don't sort on the path of the compiler
-    sort_fn = lambda x: (x.id, x.variation, x.language)
-    compilers_s = sorted(detected_versions, key=sort_fn)
+    group_fn = lambda x: (x.id, x.variation, x.language)
+    sorted_compilers = sorted(detected_versions, key=group_fn)
 
     # Gather items in a dictionary by the id, name variation and language
     compilers_d = {}
-    for sort_key, group in itertools.groupby(compilers_s, key=sort_fn):
+    for sort_key, group in itertools.groupby(sorted_compilers, key=group_fn):
         compiler_id, name_variation, language = sort_key
         by_compiler_id = compilers_d.setdefault(compiler_id, {})
         by_name_variation = by_compiler_id.setdefault(name_variation, {})
         by_name_variation[language] = next(x.path for x in group)
 
-    # For each unique compiler id select the name variation with most entries
-    # i.e. the one that supports most languages
-    compilers = []
-
-    def _default(cmp_id, paths):
+    def _default_make_compilers(cmp_id, paths):
         operating_system, compiler_name, version = cmp_id
         compiler_cls = spack.compilers.class_for_compiler_name(compiler_name)
         spec = spack.spec.CompilerSpec(compiler_cls.name, version)
@@ -677,16 +672,38 @@ def make_compiler_list(detected_versions):
         )
         return [compiler]
 
+    # For compilers with the same compiler id:
+    #
+    # - Prefer with C compiler to without
+    # - Prefer with C++ compiler to without
+    # - Prefer no variations to variations (e.g., clang to clang-gpu)
+    #
+    sort_fn = lambda variation: (
+        'cc' not in by_compiler_id[variation],  # None last
+        'cxx' not in by_compiler_id[variation],  # None last
+        variation.prefix,
+        variation.suffix,
+    )
+
+    compilers = []
     for compiler_id, by_compiler_id in compilers_d.items():
-        _, selected_name_variation = max(
-            (len(by_compiler_id[variation]), variation)
-            for variation in by_compiler_id
-        )
+        ordered = sorted(by_compiler_id, key=sort_fn)
+        selected_variation = ordered[0]
+        selected = by_compiler_id[selected_variation]
+
+        # fill any missing parts from subsequent entries
+        for lang in ['cxx', 'f77', 'fc']:
+            if lang not in selected:
+                next_lang = next((
+                    by_compiler_id[v][lang] for v in ordered
+                    if lang in by_compiler_id[v]), None)
+                if next_lang:
+                    selected[lang] = next_lang
 
-        # Add it to the list of compilers
-        selected = by_compiler_id[selected_name_variation]
         operating_system, _, _ = compiler_id
-        make_compilers = getattr(operating_system, 'make_compilers', _default)
+        make_compilers = getattr(
+            operating_system, 'make_compilers', _default_make_compilers)
+
         compilers.extend(make_compilers(compiler_id, selected))
 
     return compilers
diff --git a/lib/spack/spack/test/cmd/compiler.py b/lib/spack/spack/test/cmd/compiler.py
index f496727081..61c67ccecd 100644
--- a/lib/spack/spack/test/cmd/compiler.py
+++ b/lib/spack/spack/test/cmd/compiler.py
@@ -3,6 +3,8 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 import os
+import shutil
+import sys
 
 import pytest
 
@@ -14,7 +16,7 @@ compiler = spack.main.SpackCommand('compiler')
 
 
 @pytest.fixture
-def no_compilers_yaml(mutable_config, monkeypatch):
+def no_compilers_yaml(mutable_config):
     """Creates a temporary configuration without compilers.yaml"""
 
     for scope, local_config in mutable_config.scopes.items():
@@ -130,3 +132,121 @@ def test_compiler_add(
     new_compiler = new_compilers - old_compilers
     assert any(c.version == spack.version.Version(mock_compiler_version)
                for c in new_compiler)
+
+
+@pytest.fixture
+def clangdir(tmpdir):
+    """Create a directory with some dummy compiler scripts in it.
+
+    Scripts are:
+      - clang
+      - clang++
+      - gcc
+      - g++
+      - gfortran-8
+
+    """
+    with tmpdir.as_cwd():
+        with open('clang', 'w') as f:
+            f.write("""\
+#!/bin/sh
+if [ "$1" = "--version" ]; then
+    echo "clang version 11.0.0 (clang-1100.0.33.16)"
+    echo "Target: x86_64-apple-darwin18.7.0"
+    echo "Thread model: posix"
+    echo "InstalledDir: /dummy"
+else
+    echo "clang: error: no input files"
+    exit 1
+fi
+""")
+        shutil.copy('clang', 'clang++')
+
+        gcc_script = """\
+#!/bin/sh
+if [ "$1" = "-dumpversion" ]; then
+    echo "8"
+elif [ "$1" = "-dumpfullversion" ]; then
+    echo "8.4.0"
+elif [ "$1" = "--version" ]; then
+    echo "{0} (GCC) 8.4.0 20120313 (Red Hat 8.4.0-1)"
+    echo "Copyright (C) 2010 Free Software Foundation, Inc."
+else
+    echo "{1}: fatal error: no input files"
+    echo "compilation terminated."
+    exit 1
+fi
+"""
+        with open('gcc-8', 'w') as f:
+            f.write(gcc_script.format('gcc', 'gcc-8'))
+        with open('g++-8', 'w') as f:
+            f.write(gcc_script.format('g++', 'g++-8'))
+        with open('gfortran-8', 'w') as f:
+            f.write(gcc_script.format('GNU Fortran', 'gfortran-8'))
+        os.chmod('clang', 0o700)
+        os.chmod('clang++', 0o700)
+        os.chmod('gcc-8', 0o700)
+        os.chmod('g++-8', 0o700)
+        os.chmod('gfortran-8', 0o700)
+
+    yield tmpdir
+
+
+@pytest.mark.regression('17590')
+def test_compiler_find_mixed_suffixes(
+        no_compilers_yaml, working_env, clangdir):
+    """Ensure that we'll mix compilers with different suffixes when necessary.
+    """
+    os.environ['PATH'] = str(clangdir)
+    output = compiler('find', '--scope=site')
+
+    assert 'clang@11.0.0' in output
+    assert 'gcc@8.4.0' in output
+
+    config = spack.compilers.get_compiler_config('site', False)
+    clang = next(c['compiler'] for c in config
+                 if c['compiler']['spec'] == 'clang@11.0.0')
+    gcc = next(c['compiler'] for c in config
+               if c['compiler']['spec'] == 'gcc@8.4.0')
+
+    gfortran_path = str(clangdir.join('gfortran-8'))
+
+    assert clang['paths'] == {
+        'cc': str(clangdir.join('clang')),
+        'cxx': str(clangdir.join('clang++')),
+        # we only auto-detect mixed clang on macos
+        'f77': gfortran_path if sys.platform == 'darwin' else None,
+        'fc': gfortran_path if sys.platform == 'darwin' else None,
+    }
+
+    assert gcc['paths'] == {
+        'cc': str(clangdir.join('gcc-8')),
+        'cxx': str(clangdir.join('g++-8')),
+        'f77': gfortran_path,
+        'fc': gfortran_path,
+    }
+
+
+@pytest.mark.regression('17590')
+def test_compiler_find_prefer_no_suffix(
+        no_compilers_yaml, working_env, clangdir):
+    """Ensure that we'll pick 'clang' over 'clang-gpu' when there is a choice.
+    """
+    with clangdir.as_cwd():
+        shutil.copy('clang', 'clang-gpu')
+        shutil.copy('clang++', 'clang++-gpu')
+        os.chmod('clang-gpu', 0o700)
+        os.chmod('clang++-gpu', 0o700)
+
+    os.environ['PATH'] = str(clangdir)
+    output = compiler('find', '--scope=site')
+
+    assert 'clang@11.0.0' in output
+    assert 'gcc@8.4.0' in output
+
+    config = spack.compilers.get_compiler_config('site', False)
+    clang = next(c['compiler'] for c in config
+                 if c['compiler']['spec'] == 'clang@11.0.0')
+
+    assert clang['paths']['cc'] == str(clangdir.join('clang'))
+    assert clang['paths']['cxx'] == str(clangdir.join('clang++'))
diff --git a/lib/spack/spack/test/util/executable.py b/lib/spack/spack/test/util/executable.py
index 8baae3b453..5e8795f4bf 100644
--- a/lib/spack/spack/test/util/executable.py
+++ b/lib/spack/spack/test/util/executable.py
@@ -6,7 +6,10 @@
 import sys
 import os
 
+import pytest
+
 import llnl.util.filesystem as fs
+
 import spack
 import spack.util.executable as ex
 from spack.hooks.sbang import filter_shebangs_in_directory
@@ -35,3 +38,18 @@ print(u'\\xc3')
         # read the unicode back in and see whether things work
         script = ex.Executable('./%s' % script_name)
         assert u'\xc3' == script(output=str).strip()
+
+
+def test_which(tmpdir):
+    os.environ["PATH"] = str(tmpdir)
+    assert ex.which("spack-test-exe") is None
+    with pytest.raises(ex.CommandNotFoundError):
+        ex.which("spack-test-exe", required=True)
+
+    with tmpdir.as_cwd():
+        fs.touch("spack-test-exe")
+        fs.set_executable('spack-test-exe')
+
+    exe = ex.which("spack-test-exe")
+    assert exe is not None
+    assert exe.path == str(tmpdir.join("spack-test-exe"))
diff --git a/lib/spack/spack/util/executable.py b/lib/spack/spack/util/executable.py
index 1f5fdfb761..28656b0a32 100644
--- a/lib/spack/spack/util/executable.py
+++ b/lib/spack/spack/util/executable.py
@@ -239,7 +239,8 @@ def which_string(*args, **kwargs):
                 return exe
 
     if required:
-        tty.die("spack requires '%s'. Make sure it is in your path." % args[0])
+        raise CommandNotFoundError(
+            "spack requires '%s'. Make sure it is in your path." % args[0])
 
     return None
 
@@ -266,3 +267,7 @@ def which(*args, **kwargs):
 
 class ProcessError(spack.error.SpackError):
     """ProcessErrors are raised when Executables exit with an error code."""
+
+
+class CommandNotFoundError(spack.error.SpackError):
+    """Raised when ``which()`` can't find a required executable."""