[WIP -- looking for feedback] Unified Exec Wrapper #445
Conversation
Simple() tested, looking for overall feedback before committing more thorough testing. Signed-off-by: pdewilde <[email protected]>
| ) | ||
|
|
||
| // DefaultRunTimeout is how long commands wait if the context doesn't have a deadline. | ||
| const DefaultRunTimeout = time.Minute |
There was a problem hiding this comment.
RunTimeout and DefaultWaitDelay have different units, do you think it might be more user friendly to change this to 60 * time.Second?
There was a problem hiding this comment.
The are both time.Duration which under the hood is in nanoseconds.
They are actually just
const (
Nanosecond [Duration](https://pkg.go.dev/time#Duration) = 1
Microsecond = 1000 * [Nanosecond](https://pkg.go.dev/time#Nanosecond)
Millisecond = 1000 * [Microsecond](https://pkg.go.dev/time#Microsecond)
Second = 1000 * [Millisecond](https://pkg.go.dev/time#Millisecond)
Minute = 60 * [Second](https://pkg.go.dev/time#Second)
Hour = 60 * [Minute](https://pkg.go.dev/time#Minute)
)
IDK if 60 * time.Second is easier to grok than time.Mintue
There was a problem hiding this comment.
They are all int64 under the hood so it doesn't matter. I do wonder why we're setting such a long default? It seems like most commands should finish in like < 5s, or manually have no timeout. We also want the ability to disable the timeout entirely, since we rely on the subcommand to handle the timeouts.
There was a problem hiding this comment.
I kept the default from abc. I assume the thinking there was it likely would be calling a long-running process (compilation or something similar).
Maybe for a general library that should be shorter though
There was a problem hiding this comment.
Having written one of these before, I think there's a lot of gaps around managing the child process.
| ) | ||
|
|
||
| // DefaultRunTimeout is how long commands wait if the context doesn't have a deadline. | ||
| const DefaultRunTimeout = time.Minute |
There was a problem hiding this comment.
They are all int64 under the hood so it doesn't matter. I do wonder why we're setting such a long default? It seems like most commands should finish in like < 5s, or manually have no timeout. We also want the ability to disable the timeout entirely, since we rely on the subcommand to handle the timeouts.
| // | ||
| // If the command fails, the error message will include the contents of stdout | ||
| // and stderr. This saves boilerplate in the caller. | ||
| func Simple(ctx context.Context, args ...string) (stdout, stderr string, _ error) { |
There was a problem hiding this comment.
Are these return values just named for the purposes of docs?
There was a problem hiding this comment.
Yeah, not sure if there is a better way to do this
| // exit. | ||
| // | ||
| // If the command exits with a nonzero status code, an *exec.ExitError will be | ||
| // returned. |
There was a problem hiding this comment.
I'd also clarify that it's only good for commands that produce minimal output that can be represented as a string (since we're serializing everything to a string). If there's a command that outputs bytes, this won't work and would need a more complex version.
We could fix half that problem by returning []byte instead of string, but it can make things more annoying to work with for simple cases.
There was a problem hiding this comment.
Couldn't you just cast the string back to a []byte if that was an issue?
My understanding is go strings are just []byte under the hood, and are not required to hold valid utf-8
There was a problem hiding this comment.
There was a problem hiding this comment.
Casting copies the data...
run/exec.go
Outdated
| stdin io.Reader | ||
| stdout io.Writer | ||
| stderr io.Writer | ||
| waitDelay *time.Duration |
There was a problem hiding this comment.
This shouldn't be a pointer (it's an int64)
There was a problem hiding this comment.
Its a pointer as its an optional value where the default value has semantic meaning (Wait forever) different from the default behaivor we want (set to DefaultWaitDelay const)
That is assuming we want DefaultWaitDelay as a default, rather than cmd's normal behavior
There was a problem hiding this comment.
The default value for an int (and time.Duration) is 0, so it's functionally equivalent? Or are you trying to differentiate between "0" and "unset"?
There was a problem hiding this comment.
Yes. Default value for int* is nil, which will cause DefaultWaitDelay to be used.
0 is already well defiend by cmd.Run:
// If WaitDelay is zero (the default), I/O pipes will be read until EOF,
// which might not occur until orphaned subprocesses of the command have
// also closed their descriptors for the pipes.
which isn't the default behavior in guardian's library, assumed that behavior was wanted: https://github.com/abcxyz/guardian/blob/main/pkg/child/child.go#L93
This is my first crack at #323:
Both guardian and abc have helper libraries that wrap the os/exec library. In abcxyz/abc#531 (comment) we agreed that these probably should be factored out into a shared common library in pkg.
I have tried to take the features of both and make something combined, though I fear maybe this is getting too messy.
Changes:
Many()that the abc library had. It feels like it won't be often used and logic may better live in the client, as they may not want theSimple()semantics.Concerns: