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

Rewrite most of the shell detection stuff (do not merge) #2161

Closed
wants to merge 7 commits into from

Conversation

@uranusjr
Copy link
Member

@uranusjr uranusjr commented May 8, 2018

The goal is to fix #915 (comment)

@uranusjr uranusjr self-assigned this May 8, 2018
@uranusjr
Copy link
Member Author

@uranusjr uranusjr commented May 8, 2018

Manually tested on cmd.exe, Powershell (pwsh.exe, but powershell.exe should work), and Bash in WSL. This needs more testers.

@uranusjr
Copy link
Member Author

@uranusjr uranusjr commented May 8, 2018

Some explaination.

I did some process-digging and it turns out there is not a trace of Cmder.exe anywhere in the process tree when _win_utils.get_shell() is called, so I removed it from SHELL_NAMES. It is not useful anyway, since we really need to know not only we’re running Cmder, but also what shell it is spawning for us.

There are quite a few variants of subshell-spawning, each requiring a different call:

  • Non-bash shells on non-Windows (simplest to handle).
  • Bash (not including Msys Bash, which never worked).
  • Bare cmd or Powershell (including pwsh)
  • cmd in Cmder
  • Powershell in Cmder

The logic gets complicated pretty quickly, so I decided to refactor them into five classes (plus an intermediate for the Cmder cases). _detect_shell now returns an instance of Shell (or its subclass) for shell to use.

@uranusjr uranusjr removed the DO NOT MERGE label May 8, 2018
@uranusjr uranusjr changed the title [WIP] Rewrite most of the shell detection stuff Rewrite most of the shell detection stuff May 8, 2018
shell_cmd += ['/k', cmderrc_path]
if cwd:
os.environ['CMDER_START'] = cwd
fork_shell(env, shell_cmd, cwd)

def _detect_shell():

This comment has been minimized.

@techalchemy

techalchemy May 9, 2018
Member

The decision tree in this function is kinda messy, wonder if we can clean this up any

This comment has been minimized.

@uranusjr

uranusjr May 9, 2018
Author Member

Rewrote this a bit. Not sure if this makes more sense.

@techalchemy
Copy link
Member

@techalchemy techalchemy commented May 9, 2018

Overall these changes make sense. I think we are maintaining 2 lists of windows compatible shells, we should consolidate those. Otherwise I think we can schedule this for 11.11

@uranusjr
Copy link
Member Author

@uranusjr uranusjr commented May 9, 2018

TODO:

  • Make sure this does not overwrite #2168.
  • Generate patch.
@hstefan
Copy link

@hstefan hstefan commented May 10, 2018

hstefan and others added 5 commits May 10, 2018
Improving compatibility for bash in Windows
@kennethreitz42 kennethreitz42 changed the title Rewrite most of the shell detection stuff Rewrite most of the shell detection stuff (do not merge) May 23, 2018
@uranusjr uranusjr closed this Jun 7, 2018
@uranusjr uranusjr mentioned this pull request Jun 7, 2018
@uranusjr uranusjr deleted the pew-detect-cmder-powershell branch Jul 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.