Skip to content
This repository was archived by the owner on Aug 30, 2023. It is now read-only.

avoid using go/build to trim prefix #139

Open
benesch wants to merge 1 commit into
base: master
Choose a base branch
from
Open

avoid using go/build to trim prefix #139

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 9 additions & 21 deletions stacktrace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@ package raven

import (
"bytes"
"go/build"
"io/ioutil"
"path/filepath"
"runtime"
"strings"
"sync"
Expand Down Expand Up @@ -199,25 +197,15 @@ func fileContext(filename string, line, context int) ([][]byte, int) {
return lines[start:end], idx
}

var trimPaths []string

// Try to trim the GOROOT or GOPATH prefix off of a filename
// trimPath tries to trim the GOROOT or GOPATH prefix off of a filename.
Copy link
Copy Markdown
Contributor

@mattrobenolt mattrobenolt Aug 2, 2017

Choose a reason for hiding this comment

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

I think this comment is now irrelevant, right? Since we don't look at these values at all.

//
// We don't use `go/build` here as it will look at the _current_ values,
// so instead we just assume the last `/src/`.
func trimPath(filename string) string {
for _, prefix := range trimPaths {
if trimmed := strings.TrimPrefix(filename, prefix); len(trimmed) < len(filename) {
return trimmed
}
}
return filename
}

func init() {
// Collect all source directories, and make sure they
// end in a trailing "separator"
for _, prefix := range build.Default.SrcDirs() {
if prefix[len(prefix)-1] != filepath.Separator {
prefix += string(filepath.Separator)
}
trimPaths = append(trimPaths, prefix)
const src = "/src/"
Copy link
Copy Markdown

@tamird tamird Jun 5, 2017

Choose a reason for hiding this comment

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

should this use os.PathSeparator (or filepath.Join, equivalently) instead of hardcoding /?

Copy link
Copy Markdown
Contributor Author

@benesch benesch Jun 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Welp, what if you're running a binary cross-compiled on Linux for Windows? My point is that I think neither the current behavior nor your proposed alternative is correct. Perhaps we need to look for both /src/ and \src\ on all platforms?

Copy link
Copy Markdown

@tamird tamird Jun 5, 2017

Choose a reason for hiding this comment

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

Indeed. In fact, we're completely speculating on this behaviour at the moment. We probably need to experiment (or find the code) to find out how source locations are encoded into Go binaries. See golang/go#19784, for instance, which implies some confusion in this area.

Copy link
Copy Markdown

@dt dt Jun 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

that said, even if /src/ doesn't work for some platform or cross-compilation combinations, it should still probably do better than go/build's user-specific paths, right? so does it make sense to call this an incremental improvement even if there's potentially further we could go?

Copy link
Copy Markdown

@tamird tamird Aug 2, 2017

Choose a reason for hiding this comment

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

Indeed. @mattrobenolt?

prefix := strings.LastIndex(filename, src)
if prefix == -1 {
return filename
}
return filename[prefix+len(src):]
}