Merge #73

73: Clean command: removes installation from state if directory missing. r=Hoverbear a=amanjeev

This was a bug that if the directory of the installation is missing from the file system, and the installation is still present in the state file, the clean command will not remove the installation from the state file. This was certainly a problem because if there is accidental deletion of the toolchain directory, we need to be able to clean up the state file too.


#### 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}",
]
```

#### Install this project

```
criticalup install
```

#### Check state.json

state.json file should contain an installation with ID "c8f887068792bbf1f09ce4b6ddbe84fd7d9dcde18407d608968c26b89862d15e".

#### Delete the installation directory

In the appropriate location of toolchain installations, find the directory where this is installed and delete the toolchain of this project only. 

DO NOT delete the root directory of CriticalUp configs and installation. 

On Linux, you can see this at `~/.local/share/criticalup/toolchains/c8f887068792bbf1f09ce4b6ddbe84fd7d9dcde18407d608968c26b89862d15e/`.

#### Run clean

```
criticalup clean
```

Result: you should see that the state.json file does not have the installation of the above.

Co-authored-by: Amanjeev Sethi <amanjeev.sethi@ferrous-systems.com>

crates/criticalup-cli/src/arg.rs

@@ -24,7 +24,7 @@ pub(crate) struct Instrumentation {
     pub(crate) log_level: Vec<Directive>,
 }
 
-impl<'a> Instrumentation {
+impl Instrumentation {
     pub(crate) fn log_level(&self) -> String {
         match self.verbose {
             0 => "info",
@@ -34,7 +34,7 @@ impl<'a> Instrumentation {
         .to_string()
     }
 
-    pub(crate) async fn setup<'b: 'a>(&'b self, binary_name: &str) -> Result<(), crate::Error> {
+    pub(crate) async fn setup(&self, binary_name: &str) -> Result<(), crate::Error> {
         let filter_layer = self.filter_layer(binary_name)?;
 
         if self.verbose != 0 {

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

@@ -35,10 +35,23 @@ async fn delete_cache_directory(cache_dir: &Path) -> Result<(), Error> {
 /// Deletes installation from `State` with `InstallationId`s that have empty manifest section, and
 /// deletes the installation directory from the disk if present.
 async fn delete_unused_installations(installations_dir: &Path, state: &State) -> Result<(), Error> {
+    // We need to list all the available installations on the disk first, so we can check which
+    // installations in state file are absent from the disk.
+    let mut all_installations_on_disk: Vec<InstallationId> = Vec::new();
+    let mut entries = fs::read_dir(installations_dir).await?;
+    while let Some(item) = entries.next_entry().await? {
+        if item.file_type().await?.is_dir() {
+            let installation_dir_name = item.file_name();
+            if let Some(name) = installation_dir_name.to_str() {
+                all_installations_on_disk.push(InstallationId(name.into()));
+            }
+        }
+    }
+
     let unused_installations: Vec<InstallationId> = state
         .installations()
         .iter()
-        .filter(|item| item.1.manifests().is_empty())
+        .filter(|item| item.1.manifests().is_empty() || !all_installations_on_disk.contains(item.0))
         .map(|item| item.0.to_owned())
         .collect();
 
@@ -59,8 +72,8 @@ async fn delete_unused_installations(installations_dir: &Path, state: &State) ->
         // Remove installation directory from physical location.
         let installation_dir_to_delete = installations_dir.join(&installation.0);
         if installation_dir_to_delete.exists() {
-            tracing::info!(
-                "deleting unused installation directory {}",
+            tracing::debug!(
+                "Deleting unused installation directory {}",
                 &installation_dir_to_delete.display()
             );
             fs::remove_dir_all(&installation_dir_to_delete)
@@ -90,7 +103,7 @@ async fn delete_untracked_installation_dirs(
                 if !installations_in_state.contains_key(&InstallationId(name.into())) {
                     are_untracked_installation_dirs_present = true;
                     tracing::info!(
-                        "deleting untracked installation directory {}",
+                        "Deleting untracked installation directory {}",
                         item.path().to_path_buf().display()
                     );
 
@@ -106,7 +119,7 @@ async fn delete_untracked_installation_dirs(
     }
 
     if !are_untracked_installation_dirs_present {
-        tracing::info!("no untracked installation directories found",);
+        tracing::info!("No untracked installation directories found",);
     }
 
     Ok(())

crates/criticalup-cli/tests/cli/auth_set.rs

@@ -36,9 +36,12 @@ macro_rules! run_cmd {
             .expect("failed to execute command");
         match &$expected {
             Some(expected) => {
-                // this regex replacement dance is required because this nested macro tests
+                // Since 1.84, stable Rust Clippy will complain about Regex inside a for loop.
+                // Unblocking ourselves by ignoring this warning in tests.
+                #[allow(clippy::regex_creation_in_loops)]
+                // This regex replacement dance is required because this nested macro tests
                 // set is instantiating the test server twice which means each run gives
-                // a different local port. we replace with a stable port just for this test.
+                // a different local port. We replace with a stable port just for this test.
                 let re = Regex::new(r"127.0.0.1:\d+").expect("regex creation failed.");
                 let left_str = String::from_utf8(out.stderr.clone())
                     .expect("string creation from bytes failed.");

crates/criticalup-cli/tests/cli/clean.rs

@@ -17,13 +17,16 @@ async fn help_message() {
 #[tokio::test]
 async fn clean_deletes_only_unused_installations() {
     let test_env = TestEnvironment::prepare().await;
-
+    let root = test_env.root();
+    let toolchains_dir = root.join("toolchains");
+    fs::create_dir_all(&toolchains_dir).unwrap();
     let installation_id_1 = "installation_id_1";
     let installation_id_2 = "installation_id_2";
     let installation_id_3 = "installation_id_3";
 
     let root = test_env.root().join("state.json");
     let mut state_file = std::fs::File::create(&root).unwrap();
+
     let content = json!({
         "version": 1,
         "authentication_token": "criticalup_token_45_hahaha",
@@ -54,6 +57,14 @@ async fn clean_deletes_only_unused_installations() {
     .to_string();
     state_file.write_all(content.as_bytes()).unwrap();
 
+    fs::create_dir_all(toolchains_dir.join(installation_id_1)).unwrap();
+    fs::create_dir_all(toolchains_dir.join(installation_id_2)).unwrap();
+    fs::create_dir_all(toolchains_dir.join(installation_id_3)).unwrap();
+
+    assert!(toolchains_dir.join(installation_id_1).exists());
+    assert!(toolchains_dir.join(installation_id_2).exists());
+    assert!(toolchains_dir.join(installation_id_3).exists());
+
     assert_output!(test_env.cmd().args(["clean"]));
 
     let state_file_actual: Value =
@@ -255,3 +266,93 @@ async fn removes_unused_installations_from_disk_that_do_not_have_state() {
     assert!(!toolchains_dir.join(installation_id_2).exists()); // Does not exist.
     assert!(!toolchains_dir.join(installation_id_3).exists()); // Does not exist.
 }
+
+/// Remove the installation from the state, if the installation directory does not exist. This is
+/// true even if the installation in the state has a manifest.
+#[tokio::test]
+async fn clean_deletes_only_unused_installations_that_are_not_on_disk() {
+    let test_env = TestEnvironment::prepare().await;
+    let root = test_env.root();
+    let toolchains_dir = root.join("toolchains");
+    fs::create_dir_all(&toolchains_dir).unwrap();
+
+    let installation_id_1 = "installation_id_1";
+    let installation_id_2 = "installation_id_2";
+    let installation_id_3 = "installation_id_3";
+
+    let state_file_in_root = root.join("state.json");
+    let mut state_file = std::fs::File::create(&state_file_in_root).unwrap();
+    let content = json!({
+        "version": 1,
+        "authentication_token": "criticalup_token_45_hahaha",
+        "installations": {
+            installation_id_1: {
+                "binary_proxies": {
+                    "cargo": "/path/toolchains/bin/cargo"
+                },
+                "manifests": [
+                    "/path/to/proj/1/criticalup.toml",
+                    "/path/to/proj/2/criticalup.toml"
+                ]
+            },
+            installation_id_2: {
+                "binary_proxies": {
+                    "cargo": "/path/toolchains/bin/cargo"
+                },
+                "manifests": ["/path/to/proj/3/criticalup.toml"]
+            },
+            installation_id_3: {
+                "binary_proxies": {
+                    "cargo": "/path/toolchains/bin/rustc"
+                },
+                "manifests": []
+            }
+        }
+    })
+    .to_string();
+    state_file.write_all(content.as_bytes()).unwrap();
+
+    // Create only one of the corresponding physical directories of installations. In this case we
+    // create only Installation ID 1.
+    // TODO: We have to generate these by running `install` command, once tests for those are setup.
+    fs::create_dir_all(toolchains_dir.join(installation_id_1)).unwrap();
+
+    assert!(toolchains_dir.join(installation_id_1).exists());
+    assert!(!toolchains_dir.join(installation_id_2).exists());
+    assert!(!toolchains_dir.join(installation_id_3).exists());
+
+    // Run the `clean` command.
+    assert_output!(test_env.cmd().args(["clean"]));
+
+    // Test the actual values.
+    let state_file_actual: Value =
+        serde_json::from_reader(BufReader::new(File::open(&state_file_in_root).unwrap())).unwrap();
+
+    // "installation_id_2" is not present.
+    assert_eq!(
+        state_file_actual.pointer("/installations/installation_id_2"),
+        None
+    );
+    // "installation_id_3" is not present.
+    assert_eq!(
+        state_file_actual.pointer("/installations/installation_id_3"),
+        None
+    );
+    // "installation_id_1" is still present with correct values.
+    assert_eq!(
+        state_file_actual
+            .pointer("/installations/installation_id_1")
+            .unwrap(),
+        &json!({
+            "binary_proxies": {
+                "cargo": "/path/toolchains/bin/cargo"
+            },
+            "manifests": [
+                "/path/to/proj/1/criticalup.toml",
+                "/path/to/proj/2/criticalup.toml"
+            ]
+        })
+    );
+
+    assert!(toolchains_dir.join(installation_id_1).exists());
+}

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

@@ -2,13 +2,13 @@
 source: crates/criticalup-cli/tests/cli/clean.rs
 expression: repr
 ---
-exit: exit status: 1
+exit: exit status: 0
 
 empty stdout
 
 stderr
 ------
  INFO Deleting unused installation installation_id_2
  INFO Deleting unused installation installation_id_3
-error: No such file or directory (os error 2)
+ INFO No untracked installation directories found
 ------

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

@@ -1,7 +1,6 @@
 ---
 source: crates/criticalup-cli/tests/cli/clean.rs
 expression: repr
-snapshot_kind: text
 ---
 exit: exit status: 0
 
@@ -10,8 +9,6 @@ empty stdout
 stderr
 ------
  INFO Deleting unused installation installation_id_2
- INFO deleting unused installation directory /path/to/toolchain/installation/installation_id_2/
  INFO Deleting unused installation installation_id_3
- INFO deleting unused installation directory /path/to/toolchain/installation/installation_id_3/
- INFO no untracked installation directories found
+ INFO No untracked installation directories found
 ------

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

@@ -0,0 +1,14 @@
+---
+source: crates/criticalup-cli/tests/cli/clean.rs
+expression: repr
+---
+exit: exit status: 0
+
+empty stdout
+
+stderr
+------
+ INFO Deleting unused installation installation_id_2
+ INFO Deleting unused installation installation_id_3
+ INFO No untracked installation directories found
+------

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

@@ -9,6 +9,5 @@ empty stdout
 stderr
 ------
  INFO Deleting unused installation installation_id_2
- INFO deleting unused installation directory /path/to/toolchain/installation/installation_id_2/
- INFO deleting untracked installation directory /path/to/toolchain/installation/installation_id_3/
+ INFO Deleting untracked installation directory /path/to/toolchain/installation/installation_id_3/
 ------