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

tools: avoid mutating PATH env var when the right go can be picked up without it #679

Closed
hyangah opened this issue Sep 23, 2020 · 6 comments
Closed

tools: avoid mutating PATH env var when the right go can be picked up without it #679

hyangah opened this issue Sep 23, 2020 · 6 comments
Labels
FrozenDueToAge
Milestone
v0.17.1

Comments

@hyangah
Copy link
Contributor

hyangah commented Sep 23, 2020

When users choose to use a go different from the default go picked up from PATH
(e.g. by setting go.goroot, go.alternateTools.go, or using the extension's own
Go environment management feature), the extension appends $GOROOT/bin to
PATH in order to ensure all the underlying tools invoked by the extension to use
the same version of go the extension and its tools use for the project.

Since v0.15.x (v0.17.0 for mac) we changed the integrated terminal to use the same modified
PATH and pick up the same version of Go if the GOROOT is different from the go chosen
from the unmodified PATH.

This is not ideal if users prefers to switch between different go versions in the integrated terminal
using external environment management tools
(e.g. #544 (comment)).

Avoid changing PATH (even for tools) if users didn't configure the go version using the mean
the extension provides.
@hyangah hyangah added this to the v0.17.1 milestone Sep 23, 2020
@gopherbot
Copy link
Collaborator

Change https://golang.org/cl/256938 mentions this issue: src/goInstallTools.ts: mutate PATH only when necessary

@gopherbot
Copy link
Collaborator

Change https://golang.org/cl/258077 mentions this issue: [release] src/goInstallTools.ts: mutate PATH only when necessary

gopherbot pushed a commit that referenced this issue Sep 28, 2020
@hyangah
[release] src/goInstallTools.ts: mutate PATH only when necessary
20ba7a5 
When the extension chooses a different version of go
than the one from the system default - because the user
has configured `go.goroot` or `go.alternateTools.go`, or
used the `Go: Choose Go Environment` command - the extension
mutates the `PATH` (or `Path` on windows) environment variable
so all the underlying tools pick the same go version.
It also changes the environment variable collection used
in the integrated terminal so when the user invokes `go`
from the terminal, they use the go version consistent
with the extension.

But this behavior can conflict with external version
management software. Change the PATH environment only
if the extension is configured to choose the go binary.

Fixes #679.
Update #544.

Change-Id: I9f7acb26b752ed33dbde2b238a67ed09616b43e5
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/256938
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
(cherry picked from commit ab4b257)
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/258077
This was referenced Sep 29, 2020
Closed
Closed
Closed
@gopherbot
Copy link
Collaborator

Change https://golang.org/cl/258316 mentions this issue: src/goInstallTools.ts: add GOROOT/bin to PATH when it wasn't found from PATH

gopherbot pushed a commit that referenced this issue Sep 30, 2020
@hyangah
src/goInstallTools.ts: add GOROOT/bin to PATH when it wasn't found fr…
9bf9d64 
…om PATH

The fix for #679 changed to prepend GOROOT/bin to PATH or Path
only if users explicitly configured to use go different from what's found from PATH.
I forgot the case where go was not found from PATH and the extension picked up
a common default go installation directory. (C:\Go\bin or /usr/local/go/bin).

This change rewrote the fix - rewrote getBinPath to return why it chose the
path as well. Then, we mutate the PATH env var if getBinPath picked
go with a reason other than it's in PATH (why === 'path').

Since getBinPath and getBinPathWithPreferredGopathGoroot are
used in many places, this CL introduces getBinPathWithExplanation and
getBinPath wraps it.

Updates #679
Fixes #713

Change-Id: Ie00612fcef2cf4c2a187a263da04b342182c030b
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/258316
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
@gopherbot
Copy link
Collaborator

Change https://golang.org/cl/258519 mentions this issue: [release] src/goInstallTools.ts: add GOROOT/bin to PATH when it wasn't found from PATH

gopherbot pushed a commit that referenced this issue Sep 30, 2020
@hyangah
[release] src/goInstallTools.ts: add GOROOT/bin to PATH when it wasn'…
d9187fe 
…t found from PATH

The fix for #679 changed to prepend GOROOT/bin to PATH or Path
only if users explicitly configured to use go different from what's found from PATH.
I forgot the case where go was not found from PATH and the extension picked up
a common default go installation directory. (C:\Go\bin or /usr/local/go/bin).

This change rewrote the fix - rewrote getBinPath to return why it chose the
path as well. Then, we mutate the PATH env var if getBinPath picked
go with a reason other than it's in PATH (why === 'path').

Since getBinPath and getBinPathWithPreferredGopathGoroot are
used in many places, this CL introduces getBinPathWithExplanation and
getBinPath wraps it.

Updates #679
Fixes #713

Change-Id: Ie00612fcef2cf4c2a187a263da04b342182c030b
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/258316
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
(cherry picked from commit 9bf9d64)
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/258519
@gopherbot
Copy link
Collaborator

Change https://golang.org/cl/258557 mentions this issue: src/goEnvironmentStatus.ts: clear pre-installed terminal PATH mutation

gopherbot pushed a commit that referenced this issue Oct 1, 2020
@hyangah
src/goEnvironmentStatus.ts: clear pre-installed terminal PATH mutation
1b82f49 
Changes to vscode.EnvironmentVariableCollection persist across
vscode sessions, so when we decide not to mutate PATH, we need to clear
the preexisting changes. The new function 'clearGoRuntimeBaseFromPATH'
reverses almost all of what addGoRuntimeBaseToPATH did on the persisted
state.

Manually tested by setting/unsetting "go.alternateTools".

Also, fixes the case where 'go.alternateTools' is set as an alternate
tool name rather than an absolute path - in that case, we search the
alternate tool from PATH. In this case, the reason shouldn't be 'path'.

Manually tested by setting
   "go.alternateTools": { "go": "mygo.sh" }

where "mygo.sh" is another binary that exists in PATH and shells out go.

In addition to this, this change fixes an exception thrown while
calling updateIntegratedTerminal when no 'go' executable is found from
the default PATH. In that case, getBinPathFromEnvVar returns null,
and path.dirname throws an error for non-string params.
This results in the failure of updating terminal's PATH change and
the users will not see the picked go from the terminal.
This is a bug preexisted before 0.17.1

Manually tested by launching code without go in PATH.

Updates #679
Fixes #713

Change-Id: I240694cb4425e81998299ab38097393a0f3faf46
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/258557
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
@gopherbot
Copy link
Collaborator

Change https://golang.org/cl/258797 mentions this issue: [release] src/goEnvironmentStatus.ts: clear pre-installed terminal PATH mutation

gopherbot pushed a commit that referenced this issue Oct 1, 2020
@hyangah
[release] src/goEnvironmentStatus.ts: clear pre-installed terminal PA…
b6fe8e2 
…TH mutation

Changes to vscode.EnvironmentVariableCollection persist across
vscode sessions, so when we decide not to mutate PATH, we need to clear
the preexisting changes. The new function 'clearGoRuntimeBaseFromPATH'
reverses almost all of what addGoRuntimeBaseToPATH did on the persisted
state.

Manually tested by setting/unsetting "go.alternateTools".

Also, fixes the case where 'go.alternateTools' is set as an alternate
tool name rather than an absolute path - in that case, we search the
alternate tool from PATH. In this case, the reason shouldn't be 'path'.

Manually tested by setting
   "go.alternateTools": { "go": "mygo.sh" }

where "mygo.sh" is another binary that exists in PATH and shells out go.

In addition to this, this change fixes an exception thrown while
calling updateIntegratedTerminal when no 'go' executable is found from
the default PATH. In that case, getBinPathFromEnvVar returns null,
and path.dirname throws an error for non-string params.
This results in the failure of updating terminal's PATH change and
the users will not see the picked go from the terminal.
This is a bug preexisted before 0.17.1

Manually tested by launching code without go in PATH.

Updates #679
Fixes #713

Change-Id: I240694cb4425e81998299ab38097393a0f3faf46
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/258557
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
(cherry picked from commit 1b82f49)
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/258797
@golang golang locked and limited conversation to collaborators Oct 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
v0.17.1
Development

No branches or pull requests
2 participants
@hyangah @gopherbot