Remove lingering executables after failed installs

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,

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() {