From cd91e8d0964547a2bc7db49f9492be21922353f4 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Thu, 28 Aug 2014 14:58:15 -0700 Subject: [PATCH] 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 | 25 ++++++------ dashboard/builder/env.go | 31 +++++--------- dashboard/builder/exec.go | 83 ++++++++++++++++++++------------------ dashboard/builder/main.go | 60 ++++++++++++++------------- dashboard/builder/vcs.go | 8 ++-- 5 files changed, 101 insertions(+), 106 deletions(-) diff --git a/dashboard/builder/bench.go b/dashboard/builder/bench.go index cb2b0278..78e14f3c 100644 --- a/dashboard/builder/bench.go +++ b/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...) - if !ok || err != nil { + runOpts := []runOpt{runTimeout(*buildTimeout), runEnv(env), allOutput(&buildlog), runDir(workpath)} + 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...) - if !ok || err != nil { + err = run(exec.Command(gobin, args...), runOpts...) + 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 diff --git a/dashboard/builder/env.go b/dashboard/builder/env.go index cb38741c..554d25f5 100644 --- a/dashboard/builder/env.go +++ b/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{ - "git", - "reset", - "--hard", - strings.TrimSpace(gccRev), - } - if _, err := runOutput(*cmdTimeout, envv, ioutil.Discard, gccpath, checkoutCmd...); err != nil { + if err := run(exec.Command("git", "reset", "--hard", strings.TrimSpace(gccRev)), runEnv(envv), runDir(gccpath)); 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{ - filepath.Join(gccpath, "configure"), + if err := run(exec.Command(filepath.Join(gccpath, "configure"), "--enable-languages=c,c++,go", "--disable-bootstrap", "--disable-multilib", - } - if _, err := runOutput(*cmdTimeout, envv, ioutil.Discard, gccobjdir, gccConfigCmd...); err != nil { - return "", fmt.Errorf("Failed to configure GCC: %s", err) + ), runEnv(envv), runDir(gccobjdir)); err != nil { + return "", fmt.Errorf("Failed to configure GCC: %v", 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) } diff --git a/dashboard/builder/exec.go b/dashboard/builder/exec.go index 590eb9b7..c40301f7 100644 --- a/dashboard/builder/exec.go +++ b/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 -func run(d time.Duration, envv []string, dir string, argv ...string) error { - if *verbose { - log.Println("run", argv) +// run runs a command with optional arguments. +func run(cmd *exec.Cmd, opts ...runOpt) error { + a := runArgs{cmd, *cmdTimeout} + 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 -// process combined stdout and stderr output, exit status and error. The -// error returned is nil, if process is started successfully, even if exit -// status is not successful. -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 +// Zero or more runOpts can be passed to run to modify the command +// before it's run. +type runOpt interface { + modArgs(*runArgs) } -// runOutput runs a process and directs any output to the supplied writer. -// It returns exit status and error. The error returned is nil, if process -// is started successfully, even if exit status is not successful. -func runOutput(d time.Duration, envv []string, out io.Writer, dir string, argv ...string) (bool, error) { - if *verbose { - log.Println("runOutput", argv) +// allOutput sends both stdout and stderr to w. +func allOutput(w io.Writer) optFunc { + return func(a *runArgs) { + a.cmd.Stdout = w + a.cmd.Stderr = w } +} - cmd := exec.Command(argv[0], argv[1:]...) - cmd.Dir = dir - cmd.Env = envv - cmd.Stdout = out - cmd.Stderr = out - - startErr := cmd.Start() - if startErr != nil { - return false, startErr +func runTimeout(timeout time.Duration) optFunc { + return func(a *runArgs) { + a.timeout = timeout } +} - if err := timeout(d, cmd.Wait); err != nil { - if _, ok := err.(timeoutError); ok { - cmd.Process.Kill() - } - return false, err +func runDir(dir string) optFunc { + return func(a *runArgs) { + a.cmd.Dir = dir + } +} + +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 +} diff --git a/dashboard/builder/main.go b/dashboard/builder/main.go index 0cbcca79..b9498b5b 100644 --- a/dashboard/builder/main.go +++ b/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 + return err.Error(), 0, err } 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 - splitCmd := strings.Split(cmd, " ") + // naive splitting of command from its arguments: + 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...) - runTime = time.Now().Sub(startTime) - if !ok && err == nil { - err = fmt.Errorf("build failed") + err = run(c, runTimeout(*buildTimeout)) + runTime = time.Since(startTime) + if err != nil { + 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 { - 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 + return buildbuf.String(), runTime, err } // 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+"/...") - if err == nil && !ok { - err = fmt.Errorf("go exited with status 1") - } + var outbuf bytes.Buffer + err := run(exec.Command(goTool, "get", "-d", pkg+"/..."), runEnv(env), allOutput(&outbuf), runDir(goPath)) 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+"/...") - if err == nil && !ok { - err = fmt.Errorf("go exited with status 1") - } - return log, err + err = run(exec.Command(goTool, "test", "-short", pkg+"/..."), + runTimeout(*buildTimeout), runEnv(env), allOutput(&outbuf), runDir(goPath)) + return outbuf.String(), err } // repoForTool returns the correct RepoRoot for the buildTool, or an error if diff --git a/dashboard/builder/vcs.go b/dashboard/builder/vcs.go index 49859af3..78e8008e 100644 --- a/dashboard/builder/vcs.go +++ b/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} - if err := run(*cmdTimeout, nil, downloadPath, args...); err != nil { - return fmt.Errorf("executing %s: %v", strings.Join(args, " "), err) + cmd := exec.Command(r.Master.VCS.Cmd, "archive", "-t", "files", "-r", rev, path) + cmd.Dir = downloadPath + if err := run(cmd); err != nil { + return fmt.Errorf("executing %v: %v", cmd.Args, err) } return nil }