Throw exceptions more agressively

Also, preserve stack trace from setup.py exceptions

Author: rotu

colcon_core/package_identification/python.py

@@ -4,6 +4,7 @@
 
 import multiprocessing
 import os
+import traceback
 from typing import Optional
 
 from colcon_core.dependency_descriptor import DependencyDescriptor
@@ -32,24 +33,22 @@ def identify(self, desc):  # noqa: D102
         if not setup_py.is_file():
             return
 
-        try:
-            config = get_setup_result(setup_py, env=None)
-        except Exception as e:
-            logger.warning(e)
-            return
+        # after this point, we are convinced this is a Python package,
+        # so we should fail with an Exception instead of silently
 
-        if 'metadata' not in config:
-            return
+        config = get_setup_result(setup_py, env=None)
 
         name = config['metadata'].name
         if not name:
-            return
+            raise RuntimeError(
+                'Setup script "{}" returned invalid name {!r}'
+                .format(setup_py, name))
 
         desc.type = 'python'
         if desc.name is not None and desc.name != name:
-            msg = 'Package name already set to different value'
-            logger.error(msg)
-            raise RuntimeError(msg)
+            raise RuntimeError(
+                'Setup script "{}" returned different name {!r} != {!r}'
+                .format(setup_py, name, desc.name))
         desc.name = name
 
         desc.metadata['version'] = config['metadata'].version
@@ -79,29 +78,25 @@ def get_setup_result(setup_py, *, env: Optional[dict]):
 
     :param setup_py: Path to a setup.py script
     :param env: Environment variables to set before running setup.py
-    :return: None or Dictionary of data describing the package.
+    :return: Dictionary of data describing the package.
+    :raise: RuntimeError if the setup script encountered an error
     """
     conn_recv, conn_send = multiprocessing.Pipe(duplex=False)
-    p = multiprocessing.Process(
-        target=_get_setup_result_target,
-        args=(os.path.abspath(str(setup_py)), env, conn_send)
-    )
-    p.start()
-    p.join()
-
-    if not conn_recv.poll():
-        raise RuntimeError(
-            'Python setup script "{}" did not return any data'
-            .format(setup_py))
-
-    result_or_error = conn_recv.recv()
-    conn_recv.close()
-
-    if isinstance(result_or_error, dict):
-        return result_or_error
+    with conn_send:
+        p = multiprocessing.Process(
+            target=_get_setup_result_target,
+            args=(os.path.abspath(str(setup_py)), env, conn_send)
+        )
+        p.start()
+        p.join()
+    with conn_recv:
+        result_or_exc_info = conn_recv.recv()
+
+    if isinstance(result_or_exc_info, dict):
+        return result_or_exc_info
     raise RuntimeError(
-        'Python setup script "{}" failed: {!r}'
-        .format(setup_py, result_or_error)) from result_or_error
+        'Failure when trying to run setup script {}:\n{}'
+        .format(setup_py, traceback.format_exception(*result_or_exc_info)))
 
 
 def _get_setup_result_target(setup_py: str, env: Optional[dict], conn_send):
@@ -113,12 +108,12 @@ def _get_setup_result_target(setup_py: str, env: Optional[dict], conn_send):
 
     :param setup_py: Absolute path to a setup.py script
     :param env: Environment variables to set before running setup.py
-    :param conn_send: Connection to send the result as either a dict or an
-      Exception.
+    :param conn_send: Connection to send the result as either a dict or the
+        result of sys.exc_info
     """
+    import distutils.core
+    import sys
     try:
-        import distutils.core
-
         # need to be in setup.py's parent dir to detect any setup.cfg
         os.chdir(os.path.dirname(setup_py))
 
@@ -144,10 +139,9 @@ def _get_setup_result_target(setup_py: str, env: Optional[dict], conn_send):
                 # Private properties
                 and not attr.startswith('_')
             )})
-    except Exception as e:
-        conn_send.send(e)
-    finally:
-        conn_send.close()
+    except BaseException:
+        conn_send.send(sys.exc_info())
+        raise
 
 
 def create_dependency_descriptor(requirement_string):

test/test_package_identification_python.py

@@ -28,12 +28,14 @@ def test_identify():
 
         basepath = Path(basepath)
         (basepath / 'setup.py').write_text('')
-        assert extension.identify(desc) is None
+        with pytest.raises(RuntimeError):
+            extension.identify(desc)
         assert desc.name is None
         assert desc.type is None
 
         (basepath / 'setup.cfg').write_text('')
-        assert extension.identify(desc) is None
+        with pytest.raises(RuntimeError):
+            extension.identify(desc)
         assert desc.name is None
         assert desc.type is None
 
@@ -50,8 +52,8 @@ def test_identify():
         desc.name = 'other-name'
         with pytest.raises(RuntimeError) as e:
             extension.identify(desc)
-        assert str(e.value).endswith(
-            'Package name already set to different value')
+        assert 'different name' in str(e.value)
+        assert desc.name == 'other-name'
 
         (basepath / 'setup.cfg').write_text(
             '[metadata]\n'