Prioritize `PATH` over `py --list-paths` in Windows selection by charliermarsh · Pull Request #2057 · astral-sh/uv

charliermarsh · 2024-02-28T22:02:05Z

uv --system is failing in GitHub Actions, because py --list-paths returns all the pre-cached Pythons:

-V:3.12 *        C:\hostedtoolcache\windows\Python\3.12.2\x64\python.exe
-V:3.12-32       C:\hostedtoolcache\windows\Python\3.12.2\x86\python.exe
-V:3.11          C:\hostedtoolcache\windows\Python\3.11.8\x64\python.exe
-V:3.11-32       C:\hostedtoolcache\windows\Python\3.11.8\x86\python.exe
-V:3.10          C:\hostedtoolcache\windows\Python\3.10.11\x64\python.exe
-V:3.10-32       C:\hostedtoolcache\windows\Python\3.10.11\x86\python.exe
-V:3.9           C:\hostedtoolcache\windows\Python\3.9.13\x64\python.exe
-V:3.9-32        C:\hostedtoolcache\windows\Python\3.9.13\x86\python.exe
-V:3.8           C:\hostedtoolcache\windows\Python\3.8.10\x64\python.exe
-V:3.8-32        C:\hostedtoolcache\windows\Python\3.8.10\x86\python.exe
-V:3.7           C:\hostedtoolcache\windows\Python\3.7.9\x64\python.exe
-V:3.7-32        C:\hostedtoolcache\windows\Python\3.7.9\x86\python.exe

So, our default selector returns the first entry here. But none of these are actually in PATH except the one that the user installed via actions/setup-python@v5 -- that's the point of the action, that it puts the correct versions in PATH.

It seems to me like we should prioritize PATH over py --list-paths. Is there a good reason not to do this?

Closes: #2056

charliermarsh · 2024-02-28T22:02:12Z

\cc @AlexWaygood

ofek · 2024-02-29T01:37:26Z

This seems like a good idea to me and what users would expect to happen.

zanieb · 2024-02-29T02:10:43Z

I thought we did this already, maybe it's gone back and forth a bit during development. Makes sense to me.

charliermarsh · 2024-02-29T02:12:54Z

(Continuing from the issue...) I'm surprised because virtualenv does seem to do registry-based lookups before PATH. But I suppose for virtualenv it matters less, since the priority is getting the right version?

charliermarsh · 2024-02-29T02:15:06Z

I see Micha's change that added the fallback to PATH: #1711.

ofek · 2024-02-29T02:28:07Z

Also great because the overhead of a separate process is eliminated in most cases! I'm very excited for this.

MichaReiser

This seems like a good idea to me and what users would expect to happen.

I think this is a bit more nuanced and depends on your workflow and how you installed python. Users using py (over python) may have an additional installation on their PATH. For them, picking PATH over the default used by py can be surprising.

The reason virtual doesn't run into this problem is that it always tries to use the current Interpreter (in which it is running) first. It then doesn't matter whether a user uses python or py or a shim to run python.

The challenge we face is that we can't rely on the "current python" instance, so we have to approximate it as best as we can and using PATH first probably does the right thing in more cases than using py first, with the exception for users using py with another python installation on PATH.

Also great because the overhead of a separate process is eliminated in most cases! I'm very excited for this.

We planned to re-implement the registry lookup in Rust, in which case the PATH traversal is most likely slower, especially when we can't find a python installation on PATH and need to fall-back to py

MichaReiser · 2024-02-29T07:21:42Z

.github/workflows/system-install.yml

-#      - name: "Install Python"
-#        run: choco install python


(Intentionally kept this change.)

MichaReiser · 2024-02-29T07:32:41Z

crates/uv-interpreter/src/python_query.rs

+            Ok(None) => {
+                // No matching Python version found, continue searching PATH
+            }


This comment is outdated. We should remove this branch.

MichaReiser · 2024-02-29T07:33:18Z

crates/uv-interpreter/src/python_query.rs

