Cleaned code that performs compiler detection

* Simplified _CompilerID

* Extracted search_compiler_commands from Compiler

* Added search_regexps to Compiler

* A few functions manipulating paths that could be useful in other
  parts of the code have been moved to llnl.util.filesystem

* Removed deferred functions in favor of mapping arguments to
  functions as required in the review

* Moved most of the code involved in compiler detection in
  spack.compilers

lib/spack/llnl/util/filesystem.py

@@ -9,6 +9,7 @@
 import fileinput
 import glob
 import grp
+import itertools
 import numbers
 import os
 import pwd
@@ -1385,3 +1386,30 @@ def files_in(*search_paths):
             [(f, os.path.join(d, f)) for f in os.listdir(d)]
         ))
     return files
+
+
+def search_paths_for_executables(*path_hints):
+    """Given a list of path hints returns a list of paths where
+    to search for an executable.
+
+    Args:
+        *path_hints (list of paths): list of paths taken into
+            consideration for a search
+
+    Returns:
+        A list containing the real path of every existing directory
+        in `path_hints` and its `bin` subdirectory if it exists.
+    """
+    # Select the realpath of existing directories
+    existing_paths = filter(os.path.isdir, map(os.path.realpath, path_hints))
+
+    # Adding their 'bin' subdirectory
+    def maybe_add_bin(path):
+        bin_subdirectory = os.path.realpath(os.path.join(path, 'bin'))
+        if os.path.isdir(bin_subdirectory):
+            return [path, bin_subdirectory]
+        return [path]
+
+    return list(
+        itertools.chain.from_iterable(map(maybe_add_bin, existing_paths))
+    )

lib/spack/llnl/util/multiproc.py

@@ -8,30 +8,11 @@
 than multiprocessing.Pool.apply() can.  For example, apply() will fail
 to pickle functions if they're passed indirectly as parameters.
 """
-import functools
 from multiprocessing import Semaphore, Value
 
 __all__ = ['Barrier']
 
 
-def defer(func):
-    """Package a function call into something that can be invoked
-    at a later moment.
-
-    Args:
-        func (callable): callable that must be deferred
-
-    Returns:
-        Deferred version of the same function
-    """
-    @functools.wraps(func)
-    def _impl(*args, **kwargs):
-        def _deferred_call():
-            return func(*args, **kwargs)
-        return _deferred_call
-    return _impl
-
-
 class Barrier:
     """Simple reusable semaphore barrier.
 

lib/spack/spack/architecture.py

@@ -56,9 +56,7 @@
 attributes front_os and back_os. The operating system as described earlier,
 will be responsible for compiler detection.
 """
-import os
 import inspect
-import itertools
 
 import llnl.util.tty as tty
 from llnl.util.lang import memoized, list_modules, key_ordering
@@ -67,7 +65,6 @@
 import spack.paths
 import spack.error as serr
 from spack.util.naming import mod_to_class
-from spack.util.environment import get_path
 from spack.util.spack_yaml import syaml_dict
 
 
@@ -231,32 +228,6 @@ def __repr__(self):
     def _cmp_key(self):
         return self.name, self.version
 
-    def search_compiler_commands(self, *path_hints):
-        """Returns a list of commands that, when invoked, search for
-        compilers tied to this OS.
-
-        Args:
-            *path_hints (list of paths): path where to look for compilers
-
-        Returns:
-            (tags, commands): ``tags`` is a list of compiler tags, containing
-                all the information on a compiler, but version. ``commands``
-                is a list of commands that, when executed, will detect the
-                version of the corresponding compiler.
-        """
-        # Turn the path hints into paths that are to be searched
-        paths = executable_search_paths(path_hints or get_path('PATH'))
-
-        # NOTE: we import spack.compilers here to avoid init order cycles
-        import spack.compilers
-
-        tags, commands = [], []
-        for compiler_cls in spack.compilers.all_compiler_types():
-            t, c = compiler_cls.search_compiler_commands(self, *paths)
-            tags.extend(t), commands.extend(c)
-
-        return tags, commands
-
     def to_dict(self):
         return {
             'name': self.name,
@@ -432,30 +403,3 @@ def sys_type():
     """
     arch = Arch(platform(), 'default_os', 'default_target')
     return str(arch)
-
-
-def executable_search_paths(path_hints):
-    """Given a list of path hints returns a list of paths where
-    to search for an executable.
-
-    Args:
-        path_hints (list of paths): list of paths taken into
-            consideration for a search
-
-    Returns:
-        A list containing the real path of every existing directory
-        in `path_hints` and its `bin` subdirectory if it exists.
-    """
-    # Select the realpath of existing directories
-    existing_paths = filter(os.path.isdir, map(os.path.realpath, path_hints))
-
-    # Adding their 'bin' subdirectory
-    def maybe_add_bin(path):
-        bin_subdirectory = os.path.realpath(os.path.join(path, 'bin'))
-        if os.path.isdir(bin_subdirectory):
-            return [path, bin_subdirectory]
-        return [path]
-
-    return list(itertools.chain.from_iterable(
-        map(maybe_add_bin, existing_paths))
-    )

lib/spack/spack/compiler.py

@@ -3,19 +3,11 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-import collections
 import os
 import re
 import itertools
 
-import functools_backport
-import platform as py_platform
-import six
-
-import llnl.util.lang
-import llnl.util.multiproc
 import llnl.util.filesystem
-import llnl.util.tty as tty
 
 import spack.error
 import spack.spec
@@ -255,60 +247,19 @@ def fc_version(cls, fc):
         return cls.default_version(fc)
 
     @classmethod
-    def search_compiler_commands(cls, operating_system, *search_paths):
+    def search_regexps(cls, language):
-        """Returns a list of commands that, when invoked, search for compilers
+        # Compile all the regular expressions used for files beforehand.
-        in the paths supplied.
+        # This searches for any combination of <prefix><name><suffix>
-
+        # defined for the compiler
-        Args:
+        compiler_names = getattr(cls, '{0}_names'.format(language))
-            operating_system (OperatingSystem): the OS requesting the search
+        prefixes = [''] + cls.prefixes
-            *search_paths (list of paths): paths where to look for a
+        suffixes = [''] + cls.suffixes
-                compiler
+        regexp_fmt = r'^({0}){1}({2})$'
-
+        return [
-        Returns:
+            re.compile(regexp_fmt.format(prefix, re.escape(name), suffix))
-            (tags, commands): ``tags`` is a list of compiler tags, containing
+            for prefix, name, suffix in
-                all the information on a compiler, but version. ``commands``
+            itertools.product(prefixes, compiler_names, suffixes)
-                is a list of commands that, when executed, will detect the
+        ]
-                version of the corresponding compiler.
-        """
-        files_to_be_tested = llnl.util.filesystem.files_in(*search_paths)
-
-        tags, commands = [], []
-        for language in ('cc', 'cxx', 'f77', 'fc'):
-
-            # Get compiler names and the callback to detect their versions
-            compiler_names = getattr(cls, '{0}_names'.format(language))
-            callback = getattr(cls, '{0}_version'.format(language))
-
-            # Compile all the regular expressions used for files beforehand.
-            # This searches for any combination of <prefix><name><suffix>
-            # defined for the compiler
-            prefixes = [''] + cls.prefixes
-            suffixes = [''] + cls.suffixes
-            regexp_fmt = r'^({0}){1}({2})$'
-            search_regexps = [
-                re.compile(regexp_fmt.format(prefix, re.escape(name), suffix))
-                for prefix, name, suffix in
-                itertools.product(prefixes, compiler_names, suffixes)
-            ]
-
-            # Select only the files matching a regexp
-            for (file, full_path), regexp in itertools.product(
-                    files_to_be_tested, search_regexps
-            ):
-                match = regexp.match(file)
-                if match:
-                    tags.append(
-                        (_CompilerID(operating_system, cls, None),
-                         _NameVariation(*match.groups()), language)
-                    )
-                    commands.append(
-                        llnl.util.multiproc.defer(detect_version)
-                        (callback, full_path)
-                    )
-
-        # Reverse it here so that the dict creation (last insert wins)
-        # does not spoil the intended precedence.
-        return reversed(tags), reversed(commands)
 
     def setup_custom_environment(self, pkg, env):
         """Set any environment variables necessary to use the compiler."""
@@ -326,116 +277,6 @@ def __str__(self):
                 str(self.operating_system)))))
 
 
-@functools_backport.total_ordering
-class _CompilerID(collections.namedtuple('_CompilerIDBase', [
-    'os', 'cmp_cls', 'version'
-])):
-    """Gathers the attribute values by which a detected compiler is unique."""
-    def _tuple_repr(self):
-        return self.os, str(self.cmp_cls), self.version
-
-    def __eq__(self, other):
-        if not isinstance(other, _CompilerID):
-            return NotImplemented
-        return self._tuple_repr() == other._tuple_repr()
-
-    def __lt__(self, other):
-        if not isinstance(other, _CompilerID):
-            return NotImplemented
-        return self._tuple_repr() < other._tuple_repr()
-
-    def __hash__(self):
-        return hash(self._tuple_repr())
-
-
-#: Variations on a matched compiler name
-_NameVariation = collections.namedtuple('_NameVariation', ['prefix', 'suffix'])
-
-
-def detect_version(callback, path):
-    """Detects the version of a compiler at a given path.
-
-    Args:
-        callback (callable): function that detects the version of
-            the compiler at ``path``
-        path (path): absolute path to search
-
-    Returns:
-        (value, error): if anything is found ``value`` is a ``(version, path)``
-            tuple and ``error`` is None. If ``error`` is not None, ``value``
-            is meaningless and can be discarded.
-    """
-    try:
-        version = callback(path)
-        if version and six.text_type(version).strip():
-            return (version, path), None
-        error = "Couldn't get version for compiler {0}".format(path)
-    except spack.util.executable.ProcessError as e:
-        error = "Couldn't get version for compiler {0}\n".format(path) + \
-                six.text_type(e)
-    except Exception as e:
-        # Catching "Exception" here is fine because it just
-        # means something went wrong running a candidate executable.
-        error = "Error while executing candidate compiler {0}" \
-                "\n{1}: {2}".format(path, e.__class__.__name__,
-                                    six.text_type(e))
-    return None, error
-
-
-def make_compiler_list(tags, compiler_versions):
-    assert len(tags) == len(compiler_versions), \
-        "the two arguments must have the same length"
-
-    compilers_s = []
-    for (compiler_id, name_variation, lang), (return_value, error) \
-            in zip(tags, compiler_versions):
-        # If we had an error, move to the next element
-        if error:
-            try:
-                # This will fail on Python 2.6 if a non ascii
-                # character is in the error
-                tty.debug(error)
-            except UnicodeEncodeError:
-                pass
-            continue
-
-        # Skip unknown versions
-        version, path = return_value
-        if version == 'unknown':
-            continue
-
-        tag = compiler_id._replace(version=version), name_variation, lang
-        compilers_s.append((tag, path))
-
-    compilers_s.sort()
-
-    compilers_d = {}
-    for sort_key, group in itertools.groupby(compilers_s, key=lambda x: x[0]):
-        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] = list(x[1] for x in group).pop()
-
-    # For each unique compiler_id select the name variation with most entries
-    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
-        )
-
-        # Add it to the list of compilers
-        operating_system, compiler_cls, version = compiler_id
-        spec = spack.spec.CompilerSpec(compiler_cls.name, version)
-        paths = [by_compiler_id[selected_name_variation].get(l, None)
-                 for l in ('cc', 'cxx', 'f77', 'fc')]
-        compilers.append(
-            compiler_cls(spec, operating_system, py_platform.machine(), paths)
-        )
-
-    return compilers
-
-
 class CompilerAccessError(spack.error.SpackError):
 
     def __init__(self, path):

lib/spack/spack/compilers/__init__.py

@@ -6,10 +6,17 @@
 """This module contains functions related to finding compilers on the
 system and configuring Spack to use multiple compilers.
 """
+import collections
+import itertools
 import multiprocessing.pool
 import os
 
-from llnl.util.lang import list_modules
+import platform as py_platform
+import six
+
+import llnl.util.lang
+import llnl.util.filesystem as fs
+import llnl.util.tty as tty
 
 import spack.paths
 import spack.error
@@ -17,6 +24,7 @@
 import spack.config
 import spack.architecture
 import spack.util.imp as simp
+from spack.util.environment import get_path
 from spack.util.naming import mod_to_class
 
 _imported_compilers_module = 'spack.compilers'
@@ -177,28 +185,52 @@ def all_compiler_specs(scope=None, init_config=True):
             for s in all_compilers_config(scope, init_config)]
 
 
-def find_compilers(*paths):
+def find_compilers(*path_hints):
-    """List of compilers found in the supplied paths.
+    """Returns the list of compilers found in the paths given as arguments.
-
-    This function invokes the find_compilers() method for each operating
-    system associated with the host platform.
 
     Args:
-        *paths: paths where to search for compilers
+        *path_hints: list of path hints where to look for. If none is
+            given a sensible default based on the ``PATH`` environment
+            variable will be used
 
     Returns:
-        List of compilers found in the supplied paths
+        List of compilers found
     """
-    tags, commands = [], []
+    # Turn the path hints into real paths that are to be searched
-    for o in all_os_classes():
+    path_hints = path_hints or get_path('PATH')
-        t, c = o.search_compiler_commands(*paths)
+    paths = fs.search_paths_for_executables(*path_hints)
-        tags.extend(t), commands.extend(c)
 
+    # To detect the version of the compilers, we dispatch a certain number
+    # of function calls to different workers. Here we construct the list
+    # of arguments for each call.
+    arguments = []
+    for o in all_os_classes():
+        arguments.extend(arguments_to_detect_version_fn(o, *paths))
+
+    # Here we map the function arguments to the corresponding calls
     tp = multiprocessing.pool.ThreadPool()
-    compiler_versions = tp.map(lambda x: x(), commands)
+    detected_versions = tp.map(detect_version, arguments)
     tp.close()
 
-    return spack.compiler.make_compiler_list(tags, compiler_versions)
+    def valid_version(item):
+        value, error = item
+        if error is None:
+            return True
+        try:
+            # This will fail on Python 2.6 if a non ascii
+            # character is in the error
+            tty.debug(error)
+        except UnicodeEncodeError:
+            pass
+        return False
+
+    def remove_errors(item):
+        value, _ = item
+        return value
+
+    return make_compiler_list(
+        map(remove_errors, filter(valid_version, detected_versions))
+    )
 
 
 def supported_compilers():
@@ -207,8 +239,8 @@ def supported_compilers():
        See available_compilers() to get a list of all the available
        versions of supported compilers.
     """
-    return sorted(
+    return sorted(name for name in
-        name for name in list_modules(spack.paths.compilers_path))
+                  llnl.util.lang.list_modules(spack.paths.compilers_path))
 
 
 @_auto_compiler_spec
@@ -369,6 +401,7 @@ def get_compiler_duplicates(compiler_spec, arch_spec):
     return cfg_file_to_duplicates
 
 
+@llnl.util.lang.memoized
 def class_for_compiler_name(compiler_name):
     """Given a compiler module name, get the corresponding Compiler class."""
     assert(supported(compiler_name))
@@ -401,6 +434,177 @@ def all_compiler_types():
     return [class_for_compiler_name(c) for c in supported_compilers()]
 
 
