Skip to content

[WIP -- looking for feedback] Unified Exec Wrapper #445

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
219 changes: 219 additions & 0 deletions run/exec.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,219 @@
// Copyright 2023 The Authors (see AUTHORS file)
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

// Package run provides enhanced functionality for executing external commands,
// offering control over environment variables, process attributes, I/O,
// timeouts, and integrated logging, using a functional options pattern.
package run

import (
"bytes"
"context"
"errors"
"fmt"
"io"
"os"
"os/exec"
"path/filepath"
"slices"
"strings"
"time"

"github.com/abcxyz/pkg/logging" // Import the specific logging package
)

// DefaultRunTimeout is how long commands wait if the context doesn't have a deadline.
const DefaultRunTimeout = time.Minute
Copy link
Contributor

@sailorlqh sailorlqh Apr 21, 2025

Choose a reason for hiding this comment

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

RunTimeout and DefaultWaitDelay have different units, do you think it might be more user friendly to change this to 60 * time.Second?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/abcxyz/abc/blob/49babab96513de693386e38b4ca1ff6a335aeeb1/templates/common/run/run.go#L30

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


// DefaultWaitDelay is how long after context cancellation processes are actually killed
// if another value isn't set. See exec.Cmd.WaitDelay for more information.
const DefaultWaitDelay = time.Second

// Simple is a wrapper around [Run] that captures stdout and stderr as strings.
// This is intended to be used for commands that run non-interactively then
// exit.
//
// Sub-command inherits env variables.
//
// If the command exits with a nonzero status code, an *exec.ExitError will be
// returned.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Casting copies the data...

//
// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these return values just named for the purposes of docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, not sure if there is a better way to do this

var stdoutBuf, stderrBuf bytes.Buffer
opts := &Option{
Stdout: &stdoutBuf,
Stderr: &stderrBuf,
Env: os.Environ(),
}

_, err := Run(ctx, opts, args...)

return stdoutBuf.String(), stderrBuf.String(), err
}

// Run executes the command specified by args, applying configurations from opts.
//
// By default, a non-zero exit code results in an error (*exec.ExitError),
// unless the AllowNonzeroExit option is used.
// The error message includes stdout/stderr content if they are captured to buffers.
//
// If the context doesn't have a deadline, DefaultRunTimeout is applied.
//
// The input args must have len>=1. opts may be nil if no special options are
// needed.
//
// This doesn't execute a shell (unless of course args[0] is the name of a shell
// binary).
func Run(ctx context.Context, opts *Option, args ...string) (exitCode int, _ error) {
logger := logging.FromContext(ctx)

if len(args) == 0 {
logger.DebugContext(ctx, "run called with no arguments")
return -1, errors.New("run: must provide at least one argument (the command)")
}

// Apply default timeout if none exists on the context.
if _, ok := ctx.Deadline(); !ok {
var cancel context.CancelFunc
ctx, cancel = context.WithTimeout(ctx, DefaultRunTimeout)
defer cancel() // Ensure the derived context is cancelled
}

// #nosec G204 -- Execution of external commands is the purpose of this package. Inputs must be trusted by the caller.
cmd := exec.CommandContext(ctx, args[0], args[1:]...)

cmd.Dir = opts.Cwd
cmd.Stdin = opts.Stdin

if opts.Stdout != nil {
cmd.Stdout = opts.Stdout
} else {
cmd.Stdout = os.Stdout
}
if opts.Stderr != nil {
cmd.Stderr = opts.Stderr
} else {
cmd.Stderr = os.Stderr
}

cmd.Env = opts.Env

if opts.WaitDelay != nil {
cmd.WaitDelay = *opts.WaitDelay
} else {
cmd.WaitDelay = DefaultWaitDelay
}
logger.DebugContext(ctx, "set wait delay", "delay", cmd.WaitDelay)

logger.DebugContext(ctx, "starting command",
"args", args,
"options", opts,
)
err := cmd.Run() // This blocks until the command exits or context is cancelled
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is the right pattern. There's no way to interrupt or signal the command like this, and it blocks all execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because its using a commandcontext, canceling the context will send a stop signal.

As far as blocking goes, I think that should be fine, if someone really cares they can always wrap in a goroutine.

I could see an argument if you feel like there will likely need to be other signals sent to the command, in which case it would be nice to get the PID back somehow.


// Exit code -1 if ProcessState is nil (e.g., start failed)
exitCode = -1
if cmd.ProcessState != nil {
exitCode = cmd.ProcessState.ExitCode()
}

if err != nil {
var exitErr *exec.ExitError

if errors.As(err, &exitErr) {
if opts.AllowedExitCodes[0] == -1 || slices.Contains(opts.AllowedExitCodes, exitCode) {
logger.DebugContext(ctx, "command exited non-zero, but allowed by option", "exit_code", exitCode)
err = nil
} else {
logger.DebugContext(ctx, "command exited non-zero",
"exit_code", exitCode,
"error", err,
)
err = fmt.Errorf("command %v exited non-zero (%d): %w (context error: %v)\nstdout:\n%s\nstderr:\n%s",
args, exitCode, err, ctx.Err(), cmd.Stdout, cmd.Stderr) //nolint:errorlint
}
}
} else {
// Command ran successfully (exit code 0)
logger.DebugContext(ctx, "command finished successfully", "exit_code", exitCode)
}

return exitCode, err
}

// Option implements the functional options pattern for [Run].
// It holds all configurable settings for executing a command.
type Option struct {
// AllowNonzeroExitCodes prevents Run from returning error when one of
// the listed non-zero status codes appear. If AllowedExitCodes[0] == -1,
// all exit codes are allowed.
AllowedExitCodes []int

// Runs command in specified working directory.
Cwd string
// Env contains the list of env vars in KEY=VALUE form. Default is no env vars.
Env []string
// Sets the cmd's Stdin. Defaults is no Stdin.
Stdin io.Reader
// Sets the cmd's Stdout. Default is os.Stdout.
Stdout io.Writer
// Sets the cmd's Stderr. Default is os.Stderr.
Stderr io.Writer
// Sets a grace period for the command to exit after the context
// is cancelled before being forcefully killed.
// Default: DefautWaitDelay. 0 is no wait delay.
// See exec.Cmd.WaitDelay for more information.
WaitDelay *time.Duration
}

// TODO: handle process group and cancel semantics.

// anyGlobMatch checks if string s matches any of the glob patterns.
func anyGlobMatch(s string, patterns []string) bool {
for _, p := range patterns {
if p == "*" || p == ".*" { // Common fast-path wildcards
return true
}
}
for _, p := range patterns {
if ok, _ := filepath.Match(p, s); ok { // Ignore Match errors
return true
}
}
return false
}

// Environ is a utility to compile environment variables to pass to the child.
// The overridden environment is always added, even if not explicitly
// allowed/denied.
func Environ(osEnv, allowedKeys, deniedKeys, overrideEnv []string) []string {
finalEnv := make([]string, 0, len(osEnv)+len(overrideEnv))

// Select keys that match the allow filter (if given) but not the deny filter.
for _, v := range osEnv {
k := strings.SplitN(v, "=", 2)[0]
if (len(allowedKeys) == 0 || anyGlobMatch(k, allowedKeys)) &&
!anyGlobMatch(k, deniedKeys) {
finalEnv = append(finalEnv, v)
}
}

// Add overrides at the end, after any filtering. os/exec has "last wins"
// semantics, so appending is an overriding action.
finalEnv = append(finalEnv, overrideEnv...)

return finalEnv
}
72 changes: 72 additions & 0 deletions run/exec_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// Copyright 2025 The Authors (see AUTHORS file)
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package run

import (
"testing"

"github.com/google/go-cmp/cmp"

"github.com/abcxyz/pkg/testutil"
)

// This may not be portable due to depending on behavior of external programs.
func TestSimple(t *testing.T) {
t.Parallel()

cases := []struct {
name string
args []string
wantOut string
wantStdErr string
wantCode int
wantErr string
}{
{
name: "happy_path",
args: []string{"echo", "foo", "bar bazz"},
wantOut: "foo bar bazz\n",
},
{
name: "no_command_error",
args: []string{"echoooocrapimistyped", "foo", "bar bazz"},
wantErr: "not found",
},
{
name: "exit_code_error",
args: []string{"cat", "/fake/file/path/should/fail"},
wantStdErr: "cat: /fake/file/path/should/fail: No such file or directory\n",
wantErr: "exited non-zero (1): exit status 1 (context error: <nil>)\nstdout:\n\nstderr:\ncat:",
},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
ctx := t.Context()
stdout, stderr, err := Simple(ctx, tc.args...)

if diff := cmp.Diff(stdout, tc.wantOut); diff != "" {
t.Errorf("stdout was not as expected(-got,+want): %s", diff)
}
if diff := cmp.Diff(stderr, tc.wantStdErr); diff != "" {
t.Errorf("stderr was not as expected(-got,+want): %s", diff)
}
if diff := testutil.DiffErrString(err, tc.wantErr); diff != "" {
t.Error(diff)
}
})
}
}
Loading