new_installer: Windows IPC support#52483
Open
johnwparent wants to merge 8 commits into
Open
Conversation
864719f to
0c77060
Compare
5a9e6e8 to
ed58f0b
Compare
ae1c104 to
6860526
Compare
haampie
reviewed
Jun 5, 2026
| """Abstract base classes for new_installer: TUI terminal state, IPC channels, and job scheduling. | ||
|
|
||
| Kept as a leaf module (no imports from new_installer.py or the platform modules) to avoid circular | ||
| dependencies — new_installer_posix and new_installer_windows both import from here.""" |
Member
There was a problem hiding this comment.
Can you do a scan over this yourself and remove Claudisms? We use ascii practically everywhere.
Contributor
Author
There was a problem hiding this comment.
Ah I absolutely forgot to check the module level docstrings, thanks.
Will also of course go over the rest of the changes as well.
haampie
reviewed
Jun 5, 2026
Comment on lines
+2095
to
+2098
| # group conditions for readability | ||
| wake_on_jobserver = ( | ||
| self.pending_builds | ||
| and self.capacity | ||
| and not blocked | ||
| or not jobserver.has_target_parallelism() | ||
| ) | ||
| if wake_on_jobserver and jobserver.r not in selector.get_map(): | ||
| selector.register(jobserver.r, selectors.EVENT_READ, "jobserver") | ||
| elif not wake_on_jobserver and jobserver.r in selector.get_map(): | ||
| selector.unregister(jobserver.r) | ||
| self.pending_builds and self.capacity and not blocked | ||
| ) or not jobserver.has_target_parallelism() |
Member
There was a problem hiding this comment.
Can you please leave out every unnecessary change in a 1000 line diff? This is not relevant to your changes, so leave it like it was. It's doesn't improve readability to me, and just takes more time in review.
Contributor
Author
There was a problem hiding this comment.
Ok, I did make this change deliberately because I thought it made it more readable but I'll revert since you're generally more right about these things
Member
There was a problem hiding this comment.
Well, it's personal taste and it could've been either, for this PR it's just additional things to double-check.
6860526 to
e9a55d0
Compare
Facillitates IPC on Windows with the new installer via sockets Breaks ChildInfo down into subclasses, with the posix side untouched and the windows side output/state use socket.socketpair(). Registers socket objects and a WindowsSentinelBridge. WindowsSentinelBridge wraps a thread that calls proc.join() and writes a byte to a socket to wake the selector when the process exits (Windows cannot register process sentinels with a selector directly). read_from_key() abstracts reads across fds and sockets: uses recv() when the selector key's fileobj has it, os.read() otherwise. This lets _handle_child_logs/state use one invocation for both platforms. tee() is split into _tee_posix() (same as before) and _tee_windows() (thread-based control reader using sockets). State buffer and decoder refactored to be keyed by pid rather than fd, and _handle_child_state extracts _process_state_string() for clarity. Output is processed to support multibyte chars JobServer is essentially disabled on Windows Signed-off-by: John Parent <john.parent@kitware.com>
Move base classes to leaf module for per platform deriviation Signed-off-by: John Parent <john.parent@kitware.com>
No functional changes, just code reoganization Signed-off-by: John Parent <john.parent@kitware.com>
…dule Subclasses shared IPC logic with windows specific implementations Signed-off-by: John Parent <john.parent@kitware.com>
Alias classes to avoid dispatch methods remove IS_WINDOWs usages Signed-off-by: John Parent <john.parent@kitware.com>
Signed-off-by: John Parent <john.parent@kitware.com>
e9a55d0 to
427f44b
Compare
Signed-off-by: John Parent <john.parent@kitware.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on #52482
Facillitates IPC on Windows with the new installer via sockets
Breaks ChildInfo down into subclasses, with the posix side untouched and the windows side output/state use socket.socketpair().
Registers socket objects and a WindowsSentinelBridge.
WindowsSentinelBridge wraps a thread that calls proc.join() and writes a byte to a socket to wake the selector when the process exits (Windows cannot register process sentinels with a selector directly).
read_from_key() abstracts reads across fds and sockets: uses recv() when the selector key's fileobj has it, os.read() otherwise. This lets _handle_child_logs/state use one invocation for both platforms.
tee() is split into _tee_posix() (same as before) and _tee_windows() (thread-based control reader using sockets).
State buffer and decoder refactored to be keyed by pid rather than fd, and _handle_child_state extracts _process_state_string() for clarity.
Output is processed to support multibyte chars
JobServer is essentially disabled on Windows (to be done in later work, likely post release unless we get this in quickly somehow)