From 5d17a59c66ff35df1ae0b9c129583e170ad67017 Mon Sep 17 00:00:00 2001
From: Dan Rose <dan@digilabs.io>
Date: Fri, 13 Sep 2019 13:04:34 -0500
Subject: [PATCH 1/7] Rework Package identification tests

---
 test/spell_check.words                     |   1 +
 test/test_package_identification_python.py | 247 +++++++++++++++------
 2 files changed, 184 insertions(+), 64 deletions(-)

diff --git a/test/spell_check.words b/test/spell_check.words
index 34a6ab52..cb8f4556 100644
--- a/test/spell_check.words
+++ b/test/spell_check.words
@@ -16,6 +16,7 @@ deps
 descs
 getpid
 iterdir
+localhost
 lstrip
 mkdtemp
 nargs
diff --git a/test/test_package_identification_python.py b/test/test_package_identification_python.py
index 4447dbae..254daf8f 100644
--- a/test/test_package_identification_python.py
+++ b/test/test_package_identification_python.py
@@ -1,8 +1,7 @@
 # Copyright 2016-2018 Dirk Thomas
+# Copyright 2019 Rover Robotics
 # Licensed under the Apache License, Version 2.0
-
-from pathlib import Path
-from tempfile import TemporaryDirectory
+import re
 
 from colcon_core.package_descriptor import PackageDescriptor
 from colcon_core.package_identification.python \
@@ -12,69 +11,189 @@
 import pytest
 
 
-def test_identify():
+@pytest.fixture
+def package_descriptor(tmp_path):
+    """Create package descriptor and fail the test if its path changes."""
+    desc = PackageDescriptor(tmp_path)
+    yield desc
+    assert str(desc.path) == str(tmp_path)
+
+
+@pytest.fixture
+def unchanged_empty_descriptor(package_descriptor):
+    """Create package descriptor and fail the test if it changes."""
+    yield package_descriptor
+    assert package_descriptor.name is None
+    assert package_descriptor.type is None
+
+
+@pytest.mark.xfail
+def test_error_in_setup_py(unchanged_empty_descriptor):
+    setup_py = unchanged_empty_descriptor.path / 'setup.py'
+    error_text = 'My hovercraft is full of eels'
+    setup_py.write_text('raise OverflowError({!r})'.format(error_text))
+
+    extension = PythonPackageIdentification()
+    with pytest.raises(RuntimeError) as e:
+        extension.identify(unchanged_empty_descriptor)
+
+    assert e.match('Failure when trying to run setup script')
+    assert e.match(re.escape(str(setup_py)))
+
+    # details of the root cause should be in error string
+    assert e.match('OverflowError')
+    assert e.match(error_text)
+
+
+def test_missing_setup_py(unchanged_empty_descriptor):
+    extension = PythonPackageIdentification()
+    # should not raise
+    extension.identify(unchanged_empty_descriptor)
+
+
+@pytest.mark.xfail
+def test_empty_setup_py(unchanged_empty_descriptor):
+    extension = PythonPackageIdentification()
+    (unchanged_empty_descriptor.path / 'setup.py').write_text('')
+    with pytest.raises(RuntimeError) as e:
+        extension.identify(unchanged_empty_descriptor)
+    assert e.match('not a Distutils setup script')
+
+
+def test_re_identify_if_non_python_package(package_descriptor):
+    package_descriptor.name = 'other-package'
+    package_descriptor.type = 'other'
+    extension = PythonPackageIdentification()
+    extension.identify(package_descriptor)
+    assert package_descriptor.name == 'other-package'
+    assert package_descriptor.type == 'other'
+
+
+def test_re_identify_python_if_same_python_package(package_descriptor):
+    package_descriptor.name = 'my-package'
+    package_descriptor.type = 'python'
+
+    extension = PythonPackageIdentification()
+    (package_descriptor.path / 'setup.py').write_text(
+        'import setuptools; setuptools.setup(name="my-package")')
+
+    extension.identify(package_descriptor)
+    assert package_descriptor.name == 'my-package'
+    assert package_descriptor.type == 'python'
+
+
+@pytest.mark.xfail
+def test_re_identify_python_if_different_python_package(package_descriptor):
+    package_descriptor.name = 'other-package'
+    package_descriptor.type = 'python'
+
+    extension = PythonPackageIdentification()
+    (package_descriptor.path / 'setup.py').write_text(
+        'import setuptools; setuptools.setup(name="my-package")')
+
+    with pytest.raises(RuntimeError):
+        extension.identify(package_descriptor)
+
+    assert package_descriptor.name == 'other-package'
+    assert package_descriptor.type == 'python'
+
+
+def test_minimal_cfg(package_descriptor):
+    extension = PythonPackageIdentification()
+
+    (package_descriptor.path / 'setup.py').write_text(
+        'import setuptools; setuptools.setup()')
+    (package_descriptor.path / 'setup.cfg').write_text(
+        '[metadata]\nname = pkg-name')
+
+    extension.identify(package_descriptor)
+
+    # descriptor should be unchanged
+    assert package_descriptor.name == 'pkg-name'
+    assert package_descriptor.type == 'python'
+
+
+def test_requires(package_descriptor):
+    extension = PythonPackageIdentification()
+
+    (package_descriptor.path / 'setup.py').write_text(
+        'import setuptools; setuptools.setup()')
+
+    (package_descriptor.path / 'setup.cfg').write_text(
+        '[metadata]\n'
+        'name = pkg-name\n'
+        '[options]\n'
+        'setup_requires =\n'
+        '  setuptools; sys_platform != "imaginary_platform"\n'
+        '  imaginary-package; sys_platform == "imaginary_platform"\n'
+        'install_requires =\n'
+        '  runA > 1.2.3\n'
+        '  runB\n'
+        'tests_require = test == 2.0.0\n'
+        # prevent trying to look for setup_requires in the Package Index
+        '[easy_install]\n'
+        'allow_hosts = localhost\n')
+
+    extension.identify(package_descriptor)
+    assert package_descriptor.name == 'pkg-name'
+    assert package_descriptor.type == 'python'
+    assert package_descriptor.dependencies.keys() == {'build', 'run', 'test'}
+    assert package_descriptor.dependencies == {
+        'build': {'setuptools', 'imaginary-package'},
+        'run':   {'runA', 'runB'},
+        'test':  {'test'}
+    }
+    for dep in package_descriptor.dependencies['run']:
+        if dep == 'runA':
+            assert dep.metadata['version_gt'] == '1.2.3'
+
+    assert package_descriptor.dependencies['run']
+    assert package_descriptor.dependencies['run'] == {'runA', 'runB'}
+
+
+def test_metadata_options(package_descriptor):
+    (package_descriptor.path / 'setup.py').write_text(
+        'import setuptools; setuptools.setup()')
+
+    (package_descriptor.path / 'setup.cfg').write_text(
+        '[metadata]\n'
+        'name = pkg-name\n'
+        '[options]\n'
+        'zip_safe = false\n'
+        'packages = find:\n')
+
+    (package_descriptor.path / 'my_module').mkdir()
+    (package_descriptor.path / 'my_module' / '__init__.py').touch()
+
+    extension = PythonPackageIdentification()
+    extension.identify(package_descriptor)
+
+    options = package_descriptor.metadata['get_python_setup_options'](None)
+    assert options['zip_safe'] is False
+    assert options['packages'] == ['my_module']
+
+
+@pytest.mark.xfail
+def test_metadata_options_dynamic(package_descriptor):
+    (package_descriptor.path / 'setup.py').write_text(
+        'import setuptools; setuptools.setup()')
+    (package_descriptor.path / 'version_helper.py').write_text(
+        'import os; version = os.environ["version"]'
+    )
+
+    (package_descriptor.path / 'setup.cfg').write_text(
+        '[metadata]\n'
+        'name = my-package\n'
+        'version = attr: version_helper.version\n'
+    )
+
     extension = PythonPackageIdentification()
+    extension.identify(package_descriptor)
 
