Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

gopls does not honor go.alternateTools.go #3163

Closed
FiloSottile opened this issue Apr 8, 2020 · 10 comments
Closed

gopls does not honor go.alternateTools.go #3163

FiloSottile opened this issue Apr 8, 2020 · 10 comments

Comments

@FiloSottile
Copy link
Contributor

What version of Go, VS Code & VS Code Go extension are you using?

  • Run go version to get version of Go
    • N/A
  • Run code -v or code-insiders -v to get version of VS Code or VS Code Insiders
    • 1.43.2
      0ba0ca52957102ca3527cf479571617f0de6ed50
      x64
  • Check your installed extensions to get the version of the VS Code Go extension
    • 0.13.1
  • Run go env GOOS GOARCH to get the operating system and processor architecture details
    • darwin
      amd64

Share the Go related settings you have added/edited

{
    "go.alternateTools": {
        "go": "~/go/bin/go"
    },
    "go.toolsGopath": "~/.vscode/godev",
    "go.gopath": "~",
    "go.useLanguageServer": true,
}

Describe the bug

gopls uses the go tool in $PATH rather than the one in go.alternateTools.go.

For a custom go tool to be picked up by go/packages its directory needs to be added to $PATH. This was fixed for most tools in #2514, but it looks like gopls does not pick up the same custom env.

An inconsistent go tool between gopls and other tools is almost certainly not intended behavior.

@stamblerre
Copy link
Contributor

A work-around:

"gopls": {
    "env": {
        "PATH": "$HOME/go/bin/go:$PATH",
    }
}

We're still working on unifying the configuration between VS Code Go and gopls, so for the time being, it's safe to assume that no settings are shared between the two. The only setting that works in gopls is go.toolsEnvVars, because that tool environment is also used to start gopls.

As an additional consideration, this is more a question for @ramya-rao-a: Is this the recommended approach for specifying a go binary? How does this play with the go.goroot setting?

/cc @hyangah

@hyangah
Copy link
Contributor

hyangah commented Apr 8, 2020

I have a pending pr #3159

If alternateTools is specified for go, I think that will be picked up instead of the goroot in most cases -

if (alternateTool && path.isAbsolute(alternateTool) && executableFileExists(alternateTool)) {
- which may cause problem if both go.goroot and the alternateTools.go are both set and don't match.

@FiloSottile
Copy link
Contributor Author

It might be worth deprecating go.goroot. I am not aware of any reason to change GOROOT and not the go tool, and there should probably be one recommended way to select an alternative go tool.

@hyangah
Copy link
Contributor

hyangah commented Apr 8, 2020

The nice thing about go.goroot is it is more intuitive. Currently, vscode picks up the go from 'go.goroot/bin' directory unless it is specified through alternateTools. I use go.goroot to change my go version (not alternateTools)

@ramya-rao-a
Copy link
Contributor

We're still working on unifying the configuration between VS Code Go and gopls, so for the time being, it's safe to assume that no settings are shared between the two.

What d93a0ae did while fixing #2514 is to update the PATH env variable in the current running process to include path to the go binary. This ensured that any of the Go tools looking for the Go binary in the PATH would find it.

Is gopls not behaving like the other tools and using the PATH env var?

As an additional consideration, this is more a question for @ramya-rao-a: Is this the recommended approach for specifying a go binary? How does this play with the go.goroot setting?

The value set for the go.goroot setting is used to set the GOROOT env var
The value set for go in the alternateTools setting gets appended to the PATH env d93a0ae

Am no longer sure what the recommendation here should be. go.goroot is one of the first settings introduced for when users wanted to switch the version of go they were using. The alternateTools setting came into picture for when people wanted more control or use a custom script that eventually called go

@hyangah
Copy link
Contributor

hyangah commented Apr 9, 2020

@ramya-rao-a If go.goroot is set, that results in

  1. setting GOROOT env var,
  2. picking the go binary from GOROOT/bin (this is done by this code unless alternateTools is specified or go is found from PATH) , and
  3. appending GOROOT/bin to the end of PATH.

This is done by updateGoPathGoRootFromConfig as you pointed out. Setting through go.alternateTools causes a similar effect except it does not set GOROOT. If GOROOT is not set, that is good, but if a user starts to set go.goroot, that will be problematic. Either we should get rid of go.goroot, or treat the path to go specially by not allowing to replace go through go.alternateTools.

Personally, I like Intellij's approach of treating go SDK path specially and go.goroot is closer to that. Moreover, VS Code's setting page and settings.json intellisense works better with a simple string type than object type. Thus, I prefer using go.goroot. But still we need some code adjustment (especially in getBinPathWithPreferredGopath) and we need to avoid altering GOROOT env var completely with the modern versions of Go.

I believe gopls inherits the env vars acquired from getToolsEnvVars and gopls will pick up the go binary from the path. However, there are still problems

  1. goRuntimeBasePath is appended as the last path element. That means, if there is a preceding path element for other go binary, that will be picked up by gopls. My pending PR is an attempt to prevent that by placing the goRuntimeBasePath as the first PATH element.
  2. I am not 100% sure but it seems there is a slight chance of racing. updateGoPathGoRootFromConfig is async. If the language client creation happens first before updateGoPathGoRootFromConfig completes all its operation, I am afraid getToolsEnvVar may pick up an old PATH value. Again, I am not 100% sure about this theory.

@hyangah
Copy link
Contributor

hyangah commented Apr 22, 2020

@FiloSottile If you are using the Nightly version, this should be fixed now. Let us know if it's still an issue.

@FiloSottile

This comment has been minimized.

@FiloSottile
Copy link
Contributor Author

Switched to Nightly, dropped the workaround, and it seems to work!

@stamblerre
Copy link
Contributor

Closing since the fix is in the nightly and the extension is being migrated: #3247.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants