PackageLoading: convert most of ManifestLoader to async/await
#6624
Conversation
MaxDesiatov
changed the title
async/await
|
@swift-ci smoke test |
MaxDesiatov
changed the title
async/awaitManifestLoader to async/await
|
|
||
| extension ManifestLoader { | ||
| /// Represents behavior that can be deferred until a more appropriate time. | ||
| struct DelayableAction<T> { |
There was a problem hiding this comment.
Removed as unused, we can now use plain defer instead
|
@swift-ci smoke test |
|
@swift-ci test windows |
1 similar comment
|
@swift-ci test windows |
|
@swift-ci smoke test |
This allows `Workspace.loadPluginImports` to be implemented in a non-blocking way without any uses of `temp_await`. Also a dependency of #6624.
This allows `Workspace.loadPluginImports` to be implemented in a non-blocking way without any uses of `temp_await`. Also a dependency of #6624.
MaxDesiatov
force-pushed
the
maxd/async-manifest-loader
branch
from
b6d8578 to
818ebd1
Compare
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test windows |
|
@neonichu CI is green on this now, ready for review |
…xd/async-manifest-loader # Conflicts: # Sources/PackageLoading/ManifestLoader.swift
|
@swift-ci test |
|
@swift-ci test windows |
…xd/async-manifest-loader
|
@swift-ci test |
AndrewHoos
left a comment
AndrewHoos
left a comment
There was a problem hiding this comment.
Some minor nits... but otherwise looks good
| } | ||
|
|
||
| var evaluationResult = EvaluationResult() | ||
| return try await self.tokenBucket.withToken { |
There was a problem hiding this comment.
I don't have the background, but it is unclear why this diff is introducing the withToken indentation
There was a problem hiding this comment.
withToken introduces indentation much earlier, since with stricter concurrency checks withToken has to initialize state like evaluationResult much earlier in its scope than was previously possible with unchecked Dispatch closures. Previously this state was captured with no sendability checks whatsoever. Much broader withToken scope allows us to initialize all state in the same closure, fixing sendability warnings and errors.
| "-L", runtimePath.pathString, | ||
| "-lPackageDescription", | ||
| ] | ||
| #if !os(Windows) |
There was a problem hiding this comment.
nit: The OS if statements are now indented in a non-standard way
There was a problem hiding this comment.
I'm not sure what "non-standard" means here. AFAIK Swift as a language has no standard formatting guidelines. Formatting used in this PR is consistent with the formatting convention we've codified for SwiftPM in our SwiftFormat config: https://github.com/apple/swift-package-manager/blob/main/.swiftformat. Some parts of SwiftPM written before the config was introduced may not follow it yet, but for new and updated code we try to stick to it, converging on that consistent formatting convention over time.
There was a problem hiding this comment.
Previously the #if was flush left with the file. Now it is one indentation in, but the native level of indentation here is three levels deep. Maybe it is just how GitHub is rendering, but 1 indentation seemed odd because nothing else was at that level
| observabilityScope: ObservabilityScope, | ||
| delegateQueue: DispatchQueue, | ||
| callbackQueue: DispatchQueue | ||
| delegateQueue: DispatchQueue |
There was a problem hiding this comment.
Do we need the delegateQueue here anymore? there is no way we are FIFO when in swift concurrency so I am I am not sure that forcing a hop to the delegate queue is meaningful, but I haven't thought this all the way through
There was a problem hiding this comment.
We're still calling delegate methods from async context that may be executed on some thread from the concurrency pool that we can't know upfront. Until we have strict concurrency checks enabled or have an opportunity to rewrite delegates with AsyncSequence producing events for notifying the caller, I'd like to have delegateQueue passed around explicitly.
|
@swift-ci test |
|
@swift-ci test macos |
…xd/async-manifest-loader # Conflicts: # Tests/PackageLoadingTests/PD_4_2_LoadingTests.swift
|
@swift-ci test |
…xd/async-manifest-loader # Conflicts: # Sources/PackageLoading/ManifestLoader.swift
|
@swift-ci test |
|
@swift-ci test windows |
…xd/async-manifest-loader
|
@swift-ci test |
…xd/async-manifest-loader
|
@swift-ci test |
This is a stripped down version of #6624 that modifies only the test suite and doesn't make any substantial to `ManifestLoader.swift` other than providing an `async` overload in `extension ManifestLoaderProtocol` that uses `withCheckedThrowingContinuation` to bridge into the existing implementation.
…xd/async-manifest-loader # Conflicts: # Sources/SPMTestSupport/MockManifestLoader.swift # Sources/SPMTestSupport/XCTAssertHelpers.swift # Tests/PackageLoadingTests/PD_4_2_LoadingTests.swift
|
@swift-ci test |
|
@swift-ci test |
This is a stripped down version of swiftlang#6624 that modifies only the test suite and doesn't make any substantial to `ManifestLoader.swift` other than providing an `async` overload in `extension ManifestLoaderProtocol` that uses `withCheckedThrowingContinuation` to bridge into the existing implementation.
This is a stripped down version of swiftlang#6624 that modifies only the test suite and doesn't make any substantial to `ManifestLoader.swift` other than providing an `async` overload in `extension ManifestLoaderProtocol` that uses `withCheckedThrowingContinuation` to bridge into the existing implementation.
This keeps the public function signatures intact and only converts
privateorinternalfunctions toasync/await.