-    with TemporaryDirectory(prefix='test_colcon_') as basepath:
-        desc = PackageDescriptor(basepath)
-        desc.type = 'other'
-        assert extension.identify(desc) is None
-        assert desc.name is None
-
-        desc.type = None
-        assert extension.identify(desc) is None
-        assert desc.name is None
-        assert desc.type is None
-
-        basepath = Path(basepath)
-        (basepath / 'setup.py').write_text('')
-        assert extension.identify(desc) is None
-        assert desc.name is None
-        assert desc.type is None
-
-        (basepath / 'setup.cfg').write_text('')
-        assert extension.identify(desc) is None
-        assert desc.name is None
-        assert desc.type is None
-
-        (basepath / 'setup.cfg').write_text(
-            '[metadata]\n'
-            'name = pkg-name\n')
-        assert extension.identify(desc) is None
-        assert desc.name == 'pkg-name'
-        assert desc.type == 'python'
-
-        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')
-
-        (basepath / 'setup.cfg').write_text(
-            '[metadata]\n'
-            'name = other-name\n'
-            '[options]\n'
-            'setup_requires =\n'
-            "  build; sys_platform != 'win32'\n"
-            "  build-windows; sys_platform == 'win32'\n"
-            'install_requires =\n'
-            '  runA > 1.2.3\n'
-            '  runB\n'
-            'tests_require = test == 2.0.0\n'
-            'zip_safe = false\n')
-        assert extension.identify(desc) is None
-        assert desc.name == 'other-name'
-        assert desc.type == 'python'
-        assert set(desc.dependencies.keys()) == {'build', 'run', 'test'}
-        assert desc.dependencies['build'] == {'build', 'build-windows'}
-        assert desc.dependencies['run'] == {'runA', 'runB'}
-        dep = next(x for x in desc.dependencies['run'] if x == 'runA')
-        assert dep.metadata['version_gt'] == '1.2.3'
-        assert desc.dependencies['test'] == {'test'}
-
-        assert callable(desc.metadata['get_python_setup_options'])
-        options = desc.metadata['get_python_setup_options'](None)
-        assert 'zip_safe' in options
+    for version in ('1.0', '1.1'):
+        options = package_descriptor.metadata['get_python_setup_options'](
+            {'version': version})
+        assert options['metadata'].version == version
 
 
 def test_create_dependency_descriptor():

From dd03ac4d4aaa0126f06a76c09823429aa585e4de Mon Sep 17 00:00:00 2001
From: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Date: Fri, 13 Sep 2019 13:33:36 -0700
Subject: [PATCH 2/7] add xfail to known word list

---
 test/spell_check.words | 1 +
 1 file changed, 1 insertion(+)

diff --git a/test/spell_check.words b/test/spell_check.words
index cb8f4556..5febe34b 100644
--- a/test/spell_check.words
+++ b/test/spell_check.words
@@ -52,3 +52,4 @@ tuples
 unlinking
 veyor
 wildcards
+xfail

From a9f8179e1879785231b95a5a4b6b1c08b1019af5 Mon Sep 17 00:00:00 2001
From: Dan Rose <dan@digilabs.io>
Date: Thu, 19 Sep 2019 15:46:56 -0500
Subject: [PATCH 3/7] Add a test for setup.py with no name

---
 test/test_package_identification_python.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/test/test_package_identification_python.py b/test/test_package_identification_python.py
index 254daf8f..3acdaa66 100644
--- a/test/test_package_identification_python.py
+++ b/test/test_package_identification_python.py
@@ -60,6 +60,15 @@ def test_empty_setup_py(unchanged_empty_descriptor):
     assert e.match('not a Distutils setup script')
 
 
+@pytest.mark.xfail
+def test_setup_py_no_name(unchanged_empty_descriptor):
+    extension = PythonPackageIdentification()
+    (unchanged_empty_descriptor.path / 'setup.py').write_text(
+        'import setuptools; setuptools.setup(name="")')
+    with pytest.raises(RuntimeError):
+        extension.identify(unchanged_empty_descriptor)
+
+
 def test_re_identify_if_non_python_package(package_descriptor):
     package_descriptor.name = 'other-package'
     package_descriptor.type = 'other'

