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

Windows virtualenv -p python2.7 fails even when python2.7.exe exists #1062

Closed
asottile opened this issue Jul 9, 2017 · 8 comments
Closed

Comments

@asottile
Copy link
Contributor

asottile commented Jul 9, 2017

The problem stems from this function which doesn't honor the windows-specific PATHEXT.

Here's a nearly-identical implementation of that function which does handle PATHEXT expansion: https://github.com/pre-commit/pre-commit/blob/e2bae300fe2794e2ece25d4cd72127238704bb1e/pre_commit/parse_shebang.py#L21-L42

distutils.spawn.find_executable has a similar special case, but only attempts .exe: https://github.com/python/cpython/blob/9648088e6ccd6d0cc04f450f55628fd8eda3784c/Lib/distutils/spawn.py#L181-L182

@pfmoore
Copy link
Member

pfmoore commented Aug 19, 2017

virtualenv -p "2.7" finds Python 2.7 direct from the registry entries, the same way as py -2.7 works, so there should be no need for this.

@asottile
Copy link
Contributor Author

This is necessary when using conda

@asottile
Copy link
Contributor Author

asottile commented Aug 19, 2017

It also makes cross platform scripts which call virtualenv -ppython2.7 just work.

Also the suggested patch reduces complexity by leveraging a stdlib function instead of a home grown solution

@pfmoore
Copy link
Member

pfmoore commented Aug 19, 2017

OK. I'm not sure how much I personally care about conda (I thought the idea was that if using conda you'd use conda to create environments, not virtualenv) but if that's a reason for needing it that's fine (it's not as if it's a massive patch).

It also makes cross platform scripts which call virtualenv -ppython2.7 just work.

It doesn't, of course, because python2.7 isn't an executable on Windows.

Also the suggested patch reduces complexity by leveraging a stdlib function instead of a home grown solution

What home-grown solution do you mean? If you mean -p "2.7" then I'm a strong -1 on removing that as I use it all the time. If you just mean adding an alternative, then fine, but that doesn't "reduce" complexity (it doesn't increase it by much, though ;-)).

@asottile
Copy link
Contributor Author

If you read the patch it should be fairly obvious what I'm referring to :) (I'm not talking about the registry bit at all)

@pfmoore
Copy link
Member

pfmoore commented Aug 19, 2017

Oh, sorry - I did read the patch, but hadn't linked what I'd read to what you were saying - my mistake, it's been a long day :-)

So yes, functionally the only difference is then that the new code will

  1. Check the current directory as well as PATH
  2. Add a .exe to a file that doesn't end in .exe

I'm not sure if the different behaviour over the current directory is a big deal, and I doubt anyone is likely to use anything like a .bat file (-p mypython.bat) so agreed using a standard function seems reasonable.

@asottile
Copy link
Contributor Author

1. is already being done anyway (as the code checks afterwards here) so I think 2. is the only real difference

And I agree, the only extension that really should be cared about in this case is .exe anyway.

@asottile
Copy link
Contributor Author

@pfmoore is the patch mergeable then?

@pypa pypa locked and limited conversation to collaborators Jan 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants