swiftlang / swift-package-manager

The Package Manager for the Swift Programming Language
Apache License 2.0
9.65k stars 1.31k forks source link

[NFC] Vendor `AsyncProcess`, make more tests `async` #7679

Closed MaxDesiatov closed 1 week ago

MaxDesiatov commented 2 weeks ago

We'd like to implement more changes to TSCBasic.Process without fearing of breaking clients, in addition to the fact that TSC is deprecated.

One thing in particular we need is backpressure logic (coming in a future PR) when reading from process instances in plugins communication with the host SwiftPM process. Currently without this logic, a naive rewrite of plugins communication with structured concurrency is quite bug-prone, as it loses messages ordering and can quickly fill up all memory with resource-intensive plugins.

Thus TSCBasic.Process is slightly cleaned up and is vendored as AsyncProcess in this change. We also give it more coverage in tests by utilizing async overloads, which are the overloads we should be using more in the structured concurrency conversion of the SwiftPM codebase.

MaxDesiatov commented 2 weeks ago

@swift-ci test

MaxDesiatov commented 2 weeks ago

@swift-ci test

MaxDesiatov commented 2 weeks ago

@swift-ci test windows

MaxDesiatov commented 2 weeks ago

@swift-ci test

MaxDesiatov commented 2 weeks ago

@swift-ci test windows

rauhul commented 2 weeks ago

Could we put this in its own module instead of "Basics", that module is massive and has so much conflated code that its really hard to reason about

MaxDesiatov commented 2 weeks ago

I'm not entirely convinced there's much benefit to splitting it, the downside would be that our Package.swift is gigantic and it's impossible to get a graph of module dependencies at a glance. Also unclear on what lines it should be split on in the first place, although I wouldn't mind discussing that in a separate PR that would make an attempt to do it.

Here for now I'd prefer to roughly keep the status quo: it was in TSCBasic before, which more or less directly corresponds to SwiftPM's Basics.

rauhul commented 2 weeks ago

I'm not entirely convinced there's much benefit to splitting it, the downside would be that our Package.swift is gigantic and it's impossible to get a graph of module dependencies at a glance. Also unclear on what lines it should be split on in the first place, although I wouldn't mind discussing that in a separate PR that would make an attempt to do it.

Here for now I'd prefer to roughly keep the status quo: it was in TSCBasic before, which more or less directly corresponds to SwiftPM's Basics.

Thats a fair point, but I think moving files over is a great opportunity for better hygiene. Mega modules are both slower to build both clean & incremental and bury the lead of the functionality they provide.

IMO a good option for splitting the module is exposing async-process to the rest of SwiftPM via the basics module, e.g.: AsyncProcess -> Basics -> the rest of SwiftPM

MaxDesiatov commented 2 weeks ago

IMO a good option for splitting the module is exposing async-process to the rest of SwiftPM via the basics module, e.g.: AsyncProcess -> Basics -> the rest of SwiftPM

This won't work, as AsyncProcess depends on AbsolutePath, which belongs to Basics.

MaxDesiatov commented 2 weeks ago

@swift-ci test

MaxDesiatov commented 2 weeks ago

@swift-ci test windows

MaxDesiatov commented 2 weeks ago

@swift-ci test windows

MaxDesiatov commented 1 week ago

@swift-ci test windows

MaxDesiatov commented 1 week ago

@swift-ci test windows

MaxDesiatov commented 1 week ago

@swift-ci test windows

MaxDesiatov commented 1 week ago

@swift-ci test windows

MaxDesiatov commented 1 week ago

@swift-ci test

MaxDesiatov commented 1 week ago

@swift-ci test

MaxDesiatov commented 1 week ago

@swift-ci test

MaxDesiatov commented 1 week ago

@swift-ci test

xedin commented 1 week ago

I skimmed through all of the renames and mostly read through AsyncProcess itself which looks good to me.

MaxDesiatov commented 1 week ago

@swift-ci test

MaxDesiatov commented 1 week ago

@swift-ci test windows

MaxDesiatov commented 1 week ago

@swift-ci test

MaxDesiatov commented 1 week ago

@swift-ci test windows

MaxDesiatov commented 1 week ago

@swift-ci test windows

MaxDesiatov commented 1 week ago

@swift-ci test windows

MaxDesiatov commented 1 week ago

@swift-ci test windows

MaxDesiatov commented 1 week ago

@swift-ci test windows

MaxDesiatov commented 1 week ago

@swift-ci test