Warn when overriding packages at build time (#449)

* First prototype warning on build of overridden packages

Signed-off-by: Shane Loretz <[email protected]>

* Add FindInstalledPackagesExtensionPoint and improve override warning text

Signed-off-by: Shane Loretz <[email protected]>

* its -> their

Signed-off-by: Shane Loretz <[email protected]>

* Fix CI

Signed-off-by: Shane Loretz <[email protected]>

* Update docstring of find_installed_packages

Signed-off-by: Shane Loretz <[email protected]>

* Make unit tests aware of FindInstalledPackagesExtensionPoint

Signed-off-by: Shane Loretz <[email protected]>

* Fix import order

Signed-off-by: Shane Loretz <[email protected]>

* Test raises NotImplementedError

Signed-off-by: Shane Loretz <[email protected]>

* Add extend action for older python versions

Signed-off-by: Shane Loretz <[email protected]>

* Flake8 tuple whitespace

Signed-off-by: Shane Loretz <[email protected]>

* Spellcheck extra words

Signed-off-by: Shane Loretz <[email protected]>

* Make override warning clearer

Signed-off-by: Shane Loretz <[email protected]>

* Grammar in path arg documentation

Signed-off-by: Shane Loretz <[email protected]>

* Check packages is not None

Signed-off-by: Shane Loretz <[email protected]>

* Test warning when extension reports package that doesn't exist

Signed-off-by: Shane Loretz <[email protected]>

* Test two extensions finding same package in same install base

Signed-off-by: Shane Loretz <[email protected]>

* Fix expected string on windows

Signed-off-by: Shane Loretz <[email protected]>

Co-authored-by: Scott K Logan <[email protected]>

Author: sloretz

Diff for: 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 for: 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 for: 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 for: 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 for: 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