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 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 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 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():
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@techalchemy
Copy link
Member

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

@techalchemy techalchemy added Type: Enhancement 💡 This is a feature or enhancement request. Category: Future Issue is planned for the future. OS: Windows This issue affects the Windows Operating System. Type: Behavior Change This issue describes a behavior change. Type: Vendored Dependencies This issue affects vendored dependencies within pipenv. labels May 9, 2018
@techalchemy techalchemy added this to the 11.11.0 milestone May 9, 2018
@uranusjr
Copy link
Member Author

uranusjr commented May 9, 2018

TODO:

@hstefan
Copy link

hstefan commented May 10, 2018

Comment on #915

@kennethreitz kennethreitz 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 July 4, 2018 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Future Issue is planned for the future. OS: Windows This issue affects the Windows Operating System. Type: Behavior Change This issue describes a behavior change. Type: Enhancement 💡 This is a feature or enhancement request. Type: Vendored Dependencies This issue affects vendored dependencies within pipenv.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pipenv shell starts a CMD shell when run from powershell
3 participants