Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor utilities under Foundation.Process and CartonHelpers.Process and share implementations #477

Merged
merged 5 commits into from
Jun 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions Plugins/CartonBundlePlugin/CartonBundlePluginCommand.swift
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,6 @@ struct CartonBundlePluginCommand: CommandPlugin {
["--resources", $0.string]
} + extractor.remainingArguments
let frontend = try makeCartonFrontendProcess(context: context, arguments: frontendArguments)
frontend.forwardTerminationSignals()
try frontend.run()
frontend.waitUntilExit()
if frontend.terminationStatus == 0 {
print("Bundle written in \(bundleDirectory)")
}
frontend.checkNonZeroExit()
try frontend.checkRun(printsLoadingMessage: false, forwardExit: true)
}
}
2 changes: 1 addition & 1 deletion Plugins/CartonDevPlugin/CartonDevPluginCommand.swift
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ struct CartonDevPluginCommand: CommandPlugin {
}

frontend.waitUntilExit()
frontend.checkNonZeroExit()
frontend.forwardExit()
}

private func defaultProduct(context: PluginContext) throws -> String {
Expand Down
38 changes: 1 addition & 37 deletions Plugins/CartonPluginShared/PluginShared.swift
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,7 @@ internal func checkHelpFlag(_ arguments: [String], subcommand: String, context:
if arguments.contains("--help") || arguments.contains("-h") {
let frontend = try makeCartonFrontendProcess(
context: context, arguments: [subcommand, "--help"])
frontend.forwardTerminationSignals()
try frontend.run()
frontend.waitUntilExit()
exit(frontend.terminationStatus)
try frontend.checkRun(printsLoadingMessage: false, forwardExit: true)
}
}

Expand All @@ -203,36 +200,3 @@ internal func makeCartonFrontendProcess(context: PluginContext, arguments: [Stri
process.arguments = arguments
return process
}

internal func runCartonFrontend(context: PluginContext, arguments: [String]) throws -> Process {
let process = try makeCartonFrontendProcess(context: context, arguments: arguments)
try process.run()
return process
}

extension Process {
internal func forwardTerminationSignals() {
// Monitor termination/interrruption signals to forward them to child process
func setSignalForwarding(_ signalNo: Int32) {
signal(signalNo, SIG_IGN)
let signalSource = DispatchSource.makeSignalSource(signal: signalNo)
signalSource.setEventHandler {
signalSource.cancel()
self.interrupt()
}
signalSource.resume()
}
setSignalForwarding(SIGINT)
setSignalForwarding(SIGTERM)

self.terminationHandler = {
// Exit plugin process itself when child process exited
exit($0.terminationStatus)
}
}
internal func checkNonZeroExit() {
if terminationStatus != 0 {
exit(terminationStatus)
}
}
}
5 changes: 1 addition & 4 deletions Plugins/CartonTestPlugin/CartonTestPluginCommand.swift
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,7 @@ struct CartonTestPluginCommand: CommandPlugin {
["--resources", $0.string]
} + extractor.remainingArguments
let frontend = try makeCartonFrontendProcess(context: context, arguments: frontendArguments)
frontend.forwardTerminationSignals()
try frontend.run()
frontend.waitUntilExit()
frontend.checkNonZeroExit()
try frontend.checkRun(printsLoadingMessage: false, forwardExit: true)
}

private func buildDirectory(context: PluginContext) throws -> Path {
Expand Down
6 changes: 6 additions & 0 deletions Sources/CartonCore/CartonCoreError.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
struct CartonCoreError: Error & CustomStringConvertible {
init(_ description: String) {
self.description = description
}
var description: String
}
85 changes: 85 additions & 0 deletions Sources/CartonCore/FoundationProcessEx.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import Foundation

extension Foundation.Process {
// Monitor termination/interrruption signals to forward them to child process
public func setSignalForwarding(_ signalNo: Int32) {
signal(signalNo, SIG_IGN)
let signalSource = DispatchSource.makeSignalSource(signal: signalNo)
signalSource.setEventHandler { [self] in
signalSource.cancel()
kill(processIdentifier, signalNo)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ここが元々 interrupt 固定になっていました。

}
signalSource.resume()
}

public func forwardTerminationSignals() {
setSignalForwarding(SIGINT)
setSignalForwarding(SIGTERM)
}

public var commandLine: String {
get throws {
guard let executableURL else {
throw CartonCoreError("executableURL is none")
}

let commandLineArgs: [String] = [
executableURL.path
] + (arguments ?? [])

let q = "\""
let commandLine: String = commandLineArgs
.map { "\(q)\($0)\(q)" }
.joined(separator: " ")

return commandLine
}
}

public func checkRun(
printsLoadingMessage: Bool,
forwardExit: Bool = false
) throws {
if printsLoadingMessage {
print("Running \(try commandLine)")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ここですが、元コードだと stderr に出力して flush していたのですが、
普通に Swift.print で良いと思いました。
frontendの他の部分でも進行状況メッセージは stdout に出ていますし。

}

try run()
forwardTerminationSignals()
waitUntilExit()

if forwardExit {
self.forwardExit()
}

try checkNonZeroExit()
}

public func forwardExit() {
exit(terminationStatus)
}

public func checkNonZeroExit() throws {
if terminationStatus != 0 {
throw CartonCoreError(
"Process failed with status \(terminationStatus).\n" +
"Command line: \(try commandLine)"
)
}
}

public static func checkRun(
_ executableURL: URL,
arguments: [String],
printsLoadingMessage: Bool = true,
forwardExit: Bool = false
) throws {
let process = Foundation.Process()
process.executableURL = executableURL
process.arguments = arguments
try process.checkRun(
printsLoadingMessage: printsLoadingMessage,
forwardExit: forwardExit
)
}
}
52 changes: 7 additions & 45 deletions Sources/CartonDriver/CartonDriverCommand.swift
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
//
// This executable should be eventually removed once SwiftPM provides a way to express those requirements.

import CartonCore
import CartonHelpers
import Foundation
import SwiftToolchain
Expand All @@ -41,49 +42,6 @@ struct CartonDriverError: Error & CustomStringConvertible {
var description: String
}

extension Foundation.Process {
internal static func checkRun(
_ executableURL: URL, arguments: [String], forwardExit: Bool = false
) throws {
let commandLine: String = ([executableURL.path] + arguments)
.map { "\"\($0)\"" }.joined(separator: " ")

fputs("Running \(commandLine)\n", stderr)
fflush(stderr)

let process = Foundation.Process()
process.executableURL = executableURL
process.arguments = arguments

// Monitor termination/interrruption signals to forward them to child process
func setSignalForwarding(_ signalNo: Int32) {
signal(signalNo, SIG_IGN)
let signalSource = DispatchSource.makeSignalSource(signal: signalNo)
signalSource.setEventHandler {
signalSource.cancel()
process.interrupt()
}
signalSource.resume()
}
setSignalForwarding(SIGINT)
setSignalForwarding(SIGTERM)

try process.run()
process.waitUntilExit()

if forwardExit {
exit(process.terminationStatus)
}

if process.terminationStatus != 0 {
throw CartonDriverError(
"Process failed with status \(process.terminationStatus).\n" +
"Command line: \(commandLine)"
)
}
}
}

func derivePackageCommandArguments(
swiftExec: URL,
subcommand: String,
Expand Down Expand Up @@ -184,7 +142,10 @@ func pluginSubcommand(subcommand: String, argv0: String, arguments: [String]) as
extraArguments: extraArguments
)

try Foundation.Process.checkRun(swiftExec, arguments: pluginArguments, forwardExit: true)
try Foundation.Process.checkRun(
swiftExec, arguments: pluginArguments,
forwardExit: true
)
}

public func main(arguments: [String]) async throws {
Expand Down Expand Up @@ -213,7 +174,8 @@ public func main(arguments: [String]) async throws {
let (swiftPath, _) = try await toolchainSystem.inferSwiftPath(terminal)
try Foundation.Process.checkRun(
URL(fileURLWithPath: swiftPath.pathString),
arguments: ["package"] + arguments.dropFirst(), forwardExit: true
arguments: ["package"] + arguments.dropFirst(),
forwardExit: true
)
case "--version":
print(cartonVersion)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ struct CartonFrontendBundleCommand: AsyncParsableCommand {
topLevelResourcePaths: resources
)

terminal.write("Bundle generation finished successfully\n", inColor: .green, bold: true)
Copy link
Contributor Author

@omochi omochi Jun 4, 2024

Choose a reason for hiding this comment

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

このメッセージをfrontendで出した後で、plugin側で改めてパスを出していたため、以下のようなメッセージになっていました。

...
Bundle generation finished successfully
Bundle written in Bundle

これを統合し、frontend側で成功メッセージと配置したパスを表示するようにして、
plugin側のメッセージを出すのはやめました。
以下のような出力になります。

...
Building for debugging...
[386/386] Linking carton-frontend
Build complete! (32.48s)
Building "app"
Right after building the main binary size is 7.67 MB

After stripping debug info the main binary size is 5.49 MB


Running...
wasm-opt -Os --enable-bulk-memory /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/SandboxApp/Bundle/main.wasm -o /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/SandboxApp/Bundle/main.wasm

`wasm-opt` process finished successfully
After stripping debug info the main binary size is 3.49 MB

Copying resources to /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/SandboxApp/Bundle/JavaScriptKit_JavaScriptKit.resources
Copying resources to /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/SandboxApp/Bundle/DevServerTestApp_app.resources
Creating symlink /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/SandboxApp/Bundle/style.css
Bundle successfully generated at /Users/omochi/github/swiftwasm/carton/Tests/Fixtures/SandboxApp/Bundle

ファイルパスはフルで出るようにしました。

この修正に伴い、didExitコールバックは消しました。

terminal.write("Bundle successfully generated at \(bundleDirectory)\n", inColor: .green, bold: true)
}

func optimize(_ inputPath: AbsolutePath, outputPath: AbsolutePath, terminal: InteractiveWriter)
Expand Down
28 changes: 28 additions & 0 deletions Sources/CartonHelpers/ProcessEx.swift
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import Foundation
import Dispatch

extension ProcessResult {
public mutating func setOutput(_ value: Result<[UInt8], any Swift.Error>) {
self = ProcessResult(
Expand All @@ -17,3 +20,28 @@ extension ProcessResult {
return try utf8Output()
}
}

@discardableResult
private func osSignal(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Process.signal メソッドと Darwin.signal が被るので、
リネームのためにラッパーを置きます。
ターゲットによって Darwin. じゃなかったりするため。

_ sig: Int32,
_ fn: (@convention(c) (Int32) -> Void)?
) -> (@convention(c) (Int32) -> Void)? {
signal(sig, fn)
}

extension Process {
public func setSignalForwarding(_ signalNo: Int32) {
osSignal(signalNo, SIG_IGN)
let signalSource = DispatchSource.makeSignalSource(signal: signalNo)
signalSource.setEventHandler {
signalSource.cancel()
self.signal(signalNo)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ここが元々 signal(SIGINT) 固定になっていました。

}
signalSource.resume()
}

public func forwardTerminationSignals() {
setSignalForwarding(SIGINT)
setSignalForwarding(SIGTERM)
}
}
14 changes: 2 additions & 12 deletions Tests/CartonCommandTests/CommandTestHelper.swift
Original file line number Diff line number Diff line change
Expand Up @@ -100,17 +100,7 @@ func swiftRunProcess(

try process.launch()

func setSignalForwarding(_ signalNo: Int32) {
signal(signalNo, SIG_IGN)
let signalSource = DispatchSource.makeSignalSource(signal: signalNo)
signalSource.setEventHandler {
signalSource.cancel()
process.signal(SIGINT)
}
signalSource.resume()
}
setSignalForwarding(SIGINT)
setSignalForwarding(SIGTERM)
process.forwardTerminationSignals()

return SwiftRunProcess(
process: process,
Expand All @@ -137,7 +127,7 @@ func fetchWebContent(at url: URL, timeout: Duration) async throws -> (response:
)

let (body, response) = try await session.data(for: request)

guard let response = response as? HTTPURLResponse else {
throw CommandTestError("Response from \(url.absoluteString) is not HTTPURLResponse")
}
Expand Down
13 changes: 6 additions & 7 deletions Tests/CartonCommandTests/DevCommandTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -43,24 +43,23 @@ final class DevCommandTests: XCTestCase {
// FIXME: Don't assume a specific port is available since it can be used by others or tests
try await withFixture("EchoExecutable") { packageDirectory in
let process = try swiftRunProcess(
["carton", "dev", "--verbose", "--port", "8081", "--skip-auto-open"],
["carton", "dev", "--verbose", "--port", "8080", "--skip-auto-open"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ここなんですが、すぐ上の testWithNoArguments がサーバーを閉じられていない場合に、
8080 でぶつかってくれた方が問題を発見できるので、8081から変えたいです。

packageDirectory: packageDirectory.asURL
)

try await checkForExpectedContent(process: process, at: "http://127.0.0.1:8081")
try await checkForExpectedContent(process: process, at: "http://127.0.0.1:8080")
}
}
#endif

private func fetchDevServerWithRetry(at url: URL) async throws -> (response: HTTPURLResponse, body: Data) {
// client time out for connecting and responding
let timeOut: Duration = .seconds(60)
let timeOut: Duration = .seconds(10)

// client delay... let the server start up
let delay: Duration = .seconds(30)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

最初の1回で30秒待つのは手元の開発で不便すぎるので、
3秒にして試行回数を増やします。

let delay: Duration = .seconds(3)

// only try 5 times.
let count = 5
let count = 100

do {
return try await withRetry(maxAttempts: count, initialDelay: delay, retryInterval: delay) {
Expand All @@ -78,7 +77,7 @@ final class DevCommandTests: XCTestCase {
func checkForExpectedContent(process: SwiftRunProcess, at url: String) async throws {
defer {
// end the process regardless of success
process.process.signal(SIGTERM)
process.process.signal(SIGINT)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

swiftpm pluginを正しく貫通する SIGINT を使います。

}

let (response, data) = try await fetchDevServerWithRetry(at: try URL(string: url).unwrap("url"))
Expand Down