Return a list of files written to disk in binstalk_downloader::download::Download::and_extract (#856)

to avoid collecting extracted files from disk again in resolution stage.

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

crates/binstalk/src/bins.rs

@@ -13,6 +13,7 @@ use tracing::debug;
 use crate::{
     errors::BinstallError,
     fs::{atomic_install, atomic_symlink_file},
+    helpers::download::ExtractedFiles,
     manifests::cargo_toml_binstall::{PkgFmt, PkgMeta},
 };
 
@@ -30,7 +31,7 @@ fn is_valid_path(path: &Path) -> bool {
 
 /// Must be called after the archive is downloaded and extracted.
 /// This function might uses blocking I/O.
-pub fn infer_bin_dir_template(data: &Data) -> Cow<'static, str> {
+pub fn infer_bin_dir_template(data: &Data, extracted_files: &ExtractedFiles) -> Cow<'static, str> {
     let name = data.name;
     let target = data.target;
     let version = data.version;
@@ -55,7 +56,11 @@ pub fn infer_bin_dir_template(data: &Data) -> Cow<'static, str> {
     gen_possible_dirs
         .into_iter()
         .map(|gen_possible_dir| gen_possible_dir(name, target, version))
-        .find(|dirname| data.bin_path.join(dirname).is_dir())
+        .find(|dirname| {
+            extracted_files
+                .get_dir(&data.bin_path.join(dirname))
+                .is_some()
+        })
         .map(|mut dir| {
             dir.reserve_exact(1 + default_bin_dir_template.len());
             dir += "/";
@@ -69,6 +74,7 @@ pub fn infer_bin_dir_template(data: &Data) -> Cow<'static, str> {
 pub struct BinFile {
     pub base_name: CompactString,
     pub source: PathBuf,
+    pub archive_source_path: PathBuf,
     pub dest: PathBuf,
     pub link: Option<PathBuf>,
 }
@@ -97,8 +103,11 @@ impl BinFile {
             binary_ext,
         };
 
-        let source = if data.meta.pkg_fmt == Some(PkgFmt::Bin) {
-            data.bin_path.to_path_buf()
+        let (source, archive_source_path) = if data.meta.pkg_fmt == Some(PkgFmt::Bin) {
+            (
+                data.bin_path.to_path_buf(),
+                data.bin_path.file_name().unwrap().into(),
+            )
         } else {
             // Generate install paths
             // Source path is the download dir + the generated binary path
@@ -116,7 +125,7 @@ impl BinFile {
                 });
             }
 
-            data.bin_path.join(&path_normalized)
+            (data.bin_path.join(&path_normalized), path_normalized)
         };
 
         // Destination at install dir + base-name{.extension}
@@ -143,6 +152,7 @@ impl BinFile {
         Ok(Self {
             base_name: format_compact!("{base_name}{binary_ext}"),
             source,
+            archive_source_path,
             dest,
             link,
         })
@@ -165,16 +175,21 @@ impl BinFile {
     }
 
     /// Return `Ok` if the source exists, otherwise `Err`.
-    pub fn check_source_exists(&self) -> Result<(), BinstallError> {
-        if !self.source.try_exists()? {
-            Err(BinstallError::BinFileNotFound(self.source.clone()))
-        } else {
+    pub fn check_source_exists(
+        &self,
+        extracted_files: &ExtractedFiles,
+    ) -> Result<(), BinstallError> {
+        if extracted_files.has_file(&self.archive_source_path) {
             Ok(())
+        } else {
+            Err(BinstallError::BinFileNotFound(self.source.clone()))
         }
     }
 
     pub fn install_bin(&self) -> Result<(), BinstallError> {
-        self.check_source_exists()?;
+        if !self.source.try_exists()? {
+            return Err(BinstallError::BinFileNotFound(self.source.clone()));
+        }
 
         debug!(
             "Atomically install file from '{}' to '{}'",

crates/binstalk/src/fetchers.rs

@@ -8,7 +8,10 @@ use url::Url;
 
 use crate::{
     errors::BinstallError,
-    helpers::{gh_api_client::GhApiClient, remote::Client, tasks::AutoAbortJoinHandle},
+    helpers::{
+        download::ExtractedFiles, gh_api_client::GhApiClient, remote::Client,
+        tasks::AutoAbortJoinHandle,
+    },
     manifests::cargo_toml_binstall::{PkgFmt, PkgMeta},
 };
 
@@ -29,7 +32,7 @@ pub trait Fetcher: Send + Sync {
         Self: Sized;
 
     /// Fetch a package and extract
-    async fn fetch_and_extract(&self, dst: &Path) -> Result<(), BinstallError>;
+    async fn fetch_and_extract(&self, dst: &Path) -> Result<ExtractedFiles, BinstallError>;
 
     /// Find the package, if it is available for download
     ///

crates/binstalk/src/fetchers/gh_crate_meta.rs

@@ -13,7 +13,7 @@ use url::Url;
 use crate::{
     errors::{BinstallError, InvalidPkgFmtError},
     helpers::{
-        download::Download,
+        download::{Download, ExtractedFiles},
         futures_resolver::FuturesResolver,
         gh_api_client::{GhApiClient, GhReleaseArtifact, HasReleaseArtifact},
         remote::Client,
@@ -216,7 +216,7 @@ impl super::Fetcher for GhCrateMeta {
         })
     }
 
-    async fn fetch_and_extract(&self, dst: &Path) -> Result<(), BinstallError> {
+    async fn fetch_and_extract(&self, dst: &Path) -> Result<ExtractedFiles, BinstallError> {
         let (url, pkg_fmt) = self.resolution.get().unwrap(); // find() is called first
         debug!("Downloading package from: '{url}' dst:{dst:?} fmt:{pkg_fmt:?}");
         Ok(Download::new(self.client.clone(), url.clone())

crates/binstalk/src/fetchers/quickinstall.rs

@@ -7,7 +7,10 @@ use url::Url;
 use crate::{
     errors::BinstallError,
     helpers::{
-        download::Download, gh_api_client::GhApiClient, remote::Client, tasks::AutoAbortJoinHandle,
+        download::{Download, ExtractedFiles},
+        gh_api_client::GhApiClient,
+        remote::Client,
+        tasks::AutoAbortJoinHandle,
     },
     manifests::cargo_toml_binstall::{PkgFmt, PkgMeta},
 };
@@ -63,7 +66,7 @@ impl super::Fetcher for QuickInstall {
         })
     }
 
-    async fn fetch_and_extract(&self, dst: &Path) -> Result<(), BinstallError> {
+    async fn fetch_and_extract(&self, dst: &Path) -> Result<ExtractedFiles, BinstallError> {
         let url = self.package_url();
         debug!("Downloading package from: '{url}'");
         Ok(Download::new(self.client.clone(), Url::parse(&url)?)

crates/binstalk/src/ops/resolve.rs

@@ -21,7 +21,7 @@ use crate::{
     drivers::fetch_crate_cratesio,
     errors::{BinstallError, VersionParseError},
     fetchers::{Data, Fetcher, TargetData},
-    helpers::remote::Client,
+    helpers::{download::ExtractedFiles, remote::Client},
     manifests::cargo_toml_binstall::{Meta, PkgMeta, PkgOverride},
 };
 
@@ -200,7 +200,7 @@ async fn download_extract_and_verify(
 ) -> Result<Vec<bins::BinFile>, BinstallError> {
     // Download and extract it.
     // If that fails, then ignore this fetcher.
-    fetcher.fetch_and_extract(bin_path).await?;
+    let extracted_files = fetcher.fetch_and_extract(bin_path).await?;
 
     // Build final metadata
     let meta = fetcher.target_meta();
@@ -230,48 +230,47 @@ async fn download_extract_and_verify(
     }
 
     // Verify that all non-optional bin_files exist
-    block_in_place(|| {
-        let bin_files = collect_bin_files(
-            fetcher,
-            package_info,
-            meta,
-            bin_path,
-            install_path,
-            no_symlinks,
-        )?;
+    let bin_files = collect_bin_files(
+        fetcher,
+        package_info,
+        meta,
+        bin_path,
+        install_path,
+        no_symlinks,
+        &extracted_files,
+    )?;
 
-        let name = &package_info.name;
+    let name = &package_info.name;
 
-        package_info
-            .binaries
-            .iter()
-            .zip(bin_files)
-            .filter_map(|(bin, bin_file)| {
-                match bin_file.check_source_exists() {
-                    Ok(()) => Some(Ok(bin_file)),
+    package_info
+        .binaries
+        .iter()
+        .zip(bin_files)
+        .filter_map(|(bin, bin_file)| {
+            match bin_file.check_source_exists(&extracted_files) {
+                Ok(()) => Some(Ok(bin_file)),
 
-                    // This binary is optional
-                    Err(err) => {
-                        let required_features = &bin.required_features;
+                // This binary is optional
+                Err(err) => {
+                    let required_features = &bin.required_features;
 
-                        if required_features.is_empty() {
-                            // This bin is not optional, error
-                            Some(Err(err))
-                        } else {
-                            // Optional, print a warning and continue.
-                            let bin_name = bin.name.as_str();
-                            let features = required_features.iter().format(",");
-                            warn!(
-                                "When resolving {name} bin {bin_name} is not found. \
+                    if required_features.is_empty() {
+                        // This bin is not optional, error
+                        Some(Err(err))
+                    } else {
+                        // Optional, print a warning and continue.
+                        let bin_name = bin.name.as_str();
+                        let features = required_features.iter().format(",");
+                        warn!(
+                            "When resolving {name} bin {bin_name} is not found. \
                                 But since it requies features {features}, this bin is ignored."
-                            );
-                            None
-                        }
+                        );
+                        None
                     }
                 }
-            })
-            .collect::<Result<Vec<bins::BinFile>, BinstallError>>()
-    })
+            }
+        })
+        .collect::<Result<Vec<bins::BinFile>, BinstallError>>()
 }
 
 fn collect_bin_files(
@@ -281,6 +280,7 @@ fn collect_bin_files(
     bin_path: &Path,
     install_path: &Path,
     no_symlinks: bool,
+    extracted_files: &ExtractedFiles,
 ) -> Result<Vec<bins::BinFile>, BinstallError> {
     // List files to be installed
     // based on those found via Cargo.toml
@@ -299,7 +299,7 @@ fn collect_bin_files(
         .bin_dir
         .as_deref()
         .map(Cow::Borrowed)
-        .unwrap_or_else(|| bins::infer_bin_dir_template(&bin_data));
+        .unwrap_or_else(|| bins::infer_bin_dir_template(&bin_data, extracted_files));
 
     let mut tt = TinyTemplate::new();