From 747ab0d9f782f12658d946ea5f4655cca1fa982d Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Wed, 26 Jun 2024 16:03:01 -0400 Subject: [PATCH] Add `uv tool install --force` (#4501) Adds detection of existing entry points, avoiding clobbering entry points that were installed by another tool. If we see any existing entry point collisions, we'll stop instead of overwriting them. The `--force` flag can be used to opt-in to overwriting the files; we can't use `-f` because it's taken by `--find-links` which is silly. The `--force` flag also implies replacing a tool previously installed by uv (the environment is rebuilt). Similarly, #4504 adds support for reinstalls that _will not_ clobber entry points managed by other tools. --- 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 | 139 ++++++++++++++++++++++--- 6 files changed, 205 insertions(+), 24 deletions(-) diff --git a/crates/uv-cli/src/lib.rs b/crates/uv-cli/src/lib.rs index d582c1b5b..d1056249b 100644 --- a/crates/uv-cli/src/lib.rs +++ b/crates/uv-cli/src/lib.rs @@ -1912,6 +1912,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 72e3631ee..e08cb8718 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 c94437c7b..0be46e1cc 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 2a205fdb8..a2d3a158e 100644 --- a/crates/uv/src/main.rs +++ b/crates/uv/src/main.rs @@ -815,6 +815,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 c984a5a5f..cf7dd23a3 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 77520ebff..d2c27371a 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,13 +262,20 @@ 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!(context.filters(), context.tool_install() + 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: true - exit_code: 0 + success: false + exit_code: 2 ----- stdout ----- ----- stderr ----- @@ -282,24 +289,124 @@ fn tool_install_entry_point_exists() { + packaging==24.0 + pathspec==0.12.1 + platformdirs==4.2.0 + error: Entry point for tool already exists: black (use `--force` to overwrite) "###); - // TODO(zanieb): We happily overwrite entry points by default right now - // https://github.com/astral-sh/uv/pull/4501 should resolve this + // We should delete the virtual environment + assert!(!tool_dir.child("black").exists()); - // We should not create a virtual environment - // assert!(tool_dir.child("black").exists()); + // We should not write a tools entry + assert!(!tool_dir.join("tools.toml").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(format!("black{}", std::env::consts::EXE_SUFFIX))).unwrap(), @""); - // 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(format!("blackd{}", std::env::consts::EXE_SUFFIX)) + .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 + exit_code: 0 + ----- 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 + "###); + + tool_dir.child("black").assert(predicate::path::is_dir()); + + 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"] } + "###); + }); + + let executable = bin_dir.child(format!("black{}", std::env::consts::EXE_SUFFIX)); + assert!(executable.exists()); + + // 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`