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

Wrong "export PATH" statement for Powershell Core #378

Closed
kirides opened this issue Jul 19, 2020 · 12 comments
Closed

Wrong "export PATH" statement for Powershell Core #378

kirides opened this issue Jul 19, 2020 · 12 comments
Assignees
Milestone

Comments

@kirides
Copy link

kirides commented Jul 19, 2020

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

  • Run go version to get version of Go
    • go version go1.14.2 windows/amd64
  • Run code -v or code-insiders -v to get version of VS Code or VS Code Insiders
    • 1.47.2
      17299e413d5590b14ab0340ea477cdd86ff13daf
      x64
  • Check your installed extensions to get the version of the VS Code Go extension
    • 2020.7.820 (just ran the Extension: Check for Extension Updates command)
  • Run go env to get the go development environment details
    • set GO111MODULE=
      set GOARCH=amd64
      set GOBIN=
      set GOCACHE=C:\Users\USER\AppData\Local\go-build
      set GOENV=C:\Users\USER\AppData\Roaming\go\env
      set GOEXE=.exe
      set GOFLAGS=
      set GOHOSTARCH=amd64
      set GOHOSTOS=windows
      set GOINSECURE=
      set GONOPROXY=
      set GONOSUMDB=
      set GOOS=windows
      set GOPATH=A:\Dev\Go
      set GOPRIVATE=
      set GOPROXY=https://proxy.golang.org,direct
      set GOROOT=c:\go
      set GOSUMDB=sum.golang.org
      set GOTMPDIR=
      set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
      set GCCGO=gccgo
      set AR=ar
      set CC=gcc
      set CXX=g++
      set CGO_ENABLED=1
      set GOMOD=C:\Users\USER\Desktop\go\langsrv\go.mod
      set CGO_CFLAGS=-g -O2
      set CGO_CPPFLAGS=
      set CGO_CXXFLAGS=-g -O2
      set CGO_FFLAGS=-g -O2
      set CGO_LDFLAGS=-g -O2
      set PKG_CONFIG=pkg-config
      set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\USER\AppData\Local\Temp\go-build460855558=/tmp/go-build -gno-record-gcc-switches

Share the Go related settings you have added/edited

    "terminal.integrated.shell.windows": "pwsh.exe",
    "terminal.integrated.env.windows": {
        "PATH": "${env:PATH};C:\\msys64\\mingw64\\bin;C:\\msys64\\usr\\bin;c:\\go\\bin"
    },
    "terminal.integrated.shellArgs.windows": [
        "-NoLogo",
    ],
    "go.useLanguageServer": true,
    "[go]": {
        "editor.snippetSuggestions": "top",
        "editor.formatOnSave": true,
        "editor.codeActionsOnSave": {
            "source.organizeImports": true,
        }
    },
    "gopls": {
        "usePlaceholders": true, // add parameter placeholders when completing a function
        // Experimental settings
        "completeUnimported": true, // autocomplete unimported packages
        "deepCompletion": true, // enable deep completion
    },

Describe the bug

Using Powershell Core leads to this:
grafik

The Terminal is started using pwsh.exe as seen in configuration above. The name displayed in the integrated terminal switches to dotnet after booting up the pwsh.exe

Issue is simmilar to #314

Steps to reproduce the behavior:

  1. Set pwsh.exe (powershell core) as terminal
  2. Open the integrated terminal
  3. observe the mentioned error
@hyangah
Copy link
Contributor

hyangah commented Jul 20, 2020

@kirides thanks for the report. It's a duplicate of #314. Due to an issue in our nightly release workflow #363 we didn't publish a new Nightly version that includes the fix yet.

@kirides
Copy link
Author

kirides commented Jul 20, 2020

While that's true, i don't see the same special handling for pwsh in the committed code that closed #314
with a wild guess, i'd imagine that line would need a || ... == "pwsh", though like i said, the name of the terminal changes from pwsh to dotnet and i don't know if that can be accounted for at that point :/

@hyangah
Copy link
Contributor

hyangah commented Jul 20, 2020

@kirides thanks for the headsup.
@mcjcloud - looking at microsoft/vscode#74233 (comment), I think we need adjustment in shell detection. https://github.com/microsoft/vscode-python/blob/649156a09ccdc51c0d20f7cd44540f1918f9347b/src/client/common/terminal/shellDetector.ts#L24-L35 is a good starting point. And, for unrecognized shell types, let's just give up and do nothing.

@hyangah hyangah added this to the v0.16.0 milestone Jul 20, 2020
@gopherbot
Copy link
Collaborator

Change https://golang.org/cl/244378 mentions this issue: src/goEnvironmentStatus.ts: improve integrated terminal support

@mcjcloud
Copy link
Contributor

I've created a change which will do two of the above items: add pwsh as a recognized shell name and prevent any export command from being run as a fallback when no shell is recognized.

I'm leaving this issue open to track progress on improving the integrated terminal detection further, starting with what @hyangah mentioned (https://github.com/microsoft/vscode-python/blob/649156a09ccdc51c0d20f7cd44540f1918f9347b/src/client/common/terminal/shellDetector.ts#L24-L35).

