Skip to content

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

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

Jump to bottom
Merged
merged 17 commits into from
Jun 22, 2024
Merged

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

merged 17 commits into from
Jun 22, 2024

Conversation

MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented Jun 18, 2024 •
edited
Loading

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
Vendor AsyncProcess, make more tests async
30658a3 
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 when reading from process instances in plugins communication. Currently, without this logic a naive rewrite of plugins communication with structured concurrency is quite bug-prone.

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 MaxDesiatov added concurrency test suite improvements to SwiftPM test suite no functional change No user-visible functional changes included labels Jun 18, 2024
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov MaxDesiatov enabled auto-merge (squash) June 18, 2024 18:45
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@rauhul
Copy link
Member

rauhul commented Jun 18, 2024

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
Copy link
Contributor Author

MaxDesiatov commented Jun 18, 2024 •
edited
Loading

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
Copy link
Member

rauhul commented Jun 18, 2024 •
edited
Loading

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

1 similar comment
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov MaxDesiatov self-assigned this Jun 19, 2024
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

1 similar comment
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

xedin
xedin approved these changes Jun 21, 2024
View reviewed changes
@xedin
Copy link
Contributor

xedin commented Jun 21, 2024

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

@MaxDesiatov
Apply consistent formatting to the new file
6c0fbe6
@MaxDesiatov
Merge branch 'main' of github.com:apple/swift-package-manager into ma…
6f379ee 
…xd/async-process-tests

# Conflicts:
#	Sources/Basics/Archiver/TarArchiver.swift
#	Sources/Build/BuildOperation.swift
#	Sources/Commands/PackageCommands/Format.swift
#	Sources/Commands/SwiftTestCommand.swift
#	Sources/DriverSupport/SPMSwiftDriverExecutor.swift
#	Sources/PackageLoading/ManifestLoader.swift
#	Sources/PackageLoading/PkgConfig.swift
#	Sources/PackageModel/SwiftSDKs/SwiftSDK.swift
#	Sources/SPMTestSupport/SwiftPMProduct.swift
#	Sources/SPMTestSupport/misc.swift
#	Sources/SourceControl/GitRepository.swift
#	Sources/Workspace/DefaultPluginScriptRunner.swift
#	Sources/XCBuildSupport/XcodeBuildSystem.swift
#	Tests/BuildTests/IncrementalBuildTests.swift
#	Tests/CommandsTests/APIDiffTests.swift
#	Tests/CommandsTests/BuildCommandTests.swift
#	Tests/CommandsTests/PackageCommandTests.swift
#	Tests/CommandsTests/PackageRegistryCommandTests.swift
#	Tests/CommandsTests/RunCommandTests.swift
#	Tests/CommandsTests/SwiftSDKCommandTests.swift
#	Tests/FunctionalTests/MiscellaneousTests.swift
#	Tests/FunctionalTests/PluginTests.swift
#	Tests/PackageModelTests/MinimumDeploymentTargetTests.swift
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

rauhul
rauhul reviewed Jun 21, 2024
View reviewed changes
rauhul
rauhul reviewed Jun 21, 2024
View reviewed changes
rauhul
rauhul reviewed Jun 21, 2024
View reviewed changes
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov MaxDesiatov enabled auto-merge (squash) June 21, 2024 23:02
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov MaxDesiatov requested a review from rauhul June 21, 2024 23:18
rauhul
rauhul approved these changes Jun 22, 2024
View reviewed changes
Copy link
Member

@rauhul rauhul left a comment

Choose a reason for hiding this comment

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

very happy to see fewer tsc imports. I think with this change we shouldn't need ProcessEnvironmentBlock except for Git

Sources/Basics/AsyncProcess.swift Outdated
}
process.executableURL = executablePath.asURL
process
.environment = [String: String](
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be [String: String](environment)

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

1 similar comment
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

1 similar comment
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov MaxDesiatov merged commit b86d22e into main Jun 22, 2024
5 checks passed
@MaxDesiatov MaxDesiatov deleted the maxd/async-process-tests branch June 22, 2024 15:54
MaxDesiatov added a commit that referenced this pull request Jun 27, 2024
@MaxDesiatov
[NFC] Vendor AsyncProcess, make more tests async (#7679)
382c7c8 
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.

(cherry picked from commit b86d22e)

# Conflicts:
#	Tests/CommandsTests/PackageCommandTests.swift
#	Tests/FunctionalTests/MiscellaneousTests.swift
#	Tests/FunctionalTests/PluginTests.swift
#	Tests/FunctionalTests/TraitTests.swift
@MaxDesiatov MaxDesiatov mentioned this pull request Jun 27, 2024
Merged
MaxDesiatov added a commit that referenced this pull request Jun 27, 2024
@MaxDesiatov @xedin
@SimplyDanny @rauhul
[6.0] Cherry-pick recent NFC changes to reduce merge conflicts (#7721)
4e03b81 
Includes these PRs cherry-picked off `main`
* #7605
* #7660
* #7667
* #7682
* #7687
* #7690
* #7684
* #7679

**Explanation**: Cherry-pick of recent NFC changes, which makes it
easier to cherry-pick actual bug fixes onto 6.0 due to the reduced
number of merge conflicts.
**Scope**: broad, includes both modules graph and llbuild-related
changes.
**Risk**: low, the test suite is passing, no functional changes are
included, and cherry-picked changes were incubated on `main` for some
time.
**Testing**: Existing automated test suite.
**Issue**: N/A
**Reviewers**: @xedin @MaxDesiatov @rauhul

---------

Co-authored-by: Pavel Yaskevich <[email protected]>
Co-authored-by: Danny Mösch <[email protected]>
Co-authored-by: Rauhul Varma <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xedin xedin xedin approved these changes

@rauhul rauhul rauhul approved these changes

@bnbarham bnbarham Awaiting requested review from bnbarham

@jakepetroules jakepetroules Awaiting requested review from jakepetroules jakepetroules is a code owner

@francescomikulis francescomikulis Awaiting requested review from francescomikulis

Assignees

@MaxDesiatov MaxDesiatov

Labels
concurrency no functional change No user-visible functional changes included test suite improvements to SwiftPM test suite
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MaxDesiatov @rauhul @xedin