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

Shell information no longer returned in settings #74233

Closed
DonJayamanne opened this issue May 23, 2019 · 10 comments
Closed

Shell information no longer returned in settings #74233

DonJayamanne opened this issue May 23, 2019 · 10 comments
Assignees
Labels
terminal Integrated terminal issues under-discussion Issue is under discussion for relevance, priority, approach
Milestone

Comments

@DonJayamanne
Copy link
Contributor

  • VSCode Version: Version 1.35.0-insider (1.35.0-insider) 2019-05-23T05:07:54.582Z
  • OS Version: Mac

Steps to Reproduce:

  1. Use extension code to get the path to the shell, from the setting terminal.integrated.shell.<platform>, and you'll see the return value is null

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

Note: This works in Stable version of VS Code.

  • Operating System: Mac
  • Works in Stable
  • Does not work in Insiders

Version 1.34.0 (1.34.0)
Screen Shot 2019-05-23 at 14 28 47

Version 1.35.0-insider (1.35.0-insider) 2019-05-23T05:07:54.582Z
Screen Shot 2019-05-23 at 14 29 16

@DonJayamanne
Copy link
Contributor Author

@Tyriar Hope this will be fixed in the current release, as this will break the Python extension in a number of places.

@Tyriar Tyriar added this to the May 2019 milestone May 30, 2019
@Tyriar Tyriar added the under-discussion Issue is under discussion for relevance, priority, approach label May 30, 2019
@Tyriar
Copy link
Member

Tyriar commented May 30, 2019

This change was made due to this issue that came out of remote that was reported quite a lot: microsoft/vscode-remote-release#38

The problem here is that evaluating the setting locally meant that was passed over to the remote side and it would break as a result. Consider an Ubuntu box where $SHELL=/bin/zsh (terminal.integrated.shell.linux is derived from this) using Remote - Containers on a docker container, the terminal would exit immediately as zsh isn't installed.

While thinking about whether I can revert back and still fix the remote bug I'm not so sure I can, the reason being that then extensions like vscode-python would fail for the same reason in the remote case. I think the right thing to do here is for the python extension to have its own default shell behavior, whether that be copying how VS Code does it or defaulting to cmd/$SHELL it's up to you, I don't think it's super important for it to match exactly if the default is being used.

@DonJayamanne
Copy link
Contributor Author

DonJayamanne commented May 30, 2019

We're happy to go ahead and use the code u suggested, the one used in VS Code.

However, we'll most certainly need some kind of API from VS Code to get this value going forward. The reason is simple, if VS Code was to make some changes to this area (changes in logic), then our code could break.

This is because we need to know the shell being used by VS Code so we can send shell specific commands. If VS Code is using bash and we end up assuming that vscode is using zsh, then things break.

You could probably change the label of this issue to reflect the new state - i.e. Extension requiring a new API...

@Tyriar
Copy link
Member

Tyriar commented May 30, 2019

@DonJayamanne so you interact with terminals other than ones you create via createTerminal?

@DonJayamanne
Copy link
Contributor Author

so you interact with terminals other than ones you create via createTerminal

Yes (we interact with terminal user have created).
We assume the terminal they have opened is using the default shell.

@Tyriar
Copy link
Member

Tyriar commented May 30, 2019

@DonJayamanne you could try using Terminal.name for the terminal you're sending it to instead? This will be the name of what's running within the terminal with the directory removed (and the extension cut off for Windows). This might even be better as you will be able to detect sub-shells, for example:

Windows running cmd.exe: cmd
Windows running powershell: powershell
Windows running powershell core pwsh
Linux running bash bash
Linux running git inside bash git
Linux running python inside bash python

This is what shows up in the terminal selector dropdown.

@DonJayamanne
Copy link
Contributor Author

Yes, that would work, however we won't be able to detect the shell when a terminal is opened by vsc for debugging or similar (e.g. when VSC opens a terminal for debugging the name is Python Debug Console or similar).

@Tyriar
Copy link
Member

Tyriar commented May 31, 2019

@DonJayamanne for that case it doesn't matter much imo if you end up differing from VS Code, can't you just have a default that you use?

@Tyriar
Copy link
Member

Tyriar commented May 31, 2019

Closing as I think we agreed to fix this in vscode-python

@Tyriar
Copy link
Member

Tyriar commented Jun 7, 2019

For any extensions hitting this issue, the recommendation is to set your own default when the setting is null. The simplest fix would be to fallback to cmd.exe on Windows and bash on Linux/macOS.

@vscodebot vscodebot bot locked and limited conversation to collaborators Jul 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
terminal Integrated terminal issues under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

2 participants