dashboard/builder: modernize ancient exec wrappers

They were from a time before we had the os/exec package, if
memory serves.

Also, make verbose also mean that the main build's stdout
and stderr go to the real stdout and stderr as well.
I'll want that for the Docker-based builder.

LGTM=adg
R=adg
CC=golang-codereviews
https://golang.org/cl/135000043

dashboard/builder/bench.go

@@ -87,23 +87,24 @@ func (b *Builder) buildBenchmark(workpath string, update bool) (benchBin, log st
 		"GOPATH=" + gopath},
 		b.envv()...)
 	// First, download without installing.
-	cmd := []string{gobin, "get", "-d"}
+	args := []string{"get", "-d"}
 	if update {
-		cmd = append(cmd, "-u")
+		args = append(args, "-u")
 	}
-	cmd = append(cmd, *benchPath)
+	args = append(args, *benchPath)
 	var buildlog bytes.Buffer
-	ok, err := runOutput(*buildTimeout, env, &buildlog, workpath, cmd...)
+	runOpts := []runOpt{runTimeout(*buildTimeout), runEnv(env), allOutput(&buildlog), runDir(workpath)}
-	if !ok || err != nil {
+	err = run(exec.Command(gobin, args...), runOpts...)
+	if err != nil {
 		fmt.Fprintf(&buildlog, "go get -d %s failed: %s", *benchPath, err)
 		return "", buildlog.String(), err
 	}
 	// Then, build into workpath.
 	benchBin = filepath.Join(workpath, "benchbin") + exeExt
-	cmd = []string{gobin, "build", "-o", benchBin, *benchPath}
+	args = []string{"build", "-o", benchBin, *benchPath}
 	buildlog.Reset()
-	ok, err = runOutput(*buildTimeout, env, &buildlog, workpath, cmd...)
+	err = run(exec.Command(gobin, args...), runOpts...)
-	if !ok || err != nil {
+	if err != nil {
 		fmt.Fprintf(&buildlog, "go build %s failed: %s", *benchPath, err)
 		return "", buildlog.String(), err
 	}
@@ -172,23 +173,23 @@ func (b *Builder) executeBenchmark(workpath, hash, benchBin, bench string, procs
 		"GOGCTRACE=1",       // before Go1.2
 		fmt.Sprintf("GOMAXPROCS=%v", procs)},
 		b.envv()...)
-	cmd := []string{benchBin,
+	args := []string{
 		"-bench", bench,
 		"-benchmem", strconv.Itoa(*benchMem),
 		"-benchtime", benchTime.String(),
 		"-benchnum", strconv.Itoa(*benchNum),
 		"-tmpdir", workpath}
 	if affinity != 0 {
-		cmd = append(cmd, "-affinity", strconv.Itoa(affinity))
+		args = append(args, "-affinity", strconv.Itoa(affinity))
 	}
 	benchlog := new(bytes.Buffer)
-	ok, err := runOutput(*buildTimeout, env, benchlog, workpath, cmd...)
+	err := run(exec.Command(benchBin, args...), runEnv(env), allOutput(benchlog), runDir(workpath))
 	if strip := benchlog.Len() - 512<<10; strip > 0 {
 		// Leave the last 512K, that part contains metrics.
 		benchlog = bytes.NewBuffer(benchlog.Bytes()[strip:])
 	}
 	artifacts = []PerfArtifact{{Type: "log", Body: benchlog.String()}}
-	if !ok || err != nil {
+	if err != nil {
 		if err != nil {
 			log.Printf("Failed to execute benchmark '%v': %v", bench, err)
 			ok = false

dashboard/builder/env.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"regexp"
 	"runtime"
@@ -151,16 +152,10 @@ func (env *gccgoEnv) setup(repo *Repo, workpath, hash string, envv []string) (st
 	}
 
 	// get the modified files for this commit.
-	statusCmd := []string{
-		"hg",
-		"status",
-		"--no-status",
-		"--change",
-		hash,
-	}
 
 	var buf bytes.Buffer
-	if _, err := runOutput(*cmdTimeout, envv, &buf, repo.Path, statusCmd...); err != nil {
+	if err := run(exec.Command("hg", "status", "--no-status", "--change", hash),
+		allOutput(&buf), runDir(repo.Path), runEnv(envv)); err != nil {
 		return "", fmt.Errorf("Failed to find the modified files for %s: %s", hash, err)
 	}
 	modifiedFiles := strings.Split(buf.String(), "\n")
@@ -204,7 +199,7 @@ func (env *gccgoEnv) setup(repo *Repo, workpath, hash string, envv []string) (st
 	}
 
 	buf.Reset()
-	if _, err := runOutput(*cmdTimeout, envv, &buf, gccpath, logCmd...); err != nil {
+	if err := run(exec.Command(gccpath, logCmd...), runEnv(envv), allOutput(&buf), runDir(gccpath)); err != nil {
 		return "", fmt.Errorf("%s: %s", errMsg, err)
 	}
 	gccRev := buf.String()
@@ -214,13 +209,7 @@ func (env *gccgoEnv) setup(repo *Repo, workpath, hash string, envv []string) (st
 
 	// checkout gccRev
 	// TODO(cmang): Fix this to work in parallel mode.
-	checkoutCmd := []string{
+	if err := run(exec.Command("git", "reset", "--hard", strings.TrimSpace(gccRev)), runEnv(envv), runDir(gccpath)); err != nil {
-		"git",
-		"reset",
-		"--hard",
-		strings.TrimSpace(gccRev),
-	}
-	if _, err := runOutput(*cmdTimeout, envv, ioutil.Discard, gccpath, checkoutCmd...); err != nil {
 		return "", fmt.Errorf("Failed to checkout commit at revision %s: %s", gccRev, err)
 	}
 
@@ -231,18 +220,16 @@ func (env *gccgoEnv) setup(repo *Repo, workpath, hash string, envv []string) (st
 	}
 
 	// configure GCC with substituted gofrontend and libgo
-	gccConfigCmd := []string{
+	if err := run(exec.Command(filepath.Join(gccpath, "configure"),
-		filepath.Join(gccpath, "configure"),
 		"--enable-languages=c,c++,go",
 		"--disable-bootstrap",
 		"--disable-multilib",
-	}
+	), runEnv(envv), runDir(gccobjdir)); err != nil {
-	if _, err := runOutput(*cmdTimeout, envv, ioutil.Discard, gccobjdir, gccConfigCmd...); err != nil {
+		return "", fmt.Errorf("Failed to configure GCC: %v", err)
-		return "", fmt.Errorf("Failed to configure GCC: %s", err)
 	}
 
 	// build gcc
-	if _, err := runOutput(*buildTimeout, envv, ioutil.Discard, gccobjdir, "make"); err != nil {
+	if err := run(exec.Command("make"), runTimeout(*buildTimeout), runEnv(envv), runDir(gccobjdir)); err != nil {
 		return "", fmt.Errorf("Failed to build GCC: %s", err)
 	}
 

dashboard/builder/exec.go

@@ -5,70 +5,62 @@
 package main
 
 import (
-	"bytes"
 	"fmt"
 	"io"
 	"log"
-	"os"
 	"os/exec"
 	"time"
 )
 
-// run is a simple wrapper for exec.Run/Close
+// run runs a command with optional arguments.
-func run(d time.Duration, envv []string, dir string, argv ...string) error {
+func run(cmd *exec.Cmd, opts ...runOpt) error {
-	if *verbose {
+	a := runArgs{cmd, *cmdTimeout}
-		log.Println("run", argv)
+	for _, opt := range opts {
+		opt.modArgs(&a)
+	}
+	if *verbose {
+		log.Printf("running %v", a.cmd.Args)
 	}
-	cmd := exec.Command(argv[0], argv[1:]...)
-	cmd.Dir = dir
-	cmd.Env = envv
-	cmd.Stderr = os.Stderr
 	if err := cmd.Start(); err != nil {
 		return err
 	}
-	err := timeout(d, cmd.Wait)
+	err := timeout(a.timeout, cmd.Wait)
 	if _, ok := err.(timeoutError); ok {
 		cmd.Process.Kill()
 	}
 	return err
 }
 
-// runLog runs a process and returns the combined stdout/stderr. It returns
+// Zero or more runOpts can be passed to run to modify the command
-// process combined stdout and stderr output, exit status and error. The
+// before it's run.
-// error returned is nil, if process is started successfully, even if exit
+type runOpt interface {
-// status is not successful.
+	modArgs(*runArgs)
-func runLog(timeout time.Duration, envv []string, dir string, argv ...string) (string, bool, error) {
-	var b bytes.Buffer
-	ok, err := runOutput(timeout, envv, &b, dir, argv...)
-	return b.String(), ok, err
 }
 
-// runOutput runs a process and directs any output to the supplied writer.
+// allOutput sends both stdout and stderr to w.
-// It returns exit status and error. The error returned is nil, if process
+func allOutput(w io.Writer) optFunc {
-// is started successfully, even if exit status is not successful.
+	return func(a *runArgs) {
-func runOutput(d time.Duration, envv []string, out io.Writer, dir string, argv ...string) (bool, error) {
+		a.cmd.Stdout = w
-	if *verbose {
+		a.cmd.Stderr = w
-		log.Println("runOutput", argv)
 	}
+}
 
-	cmd := exec.Command(argv[0], argv[1:]...)
+func runTimeout(timeout time.Duration) optFunc {
-	cmd.Dir = dir
+	return func(a *runArgs) {
-	cmd.Env = envv
+		a.timeout = timeout
-	cmd.Stdout = out
-	cmd.Stderr = out
-
-	startErr := cmd.Start()
-	if startErr != nil {
-		return false, startErr
 	}
+}
 
-	if err := timeout(d, cmd.Wait); err != nil {
+func runDir(dir string) optFunc {
-		if _, ok := err.(timeoutError); ok {
+	return func(a *runArgs) {
-			cmd.Process.Kill()
+		a.cmd.Dir = dir
-		}
+	}
-		return false, err
+}
+
+func runEnv(env []string) optFunc {
+	return func(a *runArgs) {
+		a.cmd.Env = env
 	}
-	return true, nil
 }
 
 // timeout runs f and returns its error value, or if the function does not
@@ -93,3 +85,14 @@ type timeoutError time.Duration
 func (e timeoutError) Error() string {
 	return fmt.Sprintf("timed out after %v", time.Duration(e))
 }
+
+// optFunc implements runOpt with a function, like http.HandlerFunc.
+type optFunc func(*runArgs)
+
+func (f optFunc) modArgs(a *runArgs) { f(a) }
+
+// internal detail to exec.go:
+type runArgs struct {
+	cmd     *exec.Cmd
+	timeout time.Duration
+}

dashboard/builder/main.go

@@ -12,6 +12,7 @@ import (
 	"io/ioutil"
 	"log"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"regexp"
 	"runtime"
@@ -409,15 +410,14 @@ func (b *Builder) buildRepoOnHash(workpath, hash, cmd string) (buildLog string,
 	}
 
 	// build
-	var buildlog bytes.Buffer
+	var buildbuf bytes.Buffer
 	logfile := filepath.Join(workpath, "build.log")
 	f, err := os.Create(logfile)
 	if err != nil {
-		buildLog = err.Error()
+		return err.Error(), 0, err
-		return
 	}
 	defer f.Close()
-	w := io.MultiWriter(f, &buildlog)
+	w := io.MultiWriter(f, &buildbuf)
 
 	// go's build command is a script relative to the srcDir, whereas
 	// gccgo's build command is usually "make check-go" in the srcDir.
@@ -427,23 +427,28 @@ func (b *Builder) buildRepoOnHash(workpath, hash, cmd string) (buildLog string,
 		}
 	}
 
-	// make sure commands with extra arguments are handled properly
+	// naive splitting of command from its arguments:
-	splitCmd := strings.Split(cmd, " ")
+	args := strings.Split(cmd, " ")
+	c := exec.Command(args[0], args[1:]...)
+	c.Dir = srcDir
+	c.Env = b.envv()
+	if *verbose {
+		c.Stdout = io.MultiWriter(os.Stdout, w)
+		c.Stderr = io.MultiWriter(os.Stderr, w)
+	} else {
+		c.Stdout = w
+		c.Stderr = w
+	}
+
 	startTime := time.Now()
-	ok, err := runOutput(*buildTimeout, b.envv(), w, srcDir, splitCmd...)
+	err = run(c, runTimeout(*buildTimeout))
-	runTime = time.Now().Sub(startTime)
+	runTime = time.Since(startTime)
-	if !ok && err == nil {
+	if err != nil {
-		err = fmt.Errorf("build failed")
+		fmt.Fprintf(w, "Build complete, duration %v. Result: error: %v\n", runTime, err)
+	} else {
+		fmt.Fprintf(w, "Build complete, duration %v. Result: success\n", runTime)
 	}
-	errf := func() string {
+	return buildbuf.String(), runTime, err
-		if err != nil {
-			return fmt.Sprintf("error: %v", err)
-		}
-		return "success"
-	}
-	fmt.Fprintf(w, "Build complete, duration %v. Result: %v\n", runTime, errf())
-	buildLog = buildlog.String()
-	return
 }
 
 // failBuild checks for a new commit for this builder
@@ -516,13 +521,12 @@ func (b *Builder) buildSubrepo(goRoot, goPath, pkg, hash string) (string, error)
 	}
 
 	// fetch package and dependencies
-	log, ok, err := runLog(*cmdTimeout, env, goPath, goTool, "get", "-d", pkg+"/...")
+	var outbuf bytes.Buffer
-	if err == nil && !ok {
+	err := run(exec.Command(goTool, "get", "-d", pkg+"/..."), runEnv(env), allOutput(&outbuf), runDir(goPath))
-		err = fmt.Errorf("go exited with status 1")
-	}
 	if err != nil {
-		return log, err
+		return outbuf.String(), err
 	}
+	outbuf.Reset()
 
 	// hg update to the specified hash
 	pkgmaster, err := vcs.RepoRootForImportPath(pkg, *verbose)
@@ -538,11 +542,9 @@ func (b *Builder) buildSubrepo(goRoot, goPath, pkg, hash string) (string, error)
 	}
 
 	// test the package
-	log, ok, err = runLog(*buildTimeout, env, goPath, goTool, "test", "-short", pkg+"/...")
+	err = run(exec.Command(goTool, "test", "-short", pkg+"/..."),
-	if err == nil && !ok {
+		runTimeout(*buildTimeout), runEnv(env), allOutput(&outbuf), runDir(goPath))
-		err = fmt.Errorf("go exited with status 1")
+	return outbuf.String(), err
-	}
-	return log, err
 }
 
 // repoForTool returns the correct RepoRoot for the buildTool, or an error if

dashboard/builder/vcs.go

@@ -9,6 +9,7 @@ import (
 	"encoding/xml"
 	"fmt"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"strings"
 	"sync"
@@ -73,9 +74,10 @@ func (r *Repo) Export(path, rev string) error {
 		return err
 	}
 
-	args := []string{r.Master.VCS.Cmd, "archive", "-t", "files", "-r", rev, path}
+	cmd := exec.Command(r.Master.VCS.Cmd, "archive", "-t", "files", "-r", rev, path)
-	if err := run(*cmdTimeout, nil, downloadPath, args...); err != nil {
+	cmd.Dir = downloadPath
-		return fmt.Errorf("executing %s: %v", strings.Join(args, " "), err)
+	if err := run(cmd); err != nil {
+		return fmt.Errorf("executing %v: %v", cmd.Args, err)
 	}
 	return nil
 }