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

x/tools/gopls: -remote=auto doesn't use different addresses for different Go versions #50991

Open
mvdan opened this issue Feb 3, 2022 · 7 comments
Open

x/tools/gopls: -remote=auto doesn't use different addresses for different Go versions #50991

mvdan opened this issue Feb 3, 2022 · 7 comments
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/backlog

Comments

@mvdan
Copy link
Member

mvdan commented Feb 3, 2022 •
edited
Loading

If I start vim with govim with the go in PATH at go version devel go1.18-9784ef8ab1 Wed Feb 2 16:21:08 2022 +0000 linux/amd64, and then in another terminal I do the same but with go at go version go1.17.6 linux/amd64, both end up using the gopls with Go master (roughly 1.18beta2).

I imagine -remote=auto should get smarter and realise that they should both use different gopls processes, because the Go packages being loaded may be different. In my case, one of the dependencies uses build tags like go1.17 and go1.18.

@findleyr points me to https://cs.opensource.google/go/x/tools/+/master:internal/lsp/lsprpc/autostart_posix.go;l=40;drc=980829d8a1276d654bdf03f4a38f26eee797d0b1, which does include the buildid of the gopls binary itself, but not the version of Go.

I think we should include all build settings that can affect how the go list commands will behave, including:

  • go version
  • go env members such as GOOS, GOARCH, GOFLAGS, CGO_ENABLED, or CC - but not others like GOMOD or GOCACHE

In fact, we could hash most of go env alone, given that we also have the version of Go itself as GOVERSION. Hashing the entire thing is likely a bad idea, because some env vars shouldn't matter, and others aren't deterministic.

Build info
----------
golang.org/x/tools/gopls master
    golang.org/x/tools/gopls@v0.0.0-20220124164225-97de9ec46646 h1:XVKmWAnc/2QeTRtwIAQ43/oMv9ZrN73SvrSAvW00bhk=
    github.com/BurntSushi/toml@v0.4.1 h1:GaI7EiDXDRfa8VshkTj7Fym7ha+y8/XxIgD2okUIjLw=
    github.com/google/go-cmp@v0.5.6 h1:BKbKCqvP6I+rmFHt06ZmyQtvB8xAkWdhFyr0ZUNZcxQ=
    github.com/sergi/go-diff@v1.1.0 h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0=
    golang.org/x/mod@v0.5.1 h1:OJxoQ/rynoF0dcCdI7cLPktw/hR2cueqYfjm43oqK38=
    golang.org/x/sync@v0.0.0-20210220032951-036812b2e83c h1:5KslGYwFpkhGh+Q16bwMP3cOontH8FOep7tGV86Y7SQ=
    golang.org/x/sys@v0.0.0-20211019181941-9d821ace8654 h1:id054HUawV2/6IGm2IV8KZQjqtwAOo2CYlOToYqa0d0=
    golang.org/x/text@v0.3.7 h1:olpwvP2KacW1ZWvsR7uQhoyTYvKAupfQrRGBFM352Gk=
    golang.org/x/tools@v0.1.9-0.20220124164225-97de9ec46646 h1:f8aekWvlQQ8ZhD8SL7lOu18dtWslZYl029PN2F0VnS4=
    golang.org/x/xerrors@v0.0.0-20200804184101-5ec99f83aff1 h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=
    honnef.co/go/tools@v0.3.0-0.dev.0.20220120121056-246e50be7597 h1:YYX8xj3UjOv5K17GjaNsJXycbFalZnPV1JhVTIXt/b4=
    mvdan.cc/gofumpt@v0.1.1 h1:bi/1aS/5W00E2ny5q65w9SnKpWEF/UIOqDYBILpo9rA=
    mvdan.cc/xurls/v2@v2.3.0 h1:59Olnbt67UKpxF1EwVBopJvkSUBmgtb468E4GVWIZ1I=
@findleyr
Copy link
Member

findleyr commented Feb 3, 2022

Thanks for filing.

It looks like we could just hash in the output of:

go env GOVERSION GOOS GOARCH GOFLAGS CGO_ENABLED CC

@bcmills @matloob does that seem right?

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Feb 3, 2022
@gopherbot gopherbot added this to the Unreleased milestone Feb 3, 2022
@bcmills
Copy link
Contributor

bcmills commented Feb 3, 2022

GOROOT also affects the output of go list, and GOPATH (if in GOPATH mode, if gopls supports that).

Probably also anything that affects module resolution (GO111MODULE, GOINSECURE, GOMODCACHE, GONOPROXY, GONOSUMDB, GOPRIVATE, GOPROXY, GOVCS, GOWORK), unless gopls is explicitly masking those out.

Maybe it would be better to just hash the full output of go env and mask out the ones known not to matter. 😅

@myitcv
Copy link
Member

myitcv commented Feb 3, 2022

It looks like we could just hash in the output of:

@findleyr I might well be misremembering things and therefore holding this totally wrong, but aren't a number of those variables set via the editor and therefore managed per session in gopls (perhaps "session" is the wrong term).

Not to say that all of those listed by you and @bcmills can be properly managed that way, rather just trying to recall something of our last conversation in this space.

@findleyr
Copy link
Member

findleyr commented Feb 3, 2022

@myitcv you're thinking of #37830 -- gopls forwards go env. Looking at that now, it is of course naive, and doesn't differentiate go environment variables that may be set vs those that are baked into the go command (such as GOVERSION). The latter are precisely what we need to hash into the daemon address.

I can sense that we might be on the verge of going down a rabbit hole as this is probably quite tricky to get 100% right :). Pragmatically, I would propose that for now we simply hash in either go version or the go executable path.

(once Go 1.18 is out, I really want to dismantle the lsprpc package entirely in favor of using @ianthehat's new jsonrpc2 library, I don't want to spend too much time on this now)

@mvdan
Copy link
Member Author

mvdan commented Feb 3, 2022

Even just hashing the Go version would solve my immediate problem, so that seems fine with me. I'd still like to keep a tracking issue for the other build parameters, though - I regularly start vim+govim with different GOOS+GOARCH combinations to use go-to-definition and other features for different targets, like js/wasm or darwin.

@findleyr
Copy link
Member

findleyr commented Feb 3, 2022

Given that gopls forwards GOOS and GOARCH, wouldn't that be honored by go list?

@mvdan
Copy link
Member Author

mvdan commented Feb 3, 2022

Oh, maybe that works already, I haven't double checked with an existing gopls daemon :)

@suzmue suzmue modified the milestones: Unreleased, gopls/on-deck Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/backlog
Development

No branches or pull requests
6 participants
@myitcv @mvdan @bcmills @suzmue @gopherbot @findleyr