From 94a420b473e014c67b26b79317819cf7a81132aa Mon Sep 17 00:00:00 2001 From: Dan Rose Date: Fri, 6 Sep 2019 23:22:53 -0500 Subject: [PATCH] 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 254daf8f..bb1e4c78 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('') @@ -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()')