Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Relative paths in error messages #2736

Merged
merged 3 commits into from Dec 7, 2020
Merged

Relative paths in error messages #2736

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
54df87e
relativize paths in error messages
Nov 20, 2020
8c591bd
Address comments
Nov 20, 2020
93b9531
Do not relativize in runAndLogCommandToFile
Dec 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion go/tools/builders/compilepkg.go
View file
Open in desktop
Expand Up @@ -460,7 +460,7 @@ func runNogo(ctx context.Context, workDir string, nogoPath string, srcs []string
cmdLine := strings.Join(args, " ")
return fmt.Errorf("nogo command '%s' exited unexpectedly: %s", cmdLine, exitErr.String())
}
return errors.New(out.String())
return errors.New(string(relativizePaths(out.Bytes())))
} else {
if out.Len() != 0 {
fmt.Fprintln(os.Stderr, out.String())
Expand Down
36 changes: 29 additions & 7 deletions go/tools/builders/env.go
View file
Open in desktop
Expand Up @@ -60,6 +60,8 @@ type env struct {
workDirPath string

shouldPreserveWorkDir bool

execDir string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, removed

}

// envFlags registers flags common to multiple builders and returns an env
Expand All @@ -71,6 +73,7 @@ func envFlags(flags *flag.FlagSet) *env {
flags.StringVar(&env.installSuffix, "installsuffix", "", "Standard library under GOROOT/pkg")
flags.BoolVar(&env.verbose, "v", false, "Whether subprocess command lines should be printed")
flags.BoolVar(&env.shouldPreserveWorkDir, "work", false, "if true, the temporary work directory will be preserved")
env.execDir, _ = os.Getwd()
return env
}

Expand Down Expand Up @@ -133,18 +136,20 @@ func (e *env) runCommand(args []string) error {
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.Stderr = os.Stderr
return runAndLogCommand(cmd, e.verbose)
var stderr bytes.Buffer
cmd.Stdout = &stderr
cmd.Stderr = &stderr
return runAndLogCommand(cmd, &stderr, e.verbose)
}

// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot cleaner, very good idea!

cmd.Stdout = w
cmd.Stderr = os.Stderr
return runAndLogCommand(cmd, e.verbose)
cmd.Stderr = &stderr
return runAndLogCommand(cmd, &stderr, e.verbose)
}

func absEnv(envNameList []string, argList []string) error {
Expand All @@ -158,13 +163,15 @@ func absEnv(envNameList []string, argList []string) error {
return nil
}

func runAndLogCommand(cmd *exec.Cmd, verbose bool) error {
func runAndLogCommand(cmd *exec.Cmd, stderr *bytes.Buffer, verbose bool) error {
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()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil {
return fmt.Errorf("error running subcommand %s: %v", cmd.Path, err)
}
return nil
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

func relativizePaths(output []byte) []byte {
dir, err := os.Getwd()
if dir == "" || err != nil {
return output
}
dirBytes := make([]byte, len(dir), len(dir)+1)
copy(dirBytes, dir)
if !bytes.HasSuffix(dirBytes, []byte{filepath.Separator}) {
dirBytes = append(dirBytes, filepath.Separator)
}
return bytes.ReplaceAll(output, dirBytes, nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// formatCommand formats cmd as a string that can be pasted into a shell.
// Spaces in environment variables and arguments are escaped as needed.
func formatCommand(cmd *exec.Cmd) string {
Expand Down