From e034d69e121b4ad5e169df0f5ee5e3bfec38eb01 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 25 Sep 2022 22:15:20 +1000 Subject: [PATCH] Impl try multiple default bin dirs (#417) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Co-authored-by: Félix Saparelli --- .github/scripts/tests.sh | 3 +- crates/binstalk/src/bins.rs | 40 ++++++++++++++++- crates/binstalk/src/fetchers.rs | 4 +- crates/binstalk/src/fetchers/gh_crate_meta.rs | 8 ++-- .../src/fetchers/gh_crate_meta/hosting.rs | 33 +++++++------- crates/binstalk/src/fetchers/quickinstall.rs | 1 + .../src/manifests/cargo_toml_binstall.rs | 21 ++------- crates/binstalk/src/ops/resolve.rs | 45 +++++++++---------- 8 files changed, 88 insertions(+), 67 deletions(-) diff --git a/.github/scripts/tests.sh b/.github/scripts/tests.sh index 40f09e06..ad49fb70 100755 --- a/.github/scripts/tests.sh +++ b/.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 diff --git a/crates/binstalk/src/bins.rs b/crates/binstalk/src/bins.rs index 43d20db2..cb66bbf3 100644 --- a/crates/binstalk/src/bins.rs +++ b/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 { 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 { diff --git a/crates/binstalk/src/fetchers.rs b/crates/binstalk/src/fetchers.rs index ceebdb2e..650de1d8 100644 --- a/crates/binstalk/src/fetchers.rs +++ b/crates/binstalk/src/fetchers.rs @@ -11,8 +11,8 @@ use crate::{ manifests::cargo_toml_binstall::{PkgFmt, PkgMeta}, }; -mod gh_crate_meta; -mod quickinstall; +pub(crate) mod gh_crate_meta; +pub(crate) mod quickinstall; #[async_trait::async_trait] pub trait Fetcher: Send + Sync { diff --git a/crates/binstalk/src/fetchers/gh_crate_meta.rs b/crates/binstalk/src/fetchers/gh_crate_meta.rs index 54842ce4..46a42ae4 100644 --- a/crates/binstalk/src/fetchers/gh_crate_meta.rs +++ b/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 - debug!("Downloading package from: '{url}'"); + let (url, pkg_fmt) = self.resolution.get().unwrap(); // find() is called first + 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 } diff --git a/crates/binstalk/src/fetchers/gh_crate_meta/hosting.rs b/crates/binstalk/src/fetchers/gh_crate_meta/hosting.rs index 521e062c..2c491d5e 100644 --- a/crates/binstalk/src/fetchers/gh_crate_meta/hosting.rs +++ b/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 { use RepositoryHost::*; @@ -27,35 +41,24 @@ impl RepositoryHost { pub fn get_default_pkg_url_template(self) -> Option> { 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")) diff --git a/crates/binstalk/src/fetchers/quickinstall.rs b/crates/binstalk/src/fetchers/quickinstall.rs index d7d539a1..6613d308 100644 --- a/crates/binstalk/src/fetchers/quickinstall.rs +++ b/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 } diff --git a/crates/binstalk/src/manifests/cargo_toml_binstall.rs b/crates/binstalk/src/manifests/cargo_toml_binstall.rs index 21950214..a8b023b8 100644 --- a/crates/binstalk/src/manifests/cargo_toml_binstall.rs +++ b/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, /// Path template for binary files in packages - pub bin_dir: String, + pub bin_dir: Option, /// Public key for package verification (base64 encoded) pub pub_key: Option, @@ -45,18 +42,6 @@ pub struct PkgMeta { pub overrides: BTreeMap, } -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()); } } } diff --git a/crates/binstalk/src/ops/resolve.rs b/crates/binstalk/src/ops/resolve.rs index 2dc48cac..b59f848e 100644 --- a/crates/binstalk/src/ops/resolve.rs +++ b/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, _)> = 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, install_path: &Path, - // TODO: Use &[Product] - binaries: Vec, + binaries: &[Product], ) -> Result, 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, - mut meta: PkgMeta, - binaries: Vec, + meta: PkgMeta, + binaries: &[Product], bin_path: PathBuf, install_path: PathBuf, ) -> Result, 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 {