-
-
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
prototyping pythonfinder pep514 support #6140
base: main
Are you sure you want to change the base?
Conversation
key_path = r"Software\Python\PythonCore" | ||
try: | ||
import winreg | ||
with winreg.OpenKey(winreg.HKEY_LOCAL_MACHINE, key_path) as key: |
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.
There's two other registry locations we need to apply the same process to, to find user-land installations and 32-bit installs on 64-bit Windows.
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.
Thanks for your help with all this -- greatly appreciated. Do you know what the other paths would be? I can get back to looking at this soon.
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.
Per PEP-0514, the three registry paths that need to be checked are:
HKEY_CURRENT_USER\Software\Python\<Company>\<Tag>
HKEY_LOCAL_MACHINE\Software\Python\<Company>\<Tag>
HKEY_LOCAL_MACHINE\Software\Wow6432Node\Python\<Company>\<Tag>
You can check them all the same way, there's some notes there about what to do if you see the same Company-Tag combination in different registry paths, and specific notes about how to disambiguate 32-bit and 64-bit Python older than 3.5, which did not include architecture in its Tag, and so has the same Company-Tag for 32-bit and 64-bit installs.
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.
Just took a stab at the refactor, not really sure if it's 100% correct though.
@@ -302,6 +317,39 @@ def which(self, name) -> PathEntry | None: | |||
non_empty_match = next(iter(m for m in matches if m is not None), None) | |||
return non_empty_match | |||
|
|||
def find_python_versions_from_windows_launcher(self): | |||
# Open the registry key for Python launcher | |||
key_path = r"Software\Python\PythonCore" |
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.
It'd be nice to support non-PythonCore environments at some point, e.g., conda.
Accept PR suggestion Co-authored-by: Paul "TBBle" Hampson <[email protected]>
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.
Quickly plumbed through the architecture, since we should have that available for Python 3.5 onwards.
Co-authored-by: Paul "TBBle" Hampson <[email protected]>
Co-authored-by: Paul "TBBle" Hampson <[email protected]>
Co-authored-by: Paul "TBBle" Hampson <[email protected]>
Co-authored-by: Paul "TBBle" Hampson <[email protected]>
…he various registry values and fallback rules for PythonCore entries.
|
||
try: | ||
with winreg.OpenKey(tag_key, "InstallPath") as install_path_key: | ||
install_path = self._get_registry_value(install_path_key, None) |
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.
Probably need to continue if install_path
comes back as None
(or rather, as an empty string, I suspect, since this lookup cannot fail if OpenKey
on the line above passed, so we'll never get our default value back), as it's not a valid Python install in this case. (And for example, the PythonCore defaults below will be nonsensically in the root of C:
)
I suppose the except FileNotFoundError
on line 383 was supposed to take care of that, but _get_registry_value
currently handles that exception internally, so I don't see how that exception handler (nor the one on line 386) can actually get hit except for the initial winreg.OpenKey
call.
Maybe self._get_registry_value
needs to be able to know the difference between a usable None
default, and "This key is required, bubble the exception up".
That said, if the expectation is that this code can spit out invalid WindowsLauncherEntry
values that are handled later, e.g., by higher-level code checking that install_path
is a valid and rational path and actually matches the sys.prefix
in the resulting Python environment, then you can ignore this comment. ^_^
launcher_entry = WindowsLauncherEntry( | ||
version=Version(sys_version), | ||
executable_path=executable_path, | ||
windowed_executable_path=windowed_executable_path, | ||
company=company, | ||
tag=tag, | ||
display_name=display_name, | ||
support_url=support_url, | ||
architecture=sys_architecture, | ||
) |
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 doesn't capture install_path
. I'm assuming the usage in PEP-514 (i.e. sys.prefix
for that installation) is what's expected for WindowsLauncherEntry.install_path
in this code, but I don't fully understand the places it's used anyway. Is it just used to for more-efficient lookups in an internal dictionary?
display_name = self._get_registry_value(tag_key, "DisplayName", | ||
default=f"Python {tag}") | ||
support_url = self._get_registry_value(tag_key, "SupportUrl", | ||
default="http://www.python.org/") | ||
version = self._get_registry_value(tag_key, "Version", default=tag[:3]) | ||
sys_version = self._get_registry_value(tag_key, "SysVersion", default=tag[:3]) |
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.
These defaults are only applicable for company == "PythonCore"
. It might make sense to have these all default to None and have a single block which populates all missing values for PythonCore only in one hit. (Or perhaps, just add these to the block that already exists inside the InstallPath
block.)
I'd also suggest annotating the Version and SysVersion defaults to be clear that they are correct per-spec. Casual inspection of the code would note that Python 3.10 and Python 3.11 will both produce default Version and SysVersion of "3.1", which is clearly wrong. (But knowing that the defaults are only needed for Python installs older than Python 3.5 means that this case should never happen, but you can't tell that from the code.)
|
||
launcher_entry = WindowsLauncherEntry( | ||
version=Version(sys_version), | ||
executable_path=executable_path, |
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.
An empty executable_path
(after allowing for PythonCore defaults) means the environment is not executable. Should we skip outputting a WindowsLauncherEntry
in that case? Or is that still useful to expose?
# TODO: Implement PEP-514 defaults for architecture for PythonCore based on key and OS architecture. | ||
sys_architecture = None |
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.
Back-compat logic should be something like:
import platform
if key_path == r"Software\Wow6432Node\Python" or not platform.machine().endswith('64'):
# 32-bit registry tree or 32-bit OS: https://stackoverflow.com/a/12578715/166389
sys_architecture = "32bit"
elif hive == winreg.HKEY_LOCAL_MACHINE:
# 64-bit OS and system-wide install that's not in the 32-bit registry tree
sys_architecture = "64bit"
else:
# User install on a 64-bit machine: Unknown architecture, we'd need to run the executable to find out.
sys_architecture = None
We could probably capture the default value right at the top of the outer loop, since it depends only on hive
and key_path
, but not the contents of company_key
, even though using the value depends on the company
value.
Co-authored-by: Paul "TBBle" Hampson <[email protected]>
…all_path; etc and refactor methods to be more readable.
After spending a few hours on it tonight and trying to test it and seeing differences in behaviors between git bash and powershell, and also it finding my pyenv versions and maybe not even my windows versions -- I feel this still needs a ton of work. It seems nothing actually invoked the logic by default, and so I've spent a bunch more time trying on it, and have a bunch of local changes that don't work as well). |
Prototyping bringing back a level of pep514 support to pythonfinder (would back-vendor it to pythonfinder if successful).
The checklist
news/
directory to describe this fix with the extension.bugfix.rst
,.feature.rst
,.behavior.rst
,.doc.rst
..vendor.rst
. or.trivial.rst
(this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.