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

Upgrade Click-Completion and apply Shellingham for shell detection #2363

Merged
merged 4 commits into from Jun 15, 2018

Conversation

Projects
3 participants
@uranusjr
Member

uranusjr commented Jun 15, 2018

Fix #2352. I want to do this without manually patching Click-Completion if possible.

https://github.com/pypa/pipenv/projects/3

@uranusjr uranusjr added this to In progress in Clean up shell detection via automation Jun 15, 2018

uranusjr added some commits Jun 15, 2018

Purge "import *"
So I can rely on the linter to tell me what to fix.
Upgrade Click and make use of shell detection
This introduces an additional environment variable "PIPENV_SHELL". If
set, shell detection is skipped completely. The old "SHELL"
introspection is kept as a fallback if detection fails.

@uranusjr uranusjr changed the title from [WIP] Upgrade Click-Completion and apply Shellingham for shell detection to Upgrade Click-Completion and apply Shellingham for shell detection Jun 15, 2018

@uranusjr

This comment has been minimized.

Member

uranusjr commented Jun 15, 2018

This introduces an additional environment variable PIPENV_SHELL. If set, shell detection is skipped completely. The old SHELL introspection is kept as a fallback if detection fails.

A run down of how shell detection works now:

  1. If PIPENV_SHELL is set, use it.
  2. Detect surrounding shell…
    1. (Only on POSIX) The surrounding shell is the login shell. Use the value of SHELL.
    2. Use the detected shell executable.
  3. Detection failed. Check if SHELL is set; if so, use it.
  4. Prompt the user to set PIPENV_SHELL explicitly.
@techalchemy

This comment has been minimized.

Member

techalchemy commented Jun 15, 2018

looks good to me

@techalchemy techalchemy merged commit e326ce6 into master Jun 15, 2018

2 checks passed

buildkite/pipenv Build #416 passed (7 minutes, 24 seconds)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

Clean up shell detection automation moved this from In progress to Done Jun 15, 2018

@frostming

This comment has been minimized.

Collaborator

frostming commented Jun 19, 2018

Does this PR work under CPython only?(see the 'ctypes' imports in Shellingham)
I guess the classifier should be removed from setup.py. In fact PyPy is not tested at all.

@techalchemy

This comment has been minimized.

Member

techalchemy commented Jun 19, 2018

We rely heavily on ctypes on window and have done so for some time. We use this as a means of shell detection for spawning subshells. This isn’t new, although I’m guessing the complete lack of issues on the topic is a good indicator that things are ok

@uranusjr

This comment has been minimized.

Member

uranusjr commented Jun 19, 2018

ctypes does exist on PyPy. It is a wrapper of CFFI, and is slow, but works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment