x/tools/gopls: improve error message when using wrong go command with Go source distribution

russelldavis commented 4 years ago

What did you do?

gopls -rpc.trace -v check compile/main.go

What did you expect to see?

The command succeeds.

What did you see instead?

The command times out after 30s. Output: gopls.txt

Build info

golang.org/x/tools/gopls 0.2.0
    golang.org/x/tools/gopls@v0.2.0 h1:ddCHfScTYOG6auAcEKXCFN5iSeKSAnYcPv+7zVJBd+U=
    github.com/BurntSushi/toml@v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
    github.com/sergi/go-diff@v1.0.0 h1:Kpca3qRNrduNnOQeazBd0ysaKrUJiIuISHxogkT9RPQ=
    golang.org/x/sync@v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU=
    golang.org/x/tools@v0.0.0-20191108194844-46f05828f2fe h1:FNzasIzfY1IIdyTs/+o3Qv1b7YdffPbBXyjZ5VJJdIA=
    golang.org/x/xerrors@v0.0.0-20190717185122-a985d3407aa7 h1:9zdDQZ7Thm29KFXgAX/+yaf3eVbP7djjWp/dXAppNCc=
    honnef.co/go/tools@v0.0.1-2019.2.3 h1:3JgtbtFHMiCmsznwGVTUWbgGov+pVqnlf1dEJTNAXeM=

Go info

go version go1.13.4 darwin/amd64

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/russell/Library/Caches/go-build"
GOENV="/Users/russell/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/russell/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13.4/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13.4/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/russell/dev/go/go/src/cmd/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/bk/1vdjn7vd19x41x0ztsbvmw400000gq/T/go-build918707310=/tmp/go-build -gno-record-gcc-switches -fno-common"

gopherbot commented 4 years ago

Thank you for filing a gopls issue! Please take a look at the Troubleshooting guide, and make sure that you have provided all of the relevant information here.

russelldavis commented 4 years ago

I (partially) figured out what's going on here: the golang repo is special in that it can only be properly compiled or analyzed when GOROOT is set to its own location.

This is because much of its own code is importable as "standard packages" via GOROOT, and GOROOT packages take precedence. So if GOROOT points to a different installation of go, those packages will get imported instead, resulting in access errors for packages under internal directories (not to mention the code being the wrong version).

So, the fix is to set GOROOT when working with the golang repo.

I think this issue should remain open to fix the meta issue here, which is that gopls didn't output any useful info or errors - it just timed out after 30s. I'm still not totally sure exactly what it got hung up on -- I only discovered the solution after running go build inside the repo and seeing "use of internal package" errors. I'm guessing gopls might be running into something similar but failing to print the error.

ianlancetaylor commented 4 years ago

Note that generally you should not set the GOROOT environment at all. It should only be needed in unusual circumstances.

russelldavis commented 4 years ago

@ianlancetaylor thanks for jumping in. Can you clarify a bit? I just described a circumstance where setting GOROOT was needed -- are you just noting that this circumstance is unusual, or saying that it shouldn't be needed even in this case? (If the latter, can you elaborate on what the alternative would be?)

ianlancetaylor commented 4 years ago

As far as I can see, it shouldn't be needed even in this case. I don't understand why it is needed. Is GOROOT set in the environment normally? If not, what does go env GOROOT print? Why is that different from the GOROOT value you want to use? For the GOROOT value you want to use, what is in GOROOT/bin?

russelldavis commented 4 years ago

Ah, interesting, let's see if we can get to the bottom of it.

Is GOROOT set in the environment normally?

Nope

If not, what does go env GOROOT print?

/usr/local/Cellar/go/1.13.4/libexec (the location of go I installed with homebrew)

Why is that different from the GOROOT value you want to use?

To be clear, it's not that I want to use a different GOROOT, just that things break if I don't. The value I need to change it to is the location of the local clone of https://github.com/golang/go/ that I'm trying to analyze with gopls.

For the GOROOT value you want to use, what is in GOROOT/bin?

Just go and gofmt, which were created by running src/make.bash.

ianlancetaylor commented 4 years ago

Change your PATH to put the go tool that you want to use first. That is a better approach than setting GOROOT, as it means that the go tool and the GOROOT will be in synch.

russelldavis commented 4 years ago

Ah that makes sense, thanks. (For posterity: gopls figures out the root by shelling out to go env GOROOT.)

With that resolved, any thoughts on ways we could make this easier for newcomers to the golang/go repo? If they're like me, they'll find a few things very broken without obvious reasons or solutions:

In VS Code, much of the go plugin functionality (like goto-definition) won't work.
In GoLand:
- goto-definition works, but takes you to definitions at the locally installed go root, not in the repo itself
- imports of internal packages get marked as errors (because they get resolved to the wrong root)
The gopls CLI has this issue

These are all simple once you know that, when using any kind of tooling on the contents of the golang repo, you must use the go binary built from that same repo (or set GOROOT to that repo). But AFAICT, this isn't mentioned anywhere -- should we add it somewhere, maybe https://golang.org/doc/contribute.html?

Even better would be if things could be changed so that wasn't a requirement -- in a perfect world, I'd be able to analyze the code in the golang repo without having to build that repo or mess with my path. But I assume that would be a huge/difficult change at this point.

stamblerre commented 4 years ago

@russelldavis: I think you may have been looking at a file that is specifically broken with gopls (see https://github.com/golang/go/issues/33548). For other files in the Go repo, gopls should work fine, but you're right in saying that your local modifications won't be visible through gopls unless you modify your PATH (something like export PATH=/path/to/go/bin:$PATH).

russelldavis commented 4 years ago

@stamblerre sadly it's not just that file. It seems to be broken for everything under https://github.com/golang/go/tree/master/src/cmd. As I mentioned above, tooling in IDEs like VSCode and GoLand is broken for all of those files as well, for the same reason.

stamblerre commented 4 years ago

GoLand doesn't use gopls, and I just tested gopls in VS Code with a few files in different directories under go/src/cmd, and it seems to work fine with everything except for go/src/cmd/main.go. This leads me to believe that there is something about your setup that's causing this issue. Can you share the output of gopls -rpc.trace -v check /path/to/file.go for another file that's broken for you?

russelldavis commented 4 years ago

The larger issue I'm talking about here isn't specific to gopls (though I suspect gopls is being affected by it). I can create a new issue for it not tagged with gopls if you'd prefer.

The point is that an import of, e.g., cmd/compile/internal/gc, always gets resolved to the cmd module in GOROOT, even when used inside your own module named cmd (which of course happens in the golang repo). So the all the cmd/... imports in the golang repo get resolved to GOROOT's cmd rather than its own, and that causes things to break unless GOROOT happens to point back into the same repo.

This is definitely why there are issues in GoLand, and I'm pretty sure the cause of the issues in VSCode and gopls.

With that in mind, hopefully my suggestions at https://github.com/golang/go/issues/35520#issuecomment-553688653 make more sense?

Here's a trace of gopls -rpc.trace -v check doc.go from go/src/cmd/compile: gopls.txt

stamblerre commented 4 years ago

I think probably that the best option here is for us to make error messages more explicit in gopls to indicate the user needs to add the GOROOT that they are inspecting to their PATH. Maybe the editor could notice that it looks like the user is looking at the Go repo and suggest updating their configurations.

One option that is currently available to you (to avoid mucking with your PATH), is creating a workspace setting in VS Code that sets the GOROOT just for this repository ("go.goroot": "/path/to/go/").

Even better would be if things could be changed so that wasn't a requirement -- in a perfect world, I'd be able to analyze the code in the golang repo without having to build that repo or mess with my path. But I assume that would be a huge/difficult change at this point.

I don't think it's possible to achieve this through the go command or go/packages alone. We can certainly investigate options in gopls for making the experience with the Go repository a bit more seamless.

pjweinb commented 4 years ago

This just bit me too. There is a somewhat cryptic reference to GOROOT in the logs, but the message doesn't say what the user should do. If the user isn't looking at the log right then, the relevant message is hard to find in among all the other log messages.

Perhaps opening a source distribution with mismatched GOROOT deserves a special check and a visible message.

stamblerre commented 3 years ago

The last thing remaining to improve here are the error messages when someone is using the wrong go command while working on the Go source distribution--leaving this issue open to track that work.

golang / go