Yet more remote fixes (#731)

* possibly . will work better here

* extend test

* more relative paths

* implement TODO by removing exclusion

guess this means we only support 1.12+ for this repo, although it is only a test still

* make go_get a little more robust

* hack TMP_DIR onto the front of the commands for now

* bump version

* Metadata apparently does not always come back on cached actions; we don't need to rely on it

* Little more debugging for apparently stuck actions

ChangeLog

@@ -1,3 +1,9 @@
+Version 14.1.3
+--------------
+
+    * More small fixes to remote execution code.
+
+
 Version 14.1.2
 --------------
 

VERSION

@@ -1 +1 @@
-14.1.2
+14.1.3

bootstrap.sh

@@ -45,11 +45,6 @@ if [ "$GOOS" != "linux" ] ; then
     warn "cc_module tests disabled due to not being on Linux"
     EXCLUDES="${EXCLUDES} --exclude=cc_module"
 fi
-# TODO(peterebden): Once Go 1.13 is out we will need to update (or remove) this.
-if [ ! "`go version | grep 1.12`" ]; then
-    warn "Go version != 1.12 detected, excluding assembly tests"
-    EXCLUDES="${EXCLUDES} --exclude=asm --exclude=//test/go_rules/asm_binary/... --exclude=//test/go_rules/asm/..."
-fi
 if ! hash python2 2>/dev/null ; then
     warn "python2 not found, excluding python2 tests"
     EXCLUDES="${EXCLUDES} --exclude=py2"

src/core/build_env.go

@@ -62,7 +62,7 @@ func BuildEnvironment(state *BuildState, target *BuildTarget, tmpDir string) Bui
 	env := buildEnvironment(state, target)
 	sources := target.AllSourcePaths(state.Graph)
 	outEnv := target.GetTmpOutputAll(target.Outputs())
-	abs := tmpDir != ""
+	abs := path.IsAbs(tmpDir)
 
 	env = append(env,
 		"TMP_DIR="+tmpDir,
@@ -187,11 +187,12 @@ func initStampEnv() {
 }
 
 func toolPath(state *BuildState, tool BuildInput, abs bool) string {
-	label := tool.Label()
-	if label != nil {
+	if label := tool.Label(); label != nil {
 		return state.Graph.TargetOrDie(*label).toolPath(abs)
+	} else if abs {
+		return tool.Paths(state.Graph)[0]
 	}
-	return tool.Paths(state.Graph)[0]
+	return tool.LocalPaths(state.Graph)[0]
 }
 
 func toolPaths(state *BuildState, tools []BuildInput, abs bool) []string {

src/core/build_input.go

@@ -174,7 +174,7 @@ func (label SystemPathLabel) FullPaths(graph *BuildGraph) []string {
 
 // LocalPaths returns paths within the local package
 func (label SystemPathLabel) LocalPaths(graph *BuildGraph) []string {
-	return label.FullPaths(graph)
+	return []string{label.Name}
 }
 
 // Label returns the build rule associated with this input. For a SystemPathLabel it's always nil.

src/remote/action.go

@@ -47,6 +47,9 @@ func (c *Client) buildCommand(target *core.BuildTarget, stamp []byte, isTest boo
 	if isTest {
 		return c.buildTestCommand(target)
 	}
+	// We can't predict what variables like this should be so we sneakily bung something on
+	// the front of the command. It'd be nicer if there were a better way though...
+	const commandPrefix = "export TMP_DIR=\"`pwd`\" && "
 	files, dirs := outputs(target)
 	return &pb.Command{
 		Platform: &pb.Platform{
@@ -65,9 +68,9 @@ func (c *Client) buildCommand(target *core.BuildTarget, stamp []byte, isTest boo
 		// remote one (which is probably OK on the same OS, but not between say Linux and
 		// FreeBSD where bash is not idiomatically in the same place).
 		Arguments: []string{
-			c.bashPath, "--noprofile", "--norc", "-u", "-o", "pipefail", "-c", c.getCommand(target),
+			c.bashPath, "--noprofile", "--norc", "-u", "-o", "pipefail", "-c", commandPrefix + c.getCommand(target),
 		},
-		EnvironmentVariables: buildEnv(core.StampedBuildEnvironment(c.state, target, stamp, "")),
+		EnvironmentVariables: buildEnv(core.StampedBuildEnvironment(c.state, target, stamp, ".")),
 		OutputFiles:          files,
 		OutputDirectories:    dirs,
 	}
@@ -85,6 +88,7 @@ func (c *Client) buildTestCommand(target *core.BuildTarget) *pb.Command {
 	} else {
 		files = append(files, core.TestResultsFile)
 	}
+	const commandPrefix = "export TMP_DIR=\"`pwd`\" TEST_DIR=\"`pwd`\" && "
 	return &pb.Command{
 		Platform: &pb.Platform{
 			Properties: []*pb.Platform_Property{
@@ -95,7 +99,7 @@ func (c *Client) buildTestCommand(target *core.BuildTarget) *pb.Command {
 			},
 		},
 		Arguments: []string{
-			c.bashPath, "--noprofile", "--norc", "-u", "-o", "pipefail", "-c", target.GetTestCommand(c.state),
+			c.bashPath, "--noprofile", "--norc", "-u", "-o", "pipefail", "-c", commandPrefix + target.GetTestCommand(c.state),
 		},
 		EnvironmentVariables: buildEnv(core.TestEnvironment(c.state, target, "")),
 		OutputFiles:          files,
@@ -297,14 +301,13 @@ func (c *Client) buildInputRoot(target *core.BuildTarget, upload, isTest bool) (
 
 // buildMetadata converts an ActionResult into one of our BuildMetadata protos.
 func (c *Client) buildMetadata(ar *pb.ActionResult, needStdout, needStderr bool) (*core.BuildMetadata, error) {
-	if ar.ExecutionMetadata == nil {
-		return nil, fmt.Errorf("Build server returned no execution metadata for target; remote build failed or did not run")
-	}
 	metadata := &core.BuildMetadata{
-		StartTime: toTime(ar.ExecutionMetadata.ExecutionStartTimestamp),
-		EndTime:   toTime(ar.ExecutionMetadata.ExecutionCompletedTimestamp),
-		Stdout:    ar.StdoutRaw,
-		Stderr:    ar.StderrRaw,
+		Stdout: ar.StdoutRaw,
+		Stderr: ar.StderrRaw,
+	}
+	if ar.ExecutionMetadata != nil {
+		metadata.StartTime = toTime(ar.ExecutionMetadata.ExecutionStartTimestamp)
+		metadata.EndTime = toTime(ar.ExecutionMetadata.ExecutionCompletedTimestamp)
 	}
 	if needStdout && len(metadata.Stdout) == 0 {
 		if ar.StdoutDigest == nil {

src/remote/remote.go

@@ -451,12 +451,11 @@ func (c *Client) execute(tid int, target *core.BuildTarget, digest *pb.Digest, t
 					log.Debug("Bad result from build server: %+v", response)
 					return nil, nil, fmt.Errorf("Build server did not return valid result")
 				}
-				// TODO(henryaj): if messages are only emitted on error, we should upgrade this to a warning
 				if response.Message != "" {
+					// Informational messages can be emitted on successful actions.
 					log.Debug("Message from build server:\n     %s", response.Message)
 				}
-				// TODO(henryaj): are there cases where a non-zero exit code isn't a failed build?
-				if response.Result.ExitCode > 0 {
+				if response.Result.ExitCode != 0 {
 					return nil, nil, fmt.Errorf("Remotely executed command exited with %d", response.Result.ExitCode)
 				}
 				metadata, err := c.buildMetadata(response.Result, needStdout || respErr != nil, respErr != nil)
@@ -473,6 +472,9 @@ func (c *Client) execute(tid int, target *core.BuildTarget, digest *pb.Digest, t
 
 // updateProgress updates the progress of a target based on its metadata.
 func (c *Client) updateProgress(tid int, target *core.BuildTarget, metadata *pb.ExecuteOperationMetadata) {
+	if c.state.Config.Remote.DisplayURL != "" {
+		log.Debug("Remote progress for %s: %s (action: %s/action/%s/%s/%d)", target.Label, metadata.Stage, c.state.Config.Remote.DisplayURL, c.state.Config.Remote.Instance, metadata.ActionDigest.Hash, metadata.ActionDigest.SizeBytes)
+	}
 	if target.State() >= core.Built {
 		switch metadata.Stage {
 		case pb.ExecutionStage_CACHE_CHECK:

src/remote/remote_test.go

@@ -217,6 +217,20 @@ func TestNoAbsolutePaths(t *testing.T) {
 	}
 }
 
+func TestNoAbsolutePaths2(t *testing.T) {
+	c := newClientInstance("test")
+	tool := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "tool"})
+	tool.AddOutput("bin")
+	c.state.Graph.AddTarget(tool)
+	target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "target5"})
+	target.AddOutput("remote_test")
+	target.AddTool(core.SystemPathLabel{Path: []string{os.Getenv("TMP_DIR")}, Name: "remote_test"})
+	cmd := c.buildCommand(target, []byte("hello"), false)
+	for _, env := range cmd.EnvironmentVariables {
+		assert.False(t, path.IsAbs(env.Value), "Env var %s has an absolute path: %s", env.Name, env.Value)
+	}
+}
+
 func newClient() *Client {
 	return newClientInstance("")
 }

src/remote/utils.go

@@ -5,6 +5,7 @@ import (
 	"encoding/hex"
 	"fmt"
 	"os"
+	"path"
 	"strings"
 	"time"
 
@@ -147,7 +148,7 @@ func outputs(target *core.BuildTarget) (files, dirs []string) {
 	outs := target.Outputs()
 	files = make([]string, 0, len(outs))
 	for _, out := range outs {
-		if !strings.ContainsRune(out, '.') && !strings.HasSuffix(out, "file") {
+		if !strings.ContainsRune(path.Base(out), '.') && !strings.HasSuffix(out, "file") {
 			dirs = append(dirs, out)
 		} else {
 			files = append(files, out)