From f528022a7d4a3671deed9b031e878cf7a0ba950f Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Tue, 21 Jul 2020 18:48:37 -0700 Subject: [PATCH] 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() --- lib/spack/spack/compiler.py | 14 ++- lib/spack/spack/compilers/__init__.py | 51 ++++++---- lib/spack/spack/test/cmd/compiler.py | 122 +++++++++++++++++++++++- lib/spack/spack/test/util/executable.py | 18 ++++ lib/spack/spack/util/executable.py | 7 +- 5 files changed, 192 insertions(+), 20 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 @@ @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 _default(cmp_id, paths): ) return [compiler] - 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 - ) + # 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(): + 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 @@ @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 @@ def test_read_unicode(tmpdir, working_env): # 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."""