Impl try multiple default bin dirs (#417)

From @passcod :

* Make bin-dir an Option
* Use cargo-release as a test case
* WIP: Try multiple default bin dirs
* Update bins.rs
* Update cargo_toml_binstall.rs

From @NobodyXu :
* Optimize & Prepare for support multi `bin-path`s
* Ensure quickinstall bin_dir work as usual.
* Rm dup entries in `FULL_FILENAMES`
* Add second version of `NOVERSION_FILENAMES`
* Impl multiple default `bin-dir`s
* Improve doc for `BinFile::from_product`
* Fix tests.sh

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Co-authored-by: Félix Saparelli <felix@passcod.name>

.github/scripts/tests.sh

@@ -7,10 +7,11 @@ unset CARGO_HOME
 
 # Install binaries using cargo-binstall
 # shellcheck disable=SC2086
-"./$1" binstall --log-level debug --no-confirm b3sum cargo-binstall cargo-watch
+"./$1" binstall --log-level debug --no-confirm b3sum cargo-release cargo-binstall cargo-watch
 
 # Test that the installed binaries can be run
 b3sum --version
+cargo-release release --version
 cargo-binstall --help >/dev/null
 cargo binstall --help >/dev/null
 cargo watch -V

crates/binstalk/src/bins.rs

@@ -1,4 +1,7 @@
-use std::path::{Path, PathBuf};
+use std::{
+    borrow::Cow,
+    path::{Path, PathBuf},
+};
 
 use cargo_toml::Product;
 use compact_str::CompactString;
@@ -20,6 +23,8 @@ pub struct BinFile {
 }
 
 impl BinFile {
+    /// Must be called after the archive is downloaded and extracted.
+    /// This function might uses blocking I/O.
     pub fn from_product(data: &Data, product: &Product) -> Result<Self, BinstallError> {
         let base_name = CompactString::from(product.name.clone().unwrap());
 
@@ -41,7 +46,38 @@ impl BinFile {
 
         // Generate install paths
         // Source path is the download dir + the generated binary path
-        let source_file_path = ctx.render(&data.meta.bin_dir)?;
+        let source_file_path = if let Some(bin_dir) = &data.meta.bin_dir {
+            ctx.render(bin_dir)?
+        } else {
+            let name = ctx.name;
+            let target = ctx.target;
+            let version = ctx.version;
+            let bin = ctx.bin;
+
+            // Make sure to update
+            // fetchers::gh_crate_meta::hosting::{FULL_FILENAMES,
+            // NOVERSION_FILENAMES} if you update this array.
+            let possible_dirs = [
+                format!("{name}-{target}-v{version}"),
+                format!("{name}-{target}-{version}"),
+                format!("{name}-{version}-{target}"),
+                format!("{name}-v{version}-{target}"),
+                format!("{name}-{target}"),
+                name.to_string(),
+            ];
+
+            let dir = possible_dirs
+                .into_iter()
+                .find(|dirname| Path::new(dirname).is_dir())
+                .map(Cow::Owned)
+                // Fallback to no dir
+                .unwrap_or_else(|| Cow::Borrowed("."));
+
+            debug!("Using dir = {dir}");
+
+            format!("{dir}/{bin}{binary_ext}")
+        };
+
         let source = if data.meta.pkg_fmt == Some(PkgFmt::Bin) {
             data.bin_path.clone()
         } else {

crates/binstalk/src/fetchers.rs

@@ -11,8 +11,8 @@ use crate::{
     manifests::cargo_toml_binstall::{PkgFmt, PkgMeta},
 };
 
-mod gh_crate_meta;
+pub(crate) mod gh_crate_meta;
-mod quickinstall;
+pub(crate) mod quickinstall;
 
 #[async_trait::async_trait]
 pub trait Fetcher: Send + Sync {

crates/binstalk/src/fetchers/gh_crate_meta.rs

@@ -22,7 +22,7 @@ use crate::{
 
 use super::Data;
 
-mod hosting;
+pub(crate) mod hosting;
 use hosting::RepositoryHost;
 
 pub struct GhCrateMeta {
@@ -143,10 +143,10 @@ impl super::Fetcher for GhCrateMeta {
     }
 
     async fn fetch_and_extract(&self, dst: &Path) -> Result<(), BinstallError> {
-        let (url, _pkg_fmt) = self.resolution.get().unwrap(); // find() is called first
+        let (url, pkg_fmt) = self.resolution.get().unwrap(); // find() is called first
-        debug!("Downloading package from: '{url}'");
+        debug!("Downloading package from: '{url}' dst:{dst:?} fmt:{pkg_fmt:?}");
         Download::new(self.client.clone(), url.clone())
-            .and_extract(self.pkg_fmt(), dst)
+            .and_extract(*pkg_fmt, dst)
             .await
     }
 

crates/binstalk/src/fetchers/gh_crate_meta/hosting.rs

@@ -11,6 +11,20 @@ pub enum RepositoryHost {
     Unknown,
 }
 
+/// Make sure to update possible_dirs in `bins::BinFile`
+/// if you modified FULL_FILENAMES or NOVERSION_FILENAMES.
+pub const FULL_FILENAMES: &[&str] = &[
+    "{ name }-{ target }-v{ version }.{ archive-format }",
+    "{ name }-{ target }-{ version }.{ archive-format }",
+    "{ name }-{ version }-{ target }.{ archive-format }",
+    "{ name }-v{ version }-{ target }.{ archive-format }",
+];
+
+pub const NOVERSION_FILENAMES: &[&str] = &[
+    "{ name }-{ target }.{ archive-format }",
+    "{ name }.{ archive-format }",
+];
+
 impl RepositoryHost {
     pub fn guess_git_hosting_services(repo: &Url) -> Result<Self, BinstallError> {
         use RepositoryHost::*;
@@ -27,35 +41,24 @@ impl RepositoryHost {
     pub fn get_default_pkg_url_template(self) -> Option<Vec<String>> {
         use RepositoryHost::*;
 
-        let full_filenames = &[
-            "{ name }-{ target }-v{ version }.{ archive-format }",
-            "{ name }-{ target }-{ version }.{ archive-format }",
-            "{ name }-{ version }-{ target }.{ archive-format }",
-            "{ name }-v{ version }-{ target }.{ archive-format }",
-            "{ name }-{ version }-{ target }.{ archive-format }",
-            "{ name }-v{ version }-{ target }.{ archive-format }",
-        ];
-
-        let noversion_filenames = &["{ name }-{ target }.{ archive-format }"];
-
         match self {
             GitHub => Some(apply_filenames_to_paths(
                 &[
                     "{ repo }/releases/download/{ version }",
                     "{ repo }/releases/download/v{ version }",
                 ],
-                &[full_filenames, noversion_filenames],
+                &[FULL_FILENAMES, NOVERSION_FILENAMES],
             )),
             GitLab => Some(apply_filenames_to_paths(
                 &[
                     "{ repo }/-/releases/{ version }/downloads/binaries",
                     "{ repo }/-/releases/v{ version }/downloads/binaries",
                 ],
-                &[full_filenames, noversion_filenames],
+                &[FULL_FILENAMES, NOVERSION_FILENAMES],
             )),
             BitBucket => Some(apply_filenames_to_paths(
                 &["{ repo }/downloads"],
-                &[full_filenames],
+                &[FULL_FILENAMES],
             )),
             SourceForge => Some(
                 apply_filenames_to_paths(
@@ -63,7 +66,7 @@ impl RepositoryHost {
                         "{ repo }/files/binaries/{ version }",
                         "{ repo }/files/binaries/v{ version }",
                     ],
-                    &[full_filenames, noversion_filenames],
+                    &[FULL_FILENAMES, NOVERSION_FILENAMES],
                 )
                 .into_iter()
                 .map(|url| format!("{url}/download"))

crates/binstalk/src/fetchers/quickinstall.rs

@@ -61,6 +61,7 @@ impl super::Fetcher for QuickInstall {
     fn target_meta(&self) -> PkgMeta {
         let mut meta = self.data.meta.clone();
         meta.pkg_fmt = Some(self.pkg_fmt());
+        meta.bin_dir = Some("{ bin }{ binary-ext }".to_string());
         meta
     }
 

crates/binstalk/src/manifests/cargo_toml_binstall.rs

@@ -11,9 +11,6 @@ pub use package_formats::*;
 
 mod package_formats;
 
-/// Default binary name template (may be overridden in package Cargo.toml)
-pub const DEFAULT_BIN_DIR: &str = "{ name }-{ target }-v{ version }/{ bin }{ binary-ext }";
-
 /// `binstall` metadata container
 ///
 /// Required to nest metadata under `package.metadata.binstall`
@@ -26,7 +23,7 @@ pub struct Meta {
 /// Metadata for binary installation use.
 ///
 /// Exposed via `[package.metadata]` in `Cargo.toml`
-#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
+#[derive(Clone, Debug, Default, Eq, PartialEq, Serialize, Deserialize)]
 #[serde(rename_all = "kebab-case", default)]
 pub struct PkgMeta {
     /// URL template for package downloads
@@ -36,7 +33,7 @@ pub struct PkgMeta {
     pub pkg_fmt: Option<PkgFmt>,
 
     /// Path template for binary files in packages
-    pub bin_dir: String,
+    pub bin_dir: Option<String>,
 
     /// Public key for package verification (base64 encoded)
     pub pub_key: Option<String>,
@@ -45,18 +42,6 @@ pub struct PkgMeta {
     pub overrides: BTreeMap<String, PkgOverride>,
 }
 
-impl Default for PkgMeta {
-    fn default() -> Self {
-        Self {
-            pkg_url: None,
-            pkg_fmt: None,
-            bin_dir: DEFAULT_BIN_DIR.to_string(),
-            pub_key: None,
-            overrides: BTreeMap::new(),
-        }
-    }
-}
-
 impl PkgMeta {
     pub fn clone_without_overrides(&self) -> Self {
         Self {
@@ -77,7 +62,7 @@ impl PkgMeta {
             self.pkg_fmt = Some(*o);
         }
         if let Some(o) = &pkg_override.bin_dir {
-            self.bin_dir = o.clone();
+            self.bin_dir = Some(o.clone());
         }
     }
 }

crates/binstalk/src/ops/resolve.rs

@@ -176,6 +176,11 @@ async fn resolve_inner(
         manifest.bin,
     );
 
+    // Check binaries
+    if binaries.is_empty() {
+        return Err(BinstallError::UnspecifiedBinaries);
+    }
+
     let desired_targets = opts.desired_targets.get().await;
 
     let mut handles: Vec<(Arc<dyn Fetcher>, _)> = Vec::with_capacity(desired_targets.len() * 2);
@@ -229,7 +234,7 @@ async fn resolve_inner(
                     &bin_path,
                     &package,
                     &install_path,
-                    binaries.clone(),
+                    &binaries,
                 )
                 .await
                 {
@@ -266,26 +271,17 @@ async fn resolve_inner(
 }
 
 ///  * `fetcher` - `fetcher.find()` must return `Ok(true)`.
+///  * `binaries` - must not be empty
 async fn download_extract_and_verify(
     fetcher: &dyn Fetcher,
     bin_path: &Path,
     package: &Package<Meta>,
     install_path: &Path,
-    // TODO: Use &[Product]
+    binaries: &[Product],
-    binaries: Vec<Product>,
 ) -> Result<Vec<bins::BinFile>, BinstallError> {
     // Build final metadata
     let meta = fetcher.target_meta();
 
-    let bin_files = collect_bin_files(
-        fetcher,
-        package,
-        meta,
-        binaries,
-        bin_path.to_path_buf(),
-        install_path.to_path_buf(),
-    )?;
-
     // Download and extract it.
     // If that fails, then ignore this fetcher.
     fetcher.fetch_and_extract(bin_path).await?;
@@ -316,6 +312,15 @@ async fn download_extract_and_verify(
 
     // Verify that all the bin_files exist
     block_in_place(|| {
+        let bin_files = collect_bin_files(
+            fetcher,
+            package,
+            meta,
+            binaries,
+            bin_path.to_path_buf(),
+            install_path.to_path_buf(),
+        )?;
+
         for bin_file in bin_files.iter() {
             bin_file.check_source_exists()?;
         }
@@ -324,25 +329,15 @@ async fn download_extract_and_verify(
     })
 }
 
+///  * `binaries` - must not be empty
 fn collect_bin_files(
     fetcher: &dyn Fetcher,
     package: &Package<Meta>,
-    mut meta: PkgMeta,
+    meta: PkgMeta,
-    binaries: Vec<Product>,
+    binaries: &[Product],
     bin_path: PathBuf,
     install_path: PathBuf,
 ) -> Result<Vec<bins::BinFile>, BinstallError> {
-    // Update meta
-    if fetcher.source_name() == "QuickInstall" {
-        // TODO: less of a hack?
-        meta.bin_dir = "{ bin }{ binary-ext }".to_string();
-    }
-
-    // Check binaries
-    if binaries.is_empty() {
-        return Err(BinstallError::UnspecifiedBinaries);
-    }
-
     // List files to be installed
     // based on those found via Cargo.toml
     let bin_data = bins::Data {