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.

Sign up for GitHub

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

Jump to bottom

prototyping pythonfinder pep514 support #6140

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

prototyping pythonfinder pep514 support #6140

Changes from 2 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
9583587
prototyping pythonfinder pep514 support
matteius Apr 24, 2024
9f2b259
winreg is only available on, you guessed it -- windows
matteius Apr 25, 2024
00cc9f4
Merge branch 'main' into pythonfinder-pep514
matteius Apr 27, 2024
ec17820
Merge branch 'main' into pythonfinder-pep514
matteius Apr 27, 2024
9a84b15
Merge branch 'main' into pythonfinder-pep514
matteius Apr 27, 2024
6e13272
Update pipenv/vendor/pythonfinder/models/python.py
matteius Apr 29, 2024
af74dea
Update pipenv/vendor/pythonfinder/models/python.py
matteius May 2, 2024
3b3fb18
Update pipenv/vendor/pythonfinder/models/python.py
matteius May 2, 2024
914c925
Update pipenv/vendor/pythonfinder/models/python.py
matteius May 2, 2024
3123708
Update pipenv/vendor/pythonfinder/models/python.py
matteius May 2, 2024
987eb1b
Consider the three base registry keys.
matteius May 23, 2024
f09f46d
Refactor to follow the PEP 514 specification more closely, handling t…
matteius May 23, 2024
cee98c2
Merge branch 'main' into pythonfinder-pep514
matteius May 29, 2024
1ad4809
Update pipenv/vendor/pythonfinder/models/python.py
matteius May 29, 2024
7739889
Merge branch 'pythonfinder-pep514' of github.com:pypa/pipenv into pyt…
matteius May 29, 2024
1735d1a
Apply PR freedback to continue loop; add WindowsLauncherEntry to inst…
matteius May 29, 2024
d634f1f
Try actually wiring up the windows finder -- its not being invoked.
matteius May 29, 2024
a26d677
check pt progress (working on power shell, but pyenv thing needs work…
matteius May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 71 additions & 41 deletions pipenv/vendor/pythonfinder/models/python.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,50 +318,80 @@ def which(self, name) -> PathEntry | 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"
try:
import winreg
with winreg.OpenKey(winreg.HKEY_LOCAL_MACHINE, key_path) as key:
num_subkeys, _, _ = winreg.QueryInfoKey(key)

for i in range(num_subkeys):
version_key_name = winreg.EnumKey(key, i)

with winreg.OpenKey(key, version_key_name) as version_key:
try:
install_path_key = winreg.OpenKey(version_key, "InstallPath")
try:
executable_path = winreg.QueryValue(install_path_key, "ExecutablePath")
except FileNotFoundError:
# TODO: Only a valid default for PythonCore, otherwise skip.
executable_path = winreg.QueryValue(install_path_key, None)+"\\python.exe"
except FileNotFoundError:
continue
import winreg

registry_keys = [
(winreg.HKEY_CURRENT_USER, r"Software\Python"),
(winreg.HKEY_LOCAL_MACHINE, r"Software\Python"),
(winreg.HKEY_LOCAL_MACHINE, r"Software\Wow6432Node\Python")
]

try:
version = Version(winreg.QueryValue(key, "SysVersion"))
except FileNotFoundError:
# TODO: Only a valid default for PythonCore, otherwise unknown so will need to be probed later.
version = Version(version_key_name)

try:
architecture = winreg.QueryValue(key, "SysArchitecture")
except FileNotFoundError:
# TODO: Implement PEP-514 defaults for architecture for PythonCore based on key and OS architecture.
architecture = None

launcher_entry = WindowsLauncherEntry(
version=version,
executable_path=executable_path,
company="PythonCore",
architecture=architecture,
)
yield launcher_entry
for hive, key_path in registry_keys:
try:
with winreg.OpenKey(hive, key_path) as root_key:
num_companies, _, _ = winreg.QueryInfoKey(root_key)

for i in range(num_companies):
company = winreg.EnumKey(root_key, i)
if company == "PyLauncher":
continue

with winreg.OpenKey(root_key, company) as company_key:
num_tags, _, _ = winreg.QueryInfoKey(company_key)

for j in range(num_tags):
tag = winreg.EnumKey(company_key, j)

with winreg.OpenKey(company_key, tag) as tag_key:
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.)

sys_architecture = self._get_registry_value(tag_key, "SysArchitecture")

if company == "PythonCore" and not sys_architecture:
# 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.


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

executable_path = self._get_registry_value(install_path_key,
"ExecutablePath")
windowed_executable_path = self._get_registry_value(install_path_key,
"WindowedExecutablePath")

if company == "PythonCore" and not executable_path:
executable_path = os.path.join(install_path, "python.exe")
if company == "PythonCore" and not windowed_executable_path:
windowed_executable_path = os.path.join(install_path, "pythonw.exe")

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?

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?

yield launcher_entry

except FileNotFoundError:
continue

except FileNotFoundError:
pass

def _get_registry_value(self, key, value_name, default=None):
try:
import winreg
return winreg.QueryValue(key, value_name)
matteius marked this conversation as resolved.
Show resolved Hide resolved
except FileNotFoundError:
# Python launcher registry key not found, no Python versions registered
return
return default


@dataclasses.dataclass
Expand Down
Loading