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.

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

Add modern Swift Concurrency support #20

Merged
merged 8 commits into from
Oct 26, 2023

Conversation

mlink
Copy link
Contributor

@mlink mlink commented Oct 16, 2023

Adds support for running commands that can be awaited on and deprecates previous non-async closure based methods.

Only one breaking change for Subprocess.init which no longer takes a QOS argument since the underlying implementation now uses Swift Concurrency instead of GCD. This version should be tagged as a major update: 3.0.0.

Subprocess methods return AsyncSequence which can use familiar Swift sequence operators (e.g. map, filter, etc).

Shell has been deprecated in favor of replacement static convenience methods added as extensions to Subprocess. One less class to remember.

Other miscellaneous changes and updates that feel more inline with Swift best practices since the last release of this package. Minimum version has been bumped to 10.15.4 in order to use non-objective-c exception throwing methods on FileHandle.

taher-mosbah
taher-mosbah previously approved these changes Oct 17, 2023
Sources/Subprocess/SubprocessDependencyBuilder.swift Outdated Show resolved Hide resolved
Sources/Subprocess/Subprocess.swift Show resolved Hide resolved
.github/workflows/build-and-test.yml Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
s.summary = 'Wrapper for NSTask used for running processes and shell commands on macOS.'
s.license = { :type => 'MIT', :text => "" }
s.description = <<-DESC
Everything related to creating processes and running shell commands on macOS.
DESC
s.homepage = 'https://github.com/jamf/Subprocess'
s.authors = { 'Cyrus Ingraham' => '[email protected]' }
s.authors = { 'Cyrus Ingraham' => '[email protected]', 'Michael Link' => '[email protected]' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove Cyrus's name since he's not with the company any more? Not sure how all that works in this context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. If the email goes to a black hole then it's not very useful to have there. I wonder if there's a apple-natives email available for these sorts of things?

Copy link
Contributor

Choose a reason for hiding this comment

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

Cyrus has a different email address now. He used to have write access as a contributor to this open source repo, but it looks like that was removed (probably during the great permissions change to teams instead of individuals).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cyrus has a different email address now. He used to have write access as a contributor to this open source repo, but it looks like that was removed (probably during the great permissions change to teams instead of individuals).

Is the point of the authors field for credit or support? If it's credit then if anyone knows his updated email we could put that there, otherwise just leave the existing one until we know. Otherwise, I'm fine taking point on fielding emails about this project after this PR. This seems relevant only for cocoapods anyway.

JacobHearst
JacobHearst previously approved these changes Oct 18, 2023
Copy link
Contributor

@macblazer macblazer left a comment

Choose a reason for hiding this comment

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

This reasons for this PR are a bit unclear, and there has been no discussion with the community as far as I know. No GitHub Issue has been created around the desire to update to async-await.

In general, you've marked a bunch of types and functions as deprecated in favor of async-await versions of the same functionality. Is deprecation necessary? Do the functions still compile and work correctly with Xcode 15 and the latest OS releases? If the old functions do not work, they should be removed and not simply deprecated.

If some consuming code wants to not dive into async-await should they stick with the v2.0.0 release of this library, or just put up with warnings from this v3.x code?

I see that there have been some minor changes made to the SubprocessMocks library to make it compile, but no additional mocks have been made to simulate the async-await behavior. Making it easy for consuming apps to unit test their calls to other processes was one of the goals of the SubprocessMocks library.

.swiftlint.yml Outdated Show resolved Hide resolved
LICENSE Outdated Show resolved Hide resolved
Sources/Subprocess/Subprocess.swift Outdated Show resolved Hide resolved
@mlink
Copy link
Contributor Author

mlink commented Oct 19, 2023

In general, you've marked a bunch of types and functions as deprecated in favor of async-await versions of the same functionality. Is deprecation necessary? Do the functions still compile and work correctly with Xcode 15 and the latest OS releases? If the old functions do not work, they should be removed and not simply deprecated.

Yes, the functions still compile and work with the latest tools and OS. The deprecations were added in the case that a project needs to piecemeal adopt the new interface. The deprecations are also a hint that an updated usage of the package exists.

If some consuming code wants to not dive into async-await should they stick with the v2.0.0 release of this library, or just put up with warnings from this v3.x code?

I feel that's a decision the consumer would need to determine for themself. I don't see anything that would block someone from moving to an async/await interface other than their own preference for a closure based interface. I feel there are benefits to using Swift concurrency over closure based interfaces (i.e. well defined points where execution is concurrent, cancellation, adoption in other async methods fits well without bloat from continuations). Presenting a preference for Swift Concurrency presents a more concise succinct interface in the end.

I see that there have been some minor changes made to the SubprocessMocks library to make it compile, but no additional mocks have been made to simulate the async-await behavior. Making it easy for consuming apps to unit test their calls to other processes was one of the goals of the SubprocessMocks library.

An oversight that can be remedied.

@macblazer
Copy link
Contributor

Yes, the functions still compile and work with the latest tools and OS. The deprecations were added in the case that a project needs to piecemeal adopt the new interface. The deprecations are also a hint that an updated usage of the package exists.

If that's the case, I'm not sure we want to mark the closure-based functions as deprecated unless we also plan on removing them at some future time. Apple has provided async-await versions of URL loading functions but the classic functions still work and are not deprecated. It's a choice of the consuming code to use async-await or not, and it seems like either choice should be acceptable from the point of view of this library as long as the functionality of both choices is essentially the same.

Forcing code that chooses to stick with classic GCD to also put up with deprecation warnings when using the most recent version of this library is not something I feel we need to do.

@mlink
Copy link
Contributor Author

mlink commented Oct 20, 2023

If that's the case, I'm not sure we want to mark the closure-based functions as deprecated unless we also plan on removing them at some future time. Apple has provided async-await versions of URL loading functions but the classic functions still work and are not deprecated. It's a choice of the consuming code to use async-await or not, and it seems like either choice should be acceptable from the point of view of this library as long as the functionality of both choices is essentially the same.

Forcing code that chooses to stick with classic GCD to also put up with deprecation warnings when using the most recent version of this library is not something I feel we need to do.

I see your point. I have restored the closure based launch methods.

I think the Shell class should still be deprecated and removed in some future version as the async replacements in Subprocess replace that class and have well defined concurrent execution points and Shell uses a synchronous wait that can't be canceled like its newer counterparts.

@mlink mlink merged commit 5ab92a0 into main Oct 26, 2023
3 checks passed
@mlink mlink deleted the jamf/modern-swift-concurrency-support branch October 26, 2023 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants