From 9ec3452e238bf47c7e1f4ac5cf2e06ee22625745 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Wed, 26 Jun 2024 11:00:25 -0500 Subject: [PATCH] Add `uv tool install --force` --- crates/uv-cli/src/lib.rs | 6 ++ crates/uv-tool/src/lib.rs | 14 +++ crates/uv/src/commands/tool/install.rs | 66 +++++++++++-- crates/uv/src/main.rs | 1 + crates/uv/src/settings.rs | 3 + crates/uv/tests/tool_install.rs | 132 ++++++++++++++++++++++--- 6 files changed, 200 insertions(+), 22 deletions(-) diff --git a/crates/uv-cli/src/lib.rs b/crates/uv-cli/src/lib.rs index 23bff8e7b0e4..fedeeb073c1b 100644 --- a/crates/uv-cli/src/lib.rs +++ b/crates/uv-cli/src/lib.rs @@ -1908,6 +1908,12 @@ pub struct ToolInstallArgs { #[command(flatten)] pub refresh: RefreshArgs, + /// Force installation of the tool. + /// + /// Will replace any existing entry points with the same name in the executable directory. + #[arg(long)] + pub force: bool, + /// The Python interpreter to use to build the tool environment. /// /// By default, uv will search for a Python executable in the `PATH`. uv ignores virtual diff --git a/crates/uv-tool/src/lib.rs b/crates/uv-tool/src/lib.rs index 72e3631ee850..3c6d8ded5299 100644 --- a/crates/uv-tool/src/lib.rs +++ b/crates/uv-tool/src/lib.rs @@ -107,6 +107,20 @@ impl InstalledTools { Ok(()) } + pub fn remove_environment(&self, name: &str) -> Result<(), Error> { + let _lock = self.acquire_lock(); + let environment_path = self.root.join(name); + + debug!( + "Deleting environment for tool `{name}` at {}.", + environment_path.user_display() + ); + + fs_err::remove_dir_all(environment_path)?; + + Ok(()) + } + pub fn create_environment( &self, name: &str, diff --git a/crates/uv/src/commands/tool/install.rs b/crates/uv/src/commands/tool/install.rs index 7480999383b3..82d068b78466 100644 --- a/crates/uv/src/commands/tool/install.rs +++ b/crates/uv/src/commands/tool/install.rs @@ -1,10 +1,13 @@ +use std::collections::BTreeSet; +use std::ffi::OsString; use std::fmt::Write; use std::str::FromStr; use anyhow::{bail, Context, Result}; use distribution_types::Name; -use pep508_rs::Requirement; +use itertools::Itertools; +use pep508_rs::Requirement; use pypi_types::VerbatimParsedUrl; use tracing::debug; use uv_cache::Cache; @@ -31,6 +34,7 @@ pub(crate) async fn install( python: Option, from: Option, with: Vec, + force: bool, settings: ResolverInstallerSettings, preview: PreviewMode, toolchain_preference: ToolchainPreference, @@ -46,10 +50,14 @@ pub(crate) async fn install( let installed_tools = InstalledTools::from_settings()?; - // TODO(zanieb): Allow replacing an existing tool + // TODO(zanieb): Automatically replace an existing tool if the request differs if installed_tools.find_tool_entry(&name)?.is_some() { - writeln!(printer.stderr(), "Tool `{name}` is already installed.")?; - return Ok(ExitStatus::Failure); + if force { + debug!("Replacing existing tool due to `--force` flag."); + } else { + writeln!(printer.stderr(), "Tool `{name}` is already installed.")?; + return Ok(ExitStatus::Failure); + } } // TODO(zanieb): Figure out the interface here, do we infer the name or do we match the `run --from` interface? @@ -113,17 +121,59 @@ pub(crate) async fn install( fs_err::create_dir_all(&executable_directory) .context("Failed to create executable directory")?; + debug!("Installing into {}", executable_directory.user_display()); + let entrypoints = entrypoint_paths( &environment, installed_dist.name(), installed_dist.version(), )?; + // Determine the entry points targets + let targets = entrypoints + .into_iter() + .map(|(name, path)| { + let target = executable_directory.join( + path.file_name() + .map(std::borrow::ToOwned::to_owned) + .unwrap_or_else(|| OsString::from(name.clone())), + ); + (name, path, target) + }) + .collect::>(); + + // Check if they exist, before installing + let mut existing_targets = targets + .iter() + .filter(|(_, _, target)| target.exists()) + .peekable(); + if force { + for (name, _, target) in existing_targets { + debug!("Removing existing install of `{name}`"); + fs_err::remove_file(target)?; + } + } else if existing_targets.peek().is_some() { + // Clean up the environment we just created + installed_tools.remove_environment(&name)?; + + let existing_targets = existing_targets + // SAFETY: We know the target has a filename because we just constructed it above + .map(|(_, _, target)| target.file_name().unwrap().to_string_lossy()) + .collect::>(); + let (s, exists) = if existing_targets.len() == 1 { + ("", "exists") + } else { + ("s", "exist") + }; + bail!( + "Entry point{s} for tool already {exists}: {} (use `--force` to overwrite)", + existing_targets.iter().join(", ") + ) + } + // TODO(zanieb): Handle the case where there are no entrypoints - // TODO(zanieb): Better error when an entry point exists, check if they all are don't exist first - for (name, path) in entrypoints { - let target = executable_directory.join(path.file_name().unwrap()); - debug!("Installing {name} to {}", target.user_display()); + for (name, path, target) in targets { + debug!("Installing `{name}`"); #[cfg(unix)] replace_symlink(&path, &target).context("Failed to install entrypoint")?; #[cfg(windows)] diff --git a/crates/uv/src/main.rs b/crates/uv/src/main.rs index 559dd8432e65..9a9dd82bfc97 100644 --- a/crates/uv/src/main.rs +++ b/crates/uv/src/main.rs @@ -814,6 +814,7 @@ async fn run() -> Result { args.python, args.from, args.with, + args.force, args.settings, globals.preview, globals.toolchain_preference, diff --git a/crates/uv/src/settings.rs b/crates/uv/src/settings.rs index ff51351a7b37..641db3c5206c 100644 --- a/crates/uv/src/settings.rs +++ b/crates/uv/src/settings.rs @@ -241,6 +241,7 @@ pub(crate) struct ToolInstallSettings { pub(crate) python: Option, pub(crate) refresh: Refresh, pub(crate) settings: ResolverInstallerSettings, + pub(crate) force: bool, } impl ToolInstallSettings { @@ -252,6 +253,7 @@ impl ToolInstallSettings { from, with, installer, + force, build, refresh, python, @@ -262,6 +264,7 @@ impl ToolInstallSettings { from, with, python, + force, refresh: Refresh::from(refresh), settings: ResolverInstallerSettings::combine( resolver_installer_options(installer, build), diff --git a/crates/uv/tests/tool_install.rs b/crates/uv/tests/tool_install.rs index 77520ebff23f..dfac16f1c7a6 100644 --- a/crates/uv/tests/tool_install.rs +++ b/crates/uv/tests/tool_install.rs @@ -252,7 +252,7 @@ fn tool_install_twice() { }); } -/// Test installing a tool when its entry pint already exists +/// Test installing a tool when its entry point already exists #[test] fn tool_install_entry_point_exists() { let context = TestContext::new("3.12"); @@ -262,9 +262,77 @@ fn tool_install_entry_point_exists() { let executable = bin_dir.child(format!("black{}", std::env::consts::EXE_SUFFIX)); executable.touch().unwrap(); + // Drop executable suffixes for cross-platform snapshtos + let filters = context + .filters() + .into_iter() + .chain([(std::env::consts::EXE_SUFFIX, "")]) + .collect::>(); + // Install `black` + uv_snapshot!(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: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + warning: `uv tool install` is experimental and may change without warning. + Resolved 6 packages in [TIME] + Prepared 6 packages in [TIME] + Installed 6 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 + error: Entry point for tool already exists: black (use `--force` to overwrite) + "###); + + // We should delete the virtual environment + assert!(!tool_dir.child("black").exists()); + + // We should not write a tools entry + assert!(!tool_dir.join("tools.toml").exists()); + + insta::with_settings!({ + filters => context.filters(), + }, { + // Nor should we change the `black` entry point that exists + assert_snapshot!(fs_err::read_to_string(bin_dir.join("black")).unwrap(), @""); + + }); + + // Test error message when multiple entry points exist + bin_dir.child("blackd").touch().unwrap(); + uv_snapshot!(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: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + warning: `uv tool install` is experimental and may change without warning. + Resolved 6 packages in [TIME] + Installed 6 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 + error: Entry points for tool already exist: black, blackd (use `--force` to overwrite) + "###); + + // Install `black` with `--force` uv_snapshot!(context.filters(), context.tool_install() .arg("black") + .arg("--force") .env("UV_TOOL_DIR", tool_dir.as_os_str()) .env("XDG_BIN_HOME", bin_dir.as_os_str()), @r###" success: true @@ -274,7 +342,6 @@ fn tool_install_entry_point_exists() { ----- stderr ----- warning: `uv tool install` is experimental and may change without warning. Resolved 6 packages in [TIME] - Prepared 6 packages in [TIME] Installed 6 packages in [TIME] + black==24.3.0 + click==8.1.7 @@ -284,22 +351,59 @@ fn tool_install_entry_point_exists() { + platformdirs==4.2.0 "###); - // TODO(zanieb): We happily overwrite entry points by default right now - // https://github.com/astral-sh/uv/pull/4501 should resolve this + tool_dir.child("black").assert(predicate::path::is_dir()); - // We should not create a virtual environment - // assert!(tool_dir.child("black").exists()); + insta::with_settings!({ + filters => context.filters(), + }, { + // We write a tool entry + assert_snapshot!(fs_err::read_to_string(tool_dir.join("tools.toml")).unwrap(), @r###" + [tools] + black = { requirements = ["black"] } + "###); + }); - // // We should not write a tools entry - // assert!(!tool_dir.join("tools.toml").exists()); + let executable = bin_dir.child(format!("black{}", std::env::consts::EXE_SUFFIX)); + assert!(executable.exists()); - // insta::with_settings!({ - // filters => context.filters(), - // }, { - // // Nor should we change the `black` entry point that exists - // assert_snapshot!(fs_err::read_to_string(bin_dir.join("black")).unwrap(), @""); + // On Windows, we can't snapshot an executable file. + #[cfg(not(windows))] + insta::with_settings!({ + filters => context.filters(), + }, { + // Should run black in the virtual environment + assert_snapshot!(fs_err::read_to_string(executable).unwrap(), @r###" + #![TEMP_DIR]/tools/black/bin/python + # -*- coding: utf-8 -*- + import re + import sys + from black import patched_main + if __name__ == "__main__": + sys.argv[0] = re.sub(r"(-script\.pyw|\.exe)?$", "", sys.argv[0]) + sys.exit(patched_main()) + "###); + + }); - // }); + insta::with_settings!({ + filters => context.filters(), + }, { + // We should have a tool entry + assert_snapshot!(fs_err::read_to_string(tool_dir.join("tools.toml")).unwrap(), @r###" + [tools] + black = { requirements = ["black"] } + "###); + }); + + uv_snapshot!(context.filters(), Command::new("black").arg("--version").env("PATH", bin_dir.as_os_str()), @r###" + success: true + exit_code: 0 + ----- stdout ----- + black, 24.3.0 (compiled: yes) + Python (CPython) 3.12.[X] + + ----- stderr ----- + "###); } /// Test `uv tool install` when the bin directory is inferred from `$HOME`