More rex fiddling (#814)

* Minor fixes

* Sort out some tool paths

* RuntimeHash seems to be hashing sources incorrectly. It shouldn't take sources into account.

* Guard test result retrieval in case tests don't write them

* twiddle

BUILD

@@ -24,3 +24,11 @@ filegroup(
     binary = True,
     deps = ["//package:installed_files"],
 )
+
+# This is used as part of bootstrap, and is used from here to avoid subtle issues with remote execution.
+filegroup(
+    name = "jarcat_unzip",
+    srcs = ["//tools/jarcat:jarcat_unzip"],
+    binary = True,
+    visibility = ["//third_party/go:all"],
+)

build_defs/plz_e2e_test.build_defs

@@ -51,4 +51,5 @@ def plz_e2e_test(name:str, cmd:str, pre_cmd:str=None, expected_output:str=None,
         labels = ['e2e'] + (labels or []),
         no_test_output = True,
         sandbox = False,
+        local = True,
     )

src/build/incrementality.go

@@ -410,18 +410,13 @@ func mustShortTargetHash(state *core.BuildState, target *core.BuildTarget) []byt
 	return core.CollapseHash(mustTargetHash(state, target))
 }
 
-// RuntimeHash returns the target hash, source hash, config hash & runtime file hash,
+// RuntimeHash returns the target hash, config hash & runtime file hash,
 // all rolled into one. Essentially this is one hash needed to determine if the runtime
 // state is consistent.
 func RuntimeHash(state *core.BuildState, target *core.BuildTarget) ([]byte, error) {
 	hash := append(RuleHash(state, target, true, false), RuleHash(state, target, true, true)...)
 	hash = append(hash, state.Hashes.Config...)
-	sh, err := sourceHash(state, target)
-	if err != nil {
-		return nil, err
-	}
 	h := sha1.New()
-	h.Write(sh)
 	for source := range core.IterRuntimeFiles(state.Graph, target, true) {
 		result, err := state.PathHasher.Hash(source.Src, false, true)
 		if err != nil {

src/core/build_env.go

@@ -194,7 +194,11 @@ func initStampEnv() {
 
 func toolPath(state *BuildState, tool BuildInput, abs bool) string {
 	if label := tool.Label(); label != nil {
-		return state.Graph.TargetOrDie(*label).toolPath(abs)
+		path := state.Graph.TargetOrDie(*label).toolPath(abs)
+		if !strings.Contains(path, "/") {
+			path = "./" + path
+		}
+		return path
 	} else if abs {
 		return tool.Paths(state.Graph)[0]
 	}

src/core/config.go

@@ -47,13 +47,15 @@ const MachineConfigFileName = "/etc/please/plzconfig"
 const UserConfigFileName = "~/.config/please/plzconfig"
 
 func readConfigFile(config *Configuration, filename string) error {
-	log.Debug("Reading config from %s...", filename)
+	log.Debug("Attempting to read config from %s...", filename)
 	if err := gcfg.ReadFileInto(config, filename); err != nil && os.IsNotExist(err) {
 		return nil // It's not an error to not have the file at all.
 	} else if gcfg.FatalOnly(err) != nil {
 		return err
 	} else if err != nil {
 		log.Warning("Error in config file: %s", err)
+	} else {
+		log.Debug("Read config from %s", filename)
 	}
 	return nil
 }

src/remote/action.go

@@ -120,7 +120,7 @@ func (c *Client) buildTestCommand(target *core.BuildTarget) (*pb.Command, error)
 		Arguments: []string{
 			c.bashPath, "--noprofile", "--norc", "-u", "-o", "pipefail", "-c", commandPrefix + cmd,
 		},
-		EnvironmentVariables: buildEnv(core.TestEnvironment(c.state, target, "")),
+		EnvironmentVariables: buildEnv(core.TestEnvironment(c.state, target, ".")),
 		OutputFiles:          files,
 		OutputDirectories:    dirs,
 		OutputPaths:          append(files, dirs...),
@@ -373,7 +373,7 @@ func (c *Client) buildMetadata(ar *pb.ActionResult, needStdout, needStderr bool)
 	return metadata, nil
 }
 
-// digestForFilename returns the digest for an output of the given name.
+// digestForFilename returns the digest for an output of the given name, or nil if it doesn't exist.
 func (c *Client) digestForFilename(ar *pb.ActionResult, name string) *pb.Digest {
 	for _, file := range ar.OutputFiles {
 		if file.Path == name {

src/remote/remote.go

@@ -427,15 +427,19 @@ func (c *Client) Test(tid int, target *core.BuildTarget) (metadata *core.BuildMe
 	// is more relevant, but we want to still try to get results if we can, and it's an
 	// error if we can't get those results on success.
 	if !target.NoTestOutput && ar != nil {
-		results, err = c.readAllByteStream(context.Background(), c.digestForFilename(ar, core.TestResultsFile))
-		if execErr == nil && err != nil {
-			return metadata, nil, nil, err
+		if digest := c.digestForFilename(ar, core.TestResultsFile); digest != nil {
+			results, err = c.readAllByteStream(context.Background(), digest)
+			if execErr == nil && err != nil {
+				return metadata, nil, nil, err
+			}
 		}
 	}
 	if target.NeedCoverage(c.state) && ar != nil {
-		coverage, err = c.readAllByteStream(context.Background(), c.digestForFilename(ar, core.CoverageFile))
-		if execErr == nil && err != nil {
-			return metadata, results, nil, err
+		if digest := c.digestForFilename(ar, core.CoverageFile); digest != nil {
+			coverage, err = c.readAllByteStream(context.Background(), digest)
+			if execErr == nil && err != nil {
+				return metadata, results, nil, err
+			}
 		}
 	}
 	return metadata, results, coverage, execErr
@@ -511,6 +515,11 @@ func (c *Client) execute(tid int, target *core.BuildTarget, command *pb.Command,
 				var respErr error
 				if response.Status != nil {
 					respErr = convertError(response.Status)
+					if respErr != nil {
+						if url := c.actionURL(digest, false); url != "" {
+							respErr = fmt.Errorf("%s\nAction URL: %s", respErr, url)
+						}
+					}
 				}
 				if resp.Result == nil { // This is optional on failure.
 					return nil, nil, respErr
@@ -543,6 +552,9 @@ func (c *Client) execute(tid int, target *core.BuildTarget, command *pb.Command,
 					if len(metadata.Stderr) != 0 {
 						err = fmt.Errorf("%s\nStderr:\n%s", err, metadata.Stderr)
 					}
+					if url := c.actionURL(digest, true); url != "" {
+						err = fmt.Errorf("%s\n%s", err, url)
+					}
 					return nil, nil, err
 				} else if err != nil {
 					return nil, nil, err

src/remote/utils.go

@@ -267,7 +267,7 @@ func outputs(target *core.BuildTarget) (files, dirs []string) {
 	files = make([]string, 0, len(outs))
 	for _, out := range outs {
 		out = target.GetTmpOutput(out)
-		if !strings.ContainsRune(path.Base(out), '.') && !strings.HasSuffix(out, "file") {
+		if !strings.ContainsRune(path.Base(out), '.') && !strings.HasSuffix(out, "file") && !target.IsBinary {
 			dirs = append(dirs, out)
 		} else {
 			files = append(files, out)

third_party/go/BUILD

@@ -1,7 +1,7 @@
 package(default_visibility = ["PUBLIC"])
 
 # This is needed to break a circular dependency.
-package(jarcat_tool = "//tools/jarcat:jarcat_unzip")
+package(jarcat_tool = "//:jarcat_unzip")
 
 go_get(
     name = "logging",

tools/jarcat/BUILD

@@ -15,5 +15,5 @@ go_binary(
 go_binary(
     name = "jarcat_unzip",
     srcs = ["unzip_main.go"],
-    visibility = ["//third_party/go:all"],
+    visibility = ["PUBLIC"],
 )