SwiftSDKTool: convert previously blocking code to async
#6623
Conversation
|
@swift-ci smoke test |
| // This is quite ugly, but it works in terms of unambiguously specifying the async overload of `main()`. | ||
| _ = await { () async -> () in | ||
| await SwiftSDKTool.main() | ||
| }() |
There was a problem hiding this comment.
cant we make this into a struct with a @main instead?
There was a problem hiding this comment.
We had it previously in Sources/swift-experimental-sdk/SwiftSDKTool.swift, which you can see removed in the diff right above this file, but that required duplicating SwiftSDKTool from SwiftSDKTool module verbatim, which I think is much worse than this. Unfortunately, @main can't be added to a type declared in a different module.
There was a problem hiding this comment.
Why not define a private "Runner" struct with @main that calls the underlying static main as the closure does?
There was a problem hiding this comment.
That's a great point, addressed now.
| @@ -22,14 +22,14 @@ let execName = (try? AbsolutePath(validating: firstArg).basenameWithoutExt) ?? | |||
|
|
|||
| @main | |||
| struct SwiftPM { | |||
| static func main() { | |||
| static func main() async { | |||
There was a problem hiding this comment.
I think we had trouble with this change in the past, are we out of the wood on that now?
cc @neonichu
There was a problem hiding this comment.
AFAIR those were CI issues, but I don't see them reproduced now and this passes tests and runs well locally. I don't plan to cherry-pick this to 5.9, so we'll have enough time to investigate anything that could potentially go wrong.
There was a problem hiding this comment.
Windows is another question, I think? Supposedly async doesn't really work well there IIRC. cc @compnerd
There was a problem hiding this comment.
maybe also check the toolchain CI, ie try to create a toolchain with this PR, I remember we had SwiftPM green but it caused downstream issues. in any case, I think we should not merge this in the immediate timeframe to avoid 🔥 in a sensitive time
There was a problem hiding this comment.
Would it be ok to merge now? I only need a review approval for that.
|
@swift-ci smoke test |
|
@swift-ci test windows |
We previously had to roll back these changes due to CI issues, this is another attempt to bring them back.