From 70c5c4eab99ba3cafd0de84c6eb06a5d5f2f03ea Mon Sep 17 00:00:00 2001
From: Dan Rose <rotu@users.noreply.github.com>
Date: Fri, 6 Sep 2019 23:22:53 -0500
Subject: [PATCH 4/7] Better package identification

Previously, we either monkey-patched setuptools and harvested the arguments passed to setuptools.setup or we parsed setup.cfg
Now we run the setup.py script with distutils.core.run_setup to return properties from the resulting Distribution object.
---
 colcon_core/package_identification/python.py | 163 +++++++++++++------
 colcon_core/task/python/build.py             |  12 +-
 colcon_core/task/python/test/__init__.py     |   2 +-
 test/test_package_identification_python.py   |   4 -
 4 files changed, 118 insertions(+), 63 deletions(-)

diff --git a/colcon_core/package_identification/python.py b/colcon_core/package_identification/python.py
index e34e8002..e9adcc16 100644
--- a/colcon_core/package_identification/python.py
+++ b/colcon_core/package_identification/python.py
@@ -1,6 +1,12 @@
 # Copyright 2016-2019 Dirk Thomas
+# Copyright 2019 Rover Robotics
 # Licensed under the Apache License, Version 2.0
 
+import multiprocessing
+import os
+from typing import Optional
+import warnings
+
 from colcon_core.dependency_descriptor import DependencyDescriptor
 from colcon_core.package_identification import logger
 from colcon_core.package_identification \
@@ -8,24 +14,10 @@
 from colcon_core.plugin_system import satisfies_version
 from distlib.util import parse_requirement
 from distlib.version import NormalizedVersion
-try:
-    from setuptools.config import read_configuration
-except ImportError as e:
-    from pkg_resources import get_distribution
-    from pkg_resources import parse_version
-    setuptools_version = get_distribution('setuptools').version
-    minimum_version = '30.3.0'
-    if parse_version(setuptools_version) < parse_version(minimum_version):
-        e.msg += ', ' \
-            "'setuptools' needs to be at least version {minimum_version}, if" \
-            ' a newer version is not available from the package manager use ' \
-            "'pip3 install -U setuptools' to update to the latest version" \
-            .format_map(locals())
-    raise
 
 
 class PythonPackageIdentification(PackageIdentificationExtensionPoint):
-    """Identify Python packages with `setup.cfg` files."""
+    """Identify Python packages with `setup.py` and opt. `setup.cfg` files."""
 
     def __init__(self):  # noqa: D107
         super().__init__()
@@ -41,69 +33,136 @@ def identify(self, desc):  # noqa: D102
         if not setup_py.is_file():
             return
 
-        setup_cfg = desc.path / 'setup.cfg'
-        if not setup_cfg.is_file():
-            return
+        # after this point, we are convinced this is a Python package,
+        # so we should fail with an Exception instead of silently
+
+        config = get_setup_result(setup_py, env=None)
 
-        config = get_configuration(setup_cfg)
-        name = config.get('metadata', {}).get('name')
+        name = config['metadata'].name
         if not name:
-            return
+            raise RuntimeError(
+                "The Python package in '{setup_py.parent}' has an invalid "
+                'package name'.format_map(locals()))
 
         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(
+                "The Python package in '{setup_py.parent}' has the name "
+                "'{name}' which is different from the already set package "
+                "name '{desc.name}'".format_map(locals()))
         desc.name = name
 
-        version = config.get('metadata', {}).get('version')
-        desc.metadata['version'] = version
+        desc.metadata['version'] = config['metadata'].version
 
-        options = config.get('options', {})
-        dependencies = extract_dependencies(options)
-        for k, v in dependencies.items():
-            desc.dependencies[k] |= v
+        for dependency_type, option_name in [
+            ('build', 'setup_requires'),
+            ('run', 'install_requires'),
+            ('test', 'tests_require')
+        ]:
+            desc.dependencies[dependency_type] = {
+                create_dependency_descriptor(d)
+                for d in config[option_name] or ()}
 
         def getter(env):
