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

Fix bugs in Process.checkRun of CartonDriver #463

Merged
merged 1 commit into from
May 20, 2024

Conversation

omochi
Copy link
Contributor

@omochi omochi commented May 20, 2024

Process.checkRun が実装されていますが、少し変です。
forwardExit フラグが指定されている場合に、
terminationHandler 経由で exit していますが、
それとは別に waitUntilExit() の後にも exit するフローがあります。

これだと exit する経路が2つあって、
どちらを通るかは並行処理次第であり不安定に見えます。

そもそも waitUntilExit() している関数なので、
そこから復帰した後に exit へ入れば良いので、
terminationHandler を使わないシンプルな形に書き換えます。


既存のコードで waitUntilExit の後に書いてある exit は、
ステータスコードが失敗なら突入するようになっているので、
forwardExit の機能としては壊れています。
よってこれは forward しない場合の機能のようです。

実際にこちらが使われるのは、
carton test に2つあり、
plugin に対して internal-get-build-command を呼ぶところと、
受け取ったそのビルドコマンドを実行するところです。

この2箇所については、
もしコマンドが失敗した時はステータスコードを転送して即死するのではなく、
制御ロジックとして例外を投げる方が望ましいと思います。

その方が失敗した時に状況が理解しやすいです。
これらのコマンドは driver が変身してるわけではないからです。


出力例です。
test経由でビルドが始まったときに C-c を押して殺しています。
例外のメッセージを使って、
死んだコマンドのコマンドラインがコンソールに出せるので、
トラブルシュートしやすくなっていると思います。

[omochi@omochi-mbp carton (main *+=)]$ swift run carton test
Building for debugging...
[1/1] Write swift-version-3874884F1997D323.txt
Build complete! (0.27s)
- checking Swift compiler path: /Users/omochi/.carton/sdk/wasm-5.9.2-RELEASE/usr/bin/swift
- checking Swift compiler path: /Users/omochi/.swiftenv/versions/wasm-5.9.2-RELEASE/usr/bin/swift
- checking Swift compiler path: /Users/omochi/Library/Developer/Toolchains/swift-wasm-5.9.2-RELEASE.xctoolchain/usr/bin/swift
Inferring basic settings...
- swift executable: /Users/omochi/Library/Developer/Toolchains/swift-wasm-5.9.2-RELEASE.xctoolchain/usr/bin/swift
SwiftWasm Swift version 5.9.2 (swift-5.9.2-RELEASE)
Target: x86_64-apple-darwin23.5.0
Running "/Users/omochi/Library/Developer/Toolchains/swift-wasm-5.9.2-RELEASE.xctoolchain/usr/bin/swift" "package" "--triple" "wasm32-unknown-wasi" "--scratch-path" "/Users/omochi/github/swiftwasm/carton/.build/carton" "plugin" "carton-test" "internal-get-build-command" "--output" "/var/folders/qw/myjd_8ld5lg9qg057bhm467c0000gn/T/test-buildy5gQ7p"
Building for debugging...
[3/4] Merging module TSCLibc^CSwift/ErrorType.swift:200: Fatal error: Error raised at top level: Process failed with status 2.
Command line: "/Users/omochi/Library/Developer/Toolchains/swift-wasm-5.9.2-RELEASE.xctoolchain/usr/bin/swift" "package" "--triple" "wasm32-unknown-wasi" "--scratch-path" "/Users/omochi/github/swiftwasm/carton/.build/carton" "plugin" "carton-test" "internal-get-build-command" "--output" "/var/folders/qw/myjd_8ld5lg9qg057bhm467c0000gn/T/test-buildy5gQ7p"
zsh: trace trap  swift run carton test

Copy link
Contributor Author

@omochi omochi left a comment

Choose a reason for hiding this comment

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

comment

@@ -34,7 +34,7 @@ import CartonHelpers
import Foundation
import SwiftToolchain

struct CartonCommandError: Error & CustomStringConvertible {
struct CartonDriverError: Error & CustomStringConvertible {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

driverと呼ぶことに決まったので、
ソースファイル名とエラーの名前を変えました

fputs(
"Running \(([executableURL.path] + arguments).map { "\"\($0)\"" }.joined(separator: " "))\n",
stderr)
let commandLine: String = ([executableURL.path] + arguments)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

後でエラー出力にも使いたいので変数に出しました

@kateinoigakukun kateinoigakukun changed the title carton driverの Process ユーティリティの問題を修正する Fix bugs in Process.checkRun of CartonDriver May 20, 2024
@kateinoigakukun kateinoigakukun merged commit 2db7b69 into swiftwasm:main May 20, 2024
4 checks passed
@omochi omochi deleted the fix-driver-process-util branch May 20, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants