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.

Sign up for GitHub

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

SwiftSDKTool: convert previously blocking code to async #6623

Merged
merged 2 commits into from
Jun 14, 2023
Merged

SwiftSDKTool: convert previously blocking code to async #6623

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
65249e6
SwiftSDKTool: convert previously blocking code to `async`
MaxDesiatov May 26, 2023
3ac6777
Use async `@main` entrypoint for `swift-experimental-sdk`
MaxDesiatov Jun 13, 2023
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
31 changes: 14 additions & 17 deletions Sources/PackageModel/SwiftSDKBundle.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,8 @@ public struct SwiftSDKBundle {
_ fileSystem: some FileSystem,
_ archiver: some Archiver,
_ observabilityScope: ObservabilityScope
) throws {
_ = try withTemporaryDirectory(
) async throws {
_ = try await withTemporaryDirectory(
removeTreeOnDeinit: true
) { temporaryDirectory in
let bundlePath: AbsolutePath
Expand All @@ -149,21 +149,18 @@ public struct SwiftSDKBundle {
let bundleName = bundleURL.lastPathComponent
let downloadedBundlePath = temporaryDirectory.appending(component: bundleName)

let client = LegacyHTTPClient()
var request = LegacyHTTPClientRequest.download(
let client = HTTPClient()
var request = HTTPClientRequest.download(
url: bundleURL,
fileSystem: fileSystem,
destination: downloadedBundlePath
)
request.options.validResponseCodes = [200]
_ = try temp_await {
client.execute(
request,
observabilityScope: observabilityScope,
progress: nil,
completion: $0
)
}
_ = try await client.execute(
request,
observabilityScope: observabilityScope,
progress: nil
)

bundlePath = downloadedBundlePath

Expand All @@ -177,7 +174,7 @@ public struct SwiftSDKBundle {
throw DestinationError.invalidPathOrURL(bundlePathOrURL)
}

try installIfValid(
try await installIfValid(
bundlePath: bundlePath,
destinationsDirectory: swiftSDKsDirectory,
temporaryDirectory: temporaryDirectory,
Expand All @@ -204,7 +201,7 @@ public struct SwiftSDKBundle {
temporaryDirectory: AbsolutePath,
_ fileSystem: some FileSystem,
_ archiver: some Archiver
) throws -> AbsolutePath {
) async throws -> AbsolutePath {
let regex = try RegEx(pattern: "(.+\\.artifactbundle).*")

guard let bundleName = bundlePath.components.last else {
Expand All @@ -227,7 +224,7 @@ public struct SwiftSDKBundle {

print("\(bundleName) is assumed to be an archive, unpacking...")

try temp_await { archiver.extract(from: bundlePath, to: temporaryDirectory, completion: $0) }
try await archiver.extract(from: bundlePath, to: temporaryDirectory)

return temporaryDirectory.appending(component: unpackedBundleName)
}
Expand All @@ -245,15 +242,15 @@ public struct SwiftSDKBundle {
_ fileSystem: some FileSystem,
_ archiver: some Archiver,
_ observabilityScope: ObservabilityScope
) throws {
) async throws {
#if os(macOS)
// Check the quarantine attribute on bundles downloaded manually in the browser.
guard !fileSystem.hasAttribute(.quarantine, bundlePath) else {
throw DestinationError.quarantineAttributePresent(bundlePath: bundlePath)
}
#endif

let unpackedBundlePath = try unpackIfNeeded(
let unpackedBundlePath = try await unpackIfNeeded(
bundlePath: bundlePath,
destinationsDirectory: destinationsDirectory,
temporaryDirectory: temporaryDirectory,
Expand Down
4 changes: 2 additions & 2 deletions Sources/SwiftSDKTool/InstallSwiftSDK.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ public struct InstallSwiftSDK: SwiftSDKSubcommand {
buildTimeTriple: Triple,
_ destinationsDirectory: AbsolutePath,
_ observabilityScope: ObservabilityScope
) throws {
) async throws {
let cancellator = Cancellator(observabilityScope: observabilityScope)
cancellator.installSignalHandlers()
try SwiftSDKBundle.install(
try await SwiftSDKBundle.install(
bundlePathOrURL: bundlePathOrURL,
swiftSDKsDirectory: destinationsDirectory,
self.fileSystem,
Expand Down
2 changes: 1 addition & 1 deletion Sources/SwiftSDKTool/RemoveSwiftSDK.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public struct RemoveSwiftSDK: SwiftSDKSubcommand {
buildTimeTriple: Triple,
_ destinationsDirectory: AbsolutePath,
_ observabilityScope: ObservabilityScope
) throws {
) async throws {
let destinationsDirectory = try self.getOrCreateDestinationsDirectory()
let artifactBundleDirectory = destinationsDirectory.appending(component: self.sdkIDOrBundleName)

Expand Down
8 changes: 4 additions & 4 deletions Sources/SwiftSDKTool/SwiftSDKSubcommand.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import PackageModel
import var TSCBasic.stdoutStream

/// A protocol for functions and properties common to all destination subcommands.
protocol SwiftSDKSubcommand: ParsableCommand {
protocol SwiftSDKSubcommand: AsyncParsableCommand {
/// Common locations options provided by ArgumentParser.
var locations: LocationOptions { get }

Expand All @@ -32,7 +32,7 @@ protocol SwiftSDKSubcommand: ParsableCommand {
buildTimeTriple: Triple,
_ destinationsDirectory: AbsolutePath,
_ observabilityScope: ObservabilityScope
) throws
) async throws
}

extension SwiftSDKSubcommand {
Expand All @@ -59,7 +59,7 @@ extension SwiftSDKSubcommand {
return destinationsDirectory
}

public func run() throws {
public func run() async throws {
let observabilityHandler = SwiftToolObservabilityHandler(outputStream: stdoutStream, logLevel: .info)
let observabilitySystem = ObservabilitySystem(observabilityHandler)
let observabilityScope = observabilitySystem.topScope
Expand All @@ -70,7 +70,7 @@ extension SwiftSDKSubcommand {

var commandError: Error? = nil
do {
try self.run(buildTimeTriple: triple, destinationsDirectory, observabilityScope)
try await self.run(buildTimeTriple: triple, destinationsDirectory, observabilityScope)
if observabilityScope.errorsReported {
throw ExitCode.failure
}
Expand Down
4 changes: 2 additions & 2 deletions Sources/SwiftSDKTool/SwiftSDKTool.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@

import ArgumentParser
import Basics

public struct SwiftSDKTool: ParsableCommand {
public struct SwiftSDKTool: AsyncParsableCommand {
public static let configuration = CommandConfiguration(
commandName: "experimental-sdk",
_superCommandName: "swift",
Expand Down
5 changes: 1 addition & 4 deletions Sources/swift-experimental-sdk/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,10 @@
# See http://swift.org/CONTRIBUTORS.txt for Swift project authors

add_executable(swift-experimental-sdk
SwiftSDKTool.swift)
main.swift)
target_link_libraries(swift-experimental-sdk PRIVATE
SwiftSDKTool)

target_compile_options(swift-experimental-sdk PRIVATE
-parse-as-library)

if(USE_CMAKE_INSTALL)
install(TARGETS swift-experimental-sdk
RUNTIME DESTINATION bin)
Expand Down
32 changes: 0 additions & 32 deletions Sources/swift-experimental-sdk/SwiftSDKTool.swift
View file
Open in desktop

This file was deleted.

18 changes: 18 additions & 0 deletions Sources/swift-experimental-sdk/main.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift open source project
//
// Copyright (c) 2023 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See http://swift.org/LICENSE.txt for license information
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//

import SwiftSDKTool

// This is quite ugly, but it works in terms of unambiguously specifying the async overload of `main()`.
_ = await { () async -> () in
await SwiftSDKTool.main()
}()
Copy link
Contributor

Choose a reason for hiding this comment

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

cant we make this into a struct with a @main instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@tomerd tomerd Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why not define a private "Runner" struct with @main that calls the underlying static main as the closure does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great point, addressed now.

4 changes: 2 additions & 2 deletions Sources/swift-package-manager/SwiftPM.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,14 @@ let execName = (try? AbsolutePath(validating: firstArg).basenameWithoutExt) ??

@main
struct SwiftPM {
static func main() {
static func main() async {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we had trouble with this change in the past, are we out of the wood on that now?

cc @neonichu

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Windows is another question, I think? Supposedly async doesn't really work well there IIRC. cc @compnerd

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@MaxDesiatov MaxDesiatov Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Would it be ok to merge now? I only need a review approval for that.

switch execName {
case "swift-package":
SwiftPackageTool.main()
case "swift-build":
SwiftBuildTool.main()
case "swift-experimental-sdk":
SwiftSDKTool.main()
await SwiftSDKTool.main()
case "swift-test":
SwiftTestTool.main()
case "swift-run":
Expand Down
10 changes: 5 additions & 5 deletions Tests/PackageModelTests/SwiftSDKBundleTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ final class SwiftSDKBundleTests: XCTestCase {

let archiver = MockArchiver()

try SwiftSDKBundle.install(
try await SwiftSDKBundle.install(
bundlePathOrURL: bundles[0].path,
swiftSDKsDirectory: swiftSDKsDirectory,
fileSystem,
Expand All @@ -107,7 +107,7 @@ final class SwiftSDKBundleTests: XCTestCase {

let invalidPath = "foobar"
do {
try SwiftSDKBundle.install(
try await SwiftSDKBundle.install(
bundlePathOrURL: "foobar",
swiftSDKsDirectory: swiftSDKsDirectory,
fileSystem,
Expand All @@ -131,7 +131,7 @@ final class SwiftSDKBundleTests: XCTestCase {
}

do {
try SwiftSDKBundle.install(
try await SwiftSDKBundle.install(
bundlePathOrURL: bundles[0].path,
swiftSDKsDirectory: swiftSDKsDirectory,
fileSystem,
Expand All @@ -155,7 +155,7 @@ final class SwiftSDKBundleTests: XCTestCase {
}

do {
try SwiftSDKBundle.install(
try await SwiftSDKBundle.install(
bundlePathOrURL: bundles[1].path,
swiftSDKsDirectory: swiftSDKsDirectory,
fileSystem,
Expand Down Expand Up @@ -191,7 +191,7 @@ final class SwiftSDKBundleTests: XCTestCase {
let system = ObservabilitySystem.makeForTesting()

for bundle in bundles {
try SwiftSDKBundle.install(
try await SwiftSDKBundle.install(
bundlePathOrURL: bundle.path,
swiftSDKsDirectory: swiftSDKsDirectory,
fileSystem,
Expand Down