Implement interface for calling PEP 517 hooks by cottsay · Pull Request #732 · colcon/colcon-core

cottsay · 2026-04-01T20:17:16Z

This implementation is copied nearly verbatim from the prototype colcon_python_project package.

Here's the spec for PEP 517: https://peps.python.org/pep-0517/

This is the central component for enabling colcon to act as a "build frontend" and the Python package build process will be built around this infrastructure. Additionally, non-PEP 518 packages will extract package metadata during the identification and augmentation phases of the colcon workflow.

To help visualize where this fits in the overall standards-based Python build picture, this hook caller interface will invoke the appropriate backend hook (e.x. build_wheel) for a package to build a wheel file, and then colcon will install that wheel (after uninstalling any previous wheels for that package) into the install space.

The decoration infrastructure allows us to add backend-specific augmentations to hook invocations. Though this interface should allow colcon to support arbitrary backends, we'll implement decorators for "helping" the backends to support broader scenarios (like symlink installs) and optimize the overall process. For example, we'll need to augment setuptools builds in order to maintain feature parity with colcon's existing setuptools integration. We'll also use that decorator to move temporary artifacts out of the package source directory.

This implementation is copied nearly verbatim from the prototype colcon_python_project package.

codecov · 2026-04-01T20:19:14Z

Codecov Report

❌ Patch coverage is 95.00000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.42%. Comparing base (0c9f39c) to head (0727c07).
⚠️ Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
...e/python_project/hook_caller_decorator/__init__.py	79.41%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #732      +/-   ##
==========================================
+ Coverage   87.21%   87.42%   +0.21%     
==========================================
  Files          69       73       +4     
  Lines        4106     4270     +164     
  Branches      709      726      +17     
==========================================
+ Hits         3581     3733     +152     
- Misses        414      425      +11     
- Partials      111      112       +1

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

cottsay · 2026-04-01T20:20:16Z

+        extend_ignore=[
+            'D100', 'D101', 'D102', 'D103', 'D104', 'D105', 'D106', 'D107',
+        ],


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.

KmoM88 · 2026-05-19T13:07:08Z

This PR provides the low-level AsyncHookCaller tool. For the high-level integration, is the plan to develop PEP 517 package support within the colcon-python-project repository, making it a functional extension similar to colcon-python-setup-py?

KmoM88

LGTM overall. This are just some comments to discuss and get more context about this feature. Great Job!

KmoM88 · 2026-05-18T17:08:46Z

+            with os.fdopen(os.dup(transport.parent_out), 'wb') as f:
+                pickle.dump(kwargs, f)
+            have_callbacks = self._stdout_callback or self._stderr_callback
+            process = await run(


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.

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:

During package identification

During package build to detect if editable installs can be performed by the backend

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.

KmoM88 · 2026-05-18T18:28:06Z

+            with os.fdopen(os.dup(transport.parent_out), 'wb') as f:
+                pickle.dump(kwargs, f)
+            have_callbacks = self._stdout_callback or self._stderr_callback
+            process = await run(
+                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()
+            with os.fdopen(os.dup(transport.parent_in), 'rb') as f:
+                res = pickle.load(f)


I think there is a potential deadlock issue here related to IPC handling within the event loop.

Because pickle.dump is synchronous, blocking call, writing to the file descriptor can block the entire event loop if the payload exceeds the OS pipe buffer.

While this might not affect basic use cases with small payloads, it is possible to freeze in complex bigger scenarios (large dependency list or extensive config_settings).

Has a fully non-blocking IPC approach using asyncio streams been considered to make this more robust for advanced use cases? Maybe this is over-optimizing and can be considered if an issue is found and asyncio would remove the low level OS pipes from the previous comment.

Hmm, that's a good point. I see some options to consider:

Do the synchronous pickling separately from writing to the socket, the latter of which could be done asynchronously

Move the pickle-and-write operation to happen after we attempt to spawn the subprocess

Put a BufferedWriter between the pickle operation and the pipe to the subprocess.

Thoughts?

After moving from Pickle to JSON, the read/write operations got separated from the (de)serialization anyway, so I went ahead and did (1) in 70c8019.

Oof, actually it looks like that won't work on Windows. Reverted.

I just pushed the write to a background thread in 0727c07, which should improve this scenario.

KmoM88 · 2026-05-18T18:53:45Z

+from colcon_core.subprocess import run
+
+
+class _SubprocessTransport(AbstractContextManager):


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.

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.

Oh, I see. Thank you for the clarification.

KmoM88 · 2026-05-18T20:42:28Z

+                capture_output=not have_callbacks)
+            process.check_returncode()
+            with os.fdopen(os.dup(transport.parent_in), 'rb') as f:
+                res = pickle.load(f)


About the use of pickle for deserializing the hook's return value. I understand that PEP 517 operates on a trust model, but there is still a potential for remote code execution if a compromised build backend returns a malicious payload. Given that the standard PEP 517 hooks return simple, serializable data types (like strings and lists), would it be an improvement to use a safer serialization format?
Using the json library would completely mitigate this risk without adding new dependencies. Alternatively, if more complex objects are a future concern, a library like jsonpickle could also work (though it would add a new dependency). This might be a great way to make the IPC mechanism even more robust. What are your thoughts on this? Is something we should worry about at the moment?

Looking at the hooks defined today, they all seem to accept and return only Python primitives. I'm willing to bet that Pickle will be more performant, but we can't limit Pickle to only Python primitives, so switching to another serializer will give us that security (indirectly).

I'll give JSON a shot.

Updated to use JSON in fb31156. It was a pretty clean change.

cottsay

Thanks for giving this such a thorough look!

cottsay · 2026-06-12T20:12:04Z

+from colcon_core.subprocess import run
+
+
+class _SubprocessTransport(AbstractContextManager):


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.

cottsay · 2026-06-12T20:26:11Z

+            with os.fdopen(os.dup(transport.parent_out), 'wb') as f:
+                pickle.dump(kwargs, f)
+            have_callbacks = self._stdout_callback or self._stderr_callback
+            process = await run(


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:

During package identification

During package build to detect if editable installs can be performed by the backend

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.

cottsay · 2026-06-12T20:49:01Z

+            with os.fdopen(os.dup(transport.parent_out), 'wb') as f:
+                pickle.dump(kwargs, f)
+            have_callbacks = self._stdout_callback or self._stderr_callback
+            process = await run(
+                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()
+            with os.fdopen(os.dup(transport.parent_in), 'rb') as f:
+                res = pickle.load(f)


Hmm, that's a good point. I see some options to consider:

Do the synchronous pickling separately from writing to the socket, the latter of which could be done asynchronously

Move the pickle-and-write operation to happen after we attempt to spawn the subprocess

Put a BufferedWriter between the pickle operation and the pipe to the subprocess.

Thoughts?

cottsay · 2026-06-12T20:55:18Z

+                capture_output=not have_callbacks)
+            process.check_returncode()
+            with os.fdopen(os.dup(transport.parent_in), 'rb') as f:
+                res = pickle.load(f)


Looking at the hooks defined today, they all seem to accept and return only Python primitives. I'm willing to bet that Pickle will be more performant, but we can't limit Pickle to only Python primitives, so switching to another serializer will give us that security (indirectly).

I'll give JSON a shot.

We only need to send primitive Python types across this channel today, so JSON should be safer (though less efficient). Assisted-by: Gemini 3.5 Flash

Turns out asynchronous I/O on anonymous pipes just isn't possible on Windows. This reverts commit 70c8019.

Works around the possible deadlock when the pipe buffer gets filled before the consuming process is started.

KmoM88

Thanks so much for your patience and for being so thorough in addressing all the feedback.
No more comments on my side, LGTM. Happy to approve and continue with next phases to adopt PEP 517.

This implementation is copied nearly verbatim from the prototype colcon_python_project package.

Implement interface for calling PEP 517 hooks

6798cd6

This implementation is copied nearly verbatim from the prototype colcon_python_project package.

cottsay self-assigned this Apr 1, 2026

cottsay added the enhancement New feature or request label Apr 1, 2026

cottsay commented Apr 1, 2026

View reviewed changes

KmoM88 reviewed May 19, 2026

View reviewed changes

Merge branch 'master' into cottsay/hook_caller

06fce2a

KmoM88 mentioned this pull request Jun 3, 2026

Proposal: Migration of colcon-core to PEP 517/518/621/660 Standards #736

Open

cottsay commented Jun 12, 2026

View reviewed changes

cottsay added 4 commits June 12, 2026 16:30

Switch from pickle to JSON

fb31156

We only need to send primitive Python types across this channel today, so JSON should be safer (though less efficient). Assisted-by: Gemini 3.5 Flash

Make subprocess transport asynchronous

70c8019

Revert "Make subprocess transport asynchronous"

b263c42

Turns out asynchronous I/O on anonymous pipes just isn't possible on Windows. This reverts commit 70c8019.

Push argument writing to background thread

0727c07

Works around the possible deadlock when the pipe buffer gets filled before the consuming process is started.

KmoM88 approved these changes Jun 16, 2026

View reviewed changes

cottsay merged commit 5a295ca into master Jun 16, 2026
46 checks passed

cottsay deleted the cottsay/hook_caller branch June 16, 2026 16:23

cottsay added this to the 0.21.1 milestone Jun 16, 2026

LeroyR pushed a commit to CentralLabFacilities/colcon-core that referenced this pull request Jun 25, 2026

Implement interface for calling PEP 517 hooks (colcon#732)

a4b3e7b

This implementation is copied nearly verbatim from the prototype colcon_python_project package.

		from colcon_core.subprocess import run


		class _SubprocessTransport(AbstractContextManager):

Uh oh!

Conversation

cottsay commented Apr 1, 2026

Uh oh!

codecov Bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KmoM88 commented May 19, 2026

Uh oh!

KmoM88 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cottsay Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cottsay Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cottsay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cottsay Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KmoM88 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Apr 1, 2026 •

edited

Loading

cottsay Jun 12, 2026 •

edited

Loading

cottsay Jun 12, 2026 •

edited

Loading

cottsay Jun 12, 2026 •

edited

Loading