Home Original page

Relocate ProcessIO to ContainerClient. by jglogan · Pull Request #681 · apple/container

@jglogan

  • Makes ProcessIO more generally available.
  • Application.handleProcess(processIO:process:) becomes processIO.handleProcess(process:log:).

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Motivation and Context

This code shouldn't be in the CLI, it should be part of ContainerClient.

Testing

  • Tested locally
  • Added/updated tests
  • Added/updated docs

jglogan

)
}

public static func handleProcess(io: ProcessIO, process: ClientProcess) async throws -> Int32 {

Choose a reason for hiding this comment

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

this becomes an instance function in ProcessIO.swift()

jglogan

)
}

public func handleProcess(process: ClientProcess, log: Logger) async throws -> Int32 {

Choose a reason for hiding this comment

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

relocated from Application.swift:

  • now an instance function so the io parameter just becomes self
  • was using the log member from Application before, the caller now passes log in

jglogan

@@ -0,0 +1,381 @@
//===----------------------------------------------------------------------===//

Choose a reason for hiding this comment

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

extracted from ContainerRun in the ContainerCommands target to this file in the ContainerClient target

jglogan

case table
}

static let signalSet: [Int32] = [

Choose a reason for hiding this comment

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

These were only used by handleProcess so they've also moved to ProcessIO

jglogan

let stderr: Pipe?
var ioTracker: IoTracker?

static let signalSet: [Int32] = [

Choose a reason for hiding this comment

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

moved to here from Application.swift

jglogan

Choose a reason for hiding this comment

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

All of this is now down in ProcessIO.swift in ContainerClient.

@jglogan 

- Makes ProcessIO more generally available.
- `Application.handleProcess(processIO:process:)` becomes
  `processIO.handleProcess(process:log:)`.

katiewasnothere

Choose a reason for hiding this comment

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

LGTM. The annotations were very helpful, thank you for adding those :)

dcantah

)
}

public func handleProcess(process: ClientProcess, log: Logger) async throws -> Int32 {

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.