Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 160 additions & 0 deletions colcon_core/python_project/hook_caller/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
# Copyright 2022 Open Source Robotics Foundation, Inc.
# Licensed under the Apache License, Version 2.0

import asyncio
from contextlib import AbstractContextManager
import json
import os
import sys

from colcon_core.python_project.hook_caller import _call_hook
from colcon_core.python_project.hook_caller import _list_hooks
from colcon_core.python_project.spec import load_and_cache_spec
from colcon_core.subprocess import run


class _SubprocessTransport(AbstractContextManager):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about the _SubprocessTransport class here. Manually handling low-level OS pipes and platform-specific inheritance logic (like msvcrt) can be brittle. In colcon_core/subprocess.py the subprocess management is done using asyncio.create_subprocess_exec if I'm not wrong. Wouldn't be posible to simplify the IPC by using asyncio? Leveraging asyncio.create_subprocess_exec with stdin=asyncio.subprocess.PIPE and stdout=asyncio.subprocess.PIPE directly could align better with existing implementation and address the possible deadlock concern in one of the next comments.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great question. The problem with using stdin/stdout is that PEP 517 doesn't forbid backends from using stdout for normal console messages, so we can't use it as a data pipeline. The purpose of this class is really just to set up additional pipes which behave very similar to stdin/stdout already do, but with a dedicated purpose.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. Thank you for the clarification.


def __enter__(self):
self.child_in, self.parent_out = os.pipe()
self.parent_in, self.child_out = os.pipe()

try:
import msvcrt
except ImportError:
os.set_inheritable(self.child_in, True)
self.pass_in = self.child_in
os.set_inheritable(self.child_out, True)
self.pass_out = self.child_out
else:
self.pass_in = msvcrt.get_osfhandle(self.child_in)
os.set_handle_inheritable(self.pass_in, True)
self.pass_out = msvcrt.get_osfhandle(self.child_out)
os.set_handle_inheritable(self.pass_out, True)

return self

def __exit__(self, exc_type, exc_value, traceback):
os.close(self.parent_out)
os.close(self.parent_in)
os.close(self.child_out)
os.close(self.child_in)


class AsyncHookCaller:
"""Calls PEP 517 style hooks asynchronously in a new process."""

def __init__(
self, backend_name, *, project_path=None, env=None,
stdout_callback=None, stderr_callback=None,
):
"""
Initialize a new AsyncHookCaller.

:param backend_name: The name of the PEP 517 build backend.
:param project_path: Path to the project's root directory.
:param env: Environment variables to use when invoking hooks.
:param stdout_callback: Callback for stdout from the hook invocation.
:param stderr_callback: Callback for stderr from the hook invocation.
"""
self._backend_name = backend_name
self._project_path = str(project_path) if project_path else None
self._env = dict(env if env is not None else os.environ)
self._stdout_callback = stdout_callback
self._stderr_callback = stderr_callback

@property
def backend_name(self):
"""Get the name of the backend to call hooks on."""
return self._backend_name

@property
def env(self):
"""Get the environment variables to use when invoking hooks."""
return self._env

async def list_hooks(self):
"""
Call into the backend to list implemented hooks.

This function lists all callable methods on the backend, which may
include more than just the hook names.

:returns: List of hook names.
"""
args = [
sys.executable, _list_hooks.__file__,
self._backend_name]
process = await run(
args, None, self._stderr_callback,
cwd=self._project_path, env=self.env,
capture_output=True)
process.check_returncode()
hook_names = [
line.strip().decode() for line in process.stdout.splitlines()]
return [
hook for hook in hook_names if hook and not hook.startswith('_')]

async def call_hook(self, hook_name, **kwargs):
"""
Call the given hook with given arguments.

:param hook_name: Name of the hook to call.
"""
with _SubprocessTransport() as transport:
args = [
sys.executable, _call_hook.__file__,
self._backend_name, hook_name,
str(transport.pass_in), str(transport.pass_out)]

def write_args():
with os.fdopen(
os.dup(transport.parent_out), 'w',
encoding='utf-8',
) as f:
f.write(json.dumps(kwargs) + '\n')

# Start writing in a background thread to prevent deadlocks
# when the argument payload is larger than the pipe buffer size.
loop = asyncio.get_event_loop()
write_task = loop.run_in_executor(None, write_args)

have_callbacks = self._stdout_callback or self._stderr_callback
process = await run(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question about the performance of the process-per-hook design, specifically in the new AsyncHookCaller.call_hook method in colcon_core/python_project/hook_caller/__init__.py.
The pattern of spawning a new process for isolation is similar, as far as I understand, to how colcon already handles setup.py. However, PEP 517 build can involve more, smaller hook calls per package.
Given that the number of subprocess invocations per package could increase, is there any concern this might become a noticeable performance issue in very large workspaces? Just curious about your thoughts on the scalability of this pattern.

@cottsay cottsay Jun 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is absolutely a problem, and one of my biggest critiques of PEP 517. In the worst case, we may actually need to build a project just to determine what the name of the package is. :(

Still, a worst-case scenario will only make three subprocess invocations during a typical colcon operation:

  1. During package identification
  2. During package build to detect if editable installs can be performed by the backend
  3. During package build to actually build the wheel

In reality, we'll rarely do (1). For ROS packages, we'll have package.xml to facilitate metadata discovery. For (2), we could probably move to a fallback-style behavior where an editable install is attempted without first looking for the hooks and then ensure that it's communicated properly back to colcon that the hook failed because it doesn't exist, so that colcon can fall back to a traditional wheel instead of an editable one.

tl;dr - process invocations are indeed slow, but I think we can mitigate the fallout. In the end, it's more important to conform to PEP 517:

Frontends should call each hook in a fresh subprocess, so that backends are free to change process global state (such as environment variables or the working directory). A Python library will be provided which frontends can use to easily call hooks this way.

args, self._stdout_callback, self._stderr_callback,
cwd=self._project_path, env=self.env, close_fds=False,
capture_output=not have_callbacks)
process.check_returncode()

# Ensure the write has completed and propagate any write exceptions
await write_task

with os.fdopen(
os.dup(transport.parent_in), 'r',
encoding='utf-8',
) as f:
res = json.loads(f.readline())
return res


def get_hook_caller(desc, **kwargs):
"""
Create a new AsyncHookCaller instance for a package descriptor.

:param desc: The package descriptor
"""
spec = load_and_cache_spec(desc)
backend_path = spec['build-system'].get('backend-path')
if backend_path:
# TODO: This isn't *technically* the beginning of sys.path
# as PEP 517 calls for, but it's pretty darn close.
kwargs['env'] = {
**kwargs.get('env', os.environ),
'PYTHONDONTWRITEBYTECODE': '1',
}
pythonpath = kwargs['env'].get('PYTHONPATH', '')
kwargs['env']['PYTHONPATH'] = os.pathsep.join(
backend_path + ([pythonpath] if pythonpath else []))
return AsyncHookCaller(
spec['build-system']['build-backend'],
project_path=desc.path, **kwargs)
29 changes: 29 additions & 0 deletions colcon_core/python_project/hook_caller/_call_hook.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Copyright 2022 Open Source Robotics Foundation, Inc.
# Licensed under the Apache License, Version 2.0

from importlib import import_module
import json
import os
import sys


if __name__ == '__main__':
backend_name, hook_name, child_in, child_out = sys.argv[1:]
try:
import msvcrt
except ImportError:
pass
else:
child_in = msvcrt.open_osfhandle(int(child_in), os.O_RDONLY)
child_out = msvcrt.open_osfhandle(int(child_out), 0)
if ':' in backend_name:
backend_module_name, backend_object_name = backend_name.split(':', 2)
backend_module = import_module(backend_module_name)
backend = getattr(backend_module, backend_object_name)
else:
backend = import_module(backend_name)
with os.fdopen(int(child_in), 'r', encoding='utf-8') as f:
kwargs = json.loads(f.readline()) or {}
res = getattr(backend, hook_name)(**kwargs)
with os.fdopen(int(child_out), 'w', encoding='utf-8') as f:
f.write(json.dumps(res) + '\n')
19 changes: 19 additions & 0 deletions colcon_core/python_project/hook_caller/_list_hooks.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Copyright 2023 Open Source Robotics Foundation, Inc.
# Licensed under the Apache License, Version 2.0

from importlib import import_module
import sys


if __name__ == '__main__':
backend_name = sys.argv[1]
if ':' in backend_name:
backend_module_name, backend_object_name = backend_name.split(':', 2)
backend_module = import_module(backend_module_name)
backend = getattr(backend_module, backend_object_name)
else:
backend = import_module(backend_name)

for attr in dir(backend):
if callable(getattr(backend, attr)):
print(attr)
93 changes: 93 additions & 0 deletions colcon_core/python_project/hook_caller_decorator/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
# Copyright 2022 Open Source Robotics Foundation, Inc.
# Licensed under the Apache License, Version 2.0

import traceback

from colcon_core.logging import colcon_logger
from colcon_core.plugin_system import instantiate_extensions
from colcon_core.plugin_system import order_extensions_by_priority
from colcon_core.python_project.hook_caller import get_hook_caller

logger = colcon_logger.getChild(__name__)


class HookCallerDecoratorExtensionPoint:
"""
The interface for PEP 517 hook caller decorator extensions.

For each instance the attribute `HOOK_CALLER_DECORATOR_NAME` is being
set to the basename of the entry point registering the extension.
"""

"""The version of the hook caller decorator extension interface."""
EXTENSION_POINT_VERSION = '1.0'

"""The default priority of hook caller decorator extensions."""
PRIORITY = 100

def decorate_hook_caller(self, *, hook_caller):
"""
Decorate a hook caller to perform additional functionality.

This method must be overridden in a subclass.

:param hook_caller: The hook caller
:returns: A decorator
"""
raise NotImplementedError()


def get_hook_caller_extensions():
"""
Get the available hook caller decorator extensions.

The extensions are ordered by their priority and entry point name.

:rtype: OrderedDict
"""
extensions = instantiate_extensions(__name__)
for name, extension in extensions.items():
extension.HOOK_CALLER_DECORATOR_NAME = name
return order_extensions_by_priority(extensions)


def decorate_hook_caller(hook_caller):
"""
Decorate the hook caller using hook caller decorator extensions.

:param hook_caller: The hook caller

:returns: The decorated parser
"""
extensions = get_hook_caller_extensions()
for extension in extensions.values():
logger.log(
1, 'decorate_hook_caller() %s',
extension.HOOK_CALLER_DECORATOR_NAME)
try:
decorated_hook_caller = extension.decorate_hook_caller(
hook_caller=hook_caller)
assert hasattr(decorated_hook_caller, 'call_hook'), \
'decorate_hook_caller() should return something to call hooks'
except Exception as e: # noqa: F841
# catch exceptions raised in decorator extension
exc = traceback.format_exc()
logger.error(
'Exception in hook caller decorator extension '
f"'{extension.HOOK_CALLER_DECORATOR_NAME}': {e}\n{exc}")
# skip failing extension, continue with next one
else:
hook_caller = decorated_hook_caller

return hook_caller


def get_decorated_hook_caller(desc, **kwargs):
"""
Create and decorate a hook caller instance for a package descriptor.

:param desc: The package descriptor
"""
hook_caller = get_hook_caller(desc, **kwargs)
decorated_hook_caller = decorate_hook_caller(hook_caller)
return decorated_hook_caller
2 changes: 2 additions & 0 deletions test/spell_check.words
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ argparse
asyncio
autouse
backend
backends
backported
bazqux
blocklist
Expand Down Expand Up @@ -75,6 +76,7 @@ noqa
notestscollected
openpty
optionxform
osfhandle
pathlib
pkgname
pkgs
Expand Down
4 changes: 3 additions & 1 deletion test/test_flake8.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ def test_flake8():
show_source=True,
)
style_guide_tests = get_style_guide(
extend_ignore=['D100', 'D101', 'D102', 'D103', 'D104', 'D105', 'D107'],
extend_ignore=[
'D100', 'D101', 'D102', 'D103', 'D104', 'D105', 'D106', 'D107',
],
Comment on lines +28 to +30

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added D106 to this list. Evidently we've never had a nested class definition in the test sources, but it makes sense to suppress the documentation requirement there as well.

show_source=True,
)

Expand Down
Loading
Loading