feat: Impl new option --continue-on-failure (#1559)

* feat: Impl new option `--continue-on-failure`

Resolve #1548

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

* Add new e2e-tests continue-on-failure

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

* Rm dup line ion `e2e-tests/live.sh`

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

* Fix shellcheck

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

* Fix `BinstallError::crate_errors` if `errors.len()` is 1

In that case, it should return `Some(Self::CrateContext(_))` instead of
`Some(Self::Errors(_))`

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

* Add more tests to `e2e-tests/continue-on-failure.sh`

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

* Propagate crate errors on `confirm()` err

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

* Test having two errors in `e2e-tests/continue-on-failure.sh`

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

---------

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

crates/bin/src/args.rs

@@ -191,6 +191,10 @@ pub struct Args {
     #[clap(help_heading = "Options", long)]
     pub(crate) no_cleanup: bool,
 
+    /// Continue installing other crates even if one of the crate failed to install.
+    #[clap(help_heading = "Options", long)]
+    pub(crate) continue_on_failure: bool,
+
     /// By default, binstall keeps track of the installed packages with metadata files
     /// stored in the installation root directory.
     ///

crates/bin/src/bin_util.rs

@@ -1,5 +1,4 @@
 use std::{
-    future::Future,
     process::{ExitCode, Termination},
     time::Duration,
 };
@@ -48,20 +47,17 @@ impl MainExit {
 }
 
 /// This function would start a tokio multithreading runtime,
-/// spawn a new task on it that runs `f()`, then `block_on` it.
+/// then `block_on` the task it returns.
 ///
 /// It will cancel the future if user requested cancellation
 /// via signal.
-pub fn run_tokio_main<Func, Fut>(f: Func) -> Result<()>
-where
-    Func: FnOnce() -> Result<Option<Fut>>,
-    Fut: Future<Output = Result<()>> + Send + 'static,
-{
+pub fn run_tokio_main(
+    f: impl FnOnce() -> Result<Option<AutoAbortJoinHandle<Result<()>>>>,
+) -> Result<()> {
     let rt = Runtime::new().map_err(BinstallError::from)?;
     let _guard = rt.enter();
 
-    if let Some(fut) = f()? {
-        let handle = AutoAbortJoinHandle::new(rt.spawn(fut));
+    if let Some(handle) = f()? {
         rt.block_on(cancel_on_user_sig_term(handle))?
     } else {
         Ok(())

crates/bin/src/entry.rs

@@ -1,12 +1,11 @@
 use std::{
     env, fs,
-    future::Future,
     path::{Path, PathBuf},
     sync::Arc,
 };
 
 use binstalk::{
-    errors::BinstallError,
+    errors::{BinstallError, CrateContextError},
     fetchers::{Fetcher, GhCrateMeta, QuickInstall, SignaturePolicy},
     get_desired_targets,
     helpers::{
@@ -27,7 +26,7 @@ use binstalk_manifests::{
 use file_format::FileFormat;
 use home::cargo_home;
 use log::LevelFilter;
-use miette::{miette, Result, WrapErr};
+use miette::{miette, Report, Result, WrapErr};
 use tokio::task::block_in_place;
 use tracing::{debug, error, info, warn};
 
@@ -40,7 +39,7 @@ use crate::{
 pub fn install_crates(
     args: Args,
     jobserver_client: LazyJobserverClient,
-) -> Result<Option<impl Future<Output = Result<()>>>> {
+) -> Result<Option<AutoAbortJoinHandle<Result<()>>>> {
     // Compute Resolvers
     let mut cargo_install_fallback = false;
 
@@ -217,54 +216,131 @@ pub fn install_crates(
         })
         .collect();
 
-    Ok(Some(async move {
-        // Collect results
-        let mut resolution_fetchs = Vec::new();
-        let mut resolution_sources = Vec::new();
+    Ok(Some(if args.continue_on_failure {
+        AutoAbortJoinHandle::spawn(async move {
+            // Collect results
+            let mut resolution_fetchs = Vec::new();
+            let mut resolution_sources = Vec::new();
+            let mut errors = Vec::new();
 
-        for task in tasks {
-            match task.await?? {
-                Resolution::AlreadyUpToDate => {}
-                Resolution::Fetch(fetch) => {
-                    fetch.print(&binstall_opts);
-                    resolution_fetchs.push(fetch)
-                }
-                Resolution::InstallFromSource(source) => {
-                    source.print();
-                    resolution_sources.push(source)
+            for task in tasks {
+                match task.flattened_join().await {
+                    Ok(Resolution::AlreadyUpToDate) => {}
+                    Ok(Resolution::Fetch(fetch)) => {
+                        fetch.print(&binstall_opts);
+                        resolution_fetchs.push(fetch)
+                    }
+                    Ok(Resolution::InstallFromSource(source)) => {
+                        source.print();
+                        resolution_sources.push(source)
+                    }
+                    Err(BinstallError::CrateContext(err)) => errors.push(err),
+                    Err(e) => panic!("Expected BinstallError::CrateContext(_), got {}", e),
                 }
             }
-        }
 
-        if resolution_fetchs.is_empty() && resolution_sources.is_empty() {
-            debug!("Nothing to do");
-            return Ok(());
-        }
+            if resolution_fetchs.is_empty() && resolution_sources.is_empty() {
+                return if let Some(err) = BinstallError::crate_errors(errors) {
+                    Err(err.into())
+                } else {
+                    debug!("Nothing to do");
+                    Ok(())
+                };
+            }
 
-        // Confirm
-        if !dry_run && !no_confirm {
-            confirm().await?;
-        }
+            // Confirm
+            if !dry_run && !no_confirm {
+                if let Err(abort_err) = confirm().await {
+                    return if let Some(err) = BinstallError::crate_errors(errors) {
+                        Err(Report::new(abort_err).wrap_err(err))
+                    } else {
+                        Err(abort_err.into())
+                    };
+                }
+            }
 
-        do_install_fetches(
-            resolution_fetchs,
-            manifests,
-            &binstall_opts,
-            dry_run,
-            temp_dir,
-            no_cleanup,
-        )?;
+            let manifest_update_res = do_install_fetches_continue_on_failure(
+                resolution_fetchs,
+                manifests,
+                &binstall_opts,
+                dry_run,
+                temp_dir,
+                no_cleanup,
+                &mut errors,
+            );
 
-        let tasks: Vec<_> = resolution_sources
-            .into_iter()
-            .map(|source| AutoAbortJoinHandle::spawn(source.install(binstall_opts.clone())))
-            .collect();
+            let tasks: Vec<_> = resolution_sources
+                .into_iter()
+                .map(|source| AutoAbortJoinHandle::spawn(source.install(binstall_opts.clone())))
+                .collect();
 
-        for task in tasks {
-            task.await??;
-        }
+            for task in tasks {
+                match task.flattened_join().await {
+                    Ok(_) => (),
+                    Err(BinstallError::CrateContext(err)) => errors.push(err),
+                    Err(e) => panic!("Expected BinstallError::CrateContext(_), got {}", e),
+                }
+            }
 
-        Ok(())
+            match (BinstallError::crate_errors(errors), manifest_update_res) {
+                (None, Ok(())) => Ok(()),
+                (None, Err(err)) => Err(err),
+                (Some(err), Ok(())) => Err(err.into()),
+                (Some(err), Err(manifest_update_err)) => {
+                    Err(Report::new(err).wrap_err(manifest_update_err))
+                }
+            }
+        })
+    } else {
+        AutoAbortJoinHandle::spawn(async move {
+            // Collect results
+            let mut resolution_fetchs = Vec::new();
+            let mut resolution_sources = Vec::new();
+
+            for task in tasks {
+                match task.await?? {
+                    Resolution::AlreadyUpToDate => {}
+                    Resolution::Fetch(fetch) => {
+                        fetch.print(&binstall_opts);
+                        resolution_fetchs.push(fetch)
+                    }
+                    Resolution::InstallFromSource(source) => {
+                        source.print();
+                        resolution_sources.push(source)
+                    }
+                }
+            }
+
+            if resolution_fetchs.is_empty() && resolution_sources.is_empty() {
+                debug!("Nothing to do");
+                return Ok(());
+            }
+
+            // Confirm
+            if !dry_run && !no_confirm {
+                confirm().await?;
+            }
+
+            do_install_fetches(
+                resolution_fetchs,
+                manifests,
+                &binstall_opts,
+                dry_run,
+                temp_dir,
+                no_cleanup,
+            )?;
+
+            let tasks: Vec<_> = resolution_sources
+                .into_iter()
+                .map(|source| AutoAbortJoinHandle::spawn(source.install(binstall_opts.clone())))
+                .collect();
+
+            for task in tasks {
+                task.await??;
+            }
+
+            Ok(())
+        })
     }))
 }
 
@@ -449,3 +525,53 @@ fn do_install_fetches(
         Ok(())
     })
 }
+
+#[allow(clippy::vec_box)]
+fn do_install_fetches_continue_on_failure(
+    resolution_fetchs: Vec<Box<ResolutionFetch>>,
+    // Take manifests by value to drop the `FileLock`.
+    manifests: Option<Manifests>,
+    binstall_opts: &Options,
+    dry_run: bool,
+    temp_dir: tempfile::TempDir,
+    no_cleanup: bool,
+    errors: &mut Vec<Box<CrateContextError>>,
+) -> 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()
+            .filter_map(|fetch| match fetch.install(binstall_opts) {
+                Ok(crate_info) => Some(crate_info),
+                Err(BinstallError::CrateContext(err)) => {
+                    errors.push(err);
+                    None
+                }
+                Err(e) => panic!("Expected BinstallError::CrateContext(_), got {}", e),
+            })
+            .collect::<Vec<_>>();
+
+        if let Some(manifests) = manifests {
+            manifests.update(metadata_vec)?;
+        }
+
+        if no_cleanup {
+            // Consume temp_dir without removing it from fs.
+            let _ = temp_dir.into_path();
+        } else {
+            temp_dir.close().unwrap_or_else(|err| {
+                warn!("Failed to clean up some resources: {err}");
+            });
+        }
+
+        Ok(())
+    })
+}

crates/binstalk/src/errors.rs

@@ -1,5 +1,5 @@
 use std::{
-    io,
+    fmt, io, ops,
     path::PathBuf,
     process::{ExitCode, ExitStatus, Termination},
 };
@@ -9,6 +9,7 @@ use binstalk_downloader::{
 };
 use binstalk_fetchers::FetchError;
 use compact_str::CompactString;
+use itertools::Itertools;
 use miette::{Diagnostic, Report};
 use target_lexicon::ParseError as TargetTripleParseError;
 use thiserror::Error;
@@ -40,6 +41,90 @@ pub struct CrateContextError {
     err: BinstallError,
 }
 
+#[derive(Debug)]
+pub struct CrateErrors(Box<[Box<CrateContextError>]>);
+
+impl CrateErrors {
+    fn iter(&self) -> impl Iterator<Item = &CrateContextError> + Clone {
+        self.0.iter().map(ops::Deref::deref)
+    }
+
+    fn get_iter_for<'a, T: 'a>(
+        &'a self,
+        f: fn(&'a CrateContextError) -> Option<T>,
+    ) -> Option<impl Iterator<Item = T> + 'a> {
+        let iter = self.iter().filter_map(f);
+
+        if iter.clone().next().is_none() {
+            None
+        } else {
+            Some(iter)
+        }
+    }
+}
+
+impl fmt::Display for CrateErrors {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        fmt::Display::fmt(&self.0.iter().format(", "), f)
+    }
+}
+
+impl std::error::Error for CrateErrors {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        self.0.first().map(|e| e as _)
+    }
+}
+
+impl miette::Diagnostic for CrateErrors {
+    fn code<'a>(&'a self) -> Option<Box<dyn fmt::Display + 'a>> {
+        Some(Box::new("binstall::many_failure"))
+    }
+
+    fn severity(&self) -> Option<miette::Severity> {
+        self.iter().filter_map(miette::Diagnostic::severity).max()
+    }
+
+    fn help<'a>(&'a self) -> Option<Box<dyn fmt::Display + 'a>> {
+        Some(Box::new(
+            self.get_iter_for(miette::Diagnostic::help)?.format("\n"),
+        ))
+    }
+
+    fn url<'a>(&'a self) -> Option<Box<dyn fmt::Display + 'a>> {
+        Some(Box::new(
+            self.get_iter_for(miette::Diagnostic::url)?.format("\n"),
+        ))
+    }
+
+    fn source_code(&self) -> Option<&dyn miette::SourceCode> {
+        self.iter().find_map(miette::Diagnostic::source_code)
+    }
+
+    fn labels(&self) -> Option<Box<dyn Iterator<Item = miette::LabeledSpan> + '_>> {
+        let get_iter = || self.iter().filter_map(miette::Diagnostic::labels).flatten();
+
+        if get_iter().next().is_none() {
+            None
+        } else {
+            Some(Box::new(get_iter()))
+        }
+    }
+
+    fn related<'a>(&'a self) -> Option<Box<dyn Iterator<Item = &'a dyn miette::Diagnostic> + 'a>> {
+        Some(Box::new(
+            self.iter().map(|e| e as _).chain(
+                self.iter()
+                    .filter_map(miette::Diagnostic::related)
+                    .flatten(),
+            ),
+        ))
+    }
+
+    fn diagnostic_source(&self) -> Option<&dyn miette::Diagnostic> {
+        self.0.first().map(|err| &**err as _)
+    }
+}
+
 #[derive(Debug, Error)]
 #[error("Invalid pkg-url {pkg_url} for {crate_name}@{version} on {target}: {reason}")]
 pub struct InvalidPkgFmtError {
@@ -344,6 +429,11 @@ pub enum BinstallError {
     #[error(transparent)]
     #[diagnostic(transparent)]
     CrateContext(Box<CrateContextError>),
+
+    /// A wrapped error for failures of multiple crates when `--continue-on-failure` is specified.
+    #[error(transparent)]
+    #[diagnostic(transparent)]
+    Errors(CrateErrors),
 }
 
 impl BinstallError {
@@ -380,6 +470,7 @@ impl BinstallError {
             GitError(_) => 98,
             LoadManifestFromWSError(_) => 99,
             CrateContext(context) => context.err.exit_number(),
+            Errors(errors) => (errors.0)[0].err.exit_number(),
         };
 
         // reserved codes
@@ -401,10 +492,27 @@ impl BinstallError {
 
     /// Add crate context to the error
     pub fn crate_context(self, crate_name: impl Into<CompactString>) -> Self {
-        Self::CrateContext(Box::new(CrateContextError {
-            err: self,
-            crate_name: crate_name.into(),
-        }))
+        self.crate_context_inner(crate_name.into())
+    }
+
+    fn crate_context_inner(self, crate_name: CompactString) -> Self {
+        match self {
+            Self::CrateContext(mut crate_context_error) => {
+                crate_context_error.crate_name = crate_name;
+                Self::CrateContext(crate_context_error)
+            }
+            err => Self::CrateContext(Box::new(CrateContextError { err, crate_name })),
+        }
+    }
+
+    pub fn crate_errors(mut errors: Vec<Box<CrateContextError>>) -> Option<Self> {
+        if errors.is_empty() {
+            None
+        } else if errors.len() == 1 {
+            Some(Self::CrateContext(errors.pop().unwrap()))
+        } else {
+            Some(Self::Errors(CrateErrors(errors.into_boxed_slice())))
+        }
     }
 }
 

crates/binstalk/src/ops/resolve/resolution.rs

@@ -51,6 +51,12 @@ impl Resolution {
 
 impl ResolutionFetch {
     pub fn install(self, opts: &Options) -> Result<CrateInfo, BinstallError> {
+        let crate_name = self.name.clone();
+        self.install_inner(opts)
+            .map_err(|err| err.crate_context(crate_name))
+    }
+
+    fn install_inner(self, opts: &Options) -> Result<CrateInfo, BinstallError> {
         type InstallFp = fn(&bins::BinFile) -> Result<(), bins::Error>;
 
         let (install_bin, install_link): (InstallFp, InstallFp) = match (opts.no_track, opts.force)
@@ -126,6 +132,13 @@ impl ResolutionFetch {
 
 impl ResolutionSource {
     pub async fn install(self, opts: Arc<Options>) -> Result<(), BinstallError> {
+        let crate_name = self.name.clone();
+        self.install_inner(opts)
+            .await
+            .map_err(|err| err.crate_context(crate_name))
+    }
+
+    async fn install_inner(self, opts: Arc<Options>) -> Result<(), BinstallError> {
         let target = if let Some(targets) = opts.desired_targets.get_initialized() {
             Some(targets.first().ok_or(BinstallError::NoViableTargets)?)
         } else {
@@ -171,7 +184,7 @@ impl ResolutionSource {
             cmd.arg("--no-track");
         }
 
-        debug!("Running `{}`", format_cmd(&cmd),);
+        debug!("Running `{}`", format_cmd(&cmd));
 
         if !opts.dry_run {
             let mut child = opts

e2e-tests/continue-on-failure.sh

@@ -0,0 +1,58 @@
+#!/bin/bash
+
+set -euxo pipefail
+
+unset CARGO_INSTALL_ROOT
+
+CARGO_HOME=$(mktemp -d 2>/dev/null || mktemp -d -t 'cargo-home')
+export CARGO_HOME
+othertmpdir=$(mktemp -d 2>/dev/null || mktemp -d -t 'cargo-test')
+export PATH="$CARGO_HOME/bin:$othertmpdir/bin:$PATH"
+
+mkdir -p "$othertmpdir/bin"
+# Copy it to bin to test use of env var `CARGO`
+cp "./$1" "$othertmpdir/bin/"
+
+
+## Test --continue-on-failure
+set +e
+cargo binstall --no-confirm --continue-on-failure cargo-watch@8.4.0 non-existent-clippy
+exit_code="$?"
+
+set -e
+
+if [ "$exit_code" != 76 ]; then
+    echo "Expected exit code 76, but actual exit code $exit_code"
+    exit 1
+fi
+
+
+cargo_watch_version="$(cargo watch -V)"
+echo "$cargo_watch_version"
+
+[ "$cargo_watch_version" = "cargo-watch 8.4.0" ]
+
+
+## Test that it is no-op when only one crate is passed
+set +e
+cargo binstall --no-confirm --continue-on-failure non-existent-clippy
+exit_code="$?"
+
+set -e
+
+if [ "$exit_code" != 76 ]; then
+    echo "Expected exit code 76, but actual exit code $exit_code"
+    exit 1
+fi
+
+# Test if both crates are invalid
+set +e
+cargo binstall --no-confirm --continue-on-failure non-existent-clippy non-existent-clippy2
+exit_code="$?"
+
+set -e
+
+if [ "$exit_code" != 76 ]; then
+    echo "Expected exit code 76, but actual exit code $exit_code"
+    exit 1
+fi

e2e-tests/live.sh

@@ -36,7 +36,6 @@ echo "$cargo_release_version"
 
 [ "$cargo_release_version" = "cargo-release 0.24.9" ]
 
-cargo-binstall --help >/dev/null
 cargo binstall --help >/dev/null
 
 cargo_binstall_version="$(cargo-binstall -V)"

justfile

@@ -248,6 +248,7 @@ e2e-test-no-track: (e2e-test "no-track")
 e2e-test-git: (e2e-test "git")
 e2e-test-registries: (e2e-test "registries")
 e2e-test-signing: (e2e-test "signing")
+e2e-test-continue-on-failure: (e2e-test "continue-on-failure")
 
 # WinTLS (Windows in CI) does not have TLS 1.3 support
 [windows]
@@ -256,7 +257,7 @@ e2e-test-tls: (e2e-test "tls" "1.2")
 [macos]
 e2e-test-tls: (e2e-test "tls" "1.2") (e2e-test "tls" "1.3")
 
-e2e-tests: e2e-test-live e2e-test-manifest-path e2e-test-git e2e-test-other-repos e2e-test-strategies e2e-test-version-syntax e2e-test-upgrade e2e-test-tls e2e-test-self-upgrade-no-symlink e2e-test-uninstall e2e-test-subcrate e2e-test-no-track e2e-test-registries e2e-test-signing
+e2e-tests: e2e-test-live e2e-test-manifest-path e2e-test-git e2e-test-other-repos e2e-test-strategies e2e-test-version-syntax e2e-test-upgrade e2e-test-tls e2e-test-self-upgrade-no-symlink e2e-test-uninstall e2e-test-subcrate e2e-test-no-track e2e-test-registries e2e-test-signing e2e-test-continue-on-failure
 
 unit-tests: print-env
     {{cargo-bin}} test {{cargo-build-args}}