getsentry / raven-go

Sentry client in Go
https://sentry.io
BSD 3-Clause "New" or "Revised" License
561 stars 148 forks source link

avoid using go/build to trim prefix #139

Open benesch opened 7 years ago

benesch commented 7 years ago

go/build will return gopath/goroot paths for the machine running it, which isn't what you want -- the path being stripped are from the build machine. a dumber approach, just looking for /src/ does almost the same thing without using go/build.

Submitted on behalf of @dt.

mattrobenolt commented 7 years ago

Is this guaranteed to be true? I feel like there'd be cases where "/src/" doesn't exist in the path.

dt commented 7 years ago

hm pretty sure anything that was in GOPATH will have src: https://github.com/golang/go/wiki/GOPATH#directory-layout

if you somehow got Go to build something that wasn't in GOPATH, well then trimming GOPATH off it wouldn't work anyway.

That said, either way, the current code using go/build is getting the executing machine's values for GOPATH / GOROOT, not the ones from the machine used to build and thus that are in the stacks (except in the special case of running on the build machine), so I think even with its limitations, this naive heuristic is hopefully still a bit more reliable/portable than using go/build.

benesch commented 7 years ago

Actually, thinking about this more, I think this patch makes things worse. Trimming from the last /src/ is going to break whenever a Go package uses an internal src directory, e.g. github.com/cockroachdb/cockroach/src/.

Seems to me we should just avoid trimming entirely. If users want to trim, they can do so with GOROOT_FINAL and/or

go build -gcflags=-trimpath=$GOPATH -asmflags=-trimpath=$GOPATH

(I'm not necessarily suggesting that the official go-raven library default to no-trimming, but would be nice to have it exposed as an option. Thoughts, @dt @tamird?)

tamird commented 7 years ago

TIL about trimpath; indeed, path trimming is fraught. It's too bad you can't ask for the GOROOT/GOPATH that were built into the binary at runtime. +1 to removing this functionality altogether.