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

uv-tool/install: ignore existing environments on interpreter mismatch #7451

Merged
merged 3 commits into from
Sep 18, 2024
Merged

uv-tool/install: ignore existing environments on interpreter mismatch #7451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
455840e
uv-tool: ignore existing environment on interpreter mismatch
lucab Sep 17, 2024
fa1a972
fix wording
lucab Sep 17, 2024
9db055f
split test
lucab Sep 17, 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
2 changes: 1 addition & 1 deletion crates/uv-tool/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ impl InstalledTools {
match PythonEnvironment::from_root(&environment_path, cache) {
Ok(venv) => {
debug!(
"Using existing environment for tool `{name}`: {}",
"Found existing environment for tool `{name}`: {}",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: drive-by change to make debug logs more uniform. After this, the output now looks like this:

DEBUG Found existing environment for tool `poetry`: .local/share/uv/tools/poetry
DEBUG Found existing interpreter for tool `poetry`: /home/lucab/.local/share/uv/tools/poetry/bin/python3

environment_path.user_display()
);
Ok(Some(venv))
Expand Down
36 changes: 22 additions & 14 deletions crates/uv/src/commands/tool/install.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use owo_colors::OwoColorize;
use pep440_rs::{VersionSpecifier, VersionSpecifiers};
use pep508_rs::MarkerTree;
use pypi_types::{Requirement, RequirementSource};
use tracing::debug;
use tracing::trace;
use uv_cache::{Cache, Refresh};
use uv_cache_info::Timestamp;
use uv_client::{BaseClientBuilder, Connectivity};
Expand Down Expand Up @@ -276,19 +276,27 @@ pub(crate) async fn install(
installed_tools
.get_environment(&from.name, &cache)?
.filter(|environment| {
python_request.as_ref().map_or(true, |python_request| {
if python_request.satisfied(environment.interpreter(), &cache) {
debug!("Found existing environment for `{from}`", from = from.name.cyan());
true
} else {
let _ = writeln!(
printer.stderr(),
"Existing environment for `{from}` does not satisfy the requested Python interpreter",
from = from.name.cyan(),
);
false
}
})
// NOTE(lucab): this compares `base_prefix` paths as a proxy for
lucab marked this conversation as resolved.
Show resolved Hide resolved
// detecting interpreters mismatches. Directly comparing interpreters
// (by paths or binaries on-disk) would result in several false
// positives on Windows due to file-copying and shims.
let old_base_prefix = environment.interpreter().sys_base_prefix();
let selected_base_prefix = interpreter.sys_base_prefix();
if old_base_prefix == selected_base_prefix {
trace!(
"Existing interpreter matches the requested interpreter for `{}`: {}",
from.name,
environment.interpreter().sys_executable().display()
);
true
} else {
let _ = writeln!(
printer.stderr(),
"Ignoring existing environment for `{from}`: the requested Python interpreter does not match the environment interpreter",
from = from.name.cyan(),
);
false
}
});

// If the requested and receipt requirements are the same...
Expand Down
150 changes: 147 additions & 3 deletions crates/uv/tests/tool_install.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2074,7 +2074,7 @@ fn tool_install_upgrade() {

/// Test reinstalling tools with varying `--python` requests.
#[test]
fn tool_install_python_request() {
fn tool_install_python_requests() {
let context = TestContext::new_with_versions(&["3.11", "3.12"])
.with_filtered_counts()
.with_filtered_exe_suffix();
Expand Down Expand Up @@ -2122,7 +2122,7 @@ fn tool_install_python_request() {
`black` is already installed
"###);

// Install with Python 3.11 (incompatible).
// // Install with Python 3.11 (incompatible).
uv_snapshot!(context.filters(), context.tool_install()
.arg("-p")
.arg("3.11")
Expand All @@ -2135,7 +2135,7 @@ fn tool_install_python_request() {
----- stdout -----

----- stderr -----
Existing environment for `black` does not satisfy the requested Python interpreter
Ignoring existing environment for `black`: the requested Python interpreter does not match the environment interpreter
Resolved [N] packages in [TIME]
Prepared [N] packages in [TIME]
Installed [N] packages in [TIME]
Expand All @@ -2149,6 +2149,150 @@ fn tool_install_python_request() {
"###);
}

/// Test reinstalling tools with varying `--python` and
/// `--python-preference` parameters.
#[ignore = "https://github.com/astral-sh/uv/issues/7473"]
#[test]
fn tool_install_python_preference() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

$ cargo nextest run --run-ignored all tool_install_python_preference
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.29s
------------
 Nextest run ID 110712ea-a0a0-436a-ab95-4cdfbb384338 with nextest profile: default
    Starting 1 test across 86 binaries (1651 tests skipped)
        PASS [   6.088s] uv::tool_install tool_install_python_preference
------------
     Summary [   6.089s] 1 test run: 1 passed, 1651 skipped

let context = TestContext::new_with_versions(&["3.11", "3.12"])
.with_filtered_counts()
.with_filtered_exe_suffix();
let tool_dir = context.temp_dir.child("tools");
let bin_dir = context.temp_dir.child("bin");

// Install `black`.
uv_snapshot!(context.filters(), context.tool_install()
.arg("-p")
.arg("3.12")
.arg("black")
.env("UV_TOOL_DIR", tool_dir.as_os_str())
.env("XDG_BIN_HOME", bin_dir.as_os_str())
.env("PATH", bin_dir.as_os_str()), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
Resolved [N] packages in [TIME]
Prepared [N] packages in [TIME]
Installed [N] packages in [TIME]
+ black==24.3.0
+ click==8.1.7
+ mypy-extensions==1.0.0
+ packaging==24.0
+ pathspec==0.12.1
+ platformdirs==4.2.0
Installed 2 executables: black, blackd
"###);

// Install with Python 3.12 (compatible).
uv_snapshot!(context.filters(), context.tool_install()
.arg("-p")
.arg("3.12")
.arg("black")
.env("UV_TOOL_DIR", tool_dir.as_os_str())
.env("XDG_BIN_HOME", bin_dir.as_os_str())
.env("PATH", bin_dir.as_os_str()), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
`black` is already installed
"###);

// Install with system Python 3.11 (different version, incompatible).
uv_snapshot!(context.filters(), context.tool_install()
.arg("-p")
.arg("3.11")
.arg("--python-preference")
.arg("only-system")
.arg("black")
.env("UV_TOOL_DIR", tool_dir.as_os_str())
.env("XDG_BIN_HOME", bin_dir.as_os_str())
.env("PATH", bin_dir.as_os_str()), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
Ignoring existing environment for `black`: the requested Python interpreter does not match the environment interpreter
Resolved [N] packages in [TIME]
Prepared [N] packages in [TIME]
Installed [N] packages in [TIME]
+ black==24.3.0
+ click==8.1.7
+ mypy-extensions==1.0.0
+ packaging==24.0
+ pathspec==0.12.1
+ platformdirs==4.2.0
Installed 2 executables: black, blackd
"###);

// Install with system Python 3.11 (compatible).
uv_snapshot!(context.filters(), context.tool_install()
.arg("-p")
.arg("3.11")
.arg("--python-preference")
.arg("only-system")
.arg("black")
.env("UV_TOOL_DIR", tool_dir.as_os_str())
.env("XDG_BIN_HOME", bin_dir.as_os_str())
.env("PATH", bin_dir.as_os_str()), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
`black` is already installed
"###);

// Install with managed Python 3.11 (different source, incompatible).
uv_snapshot!(context.filters(), context.tool_install()
.arg("-p")
.arg("3.11")
.arg("--python-preference")
.arg("only-managed")
Comment on lines +2254 to +2255
Copy link
Member

Choose a reason for hiding this comment

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

This isn't super obvious, but I think this can fetch a Python managed version during the test which we currently avoid (downstream packagers do not like it). I think this test may also be brittle depending on the developer's machine — the test context is happy with whatever Python versions it can find whether they are system or managed. I am not sure how to properly write a test for this though :/ it seems like a pain.

Copy link
Member

Choose a reason for hiding this comment

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

For the sake of your time, I'd recommend omitting test coverage and opening an issue to add it. We'll need to consider a comprehensive solution here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can split the new cases apart in a different test and mark it as ignored, if you like. That should still allow running the test manually if needed, but won't impact packagers.
I'll open a ticket about testing with system interpreters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#7473

.arg("black")
.env("UV_TOOL_DIR", tool_dir.as_os_str())
.env("XDG_BIN_HOME", bin_dir.as_os_str())
.env("PATH", bin_dir.as_os_str()), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
Ignoring existing environment for `black`: the requested Python interpreter does not match the environment interpreter
Resolved [N] packages in [TIME]
Installed [N] packages in [TIME]
+ black==24.3.0
+ click==8.1.7
+ mypy-extensions==1.0.0
+ packaging==24.0
+ pathspec==0.12.1
+ platformdirs==4.2.0
Installed 2 executables: black, blackd
"###);

// Install with managed Python 3.11 (compatible).
uv_snapshot!(context.filters(), context.tool_install()
.arg("-p")
.arg("3.11")
.arg("--python-preference")
.arg("only-managed")
.arg("black")
.env("UV_TOOL_DIR", tool_dir.as_os_str())
.env("XDG_BIN_HOME", bin_dir.as_os_str())
.env("PATH", bin_dir.as_os_str()), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
`black` is already installed
"###);
}

/// Test preserving a tool environment when new but incompatible requirements are requested.
#[test]
fn tool_install_preserve_environment() {
Expand Down
Loading