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: handle discrepancies in Go environment when connecting to daemon #37830

Closed
findleyr opened this issue Mar 12, 2020 · 7 comments
Closed
Assignees
Labels
Milestone

Comments

@findleyr
Copy link
Contributor

@findleyr findleyr commented Mar 12, 2020

Forked from @zikaeroh's comment on #34111:
#34111 (comment)

Another thing I thought of: how does gopls in this mode determine which go binary to use? I'm guessing it inherits PATH from whichever process created it, which might be wrong for future gopls invocations. For example, if I have more than one Go version, I might set PATH accordingly, or if I'm working on the go tree itself, I might set PATH to point to that to see how things behave.

This point could be generalized to any change in go env that would affect the behavior of the daemon. One solution is to hash all relevant environment into the automatic daemon socket path (so that different environments should get different daemons) -- we do this for the gopls build ID. But as @zikaeroh points out, this becomes a slippery slope.

Another option would be to carefully document this behavior, compare environments in the forwarder-daemon handshake, and issue loud warnings.

@gopherbot gopherbot added this to the Unreleased milestone Mar 12, 2020
@gopherbot gopherbot added the Tools label Mar 12, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 12, 2020

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 Mar 12, 2020
@findleyr findleyr self-assigned this Mar 12, 2020
@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Mar 12, 2020

My personal recommendation would be something in the middle, hashing a few known-common things to get most of the cases correct and leaving the rest to documentation/warnings. Off the top of my head I would think this list for the average user is:

  • go version (build ID?) - changes on Go upgrades
  • go env GOROOT (maybe GOTOOLDIR?) - often lives in different folders between versions on Linux distros
  • Maybe go env GOPATH

Many other things can end up mattering (GOPACKAGESDRIVER, cgo-related options, GOPROXY/GONOPROXY which may have security implications) but are much more likely to have been explicitly set by a person who understands the impact when these values change and knows how to deal with a warning.

@stamblerre stamblerre modified the milestones: Unreleased, gopls/v0.4.0 Mar 12, 2020
@myitcv
Copy link
Member

@myitcv myitcv commented Mar 13, 2020

We can still set environment variables used by gopls in calls to go/packages so I don't think it's critical to include those in the instance key.

My two cents: keep things super simple for the majority (for now), and experiment with making the caller do the hard work if they want to handle more complex situations. This is, incidentally, why I requested support for what @findleyr kindly delivered via -remote=auto;id. In my case I use process mount namespaces for different go versions, hence the ID I use will at least include a hash of the significant mounts (one of which includes Go version). But the onus is always on me as the caller to derive the ID by hashing whatever I want to be the key. Not least because gopls can't determine what should/shouldn't be in the ID in all cases.

This way if patterns of usage start to emerge where lots of people are running into problem X then we could look at have -remote=auto automatically hash A, B and C.

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Mar 13, 2020

experiment with making the caller do the hard work if they want to handle more complex situations

My issue with this approach is that I think gopls knows much better than the client what things could affect the analysis. I think there are a few things that are common enough that with that approach, every client's going to be doing some hashing if they want to work around environment-related issues.

Not least because gopls can't determine what should/shouldn't be in the ID in all cases.

Which cases would gopls not know? The example you've given is Go version, which gopls would know, but I'm curious which other things you think might also affect the ID that only the client would know but would still affect gopls internally.

@myitcv
Copy link
Member

@myitcv myitcv commented Mar 13, 2020

My issue with this approach is that I think gopls knows much better than the client what things could affect the analysis

I completely agree with this point. I'm very much in favour of having zero logic in the client/editor. The distinction I draw here is that it's the user who is necessarily defining the key, because the client/editor also can't know.

Which cases would gopls not know?

Basically any path. e.g. where go env GOPATH is the same value, e.g. /path/to/gopath, but because of process mount namespaces (or FUSE file systems or whatever) that path points to a different actual/real (terminology?) location.

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Mar 19, 2020

My personal recommendation would be something in the middle, hashing a few known-common things to get most of the cases correct and leaving the rest to documentation/warnings.

That seems reasonable. Here's what I'll do: first, I'll create a key-value map of environment values that affect execution, compare them in the handshake, and raise an error if there is a mismatch. Then we can wait for people to complain that this error occurs under normal operation, and in those cases we can hash the environment value in question into the daemon path.

Enough people use different versions of the go command to warrant including this in the daemon path. Hashing either go version or go tool buildid $(which go) would work.

@stamblerre stamblerre modified the milestones: gopls/v0.4.0, gopls/v0.5.0 Apr 2, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented May 15, 2020

Change https://golang.org/cl/234109 mentions this issue: internal/lsp/lsprpc: forward the go environment in initialize requests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.