From 1be25f81b53945c5dc86d79ca1f6eec52c455ce4 Mon Sep 17 00:00:00 2001
From: Jiahao XU <Jiahao_XU@outlook.com>
Date: Thu, 12 Jan 2023 16:45:36 +1100
Subject: [PATCH] Fix `FileLock` for `.crates.toml` not dropped (#697)

* Fix `FileLock` for `.crates.toml` not dropped

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

* Rm unneccessary ret type annotation for the closure

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

* Add regression test

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
---
 crates/bin/src/entry.rs | 81 ++++++++++++++++++++++++++---------------
 e2e-tests/strategies.sh |  3 ++
 2 files changed, 54 insertions(+), 30 deletions(-)

diff --git a/crates/bin/src/entry.rs b/crates/bin/src/entry.rs
index 020fedb2..69da24ee 100644
--- a/crates/bin/src/entry.rs
+++ b/crates/bin/src/entry.rs
@@ -7,7 +7,7 @@ use binstalk::{
     helpers::{jobserver_client::LazyJobserverClient, remote::Client, tasks::AutoAbortJoinHandle},
     ops::{
         self,
-        resolve::{CrateName, Resolution, VersionReqExt},
+        resolve::{CrateName, Resolution, ResolutionFetch, VersionReqExt},
         Resolver,
     },
 };
@@ -146,35 +146,14 @@ pub async fn install_crates(args: Args, jobserver_client: LazyJobserverClient) -
         confirm().await?;
     }
 
-    if !resolution_fetchs.is_empty() {
-        if dry_run {
-            info!("Dry-run: Not proceeding to install fetched binaries");
-        } else {
-            let f = || -> Result<()> {
-                let metadata_vec = resolution_fetchs
-                    .into_iter()
-                    .map(|fetch| fetch.install(&binstall_opts))
-                    .collect::<Result<Vec<_>, BinstallError>>()?;
-
-                if let Some(manifests) = manifests {
-                    manifests.update(metadata_vec)?;
-                }
-
-                if no_cleanup {
-                    // Consume temp_dir without removing it from fs.
-                    temp_dir.into_path();
-                } else {
-                    temp_dir.close().unwrap_or_else(|err| {
-                        warn!("Failed to clean up some resources: {err}");
-                    });
-                }
-
-                Ok(())
-            };
-
-            block_in_place(f)?;
-        }
-    }
+    do_install_fetches(
+        resolution_fetchs,
+        manifests,
+        &binstall_opts,
+        dry_run,
+        temp_dir,
+        no_cleanup,
+    )?;
 
     let tasks: Vec<_> = resolution_sources
         .into_iter()
@@ -276,3 +255,45 @@ fn filter_out_installed_crates(
         }
     }))
 }
+
+#[allow(clippy::vec_box)]
+fn do_install_fetches(
+    resolution_fetchs: Vec<Box<ResolutionFetch>>,
+    // Take manifests by value to drop the `FileLock`.
+    manifests: Option<Manifests>,
+    binstall_opts: &ops::Options,
+    dry_run: bool,
+    temp_dir: tempfile::TempDir,
+    no_cleanup: bool,
+) -> Result<()> {
+    if resolution_fetchs.is_empty() {
+        return Ok(());
+    }
+
+    if dry_run {
+        info!("Dry-run: Not proceeding to install fetched binaries");
+        return Ok(());
+    }
+
+    block_in_place(|| {
+        let metadata_vec = resolution_fetchs
+            .into_iter()
+            .map(|fetch| fetch.install(binstall_opts))
+            .collect::<Result<Vec<_>, BinstallError>>()?;
+
+        if let Some(manifests) = manifests {
+            manifests.update(metadata_vec)?;
+        }
+
+        if no_cleanup {
+            // Consume temp_dir without removing it from fs.
+            temp_dir.into_path();
+        } else {
+            temp_dir.close().unwrap_or_else(|err| {
+                warn!("Failed to clean up some resources: {err}");
+            });
+        }
+
+        Ok(())
+    })
+}
diff --git a/e2e-tests/strategies.sh b/e2e-tests/strategies.sh
index 741cdddd..649dfec7 100755
--- a/e2e-tests/strategies.sh
+++ b/e2e-tests/strategies.sh
@@ -24,3 +24,6 @@ if [ "$exit_code" != 94 ]; then
     echo "Expected exit code 94, but actual exit code $exit_code"
     exit 1
 fi
+
+## Test compile-only strategy
+"./$1" binstall --no-confirm --strategies compile cargo-quickinstall