+#: Gathers the attribute values by which a detected compiler is considered
+#: unique in Spack.
+#:
+#:  - os: the operating system
+#:  - compiler_name: the name of the compiler (e.g. 'gcc', 'clang', etc.)
+#:  - version: the version of the compiler
+#:
+CompilerID = collections.namedtuple(
+    'CompilerID', ['os', 'compiler_name', 'version']
+)
+
+#: Variations on a matched compiler name
+NameVariation = collections.namedtuple('NameVariation', ['prefix', 'suffix'])
+
+#: Groups together the arguments needed by `detect_version`. The four entries
+#: in the tuple are:
+#:
+#: - id: An instance of the CompilerID named tuple (version can be set to None
+#:       as it will be detected later)
+#: - variation: a NameVariation for file being tested
+#: - language: compiler language being tested (one of 'cc', 'cxx', 'fc', 'f77')
+#: - path: full path to the executable being tested
+#:
+DetectVersionArgs = collections.namedtuple(
+    'DetectVersionArgs', ['id', 'variation', 'language', 'path']
+)
+
+
+def arguments_to_detect_version_fn(operating_system, *paths):
+    """Returns a list of DetectVersionArgs tuples to be used in a
+    corresponding function to detect compiler versions.
+
+    The ``operating_system`` instance can customize the behavior of this
+    function by providing a method called with the same name.
+
+    Args:
+        operating_system (OperatingSystem): the operating system on which
+            we are looking for compilers
+        *paths: paths to search for compilers
+
+    Returns:
+        List of DetectVersionArgs tuples. Each item in the list will be later
+        mapped to the corresponding function call to detect the version of the
+        compilers in this OS.
+    """
+    def _default(*search_paths):
+        command_arguments = []
+        files_to_be_tested = fs.files_in(*search_paths)
+        for compiler_name in spack.compilers.supported_compilers():
+
+            compiler_cls = class_for_compiler_name(compiler_name)
+
+            for language in ('cc', 'cxx', 'f77', 'fc'):
+
+                # Select only the files matching a regexp
+                for (file, full_path), regexp in itertools.product(
+                        files_to_be_tested,
+                        compiler_cls.search_regexps(language)
+                ):
+                    match = regexp.match(file)
+                    if match:
+                        compiler_id = CompilerID(
+                            operating_system, compiler_name, None
+                        )
+                        detect_version_args = DetectVersionArgs(
+                            id=compiler_id,
+                            variation=NameVariation(*match.groups()),
+                            language=language, path=full_path
+                        )
+                        command_arguments.append(detect_version_args)
+
+        # Reverse it here so that the dict creation (last insert wins)
+        # does not spoil the intended precedence.
+        return reversed(command_arguments)
+
+    fn = getattr(operating_system, 'arguments_to_detect_version_fn', _default)
+    return fn(*paths)
+
+
+def detect_version(detect_version_args):
+    """Computes the version of a compiler and adds it to the information
+    passed as input.
+
+    As this function is meant to be executed by worker processes it won't
+    raise any exception but instead will return a (value, error) tuple that
+    needs to be checked by the code dispatching the calls.
+
+    Args:
+        detect_version_args (DetectVersionArgs): information on the
+            compiler for which we should detect the version.
+
+    Returns:
+        A ``(DetectVersionArgs, error)`` tuple. If ``error`` is ``None`` the
+        version of the compiler was computed correctly and the first argument
+        of the tuple will contain it. Otherwise ``error`` is a string
+        containing an explanation on why the version couldn't be computed.
+    """
+    compiler_id = detect_version_args.id
+    language = detect_version_args.language
+    compiler_cls = class_for_compiler_name(compiler_id.compiler_name)
+    path = detect_version_args.path
+
+    # Get compiler names and the callback to detect their versions
+    callback = getattr(compiler_cls, '{0}_version'.format(language))
+
+    try:
+        version = callback(path)
+        if version and six.text_type(version).strip() and version != 'unknown':
+            value = detect_version_args._replace(
+                id=compiler_id._replace(version=version)
+            )
+            return value, None
+
+        error = "Couldn't get version for compiler {0}".format(path)
+    except spack.util.executable.ProcessError as e:
+        error = "Couldn't get version for compiler {0}\n".format(path) + \
+                six.text_type(e)
+    except Exception as e:
+        # Catching "Exception" here is fine because it just
+        # means something went wrong running a candidate executable.
+        error = "Error while executing candidate compiler {0}" \
+                "\n{1}: {2}".format(path, e.__class__.__name__,
+                                    six.text_type(e))
+    return None, error
+
+
+def make_compiler_list(detected_versions):
+    """Process a list of detected versions and turn them into a list of
+    compiler specs.
+
+    Args:
+        detected_versions (list): list of DetectVersionArgs containing a
+            valid version
+
+    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)
+
+    # 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):
+        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 = []
+    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
+        )
+
+        # Add it to the list of compilers
+        operating_system, compiler_name, version = compiler_id
+        compiler_cls = spack.compilers.class_for_compiler_name(compiler_name)
+        spec = spack.spec.CompilerSpec(compiler_cls.name, version)
+        paths = [by_compiler_id[selected_name_variation].get(l, None)
+                 for l in ('cc', 'cxx', 'f77', 'fc')]
+        compilers.append(
+            compiler_cls(spec, operating_system, py_platform.machine(), paths)
+        )
+
+    return compilers
+
+
 class InvalidCompilerConfigurationError(spack.error.SpackError):
 
     def __init__(self, compiler_spec):

lib/spack/spack/test/compilers.py

@@ -23,7 +23,7 @@
 import spack.compilers.xl_r
 import spack.compilers.fj
 
-from spack.compiler import detect_version, Compiler
+from spack.compiler import Compiler
 
 
 def test_get_compiler_duplicates(config):
@@ -45,15 +45,16 @@ def test_all_compilers(config):
     assert len(filtered) == 1
 
 
-def test_version_detection_is_empty():
+# FIXME: Write better unit tests for this function
-    version = detect_version(lambda x: None, path='/usr/bin/gcc')
+# def test_version_detection_is_empty():
-    expected = (None,  "Couldn't get version for compiler /usr/bin/gcc")
+#     version = detect_version(lambda x: None, path='/usr/bin/gcc')
-    assert version == expected
+#     expected = (None,  "Couldn't get version for compiler /usr/bin/gcc")
-
+#     assert version == expected
-
+#
-def test_version_detection_is_successful():
+#
-    version = detect_version(lambda x: '4.9', path='/usr/bin/gcc')
+# def test_version_detection_is_successful():
-    assert version == (('4.9', '/usr/bin/gcc'), None)
+#     version = detect_version(lambda x: '4.9', path='/usr/bin/gcc')
+#     assert version == (('4.9', '/usr/bin/gcc'), None)
 
 
 def test_compiler_flags_from_config_are_grouped():