+            }
+            Err(Error::PyList(error)) => {
+                if error.kind() == std::io::ErrorKind::NotFound {
+                    debug!("`py` is not installed. Falling back to searching Python on the path");


This message is outdated. We already tried searching the PATh. We should keep the graceful recovery

konstin · 2024-02-29T08:45:06Z

A minor feature we're losing is that py is intended to be the main tool on windows and has a friendly UX (py -0) that avoids dealing with PATH on windows (which is hidden in a GUI behind another GUI, it's nothing like adding a line in your .bashrc on linux).

From PEP 514:

This PEP defines a schema for the Python registry key to allow third-party installers to register their installation, and to allow tools and applications to detect and correctly display all Python environments on a user’s machine. No implementation changes to Python are proposed with this PEP.

Python environments are not required to be registered unless they want to be automatically discoverable by external tools.

In this sense github actions is at odds with the PEP, the versions shouldn't be in the registry if not selected by the setup python action (or setup python should at least ensure that py has the right default).

I think either choice of precedence between PATH and py is fine for us to make, both are standards, one of posix-and-windows ubiquity, the other of PEP 514, and if the user wants a specific python version they can pass it explictly. In this case i agree that we can't break github actions.

konstin · 2024-02-29T08:47:16Z

.github/workflows/system-install.yml

 name: System Install

 on:
+  pull_request:


Suggested change

pull_request:

(Am i understanding correctly that this should only be running manually?)

Yup! This is just set so I can test it during development of this PR. It will be removed prior to landing.

AlexWaygood · 2024-02-29T10:38:52Z

I agree that this is the right way to go.

FWIW, I always tick the "add to PATH" box when installing Python on Windows. I have a vague memory of things going wrong (or, not working the way I expected to) the one time I didn't do that. But that was a very long time ago, and it might very well be that I've just been cargo-culting from old tutorials that predated PEP-514 this whole time ¯\_(ツ)_/¯

ofek · 2024-02-29T14:08:20Z

FYI I wrote a bit here #2056 (comment) the relevant part is:

I have been exclusively a Windows user my entire life. I cannot express enough how few people use the py launcher and how often the registry stuff is unused.

🙂

charliermarsh marked this pull request as draft February 28, 2024 22:02

charliermarsh force-pushed the charlie/win branch 9 times, most recently from 90e3b2d to 2ff1d2e Compare February 29, 2024 00:50

charliermarsh changed the title ~~Debug GitHub Actions failures for Python 3.10 on Windows~~ Prioritize PATH over py --list-paths in Windows selection Feb 29, 2024

charliermarsh requested review from AlexWaygood and konstin February 29, 2024 01:13

charliermarsh marked this pull request as ready for review February 29, 2024 01:13

charliermarsh added the bug Something isn't working label Feb 29, 2024

charliermarsh mentioned this pull request Feb 29, 2024

--system doesn't work on Windows on Python <3.12 in GitHub Actions #2056

Closed

charliermarsh requested a review from MichaReiser February 29, 2024 02:15

MichaReiser approved these changes Feb 29, 2024

View reviewed changes

MichaReiser reviewed Feb 29, 2024

View reviewed changes

konstin approved these changes Feb 29, 2024

View reviewed changes

charliermarsh added 2 commits February 29, 2024 09:50

Debug GitHub Actions failures for Python 3.10 on Windows

af24ce4

Look in PATH first

3da520c

charliermarsh force-pushed the charlie/win branch 3 times, most recently from f04bfcf to 620f7aa Compare February 29, 2024 14:57

Fix comments

d8a2b20

charliermarsh force-pushed the charlie/win branch from 620f7aa to d8a2b20 Compare February 29, 2024 15:00

charliermarsh enabled auto-merge (squash) February 29, 2024 15:00

charliermarsh added the windows Specific to the Windows platform label Feb 29, 2024

charliermarsh merged commit b983ff4 into main Feb 29, 2024

charliermarsh deleted the charlie/win branch February 29, 2024 15:06

Porkepix mentioned this pull request Feb 29, 2024

uv 0.1.13 Homebrew/homebrew-core#164652

Merged

bbayles mentioned this pull request Mar 12, 2024

Use uv ada-url/ada-python#58

Open

Conversation

charliermarsh commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charliermarsh commented Feb 28, 2024

Uh oh!

ofek commented Feb 29, 2024

Uh oh!

zanieb commented Feb 29, 2024

Uh oh!

charliermarsh commented Feb 29, 2024

Uh oh!

charliermarsh commented Feb 29, 2024

Uh oh!

ofek commented Feb 29, 2024

Uh oh!

MichaReiser left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

charliermarsh Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

MichaReiser Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

MichaReiser Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

konstin commented Feb 29, 2024

Uh oh!

konstin Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

charliermarsh Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

AlexWaygood commented Feb 29, 2024

Uh oh!

ofek commented Feb 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

charliermarsh commented Feb 28, 2024 •

edited

Loading

MichaReiser left a comment •

edited

Loading