Fix Python variant tagging in the Windows registry#19012
Conversation
e919f2d to
4b848ea
Compare
|
Another thing to note: I'm not sure how this interacts with existing installations -- this fixes the "from scratch" pathway, but there are probably users out there already who have both GIL and freethreaded variants installed and marked in the registry. I'm going to look more into that. Edit: I think what happens is that users will still see the stale registry/tag state until they explicitly upgrade or reinstall one of the variants. We could probably "fix" that by doing our orphaned registry entry removal on |
|
@konstin is the owner of the Windows registry writes, fyi. |
Why aren't other tests observing the state? |
The combination of #19012 (review) and Line 1235 in 9c0eec6 (So IIUC |
|
I think it'd make sense to add a We'd also need to force the tests to run serially if we ever add more. Alas. Can we use something like |
SG, will do 🙂
Yeah, I think so! This is what I meant with the first option in the PR comment:
I can look into that with a follow-up PR, I think it's a good idea regardless (because at some point we'll probably want more coverage on these routes anyways). |
|
I've so far avoided any tests that (intentionally) modify registry. The risk is that we permanently modify a developer machine, as there's no temp dir root or virtual filesystem for the registry. It's helpful if we can run it in CI, but I wouldn't run this test by default on developer machines. The fix itself looks good. |
|
Agreed, I'll tweak this so that we only run |
Signed-off-by: William Woodruff <william@astral.sh>
Signed-off-by: William Woodruff <william@astral.sh>
Signed-off-by: William Woodruff <william@astral.sh>
ddeb23f to
7dc3dbf
Compare
|
Done, PTAL! |
Signed-off-by: William Woodruff <william@astral.sh>
This adds a regression test and fix for astral-sh#18795. I ran the test and confirmed reproduction before implementing the fix. The underlying bug here happens only on Windows, and only when exercising the PEP 514 Python installation registration pathway (which the integration tests disable by default, since it involves global mutable state that leaks between tests). The bug itself is just an imprecision in how we compute the "tag" for the Python entry -- we weren't including the variant (the `t` in `3.14t`), so two distinct installs (`3.14` and `3.14t`) would end up with the same registry tag. For an end user, this surfaces as Python installation entries missing when running `uv python list`. One thing to note about the test here is that it _does_ exercise the Windows registry pathway, which means that it intentionally bypasses the guardrail around global mutations in the integration tests. This is "fine" in the sense that there are on other tests observing that state at the moment, but I think it's a risk in terms of isolation (in the sense that devs who run our integration tests will actually observe global changes to their Python installations, plus any failure in the test means we won't clean up our global changes). Two options there: - I could try and harden/isolate the registry mutation pathways a bit more, e.g. we could add `UV_DEV_WINDOWS_REGISTRY_COMPANY_KEY` or something like that to do some more test-level isolation of HKCU writes. This still modifies global state, but at least it'll be more namespaced. - I could remove the integration test entirely, now that we've confirmed that the fix itself works. This leaves us without coverage, but given that the fix itself is ~2 lines that might be acceptable. Fixes astral-sh#18795. This PR includes a regression test. --------- Signed-off-by: William Woodruff <william@astral.sh> (cherry picked from commit 69e1f5f)
Summary
This adds a regression test and fix for #18795. I ran the test and confirmed reproduction before implementing the fix.
The underlying bug here happens only on Windows, and only when exercising the PEP 514 Python installation registration pathway (which the integration tests disable by default, since it involves global mutable state that leaks between tests). The bug itself is just an imprecision in how we compute the "tag" for the Python entry -- we weren't including the variant (the
tin3.14t), so two distinct installs (3.14and3.14t) would end up with the same registry tag. For an end user, this surfaces as Python installation entries missing when runninguv python list.One thing to note about the test here is that it does exercise the Windows registry pathway, which means that it intentionally bypasses the guardrail around global mutations in the integration tests. This is "fine" in the sense that there are on other tests observing that state at the moment, but I think it's a risk in terms of isolation (in the sense that devs who run our integration tests will actually observe global changes to their Python installations, plus any failure in the test means we won't clean up our global changes). Two options there:
UV_DEV_WINDOWS_REGISTRY_COMPANY_KEYor something like that to do some more test-level isolation of HKCU writes. This still modifies global state, but at least it'll be more namespaced.Fixes #18795.
Test Plan
This PR includes a regression test.