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 01/11] 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 02/11] 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 03/11] 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 04/11] 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 05/11] 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 06/11] 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 971d91623b447254e0fd7a167a04313838e520ac Mon Sep 17 00:00:00 2001
From: Dan Rose <dan@digilabs.io>
Date: Thu, 26 Sep 2019 18:24:34 -0500
Subject: [PATCH 07/11] Don't use process isolation when bootstrapping

---
 colcon_core/package_identification/python.py | 79 +++++++++++++++-----
 1 file changed, 60 insertions(+), 19 deletions(-)

diff --git a/colcon_core/package_identification/python.py b/colcon_core/package_identification/python.py
index e9adcc16..fbb6d787 100644
--- a/colcon_core/package_identification/python.py
+++ b/colcon_core/package_identification/python.py
@@ -2,6 +2,7 @@
 # Copyright 2019 Rover Robotics
 # Licensed under the Apache License, Version 2.0
 
+import distutils.core
 import multiprocessing
 import os
 from typing import Optional
@@ -36,7 +37,16 @@ def identify(self, desc):  # noqa: D102
         # 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)
+        if os.path.realpath(__file__).startswith(
+            os.path.realpath(str(desc.path))
+        ):
+            # Bootstrapping colcon.
+            # todo: is this really necessary?
+            get_setup_fn = get_setup_result_in_process
+        else:
+            get_setup_fn = get_setup_result
+
+        config = get_setup_fn(setup_py, env=None)
 
         name = config['metadata'].name
         if not name:
@@ -64,8 +74,8 @@ def identify(self, desc):  # noqa: D102
                 for d in config[option_name] or ()}
 
         def getter(env):
-            nonlocal setup_py
-            return get_setup_result(setup_py, env=env)
+            nonlocal setup_py, get_setup_fn
+            return get_setup_fn(setup_py, env=env)
 
         desc.metadata['get_python_setup_options'] = getter
 
@@ -134,7 +144,6 @@ def _get_setup_result_target(setup_py: str, env: dict, conn_send):
     :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
@@ -146,25 +155,57 @@ def _get_setup_result_target(setup_py: str, env: dict, conn_send):
         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')
-            )})
+        conn_send.send(_distribution_to_dict(result))
     except BaseException:
         conn_send.send(traceback.format_exc())
 
 
