Harden compiler detection against errors (#39736)

Fixes #39622

Add a timeout to compiler detection and allow Spack to proceed when
this timeout occurs.

In all cases, the timeout is 120 seconds: it is assumed any compiler
invocation we do for the purposes of verifying it would resolve in
that amount of time.

Also refine executables that are tested as being possible MSVC
instances, and limit where we try to detect MSVC. In more detail:

* Compiler detection should timeout after a certain period of time.
  Because compiler detection executes arbitrary executables on the
  system, we could encounter a program that just hangs, or even a
  compiler that hangs on a license key or similar. A timeout
  prevents this from hanging Spack.
* Prevents things like cl-.* from being detected as potential MSVC
  installs. cl is always just cl in all cases that Spack supports.
  Change the MSVC class to indicate this.
* Prevent compilers unsupported on certain platforms from being
  detected there (i.e. don't look for MSVC on systems other than
  Windows).

The first point alone is sufficient to address #39622, but the
next two reduce the likelihood of timeouts (which is useful since
those slow down the user even if they are survivable).

4 files changed, 106 insertions, 36 deletions

diff --git a/lib/spack/spack/compiler.py b/lib/spack/spack/compiler.py
index 396ec6b2fb..3ec7d1421a 100644
--- a/lib/spack/spack/compiler.py
+++ b/lib/spack/spack/compiler.py
@@ -39,10 +39,17 @@ def _get_compiler_version_output(compiler_path, version_arg, ignore_errors=()):
         version_arg (str): the argument used to extract version information
     """
     compiler = spack.util.executable.Executable(compiler_path)
+    compiler_invocation_args = {
+        "output": str,
+        "error": str,
+        "ignore_errors": ignore_errors,
+        "timeout": 120,
+        "fail_on_error": True,
+    }
     if version_arg:
-        output = compiler(version_arg, output=str, error=str, ignore_errors=ignore_errors)
+        output = compiler(version_arg, **compiler_invocation_args)
     else:
-        output = compiler(output=str, error=str, ignore_errors=ignore_errors)
+        output = compiler(**compiler_invocation_args)
     return output
 
 
@@ -229,6 +236,9 @@ class Compiler:
     # by any compiler
     _all_compiler_rpath_libraries = ["libc", "libc++", "libstdc++"]
 
+    #: Platform matcher for Platform objects supported by compiler
+    is_supported_on_platform = lambda x: True
+
     # Default flags used by a compiler to set an rpath
     @property
     def cc_rpath_arg(self):
@@ -594,8 +604,6 @@ class Compiler:
         compiler_names = getattr(cls, "{0}_names".format(language))
         prefixes = [""] + cls.prefixes
         suffixes = [""]
-        # Windows compilers generally have an extension of some sort
-        # as do most files on Windows, handle that case here
         if sys.platform == "win32":
             ext = r"\.(?:exe|bat)"
             cls_suf = [suf + ext for suf in cls.suffixes]
diff --git a/lib/spack/spack/compilers/__init__.py b/lib/spack/spack/compilers/__init__.py
index 082577ac6c..3f9663d21e 100644
--- a/lib/spack/spack/compilers/__init__.py
+++ b/lib/spack/spack/compilers/__init__.py
@@ -10,7 +10,7 @@ import collections
 import itertools
 import multiprocessing.pool
 import os
-from typing import Dict
+from typing import Dict, List
 
 import archspec.cpu
 
@@ -298,7 +298,7 @@ def select_new_compilers(compilers, scope=None):
     return compilers_not_in_config
 
 
-def supported_compilers():
+def supported_compilers() -> List[str]:
     """Return a set of names of compilers supported by Spack.
 
     See available_compilers() to get a list of all the available
@@ -306,10 +306,41 @@ def supported_compilers():
     """
     # Hack to be able to call the compiler `apple-clang` while still
     # using a valid python name for the module
-    return sorted(
-        name if name != "apple_clang" else "apple-clang"
-        for name in llnl.util.lang.list_modules(spack.paths.compilers_path)
-    )
+    return sorted(all_compiler_names())
+
+
+def supported_compilers_for_host_platform() -> List[str]:
+    """Return a set of compiler class objects supported by Spack
+    that are also supported by the current host platform
+    """
+    host_plat = spack.platforms.real_host()
+    return supported_compilers_for_platform(host_plat)
+
+
+def supported_compilers_for_platform(platform: spack.platforms.Platform) -> List[str]:
+    """Return a set of compiler class objects supported by Spack
+    that are also supported by the provided platform
+
+    Args:
+        platform (str): string representation of platform
+            for which compiler compatability should be determined
+    """
+    return [
+        name
+        for name in supported_compilers()
+        if class_for_compiler_name(name).is_supported_on_platform(platform)
+    ]
+
+
+def all_compiler_names() -> List[str]:
+    def replace_apple_clang(name):
+        return name if name != "apple_clang" else "apple-clang"
+
+    return [replace_apple_clang(name) for name in all_compiler_module_names()]
+
+
+def all_compiler_module_names() -> List[str]:
+    return [name for name in llnl.util.lang.list_modules(spack.paths.compilers_path)]
 
 
 @_auto_compiler_spec
@@ -628,7 +659,7 @@ def arguments_to_detect_version_fn(operating_system, paths):
     def _default(search_paths):
         command_arguments = []
         files_to_be_tested = fs.files_in(*search_paths)
-        for compiler_name in spack.compilers.supported_compilers():
+        for compiler_name in spack.compilers.supported_compilers_for_host_platform():
             compiler_cls = class_for_compiler_name(compiler_name)
 
             for language in ("cc", "cxx", "f77", "fc"):
@@ -687,9 +718,11 @@ def detect_version(detect_version_args):
                 value = fn_args._replace(id=compiler_id._replace(version=version))
                 return value, None
 
-            error = "Couldn't get version for compiler {0}".format(path)
+            error = f"Couldn't get version for compiler {path}".format(path)
         except spack.util.executable.ProcessError as e:
-            error = "Couldn't get version for compiler {0}\n".format(path) + str(e)
+            error = f"Couldn't get version for compiler {path}\n" + str(e)
+        except spack.util.executable.ProcessTimeoutError as e:
+            error = f"Couldn't get version for compiler {path}\n" + str(e)
         except Exception as e:
             # Catching "Exception" here is fine because it just
             # means something went wrong running a candidate executable.
diff --git a/lib/spack/spack/compilers/msvc.py b/lib/spack/spack/compilers/msvc.py
index 7e91e8eca2..27375a804a 100644
--- a/lib/spack/spack/compilers/msvc.py
+++ b/lib/spack/spack/compilers/msvc.py
@@ -154,9 +154,12 @@ class Msvc(Compiler):
 
     #: Regex used to extract version from compiler's output
     version_regex = r"([1-9][0-9]*\.[0-9]*\.[0-9]*)"
+    # The MSVC compiler class overrides this to prevent instances
+    # of erroneous matching on executable names that cannot be msvc
+    # compilers
+    suffixes = []
 
-    # Initialize, deferring to base class but then adding the vcvarsallfile
-    # file based on compiler executable path.
+    is_supported_on_platform = lambda x: isinstance(x, spack.platforms.Windows)
 
     def __init__(self, *args, **kwargs):
         # This positional argument "paths" is later parsed and process by the base class
@@ -167,6 +170,8 @@ class Msvc(Compiler):
         cspec = args[0]
         new_pth = [pth if pth else get_valid_fortran_pth(cspec.version) for pth in paths]
         paths[:] = new_pth
+        # Initialize, deferring to base class but then adding the vcvarsallfile
+        # file based on compiler executable path.
         super().__init__(*args, **kwargs)
         # To use the MSVC compilers, VCVARS must be invoked
         # VCVARS is located at a fixed location, referencable
diff --git a/lib/spack/spack/util/executable.py b/lib/spack/spack/util/executable.py
index a3f3d16c77..48dca0ffa3 100644
--- a/lib/spack/spack/util/executable.py
+++ b/lib/spack/spack/util/executable.py
@@ -100,6 +100,8 @@ class Executable:
                 an exception even if ``fail_on_error`` is set to ``True``
             ignore_quotes (bool): If False, warn users that quotes are not needed
                 as Spack does not use a shell. Defaults to False.
+            timeout (int or float): The number of seconds to wait before killing
+                the child process
             input: Where to read stdin from
             output: Where to send stdout
             error: Where to send stderr
@@ -120,6 +122,29 @@ class Executable:
         By default, the subprocess inherits the parent's file descriptors.
 
         """
+
+        def process_cmd_output(out, err):
+            result = None
+            if output in (str, str.split) or error in (str, str.split):
+                result = ""
+                if output in (str, str.split):
+                    if sys.platform == "win32":
+                        outstr = str(out.decode("ISO-8859-1"))
+                    else:
+                        outstr = str(out.decode("utf-8"))
+                    result += outstr
+                    if output is str.split:
+                        sys.stdout.write(outstr)
+                if error in (str, str.split):
+                    if sys.platform == "win32":
+                        errstr = str(err.decode("ISO-8859-1"))
+                    else:
+                        errstr = str(err.decode("utf-8"))
+                    result += errstr
+                    if error is str.split:
+                        sys.stderr.write(errstr)
+            return result
+
         # Environment
         env_arg = kwargs.get("env", None)
 
@@ -150,6 +175,7 @@ class Executable:
         fail_on_error = kwargs.pop("fail_on_error", True)
         ignore_errors = kwargs.pop("ignore_errors", ())
         ignore_quotes = kwargs.pop("ignore_quotes", False)
+        timeout = kwargs.pop("timeout", None)
 
         # If they just want to ignore one error code, make it a tuple.
         if isinstance(ignore_errors, int):
@@ -196,28 +222,9 @@ class Executable:
             proc = subprocess.Popen(
                 cmd, stdin=istream, stderr=estream, stdout=ostream, env=env, close_fds=False
             )
-            out, err = proc.communicate()
-
-            result = None
-            if output in (str, str.split) or error in (str, str.split):
-                result = ""
-                if output in (str, str.split):
-                    if sys.platform == "win32":
-                        outstr = str(out.decode("ISO-8859-1"))
-                    else:
-                        outstr = str(out.decode("utf-8"))
-                    result += outstr
-                    if output is str.split:
-                        sys.stdout.write(outstr)
-                if error in (str, str.split):
-                    if sys.platform == "win32":
-                        errstr = str(err.decode("ISO-8859-1"))
-                    else:
-                        errstr = str(err.decode("utf-8"))
-                    result += errstr
-                    if error is str.split:
-                        sys.stderr.write(errstr)
+            out, err = proc.communicate(timeout=timeout)
 
+            result = process_cmd_output(out, err)
             rc = self.returncode = proc.returncode
             if fail_on_error and rc != 0 and (rc not in ignore_errors):
                 long_msg = cmd_line_string
@@ -246,6 +253,18 @@ class Executable:
                     "\nExit status %d when invoking command: %s"
                     % (proc.returncode, cmd_line_string),
                 )
+        except subprocess.TimeoutExpired as te:
+            proc.kill()
+            out, err = proc.communicate()
+            result = process_cmd_output(out, err)
+            long_msg = cmd_line_string + f"\n{result}"
+            if fail_on_error:
+                raise ProcessTimeoutError(
+                    f"\nProcess timed out after {timeout}s"
+                    f"We expected the following command to run quickly but\
+it did not, please report this as an issue: {long_msg}",
+                    long_message=long_msg,
+                ) from te
 
         finally:
             if close_ostream:
@@ -344,5 +363,10 @@ class ProcessError(spack.error.SpackError):
     """ProcessErrors are raised when Executables exit with an error code."""
 
 
+class ProcessTimeoutError(ProcessError):
+    """ProcessTimeoutErrors are raised when Executable calls with a
+    specified timeout exceed that time"""
+
+
 class CommandNotFoundError(spack.error.SpackError):
     """Raised when ``which()`` can't find a required executable."""