-
Notifications
You must be signed in to change notification settings - Fork 985
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
Install managed Python into the Windows Registry (PEP 514) #10634
base: main
Are you sure you want to change the base?
Conversation
It's not terribly pleasant but you can do runs-in-serial integration tests that save+restore (or clear) the relevant keys if they're vaguely easy to enumerate (e.g. if they're all under |
Need to review still, but could we add coverage to one of our integration tests? Like integration test | free-threaded on windows could assert that we can find it with |
/// | ||
/// If windowed is true, `pythonw.exe` is selected over `python.exe` on windows, with no changes | ||
/// on non-windows. | ||
pub fn executable(&self, windowed: bool) -> PathBuf { |
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.
Is adding this bool here just for this one use?
install_path.set_value(
"WindowedExecutablePath",
&Value::from(&HSTRING::from(installation.executable(true).as_os_str())),
)?;
It makes me a little sad to need to pass false
everywhere. Unfortunately the logic here is fairly involved... hm.
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.
I bailed on rewriting the entire logic here, not sure either.
Looks good to me! I defer to @Gankra on approval since she's more familiar with Windows. |
Just wondering, what's the motivation for
|
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.
Looks good. Biggest concern is the lack of special handling for the "official python install" special cases described in the PEP.
/// Link the binaries of a managed Python installation to the bin directory. | ||
#[allow(clippy::fn_params_excessive_bools)] | ||
fn create_bin_links( |
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.
i'm going to assume this is a direct cut-paste and not look at it unless you tell me otherwise
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.
yes the first two commits are just moving code around
if preview.is_enabled() { | ||
uv_python::windows_registry::create_registry_entry(installation, &mut errors)?; |
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.
(i wonder if it would be better to pass in preview and have these functions decide whether they're enabled... nbd tho)
/// Find all Pythons registered in the Windows registry following PEP 514. | ||
pub(crate) fn registry_pythons() -> Result<Vec<WindowsPython>, windows_result::Error> { | ||
let mut registry_pythons = Vec::new(); | ||
for root_key in [CURRENT_USER, LOCAL_MACHINE] { |
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.
Oh I just realized, it's not obvious to me that the current approach actually applies the "CURRENT_USER should shadow LOCAL_MACHINE if they overlap" rule? It seems like the logic here throws out that information completely. (I don't think this has to be a blocker, but it should at least be noted that this is "future work / a looming issue")?
Tools that list every installed environment may choose to include those even where the Company-Tag pairs match. They should ensure users can easily identify whether the registration was per-user or per-machine, and which registration has the higher priority.
Tools that aim to select a single installed environment from all registered environments based on the Company-Tag pair, such as the py.exe launcher, should always select the environment registered in HKEY_CURRENT_USER when than the matching one in HKEY_LOCAL_MACHINE.
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.
The way its written sounds like its targeted at a different case than ours: When a user says py -V:Astral/CPython3.13.1
, it should use HKEY_CURRENT_USER over HKEY_LOCAL_MACHINE if both have Astral\CPython3.13.1
. In uv's discovery, we only support asking of an interpreter kind and a version or version range, and we'll return the first Python we find matching. If a user has the case that they have multiple installations of the same version and they have a specific preference, they can use an absolute path (this should be an edge case). By the order in the list we should still prefer HKEY_CURRENT_USER over HKEY_LOCAL_MACHINE if the version is the same, i added a comment about it.
8bbc96e
to
e4833a9
Compare
Summary
In preview mode on windows, register und un-register the managed python build standalone installations in the Windows registry following PEP 514.
We write the values defined in the PEP plus the download URL and hash. We add an entry when installing a version, remove an entry when uninstalling and removing all values when uninstalling with
--all
. We update entries only by overwriting existing values, there is no "syncing" involved.Since they are not official builds, pbs gets a prefix.
py -V:Astral/CPython3.13.1
works,py -3.13
doesn't.Registry errors are reported but not fatal, except for operations on the company key since it's not bound to any specific python interpreter.
The code uses the official
windows_registry
crate of thewinreg
crate.Best reviewed commit-by-commit.
Test Plan
I manually tested:
py --list-paths
--all
removes the entry and unrelated entries from the registrySince the registry is global, we can't isolate this into integration tests.