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.

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

x/tools/gopls: improve experience editing golang/go #35520

Open
russelldavis opened this issue Nov 12, 2019 · 13 comments
Labels
Milestone

Comments

@russelldavis
Copy link

@russelldavis russelldavis commented Nov 12, 2019

What did you do?

From the go/src/cmd dir:

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 12, 2019

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.

@gopherbot gopherbot added the gopls label Nov 12, 2019
@russelldavis

This comment has been minimized.

Copy link
Author

@russelldavis russelldavis commented Nov 13, 2019

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

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 13, 2019

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

@russelldavis

This comment has been minimized.

Copy link
Author

@russelldavis russelldavis commented Nov 13, 2019

@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

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 14, 2019

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

This comment has been minimized.

Copy link
Author

@russelldavis russelldavis commented Nov 14, 2019

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

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 14, 2019

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

This comment has been minimized.

Copy link
Author

@russelldavis russelldavis commented Nov 14, 2019

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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Nov 14, 2019

@russelldavis: I think you may have been looking at a file that is specifically broken with gopls (see #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

This comment has been minimized.

Copy link
Author

@russelldavis russelldavis commented Nov 14, 2019

@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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Nov 14, 2019

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

This comment has been minimized.

Copy link
Author

@russelldavis russelldavis commented Nov 14, 2019

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 #35520 (comment) make more sense?

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

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Nov 14, 2019

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.

@stamblerre stamblerre changed the title gopls: `check` on golang repo files times out x/tools/gopls: improve experience editing GOROOT with multiple GOROOTs Nov 14, 2019
@gopherbot gopherbot added this to the Unreleased milestone Nov 14, 2019
@gopherbot gopherbot added the Tools label Nov 14, 2019
@stamblerre stamblerre changed the title x/tools/gopls: improve experience editing GOROOT with multiple GOROOTs x/tools/gopls: improve experience editing golang/go with multiple copies Nov 14, 2019
@stamblerre stamblerre changed the title x/tools/gopls: improve experience editing golang/go with multiple copies x/tools/gopls: improve experience editing golang/go Dec 4, 2019
@stamblerre stamblerre modified the milestones: Unreleased, gopls v1.0 Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.