From 3e33c4983eedf15af4e4c1e3701d7e92897111c1 Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Mon, 6 Dec 2021 10:55:30 -0800 Subject: [PATCH] Warn when overriding packages at build time (#449) * First prototype warning on build of overridden packages Signed-off-by: Shane Loretz * Add FindInstalledPackagesExtensionPoint and improve override warning text Signed-off-by: Shane Loretz * its -> their Signed-off-by: Shane Loretz * Fix CI Signed-off-by: Shane Loretz * Update docstring of find_installed_packages Signed-off-by: Shane Loretz * Make unit tests aware of FindInstalledPackagesExtensionPoint Signed-off-by: Shane Loretz * Fix import order Signed-off-by: Shane Loretz * Test raises NotImplementedError Signed-off-by: Shane Loretz * Add extend action for older python versions Signed-off-by: Shane Loretz * Flake8 tuple whitespace Signed-off-by: Shane Loretz * Spellcheck extra words Signed-off-by: Shane Loretz * Make override warning clearer Signed-off-by: Shane Loretz * Grammar in path arg documentation Signed-off-by: Shane Loretz * Check packages is not None Signed-off-by: Shane Loretz * Test warning when extension reports package that doesn't exist Signed-off-by: Shane Loretz * Test two extensions finding same package in same install base Signed-off-by: Shane Loretz * Fix expected string on windows Signed-off-by: Shane Loretz Co-authored-by: Scott K Logan --- colcon_core/shell/__init__.py | 110 +++++++++++----- colcon_core/shell/installed_packages.py | 57 +++++++++ colcon_core/verb/build.py | 67 ++++++++++ setup.cfg | 4 + test/spell_check.words | 3 + test/test_shell.py | 159 +++++++++++++++++------- 6 files changed, 326 insertions(+), 74 deletions(-) create mode 100644 colcon_core/shell/installed_packages.py diff --git a/colcon_core/shell/__init__.py b/colcon_core/shell/__init__.py index 17cf1ca5..b62fa3d0 100644 --- a/colcon_core/shell/__init__.py +++ b/colcon_core/shell/__init__.py @@ -12,7 +12,6 @@ import warnings from colcon_core.environment_variable import EnvironmentVariable -from colcon_core.location import get_relative_package_index_path from colcon_core.logging import colcon_logger from colcon_core.plugin_system import instantiate_extensions from colcon_core.plugin_system import order_extensions_grouped_by_priority @@ -563,46 +562,93 @@ def find_installed_packages_in_environment(): return packages -def find_installed_packages(install_base): +class FindInstalledPackagesExtensionPoint: + """ + The interface for extensions to find installed packages. + + This type of extension locates installed packages inside a prefix path. + """ + + """The version of this extension interface.""" + EXTENSION_POINT_VERSION = '1.0' + + """The default priority of an extension.""" + PRIORITY = 100 + + def find_installed_packages(self, install_base: Path): + """ + Find installed packages in an install path. + + This method must be overridden in a subclass. + + :param Path prefix_path: The path of the install prefix + :returns: The mapping from a package name to the prefix path, or None + if the path is not an install layout supported by this extension. + :rtype: Dict or None + """ + raise NotImplementedError() + + +def get_find_installed_packages_extensions(): + """ + Get the available package identification extensions. + + The extensions are grouped by their priority and each group is ordered by + the entry point name. + + :rtype: OrderedDict + """ + extensions = instantiate_extensions(__name__ + '.find_installed_packages') + for name, extension in extensions.items(): + extension.PACKAGE_IDENTIFICATION_NAME = name + return order_extensions_grouped_by_priority(extensions) + + +def find_installed_packages(install_base: Path): """ Find install packages under the install base path. - The path must contain a marker file with the install layout. - Based on the install layout the packages are discovered i different + Based on the install layout the packages may be discovered in different locations. :param Path install_base: The base path to find installed packages in :returns: The mapping from a package name to the prefix path, None if the - path doesn't exist or doesn't contain a marker file with a valid install - layout - :rtype: OrderedDict or None + path is not a supported install layout or it doesn't exist + :rtype: Dict or None """ - marker_file = install_base / '.colcon_install_layout' - if not marker_file.is_file(): - return None - install_layout = marker_file.read_text().rstrip() - if install_layout not in ('isolated', 'merged'): - return None + # priority means getting invoked first, but maybe that doesn't matter + extensions = [] + prioritized_extensions = get_find_installed_packages_extensions() + for ext_list in prioritized_extensions.values(): + extensions.extend(ext_list.values()) + # Combine packages found by all extensions packages = {} - if install_layout == 'isolated': - # for each subdirectory look for the package specific file - for p in install_base.iterdir(): - if not p.is_dir(): - continue - if p.name.startswith('.'): + valid_prefix = False + + for ext in extensions: + ext_packages = ext.find_installed_packages(install_base) + if ext_packages is None: + continue + + valid_prefix = True + for pkg, path in ext_packages.items(): + if not path.exists(): + logger.warning( + "Ignoring '{pkg}' found at '{path}' because the path" + ' does not exist.'.format_map(locals())) continue - marker = p / get_relative_package_index_path() / p.name - if marker.is_file(): - packages[p.name] = p - else: - # find all files in the subdirectory - if (install_base / get_relative_package_index_path()).is_dir(): - package_index = install_base / get_relative_package_index_path() - for p in package_index.iterdir(): - if not p.is_file(): - continue - if p.name.startswith('.'): - continue - packages[p.name] = install_base + if pkg in packages and not path.samefile(packages[pkg]): + # Same package found at different paths in the same prefix + first_path = packages[pkg] + logger.warning( + "The package '{pkg}' previously found at " + "'{first_path}' was found again at '{path}'." + " Ignoring '{path}'".format_map(locals())) + else: + packages[pkg] = path + + if not valid_prefix: + # No extension said this was a valid prefix + return None return packages diff --git a/colcon_core/shell/installed_packages.py b/colcon_core/shell/installed_packages.py new file mode 100644 index 00000000..f7f6885e --- /dev/null +++ b/colcon_core/shell/installed_packages.py @@ -0,0 +1,57 @@ +# Copyright 2016-2021 Dirk Thomas +# Licensed under the Apache License, Version 2.0 + +from pathlib import Path + +from colcon_core.location import get_relative_package_index_path +from colcon_core.shell import FindInstalledPackagesExtensionPoint + + +class IsolatedInstalledPackageFinder(FindInstalledPackagesExtensionPoint): + """Find installed packages in colcon isolated install spaces.""" + + def find_installed_packages(self, install_base: Path): + """Find installed packages in colcon isolated install spaces.""" + marker_file = install_base / '.colcon_install_layout' + if not marker_file.is_file(): + return None + install_layout = marker_file.read_text().rstrip() + if install_layout != 'isolated': + return None + + packages = {} + # for each subdirectory look for the package specific file + for p in install_base.iterdir(): + if not p.is_dir(): + continue + if p.name.startswith('.'): + continue + marker = p / get_relative_package_index_path() / p.name + if marker.is_file(): + packages[p.name] = p + return packages + + +class MergedInstalledPackageFinder(FindInstalledPackagesExtensionPoint): + """Find installed packages in colcon merged install spaces.""" + + def find_installed_packages(self, install_base: Path): + """Find installed packages in colcon isolated install spaces.""" + marker_file = install_base / '.colcon_install_layout' + if not marker_file.is_file(): + return None + install_layout = marker_file.read_text().rstrip() + if install_layout != 'merged': + return None + + packages = {} + # find all files in the subdirectory + if (install_base / get_relative_package_index_path()).is_dir(): + package_index = install_base / get_relative_package_index_path() + for p in package_index.iterdir(): + if not p.is_file(): + continue + if p.name.startswith('.'): + continue + packages[p.name] = install_base + return packages diff --git a/colcon_core/verb/build.py b/colcon_core/verb/build.py index 8a82d966..c52ae7eb 100644 --- a/colcon_core/verb/build.py +++ b/colcon_core/verb/build.py @@ -1,10 +1,12 @@ # Copyright 2016-2018 Dirk Thomas # Licensed under the Apache License, Version 2.0 +import argparse from collections import OrderedDict import os import os.path from pathlib import Path +import sys import traceback from colcon_core.argument_parser.destination_collector \ @@ -20,6 +22,8 @@ as add_packages_arguments from colcon_core.package_selection import get_packages from colcon_core.plugin_system import satisfies_version +from colcon_core.prefix_path import get_chained_prefix_path +from colcon_core.shell import find_installed_packages from colcon_core.shell import get_shell_extensions from colcon_core.task import add_task_arguments from colcon_core.task import get_task_extension @@ -31,6 +35,19 @@ from colcon_core.verb import VerbExtensionPoint +if sys.version_info < (3, 8): + # TODO(sloretz) remove when minimum supported Python version is 3.8 + # https://stackoverflow.com/a/41153081 + class _ExtendAction(argparse.Action): + """Add argparse action to extend a list.""" + + def __call__(self, parser, namespace, values, option_string=None): + """Extend the list with new arguments.""" + items = getattr(namespace, self.dest) or [] + items.extend(values) + setattr(namespace, self.dest, items) + + class BuildPackageArguments: """Arguments to build a specific package.""" @@ -81,6 +98,9 @@ def __init__(self): # noqa: D107 satisfies_version(VerbExtensionPoint.EXTENSION_POINT_VERSION, '^1.0') def add_arguments(self, *, parser): # noqa: D102 + if sys.version_info < (3, 8): + # TODO(sloretz) remove when minimum supported Python version is 3.8 + parser.register('action', 'extend', _ExtendAction) parser.add_argument( '--build-base', default='build', @@ -106,6 +126,13 @@ def add_arguments(self, *, parser): # noqa: D102 help='Continue other packages when a package fails to build ' '(packages recursively depending on the failed package are ' 'skipped)') + parser.add_argument( + '--allow-overriding', + action='extend', + default=[], + metavar='PKG_NAME', + nargs='+', + help='Allow building packages that exist in underlay workspaces') add_executor_arguments(parser) add_event_handler_arguments(parser) @@ -133,6 +160,46 @@ def main(self, *, context): # noqa: D102 jobs, unselected_packages = self._get_jobs( context.args, decorators, install_base) + underlay_packages = {} + for prefix_path in get_chained_prefix_path(): + packages = find_installed_packages(Path(prefix_path)) + if packages: + for pkg, path in packages.items(): + if pkg not in underlay_packages: + underlay_packages[pkg] = [] + underlay_packages[pkg].append(str(path)) + + override_messages = {} + for overlay_package in jobs.keys(): + if overlay_package in underlay_packages: + if overlay_package not in context.args.allow_overriding: + override_messages[overlay_package] = ( + "'{overlay_package}'".format_map(locals()) + + ' is in: ' + + ', '.join(underlay_packages[overlay_package])) + + if override_messages: + override_msg = ( + 'Some selected packages are already built in one or more' + ' underlay workspaces:' + '\n\t' + + '\n\t'.join(override_messages.values()) + + '\nIf a package in a merged underlay workspace is overridden' + ' and it installs headers, then all packages in the overlay' + ' must sort their include directories by workspace order.' + ' Failure to do so may result in build failures or undefined' + ' behavior at run time.' + '\nIf the overridden package is used by another package' + ' in any underlay, then the overriding package in the' + ' overlay must be API and ABI compatible or undefined' + ' behavior at run time may occur.' + '\n\nIf you understand the risks and want to override a' + ' package anyways, add the following to the command' + ' line:' + '\n\t--allow-overriding ' + + ' '.join(sorted(override_messages.keys()))) + raise RuntimeError(override_msg) + on_error = OnError.interrupt \ if not context.args.continue_on_error else OnError.skip_downstream diff --git a/setup.cfg b/setup.cfg index e76aacc5..41514513 100644 --- a/setup.cfg +++ b/setup.cfg @@ -106,6 +106,7 @@ colcon_core.extension_point = colcon_core.prefix_path = colcon_core.prefix_path:PrefixPathExtensionPoint colcon_core.python_testing = colcon_core.task.python.test:PythonTestingStepExtensionPoint colcon_core.shell = colcon_core.shell:ShellExtensionPoint + colcon_core.shell.find_installed_packages = colcon_core.shell:FindInstalledPackagesExtensionPoint colcon_core.task.build = colcon_core.task:TaskExtensionPoint colcon_core.task.test = colcon_core.task:TaskExtensionPoint colcon_core.verb = colcon_core.verb:VerbExtensionPoint @@ -126,6 +127,9 @@ colcon_core.shell = bat = colcon_core.shell.bat:BatShell dsv = colcon_core.shell.dsv:DsvShell sh = colcon_core.shell.sh:ShShell +colcon_core.shell.find_installed_packages = + colcon_isolated = colcon_core.shell.installed_packages:IsolatedInstalledPackageFinder + colcon_merged = colcon_core.shell.installed_packages:MergedInstalledPackageFinder colcon_core.task.build = python = colcon_core.task.python.build:PythonBuildTask colcon_core.task.test = diff --git a/test/spell_check.words b/test/spell_check.words index 6575524d..fcec3fe9 100644 --- a/test/spell_check.words +++ b/test/spell_check.words @@ -95,7 +95,9 @@ setupscript setuptools shlex sigint +sloretz stacklevel +stackoverflow staticmethod stdeb stringify @@ -122,3 +124,4 @@ unlinking unrenamed wildcards workaround +workspaces diff --git a/test/test_shell.py b/test/test_shell.py index bc529cc6..4996da49 100644 --- a/test/test_shell.py +++ b/test/test_shell.py @@ -13,11 +13,15 @@ from colcon_core.shell import create_environment_hook from colcon_core.shell import find_installed_packages from colcon_core.shell import find_installed_packages_in_environment +from colcon_core.shell import FindInstalledPackagesExtensionPoint from colcon_core.shell import get_colcon_prefix_path from colcon_core.shell import get_command_environment from colcon_core.shell import get_environment_variables +from colcon_core.shell import get_find_installed_packages_extensions from colcon_core.shell import get_shell_extensions from colcon_core.shell import ShellExtensionPoint +from colcon_core.shell.installed_packages import IsolatedInstalledPackageFinder +from colcon_core.shell.installed_packages import MergedInstalledPackageFinder import pytest from .entry_point_context import EntryPointContext @@ -312,6 +316,27 @@ def test_check_dependency_availability(): assert '--packages-ignore pkgA' in warn.call_args[0][0] +class FIExtension1(FindInstalledPackagesExtensionPoint): + PRIORITY = 90 + + +class FIExtension2(FindInstalledPackagesExtensionPoint): + pass + + +def test_get_find_installed_packages_extensions(): + with EntryPointContext(extension1=FIExtension1, extension2=FIExtension2): + extensions = get_find_installed_packages_extensions() + assert list(extensions.keys()) == [100, 90] + assert list(extensions[100].keys()) == ['extension2'] + assert list(extensions[90].keys()) == ['extension1'] + + +def test_find_installed_packages_extension_not_implemented(): + with pytest.raises(NotImplementedError): + FindInstalledPackagesExtensionPoint().find_installed_packages(Path()) + + def test_find_installed_packages_in_environment(): with TemporaryDirectory(prefix='test_colcon_') as prefix_path: prefix_path = Path(prefix_path) @@ -339,50 +364,100 @@ def test_find_installed_packages_in_environment(): def test_find_installed_packages(): - with TemporaryDirectory(prefix='test_colcon_') as install_base: - install_base = Path(install_base) - - # install base doesn't exist - assert find_installed_packages(install_base) is None - - # unknown install layout - marker_file = install_base / '.colcon_install_layout' - marker_file.write_text('unknown') - assert find_installed_packages(install_base) is None + with EntryPointContext( + colcon_isolated=IsolatedInstalledPackageFinder, + colcon_merged=MergedInstalledPackageFinder + ): + with TemporaryDirectory(prefix='test_colcon_') as install_base: + install_base = Path(install_base) - # package index directory doesn't exist - marker_file.write_text('merged') - packages = find_installed_packages(install_base) - assert len(packages) == 0 + # install base doesn't exist + assert find_installed_packages(install_base) is None - with patch( - 'colcon_core.shell.get_relative_package_index_path', - return_value=Path('relative/package/index') - ) as rel_path: - # setup for isolated case - (install_base / 'dummy_file').write_text('') - (install_base / '.hidden_dir').mkdir() - (install_base / 'dummy_dir' / rel_path() / 'dummy_dir').mkdir( - parents=True) - (install_base / 'pkgA' / rel_path()).mkdir(parents=True) - (install_base / 'pkgA' / rel_path() / 'pkgA').write_text('') - - # setup for merged case - (install_base / rel_path() / 'dummy_dir').mkdir(parents=True) - (install_base / rel_path() / '.dummy').write_text('') - (install_base / rel_path() / 'pkgB').write_text('') - (install_base / rel_path() / 'pkgC').write_text('') - - marker_file.write_text('isolated') - packages = find_installed_packages(install_base) - assert len(packages) == 1 - assert 'pkgA' in packages.keys() - assert packages['pkgA'] == install_base / 'pkgA' + # unknown install layout + marker_file = install_base / '.colcon_install_layout' + marker_file.write_text('unknown') + assert find_installed_packages(install_base) is None + # package index directory doesn't exist marker_file.write_text('merged') packages = find_installed_packages(install_base) - assert len(packages) == 2 - assert 'pkgB' in packages.keys() - assert packages['pkgC'] == install_base - assert 'pkgC' in packages.keys() - assert packages['pkgB'] == install_base + assert len(packages) == 0 + + with patch( + 'colcon_core.shell.installed_packages' + '.get_relative_package_index_path', + return_value=Path('relative/package/index') + ) as rel_path: + # setup for isolated case + (install_base / 'dummy_file').write_text('') + (install_base / '.hidden_dir').mkdir() + (install_base / 'dummy_dir' / rel_path() / 'dummy_dir').mkdir( + parents=True) + (install_base / 'pkgA' / rel_path()).mkdir(parents=True) + (install_base / 'pkgA' / rel_path() / 'pkgA').write_text('') + + # setup for merged case + (install_base / rel_path() / 'dummy_dir').mkdir(parents=True) + (install_base / rel_path() / '.dummy').write_text('') + (install_base / rel_path() / 'pkgB').write_text('') + (install_base / rel_path() / 'pkgC').write_text('') + + marker_file.write_text('isolated') + packages = find_installed_packages(install_base) + assert len(packages) == 1 + assert 'pkgA' in packages.keys() + assert packages['pkgA'] == install_base / 'pkgA' + + marker_file.write_text('merged') + packages = find_installed_packages(install_base) + assert len(packages) == 2 + assert 'pkgB' in packages.keys() + assert packages['pkgC'] == install_base + assert 'pkgC' in packages.keys() + assert packages['pkgB'] == install_base + + +class FIExtensionPathNotExist(FindInstalledPackagesExtensionPoint): + + def find_installed_packages(self, install_base: Path): + return {'pkgA': Path('/does/not/exist')} + + +def test_inconsistent_package_finding_extensions(): + with EntryPointContext(dne=FIExtensionPathNotExist): + with TemporaryDirectory(prefix='test_colcon_') as install_base: + install_base = Path(install_base) + with patch('colcon_core.shell.logger.warning') as mock_warn: + assert {} == find_installed_packages(install_base) + dne_path = Path('/does/not/exist') + mock_warn.assert_called_once_with( + "Ignoring 'pkgA' found at '{0}'" + ' because the path does not exist.'.format(dne_path)) + + +def test_find_package_two_locations(): + with TemporaryDirectory(prefix='test_colcon_') as base: + base = Path(base) + location1 = base / 'pkgA' + location2 = base / 'pkgB' + location1.mkdir() + location2.mkdir() + + class PackageLocation1(FindInstalledPackagesExtensionPoint): + + def find_installed_packages(self, base: Path): + return {'pkgA': location1} + + class PackageLocation2(FindInstalledPackagesExtensionPoint): + + def find_installed_packages(self, base: Path): + return {'pkgA': location2} + + with EntryPointContext(loc1=PackageLocation1, loc2=PackageLocation2): + with patch('colcon_core.shell.logger.warning') as mock_warn: + assert {'pkgA': location1} == find_installed_packages(base) + mock_warn.assert_called_once_with( + "The package 'pkgA' previously found at" + " '{location1}' was found again at '{location2}'." + " Ignoring '{location2}'".format_map(locals()))