Introduce GroupedExceptionHandler and use it to simplify bootstrap error handling (#30192)

lib/spack/llnl/util/lang.py

@@ -11,7 +11,9 @@
 import os
 import re
 import sys
+import traceback
 from datetime import datetime, timedelta
+from typing import List, Tuple
 
 import six
 from six import string_types
@@ -1009,3 +1011,64 @@ def __repr__(self):
 
     def __str__(self):
         return str(self.data)
+
+
+class GroupedExceptionHandler(object):
+    """A generic mechanism to coalesce multiple exceptions and preserve tracebacks."""
+
+    def __init__(self):
+        self.exceptions = []    # type: List[Tuple[str, Exception, List[str]]]
+
+    def __bool__(self):
+        """Whether any exceptions were handled."""
+        return bool(self.exceptions)
+
+    def forward(self, context):
+        # type: (str) -> GroupedExceptionForwarder
+        """Return a contextmanager which extracts tracebacks and prefixes a message."""
+        return GroupedExceptionForwarder(context, self)
+
+    def _receive_forwarded(self, context, exc, tb):
+        # type: (str, Exception, List[str]) -> None
+        self.exceptions.append((context, exc, tb))
+
+    def grouped_message(self, with_tracebacks=True):
+        # type: (bool) -> str
+        """Print out an error message coalescing all the forwarded errors."""
+        each_exception_message = [
+            '{0} raised {1}: {2}{3}'.format(
+                context,
+                exc.__class__.__name__,
+                exc,
+                '\n{0}'.format(''.join(tb)) if with_tracebacks else '',
+            )
+            for context, exc, tb in self.exceptions
+        ]
+        return 'due to the following failures:\n{0}'.format(
+            '\n'.join(each_exception_message)
+        )
+
+
+class GroupedExceptionForwarder(object):
+    """A contextmanager to capture exceptions and forward them to a
+    GroupedExceptionHandler."""
+
+    def __init__(self, context, handler):
+        # type: (str, GroupedExceptionHandler) -> None
+        self._context = context
+        self._handler = handler
+
+    def __enter__(self):
+        return None
+
+    def __exit__(self, exc_type, exc_value, tb):
+        if exc_value is not None:
+            self._handler._receive_forwarded(
+                self._context,
+                exc_value,
+                traceback.format_tb(tb),
+            )
+
+        # Suppress any exception from being re-raised:
+        # https://docs.python.org/3/reference/datamodel.html#object.__exit__.
+        return True

lib/spack/spack/bootstrap.py

@@ -21,6 +21,7 @@
 
 import llnl.util.filesystem as fs
 import llnl.util.tty as tty
+from llnl.util.lang import GroupedExceptionHandler
 
 import spack.binary_distribution
 import spack.config
@@ -417,11 +418,10 @@ def _make_bootstrapper(conf):
     return _bootstrap_methods[btype](conf)
 
 
-def _source_is_trusted(conf):
+def _validate_source_is_trusted(conf):
     trusted, name = spack.config.get('bootstrap:trusted'), conf['name']
     if name not in trusted:
-        return False
-    return trusted[name]
+        raise ValueError('source is not trusted')
 
 
 def spec_for_current_python():
@@ -488,34 +488,25 @@ def ensure_module_importable_or_raise(module, abstract_spec=None):
     abstract_spec = abstract_spec or module
     source_configs = spack.config.get('bootstrap:sources', [])
 
-    errors = {}
+    h = GroupedExceptionHandler()
 
     for current_config in source_configs:
-        if not _source_is_trusted(current_config):
-            msg = ('[BOOTSTRAP MODULE {0}] Skipping source "{1}" since it is '
-                   'not trusted').format(module, current_config['name'])
-            tty.debug(msg)
-            continue
+        with h.forward(current_config['name']):
+            _validate_source_is_trusted(current_config)
 
             b = _make_bootstrapper(current_config)
-        try:
             if b.try_import(module, abstract_spec):
                 return
-        except Exception as e:
-            msg = '[BOOTSTRAP MODULE {0}] Unexpected error "{1}"'
-            tty.debug(msg.format(module, str(e)))
-            errors[current_config['name']] = e
 
-    # We couldn't import in any way, so raise an import error
-    msg = 'cannot bootstrap the "{0}" Python module'.format(module)
+    assert h, 'expected at least one exception to have been raised at this point: while bootstrapping {0}'.format(module)  # noqa: E501
+    msg = 'cannot bootstrap the "{0}" Python module '.format(module)
     if abstract_spec:
-        msg += ' from spec "{0}"'.format(abstract_spec)
-    msg += ' due to the following failures:\n'
-    for method in errors:
-        err = errors[method]
-        msg += "    '{0}' raised {1}: {2}\n".format(
-            method, err.__class__.__name__, str(err))
-    msg += '    Please run `spack -d spec zlib` for more verbose error messages'
+        msg += 'from spec "{0}" '.format(abstract_spec)
+    if tty.is_debug():
+        msg += h.grouped_message(with_tracebacks=True)
+    else:
+        msg += h.grouped_message(with_tracebacks=False)
+        msg += '\nRun `spack --debug ...` for more detailed errors'
     raise ImportError(msg)
 
 
@@ -539,15 +530,14 @@ def ensure_executables_in_path_or_raise(executables, abstract_spec):
 
     executables_str = ', '.join(executables)
     source_configs = spack.config.get('bootstrap:sources', [])
+
+    h = GroupedExceptionHandler()
+
     for current_config in source_configs:
-        if not _source_is_trusted(current_config):
-            msg = ('[BOOTSTRAP EXECUTABLES {0}] Skipping source "{1}" since it is '
-                   'not trusted').format(executables_str, current_config['name'])
-            tty.debug(msg)
-            continue
+        with h.forward(current_config['name']):
+            _validate_source_is_trusted(current_config)
 
             b = _make_bootstrapper(current_config)
-        try:
             if b.try_search_path(executables, abstract_spec):
                 # Additional environment variables needed
                 concrete_spec, cmd = b.last_search['spec'], b.last_search['command']
@@ -562,14 +552,16 @@ def ensure_executables_in_path_or_raise(executables, abstract_spec):
                     )
                 cmd.add_default_envmod(env_mods)
                 return cmd