gopherbot pushed a commit that referenced this issue Jul 22, 2020
This CL adds `pwsh` and several Linux shells to the list of supported
shells. It also remove the Linux shell export command as the fallback.

Updates #378

Change-Id: I43270344df2e53676e8993f3be81445c44d70d1c
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/244378
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
@hyangah
Copy link
Contributor

hyangah commented Jul 23, 2020

#146 (comment) @segevfiner suggested ctx.environmentVariableCollection which looks promising. I am not yet sure if this also applies to the terminal already open before the extension determines the GOPATH, but in worst case, the extension can suggest close/open if the extension can't do it itself.

cc @mcjcloud

@segevfiner
Copy link
Contributor

@hyangah It is supposed to come with handling for those race conditions.

@hyangah
Copy link
Contributor

hyangah commented Jul 23, 2020

microsoft/vscode#46696 is the overview of the API history,
https://github.com/microsoft/vscode-extension-samples/blob/2e5b265e00417dde0fea453eb8abdd34a8ec7d2c/terminal-sample/src/extension.ts is the sample.

I am still struggling with the interaction with the terminal login shell.
After prepending the desirable go path to the existing PATH, vscode suggested to relaunch the terminal.

Then, when relaunching the terminal, it seems like the integrated terminal runs .bash_profile again
where I have various PATH prepending again.
As a result, the go path I prepended ends up in the middle of the PATH. :-(

@hyangah
Copy link
Contributor

hyangah commented Jul 23, 2020

Maybe this microsoft/vscode#99878

And the current suggested workaround still requires to detect the underlying shell and send the right command.
microsoft/vscode#99878 (comment)

@segevfiner
Copy link
Contributor

Also note that the Python extension does a similar thing for selecting Python/virtualenv so its something to look at. I actually won't be suprised if that API was created for it. But it might indeed still have issues, I'm not sure if the Python extension is using it yet either.

@gopherbot
Copy link
Collaborator

Change https://golang.org/cl/245127 mentions this issue: src/goEnvironmentStatus.ts: use the new EnvironmentVariableCollection API

hyangah added a commit to hyangah/vscode-go that referenced this issue Jul 28, 2020
… API

vscode.ExtensionContext.EnvironmentVariableCollection is available since
1.46 (May 2020 release https://code.visualstudio.com/updates/v1_46#_environment-variable-collection)
This API is specifically designed to help easy mutation of terminal
environment, so we are trying to use it now.

I hope this helps to address a couple of bugs related to the shell
detection and the correct command and path escaping discovered
during testing on Windows.

Unfortunately, this API does not work well on Mac
microsoft/vscode#99878 due to the peculiarity
described in
https://code.visualstudio.com/docs/editor/integrated-terminal#_why-are-there-duplicate-paths-in-the-terminals-path-environment-variable-andor-why-are-they-reversed

So, we use the new API only for non-darwin OSes for now. On Mac,
we send the command as done before unless users explicitly
chose to use `-l` or `--login` parameter.

And moved addGoRuntimeBaseToPATH to goEnvironmentStatus.ts
because I think this is a better place.

Updates golang#378

Change-Id: I56ada362fb422bca9c789eae30c6239cac5b4711
gopherbot pushed a commit that referenced this issue Jul 28, 2020
… API

vscode.ExtensionContext.EnvironmentVariableCollection is available since
1.46 (May 2020 release https://code.visualstudio.com/updates/v1_46#_environment-variable-collection)
This API is specifically designed to help easy mutation of terminal
environment, so we are trying to use it now.

I hope this helps to address a couple of bugs related to the shell
detection and the correct command and path escaping discovered
during testing on Windows.

Unfortunately, this API does not work well on Mac
microsoft/vscode#99878 due to the peculiarity
described in
https://code.visualstudio.com/docs/editor/integrated-terminal#_why-are-there-duplicate-paths-in-the-terminals-path-environment-variable-andor-why-are-they-reversed

So, we use the new API only for non-darwin OSes for now. On Mac,
we send the command as done before unless users explicitly
chose to use `-l` or `--login` parameter.

And moved addGoRuntimeBaseToPATH to goEnvironmentStatus.ts
because I think this is a better place.

Updates #378

Change-Id: I56ada362fb422bca9c789eae30c6239cac5b4711
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/245127
Reviewed-by: Brayden Cloud <bcloud@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
@hyangah
Copy link
Contributor

hyangah commented Jul 30, 2020

Fixes for this is included in v0.16.0-rc.1
https://github.com/golang/vscode-go/releases/tag/v0.16.0-rc.1

To install the rc version, download the vsix file from the release page,
and from VSCode Command Palette -> 'Extensions: Install from VSIX...', and choose the downloaded vsix file.

@hyangah hyangah closed this as completed Jul 30, 2020
@golang golang locked and limited conversation to collaborators Jul 30, 2021
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

5 participants