From 76fa5b0dd366df6110d30eb3aabdb0040f88fcbc Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 31 Jul 2024 14:51:22 -0400 Subject: [PATCH] Remove lingering executables after failed installs --- crates/uv/src/commands/tool/install.rs | 48 +++++++-- crates/uv/tests/tool_install.rs | 137 +++++++++++++++++++++++++ 2 files changed, 177 insertions(+), 8 deletions(-) diff --git a/crates/uv/src/commands/tool/install.rs b/crates/uv/src/commands/tool/install.rs index 6bf37f0c8312..0f9f4e521e5e 100644 --- a/crates/uv/src/commands/tool/install.rs +++ b/crates/uv/src/commands/tool/install.rs @@ -196,10 +196,10 @@ pub(crate) async fn install( // // (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 (existing_tool_receipt, reinstall_entry_points) = + let (existing_tool_receipt, invalid_tool_receipt) = match installed_tools.get_tool_receipt(&from.name) { Ok(None) => (None, false), - Ok(Some(receipt)) => (Some(receipt), true), + Ok(Some(receipt)) => (Some(receipt), false), Err(_) => { // If the tool is not installed properly, remove the environment and continue. match installed_tools.remove_environment(&from.name) { @@ -276,7 +276,7 @@ pub(crate) async fn install( // entrypoints always contain an absolute path to the relevant Python interpreter, which would // be invalidated by moving the environment. let environment = if let Some(environment) = existing_environment { - update_environment( + let environment = update_environment( environment, spec, &settings, @@ -288,7 +288,15 @@ pub(crate) async fn install( cache, printer, ) - .await? + .await?; + + // At this point, we updated the existing environment, so we should remove any of its + // existing executables. + if let Some(existing_receipt) = existing_tool_receipt { + remove_entrypoints(&existing_receipt); + } + + environment } else { // If we're creating a new environment, ensure that we can resolve the requirements prior // to removing any existing tools. @@ -308,6 +316,12 @@ pub(crate) async fn install( let environment = installed_tools.create_environment(&from.name, interpreter)?; + // At this point, we removed any existing environment, so we should remove any of its + // executables. + if let Some(existing_receipt) = existing_tool_receipt { + remove_entrypoints(&existing_receipt); + } + // Sync the environment with the resolved requirements. sync_environment( environment, @@ -370,8 +384,9 @@ pub(crate) async fn install( hint_executable_from_dependency(&from, &environment, printer)?; - // Clean up the environment we just created + // Clean up the environment we just created. installed_tools.remove_environment(&from.name)?; + return Ok(ExitStatus::Failure); } @@ -381,9 +396,9 @@ pub(crate) async fn install( .filter(|(_, _, target_path)| target_path.exists()) .peekable(); - // Note we use `reinstall_entry_points` here instead of `reinstall`; requesting reinstall - // will _not_ remove existing entry points when they are not managed by uv. - if force || reinstall_entry_points { + // Ignore any existing entrypoints if the user passed `--force`, or the existing recept was + // broken. + if force || invalid_tool_receipt { for (name, _, target) in existing_entry_points { debug!("Removing existing executable: `{name}`"); fs_err::remove_file(target)?; @@ -478,6 +493,23 @@ pub(crate) async fn install( Ok(ExitStatus::Success) } +/// Remove any entrypoints attached to the [`Tool`]. +fn remove_entrypoints(tool: &Tool) { + for executable in tool + .entrypoints() + .iter() + .map(|entrypoint| &entrypoint.install_path) + { + debug!("Removing executable: `{}`", executable.simplified_display()); + if let Err(err) = fs_err::remove_file(executable) { + warn!( + "Failed to remove executable: `{}`: {err}", + executable.simplified_display() + ); + } + } +} + /// Displays a hint if an executable matching the package name can be found in a dependency of the package. fn hint_executable_from_dependency( from: &Requirement, diff --git a/crates/uv/tests/tool_install.rs b/crates/uv/tests/tool_install.rs index 3540d85fc804..3ca3259c901a 100644 --- a/crates/uv/tests/tool_install.rs +++ b/crates/uv/tests/tool_install.rs @@ -467,6 +467,143 @@ fn tool_install_editable() { }); } +/// Ensure that we remove any existing entrypoints upon error. +#[test] +fn tool_install_remove_on_empty() -> Result<()> { + let context = TestContext::new("3.12").with_filtered_exe_suffix(); + let tool_dir = context.temp_dir.child("tools"); + let bin_dir = context.temp_dir.child("bin"); + + // Request `black`. It should reinstall from the registry. + 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()) + .env("PATH", 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] + 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 + Installed 2 executables: 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 = [{ name = "black" }] + entrypoints = [ + { name = "black", install-path = "[TEMP_DIR]/bin/black" }, + { name = "blackd", install-path = "[TEMP_DIR]/bin/blackd" }, + ] + "###); + }); + + // Install `black` as an editable package, but without any entrypoints. + let black = context.temp_dir.child("black"); + fs_err::create_dir_all(black.path())?; + + let pyproject_toml = black.child("pyproject.toml"); + pyproject_toml.write_str(indoc! {r#" + [project] + name = "black" + version = "0.1.0" + description = "Black without any entrypoints" + authors = [] + dependencies = [] + requires-python = ">=3.11,<3.13" + + [build-system] + requires = ["hatchling"] + build-backend = "hatchling.build" + "# + })?; + + let src = black.child("src").child("black"); + fs_err::create_dir_all(src.path())?; + + let init = src.child("__init__.py"); + init.touch()?; + + uv_snapshot!(context.filters(), context.tool_install() + .arg("-e") + .arg(black.path()) + .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: false + exit_code: 1 + ----- stdout ----- + No executables are provided by `black` + + ----- stderr ----- + warning: `uv tool install` is experimental and may change without warning + Resolved 1 package in [TIME] + Prepared 1 package in [TIME] + Uninstalled 6 packages in [TIME] + Installed 1 package in [TIME] + - black==24.3.0 + + black==0.1.0 (from file://[TEMP_DIR]/black) + - click==8.1.7 + - mypy-extensions==1.0.0 + - packaging==24.0 + - pathspec==0.12.1 + - platformdirs==4.2.0 + "###); + + // Re-request `black`. It should reinstall, without requiring `--force`. + 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()) + .env("PATH", 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 + Installed 2 executables: 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 = [{ name = "black" }] + entrypoints = [ + { name = "black", install-path = "[TEMP_DIR]/bin/black" }, + { name = "blackd", install-path = "[TEMP_DIR]/bin/blackd" }, + ] + "###); + }); + + Ok(()) +} + /// Test an editable installation of a tool using `--from`. #[test] fn tool_install_editable_from() {