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

@@ -1,31 +1,23 @@
 # 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 \
     import PackageIdentificationExtensionPoint
 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):

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):

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: ' ', <, >, <=, >=, ==, !=

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('')
@@ -82,7 +80,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'
@@ -173,7 +170,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()')