all: add env support, freebsd-386, freebsd-amd64-race, re-enable plan9

Update golang/go#9491
Update golang/go#8639

Change-Id: I1f43c751777f10a6d5870ca9bbb8c2a3430189bf
Reviewed-on: https://go-review.googlesource.com/3170
Reviewed-by: Andrew Gerrand <[email protected]>

Author: bradfitz

buildlet/buildletclient.go

@@ -128,6 +128,10 @@ type ExecOpts struct {
 	// Args are the arguments to pass to the cmd given to Client.Exec.
 	Args []string
 
+	// ExtraEnv are KEY=VALUE pairs to append to the buildlet
+	// process's environment.
+	ExtraEnv []string
+
 	// SystemLevel controls whether the command is run outside of
 	// the buildlet's environment.
 	SystemLevel bool
@@ -154,6 +158,7 @@ func (c *Client) Exec(cmd string, opts ExecOpts) (remoteErr, execErr error) {
 		"cmd":    {cmd},
 		"mode":   {mode},
 		"cmdArg": opts.Args,
+		"env":    opts.ExtraEnv,
 	}
 	req, err := http.NewRequest("POST", c.URL()+"/exec", strings.NewReader(form.Encode()))
 	if err != nil {

cmd/buildlet/Makefile

@@ -7,7 +7,12 @@ buildlet.darwin-amd64: buildlet.go
 
 buildlet.freebsd-amd64: buildlet.go
 	GOOS=freebsd GOARCH=amd64 go build -o $@
-	cat $@ | (cd ../upload && go run upload.go --public go-builder-data/$@)
+
+# We use the same binary for freebsd-386 as freebsd-amd64:
+.PHONY: freebsd
+freebsd: buildlet.freebsd-amd64
+	cat buildlet.freebsd-amd64 | (cd ../upload && go run upload.go --public go-builder-data/buildlet.freebsd-amd64)
+	cat buildlet.freebsd-amd64 | (cd ../upload && go run upload.go --public go-builder-data/buildlet.freebsd-386)
 
 buildlet.linux-amd64: buildlet.go
 	GOOS=linux GOARCH=amd64 go build -o $@

cmd/buildlet/buildlet.go

@@ -39,7 +39,7 @@ import (
 
 var (
 	haltEntireOS = flag.Bool("halt", true, "halt OS in /halt handler. If false, the buildlet process just ends.")
-	scratchDir   = flag.String("scratchdir", "", "Temporary directory to use. The contents of this directory may be deleted at any time. If empty, TempDir is used to create one.")
+	workDir      = flag.String("workdir", "", "Temporary directory to use. The contents of this directory may be deleted at any time. If empty, TempDir is used to create one.")
 	listenAddr   = flag.String("listen", defaultListenAddr(), "address to listen on. Warning: this service is inherently insecure and offers no protection of its own. Do not expose this port to the world.")
 )
 
@@ -72,33 +72,21 @@ func main() {
 	if onGCE {
 		fixMTU()
 	}
-	if runtime.GOOS == "plan9" {
-		// Plan 9 is too slow on GCE, so stop running run.rc after the basics.
-		// See https://golang.org/cl/2522 and https://golang.org/issue/9491
-		// TODO(bradfitz): once the buildlet has environment variable support,
-		// the coordinator can send this in, and this variable can be part of
-		// the build configuration struct instead of hard-coded here.
-		// But no need for environment variables quite yet.
-		os.Setenv("GOTESTONLY", "std")
-	}
-	if *scratchDir == "" {
+	if *workDir == "" {
 		dir, err := ioutil.TempDir("", "buildlet-scatch")
 		if err != nil {
-			log.Fatalf("error creating scratchdir with ioutil.TempDir: %v", err)
+			log.Fatalf("error creating workdir with ioutil.TempDir: %v", err)
 		}
-		*scratchDir = dir
+		*workDir = dir
 	}
-	// TODO(bradfitz): if this becomes more of a general tool,
-	// perhaps we want to remove this hard-coded here. Also,
-	// if/once the exec handler ever gets generic environment
-	// variable support, it would make sense to remove this too
-	// and push it to the client.  This hard-codes policy. But
-	// that's okay for now.
-	os.Setenv("GOROOT_BOOTSTRAP", filepath.Join(*scratchDir, "go1.4"))
-	os.Setenv("WORKDIR", *scratchDir) // mostly for demos
+	// This is hard-coded because the client-supplied environment has
+	// no way to expand relative paths from the workDir.
+	// TODO(bradfitz): if we ever need more than this, make some mechanism.
+	os.Setenv("GOROOT_BOOTSTRAP", filepath.Join(*workDir, "go1.4"))
+	os.Setenv("WORKDIR", *workDir) // mostly for demos
 
-	if _, err := os.Lstat(*scratchDir); err != nil {
-		log.Fatalf("invalid --scratchdir %q: %v", *scratchDir, err)
+	if _, err := os.Lstat(*workDir); err != nil {
+		log.Fatalf("invalid --workdir %q: %v", *workDir, err)
 	}
 	http.HandleFunc("/", handleRoot)
 	http.HandleFunc("/debug/goroutines", handleGoroutines)
@@ -208,7 +196,7 @@ func fixMTU_plan9() error {
 	if err != nil {
 		return err
 	}
-	if _, err := io.WriteString(f, "mtu 1400\n"); err != nil { // not 1460
+	if _, err := io.WriteString(f, "mtu 1460\n"); err != nil {
 		f.Close()
 		return err
 	}
@@ -230,46 +218,18 @@ func fixMTU() {
 	}
 }
 
-// mtuWriter is a hack for environments where we can't (or can't yet)
-// fix the machine's MTU.
-// Instead of telling the operating system the MTU, we just cut up our
-// writes into small pieces to make sure we don't get too near the
-// MTU, and we hope the kernel doesn't coalesce different flushed
-// writes back together into the same TCP IP packets.
-// TODO(bradfitz): this has never proven to work. See:
-//    https://github.com/golang/go/issues/9491#issuecomment-70881865
-// But we do depend on its http.Flusher.Flush-per-Write behavior, so we
-// can't delete this entirely.
-type mtuWriter struct {
+// flushWriter is an io.Writer that Flushes after each Write if the
+// underlying Writer implements http.Flusher.
+type flushWriter struct {
 	rw http.ResponseWriter
 }
 
-func (mw mtuWriter) Write(p []byte) (n int, err error) {
-	const mtu = 1000 // way less than 1460; since HTTP response headers might be in there too
-	for len(p) > 0 {
-		chunk := p
-		if len(chunk) > mtu {
-			chunk = p[:mtu]
-		}
-		n0, err := mw.rw.Write(chunk)
-		n += n0
-		if n0 != len(chunk) && err == nil {
-			err = io.ErrShortWrite
-		}
-		if err != nil {
-			return n, err
-		}
-		p = p[n0:]
-		mw.rw.(http.Flusher).Flush()
-		if len(p) > 0 && runtime.GOOS == "plan9" {
-			// Try to prevent the kernel from Nagel-ing the IP packets
-			// together into one that's too large.
-			// This didn't fix anything, though.
-			// See https://github.com/golang/go/issues/9491#issuecomment-70881865
-			//time.Sleep(250 * time.Millisecond)
-		}
+func (fw flushWriter) Write(p []byte) (n int, err error) {
+	n, err = fw.rw.Write(p)
+	if f, ok := fw.rw.(http.Flusher); ok {
+		f.Flush()
 	}
-	return n, nil
+	return
 }
 
 func handleRoot(w http.ResponseWriter, r *http.Request) {
@@ -281,10 +241,9 @@ func handleRoot(w http.ResponseWriter, r *http.Request) {
 }
 
 // unauthenticated /debug/goroutines handler
-func handleGoroutines(rw http.ResponseWriter, r *http.Request) {
-	w := mtuWriter{rw}
+func handleGoroutines(w http.ResponseWriter, r *http.Request) {
 	log.Printf("Dumping goroutines.")
-	rw.Header().Set("Content-Type", "text/plain; charset=utf-8")
+	w.Header().Set("Content-Type", "text/plain; charset=utf-8")
 	buf := make([]byte, 2<<20)
 	buf = buf[:runtime.Stack(buf, true)]
 	w.Write(buf)
@@ -319,19 +278,19 @@ func validRelativeDir(dir string) bool {
 	return true
 }
 
-func handleGetTGZ(rw http.ResponseWriter, r *http.Request) {
+func handleGetTGZ(w http.ResponseWriter, r *http.Request) {
 	if r.Method != "GET" {
-		http.Error(rw, "requires GET method", http.StatusBadRequest)
+		http.Error(w, "requires GET method", http.StatusBadRequest)
 		return
 	}
 	dir := r.FormValue("dir")
 	if !validRelativeDir(dir) {
-		http.Error(rw, "bogus dir", http.StatusBadRequest)
+		http.Error(w, "bogus dir", http.StatusBadRequest)
 		return
 	}
-	zw := gzip.NewWriter(mtuWriter{rw})
+	zw := gzip.NewWriter(w)
 	tw := tar.NewWriter(zw)
-	base := filepath.Join(*scratchDir, filepath.FromSlash(dir))
+	base := filepath.Join(*workDir, filepath.FromSlash(dir))
 	err := filepath.Walk(base, func(path string, fi os.FileInfo, err error) error {
 		if err != nil {
 			return err
@@ -374,7 +333,7 @@ func handleGetTGZ(rw http.ResponseWriter, r *http.Request) {
 		log.Printf("Walk error: %v", err)
 		// Decent way to signal failure to the caller, since it'll break
 		// the chunked response, rather than have a valid EOF.
-		conn, _, _ := rw.(http.Hijacker).Hijack()
+		conn, _, _ := w.(http.Hijacker).Hijack()
 		conn.Close()
 	}
 	tw.Close()
@@ -409,7 +368,7 @@ func handleWriteTGZ(w http.ResponseWriter, r *http.Request) {
 	}
 
 	urlParam, _ := url.ParseQuery(r.URL.RawQuery)
-	baseDir := *scratchDir
+	baseDir := *workDir
 	if dir := urlParam.Get("dir"); dir != "" {
 		if !validRelativeDir(dir) {
 			http.Error(w, "bogus dir", http.StatusBadRequest)
@@ -527,7 +486,7 @@ func handleExec(w http.ResponseWriter, r *http.Request) {
 			http.Error(w, "requires 'cmd' parameter", http.StatusBadRequest)
 			return
 		}
-		absCmd = filepath.Join(*scratchDir, filepath.FromSlash(cmdPath))
+		absCmd = filepath.Join(*workDir, filepath.FromSlash(cmdPath))
 	}
 
 	if f, ok := w.(http.Flusher); ok {
@@ -536,13 +495,14 @@ func handleExec(w http.ResponseWriter, r *http.Request) {
 
 	cmd := exec.Command(absCmd, r.PostForm["cmdArg"]...)
 	if sysMode {
-		cmd.Dir = *scratchDir
+		cmd.Dir = *workDir
 	} else {
 		cmd.Dir = filepath.Dir(absCmd)
 	}
-	cmdOutput := mtuWriter{w}
+	cmdOutput := flushWriter{w}
 	cmd.Stdout = cmdOutput
 	cmd.Stderr = cmdOutput
+	cmd.Env = append(os.Environ(), r.PostForm["env"]...)
 	err := cmd.Start()
 	if err == nil {
 		go func() {

cmd/coordinator/coordinator.go

@@ -42,8 +42,6 @@ import (
 )
 
 func init() {
-	// Disabled until its networking/MTU problems are fixed:
-	// https://github.com/golang/go/issues/9491#issuecomment-70881865
 	delete(dashboard.Builders, "plan9-386-gcepartial")
 }
 
@@ -797,6 +795,7 @@ func buildInVM(conf dashboard.BuildConfig, st *buildStatus) (retErr error) {
 	remoteErr, err := bc.Exec(path.Join("go", conf.AllScript()), buildlet.ExecOpts{
 		Output:      st,
 		OnStartExec: func() { st.logEventTime("running_exec") },
+		ExtraEnv:    conf.Env(),
 	})
 	if err != nil {
 		return err

cmd/gomote/run.go

@@ -22,6 +22,8 @@ func run(args []string) error {
 	}
 	var sys bool
 	fs.BoolVar(&sys, "system", false, "run inside the system, and not inside the workdir; this is implicit if cmd starts with '/'")
+	var env stringSlice
+	fs.Var(&env, "e", "Environment variable KEY=value. The -e flag may be repeated multiple times to add multiple things to the environment.")
 
 	fs.Parse(args)
 	if fs.NArg() < 2 {
@@ -37,9 +39,26 @@ func run(args []string) error {
 		SystemLevel: sys || strings.HasPrefix(cmd, "/"),
 		Output:      os.Stdout,
 		Args:        fs.Args()[2:],
+		ExtraEnv:    []string(env),
 	})
 	if execErr != nil {
 		return fmt.Errorf("Error trying to execute %s: %v", cmd, execErr)
 	}
 	return remoteErr
 }
+
+// stringSlice implements flag.Value, specifically for storing environment
+// variable key=value pairs.
+type stringSlice []string
+
+func (*stringSlice) String() string { return "" } // default value
+
+func (ss *stringSlice) Set(v string) error {
+	if v != "" {
+		if !strings.Contains(v, "=") {
+			return fmt.Errorf("-e argument %q doesn't contains an '=' sign.", v)
+		}
+		*ss = append(*ss, v)
+	}
+	return nil
+}

dashboard/builders.go

@@ -30,13 +30,17 @@ type BuildConfig struct {
 	Go14URL     string // URL to built Go 1.4 tar.gz
 
 	// Docker-specific settings: (used if VMImage == "")
-	Image   string   // Docker image to use to build
-	cmd     string   // optional -cmd flag (relative to go/src/)
-	env     []string // extra environment ("key=value") pairs
-	dashURL string   // url of the build dashboard
-	tool    string   // the tool this configuration is for
+	Image   string // Docker image to use to build
+	cmd     string // optional -cmd flag (relative to go/src/)
+	dashURL string // url of the build dashboard
+	tool    string // the tool this configuration is for
+
+	// Use by both VMs and Docker:
+	env []string // extra environment ("key=value") pairs
 }
 
+func (c *BuildConfig) Env() []string { return append([]string(nil), c.env...) }
+
 func (c *BuildConfig) GOOS() string { return c.Name[:strings.Index(c.Name, "-")] }
 
 func (c *BuildConfig) GOARCH() string {
@@ -52,14 +56,18 @@ func (c *BuildConfig) GOARCH() string {
 // do the build and run its standard set of tests.
 // Example values are "src/all.bash", "src/all.bat", "src/all.rc".
 func (c *BuildConfig) AllScript() string {
+	if strings.HasSuffix(c.Name, "-race") {
+		if strings.HasPrefix(c.Name, "windows-") {
+			return "src/race.bat"
+		}
+		return "src/race.bash"
+	}
 	if strings.HasPrefix(c.Name, "windows-") {
 		return "src/all.bat"
 	}
 	if strings.HasPrefix(c.Name, "plan9-") {
 		return "src/all.rc"
 	}
-	// TODO(bradfitz): race.bash, etc, once the race builder runs
-	// via the buildlet.
 	return "src/all.bash"
 }
 
@@ -148,20 +156,42 @@ func init() {
 	addBuilder(BuildConfig{Name: "linux-amd64-clang", Image: "gobuilders/linux-x86-clang"})
 
 	// VMs:
+	addBuilder(BuildConfig{
+		Name:        "freebsd-amd64-gce101",
+		VMImage:     "freebsd-amd64-gce101",
+		machineType: "n1-highcpu-2",
+		Go14URL:     "https://storage.googleapis.com/go-builder-data/go1.4-freebsd-amd64.tar.gz",
+		env:         []string{"CC=clang"},
+	})
+	addBuilder(BuildConfig{
+		Name:        "freebsd-amd64-race",
+		VMImage:     "freebsd-amd64-gce101",
+		machineType: "n1-highcpu-4",
+		Go14URL:     "https://storage.googleapis.com/go-builder-data/go1.4-freebsd-amd64.tar.gz",
+		env:         []string{"CC=clang"},
+	})
+	addBuilder(BuildConfig{
+		Name:        "freebsd-386-gce101",
+		VMImage:     "freebsd-amd64-gce101",
+		machineType: "n1-highcpu-2",
+		Go14URL:     "https://storage.googleapis.com/go-builder-data/go1.4-freebsd-amd64.tar.gz",
+		env:         []string{"GOARCH=386", "CC=clang"},
+	})
 	addBuilder(BuildConfig{
 		Name:        "openbsd-amd64-gce56",
 		VMImage:     "openbsd-amd64-56",
 		machineType: "n1-highcpu-2",
 		Go14URL:     "https://storage.googleapis.com/go-builder-data/go1.4-openbsd-amd64.tar.gz",
 	})
 	addBuilder(BuildConfig{
+		Name:    "plan9-386-gcepartial",
+		VMImage: "plan9-386",
+		Go14URL: "https://storage.googleapis.com/go-builder-data/go1.4-plan9-386.tar.gz",
 		// It's named "partial" because the buildlet sets
 		// GOTESTONLY=std to stop after the "go test std"
 		// tests because it's so slow otherwise.
-		// TODO(braditz): move that env variable to the
-		// coordinator and into this config.
-		Name:    "plan9-386-gcepartial",
-		VMImage: "plan9-386",
+		env: []string{"GOTESTONLY=std"},
+
 		// We *were* using n1-standard-1 because Plan 9 can only
 		// reliably use a single CPU. Using 2 or 4 and we see
 		// test failures. See: