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

Detect shells ending in .exe (for Windows/Cygwin) #17431

Merged
merged 3 commits into from
Sep 18, 2021

Conversation

edemaine
Copy link

@edemaine edemaine commented Sep 15, 2021

Strip .exe extension before detecting shells, so all detections work in Windows / Cygwin. Fixes #17426 (specifically subissue 1).

Side note: I feel like the regular expressions should start with \b or something to ensure the shell names aren't suffixes. Currently, notzsh would be detected as zsh. But I haven't changed that; as far as I know, no shell name is a prefix of another, so it's probably fine as is. Well... many shells are a prefix of sh, but that's not a recognized shell (should it be?).

Strip `.exe` extension before detecting shells, so all detections work
in Windows / Cygwin.  Fixes part of microsoft#17426.
@ghost
Copy link

ghost commented Sep 15, 2021

CLA assistant check
All CLA requirements met.

@karthiknadig
Copy link
Member

@edemaine thanks for the PR. Can you add a news item so that you get credits for this change in our changelogs?
Create 17426.md under news/2 Fixes, with content like:

Improve pattern matching for shell detection.
(thanks [Erik Demaine](https://github.com/edemaine/))

@edemaine
Copy link
Author

Added. Thanks for your help!

Copy link

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Can you please open up a different issue number for part 2 and close 17426 as part of this PR? Looking at #17426 right now it seems like there's 2 issues to fix.

Copy link

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please update the tests here to include zsh shells ending in .exe:

shellPathsAndIdentification.set('c:\\windows\\system32\\cmd.exe', TerminalShellType.commandPrompt);
shellPathsAndIdentification.set('c:\\windows\\system32\\bash.exe', TerminalShellType.bash);
shellPathsAndIdentification.set('c:\\windows\\system32\\wsl.exe', TerminalShellType.wsl);
shellPathsAndIdentification.set('c:\\windows\\system32\\gitbash.exe', TerminalShellType.gitbash);
shellPathsAndIdentification.set('/usr/bin/bash', TerminalShellType.bash);
shellPathsAndIdentification.set('/usr/bin/zsh', TerminalShellType.zsh);
shellPathsAndIdentification.set('/usr/bin/ksh', TerminalShellType.ksh);
shellPathsAndIdentification.set('c:\\windows\\system32\\powershell.exe', TerminalShellType.powershell);
shellPathsAndIdentification.set('c:\\windows\\system32\\pwsh.exe', TerminalShellType.powershellCore);
shellPathsAndIdentification.set('/usr/microsoft/xxx/powershell/powershell', TerminalShellType.powershell);
shellPathsAndIdentification.set('/usr/microsoft/xxx/powershell/pwsh', TerminalShellType.powershellCore);
shellPathsAndIdentification.set('/usr/bin/fish', TerminalShellType.fish);
shellPathsAndIdentification.set('c:\\windows\\system32\\shell.exe', TerminalShellType.other);
shellPathsAndIdentification.set('/usr/bin/shell', TerminalShellType.other);
shellPathsAndIdentification.set('/usr/bin/csh', TerminalShellType.cshell);
shellPathsAndIdentification.set('/usr/bin/tcsh', TerminalShellType.tcshell);
shellPathsAndIdentification.set('/usr/bin/xonsh', TerminalShellType.xonsh);
shellPathsAndIdentification.set('/usr/bin/xonshx', TerminalShellType.other);

@edemaine
Copy link
Author

Sorry for the delay. I pushed tests. (Thanks for the pointer!)

I'm OK with #17426 being closed by this PR; in particular, it fixes the title of that issue, and solves my problem. And now that I understand how shell detection works, the rest of the behavior makes some sense.

If I were to create a new issue, it would be one of the following feature requests:

  1. Upon failing to detect the shell type, override the shell setting to a standard built-in shell for the current architecture (instead of running the user's shell, assuming standard shell path exists) where we know how to execute commands.
  2. Add an extension-specific configuration option that allows overriding the detected shell type (useful especially for undetected shells).
  3. Use the automation shell instead of the integrated terminal profile.

But I'm not particularly sure whether any of these are the right decision / important. If you find any of them appealing, let me know, and I'll add them; otherwise, I'm happy to let #17426 end.

@karrtikr
Copy link

I have described the actual problem here thanks to your diagnosis: #17426 (comment).

We should simply rely on the shell path returned by VSCode API, so changing the terminal using any of your suggestions won't work unfortunately. All fallbacks should be removed with #16023 which should solve the issue.

@karrtikr karrtikr merged commit 502cf21 into microsoft:main Sep 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shell doesn't support Cygwin zsh
3 participants