fix: Fix broken rollback in xtask pipeline. (#132)

* fix: Fix broken rollback in xtask pipeline.

The prior implementation of step pipelining in xtask actually didn't
implement rollback correctly, and would "undo" steps it hadn't done
yet. This new implementation is correct.

Signed-off-by: Andrew Lilley Brinker <[email protected]>

* fix: Replace while loop with for loop for Clippy

Clippy flagged the prior version of the loop, and this change satisfies
Clippy.

Signed-off-by: Andrew Lilley Brinker <[email protected]>

---------

Signed-off-by: Andrew Lilley Brinker <[email protected]>

xtask/Cargo.toml

@@ -15,3 +15,4 @@ clap = "4.5.1"
 duct = "0.13.7"
 env_logger = "0.11.2"
 log = "0.4.20"
+which = "6.0.0"

xtask/src/cli.rs

@@ -24,7 +24,7 @@ pub fn args() -> ArgMatches {
                     arg!(--execute)
                         .required(false)
                         .value_parser(value_parser!(bool))
-                        .help("not a dry run, actually execute the release")
+                        .help("not a dry run, actually execute the release"),
                 ),
         )
         .get_matches()

xtask/src/main.rs

@@ -2,16 +2,31 @@ mod cli;
 mod pipeline;
 mod release;
 
-use anyhow::Result;
-use env_logger::{Env, Builder as LoggerBuilder};
+use env_logger::{Builder as LoggerBuilder, Env};
+use std::io::Write as _;
+use std::process::ExitCode;
 
