-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Call virtualenv by module #2198
Conversation
202182a
to
4fee8ec
Compare
4fee8ec
to
55d77b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a regression, but actually won't it just always
@@ -887,7 +887,7 @@ def do_create_virtualenv(python=None, site_packages=False): | |||
# The user wants the virtualenv in the project. | |||
if project.is_venv_in_project(): | |||
cmd = [ | |||
'virtualenv', | |||
sys.executable, '-m', 'virtualenv', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to use which('virtualenv')
and fallback to sys.executable
in case we can't find virtualenv or something, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what the merit is. Is it so the user can set virtualenv
to, like, a “better virtualenv”? Using our own dependency (even if not vendored) feels a much safer, predictable solution to me. Also in practice virtualenv hasn’t been updated in almost two years (last in November 2016), so the extra which
check is more than likely just wasted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i thought about it more and this does guarantee that we get the one that got installed with pipenv, so I like this
I think this is actually a regression?