(Thanks for all this work and everyone's contribution! I'm eagerly awaiting Subprocess, having written thousands of lines of Swift scripts and scripting libraries, wrapping 10's of tools...)
I really like the Subprocess/+Foundation split, String support. Very helpful. I'm less certain about dropping the workflow of configuration, then invocation.
The run method has 8 parameters, well beyond the usual recommendation for encapsulating in some configuration object.
And there are multiple run methods, each capturing different variants of the involved aspects, but which are confusingly similar. If you read the first line of the docc comment, it's hard to know which to pick, and using the same name is not helpful in that regard.
Both suggest using a builder to distinguish configuration and invocation. Some benefits:
The invocation aspects are cleanly separated from the question of what's being run where.
You can configure properties for the platform and use-case once, and then use variations of the same builder to create and run different processes.
For the user, the configuration is a complete picture of set-up for a process, where they can dive (progressively) into the platform-dependent variations.
The point of use (run) ends up consistent across platforms and tools (isolating variants in the configuration phase), so run calls are distinguished by the invocation context.
It's easy to capture the configuration itself, for logging, error-reporting, and perhaps even capture/replay or otherwise driving process invocations.
For the Subprocess library, the configuration ends up as a measure of the logical/interface evolution distinct from the invocation implementation.
But isn't a builder that too complex, Java-like?
For efficiency purposes, any extra cost of configuration is trivial compared to spawning a process. That mostly leave ergonomics -- and wrapping.
The Subprocess library or users can always wrap as needed for brevity. Make a fast-path for the run from the builder (config.make().run(..)). Make a free+global or name-spaced static function if you want (with exactly the parameters proposed now). Those would be implemented with the builder/configuration/run workflow. You can make the configuration and invocation in turn as ergonomic as you want for th full range of disclosure.
To me the ergonomic standard is not just how simple it is to invoke in simple cases. The standard should focus on how much code is required to configure a bunch of related processes in different environments, and manage and interpret the errors that could result. The measure is whether users can somewhat obliviously follow the API, and get the right results, the first time.
My priorities for Subprocess would be
Correctness, and clarity thereof for users
Platform currency/accuracy, with any ugly details
Robustness to error: how likely is the user to find and solve a problem
Ergonomics: API discoverability & progression
Ergonomics: brevity
Finally, splitting configuration and invocations, and splitting complete-specification from fast-path, might help this proposal finish faster because different concerns are handled in different places.
I think the proposal here was that an explicit import Subprocess was required for the freestanding run() function:
I agree that if the freestanding function were exposed by a simple import Foundation that would be problematic (and potentially source-breaking), but I think requiring an explicit import Subprocess is a good compromise between the simplicity when writing scripts (which is currently a big pain point in Swift) while retaining the ability to use a qualified Subprocess.run() for disambiguation.
Ok, swap import Foundation with import Subprocess in that example code and it still won't compile and it will produce exactly the same misleading error diagnostic message.
IMO the ability to use ArgumentParser in scripts is crucial. Would be a shame if it were possible, but then hampered by confusing diagnostic messages as soon as someone tries to launch a process from an ArgumentParser command.
Swift doesn't have fully qualified name lookup and disambiguation with modules is questionable. If we introduce the struct Subprocess later, we are shooting ourselves in the foot.
While I personally don't have a strong opinion on either, using current disambiguation techniques cannot be a tangible solution to an "official" API.
What would be really cool is if in the future we could typealias (funcalias?) the run for those who really don't want that Subprocess.
Can't you just literally do this today? (only if run is a static func on Subprocess, I don't expect this to work with a free function and module disambiguation)
Sure, let was only chosen in that example for conciseness, your own free func run that calls into namespaced Subprocess.run is the right approach if default arguments are lost.
Collisions with ArgumentParser have been mentioned a few times here, and not without good reason. run is a very general, common term. In the interest of making sure we've explored the full design space, would there be less objection if the proposed function's name was a more specific verb instead that was less likely to collide, like spawn?
…or equivalent with “launch” instead of “run”, or “subprocess” instead of “process”, or some other reasonable synonyms. I’m not too concerned with the specific wording, but I think it is important for the API to clearly state what it is doing, which is launching a subprocess.
Quick comment .. I came here to comment on disliking the freestanding run function, and read the discussion on that, so it's already been covered. Consider this another vote for not adding it to the global namespace.
But if making Swift an ergonomic scripting language really is the goal, then
try await run(.name("ls"))
would read cleaner as
try await run("ls"))
Do we need the label?
(Also, I really don't think a strongly typed compiled language is going to be widely accepted as a scripting language, that's just my opinion, but it's why I'm reluctant to let that particular use case into the global namespace).
IMO I'd rather keep scripting (as people usually want it to be equivalent to single-file Bash/Python scripts) out of scope of this discussion. There's already a non-trivial amount of boilerplate required for every call to start a new Subprocess instance, like try await. Subprocess as proposed reduces the amount of boilerplate considerably, when compared to Process, but doesn't make it completely go away. IO redirection, chaining commands etc is quite convoluted, especially if you'd like everything to work cleanly WRT concurrency and scheduling. Making it a free function won't make scripting that much less tedious.
What I'd like to see is for this proposal to make creation of shell-like scripting libraries easier. One could imagine something built on top of Subprocess with function body macros that looks like this:
and for this trivial straw-man example, expands to
func originFetcher() async throws -> String {
let output1 = try await ShellScript.fetch("https://httpbin.org/get")
let output2 = ShellScript.jsonParse(output1)
let result = ShellScript.getField(path: "origin")(output2)
return result
}
There are a lot of details that such library could abstract away and solve under the hood, but I wouldn't expect Subprocess to do that on its own.
Scripting was identified as a justification for run being a freestanding function, so it seems in scope? If we're not considering scripting then I don't see a reason for run being a freestanding function.
Making the freestanding functions part of a scripting library - that sounds good to me.
I have been thinking a lot about this proposal over the past days and the evolution of this proposal over the various pitches and reviews. All the engagement throughout shows how important this is to many Swift contributors and that it fills a gap in the current ecosystem. During my various review comments I had one constant train of thought: "How can we be sure this works after review on all platforms?" I was never able to answer that myself because I was lacking the knowledge of how processes are handled on platforms like Windows or potentially future Swift supported platforms.
When I saw that this is pitched now to be created in a separate repository outside of Foundation I was incredibly happy since it allowed us to iterate on it more and not tie it to the high API and ABI bar that Foundation is upheld to. I think we should actually go one step further and position this review as a round of feedback to get an initial package out there but fully accept that this package needs time to incubate and be battle tested before we can commit to the APIs. I fully expect that if we start to use any API in production use-cases we will find small and potentially large bits that need to change.
I have been looking through the updated implementation and re-read the proposal and I disagree that this is progressive disclosure. To me this looks rather that we are lacking core primitive abstractions that Subprocess is trying to invent here. The reason why I'm thinking this way is two-fold:
With the proposed APIs the underlying implementation is forced to make synchronization decisions that are bad for performance. A lot of the code is using mutexes to protect the file descriptors or an actor in the case of the writer. This shouldn't be necessary really. At any point in time we should have a clear idea who the owner of the file descriptor is and make it possible to do the I/O completely lock/actor free.
@johannesweiss mentioned this regarding async creation/closure of the file descriptors already. I agree that subprocess is not in the game of defining FileDescriptor APIs but it shows that the we don't have proper APIs right now. swift-system only provides synchronous APIs and they are not correct working with things like IO_URING.
Now the previous iteration of this proposal didn't have this problem since it hide all of these details in the high level run methods allowing us to correctly implement it below or move the backing implementation to things like IO_URING if we wanted to. The updated proposal is exposing so many implementation details which opens up a completely new problems spaces that ought to be solve by lower level building blocks.
I personally think we should take a step back and reduce the complexity we added with the new protocols here and instead overload the run method again. I would even go as far and drop a few of the overloads that the run method had even if that requires a bit more boilerplatey code.
I think it is important to look for opportunities to reduce the amount of ceremony required to do simple things. It improves the approachability of the language as a whole.
There is always an option to add the module name to the function to disambiguate where needed. The compiler produces a reasonable fix-it in this case, where the combination of ArgumentParser and Subprocess creates an error.
If this were a function in Foundation, and therefore automatically imported into nearly every Swift file, I would feel differently. But - it's not. It'll only be there in files where you import the module.
In addition, this pitch is for a 0.1 version of the package. If we truly come to regret this decision (or any other of the API decisions here), we have an opportunity to correct it before we get to a 1.0.
As long as it doesn't adopt an unwritten "never make breaking changes" then that seems fine until a major release.
Probably for another thread: I vouch that there needs to be some kind of official documentation/Swift Lang Manifesto for non-stdlib (or even Foundation) packages on how evolution of APIs can or should take place. I'm not talking about the forum evolution process, but how package APIs can even be pitched to change in the first place.
If there's no standardization, I don't feel confident that all Swift Lang packages will adhere to similar rules that allow API evolution.
I did try out this configuration. While it's unfortunately that Swift is unable to resolve to the correct run method (which IMO is a bug in Swift), it does produce the correct diagnostics and fix me:
Use of 'run' refers to instance method rather than global function 'run(_:arguments:environment:workingDirectory:platformOptions:input:output:error:)' in module 'Subprocess'
...
use 'Subprocess.' to reference the global function in module 'Subprocess'
let result = try await run(.name("ls"))
This diagnostics here is correct and the fix me suggested is also correct. To me this error message and fix me is sufficient to guide developers to use Subprocess. when they use ArgumentParser.
I understand that using ArgumentParser means you will have to write Subprocess.run. However, I don't think we should force other use sites to always write Subprocess. just because this common use case requires it.
1 day past the review deadline, but I'm not getting paid for writing this anyway (as in: I using my own free time), so I do not need to stress about it.
Other packages
Do you remember what was wrong with the other packages? It would be nice if the proposal included sentences like:
There is a potential problem X which this proposal solves by doing Y [and the user is required to do Z].
Sometimes I go back to the proposals months after they were accepted, mostly when I start using the proposed feature. Having a reminder why a certain decision was made, without going to the forum, would be helpful.
posix_spawnattr_t etc.
public struct PlatformOptions: Sendable, Hashable {
public var preSpawnProcessConfigurator: (
@Sendable (
inout posix_spawnattr_t?,
inout posix_spawn_file_actions_t?
) throws -> Void
)? = nil
}
I agree with others that exposing the posix_spawnattr_t/posix_spawn_file_actions_t may not be the best idea.
For years I have been using Hexagonal architecture (“Ports and adapters” thingie): you do not allow dependencies into the “core” business logic, so for every dependency you create an abstraction layer.
Subprocess lives at the edge of the system, so if we wanted to follow the “Hexagonal architecture” we would wrap the posix_spawnattr_t/posix_spawn_file_actions_t. This way we can change the Subprocess internals without changing the business logic.
That said posix_spawnattr_t/posix_spawn_file_actions_t are fairly big and difficult to wrap.
I'm not sure that is is enough. Personally I would store Bool in a property after the monitorProcessTermination finishes. If termination sequence was delayed (or somehow run again) it would do nothing, even in there is a new process with the same pid.
In theory this is still a race condition, but even the current implementation has a race condition: process exits after isAlive() but before we send signal. IIRC this is not a problem on Linux because pids are reused “going up”, so we would have to cycle all of the pids to get back at the one we had.
Async IO
In theory on Linux you can use a single epoll for everything.
When reading from the pipe (I simplified things, to make it more readable):
Create continuation
epoll determines that the file is readable -> resume continuation
Read
When writing:
Create continuation
epoll determines that the file is writable -> resume continuation
Write
Repeat until all bytes are written
Note that epoll will return “writable” as long as the pipe is not full, for example:
we want to write 4 bytes
pipe buffer is 16 bytes
pipe already contains 15 bytes
epoll determines it as writable - we write 1 byte, and try again with the remaining 3 bytes.
With non-blocking IO we can do a preemptive read/write. In the worst case nothing will happen. We (obviously) split our data into a PIPE_BUF sized chunks.
While one thread is blocked in a call to epoll_wait(), it is
possible for another thread to add a file descriptor to the
waited-upon epoll instance. If the new file descriptor becomes
ready, it will cause the epoll_wait() call to unblock.
This is where things get more interesting. As far as I know you can use pidfile in epoll. This replaces the “wait” part of waitpid:
epoll waits for the pidfile to know when the child exists
then waitpid to reap the process
This puts us in a fairy () interesting place, as in a way Subprocess becomes an empty shell:
waitpid is mostly done by epoll
async reading/writing is also done by epoll
This archives the goal of being 100% Swift concurrency neutral - we do not block global concurrent executor. Also, we only need 1 epoll regardless of the number of child processes, so the whole solution scales in the same way as epoll.
Child watcher implementation using Linux's pid file descriptors.
This child watcher polls process file descriptors (pidfds) to await child
process termination. In some respects, PidfdChildWatcher is a "Goldilocks"
child watcher implementation. It doesn't require signals or threads, doesn't
interfere with any processes launched outside the event loop, and scales
linearly with the number of subprocesses launched by the event loop. The
main disadvantage is that pidfds are specific to Linux, and only work on
recent (5.3+) kernels.
This is for epoll, I don't know about the kqueue or Windows. Those OS will use their own solutions. I have never used io_uring.
It would be nice if Swift had some abstraction over different poll implementations, similar to Rust/tokio-rs/mio.
Read all
If I wanted to capture the whole output how would I do that? Probably StringOutput(limit: Int, encoding: Encoding.Type), but what limit should I use?
Btw. shouldn't “read all” be the default? Otherwise we run into a risk of:
During development/testing I use small file - this works
In production I suddenly receive a big file - this will read only the first 128kb
This can be prevented by carefully reading the docs, but there are a lot of docs to read.
SubprocessFoundation
Can't this be replaced by #if canImport(Foundation)? Then we would have only 1 module.
Documentation
If we look at the very 1st proposed API:
/// Run a executable with given parameters and a custom closure
/// to manage the running subprocess' lifetime and its IOs.
public func run<…>(
_ executable: Executable,
arguments: Arguments = [],
environment: Environment = .inherit,
workingDirectory: FilePath? = nil,
platformOptions: PlatformOptions = PlatformOptions(),
input: Input = .none,
output: Output = .string,
error: Error = .discarded
) async throws -> CollectedResult<Output, Error>
Where is the “custom closure”? This method has the same documentation as: body: (@escaping (Execution<Output, Error>) async throws -> Result).
There were so many changes to the proposal, that I think it needs an editor - somebody who would proof read the whole thing (just like in newspapers).
Complicated input
I feel like IO became quite complicated, for input we have (this is a copy from the proposal to show of the volume of the new API):
public final class Pipe: Sendable {
public init() {}
}
public protocol InputProtocol: Sendable {
func readFileDescriptor() throws -> FileDescriptor?
func writeFileDescriptor() throws -> FileDescriptor?
func closeReadFileDescriptor() throws
func closeWriteFileDescriptor() throws
func write(into writeFileDescriptor: FileDescriptor) async throws
}
public protocol ManagedInputProtocol: InputProtocol {
var pipe: Pipe { get }
}
public final class NoInput: InputProtocol { }
public final class FileDescriptorInput: InputProtocol { }
public final class StringInput<InputString, Encoding>: ManagedInputProtocol { }
public final class ArrayInput: ManagedInputProtocol { }
/// A concrete `Input` type for subprocess that indicates that
/// the Subprocess should read its input from `StandardInputWriter`.
public struct CustomWriteInput: ManagedInputProtocol { }
extension InputProtocol where Self == NoInput {
public static var none: Self { get }
}
extension InputProtocol where Self == FileDescriptorInput {
public static func fileDescriptor(_ fd: FileDescriptor, closeAfterSpawningProcess: Bool) -> Self
}
extension InputProtocol {
public static func array(_ array: [UInt8]) -> Self where Self == ArrayInput
public static func string<InputString>(_ string: InputString) -> Self where Self == StringInput<InputString, UTF8>
public static func string<InputString, Encoding>(_ string: InputString, using encoding: Encoding.Type) -> Self where Self == StringInput<InputString, Encoding>
}
// SubprocessFoundation
public final class DataInput: ManagedInputProtocol { }
public struct DataSequenceInput<InputSequence>: ManagedInputProtocol where InputSequence.Element == Data { }
public struct DataAsyncSequenceInput<InputSequence>: ManagedInputProtocol where InputSequence.Element == Data { }
extension InputProtocol {
public static func data(_ data: Data) -> Self where Self == DataInput
public static func sequence<InputSequence>(_ sequence: InputSequence) -> Self where Self == DataSequenceInput<InputSequence>
public static func sequence<InputSequence>(_ asyncSequence: InputSequence) -> Self where Self == DataAsyncSequenceInput<InputSequence>
}
(That was only the input part!)
Why Encoding is a generic argument to StringInput? Can't it be a private property? In which scenario(s) does it make a difference? Do we actually get anything out of it?
Pipe has only a init. This is weird.
Pipe has the same name as Foundation.Pipe, so in 99% of the cases users would have to write it as Subprocess.Pipe.
Just by looking at the proposal the ManagedInputProtocol makes no sense. We do not gain anything from conforming to it. The whole ManagedOutputProtocol seems under-described in the proposal. The code makes it more clear:
Why NoInput, FileDescriptorInput, StringInput, ArrayInput are final class? They should be struct.
I do not like “Custom” in class/function names as it does not tell anything to the user. What is so “custom” about CustomWriteInput? Also, the documentation is a 1 sentence that contains “input/that/subprocess” 2 times each:
/// A concrete `Input` type for subprocess that indicates that
/// the Subprocess should read its input from `StandardInputWriter`.
public struct CustomWriteInput: ManagedInputProtocol { }
Btw. So many symbols! And the output is largely the same which makes the whole IO huge.
Can you generate docs for this module? Over the years I discovered that there is correlation between the size of the documentation and the amount of people who actually read it.
I think that the proposal overestimates how much time people spend reading the docs, and it would be hugely beneficial if we removed some things.
NoInput
Why do we need NoInput? Can't we use nil? This is the default, so most of the time users will never write .none. NoInput just clutters the namespace.
// Before
_ = try await run(
.name("cat"),
input: .none // This is the default, but I made it explicit
)
// After
_ = try await run(
.name("cat"),
input: nil // Still the default
)
Changes:
Make input argument optional with nil as default.
Make NoInput private
Use let realInput = inputFromArgument ?? NoInput()
Remove ManagedInputProtocol (controversial one)
In proposal we have:
public final class Pipe: Sendable {
public init() {}
}
/// `ManagedInputProtocol` is managed by `Subprocess` and
/// utilizes its `Pipe` type to facilitate input writing.
/// Developers have the option to implement custom
/// input types by conforming to `ManagedInputProtocol`
/// and implementing the `write(into:)` method.
public protocol ManagedInputProtocol: InputProtocol {
var pipe: Pipe { get }
}
extension ManagedInputProtocol {
public func readFileDescriptor() throws -> FileDescriptor? {
return try self.pipe.readFileDescriptor(creatingIfNeeded true)
}
public func writeFileDescriptor() throws -> FileDescriptor? {
return try self.pipe.writeFileDescriptor(creatingIfNeeded true)
}
public func closeReadFileDescriptor() throws {
try self.pipe.closeReadFileDescriptor()
}
public func closeWriteFileDescriptor() throws {
try self.pipe.closeWriteFileDescriptor()
}
}
Pipe is empty and ManagedInputProtocol is a convenience mixin that acts as a proxy.
ManagedInputProtocol can be confused with InputProtocol, at least I was confused when reading the proposal.
ManagedInputProtocol.XXX just calls pipe.XXX.
In total ManagedInputProtocol saves users from writing exactly 4 lines of code.
This protocol is not pulling its weight.
Changes:
Make those 4 methods on Pipe public.
Make ManagedInputProtocol internal for StringInput and friends.
After those changes InputProtocol is the only InputProtocol we export -> no confusion here. I think it is cleaner and maps to a simpler mental model.
Complicated output
Same as input, but slightly “noisier”. In the proposal we have:
@available(macOS 9999, *) // Span equivelent
public protocol ManagedOutputProtocol: OutputProtocol {
var pipe: Pipe { get }
func output(from span: RawSpan) throws -> OutputType
/// The max amount of data to collect for this output.
var maxSize: Int { get }
}
@available(macOS 9999, *) // Span equivelent
public struct StringOutput<Encoding: Unicode.Encoding>: ManagedOutputProtocol { }
StringOutput has to export public let maxSize. I think that maxSize should only be present in the initializer and stored in a private property. Is there any scenario where it would be useful later?
maxSize does not have a unit. The standard naming in Swift for a size/length is count, for example Array.count. Maybe [max]byteCount would be better?
Btw2. the naming is inconsistent, we have maxSize, but in OutputProtocol we have limit.
Btw3. I'm not sure what is the convention here, but usually method names are verbs/actions, so OutputProtocol.bytes is slightly weird. In defense of the proposal there is something like that already in Swift - probably in async URLSession or AsyncSequence.
Same naming remark for OutputProtocol - I added comments in the code below:
public protocol OutputProtocol: Sendable {
// This should be 'create[Read/Write]FileDescriptor'.
// 'readFileDescriptor' suggests 'FileDescriptor.read' not creating it.
func readFileDescriptor() throws -> FileDescriptor?
func writeFileDescriptor() throws -> FileDescriptor?
// Those are good.
func consumeReadFileDescriptor() -> FileDescriptor?
func closeReadFileDescriptor() throws
func closeWriteFileDescriptor() throws
func captureOutput() async throws -> OutputType
}
Btw4. in which case I would have a readFileDescriptor without the writeFileDescriptor? We pass no write handle to the child, but we still expect to read something. I have not thought about this, but the API allows me to do this, so I'm wondering what would be the meaning of that. Are all 4 ((read, write)/(read, nil)/(nil, write)/(nil, nil)) combinations supported? If some combination is not supported then the code should not compile.
) interesting place, as in a way