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

Search PATH when python can't be found with py #1711

Merged
merged 7 commits into from
Feb 22, 2024

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Feb 19, 2024

Summary

This PR improves uv's compatibility with PIP when discovering python installations.

PIP runs the following step

  • if -p is a path, resolve that specific instance
  • test if the current python version sys.executable matches the requested version (or if no version was requested, always use it)
  • (windows): Use PEP514 to resolve the python version
  • Iterate over the paths and for each path test for python{major}.{minor}.{patch}, python{major}.{minor}, python{major}, python

Our existing implementation already did some of that but not all. We now

  • search for executables that match the requested version name: python3.9 python3.9.3 for Windows and Unix
  • Search the path on windows (rather than just relying on py)
  • Support pyenv shims (that's a hack)
  • Filter out the windows store python shim.

I merged most of the implementation between windows and unix because they are the same in venv too. The only real difference is that Windows must support py (PEP514) and shims are awkward.

Fixes #1310
Fixes #1660
Fixes #1168

Test Plan

  • Verified that running uv venv on Windows with no Python installed but the Windows shims activate fails with an error that Python wasn't found rather than invoking the shim
  • Verified that running uv venv with a local pyenv environment activate selects that Python version (because it is the first in the path)
  • If there's a python.exe on path and the python.bat shim, then the installation takes the python.exe (avoid indirection) except if the python.exe doesn't satisfy the requested version.
  • Verified on MacOs that uv venv picks up the different python versions (including pyenv)

@MichaReiser MichaReiser force-pushed the windows-path-discovery branch 3 times, most recently from 0b1c7e7 to 2275997 Compare February 20, 2024 12:01
@MichaReiser MichaReiser added the compatibility Compatibility with a specification or another tool label Feb 20, 2024
@MichaReiser MichaReiser force-pushed the windows-path-discovery branch 4 times, most recently from 17d8b2e to e580f5a Compare February 20, 2024 12:32
cache: &Cache,
) -> Result<Option<Interpreter>, Error> {
#[allow(non_snake_case)]
let UV_TEST_PYTHON_PATH = env::var_os("UV_TEST_PYTHON_PATH");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intentionally removed the TEST early return. We want that as much code as possible in this function is run during tests.

@MichaReiser MichaReiser force-pushed the windows-path-discovery branch 4 times, most recently from 200263d to 3bd2d59 Compare February 20, 2024 14:30
@MichaReiser MichaReiser marked this pull request as ready for review February 20, 2024 14:37
@zanieb zanieb self-requested a review February 20, 2024 18:37
cache,
),
// SAFETY: Guaranteed by the Ok(versions) guard
_ => unreachable!(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can request be empty? I just realized then we could hit this path

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pub fn main() {
    let versions = ""
        .splitn(3, '.')
        .map(str::parse::<u8>)
        .collect::<Result<Vec<_>, _>>();

    dbg!(versions);
}

gives me

[src/main.rs:8:9] versions = Err(
    ParseIntError {
        kind: Empty,
    },
)

So we're good

crates/uv-interpreter/src/python_query.rs Show resolved Hide resolved
crates/uv-interpreter/src/python_query.rs Outdated Show resolved Hide resolved
crates/uv-interpreter/src/python_query.rs Outdated Show resolved Hide resolved
crates/uv-interpreter/src/python_query.rs Outdated Show resolved Hide resolved
crates/uv-interpreter/src/python_query.rs Show resolved Hide resolved
for name in possible_names.iter().flatten() {
if let Ok(paths) = which::which_in_global(&**name, Some(&path)) {
for path in paths {
if checked_installs.contains(&path) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When do we see a path multiple times?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied that over from virtualenv but I'm not sure. Let's remove it for now.

crates/uv-interpreter/src/python_query.rs Outdated Show resolved Hide resolved
crates/uv/tests/venv.rs Show resolved Hide resolved
crates/uv-interpreter/src/lib.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility Compatibility with a specification or another tool windows Specific to the Windows platform
Projects
None yet
3 participants