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

prototyping pythonfinder pep514 support #6140

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft

Conversation

matteius
Copy link
Member

Prototyping bringing back a level of pep514 support to pythonfinder (would back-vendor it to pythonfinder if successful).

The checklist

  • Associated issue
  • A news fragment in the 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 #.

pipenv/vendor/pythonfinder/models/python.py Outdated Show resolved Hide resolved
key_path = r"Software\Python\PythonCore"
try:
import winreg
with winreg.OpenKey(winreg.HKEY_LOCAL_MACHINE, key_path) as key:
Copy link

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.

Copy link
Member Author

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.

Copy link

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.

Copy link
Member Author

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"
Copy link

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]>
Copy link

@TBBle TBBle left a 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.

pipenv/vendor/pythonfinder/models/python.py Outdated Show resolved Hide resolved
pipenv/vendor/pythonfinder/models/python.py Outdated Show resolved Hide resolved
pipenv/vendor/pythonfinder/models/python.py Outdated Show resolved Hide resolved
pipenv/vendor/pythonfinder/models/python.py Outdated Show resolved Hide resolved

try:
with winreg.OpenKey(tag_key, "InstallPath") as install_path_key:
install_path = self._get_registry_value(install_path_key, None)
Copy link

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. ^_^

Comment on lines 371 to 380
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,
)
Copy link

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?

Comment on lines 346 to 351
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])
Copy link

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,
Copy link

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?

Comment on lines 355 to 356
# TODO: Implement PEP-514 defaults for architecture for PythonCore based on key and OS architecture.
sys_architecture = None
Copy link

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.

pipenv/vendor/pythonfinder/models/python.py Outdated Show resolved Hide resolved
@matteius
Copy link
Member Author

matteius commented May 29, 2024

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).

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

Successfully merging this pull request may close these issues.

2 participants