Relocate ProcessIO to ContainerClient. #681
Conversation
| ) | ||
| } | ||
|
|
||
| public static func handleProcess(io: ProcessIO, process: ClientProcess) async throws -> Int32 { |
There was a problem hiding this comment.
this becomes an instance function in ProcessIO.swift()
| ) | ||
| } | ||
|
|
||
| public func handleProcess(process: ClientProcess, log: Logger) async throws -> Int32 { |
There was a problem hiding this comment.
relocated from Application.swift:
- now an instance function so the
ioparameter just becomesself - was using the
logmember from Application before, the caller now passeslogin
| @@ -0,0 +1,381 @@ | |||
| //===----------------------------------------------------------------------===// | |||
There was a problem hiding this comment.
extracted from ContainerRun in the ContainerCommands target to this file in the ContainerClient target
| case table | ||
| } | ||
|
|
||
| static let signalSet: [Int32] = [ |
There was a problem hiding this comment.
These were only used by handleProcess so they've also moved to ProcessIO
| let stderr: Pipe? | ||
| var ioTracker: IoTracker? | ||
|
|
||
| static let signalSet: [Int32] = [ |
There was a problem hiding this comment.
moved to here from Application.swift
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
All of this is now down in ProcessIO.swift in ContainerClient.
- Makes ProcessIO more generally available. - `Application.handleProcess(processIO:process:)` becomes `processIO.handleProcess(process:log:)`.
jglogan
force-pushed
the
processio-refactor
branch
from
3c4e9b1 to
bd63ced
Compare
katiewasnothere
left a comment
katiewasnothere
left a comment
There was a problem hiding this comment.
LGTM. The annotations were very helpful, thank you for adding those :)
| ) | ||
| } | ||
|
|
||
| public func handleProcess(process: ClientProcess, log: Logger) async throws -> Int32 { |
There was a problem hiding this comment.
I like the ProcessIO move, but I'm not as big of a fan about this method on here. To me anything that would call start() or handle process lifecycle makes more sense on the ClientProcess type itself. A run() method on ClientProcess might be a decent naming, as that's essentially what this is. We start + wait + consume IO until completion.
There was a problem hiding this comment.
Thinking more about this, not entirely related but, to even be able to get a ClientProcess object we need to pass the io type into bootstrap() or createProcess(), why don't we just have ClientProcess manage the IO type directly? e.g. it can call closeAfterStart() inside the type and not burden the caller with doing it. Additionally, in process.wait() we can do the try await io.wait() just as we do in handleProcess right now. Seems like it'd clean things up a bit.
3 participants
Application.handleProcess(processIO:process:)becomesprocessIO.handleProcess(process:log:).Type of Change
Motivation and Context
This code shouldn't be in the CLI, it should be part of ContainerClient.
Testing