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 registry Python discovery (PEP 514) not working for "cpython" specs for distributions by unknown orgs #2491

Closed
faph opened this issue Jan 30, 2023 · 10 comments

Comments

@faph
Copy link
Contributor

faph commented Jan 30, 2023

Issue

On Windows:

$ python --version
Python 3.11.1
$ python -m virtualenv --version
virtualenv 20.17.1 from C:\<some path>\tools\python3.11\latest\Lib\site-packages\virtualenv\__init__.py
$ python -m virtualenv .venv -p cpython3.10 -vv

Output:

180 setup logging to DEBUG [DEBUG report:35]
195 find interpreter for spec PythonSpec(implementation=cpython, major=3, minor=10) [INFO builtin:56]
195 proposed PythonInfo(spec=CPython3.11.1.final.0-64, exe=C:\<some path>\tools\python3.11\latest\python.exe, platform=win32, version='3.11.1 (tags/v3.11.1:a7a450f, Dec
6 2022, 19:58:39) [MSC v.1934 64 bit (AMD64)]', encoding_fs_io=utf-8-utf-8) [INFO builtin:63]
202 discover PATH[0]=C:\<some path>\tools\zip\latest\bin\ [DEBUG builtin:108]
205 filesystem is not case-sensitive [DEBUG info:24]
206 discover PATH[1]=C:\<some path>\tools\yq\latest [DEBUG builtin:108]
...
249 RuntimeError: failed to find interpreter for Builtin discover of python_spec='cpython3.10' [ERROR __main__:61]

Seems this is due to logic here: https://github.com/pypa/virtualenv/blob/main/src/virtualenv/discovery/windows/__init__.py#L23

It lists PythonCore and ContinuumAnalytics as the 2 organizations mapping to the CPython implementation. That logic is unfortunate, since many more organizations can decide to distribute CPython. For example, my firm uses a different organization name in the Windows registry, compliant with PEP 514 https://peps.python.org/pep-0514/.

I guess the solution should be similar to how the implementation is looked up for PATH OS environment variable entries: execute the Python binary and inspect the "pyinfo" output.

@faph faph added the bug label Jan 30, 2023
@faph
Copy link
Contributor Author

faph commented Jan 30, 2023

For the avoidance of doubt, none of my Python interpreters are available on PATH.

If I do add the relevant interpreters to PATH, then the discover works as expected.

Also, if I change the Windows Registry keys, and use the PythonCore organization name, it also works.

@faph
Copy link
Contributor Author

faph commented Jan 30, 2023

See also #1796 which introduced the organization name to implementation mapping.

@faph
Copy link
Contributor Author

faph commented Jan 30, 2023

An alternative solution could be to accept additional custom keys. PEP 514 gives the following example:

HKEY_CURRENT_USER\Software\Python\PythonCore\3.6
    (Default) = (value not set)
    DisplayName = "Python 3.6 (64-bit)"
    SupportUrl = "http://www.python.org/"
    Version = "3.6.0"
    SysVersion = "3.6"
    SysArchitecture = "64bit"

Which could be extended for example using SysImplementation = "CPython".

Downside is that such a key would be by convention and it may be undesirable to expect Python distributors to set such a key.

The advantage would be speed since it would no longer require the interpreter to be executed.

@gaborbernat
Copy link
Contributor

PR welcome.

@faph
Copy link
Contributor Author

faph commented Feb 7, 2023

Sure. As an initial improvement, would the “SysImplementation“ solution be ok, @gaborbernat ?

@gaborbernat
Copy link
Contributor

gaborbernat commented Feb 7, 2023

Sure. But note this must not be a breaking change, and what's there should still work as is today.

@faph
Copy link
Contributor Author

faph commented Feb 14, 2023

@gaborbernat I have started to experiment with a solution to remove the Windows Registry-based pre-filtering. Let me know if that's a direction you're willing to consider.

@faph
Copy link
Contributor Author

faph commented Mar 1, 2023

Thank you for your help on this @gaborbernat

@faph faph closed this as completed Mar 1, 2023
@gaborbernat
Copy link
Contributor

Thanks for the PR.

@MeCorey
Copy link

MeCorey commented Mar 4, 2023

Sure. As an initial improvement, would the “SysImplementation“ solution be ok, @gaborbernat ?

hmm...possibility

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

No branches or pull requests

3 participants