Conversation
Powers0909
changed the title
goodboy
added
windows_related
IPC and transport
macos_related
There was a problem hiding this comment.
Pull request overview
This PR adds Windows compatibility fixes by conditionally disabling Unix Domain Socket (UDS) support on Windows platforms. The changes aim to make the codebase work on Windows where AF_UNIX socket support may not be available or is intentionally excluded from this project's scope.
Changes:
- Conditional UDS backend loading with platform detection across multiple modules
- SIGUSR1 signal handling made Windows-compatible in the stackscope debugging module
- Warning logs added when UDS backend is unavailable
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| tractor/ipc/_types.py | Implements conditional UDS import with HAS_UDS flag, dynamically builds transport registries based on availability |
| tractor/ipc/_server.py | Adds conditional UDS import with platform check for Windows compatibility |
| tractor/devx/_stackscope.py | Makes SIGUSR1 signal handling optional on Windows where the signal doesn't exist |
| tractor/_addr.py | Conditionally imports UDS address types and registers them only when available |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if TYPE_CHECKING: | ||
| from ._runtime import Actor | ||
|
|
There was a problem hiding this comment.
The duplicate if TYPE_CHECKING: block should be removed. There's already a TYPE_CHECKING block at line 42-43, and this duplicate at lines 62-63 is redundant.
| if TYPE_CHECKING: | |
| from ._runtime import Actor |
| import platform | ||
| import signal | ||
|
|
||
| from signal import ( |
There was a problem hiding this comment.
Trailing whitespace detected at the end of line 38. This should be removed for code cleanliness.
| from signal import ( | |
| from signal import ( |
|
|
||
| UDSAddress = None | ||
|
|
||
| if getattr(socket, "AF_UNIX", None) is not None and platform.system() != "Windows": |
There was a problem hiding this comment.
The platform detection pattern here differs from the approach used in _types.py and _addr.py. For consistency, consider defining HAS_AF_UNIX and IS_WINDOWS variables at the module level (like in the other files) and reusing them here: if HAS_AF_UNIX and not IS_WINDOWS:. This would make the codebase more maintainable and ensure consistent behavior across all Windows compatibility checks.
| case socket.AF_UNIX: | ||
| transport = 'uds' | ||
| # Only consider AF_UNIX when both Python exposes it and our backend is active | ||
| if transport is None and HAS_UDS and HAS_AF_UNIX and fam == socket.AF_UNIX: # type: ignore[attr-defined] |
There was a problem hiding this comment.
The condition checks HAS_AF_UNIX and then accesses socket.AF_UNIX, but HAS_AF_UNIX is already defined as getattr(socket, "AF_UNIX", None) is not None, so the check is redundant. Consider simplifying to just if transport is None and HAS_UDS and fam == socket.AF_UNIX: since HAS_AF_UNIX is already validated by HAS_UDS being True (line 48 ensures UDS is only enabled when HAS_AF_UNIX is True).
| if transport is None and HAS_UDS and HAS_AF_UNIX and fam == socket.AF_UNIX: # type: ignore[attr-defined] | |
| if transport is None and HAS_UDS and fam == socket.AF_UNIX: # type: ignore[attr-defined] |
| from signal import ( | ||
|
|
||
| import platform | ||
| import signal |
There was a problem hiding this comment.
Module 'signal' is imported with both 'import' and 'import from'.
| import signal |
| TYPE_CHECKING, | ||
| ) | ||
|
|
||
| import logging |
There was a problem hiding this comment.
Import of 'logging' is not used.
| import logging |
Fixes to attempt to get baseline windows goin again per our
oustandings,
Disable win tests in CI #327
Iter-related mult-platform ToDos,
macossupport,https://pikers.dev/goodboy/tractor/pulls/29