-        except Exception as e:
-            msg = '[BOOTSTRAP EXECUTABLES {0}] Unexpected error "{1}"'
-            tty.debug(msg.format(executables_str, str(e)))
 
-    # We couldn't import in any way, so raise an import error
-    msg = 'cannot bootstrap any of the {0} executables'.format(executables_str)
+    assert h, 'expected at least one exception to have been raised at this point: while bootstrapping {0}'.format(executables_str)  # noqa: E501
+    msg = 'cannot bootstrap any of the {0} executables '.format(executables_str)
     if abstract_spec:
-        msg += ' from spec "{0}"'.format(abstract_spec)
+        msg += 'from spec "{0}" '.format(abstract_spec)
+    if tty.is_debug():
+        msg += h.grouped_message(with_tracebacks=True)
+    else:
+        msg += h.grouped_message(with_tracebacks=False)
+        msg += '\nRun `spack --debug ...` for more detailed errors'
     raise RuntimeError(msg)
 
 

lib/spack/spack/test/bootstrap.py

@@ -62,6 +62,22 @@ def test_raising_exception_if_bootstrap_disabled(mutable_config):
         spack.bootstrap.store_path()
 
 
+def test_raising_exception_module_importable():
+    with pytest.raises(
+        ImportError,
+        match='cannot bootstrap the "asdf" Python module',
+    ):
+        spack.bootstrap.ensure_module_importable_or_raise("asdf")
+
+
+def test_raising_exception_executables_in_path():
+    with pytest.raises(
+        RuntimeError,
+        match="cannot bootstrap any of the asdf, fdsa executables",
+    ):
+        spack.bootstrap.ensure_executables_in_path_or_raise(["asdf", "fdsa"], "python")
+
+
 @pytest.mark.regression('25603')
 def test_bootstrap_deactivates_environments(active_mock_environment):
     assert spack.environment.active_environment() == active_mock_environment

lib/spack/spack/test/llnl/util/lang.py

@@ -6,6 +6,7 @@
 import os.path
 import sys
 from datetime import datetime, timedelta
+from textwrap import dedent
 
 import pytest
 
@@ -270,3 +271,37 @@ def f(*args, **kwargs):
 def test_dedupe():
     assert [x for x in dedupe([1, 2, 1, 3, 2])] == [1, 2, 3]
     assert [x for x in dedupe([1, -2, 1, 3, 2], key=abs)] == [1, -2, 3]
+
+
+def test_grouped_exception():
+    h = llnl.util.lang.GroupedExceptionHandler()
+
+    def inner():
+        raise ValueError('wow!')
+
+    with h.forward('inner method'):
+        inner()
+
+    with h.forward('top-level'):
+        raise TypeError('ok')
+
+    assert h.grouped_message(with_tracebacks=False) == dedent("""\
+    due to the following failures:
+    inner method raised ValueError: wow!
+    top-level raised TypeError: ok""")
+
+    assert h.grouped_message(with_tracebacks=True) == dedent("""\
+    due to the following failures:
+    inner method raised ValueError: wow!
+      File "{0}", \
+line 283, in test_grouped_exception
+        inner()
+      File "{0}", \
+line 280, in inner
+        raise ValueError('wow!')
+
+    top-level raised TypeError: ok
+      File "{0}", \
+line 286, in test_grouped_exception
+        raise TypeError('ok')
+    """).format(__file__)