-fn main() -> Result<()> {
-    LoggerBuilder::from_env(Env::default().default_filter_or("info")).init();
+fn main() -> ExitCode {
+    LoggerBuilder::from_env(Env::default().default_filter_or("info"))
+        .format(|buf, record| writeln!(buf, "{:>10}: {}", record.level(), record.args()))
+        .init();
 
     let args = cli::args();
 
-    match args.subcommand() {
+    let res = match args.subcommand() {
         Some(("release", args)) => release::run(args),
         Some(_) | None => Ok(()),
+    };
+
+    if let Err(err) = res {
+        log::error!("{}", err);
+
+        for err in err.chain().skip(1) {
+            log::error!("\tcaused by: {}", err);
+        }
+
+        return ExitCode::FAILURE;
     }
+
+    ExitCode::SUCCESS
 }

xtask/src/pipeline.rs

@@ -1,7 +1,7 @@
 use anyhow::{bail, Error, Result};
 use std::error::Error as StdError;
 use std::fmt::{Display, Formatter, Result as FmtResult};
-use std::iter::DoubleEndedIterator;
+use std::iter::Iterator;
 use std::result::Result as StdResult;
 
 /// A mutable-reference [`Step`]` trait object.
@@ -17,20 +17,33 @@ pub type DynStep<'step> = &'step mut dyn Step;
 /// also assisted by the `step!` macro.
 pub fn run<'step, I, It>(steps: I) -> Result<()>
 where
-    It: DoubleEndedIterator<Item = DynStep<'step>>,
+    It: Iterator<Item = DynStep<'step>>,
     I: IntoIterator<Item = DynStep<'step>, IntoIter = It>,
 {
-    fn inner<'step>(mut steps: impl DoubleEndedIterator<Item = DynStep<'step>>) -> Result<()> {
-        while let Some(step) = steps.next() {
+    fn inner<'step>(steps: impl Iterator<Item = DynStep<'step>>) -> Result<()> {
+        let mut forward_err = None;
+        let mut completed_steps = Vec::new();
+
+        // Run the steps forward.
+        for step in steps {
             if let Err(forward) = forward(step) {
-                while let Some(reverse_step) = steps.next_back() {
-                    if let Err(backward) = backward(reverse_step) {
-                        bail!(PipelineError::rollback(forward, backward));
-                    }
-                }
+                forward_err = Some(forward);
+                completed_steps.push(step);
+                break;
+            }
 
-                bail!(PipelineError::forward(forward));
+            completed_steps.push(step);
+        }
+
+        // If forward had an error, initiate rollback.
+        if let Some(forward_err) = forward_err {
+            for step in completed_steps {
+                if let Err(backward_err) = backward(step) {
+                    bail!(PipelineError::rollback(forward_err, backward_err));
+                }
             }
+
+            bail!(PipelineError::forward(forward_err));
         }
 
         Ok(())
@@ -83,6 +96,11 @@ pub trait Step {
     /// you cancel an operation with a kill signal before the `undo`
     /// operation can complete.
     fn undo(&mut self) -> Result<()>;
+
+    /// Check if a step mutates the environment, so undo might be skipped.
+    fn can_skip_undo(&self) -> bool {
+        false
+    }
 }
 
 /// Helper function to run a step forward and convert the error to [`StepError`]
@@ -97,6 +115,11 @@ fn forward(step: &mut dyn Step) -> StdResult<(), StepError> {
 
 /// Helper function to run a step backward and convert the error to [`StepError`]
 fn backward(step: &mut dyn Step) -> StdResult<(), StepError> {
+    if step.can_skip_undo() {
+        log::info!("skipping rollback for step '{}'", step.name());
+        return Ok(());
+    }
+
     log::info!("rolling back step '{}'", step.name());
 
     step.undo().map_err(|error| StepError {

xtask/src/release.rs

@@ -1,33 +1,40 @@
+use crate::cli::{Bump, Crate};
 use crate::pipeline::{self, Step};
 use crate::step;
-use crate::cli::{Crate, Bump};
-use anyhow::{anyhow, Result};
+use anyhow::{anyhow, Context as _, Result};
 use clap::ArgMatches;
 
 /*
-    # Run `git-cliff` to generate a changelog.
-    # Commit the changelog w/ commit msg in Conventional Commit fmt.
-    # Run `cargo-release` to release the new version.
-    # If anything fails, rollback prior steps in reverse order.
-    # Probably good for each step to have a "do" and "undo" operation.
-    #
-    # ... In fact I'll probably write this in Rust lol.
-
-    # Need:
-    #
-    # - git
-    # - git-cliff
-    # - cargo
-    # - cargo-release
- */
-
+   # Run `git-cliff` to generate a changelog.
+   # Commit the changelog w/ commit msg in Conventional Commit fmt.
+   # Run `cargo-release` to release the new version.
+   # If anything fails, rollback prior steps in reverse order.
+   # Probably good for each step to have a "do" and "undo" operation.
+   #
+   # ... In fact I'll probably write this in Rust lol.
+
+   # Need:
+   #
+   # - git
+   # - git-cliff
+   # - cargo
+   # - cargo-release
+*/
 
 /// Run the release command.
 pub fn run(args: &ArgMatches) -> Result<()> {
-    let krate: Crate = *args.get_one("crate").ok_or_else(|| anyhow!("'--crate' is a required argument"))?;
-    let bump: Bump = *args.get_one("bump").ok_or_else(|| anyhow!("'--bump' is a required argument"))?;
-
-    log::info!("running 'release', bumping the {} number for crate '{}'", bump, krate);
+    let krate: Crate = *args
+        .get_one("crate")
+        .ok_or_else(|| anyhow!("'--crate' is a required argument"))?;
+    let bump: Bump = *args
+        .get_one("bump")
+        .ok_or_else(|| anyhow!("'--bump' is a required argument"))?;
+
+    log::info!(
+        "running 'release', bumping the {} number for crate '{}'",
+        bump,
+        krate
+    );
 
     pipeline::run([
         step!(CheckDependencies),
@@ -45,12 +52,20 @@ impl Step for CheckDependencies {
     }
 
     fn run(&mut self) -> Result<()> {
+        check_cmd("git")?;
+        check_cmd("git-cliff")?;
+        check_cmd("cargo")?;
+        check_cmd("cargo-release")?;
         Ok(())
     }
 
     fn undo(&mut self) -> Result<()> {
         Ok(())
     }
+
+    fn can_skip_undo(&self) -> bool {
+        true
+    }
 }
 
 struct GenerateChangelog;
@@ -100,3 +115,10 @@ impl Step for ReleaseCrate {
         Ok(())
     }
 }
+
+/// Check if a command exists on the command line.
+fn check_cmd(name: &str) -> Result<()> {
+    which::which(name)
+        .map(|_| ())
+        .context(format!("failed to find command '{}'", name))
+}