Better python package identification

colcon_core/package_identification/python.py

@@ -1,31 +1,27 @@
 # Copyright 2016-2019 Dirk Thomas
+# 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
+
 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 colcon_core.run_setup_py import run_setup_py
 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
+
+_process_pool = multiprocessing.Pool()
 
 
 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 +37,91 @@ 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)
+
+    try:
+        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):

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')
+        )
+    }

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/spell_check.words

@@ -16,6 +16,7 @@ deps
 descs
 getpid
 iterdir
+localhost
 lstrip
 mkdtemp
 nargs