From 32dc9bef599cc99335113df969d7a5f955ff7b81 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 2 Jul 2024 16:46:31 -0400 Subject: [PATCH] Respect tool upgrades in `uv tool install` (#4736) ## Summary For now the semantics are such that if the requested requirements from the command line don't match the receipt (or if any `--reinstall` or `--upgrade` is requested), we proceed with an install, passing the `--reinstall` and `--upgrade` to the underlying Python environment. This may lead to some unintuitive behaviors, but it's simplest for now. For example: - `uv tool install black<24` followed by `uv tool install black --upgrade` will install the latest version of `black`, removing the `<24` constraint. - `uv tool install black --with black-plugin` followed by `uv tool install black` will remove `black-plugin`. Closes https://github.com/astral-sh/uv/issues/4659. --- Cargo.lock | 1 + crates/pypi-types/src/requirement.rs | 76 ++++++++++- crates/uv-tool/Cargo.toml | 1 + crates/uv-tool/src/tool.rs | 9 +- crates/uv/src/commands/project/mod.rs | 6 +- crates/uv/src/commands/project/run.rs | 2 + crates/uv/src/commands/tool/install.rs | 84 +++++------- crates/uv/src/commands/tool/run.rs | 2 + crates/uv/tests/common/mod.rs | 4 +- crates/uv/tests/tool_install.rs | 170 ++++++++++++++++++++++++- 10 files changed, 293 insertions(+), 62 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c65a5fa33..4e7b9e528 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5039,6 +5039,7 @@ dependencies = [ "pathdiff", "pep440_rs", "pep508_rs", + "pypi-types", "serde", "thiserror", "toml", diff --git a/crates/pypi-types/src/requirement.rs b/crates/pypi-types/src/requirement.rs index 115e69a0e..0ed3abceb 100644 --- a/crates/pypi-types/src/requirement.rs +++ b/crates/pypi-types/src/requirement.rs @@ -5,7 +5,7 @@ use url::Url; use pep440_rs::VersionSpecifiers; use pep508_rs::{MarkerEnvironment, MarkerTree, RequirementOrigin, VerbatimUrl, VersionOrUrl}; -use uv_git::{GitReference, GitSha}; +use uv_git::{GitReference, GitSha, GitUrl}; use uv_normalize::{ExtraName, PackageName}; use crate::{ @@ -66,6 +66,80 @@ impl From for pep508_rs::Requirement { } } +impl From for pep508_rs::Requirement { + /// Convert a [`Requirement`] to a [`pep508_rs::Requirement`]. + fn from(requirement: Requirement) -> Self { + pep508_rs::Requirement { + name: requirement.name, + extras: requirement.extras, + marker: requirement.marker, + origin: requirement.origin, + version_or_url: match requirement.source { + RequirementSource::Registry { specifier, .. } => { + Some(VersionOrUrl::VersionSpecifier(specifier)) + } + RequirementSource::Url { + subdirectory, + location, + url, + } => Some(VersionOrUrl::Url(VerbatimParsedUrl { + parsed_url: ParsedUrl::Archive(ParsedArchiveUrl { + url: location, + subdirectory, + }), + verbatim: url, + })), + RequirementSource::Git { + repository, + reference, + precise, + subdirectory, + url, + } => { + let git_url = if let Some(precise) = precise { + GitUrl::new(repository, reference).with_precise(precise) + } else { + GitUrl::new(repository, reference) + }; + Some(VersionOrUrl::Url(VerbatimParsedUrl { + parsed_url: ParsedUrl::Git(ParsedGitUrl { + url: git_url, + subdirectory, + }), + verbatim: url, + })) + } + RequirementSource::Path { + install_path, + lock_path, + url, + } => Some(VersionOrUrl::Url(VerbatimParsedUrl { + parsed_url: ParsedUrl::Path(ParsedPathUrl { + url: url.to_url(), + install_path, + lock_path, + }), + verbatim: url, + })), + RequirementSource::Directory { + install_path, + lock_path, + editable, + url, + } => Some(VersionOrUrl::Url(VerbatimParsedUrl { + parsed_url: ParsedUrl::Directory(ParsedDirectoryUrl { + url: url.to_url(), + install_path, + lock_path, + editable, + }), + verbatim: url, + })), + }, + } + } +} + impl From> for Requirement { /// Convert a [`pep508_rs::Requirement`] to a [`Requirement`]. fn from(requirement: pep508_rs::Requirement) -> Self { diff --git a/crates/uv-tool/Cargo.toml b/crates/uv-tool/Cargo.toml index 2ed6b431a..33426caba 100644 --- a/crates/uv-tool/Cargo.toml +++ b/crates/uv-tool/Cargo.toml @@ -16,6 +16,7 @@ workspace = true install-wheel-rs = { workspace = true } pep440_rs = { workspace = true } pep508_rs = { workspace = true } +pypi-types = { workspace = true } uv-cache = { workspace = true } uv-fs = { workspace = true } uv-state = { workspace = true } diff --git a/crates/uv-tool/src/tool.rs b/crates/uv-tool/src/tool.rs index a24b94572..1ae5449fa 100644 --- a/crates/uv-tool/src/tool.rs +++ b/crates/uv-tool/src/tool.rs @@ -1,6 +1,7 @@ use std::path::PathBuf; use path_slash::PathBufExt; +use pypi_types::VerbatimParsedUrl; use serde::Deserialize; use toml_edit::value; use toml_edit::Array; @@ -14,7 +15,7 @@ use toml_edit::Value; #[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] pub struct Tool { /// The requirements requested by the user during installation. - requirements: Vec, + requirements: Vec>, /// The Python requested by the user during installation. python: Option, /// A mapping of entry point names to their metadata. @@ -58,7 +59,7 @@ fn each_element_on_its_line_array(elements: impl Iterator, + requirements: Vec>, python: Option, entrypoints: impl Iterator, ) -> Self { @@ -108,6 +109,10 @@ impl Tool { pub fn entrypoints(&self) -> &[ToolEntrypoint] { &self.entrypoints } + + pub fn requirements(&self) -> &[pep508_rs::Requirement] { + &self.requirements + } } impl ToolEntrypoint { diff --git a/crates/uv/src/commands/project/mod.rs b/crates/uv/src/commands/project/mod.rs index 2caf6ddee..c2a382446 100644 --- a/crates/uv/src/commands/project/mod.rs +++ b/crates/uv/src/commands/project/mod.rs @@ -24,6 +24,7 @@ use uv_toolchain::{ use uv_types::{BuildIsolation, HashStrategy, InFlight}; use crate::commands::pip; +use crate::commands::pip::operations::Modifications; use crate::commands::reporters::ResolverReporter; use crate::printer::Printer; use crate::settings::ResolverInstallerSettings; @@ -375,6 +376,7 @@ pub(crate) async fn resolve_names( pub(crate) async fn update_environment( venv: PythonEnvironment, spec: RequirementsSpecification, + modifications: Modifications, settings: &ResolverInstallerSettings, state: &SharedState, preview: PreviewMode, @@ -402,7 +404,7 @@ pub(crate) async fn update_environment( // Check if the current environment satisfies the requirements let site_packages = SitePackages::from_environment(&venv)?; - if spec.source_trees.is_empty() && reinstall.is_none() { + if spec.source_trees.is_empty() && reinstall.is_none() && upgrade.is_none() { match site_packages.satisfies(&spec.requirements, &spec.constraints)? { // If the requirements are already satisfied, we're done. SatisfiesResult::Fresh { @@ -554,7 +556,7 @@ pub(crate) async fn update_environment( pip::operations::install( &resolution, site_packages, - pip::operations::Modifications::Sufficient, + modifications, reinstall, build_options, *link_mode, diff --git a/crates/uv/src/commands/project/run.rs b/crates/uv/src/commands/project/run.rs index 9ae09be36..8e05bbd69 100644 --- a/crates/uv/src/commands/project/run.rs +++ b/crates/uv/src/commands/project/run.rs @@ -111,6 +111,7 @@ pub(crate) async fn run( let environment = project::update_environment( venv, spec, + Modifications::Sufficient, &settings, &state, preview, @@ -300,6 +301,7 @@ pub(crate) async fn run( project::update_environment( venv, spec, + Modifications::Sufficient, &settings, &state, preview, diff --git a/crates/uv/src/commands/tool/install.rs b/crates/uv/src/commands/tool/install.rs index a1d5e1a31..f9446180a 100644 --- a/crates/uv/src/commands/tool/install.rs +++ b/crates/uv/src/commands/tool/install.rs @@ -11,7 +11,7 @@ use distribution_types::Name; use pypi_types::Requirement; use uv_cache::Cache; use uv_client::{BaseClientBuilder, Connectivity}; -use uv_configuration::{Concurrency, PreviewMode, Reinstall}; +use uv_configuration::{Concurrency, PreviewMode}; #[cfg(unix)] use uv_fs::replace_symlink; use uv_fs::Simplified; @@ -25,6 +25,7 @@ use uv_toolchain::{ }; use uv_warnings::warn_user_once; +use crate::commands::pip::operations::Modifications; use crate::commands::project::{update_environment, SharedState}; use crate::commands::{project, ExitStatus}; use crate::printer::Printer; @@ -122,37 +123,6 @@ pub(crate) async fn install( .unwrap() }; - let installed_tools = InstalledTools::from_settings()?; - - let existing_tool_receipt = installed_tools.get_tool_receipt(&from.name)?; - // TODO(zanieb): Automatically replace an existing tool if the request differs - let reinstall_entry_points = if existing_tool_receipt.is_some() { - if force { - debug!("Replacing existing tool due to `--force` flag."); - true - } else { - match settings.reinstall { - Reinstall::All => { - debug!("Replacing existing tool due to `--reinstall` flag."); - true - } - // Do not replace the entry points unless the tool is explicitly requested - Reinstall::Packages(ref packages) => packages.contains(&from.name), - // If not reinstalling... then we're done - Reinstall::None => { - writeln!( - printer.stderr(), - "Tool `{}` is already installed", - from.name - )?; - return Ok(ExitStatus::Failure); - } - } - } - } else { - false - }; - // Combine the `from` and `with` requirements. let requirements = { let mut requirements = Vec::with_capacity(1 + with.len()); @@ -175,23 +145,44 @@ pub(crate) async fn install( requirements }; + let installed_tools = InstalledTools::from_settings()?; + let existing_tool_receipt = installed_tools.get_tool_receipt(&from.name)?; + + // If the requested and receipt requirements are the same... + if let Some(tool_receipt) = existing_tool_receipt.as_ref() { + let receipt = tool_receipt + .requirements() + .iter() + .cloned() + .map(Requirement::from) + .collect::>(); + if requirements == receipt { + // And the user didn't request a reinstall or upgrade... + if !force && settings.reinstall.is_none() && settings.upgrade.is_none() { + // We're done. + writeln!(printer.stderr(), "Tool `{from}` is already installed")?; + return Ok(ExitStatus::Failure); + } + } + } + + // Replace entrypoints if the tool already exists (and we made it this far). If we find existing + // entrypoints later on, and the tool _doesn't_ exist, we'll avoid removing the external tool's + // entrypoints (without `--force`). + let reinstall_entry_points = existing_tool_receipt.is_some(); + // TODO(zanieb): Build the environment in the cache directory then copy into the tool directory // This lets us confirm the environment is valid before removing an existing install - let environment = installed_tools.environment( - &from.name, - // Do not remove the existing environment if we're reinstalling a subset of packages - !matches!(settings.reinstall, Reinstall::Packages(_)), - interpreter, - cache, - )?; + let environment = installed_tools.environment(&from.name, force, interpreter, cache)?; // Install the ephemeral requirements. let spec = RequirementsSpecification::from_requirements(requirements.clone()); let environment = update_environment( environment, spec, + Modifications::Exact, &settings, - &SharedState::default(), + &state, preview, connectivity, concurrency, @@ -207,17 +198,6 @@ pub(crate) async fn install( bail!("Expected at least one requirement") }; - // Exit early if we're not supposed to be reinstalling entry points - // e.g. `--reinstall-package` was used for some dependency - if existing_tool_receipt.is_some() && !reinstall_entry_points { - writeln!( - printer.stderr(), - "Updated environment for tool `{}`", - from.name - )?; - return Ok(ExitStatus::Success); - } - // Find a suitable path to install into // TODO(zanieb): Warn if this directory is not on the PATH let executable_directory = find_executable_directory()?; @@ -324,7 +304,7 @@ pub(crate) async fn install( } /// Resolve any [`UnnamedRequirements`]. -pub(crate) async fn resolve_requirements( +async fn resolve_requirements( requirements: impl Iterator, interpreter: &Interpreter, settings: &ResolverInstallerSettings, diff --git a/crates/uv/src/commands/tool/run.rs b/crates/uv/src/commands/tool/run.rs index 65bd0f2fa..cdf45141e 100644 --- a/crates/uv/src/commands/tool/run.rs +++ b/crates/uv/src/commands/tool/run.rs @@ -21,6 +21,7 @@ use uv_toolchain::{ }; use uv_warnings::warn_user_once; +use crate::commands::pip::operations::Modifications; use crate::commands::project::{update_environment, SharedState}; use crate::commands::ExitStatus; use crate::printer::Printer; @@ -103,6 +104,7 @@ pub(crate) async fn run( update_environment( venv, spec, + Modifications::Sufficient, &settings, &SharedState::default(), preview, diff --git a/crates/uv/tests/common/mod.rs b/crates/uv/tests/common/mod.rs index 89f0ab41d..1da697e68 100644 --- a/crates/uv/tests/common/mod.rs +++ b/crates/uv/tests/common/mod.rs @@ -792,9 +792,9 @@ pub fn run_and_format>( // The optional leading +/- is for install logs, the optional next line is for lock files let windows_only_deps = [ ("( [+-] )?colorama==\\d+(\\.[\\d+])+\n( # via .*\n)?"), - ("( [+-] )?colorama==\\d+(\\.[\\d+])+\\s+(# via .*\n)?"), + ("( [+-] )?colorama==\\d+(\\.[\\d+])+(\\s+# via .*)?\n"), ("( [+-] )?tzdata==\\d+(\\.[\\d+])+\n( # via .*\n)?"), - ("( [+-] )?tzdata==\\d+(\\.[\\d+])+\\s+(# via .*\n)?"), + ("( [+-] )?tzdata==\\d+(\\.[\\d+])+(\\s+# via .*)?\n"), ]; let mut removed_packages = 0; for windows_only_dep in windows_only_deps { diff --git a/crates/uv/tests/tool_install.rs b/crates/uv/tests/tool_install.rs index 77cf1bf9e..f081b183a 100644 --- a/crates/uv/tests/tool_install.rs +++ b/crates/uv/tests/tool_install.rs @@ -436,12 +436,19 @@ fn tool_install_already_installed() { ----- stderr ----- warning: `uv tool install` is experimental and may change without warning. Resolved [N] packages in [TIME] + Uninstalled [N] packages in [TIME] Installed [N] packages in [TIME] + - black==24.3.0 + black==24.3.0 + - click==8.1.7 + click==8.1.7 + - mypy-extensions==1.0.0 + mypy-extensions==1.0.0 + - packaging==24.0 + packaging==24.0 + - pathspec==0.12.1 + pathspec==0.12.1 + - platformdirs==4.2.0 + platformdirs==4.2.0 Installed: black, blackd "###); @@ -469,7 +476,7 @@ fn tool_install_already_installed() { "###); // Install `black` again with `--reinstall-package` for a dependency - // We should reinstall `click` in the environment but not reinstall the entry points + // We should reinstall `click` in the environment but not reinstall `black` uv_snapshot!(context.filters(), context.tool_install() .arg("black") .arg("--reinstall-package") @@ -487,7 +494,7 @@ fn tool_install_already_installed() { Installed [N] packages in [TIME] - click==8.1.7 + click==8.1.7 - Updated environment for tool `black` + Installed: black, blackd "###); } @@ -683,12 +690,19 @@ fn tool_install_entry_point_exists() { ----- stderr ----- warning: `uv tool install` is experimental and may change without warning. Resolved [N] packages in [TIME] + Uninstalled [N] packages in [TIME] Installed [N] packages in [TIME] + - black==24.3.0 + black==24.3.0 + - click==8.1.7 + click==8.1.7 + - mypy-extensions==1.0.0 + mypy-extensions==1.0.0 + - packaging==24.0 + packaging==24.0 + - pathspec==0.12.1 + pathspec==0.12.1 + - platformdirs==4.2.0 + platformdirs==4.2.0 Installed: black, blackd "###); @@ -716,7 +730,7 @@ fn tool_install_entry_point_exists() { }, { // Should run black in the virtual environment assert_snapshot!(fs_err::read_to_string(executable).unwrap(), @r###" - #![TEMP_DIR]/tools/black/bin/python + #![TEMP_DIR]/tools/black/bin/python3 # -*- coding: utf-8 -*- import re import sys @@ -1165,3 +1179,153 @@ fn tool_install_unnamed_with() { ----- stderr ----- "###); } + +/// Test upgrading an already installed tool. +#[test] +fn tool_install_upgrade() { + let context = TestContext::new("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("black==24.1.1") + .env("UV_TOOL_DIR", tool_dir.as_os_str()) + .env("XDG_BIN_HOME", bin_dir.as_os_str()), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + warning: `uv tool install` is experimental and may change without warning. + Resolved [N] packages in [TIME] + Prepared [N] packages in [TIME] + Installed [N] packages in [TIME] + + black==24.1.1 + + click==8.1.7 + + mypy-extensions==1.0.0 + + packaging==24.0 + + pathspec==0.12.1 + + platformdirs==4.2.0 + Installed: black, blackd + "###); + + insta::with_settings!({ + filters => context.filters(), + }, { + // We should have a tool receipt + assert_snapshot!(fs_err::read_to_string(tool_dir.join("black").join("uv-receipt.toml")).unwrap(), @r###" + [tool] + requirements = ["black==24.1.1"] + entrypoints = [ + { name = "black", install-path = "[TEMP_DIR]/bin/black" }, + { name = "blackd", install-path = "[TEMP_DIR]/bin/blackd" }, + ] + "###); + }); + + // Install without the constraint. It should be replaced, but the package shouldn't be installed + // since it's already satisfied in the environment. + uv_snapshot!(context.filters(), context.tool_install() + .arg("black") + .env("UV_TOOL_DIR", tool_dir.as_os_str()) + .env("XDG_BIN_HOME", bin_dir.as_os_str()), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + warning: `uv tool install` is experimental and may change without warning. + Installed: black, blackd + "###); + + insta::with_settings!({ + filters => context.filters(), + }, { + // We should have a tool receipt + assert_snapshot!(fs_err::read_to_string(tool_dir.join("black").join("uv-receipt.toml")).unwrap(), @r###" + [tool] + requirements = ["black"] + entrypoints = [ + { name = "black", install-path = "[TEMP_DIR]/bin/black" }, + { name = "blackd", install-path = "[TEMP_DIR]/bin/blackd" }, + ] + "###); + }); + + // Install with a `with`. It should be added to the environment. + uv_snapshot!(context.filters(), context.tool_install() + .arg("black") + .arg("--with") + .arg("iniconfig @ https://files.pythonhosted.org/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl") + .env("UV_TOOL_DIR", tool_dir.as_os_str()) + .env("XDG_BIN_HOME", bin_dir.as_os_str()), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + warning: `uv tool install` is experimental and may change without warning. + Resolved [N] packages in [TIME] + Prepared [N] packages in [TIME] + Installed [N] packages in [TIME] + + iniconfig==2.0.0 (from https://files.pythonhosted.org/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl) + Installed: black, blackd + "###); + + insta::with_settings!({ + filters => context.filters(), + }, { + // We should have a tool receipt + assert_snapshot!(fs_err::read_to_string(tool_dir.join("black").join("uv-receipt.toml")).unwrap(), @r###" + [tool] + requirements = [ + "black", + "iniconfig @ https://files.pythonhosted.org/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl", + ] + entrypoints = [ + { name = "black", install-path = "[TEMP_DIR]/bin/black" }, + { name = "blackd", install-path = "[TEMP_DIR]/bin/blackd" }, + ] + "###); + }); + + // Install with `--upgrade`. `black` should be reinstalled with a more recent version, and + // `iniconfig` should be removed. + uv_snapshot!(context.filters(), context.tool_install() + .arg("black") + .arg("--upgrade") + .env("UV_TOOL_DIR", tool_dir.as_os_str()) + .env("XDG_BIN_HOME", bin_dir.as_os_str()), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + warning: `uv tool install` is experimental and may change without warning. + Resolved [N] packages in [TIME] + Prepared [N] packages in [TIME] + Uninstalled [N] packages in [TIME] + Installed [N] packages in [TIME] + - black==24.1.1 + + black==24.3.0 + - iniconfig==2.0.0 (from https://files.pythonhosted.org/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl) + Installed: black, blackd + "###); + + insta::with_settings!({ + filters => context.filters(), + }, { + // We should have a tool receipt + assert_snapshot!(fs_err::read_to_string(tool_dir.join("black").join("uv-receipt.toml")).unwrap(), @r###" + [tool] + requirements = ["black"] + entrypoints = [ + { name = "black", install-path = "[TEMP_DIR]/bin/black" }, + { name = "blackd", install-path = "[TEMP_DIR]/bin/blackd" }, + ] + "###); + }); +}