microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
163.09k stars 28.81k forks source link

integrated terminal assumes GNU bash is used as user shell, passes --login #123332

Open mirabilos opened 3 years ago

mirabilos commented 3 years ago

Steps to Reproduce:

  1. set the user shell to mksh
  2. try to use the integrated terminal

I’m the developer of mksh, a Unix Korn Shell. I’m proxying this bugreport for an mksh user.

The user receives the error The terminal process "/usr/local/bin/mksh '--login'" failed to launch (exit code: 1). This is because, apparently, vscode forcibly adds the flag --login, which only exists in GNU bash, to shell invocations, which is wrong.

Many shells have flags to indicate to them they are the login shell, but most often this is -l instead, so if you do an execv("/path/to/shell", "/path/to/shell", "-l", NULL); they would work, but that would also be wrong (even if that’d work with both GNU bash and mksh).

The proper way to ask a shell to be a login shell is to prepend a hyphen-minus to its zero-th argument, that is: execv("/path/to/shell", "-shell", NULL); You can also prepend the hyphen-minus to the full path: execv("/path/to/shell", "-/path/to/shell", NULL);

Does this issue occur when all extensions are disabled?: Yes

saltymouse commented 3 years ago

I'm getting this error in VS Code with mksh installed with Homebrew and set as my default shell.

Previous version that worked without issue launching the integrated terminal with mksh as default shell.

Version: 1.55.2
Commit: 3c4e3df9e89829dce27b7b5c24508306b151f30d
Date: 2021-04-13T09:36:32.643Z
Electron: 11.3.0
Chrome: 87.0.4280.141
Node.js: 12.18.3
V8: 8.7.220.31-electron.0
OS: Darwin x64 19.6.0

New version that presents the '--login' error upon launching the integrated terminal with mksh as default shell.

Version: 1.56.0
Commit: cfa2e218100323074ac1948c885448fdf4de2a7f
Date: 2021-05-04T22:06:21.189Z
Electron: 12.0.4
Chrome: 89.0.4389.114
Node.js: 14.16.0
V8: 8.9.255.24-electron.0
OS: Darwin x64 19.6.0
mirabilos commented 3 years ago

Way forward: once https://github.com/microsoft/node-pty/issues/472 is fixed, drop the --login from the terminal profiles and add a new boolean option, maybe loginShell; set that to true in the profiles that should have one, and if it’s set do the hyphen-dash prepending just before calling node-pty.spawn*() depending on how they implement that.

Tyriar commented 3 years ago

Here's the responsible code:

https://github.com/microsoft/vscode/blob/2fc3214ba42050459096e5731513833efd4ac270/src/vs/workbench/contrib/terminal/browser/terminalProfileResolverService.ts#L190-L198

Guessing the repro is installing mksh via homebrew and setting it as the default shell.

These settings should help workaround:

"terminal.integrated.profiles.osx": {
  "mksh": {
    "path": "mksh"
  }
},
"terminal.integrated.defaultProfile.osx": "mksh"
saltymouse commented 3 years ago

Using the above VS Code settings indeed allows the integrated terminal to launch without error with mksh. However, the resulting shell session only has $LOGNAME available while the $USER variable isn't set (causing some issues with some CLI tools depending on $USER).

So, in addition to the above workaround settings, I also run login (and then login to my user account) on each integrated terminal session to gain access to the $USER variable.

mirabilos commented 3 years ago

saltymouse dixit:

However, the resulting shell session only has $LOGNAME available while the $USER variable isn't set (causing some issues with some CLI tools depending on $USER).

Yes, $USER is set by login tools, not by the shell.

So, in addition to the above workaround settings, I also run login (and then login to my user account) on each integrated terminal session to gain access to the $USER variable.

Why is the $USER in the environment of vscode itself not passed down to the terminal? If vscode is run under a user account, the variable should be in its environment.

mattieb commented 1 year ago

I just submitted a PR for node-pty that adds the necessary support to set argv[0]. I'm using it in my own app and can launch Zsh as a login shell by setting the new "argv0" option to "-zsh". https://github.com/microsoft/node-pty/pull/627