Relative paths in error messages #2736

robfig · 2020-11-20T02:46:48Z

What type of PR is this?
Feature

What does this PR do? Why is it needed?
Improve the error messages seen by everyday users of rules_go

Which issues(s) does this PR fix?
Fixes #2061

google-cla · 2020-11-20T02:47:00Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

jayconrod

Thanks for working on this.

Several comments below though.

go/tools/builders/env.go

jayconrod · 2020-11-20T17:54:01Z

go/tools/builders/env.go

+		return output
+	}
+	if !strings.HasSuffix(dir, "/")  {
+		dir += "/"


This won't work on Windows.

Also, I don't think this will replace the path itself with ..

Updated to use filepath.PathSeparator.

You're correct - if Getwd appeared in an error message with no following separator, it would not be converted to .. Do you think that's worth adding a case for?

It seems unlikely to come up, and I have a personal preference for e.g. path/to.go instead of ./path/to.go which would allow an implementation that "just works" for this case, so I'd be inclined to leave as-is.

Before: /private/var/tmp/_bazel_robfig/1db08896ecff7e5e96a745981c3b77e6/sandbox/darwin-sandbox/28708/execroot/corp/gocode/src/corp/analytics/searchterms/redshiftcopier.go:14:2: imported and not used: "corp/net/rpc" compilepkg: nogo: errors found by nogo during build-time code analysis: /private/var/tmp/_bazel_robfig/1db08896ecff7e5e96a745981c3b77e6/sandbox/darwin-sandbox/29923/execroot/corp/gocode/src/corp/net/rpc/server.go:125:7: the cancel function returned by context.WithTimeout should be called, not discarded, to avoid a context leak After: gocode/src/corp/analytics/searchterms/redshiftcopier.go:14:2: imported and not used: "corp/net/rpc" compilepkg: nogo: errors found by nogo during build-time code analysis: gocode/src/corp/net/rpc/server.go:125:7: the cancel function returned by context.WithTimeout should be called, not discarded, to avoid a context leak Fixes bazelbuild#2061

robfig · 2020-11-20T19:34:21Z

Thanks for the review. I made all the updates you requested, with one caveat about the . case, and re-tested.

jayconrod · 2020-11-20T19:42:48Z

go/tools/builders/env.go

 }

 // runCommandToFile executes a subprocess and writes the output to the given
 // writer.
 func (e *env) runCommandToFile(w io.Writer, args []string) error {
 	cmd := exec.Command(args[0], args[1:]...)
+	var stderr bytes.Buffer


Let's make sure this function doesn't change. It needs to produce a file, and we shouldn't alter the content of that file.

Maybe some other function can call runAndLogCommand with a *bytes.Buffer, then relativizePaths on that and copy to os.Stderr? Then neither runAndLogCommand nor this function need to change.

This is a lot cleaner, very good idea!

jayconrod · 2020-11-20T19:45:16Z

go/tools/builders/env.go

 	if verbose {
 		fmt.Fprintln(os.Stderr, formatCommand(cmd))
 	}
 	cleanup := passLongArgsInResponseFiles(cmd)
 	defer cleanup()
-	if err := cmd.Run(); err != nil {
+	err := cmd.Run()
+	os.Stderr.Write(relativizePaths(stderr.Bytes()))


In general, use fmt.Fprint or io.Copy when writing to an *os.File or any unknown kind of io.Writer. The Write may write fewer bytes than its given, and it's important to keep calling it until there's nothing left to write.

Good point, done.

I was wrong about this: io.Copy returns early if a Write call doesn't write the entire slice. I was thinking of Read, which may return fewer bytes without error. I get them confused, since they're different than the read and write system calls.

No need to change anything, just wanted to point that out.

jayconrod · 2020-11-20T19:47:47Z

go/tools/builders/env.go

@@ -60,6 +60,8 @@ type env struct {
 	workDirPath string

 	shouldPreserveWorkDir bool
+
+	execDir string


It doesn't look like this is actually used anymore. Can it be removed?

Good catch, removed

jayconrod · 2020-11-20T19:51:22Z

go/tools/builders/env.go

@@ -332,6 +339,21 @@ func absArgs(args []string, flags []string) {
 	}
 }

+// relativizePaths converts absolute paths found in the given output string to
+// relative, if they are within the PWD.


Replace PWD here with working directory. PWD means "print working directory", and since we're not printing it, I don't think we should use that acronym here.

TIL! I always thought it was "present working directory". Now the distinction between pwd and cwd makes more sense to me. Thanks

jayconrod · 2020-11-20T19:55:07Z

go/tools/builders/env.go

+	if !bytes.HasSuffix(dirBytes, []byte{filepath.Separator})  {
+		dirBytes = append(dirBytes, filepath.Separator)
+	}
+	return bytes.ReplaceAll(output, dirBytes, nil)


It looks like this still doesn't convert the working directory to .. For completeness, I think it ought to.

(I couldn't respond to the other comment chain; maybe there was a force push?)

OK, I didn't add that because there didn't seem to be a use case and it complicates the code further, but it was easy enough to add.

Ah, I didn't know that a force push prevents response. In the future I'll avoid that

robfig

Thanks for the review! Updated.

robfig · 2020-12-03T20:41:57Z

Bump

jayconrod

Sorry for the delay. I took last week off, and I'm still catching up on everything.

Two more small comments. Looking good though.

jayconrod · 2020-12-04T14:50:37Z

go/tools/builders/env.go

-	cmd.Stdout = w
-	cmd.Stderr = os.Stderr
-	return runAndLogCommand(cmd, e.verbose)
+	return runAndLogCommandOutput(cmd, w, os.Stderr, e.verbose)


Let's be careful that runCommandToFile does not relativize the output. It should just call runAndLogCommand.

Apologies, missed that requirement. Updated

jayconrod · 2020-12-04T14:54:43Z

go/tools/builders/env.go

-	cmd.Stdout = os.Stderr
-	cmd.Stderr = os.Stderr
-	return runAndLogCommand(cmd, e.verbose)
+	return runAndLogCommandOutput(cmd, os.Stderr, os.Stderr, e.verbose)


Instead of introducing runAndLogCommandOutput, I think it would be simpler to add the relativization here, especially since runCommandToFile should not use use it. Something like:

buf := &bytes.Buffer{} cmd := exec.Command(args[0], args[1:]...) // Redirecting stdout to stderr. This mirrors behavior in the go command: // https://go.googlesource.com/go/+/refs/tags/go1.15.2/src/cmd/go/internal/work/exec.go#1958 cmd.Stdout = os.Stderr cmd.Stdout = buf cmd.Stderr = buf err := runAndLogCommand(cmd, e.verbose) fmt.Fprint(os.Stderr, relativizePaths(buf.Bytes()) return err

LGTM! Updated

robfig · 2020-12-04T18:15:50Z

Updated, thanks for the review!

jayconrod

Looks good, thanks for fixing this!

Use os.Stderr.Write instead of fmt.Fprint. The latter formats the value as a []byte, not a string. (This was due to my own bad review comment on bazelbuild#2736. Sorry.

Use os.Stderr.Write instead of fmt.Fprint. The latter formats the value as a []byte, not a string. (This was due to my own bad review comment on bazelbuild#2736. Sorry.)

Use os.Stderr.Write instead of fmt.Fprint. The latter formats the value as a []byte, not a string. (This was due to my own bad review comment on #2736. Sorry.)

Before: /private/var/tmp/_bazel_robfig/1db08896ecff7e5e96a745981c3b77e6/sandbox/darwin-sandbox/28708/execroot/corp/gocode/src/corp/analytics/searchterms/redshiftcopier.go:14:2: imported and not used: "corp/net/rpc" compilepkg: nogo: errors found by nogo during build-time code analysis: /private/var/tmp/_bazel_robfig/1db08896ecff7e5e96a745981c3b77e6/sandbox/darwin-sandbox/29923/execroot/corp/gocode/src/corp/net/rpc/server.go:125:7: the cancel function returned by context.WithTimeout should be called, not discarded, to avoid a context leak After: gocode/src/corp/analytics/searchterms/redshiftcopier.go:14:2: imported and not used: "corp/net/rpc" compilepkg: nogo: errors found by nogo during build-time code analysis: gocode/src/corp/net/rpc/server.go:125:7: the cancel function returned by context.WithTimeout should be called, not discarded, to avoid a context leak Fixes #2061

Use os.Stderr.Write instead of fmt.Fprint. The latter formats the value as a []byte, not a string. (This was due to my own bad review comment on #2736. Sorry.)

Before: /private/var/tmp/_bazel_robfig/1db08896ecff7e5e96a745981c3b77e6/sandbox/darwin-sandbox/28708/execroot/corp/gocode/src/corp/analytics/searchterms/redshiftcopier.go:14:2: imported and not used: "corp/net/rpc" compilepkg: nogo: errors found by nogo during build-time code analysis: /private/var/tmp/_bazel_robfig/1db08896ecff7e5e96a745981c3b77e6/sandbox/darwin-sandbox/29923/execroot/corp/gocode/src/corp/net/rpc/server.go:125:7: the cancel function returned by context.WithTimeout should be called, not discarded, to avoid a context leak After: gocode/src/corp/analytics/searchterms/redshiftcopier.go:14:2: imported and not used: "corp/net/rpc" compilepkg: nogo: errors found by nogo during build-time code analysis: gocode/src/corp/net/rpc/server.go:125:7: the cancel function returned by context.WithTimeout should be called, not discarded, to avoid a context leak Fixes #2061

Use os.Stderr.Write instead of fmt.Fprint. The latter formats the value as a []byte, not a string. (This was due to my own bad review comment on #2736. Sorry.)

robfig requested a review from jayconrod as a code owner November 20, 2020 02:46

google-cla bot added the cla: no label Nov 20, 2020

robfig force-pushed the pr_relative_paths branch from 20d890d to 835dda8 Compare November 20, 2020 02:49

google-cla bot added cla: yes and removed cla: no labels Nov 20, 2020

jayconrod reviewed Nov 20, 2020

View reviewed changes

robfig force-pushed the pr_relative_paths branch from 835dda8 to 54df87e Compare November 20, 2020 19:20

jayconrod reviewed Nov 20, 2020

View reviewed changes

Address comments

8c591bd

robfig commented Nov 20, 2020

View reviewed changes

jayconrod reviewed Dec 4, 2020

View reviewed changes

Do not relativize in runAndLogCommandToFile

93b9531

jayconrod approved these changes Dec 7, 2020

View reviewed changes

jayconrod merged commit b337b84 into bazelbuild:master Dec 7, 2020

jayconrod mentioned this pull request Dec 21, 2020

go/tools/builders: don't format subcommand output as []byte #2766

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relative paths in error messages #2736

Relative paths in error messages #2736

robfig commented Nov 20, 2020

google-cla bot commented Nov 20, 2020

jayconrod left a comment

jayconrod Nov 20, 2020

robfig Nov 20, 2020

robfig commented Nov 20, 2020

jayconrod Nov 20, 2020

robfig Nov 20, 2020

jayconrod Nov 20, 2020

robfig Nov 20, 2020

jayconrod Dec 7, 2020

jayconrod Nov 20, 2020

robfig Nov 20, 2020

jayconrod Nov 20, 2020

robfig Nov 20, 2020

jayconrod Nov 20, 2020

robfig Nov 20, 2020

robfig left a comment

robfig commented Dec 3, 2020

jayconrod left a comment

jayconrod Dec 4, 2020

robfig Dec 4, 2020

jayconrod Dec 4, 2020

robfig Dec 4, 2020

robfig commented Dec 4, 2020

jayconrod left a comment

Relative paths in error messages #2736

Relative paths in error messages #2736

Conversation

robfig commented Nov 20, 2020

google-cla bot commented Nov 20, 2020

jayconrod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfig commented Nov 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfig left a comment

Choose a reason for hiding this comment

robfig commented Dec 3, 2020

jayconrod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfig commented Dec 4, 2020

jayconrod left a comment

Choose a reason for hiding this comment