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

If terminal.integrated.shell doesn't have a matching terminal.integrated.profiles entry the shell path will be ' ' #121760

Closed
kimadeline opened this issue Apr 20, 2021 · 17 comments · Fixed by #123729
Assignees
Labels
api bug Issue identified by VS Code Team member as probable bug regression Something that used to work is now broken terminal Integrated terminal issues terminal-profiles verified Verification succeeded
Milestone

Comments

@kimadeline
Copy link

  • VS Code Version: 1.55.1
  • OS Version: Windows 10 20H2 build 19042.867

Hello 👋 The Python extension uses the content of the terminal.integrated.shell.<os> setting to determine which shell is currently used and which virtual environment activation command should be sent.

However, as reported in microsoft/vscode-python#15919 (comment), if the path specified in terminal.integrated.shell.<os> does not have a corresponding terminal.integrated.profiles.<os> entry, the value returned by terminal.integrated.shell.<os> will be '', and terminal type detection will fail.

Steps to Reproduce:

  1. Install Powershell 7 in a non-standard path (for example, outside of the system drive)
  2. Add the path to the newly installed shell to terminal.integrated.shell.windows
  3. Retrieve the value of terminal.integrated.shell.windows using workspace.getConfiguration()

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

@meganrogge meganrogge added regression Something that used to work is now broken terminal Integrated terminal issues labels Apr 22, 2021
@Tyriar
Copy link
Member

Tyriar commented Apr 22, 2021

shell/profiles have changed a lot in the upcoming version, but the python extension should not be doing this (definitely in 1.56) as there's a env.shell extension API for this purpose which should work. In fact that extension was added specifically for the Python extension so I'm a bit confused microsoft/vscode-python#6050

@Tyriar Tyriar added the info-needed Issue requires more information from poster label Apr 22, 2021
@kimadeline
Copy link
Author

kimadeline commented Apr 22, 2021

Oh sorry I linked to the wrong file! We do use the env.shell API you mention (in VSCEnvironmentShellDetector), but for some reason the shell path being detected is C:\Program Files\Git\bin\bash.exe and not F:\\Program Files\\PowerShell\\7\\pwsh.exe:

Info 2021-04-15 16:39:05: Shell path ''
Info 2021-04-15 16:39:05: Shell path identified as shell 'other'
Info 2021-04-15 16:39:05: Terminal name '' identified as shell 'other'
Info 2021-04-15 16:39:05: [object Object]. Shell identified as other (Terminal name is )
Info 2021-04-15 16:39:05: Shell path 'C:\Program Files\Git\bin\bash.exe'
Info 2021-04-15 16:39:05: Shell path identified as shell 'bash'
Info 2021-04-15 16:39:05: Terminal shell path 'C:\Program Files\Git\bin\bash.exe' identified as shell 'bash'
Info 2021-04-15 16:39:05: [object Object]. Shell identified as bash (Terminal name is )
Info 2021-04-15 16:39:05: Shell identified as 'bash'

Let me close it and dig a little bit further on the extension side, especially since shells and profiles are being revamped 🙂 Thank you!

Edit: reopening because the shell returned by vscode.env.shell is bash and not pwsh.

@Tyriar
Copy link
Member

Tyriar commented Apr 23, 2021

I don't totally understand how profiles would interact with shell in stable but we've made some major changes in this area and introduced a new defaultProfile setting in Insiders. I'll double check that all plays nice with env.shell.

@meganrogge
Copy link
Contributor

Can you try this on Insider's? I believe this issue has been fixed.

@meganrogge meganrogge added info-needed Issue requires more information from poster and removed info-needed Issue requires more information from poster labels Apr 29, 2021
@meganrogge
Copy link
Contributor

Closing because I think this has been resolved

recording - 2021-04-29T112154 362

@meganrogge
Copy link
Contributor

to verify:

using the extension terminal sample, log the values of env.shell and getConfiguration().get('terminal.integrated.shell.windows')

these should be non-null and reflect what you've set terminal.integrated.shell.windows to, regardless of whether or not that path is found in terminal.integrated.profiles.windows.

@Tyriar
Copy link
Member

Tyriar commented May 10, 2021

I suspect this isn't fixed

@Tyriar Tyriar reopened this May 10, 2021
@Tyriar Tyriar added the bug Issue identified by VS Code Team member as probable bug label May 10, 2021
@Tyriar Tyriar modified the milestones: April 2021, May 2021 May 10, 2021
@karrtikr
Copy link
Contributor

karrtikr commented May 12, 2021

@Tyriar vscode.env.shell returns the value of terminal.integrated.shell.windows instead of using terminal.integrated.defaultProfile.windows in combination of terminal.integrated.profiles.windows to get the shell path, is this expected? This is causing microsoft/vscode-python#16175 for a lot of our users.

@karrtikr
Copy link
Contributor

karrtikr commented May 12, 2021

Following up regarding microsoft/vscode-python#16175 (comment),

  • This also means that such users won't be able to change their default shell using the Terminal: Select default profile command, which can be confusing.
  • Currently the API vscode.env.shell doesn't use terminal.integrated.defaultProfile even if the shell setting is not specified, and returns the default value instead. Does that also change with Switch priority of shell/shellArgs and defaultProfile setting #123174? I expect it to fallback on the profile setting instead.
    Update: I can verify on latest insiders that the API doesn't use the profile setting.

@Tyriar
Copy link
Member

Tyriar commented May 12, 2021

@karrtikr you're right, #123174 won't fix that because it's actually quite a big change to make that happen which I've been working on most of today, I believe this is too risky to try to get into the recovery build as it involves moving a bunch of stuff around to live under different processes. It's unfortunate that users may get confused and I'm not happy at all with the issues that came with default profiles.

I suggest once the recovery build ships (tomorrow?), in the Python extension check if the user has set defaultProfile but not shell or shellArgs and warn them that the default profile won't be used in this case. Since #123174 will be in the recovery build, that should catch all the bad cases

@karrtikr
Copy link
Contributor

karrtikr commented May 14, 2021

@Tyriar Thanks for the quick fix, although I'm still having some issues with the vscode.env.shell API when trying the latest insiders build. Steps to repro on windows:

  1. Make sure settings related to shell and defaultProfile are clean.
  2. Observe API detects the default shell type correctly.
  3. Change default profile to Command prompt and quickly open a new terminal which triggers an event.
  4. API still detects the shell as powershell so activation command fails.
  5. Open another terminal .
  6. API is now detecting the shell type correctly.

It seems it takes some time for the terminal change to be propagated to to the API. If a wait a bit before opening the terminal, the problem does not happen.

@aravindvnair99
Copy link

@karrtikr Same here as well. Sometimes on reopening VS Code, the settings seem to vanish as well.

@meganrogge
Copy link
Contributor

@connor4312 is this verification-found?

@connor4312
Copy link
Member

I did not try to verify, but after reading the thread reopened from @karrtikr's report of continued problems.

@Tyriar
Copy link
Member

Tyriar commented Jun 7, 2021

@connor4312 it takes up to 2 seconds after changing a profile to apply the changes by design.

@ejizba
Copy link

ejizba commented Jun 16, 2021

it takes up to 2 seconds after changing a profile to apply the changes by design.

This is true for changes to "terminal.integrated.defaultProfile.windows", but no matter how long I seem to wait, env.shell is not updated after changes to "terminal.integrated.shell.windows". The only way I see it updated is if I reload the window

Version: 1.58.0-insider (user setup)
Commit: 3e0c442
Date: 2021-06-16T05:17:51.956Z
Electron: 12.0.11
Chrome: 89.0.4389.128
Node.js: 14.16.0
V8: 8.9.255.25-electron.0
OS: Windows_NT x64 10.0.19043

@Tyriar
Copy link
Member

Tyriar commented Jul 6, 2021

@ejizba created #128079 for that, thanks

@github-actions github-actions bot locked and limited conversation to collaborators Jul 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api bug Issue identified by VS Code Team member as probable bug regression Something that used to work is now broken terminal Integrated terminal issues terminal-profiles verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants
@Tyriar @connor4312 @ejizba @karrtikr @aravindvnair99 @meganrogge @kimadeline and others