Merge #72

72: On clean update binary proxies r=Hoverbear a=amanjeev

This was quick when you figure out where things are already done. A small one.

### Testing

#### Start with a clean state.json. 

For best results, delete the entire criticalup installation directory.

#### Install project 1

Use this manifest to install first project. Name this file project1.toml.

```toml
manifest-version = 1

[products.ferrocene]
release = "stable-24.11.0"
packages = [
    "cargo-${rustc-host}",
    "rustc-${rustc-host}",
    "flip-link-${rustc-host}",
]
```

#### Install project 2

Use this manifest to install first project. Name this file project2.toml.

This is same as above except flip-link is removed. 

```toml
manifest-version = 1

[products.ferrocene]
release = "stable-24.11.0"
packages = [
    "cargo-${rustc-host}",
    "rustc-${rustc-host}",
]
```

#### Check binary proxies

You should see following binary proxies:

- cargo
- flip-link
- rustc
- rustdoc
- rust-gdb
- rust-gdbgui
- rust-lldb

#### Remove project 1

```sh
cargo run -q remove --project project1.toml.
```

This will remove the project from state file but won't clean up. For cleaning up, we run clean command next.

#### Run clean command

```sh
cargo run -q clean
```

#### You should see flip-link binary proxy removed; you should see the following binary proxies.

- cargo
- rustc
- rustdoc
- rust-gdb
- rust-gdbgui
- rust-lldb

This should show that only flip-link was removed because all other binary proxies were still being referenced by another installation (project 2).




Co-authored-by: Amanjeev Sethi <amanjeev.sethi@ferrous-systems.com>
Co-authored-by: Ana Hobden <operator@hoverbear.org>

CHANGELOG.md

@@ -11,6 +11,10 @@ All notable changes to this project will be documented in this file.
 
 - Release instructions to README.
 
+### Fixed
+
+- Running Clean command now ensures that there are no leftover unused binary proxies.
+
 ## [1.2.0] - 2024-11-25
 
 ### Changed

crates/criticalup-cli/src/commands/clean.rs

@@ -4,6 +4,7 @@
 use std::path::{Path, PathBuf};
 use tokio::fs;
 
+use criticalup_core::binary_proxies;
 use criticalup_core::project_manifest::InstallationId;
 use criticalup_core::state::State;
 
@@ -16,6 +17,8 @@ pub(crate) async fn run(ctx: &Context) -> Result<(), Error> {
 
     delete_cache_directory(&ctx.config.paths.cache_dir).await?;
     delete_unused_installations(installations_dir, &state).await?;
+    // Deletes unused binary proxies after state cleanup.
+    binary_proxies::update(&ctx.config, &state, &std::env::current_exe()?).await?;
     delete_untracked_installation_dirs(installations_dir, state).await?;
 
     Ok(())

crates/criticalup-cli/src/lib.rs

@@ -151,7 +151,7 @@ enum Commands {
         offline: bool,
     },
 
-    /// Delete all unused and untracked installations
+    /// Delete cache and unused installations
     Clean,
 
     /// Run a command for a given toolchain

crates/criticalup-cli/tests/snapshots/cli__clean__clean_deletes_only_unused_installations_also_from_disk.snap

@@ -1,6 +1,7 @@
 ---
 source: crates/criticalup-cli/tests/cli/clean.rs
 expression: repr
+snapshot_kind: text
 ---
 exit: exit status: 0
 

crates/criticalup-cli/tests/snapshots/cli__clean__help_message.snap

@@ -1,14 +1,15 @@
 ---
 source: crates/criticalup-cli/tests/cli/clean.rs
 expression: repr
+snapshot_kind: text
 ---
 exit: exit status: 0
 
 empty stdout
 
 stderr
 ------
-Delete all unused and untracked installations
+Delete cache and unused installations
 
 Usage:
   criticalup-test clean [OPTIONS]

crates/criticalup-cli/tests/snapshots/cli__root__no_args.snap

@@ -1,6 +1,7 @@
 ---
 source: crates/criticalup-cli/tests/cli/root.rs
 expression: repr
+snapshot_kind: text
 ---
 exit: exit status: 1
 
@@ -16,7 +17,7 @@ Usage:
 Commands:
   auth     Show and change authentication with the download server
   install  Install the toolchain for the given project based on the manifest `criticalup.toml`
-  clean    Delete all unused and untracked installations
+  clean    Delete cache and unused installations
   run      Run a command for a given toolchain
   remove   Delete all the products specified in the manifest `criticalup.toml`
   verify   Verify a given toolchain

crates/criticalup-core/src/binary_proxies.rs

@@ -27,6 +27,12 @@ pub async fn update(
     state: &State,
     proxy_binary: &Path,
 ) -> Result<(), BinaryProxyUpdateError> {
+    if proxy_binary.is_dir() {
+        return Err(BinaryProxyUpdateError::ProxyBinaryShouldNotBeDir(
+            proxy_binary.into(),
+        ));
+    }
+
     let mut expected_proxies = state
         .all_binary_proxy_names()
         .into_iter()
@@ -352,4 +358,26 @@ mod tests {
             );
         }
     }
+
+    #[tokio::test]
+    async fn update_should_not_accept_dir_for_proxy_binary_arg() {
+        let test_env = TestEnvironment::with().state().prepare().await;
+        let state = test_env.state();
+
+        // If the directory does not exist, the `.is_dir()` method fails to recognize it's a dir.
+        tokio::fs::create_dir_all(&test_env.config().paths.proxies_dir)
+            .await
+            .unwrap();
+        assert!(test_env.config().paths.proxies_dir.exists());
+
+        assert!(matches!(
+            update(
+                test_env.config(),
+                state,
+                &test_env.config().paths.proxies_dir
+            )
+            .await,
+            Err(BinaryProxyUpdateError::ProxyBinaryShouldNotBeDir(_))
+        ))
+    }
 }

crates/criticalup-core/src/errors.rs

@@ -186,4 +186,8 @@ pub enum BinaryProxyUpdateError {
     },
     #[error("Failed to create the parent directory {}.", .0.display())]
     ParentDirectoryCreationFailed(PathBuf, #[source] std::io::Error),
+    #[error(
+        "The path of the proxy binary provided is a directory but needs to be a file or symlink."
+    )]
+    ProxyBinaryShouldNotBeDir(PathBuf),
 }