+def get_setup_result_in_process(setup_py, *, env: Optional[dict]):
+    """
+    Run setup.py in this process.
+
+    Prefer get_setup_result, since it provides process isolation is
+    threadsafe, and returns predictable errors.
+    :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 script doesn't appear to be a setup script.
+            Any exception raised in the setup.py script.
+    """
+    save_env = os.environ.copy()
+    save_cwd = os.getcwd()
+
+    try:
+        if env is not None:
+            os.environ.update(env)
+        os.chdir(str(setup_py.parent))
+        dist = distutils.core.run_setup(
+            'setup.py', ('--dry-run',), stop_after='config')
+    finally:
+        if env is not None:
+            os.environ.clear()
+            os.environ.update(save_env)
+        os.chdir(save_cwd)
+    return _distribution_to_dict(dist)
+
+
+def _distribution_to_dict(distribution_object) -> dict:
+    """Turn a distribution into a dict, discarding unpicklable attributes."""
+    return {
+        attr: value for attr, value in distribution_object.__dict__.items()
+        if (
+            # These *seem* useful but always have the value 0.
+            # Look for their values in the 'metadata' object instead.
+            attr not in distribution_object.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')
+        )}
+
+
 def create_dependency_descriptor(requirement_string):
     """
     Create a DependencyDescriptor from a PEP440 compliant string.

From f63205485ac458cf3ff3241b5655b991e895d254 Mon Sep 17 00:00:00 2001
From: Dan Rose <dan@digilabs.io>
Date: Fri, 27 Sep 2019 20:26:34 -0500
Subject: [PATCH 08/11] Modify build test also test --symlink-install

---
 test/spell_check.words    |  2 ++
 test/test_build_python.py | 21 +++++++++++++--------
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/test/spell_check.words b/test/spell_check.words
index cceac5b8..92e86287 100644
--- a/test/spell_check.words
+++ b/test/spell_check.words
@@ -15,6 +15,7 @@ decos
 deps
 descs
 getpid
+importlib
 iterdir
 lstrip
 mkdtemp
@@ -45,6 +46,7 @@ stdeb
 subparsers
 symlink
 sysconfig
+syspath
 tempfile
 thomas
 todo
diff --git a/test/test_build_python.py b/test/test_build_python.py
index d06d7cee..6313455e 100644
--- a/test/test_build_python.py
+++ b/test/test_build_python.py
@@ -2,6 +2,7 @@
 # Licensed under the Apache License, Version 2.0
 
 import asyncio
+from distutils.sysconfig import get_python_lib
 from pathlib import Path
 from types import SimpleNamespace
 
@@ -14,6 +15,7 @@
 from colcon_core.task import TaskContext
 from colcon_core.task.python.build import PythonBuildTask
 import pytest
+import importlib
 
 
 @pytest.fixture(autouse=True)
@@ -45,8 +47,8 @@ def monkey_patch_put_event_into_queue(monkeypatch):
     )
 
 
-@pytest.fixture
-def python_build_task(tmp_path):
+@pytest.fixture(params=('symlink', 'copy'))
+def python_build_task(tmp_path, request):
     task = PythonBuildTask()
     package = PackageDescriptor(tmp_path / 'src')
     package.name = 'test_package'
@@ -58,7 +60,7 @@ def python_build_task(tmp_path):
             path=str(tmp_path / 'src'),
             build_base=str(tmp_path / 'build'),
             install_base=str(tmp_path / 'install'),
-            symlink_install=False,
+            symlink_install=(request.param == 'symlink'),
         ),
         dependencies={}
     )
@@ -74,7 +76,9 @@ def event_loop():
     loop.close()
 
 
-def test_build_package(python_build_task: PythonBuildTask, event_loop):
+def test_build_package(
+    python_build_task: PythonBuildTask, event_loop, monkeypatch
+):
     pkg = python_build_task.context.pkg
 
     pkg.path.mkdir()
@@ -96,10 +100,11 @@ def test_build_package(python_build_task: PythonBuildTask, event_loop):
     assert source_files_before == source_files_after
 
     build_base = Path(python_build_task.context.args.build_base)
-    assert 1 == len(list(build_base.rglob('my_module/__init__.py')))
+    assert 1 >= len(list(build_base.rglob('my_module/__init__.py')))
 
     install_base = Path(python_build_task.context.args.install_base)
-    assert 1 == len(list(install_base.rglob('my_module/__init__.py')))
 
-    pkg_info, = install_base.rglob('PKG-INFO')
-    assert 'Name: test-package' in pkg_info.read_text().splitlines()
+    assert importlib.util.find_spec('my_module') is None
+    importlib.invalidate_caches()
+    monkeypatch.syspath_prepend(get_python_lib(prefix=str(install_base)))
+    assert importlib.util.find_spec('my_module') is not None

From d07f7a3ffbb078239c01bdfcb88e74e7563266f9 Mon Sep 17 00:00:00 2001
From: Dan Rose <dan@digilabs.io>
Date: Fri, 27 Sep 2019 14:02:40 -0500
Subject: [PATCH 09/11] Symlink packages as eggs Modify build tests to build in
 symlink mode as well

---
 colcon_core/task/python/build.py | 109 ++++++++++++++-----------------
 1 file changed, 48 insertions(+), 61 deletions(-)

diff --git a/colcon_core/task/python/build.py b/colcon_core/task/python/build.py
index 5a9f9d68..f856e7c0 100644
--- a/colcon_core/task/python/build.py
+++ b/colcon_core/task/python/build.py
@@ -73,9 +73,10 @@ async def build(self, *, additional_hooks=None):  # noqa: D102
                 '--single-version-externally-managed',
             ]
             self._append_install_layout(args, cmd)
-            rc = await check_call(self.context, cmd, cwd=args.path, env=env)
-            if rc and rc.returncode:
-                return rc.returncode
+            completed_process = await check_call(self.context, cmd,
+                cwd=args.path, env=env)
+            if completed_process.returncode:
+                return completed_process.returncode
 
         else:
             self._undo_install(pkg, args, setup_py_data, python_lib)
@@ -84,6 +85,28 @@ async def build(self, *, additional_hooks=None):  # noqa: D102
             # invoke `setup.py develop` step in build space
             # to avoid placing any files in the source space
 
+            for filename in os.listdir(args.build_base):
+                path = os.path.join(args.build_base, filename)
+                try:
+                    os.unlink(path)
+                except IsADirectoryError:
+                    shutil.rmtree(path)
+                except FileNotFoundError:
+                    pass
+
+            for path in os.listdir(args.path):
+                src = os.path.join(args.path, path)
+                dst = os.path.join(args.build_base, path)
+
+                try:
+                    os.remove(dst)
+                except IsADirectoryError:
+                    shutil.rmtree(dst)
+                except FileNotFoundError:
+                    pass
+
+                os.symlink(src, dst, os.path.isdir(src))
+
             # --editable causes this to skip creating/editing the
             # easy-install.pth file
             cmd = [
@@ -101,13 +124,28 @@ async def build(self, *, additional_hooks=None):  # noqa: D102
             if rc and rc.returncode:
                 return rc.returncode
 
-            # explicitly add the build directory to the PYTHONPATH
-            # to maintain the desired order
-            if additional_hooks is None:
-                additional_hooks = []
-            additional_hooks += create_environment_hook(
-                'pythonpath_develop', Path(args.build_base), pkg.name,
-                'PYTHONPATH', args.build_base, mode='prepend')
+            Path(args.build_base,
+                pkg.name.replace('-', '_') + '.egg-info').rename(
+                Path(args.build_base, 'EGG-INFO'))
+
+            meta = Path(args.build_base, 'EGG-INFO').read_text()
+
+            mkli
+            egg_link = os.path.join(
+                python_lib, pkg.name + '.egg-link')
+            assert Path(egg_link).exists()
+
+            link_target = Path(Path(egg_link).read_text().splitlines()[0])
+
+            link_source = Path(python_lib, pkg.name + '.egg')
+            try:
+                link_source.unlink()
+            except IsADirectoryError:
+                link_source.rmdir()
+            except FileNotFoundError:
+                pass
+
+            link_source.symlink_to(link_target, target_is_directory=True)
 
         hooks = create_environment_hooks(args.install_base, pkg.name)
         create_environment_scripts(
@@ -175,57 +213,6 @@ def _undo_install(self, pkg, args, setup_py_data, python_lib):
                 pass
         os.remove(install_log)
 
-    def _symlinks_in_build(self, args, setup_py_data):
-        items = ['setup.py']
-        # add setup.cfg if available
-        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') or {}
-        for package in setup_py_data.get('packages') or []:
-            if '.' in package:
-                continue
-            if package in package_dir:
-                items.append(package_dir[package])
-            elif '' in package_dir:
-                items.append(os.path.join(package_dir[''], package))
-            else:
-                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') or []
-        if py_modules:
-            py_modules_list = [
-                p.replace('.', os.path.sep) + '.py' for p in py_modules]
-            for py_module in py_modules_list:
-                if not os.path.exists(os.path.join(args.path, py_module)):
-                    raise RuntimeError(
-                        "Provided py_modules '{py_module}' does not exist"
-                        .format_map(locals()))
-            items += py_modules_list
-        data_files = get_data_files_mapping(
-            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):
-                source = os.path.relpath(source, args.path)
-            items.append(source)
-
-        # symlink files / directories from source space into build space
-        for item in items:
-            src = os.path.join(args.path, item)
-            dst = os.path.join(args.build_base, item)
-            os.makedirs(os.path.dirname(dst), exist_ok=True)
-            if os.path.islink(dst):
-                if not os.path.exists(dst) or not os.path.samefile(src, dst):
-                    os.unlink(dst)
-            elif os.path.isfile(dst):
-                os.remove(dst)
-            elif os.path.isdir(dst):
-                shutil.rmtree(dst)
-            if not os.path.exists(dst):
-                os.symlink(src, dst)
-
     def _get_python_lib(self, args):
         path = get_python_lib(prefix=args.install_base)
         return os.path.relpath(path, start=args.install_base)

From c8fa55443a2612432f2e2bab4aedd89a36888060 Mon Sep 17 00:00:00 2001
From: Dan Rose <dan@digilabs.io>
Date: Fri, 27 Sep 2019 16:40:49 -0500
Subject: [PATCH 10/11] Sweet. It seems to work (still WIP though)

Eggs was a bad idea. They still have to each be in PYTHONPATH to work.
---
 colcon_core/task/python/build.py | 100 ++++++++++---------------------
 1 file changed, 31 insertions(+), 69 deletions(-)

diff --git a/colcon_core/task/python/build.py b/colcon_core/task/python/build.py
index f856e7c0..60e6ce42 100644
--- a/colcon_core/task/python/build.py
+++ b/colcon_core/task/python/build.py
@@ -12,11 +12,9 @@
 from colcon_core.environment import create_environment_scripts
 from colcon_core.logging import colcon_logger
 from colcon_core.plugin_system import satisfies_version
-from colcon_core.shell import create_environment_hook
 from colcon_core.shell import get_command_environment
 from colcon_core.task import check_call
 from colcon_core.task import TaskExtensionPoint
-from colcon_core.task.python import get_data_files_mapping
 from colcon_core.task.python import get_setup_data
 
 logger = colcon_logger.getChild(__name__)
@@ -44,22 +42,17 @@ async def build(self, *, additional_hooks=None):  # noqa: D102
             return 1
         setup_py_data = get_setup_data(self.context.pkg, env)
 
+        shutil.rmtree(args.build_base)
         # `setup.py egg_info` requires the --egg-base to exist
         os.makedirs(args.build_base, exist_ok=True)
-        # `setup.py develop|install` requires the python lib path to exist
+        # `setup.py` requires the python lib path to exist
         python_lib = os.path.join(
             args.install_base, self._get_python_lib(args))
         os.makedirs(python_lib, exist_ok=True)
-        # and being in the  PYTHONPATH
-        env = dict(env)
-        env['PYTHONPATH'] = python_lib + os.pathsep + \
-            env.get('PYTHONPATH', '')
 
-        if not args.symlink_install:
-            rc = await self._undo_develop(pkg, args, env)
-            if rc and rc.returncode:
-                return rc.returncode
+        self._undo_install(pkg, args, setup_py_data, python_lib)
 
+        if not args.symlink_install:
             # invoke `setup.py install` step with lots of arguments
             # to avoid placing any files in the source space
             cmd = [
@@ -73,27 +66,15 @@ async def build(self, *, additional_hooks=None):  # noqa: D102
                 '--single-version-externally-managed',
             ]
             self._append_install_layout(args, cmd)
-            completed_process = await check_call(self.context, cmd,
-                cwd=args.path, env=env)
+            completed_process = await check_call(
+                self.context, cmd, cwd=args.path, env=env)
             if completed_process.returncode:
                 return completed_process.returncode
 
         else:
-            self._undo_install(pkg, args, setup_py_data, python_lib)
-            self._symlinks_in_build(args, setup_py_data)
-
-            # invoke `setup.py develop` step in build space
+            # invoke `setup.py` step in build space
             # to avoid placing any files in the source space
 
-            for filename in os.listdir(args.build_base):
-                path = os.path.join(args.build_base, filename)
-                try:
-                    os.unlink(path)
-                except IsADirectoryError:
-                    shutil.rmtree(path)
-                except FileNotFoundError:
-                    pass
-
             for path in os.listdir(args.path):
                 src = os.path.join(args.path, path)
                 dst = os.path.join(args.build_base, path)
@@ -111,63 +92,44 @@ async def build(self, *, additional_hooks=None):  # noqa: D102
             # easy-install.pth file
             cmd = [
                 executable, 'setup.py',
-                'develop', '--prefix', args.install_base,
-                '--editable',
-                '--build-directory', os.path.join(args.build_base, 'build'),
-                '--no-deps',
+                'build_py', '--build-lib', '.',
+                # ^todo: does this present a problem with modules under
+                #  weird subdirectories?
+                'egg_info',
+                'build_ext', '--inplace'
             ]
             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(
                 self.context, cmd, cwd=args.build_base, env=env)
             if rc and rc.returncode:
                 return rc.returncode
 
-            Path(args.build_base,
-                pkg.name.replace('-', '_') + '.egg-info').rename(
-                Path(args.build_base, 'EGG-INFO'))
-
-            meta = Path(args.build_base, 'EGG-INFO').read_text()
-
-            mkli
-            egg_link = os.path.join(
-                python_lib, pkg.name + '.egg-link')
-            assert Path(egg_link).exists()
-
-            link_target = Path(Path(egg_link).read_text().splitlines()[0])
-
-            link_source = Path(python_lib, pkg.name + '.egg')
-            try:
-                link_source.unlink()
-            except IsADirectoryError:
-                link_source.rmdir()
-            except FileNotFoundError:
-                pass
-
-            link_source.symlink_to(link_target, target_is_directory=True)
+            install_log = []
+            # Install the built files via symlink.
+            # todo: I think we're copying too many things.
+            #       How does `setup.py install` choose what to copy?
+            for path in os.listdir(args.build_base):
+                dst = os.path.join(python_lib, path)
+                src = Path(args.build_base, path).resolve()
+
+                install_log.append(dst)
+                if str(src).startswith(args.build_base):
+                    try:
+                        shutil.copy(src, dst)
+                    except IsADirectoryError:
+                        shutil.copytree(src, dst)
+                else:
+                    os.symlink(src, dst, os.path.isdir(src))
+            Path(args.build_base, 'install.log').write_text(
+                '\n'.join(install_log))
 
         hooks = create_environment_hooks(args.install_base, pkg.name)
         create_environment_scripts(
             pkg, args, default_hooks=hooks, additional_hooks=additional_hooks)
 
-    async def _undo_develop(self, pkg, args, env):
-        # undo previous develop if .egg-info is found and develop symlinks
-        egg_info = os.path.join(
-            args.build_base, '%s.egg-info' % pkg.name.replace('-', '_'))
-        setup_py_build_space = os.path.join(args.build_base, 'setup.py')
-        if os.path.exists(egg_info) and os.path.islink(setup_py_build_space):
-            cmd = [
-                executable, 'setup.py',
-                'develop', '--prefix', args.install_base,
-                '--uninstall', '--editable',
-                '--build-directory', os.path.join(args.build_base, 'build')
-            ]
-            rc = await check_call(
-                self.context, cmd, cwd=args.build_base, env=env)
-            if rc:
-                return rc
-
     def _undo_install(self, pkg, args, setup_py_data, python_lib):
         # undo previous install if install.log is found
         install_log = os.path.join(args.build_base, 'install.log')

From ac0485475b030e770bb1010e2fb4e941353bb2a1 Mon Sep 17 00:00:00 2001
From: Dan Rose <dan@digilabs.io>
Date: Fri, 27 Sep 2019 19:45:21 -0500
Subject: [PATCH 11/11] Install modules via symlink even if they are in
 subdirectories. Only install subdirectories of Log installed files w/
 symlink_install

---
 colcon_core/task/python/build.py | 98 ++++++++++++++++++++------------
 1 file changed, 62 insertions(+), 36 deletions(-)

diff --git a/colcon_core/task/python/build.py b/colcon_core/task/python/build.py
index 60e6ce42..afe03af7 100644
--- a/colcon_core/task/python/build.py
+++ b/colcon_core/task/python/build.py
@@ -1,6 +1,5 @@
 # Copyright 2016-2018 Dirk Thomas
 # Licensed under the Apache License, Version 2.0
-
 from distutils.sysconfig import get_python_lib
 import os
 from pathlib import Path
@@ -11,6 +10,7 @@
 from colcon_core.environment import create_environment_hooks
 from colcon_core.environment import create_environment_scripts
 from colcon_core.logging import colcon_logger
+from colcon_core.package_identification.python import get_setup_result
 from colcon_core.plugin_system import satisfies_version
 from colcon_core.shell import get_command_environment
 from colcon_core.task import check_call
@@ -42,7 +42,11 @@ async def build(self, *, additional_hooks=None):  # noqa: D102
             return 1
         setup_py_data = get_setup_data(self.context.pkg, env)
 
-        shutil.rmtree(args.build_base)
+        try:
+            shutil.rmtree(args.build_base)
+        except FileNotFoundError:
+            pass
+
         # `setup.py egg_info` requires the --egg-base to exist
         os.makedirs(args.build_base, exist_ok=True)
         # `setup.py` requires the python lib path to exist
@@ -75,56 +79,55 @@ async def build(self, *, additional_hooks=None):  # noqa: D102
             # invoke `setup.py` step in build space
             # to avoid placing any files in the source space
 
-            for path in os.listdir(args.path):
-                src = os.path.join(args.path, path)
-                dst = os.path.join(args.build_base, path)
-
-                try:
-                    os.remove(dst)
-                except IsADirectoryError:
-                    shutil.rmtree(dst)
-                except FileNotFoundError:
-                    pass
+            # symlink *everything* from source space to build space
+            for src_path in Path(args.path).iterdir():
+                dst_path = Path(args.build_base, src_path.name)
+                _clear_path(dst_path)
+                dst_path.symlink_to(
+                    src_path, target_is_directory=src_path.is_dir())
 
-                os.symlink(src, dst, os.path.isdir(src))
-
-            # --editable causes this to skip creating/editing the
-            # easy-install.pth file
             cmd = [
                 executable, 'setup.py',
                 'build_py', '--build-lib', '.',
-                # ^todo: does this present a problem with modules under
-                #  weird subdirectories?
                 'egg_info',
                 'build_ext', '--inplace'
             ]
             if setup_py_data.get('data_files'):
                 cmd += ['install_data', '--install-dir', args.install_base]
 
-            self._append_install_layout(args, cmd)
+            # todo: We just ran setup.py. get_setup_result runs it again.
+            #       We should use distutils.core.run_setup to get access to the
+            #       distribution object
+            setup_result = get_setup_result(
+                Path(args.build_base, 'setup.py'), env=None)
+            package_dir = Path(setup_result['package_dir'] or args.build_base)
+
             rc = await check_call(
                 self.context, cmd, cwd=args.build_base, env=env)
             if rc and rc.returncode:
                 return rc.returncode
 
             install_log = []
-            # Install the built files via symlink.
-            # todo: I think we're copying too many things.
-            #       How does `setup.py install` choose what to copy?
-            for path in os.listdir(args.build_base):
-                dst = os.path.join(python_lib, path)
-                src = Path(args.build_base, path).resolve()
-
-                install_log.append(dst)
-                if str(src).startswith(args.build_base):
-                    try:
-                        shutil.copy(src, dst)
-                    except IsADirectoryError:
-                        shutil.copytree(src, dst)
-                else:
-                    os.symlink(src, dst, os.path.isdir(src))
-            Path(args.build_base, 'install.log').write_text(
-                '\n'.join(install_log))
+
+            # symlink *just Python modules* from build space to install space
+            try:
+                for src_path in _iter_modules(package_dir):
+                    # Every package will have a top-level setup.py
+                    # No need to install it.
+                    if src_path == package_dir / 'setup.py':
+                        continue
+                    rel_path = src_path.relative_to(package_dir)
+                    dst_path = Path(python_lib, rel_path)
+                    if dst_path.exists():
+                        logger.warning('Overwriting existing file at '
+                                       + dst_path)
+                        _clear_path(dst_path)
+                    dst_path.parent.mkdir(parents=True, exist_ok=True)
+                    dst_path.symlink_to(src_path.resolve())
+                    install_log.append(str(dst_path))
+            finally:
+                Path(args.build_base, 'install.log') \
+                    .write_text('\n'.join(install_log))
 
         hooks = create_environment_hooks(args.install_base, pkg.name)
         create_environment_scripts(
@@ -182,3 +185,26 @@ def _get_python_lib(self, args):
     def _append_install_layout(self, args, cmd):
         if 'dist-packages' in self._get_python_lib(args):
             cmd += ['--install-layout', 'deb']
+
+
+def _clear_path(path):
+    """Remove any file or directory at the given path."""
+    try:
+        os.unlink(str(path))
+    except IsADirectoryError:
+        shutil.rmtree(str(path))
+    except FileNotFoundError:
+        pass
+
+
+def _iter_modules(base_path: Path):
+    """Find all top-level modules (*/__init__.py or *.py) below base_path."""
+    if (base_path / '__init__.py').resolve().is_file():
+        yield base_path
+        return
+
+    for child in base_path.iterdir():
+        if child.resolve().is_file() and child.suffix in ('.py', '.pyc'):
+            yield child
+        if child.resolve().is_dir():
+            yield from _iter_modules(child)