exec: new API, replace Run with Command

This removes exec.Run and replaces exec.Cmd with a
new implementation. The new exec.Cmd represents
both a currently-running command and also a command
being prepared. It has a good zero value.

You can Start + Wait on a Cmd, or simply Run it.
Start (and Run) deal with copying stdout, stdin,
and stderr between the Cmd's io.Readers and
io.Writers.

There are convenience methods to capture a command's
stdout and/or stderr.

R=r, n13m3y3r, rsc, gustavo, alex.brainman, dsymonds, r, adg, duzy.chan, mike.rosset, kevlar
CC=golang-dev
https://golang.org/cl/4552052

misc/dashboard/builder/exec.go

@@ -19,16 +19,11 @@ func run(envv []string, dir string, argv ...string) os.Error {
 		log.Println("run", argv)
 	}
 	argv = useBash(argv)
-	bin, err := lookPath(argv[0])
-	if err != nil {
-		return err
-	}
-	p, err := exec.Run(bin, argv, envv, dir,
-		exec.DevNull, exec.DevNull, exec.PassThrough)
-	if err != nil {
-		return err
-	}
-	return p.Close()
+	cmd := exec.Command(argv[0], argv[1:]...)
+	cmd.Dir = dir
+	cmd.Env = envv
+	cmd.Stderr = os.Stderr
+	return cmd.Run()
 }
 
 // runLog runs a process and returns the combined stdout/stderr, 
@@ -38,16 +33,7 @@ func runLog(envv []string, logfile, dir string, argv ...string) (output string,
 		log.Println("runLog", argv)
 	}
 	argv = useBash(argv)
-	bin, err := lookPath(argv[0])
-	if err != nil {
-		return
-	}
-	p, err := exec.Run(bin, argv, envv, dir,
-		exec.DevNull, exec.Pipe, exec.MergeWithStdout)
-	if err != nil {
-		return
-	}
-	defer p.Close()
+
 	b := new(bytes.Buffer)
 	var w io.Writer = b
 	if logfile != "" {
@@ -58,23 +44,22 @@ func runLog(envv []string, logfile, dir string, argv ...string) (output string,
 		defer f.Close()
 		w = io.MultiWriter(f, b)
 	}
-	_, err = io.Copy(w, p.Stdout)
-	if err != nil {
-		return
-	}
-	wait, err := p.Wait(0)
+
+	cmd := exec.Command(argv[0], argv[1:]...)
+	cmd.Dir = dir
+	cmd.Env = envv
+	cmd.Stdout = w
+	cmd.Stderr = w
+
+	err = cmd.Run()
+	output = b.String()
 	if err != nil {
+		if ws, ok := err.(*os.Waitmsg); ok {
+			exitStatus = ws.ExitStatus()
+		}
 		return
 	}
-	return b.String(), wait.WaitStatus.ExitStatus(), nil
-}
-
-// lookPath looks for cmd in $PATH if cmd does not begin with / or ./ or ../.
-func lookPath(cmd string) (string, os.Error) {
-	if strings.HasPrefix(cmd, "/") || strings.HasPrefix(cmd, "./") || strings.HasPrefix(cmd, "../") {
-		return cmd, nil
-	}
-	return exec.LookPath(cmd)
+	return
 }
 
 // useBash prefixes a list of args with 'bash' if the first argument

misc/goplay/goplay.go

@@ -5,7 +5,6 @@
 package main
 
 import (
-	"bytes"
 	"exec"
 	"flag"
 	"http"
@@ -140,32 +139,7 @@ func error(w http.ResponseWriter, out []byte, err os.Error) {
 
 // run executes the specified command and returns its output and an error.
 func run(cmd ...string) ([]byte, os.Error) {
-	// find the specified binary
-	bin, err := exec.LookPath(cmd[0])
-	if err != nil {
-		// report binary as well as the error
-		return nil, os.NewError(cmd[0] + ": " + err.String())
-	}
-
-	// run the binary and read its combined stdout and stderr into a buffer
-	p, err := exec.Run(bin, cmd, os.Environ(), "", exec.DevNull, exec.Pipe, exec.MergeWithStdout)
-	if err != nil {
-		return nil, err
-	}
-	var buf bytes.Buffer
-	io.Copy(&buf, p.Stdout)
-	w, err := p.Wait(0)
-	p.Close()
-	if err != nil {
-		return nil, err
-	}
-
-	// set the error return value if the program had a non-zero exit status
-	if !w.Exited() || w.ExitStatus() != 0 {
-		err = os.ErrorString("running " + cmd[0] + ": " + w.String())
-	}
-
-	return buf.Bytes(), err
+	return exec.Command(cmd[0], cmd[1:]...).CombinedOutput()
 }
 
 var frontPage, output *template.Template // HTML templates

src/cmd/gofix/main.go

@@ -248,17 +248,5 @@ func diff(b1, b2 []byte) (data []byte, err os.Error) {
 	f1.Write(b1)
 	f2.Write(b2)
 
-	diffcmd, err := exec.LookPath("diff")
-	if err != nil {
-		return nil, err
-	}
-
-	c, err := exec.Run(diffcmd, []string{"diff", f1.Name(), f2.Name()}, nil, "",
-		exec.DevNull, exec.Pipe, exec.MergeWithStdout)
-	if err != nil {
-		return nil, err
-	}
-	defer c.Close()
-
-	return ioutil.ReadAll(c.Stdout)
+	return exec.Command("diff", f1.Name(), f2.Name()).CombinedOutput()
 }

src/cmd/gofmt/gofmt.go

@@ -245,14 +245,14 @@ func gofmtMain() {
 func diff(b1, b2 []byte) (data []byte, err os.Error) {
 	f1, err := ioutil.TempFile("", "gofmt")
 	if err != nil {
-		return nil, err
+		return
 	}
 	defer os.Remove(f1.Name())
 	defer f1.Close()
 
 	f2, err := ioutil.TempFile("", "gofmt")
 	if err != nil {
-		return nil, err
+		return
 	}
 	defer os.Remove(f2.Name())
 	defer f2.Close()
@@ -260,17 +260,5 @@ func diff(b1, b2 []byte) (data []byte, err os.Error) {
 	f1.Write(b1)
 	f2.Write(b2)
 
-	diffcmd, err := exec.LookPath("diff")
-	if err != nil {
-		return nil, err
-	}
-
-	c, err := exec.Run(diffcmd, []string{"diff", "-u", f1.Name(), f2.Name()},
-		nil, "", exec.DevNull, exec.Pipe, exec.MergeWithStdout)
-	if err != nil {
-		return nil, err
-	}
-	defer c.Close()
-
-	return ioutil.ReadAll(c.Stdout)
+	return exec.Command("diff", "-u", f1.Name(), f2.Name()).CombinedOutput()
 }

src/cmd/goinstall/main.go

@@ -12,7 +12,6 @@ import (
 	"flag"
 	"fmt"
 	"go/token"
-	"io"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -246,37 +245,22 @@ func quietRun(dir string, stdin []byte, cmd ...string) os.Error {
 }
 
 // genRun implements run and quietRun.
-func genRun(dir string, stdin []byte, cmd []string, quiet bool) os.Error {
-	bin, err := exec.LookPath(cmd[0])
+func genRun(dir string, stdin []byte, arg []string, quiet bool) os.Error {
+	cmd := exec.Command(arg[0], arg[1:]...)
+	cmd.Stdin = bytes.NewBuffer(stdin)
+	cmd.Dir = dir
+	vlogf("%s: %s %s\n", dir, cmd.Path, strings.Join(arg[1:], " "))
+	out, err := cmd.CombinedOutput()
 	if err != nil {
-		return err
-	}
-	p, err := exec.Run(bin, cmd, os.Environ(), dir, exec.Pipe, exec.Pipe, exec.MergeWithStdout)
-	vlogf("%s: %s %s\n", dir, bin, strings.Join(cmd[1:], " "))
-	if err != nil {
-		return err
-	}
-	go func() {
-		p.Stdin.Write(stdin)
-		p.Stdin.Close()
-	}()
-	var buf bytes.Buffer
-	io.Copy(&buf, p.Stdout)
-	w, err := p.Wait(0)
-	p.Close()
-	if err != nil {
-		return err
-	}
-	if !w.Exited() || w.ExitStatus() != 0 {
 		if !quiet || *verbose {
 			if dir != "" {
 				dir = "cd " + dir + "; "
 			}
-			fmt.Fprintf(os.Stderr, "%s: === %s%s\n", argv0, dir, strings.Join(cmd, " "))
-			os.Stderr.Write(buf.Bytes())
-			fmt.Fprintf(os.Stderr, "--- %s\n", w)
+			fmt.Fprintf(os.Stderr, "%s: === %s%s\n", cmd.Path, dir, strings.Join(cmd.Args, " "))
+			os.Stderr.Write(out)
+			fmt.Fprintf(os.Stderr, "--- %s\n", err)
 		}
-		return os.ErrorString("running " + cmd[0] + ": " + w.String())
+		return os.ErrorString("running " + arg[0] + ": " + err.String())
 	}
 	return nil
 }

src/cmd/hgpatch/main.go

@@ -10,7 +10,6 @@ import (
 	"exec"
 	"flag"
 	"fmt"
-	"io"
 	"io/ioutil"
 	"os"
 	"patch"
@@ -333,6 +332,7 @@ func run(argv []string, input []byte) (out string, err os.Error) {
 		err = os.EINVAL
 		goto Error
 	}
+
 	prog, ok := lookPathCache[argv[0]]
 	if !ok {
 		prog, err = exec.LookPath(argv[0])
@@ -341,40 +341,15 @@ func run(argv []string, input []byte) (out string, err os.Error) {
 		}
 		lookPathCache[argv[0]] = prog
 	}
-	// fmt.Fprintf(os.Stderr, "%v\n", argv);
-	var cmd *exec.Cmd
-	if len(input) == 0 {
-		cmd, err = exec.Run(prog, argv, os.Environ(), "", exec.DevNull, exec.Pipe, exec.MergeWithStdout)
-		if err != nil {
-			goto Error
-		}
-	} else {
-		cmd, err = exec.Run(prog, argv, os.Environ(), "", exec.Pipe, exec.Pipe, exec.MergeWithStdout)
-		if err != nil {
-			goto Error
-		}
-		go func() {
-			cmd.Stdin.Write(input)
-			cmd.Stdin.Close()
-		}()
-	}
-	defer cmd.Close()
-	var buf bytes.Buffer
-	_, err = io.Copy(&buf, cmd.Stdout)
-	out = buf.String()
-	if err != nil {
-		cmd.Wait(0)
-		goto Error
-	}
-	w, err := cmd.Wait(0)
-	if err != nil {
-		goto Error
+
+	cmd := exec.Command(prog, argv[1:]...)
+	if len(input) > 0 {
+		cmd.Stdin = bytes.NewBuffer(input)
 	}
-	if !w.Exited() || w.ExitStatus() != 0 {
-		err = w
-		goto Error
+	bs, err := cmd.CombinedOutput()
+	if err == nil {
+		return string(bs), nil
 	}
-	return
 
 Error:
 	err = &runError{dup(argv), err}

src/pkg/exec/exec_test.go

@@ -5,163 +5,139 @@
 package exec
 
 import (
+	"fmt"
 	"io"
-	"io/ioutil"
 	"testing"
 	"os"
+	"strconv"
+	"strings"
 )
 
-func run(argv []string, stdin, stdout, stderr int) (p *Cmd, err os.Error) {
-	exe, err := LookPath(argv[0])
-	if err != nil {
-		return nil, err
-	}
-	return Run(exe, argv, nil, "", stdin, stdout, stderr)
+func helperCommand(s ...string) *Cmd {
+	cs := []string{"-test.run=exec.TestHelperProcess", "--"}
+	cs = append(cs, s...)
+	cmd := Command(os.Args[0], cs...)
+	cmd.Env = append([]string{"GO_WANT_HELPER_PROCESS=1"}, os.Environ()...)
+	return cmd
 }
 
-func TestRunCat(t *testing.T) {
-	cmd, err := run([]string{"cat"}, Pipe, Pipe, DevNull)
-	if err != nil {
-		t.Fatal("run:", err)
-	}
-	io.WriteString(cmd.Stdin, "hello, world\n")
-	cmd.Stdin.Close()
-	buf, err := ioutil.ReadAll(cmd.Stdout)
+func TestEcho(t *testing.T) {
+	bs, err := helperCommand("echo", "foo bar", "baz").Output()
 	if err != nil {
-		t.Fatal("read:", err)
+		t.Errorf("echo: %v", err)
 	}
-	if string(buf) != "hello, world\n" {
-		t.Fatalf("read: got %q", buf)
-	}
-	if err = cmd.Close(); err != nil {
-		t.Fatal("close:", err)
+	if g, e := string(bs), "foo bar baz\n"; g != e {
+		t.Errorf("echo: want %q, got %q", e, g)
 	}
 }
 
-func TestRunEcho(t *testing.T) {
-	cmd, err := run([]string{"bash", "-c", "echo hello world"},
-		DevNull, Pipe, DevNull)
-	if err != nil {
-		t.Fatal("run:", err)
-	}
-	buf, err := ioutil.ReadAll(cmd.Stdout)
+func TestCatStdin(t *testing.T) {
+	// Cat, testing stdin and stdout.
+	input := "Input string\nLine 2"
+	p := helperCommand("cat")
+	p.Stdin = strings.NewReader(input)
+	bs, err := p.Output()
 	if err != nil {
-		t.Fatal("read:", err)
-	}
-	if string(buf) != "hello world\n" {
-		t.Fatalf("read: got %q", buf)
+		t.Errorf("cat: %v", err)
 	}
-	if err = cmd.Close(); err != nil {
-		t.Fatal("close:", err)
+	s := string(bs)
+	if s != input {
+		t.Errorf("cat: want %q, got %q", input, s)
 	}
 }
 
-func TestStderr(t *testing.T) {
-	cmd, err := run([]string{"bash", "-c", "echo hello world 1>&2"},
-		DevNull, DevNull, Pipe)
-	if err != nil {
-		t.Fatal("run:", err)
+func TestCatGoodAndBadFile(t *testing.T) {
+	// Testing combined output and error values.
+	bs, err := helperCommand("cat", "/bogus/file.foo", "exec_test.go").CombinedOutput()
+	if _, ok := err.(*os.Waitmsg); !ok {
+		t.Errorf("expected Waitmsg from cat combined; got %T: %v", err, err)
 	}
-	buf, err := ioutil.ReadAll(cmd.Stderr)
-	if err != nil {
-		t.Fatal("read:", err)
+	s := string(bs)
+	sp := strings.Split(s, "\n", 2)
+	if len(sp) != 2 {
+		t.Fatalf("expected two lines from cat; got %q", s)
 	}
-	if string(buf) != "hello world\n" {
-		t.Fatalf("read: got %q", buf)
+	errLine, body := sp[0], sp[1]
+	if !strings.HasPrefix(errLine, "Error: open /bogus/file.foo") {
+		t.Errorf("expected stderr to complain about file; got %q", errLine)
 	}
-	if err = cmd.Close(); err != nil {
-		t.Fatal("close:", err)
+	if !strings.Contains(body, "func TestHelperProcess(t *testing.T)") {
+		t.Errorf("expected test code; got %q (len %d)", body, len(body))
 	}
 }
 
-func TestMergeWithStdout(t *testing.T) {
-	cmd, err := run([]string{"bash", "-c", "echo hello world 1>&2"},
-		DevNull, Pipe, MergeWithStdout)
-	if err != nil {
-		t.Fatal("run:", err)
-	}
-	buf, err := ioutil.ReadAll(cmd.Stdout)
-	if err != nil {
-		t.Fatal("read:", err)
-	}
-	if string(buf) != "hello world\n" {
-		t.Fatalf("read: got %q", buf)
-	}
-	if err = cmd.Close(); err != nil {
-		t.Fatal("close:", err)
+
+func TestNoExistBinary(t *testing.T) {
+	// Can't run a non-existent binary
+	err := Command("/no-exist-binary").Run()
+	if err == nil {
+		t.Error("expected error from /no-exist-binary")
 	}
 }
 
-func TestAddEnvVar(t *testing.T) {
-	err := os.Setenv("NEWVAR", "hello world")
-	if err != nil {
-		t.Fatal("setenv:", err)
-	}
-	cmd, err := run([]string{"bash", "-c", "echo $NEWVAR"},
-		DevNull, Pipe, DevNull)
-	if err != nil {
-		t.Fatal("run:", err)
-	}
-	buf, err := ioutil.ReadAll(cmd.Stdout)
-	if err != nil {
-		t.Fatal("read:", err)
-	}
-	if string(buf) != "hello world\n" {
-		t.Fatalf("read: got %q", buf)
-	}
-	if err = cmd.Close(); err != nil {
-		t.Fatal("close:", err)
+func TestExitStatus(t *testing.T) {
+	// Test that exit values are returned correctly
+	err := helperCommand("exit", "42").Run()
+	if werr, ok := err.(*os.Waitmsg); ok {
+		if s, e := werr.String(), "exit status 42"; s != e {
+			t.Errorf("from exit 42 got exit %q, want %q", s, e)
+		}
+	} else {
+		t.Fatalf("expected Waitmsg from exit 42; got %T: %v", err, err)
 	}
 }
 
-var tryargs = []string{
-	`2`,
-	`2 `,
-	"2 \t",
-	`2" "`,
-	`2 ab `,
-	`2 "ab" `,
-	`2 \ `,
-	`2 \\ `,
-	`2 \" `,
-	`2 \`,
-	`2\`,
-	`2"`,
-	`2\"`,
-	`2 "`,
-	`2 \"`,
-	``,
-	`2 ^ `,
-	`2 \^`,
-}
+// TestHelperProcess isn't a real test. It's used as a helper process
+// for TestParameterRun.
+func TestHelperProcess(*testing.T) {
+	if os.Getenv("GO_WANT_HELPER_PROCESS") != "1" {
+		return
+	}
+	defer os.Exit(0)
 
-func TestArgs(t *testing.T) {
-	for _, a := range tryargs {
-		argv := []string{
-			"awk",
-			`BEGIN{printf("%s|%s|%s",ARGV[1],ARGV[2],ARGV[3])}`,
-			"/dev/null",
-			a,
-			"EOF",
-		}
-		exe, err := LookPath(argv[0])
-		if err != nil {
-			t.Fatal("run:", err)
-		}
-		cmd, err := Run(exe, argv, nil, "", DevNull, Pipe, DevNull)
-		if err != nil {
-			t.Fatal("run:", err)
+	args := os.Args
+	for len(args) > 0 {
+		if args[0] == "--" {
+			args = args[1:]
+			break
 		}
-		buf, err := ioutil.ReadAll(cmd.Stdout)
-		if err != nil {
-			t.Fatal("read:", err)
+		args = args[1:]
+	}
+	if len(args) == 0 {
+		fmt.Fprintf(os.Stderr, "No command\n")
+		os.Exit(2)
+	}
+
+	cmd, args := args[0], args[1:]
+	switch cmd {
+	case "echo":
+		iargs := []interface{}{}
+		for _, s := range args {
+			iargs = append(iargs, s)
 		}
-		expect := "/dev/null|" + a + "|EOF"
-		if string(buf) != expect {
-			t.Errorf("read: got %q expect %q", buf, expect)
+		fmt.Println(iargs...)
+	case "cat":
+		if len(args) == 0 {
+			io.Copy(os.Stdout, os.Stdin)
+			return
 		}
-		if err = cmd.Close(); err != nil {
-			t.Fatal("close:", err)
+		exit := 0
+		for _, fn := range args {
+			f, err := os.Open(fn)
+			if err != nil {
+				fmt.Fprintf(os.Stderr, "Error: %v\n", err)
+				exit = 2
+			} else {
+				defer f.Close()
+				io.Copy(os.Stdout, f)
+			}
 		}
+		os.Exit(exit)
+	case "exit":
+		n, _ := strconv.Atoi(args[0])
+		os.Exit(n)
+	default:
+		fmt.Fprintf(os.Stderr, "Unknown command %q\n", cmd)
+		os.Exit(2)
 	}
 }

src/pkg/go/types/gcimporter_test.go

@@ -37,24 +37,14 @@ func init() {
 
 
 func compile(t *testing.T, dirname, filename string) {
-	cmd, err := exec.Run(gcPath, []string{gcPath, filename}, nil, dirname, exec.DevNull, exec.Pipe, exec.MergeWithStdout)
+	cmd := exec.Command(gcPath, filename)
+	cmd.Dir = dirname
+	out, err := cmd.CombinedOutput()
 	if err != nil {
 		t.Errorf("%s %s failed: %s", gcName, filename, err)
 		return
 	}
-	defer cmd.Close()
-
-	msg, err := cmd.Wait(0)
-	if err != nil {
-		t.Errorf("%s %s failed: %s", gcName, filename, err)
-		return
-	}
-
-	if !msg.Exited() || msg.ExitStatus() != 0 {
-		t.Errorf("%s %s failed: exit status = %d", gcName, filename, msg.ExitStatus())
-		output, _ := ioutil.ReadAll(cmd.Stdout)
-		t.Log(string(output))
-	}
+	t.Logf("%s", string(out))
 }
 
 

src/pkg/http/cgi/host.go

@@ -156,34 +156,38 @@ func (h *Handler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 		cwd = "."
 	}
 
-	args := []string{h.Path}
-	args = append(args, h.Args...)
-
-	cmd, err := exec.Run(
-		pathBase,
-		args,
-		env,
-		cwd,
-		exec.Pipe,        // stdin
-		exec.Pipe,        // stdout
-		exec.PassThrough, // stderr (for now)
-	)
-	if err != nil {
+	internalError := func(err os.Error) {
 		rw.WriteHeader(http.StatusInternalServerError)
 		h.printf("CGI error: %v", err)
+	}
+
+	stdoutRead, stdoutWrite, err := os.Pipe()
+	if err != nil {
+		internalError(err)
 		return
 	}
-	defer func() {
-		cmd.Stdin.Close()
-		cmd.Stdout.Close()
-		cmd.Wait(0) // no zombies
-	}()
 
+	cmd := &exec.Cmd{
+		Path:   pathBase,
+		Args:   append([]string{h.Path}, h.Args...),
+		Dir:    cwd,
+		Env:    env,
+		Stdout: stdoutWrite,
+		Stderr: os.Stderr, // for now
+	}
 	if req.ContentLength != 0 {
-		go io.Copy(cmd.Stdin, req.Body)
+		cmd.Stdin = req.Body
+	}
+
+	err = cmd.Start()
+	if err != nil {
+		internalError(err)
+		return
 	}
+	stdoutWrite.Close()
+	defer cmd.Wait()
 
-	linebody, _ := bufio.NewReaderSize(cmd.Stdout, 1024)
+	linebody, _ := bufio.NewReaderSize(stdoutRead, 1024)
 	headers := make(http.Header)
 	statusCode := 0
 	for {

src/pkg/http/cgi/host_test.go

@@ -17,20 +17,6 @@ import (
 	"testing"
 )
 
-var cgiScriptWorks = canRun("./testdata/test.cgi")
-
-func canRun(s string) bool {
-	c, err := exec.Run(s, []string{s}, nil, ".", exec.DevNull, exec.DevNull, exec.DevNull)
-	if err != nil {
-		return false
-	}
-	w, err := c.Wait(0)
-	if err != nil {
-		return false
-	}
-	return w.Exited() && w.ExitStatus() == 0
-}
-
 func newRequest(httpreq string) *http.Request {
 	buf := bufio.NewReader(strings.NewReader(httpreq))
 	req, err := http.ReadRequest(buf)
@@ -76,8 +62,15 @@ readlines:
 	return rw
 }
 
+var cgiTested = false
+var cgiWorks bool
+
 func skipTest(t *testing.T) bool {
-	if !cgiScriptWorks {
+	if !cgiTested {
+		cgiTested = true
+		cgiWorks = exec.Command("./testdata/test.cgi").Run() == nil
+	}
+	if !cgiWorks {
 		// No Perl on Windows, needed by test.cgi
 		// TODO: make the child process be Go, not Perl.
 		t.Logf("Skipping test: test.cgi failed.")