-            nonlocal options
-            return options
+            nonlocal setup_py
+            return get_setup_result(setup_py, env=env)
 
         desc.metadata['get_python_setup_options'] = getter
 
 
 def get_configuration(setup_cfg):
     """
-    Read the setup.cfg file.
+    Return the configuration values defined in the setup.cfg file.
+
+    The function exists for backward compatibility with older versions of
+    colcon-ros.
 
     :param setup_cfg: The path of the setup.cfg file
     :returns: The configuration data
     :rtype: dict
     """
-    return read_configuration(str(setup_cfg))
+    warnings.warn(
+        'colcon_core.package_identification.python.get_configuration() will '
+        'be removed in the future', DeprecationWarning, stacklevel=2)
+    config = get_setup_result(setup_cfg.parent / 'setup.py', env=None)
+    return {
+        'metadata': {'name': config['metadata'].name},
+        'options': config
+    }
 
 
-def extract_dependencies(options):
+def get_setup_result(setup_py, *, env: Optional[dict]):
     """
-    Get the dependencies of the package.
+    Spin up a subprocess to run setup.py, with the given environment.
 
-    :param options: The dictionary from the options section of the setup.cfg
-      file
-    :returns: The dependencies
-    :rtype: dict(string, set(DependencyDescriptor))
+    :param setup_py: Path to a setup.py script
+    :param env: Environment variables to set before running setup.py
+    :return: Dictionary of data describing the package.
+    :raise: RuntimeError if the setup script encountered an error
     """
-    mapping = {
-        'setup_requires': 'build',
-        'install_requires': 'run',
-        'tests_require': 'test',
-    }
-    dependencies = {}
-    for option_name, dependency_type in mapping.items():
-        dependencies[dependency_type] = set()
-        for dep in options.get(option_name, []):
-            dependencies[dependency_type].add(
-                create_dependency_descriptor(dep))
-    return dependencies
+    env_copy = os.environ.copy()
+    if env is not None:
+        env_copy.update(env)
+
+    conn_recv, conn_send = multiprocessing.Pipe(duplex=False)
+    with conn_send:
+        p = multiprocessing.Process(
+            target=_get_setup_result_target,
+            args=(os.path.abspath(str(setup_py)), env_copy, conn_send),
+        )
+        p.start()
+        p.join()
+    with conn_recv:
+        result_or_exception_string = conn_recv.recv()
+
+    if isinstance(result_or_exception_string, dict):
+        return result_or_exception_string
+    raise RuntimeError(
+        'Failure when trying to run setup script {}:\n{}'
+        .format(setup_py, result_or_exception_string))
+
+
+def _get_setup_result_target(setup_py: str, env: dict, conn_send):
+    """
+    Run setup.py in a modified environment.
+
+    Helper function for get_setup_metadata. The resulting dict or error
+    will be sent via conn_send instead of returned or thrown.
+
+    :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
+        error string
+    """
+    import distutils.core
+    import traceback
+    try:
+        # need to be in setup.py's parent dir to detect any setup.cfg
+        os.chdir(os.path.dirname(setup_py))
+
+        os.environ.clear()
+        os.environ.update(env)
+
+        result = distutils.core.run_setup(
+            str(setup_py), ('--dry-run',), stop_after='config')
+
+        # could just return all attrs in result.__dict__, but we take this
+        # opportunity to filter a few things that don't need to be there
+        conn_send.send({
+            attr: value for attr, value in result.__dict__.items()
+            if (
+                # These *seem* useful but always have the value 0.
+                # Look for their values in the 'metadata' object instead.
+                attr not in result.display_option_names
+                # Getter methods
+                and not callable(value)
+                # Private properties
+                and not attr.startswith('_')
+                # Objects that are generally not picklable
+                and attr not in ('cmdclass', 'distclass', 'ext_modules')
+            )})
+    except BaseException:
+        conn_send.send(traceback.format_exc())
 
 
 def create_dependency_descriptor(requirement_string):
diff --git a/colcon_core/task/python/build.py b/colcon_core/task/python/build.py
index cd3ed3ec..9a8b5575 100644
--- a/colcon_core/task/python/build.py
+++ b/colcon_core/task/python/build.py
@@ -97,7 +97,7 @@ async def build(self, *, additional_hooks=None):  # noqa: D102
                 '--build-directory', os.path.join(args.build_base, 'build'),
                 '--no-deps',
             ]
-            if setup_py_data.get('data_files', []):
+            if setup_py_data.get('data_files'):
                 cmd += ['install_data', '--install-dir', args.install_base]
             self._append_install_layout(args, cmd)
             rc = await check_call(
@@ -142,7 +142,7 @@ def _undo_install(self, pkg, args, setup_py_data, python_lib):
         with open(install_log, 'r') as h:
             lines = [l.rstrip() for l in h.readlines()]
 
-        packages = setup_py_data.get('packages', [])
+        packages = setup_py_data.get('packages') or []
         for module_name in packages:
             if module_name in sys.modules:
                 logger.warning(
@@ -185,8 +185,8 @@ def _symlinks_in_build(self, args, setup_py_data):
         if os.path.exists(os.path.join(args.path, 'setup.cfg')):
             items.append('setup.cfg')
         # add all first level packages
-        package_dir = setup_py_data.get('package_dir', {})
-        for package in setup_py_data.get('packages', []):
+        package_dir = setup_py_data.get('package_dir') or {}
+        for package in setup_py_data.get('packages') or []:
             if '.' in package:
                 continue
             if package in package_dir:
@@ -197,7 +197,7 @@ def _symlinks_in_build(self, args, setup_py_data):
                 items.append(package)
         # relative python-ish paths are allowed as entries in py_modules, see:
         # https://docs.python.org/3.5/distutils/setupscript.html#listing-individual-modules
-        py_modules = setup_py_data.get('py_modules')
+        py_modules = setup_py_data.get('py_modules') or []
         if py_modules:
             py_modules_list = [
                 p.replace('.', os.path.sep) + '.py' for p in py_modules]
@@ -208,7 +208,7 @@ def _symlinks_in_build(self, args, setup_py_data):
                         .format_map(locals()))
             items += py_modules_list
         data_files = get_data_files_mapping(
-            setup_py_data.get('data_files', []))
+            setup_py_data.get('data_files') or [])
         for source in data_files.keys():
             # work around data files incorrectly defined as not relative
             if os.path.isabs(source):
diff --git a/colcon_core/task/python/test/__init__.py b/colcon_core/task/python/test/__init__.py
index bd189e0e..fd440b04 100644
--- a/colcon_core/task/python/test/__init__.py
+++ b/colcon_core/task/python/test/__init__.py
@@ -210,7 +210,7 @@ def has_test_dependency(setup_py_data, name):
       False otherwise
     :rtype: bool
     """
-    tests_require = setup_py_data.get('tests_require', [])
+    tests_require = setup_py_data.get('tests_require') or ()
     for d in tests_require:
         # the name might be followed by a version
         # separated by any of the following: ' ', <, >, <=, >=, ==, !=
diff --git a/test/test_package_identification_python.py b/test/test_package_identification_python.py
index 3acdaa66..403c3658 100644
--- a/test/test_package_identification_python.py
+++ b/test/test_package_identification_python.py
@@ -27,7 +27,6 @@ def unchanged_empty_descriptor(package_descriptor):
     assert package_descriptor.type is None
 
 
-@pytest.mark.xfail
 def test_error_in_setup_py(unchanged_empty_descriptor):
     setup_py = unchanged_empty_descriptor.path / 'setup.py'
     error_text = 'My hovercraft is full of eels'
@@ -51,7 +50,6 @@ def test_missing_setup_py(unchanged_empty_descriptor):
     extension.identify(unchanged_empty_descriptor)
 
 
-@pytest.mark.xfail
 def test_empty_setup_py(unchanged_empty_descriptor):
     extension = PythonPackageIdentification()
     (unchanged_empty_descriptor.path / 'setup.py').write_text('')
@@ -91,7 +89,6 @@ def test_re_identify_python_if_same_python_package(package_descriptor):
     assert package_descriptor.type == 'python'
 
 
-@pytest.mark.xfail
 def test_re_identify_python_if_different_python_package(package_descriptor):
     package_descriptor.name = 'other-package'
     package_descriptor.type = 'python'
@@ -182,7 +179,6 @@ def test_metadata_options(package_descriptor):
     assert options['packages'] == ['my_module']
 
 
-@pytest.mark.xfail
 def test_metadata_options_dynamic(package_descriptor):
     (package_descriptor.path / 'setup.py').write_text(
         'import setuptools; setuptools.setup()')

From efde854c8a5da9ba6af0ef91d76485271f6e8661 Mon Sep 17 00:00:00 2001
From: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Date: Fri, 13 Sep 2019 17:20:40 -0700
Subject: [PATCH 5/7] remove xfail from known words again

---
 test/spell_check.words | 1 -
 1 file changed, 1 deletion(-)

diff --git a/test/spell_check.words b/test/spell_check.words
index 5febe34b..cb8f4556 100644
--- a/test/spell_check.words
+++ b/test/spell_check.words
@@ -52,4 +52,3 @@ tuples
 unlinking
 veyor
 wildcards
-xfail

From 18643867f9bf17bd004f5294afe3cbda71249821 Mon Sep 17 00:00:00 2001
From: Dan Rose <dan@digilabs.io>
Date: Thu, 19 Sep 2019 17:37:11 -0500
Subject: [PATCH 6/7] remove xfail for new test

---
 test/test_package_identification_python.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/test/test_package_identification_python.py b/test/test_package_identification_python.py
index 403c3658..3b073bdd 100644
--- a/test/test_package_identification_python.py
+++ b/test/test_package_identification_python.py
@@ -58,7 +58,6 @@ def test_empty_setup_py(unchanged_empty_descriptor):
     assert e.match('not a Distutils setup script')
 
 
-@pytest.mark.xfail
 def test_setup_py_no_name(unchanged_empty_descriptor):
     extension = PythonPackageIdentification()
     (unchanged_empty_descriptor.path / 'setup.py').write_text(

From 3f670a8fa284458127140d0cd3abfac5c2c0ca78 Mon Sep 17 00:00:00 2001
From: Dan Rose <dan@digilabs.io>
Date: Fri, 27 Sep 2019 23:35:06 -0500
Subject: [PATCH 7/7] switch to ProcessPool

It turns out most of what I was doing with multiprocessing.Process was reinventing Pool.apply.
This also naturally throttles the number of simultaneous processes, which was a possible concern.
Move setup.py-specific stuff to run_setup_py. I expect to reuse this for python build, plus isolating it makes the subprocess workload minimal.
---
 colcon_core/package_identification/python.py | 77 +++++---------------
 colcon_core/run_setup_py.py                  | 46 ++++++++++++
 2 files changed, 64 insertions(+), 59 deletions(-)
 create mode 100644 colcon_core/run_setup_py.py

diff --git a/colcon_core/package_identification/python.py b/colcon_core/package_identification/python.py
index e9adcc16..31b65d48 100644
--- a/colcon_core/package_identification/python.py
+++ b/colcon_core/package_identification/python.py
@@ -1,9 +1,10 @@
 # Copyright 2016-2019 Dirk Thomas
-# Copyright 2019 Rover Robotics
+# Copyright 2019 Rover Robotics via Dan Rose
 # Licensed under the Apache License, Version 2.0
 
 import multiprocessing
 import os
+from traceback import format_exc
 from typing import Optional
 import warnings
 
@@ -12,9 +13,12 @@
 from colcon_core.package_identification \
     import PackageIdentificationExtensionPoint
 from colcon_core.plugin_system import satisfies_version
+from colcon_core.run_setup_py import run_setup_py
 from distlib.util import parse_requirement
 from distlib.version import NormalizedVersion
 
+_process_pool = multiprocessing.Pool()
+
 
 class PythonPackageIdentification(PackageIdentificationExtensionPoint):
     """Identify Python packages with `setup.py` and opt. `setup.cfg` files."""
@@ -104,65 +108,20 @@ def get_setup_result(setup_py, *, env: Optional[dict]):
     if env is not None:
         env_copy.update(env)
 
-    conn_recv, conn_send = multiprocessing.Pipe(duplex=False)
-    with conn_send:
-        p = multiprocessing.Process(
-            target=_get_setup_result_target,
-            args=(os.path.abspath(str(setup_py)), env_copy, conn_send),
-        )
-        p.start()
-        p.join()
-    with conn_recv:
-        result_or_exception_string = conn_recv.recv()
-
-    if isinstance(result_or_exception_string, dict):
-        return result_or_exception_string
-    raise RuntimeError(
-        'Failure when trying to run setup script {}:\n{}'
-        .format(setup_py, result_or_exception_string))
-
-
-def _get_setup_result_target(setup_py: str, env: dict, conn_send):
-    """
-    Run setup.py in a modified environment.
-
-    Helper function for get_setup_metadata. The resulting dict or error
-    will be sent via conn_send instead of returned or thrown.
-
-    :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
-        error string
-    """
-    import distutils.core
-    import traceback
     try:
-        # need to be in setup.py's parent dir to detect any setup.cfg
-        os.chdir(os.path.dirname(setup_py))
-
-        os.environ.clear()
-        os.environ.update(env)
-
-        result = distutils.core.run_setup(
-            str(setup_py), ('--dry-run',), stop_after='config')
-
-        # could just return all attrs in result.__dict__, but we take this
-        # opportunity to filter a few things that don't need to be there
-        conn_send.send({
-            attr: value for attr, value in result.__dict__.items()
-            if (
-                # These *seem* useful but always have the value 0.
-                # Look for their values in the 'metadata' object instead.
-                attr not in result.display_option_names
-                # Getter methods
-                and not callable(value)
-                # Private properties
-                and not attr.startswith('_')
-                # Objects that are generally not picklable
-                and attr not in ('cmdclass', 'distclass', 'ext_modules')
-            )})
-    except BaseException:
-        conn_send.send(traceback.format_exc())
+        return _process_pool.apply(
+            run_setup_py,
+            kwds={
+                'cwd': os.path.abspath(str(setup_py.parent)),
+                'env': env_copy,
+                'script_args': ('--dry-run',),
+                'stop_after': 'config'
+            }
+        )
+    except Exception as e:
+        raise RuntimeError(
+            'Failure when trying to run setup script {}: {}'
+            .format(setup_py, format_exc())) from e
 
 
 def create_dependency_descriptor(requirement_string):
diff --git a/colcon_core/run_setup_py.py b/colcon_core/run_setup_py.py
new file mode 100644
index 00000000..fa4c88ae
--- /dev/null
+++ b/colcon_core/run_setup_py.py
@@ -0,0 +1,46 @@
+# Copyright 2019 Rover Robotics via Dan Rose
+# Licensed under the Apache License, Version 2.0
+
+import distutils.core
+import os
+
+
+def run_setup_py(cwd: str, env: dict, script_args=(), stop_after='run'):
+    """
+    Modify the current process and run setup.py.
+
+    This should be run in a subprocess so as not to dirty the state of the
+    current process.
+
+    :param cwd: Absolute path to a directory containing a setup.py script
+    :param env: Environment variables to set before running setup.py
+    :param script_args: command-line arguments to pass to setup.py
+    :param stop_after: which
+    :returns: The properties of a Distribution object, minus some useless
+              and/or unpicklable properties
+    """
+    # need to be in setup.py's parent dir to detect any setup.cfg
+    os.chdir(cwd)
+
+    os.environ.clear()
+    os.environ.update(env)
+
+    result = distutils.core.run_setup(
+        'setup.py', script_args=script_args, stop_after=stop_after)
+
+    # could just return all attrs in result.__dict__, but we take this
+    # opportunity to filter a few things that don't need to be there
+    return {
+        attr: value for attr, value in result.__dict__.items()
+        if (
+            # These *seem* useful but always have the value 0.
+            # Look for their values in the 'metadata' object instead.
+            attr not in result.display_option_names
+            # Getter methods
+            and not callable(value)
+            # Private properties
+            and not attr.startswith('_')
+            # Objects that are generally not picklable
+            and attr not in ('cmdclass', 'distclass', 'ext_modules')
+        )
+    }