feat: improve cli error handling and add git commit info (#1246)

Author: blckngm

Cargo.lock

@@ -0,0 +1,10 @@
+fn main() {
+    let desc = std::process::Command::new("git")
+        .args(["describe", "--always", "--dirty", "--exclude", "*"])
+        .output()
+        .ok()
+        .and_then(|r| String::from_utf8(r.stdout).ok())
+        .map(|d| format!(" {d}"))
+        .unwrap_or_default();
+    println!("cargo:rustc-env=AXON_GIT_DESCRIPTION={desc}");
+}

build.rs

@@ -10,6 +10,7 @@ semver = "1.0"
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"
 tempfile = "3.6"
+thiserror = "1.0"
 
 common-config-parser = { path = "../../common/config-parser" }
 common-logger = { path = "../../common/logger" }

core/cli/Cargo.toml

@@ -1,23 +1,58 @@
 use std::io::{self, Write};
 use std::path::Path;
 
-use clap::builder::{IntoResettable, Str};
 use clap::{Arg, ArgMatches, Command};
+use protocol::ProtocolError;
 use semver::Version;
+use thiserror::Error;
 
-use common_config_parser::{parse_file, types::Config};
+use common_config_parser::{parse_file, types::Config, ParseError};
 use core_run::{Axon, KeyProvider, SecioKeyPair};
 use protocol::types::RichBlock;
 
+#[non_exhaustive]
+#[derive(Error, Debug)]
+pub enum Error {
+    // Boxing so the error type isn't too large (clippy::result-large-err).
+    #[error(transparent)]
+    CheckingVersion(Box<CheckingVersionError>),
+    #[error("reading data version: {0}")]
+    ReadingVersion(#[source] io::Error),
+    #[error("writing data version: {0}")]
+    WritingVersion(#[source] io::Error),
+
+    #[error("parsing config: {0}")]
+    ParsingConfig(#[source] ParseError),
+    #[error("getting parent directory of config file")]
+    GettingParent,
+    #[error("parsing genesis: {0}")]
+    ParsingGenesis(#[source] ParseError),
+
+    #[error(transparent)]
+    Running(ProtocolError),
+}
+
+#[non_exhaustive]
+#[derive(Error, Debug)]
+#[cfg_attr(test, derive(PartialEq, Eq))]
+#[error("data version({data}) is not compatible with the current axon version({current}), version >= {least_compatible} is supported")]
+pub struct CheckingVersionError {
+    pub current:          Version,
+    pub data:             Version,
+    pub least_compatible: Version,
+}
+
+pub type Result<T, E = Error> = std::result::Result<T, E>;
+
 pub struct AxonCli {
     version: Version,
     matches: ArgMatches,
 }
 
 impl AxonCli {
-    pub fn init(ver: impl IntoResettable<Str>) -> Self {
+    pub fn init(axon_version: Version, cli_version: &'static str) -> Self {
         let matches = Command::new("axon")
-            .version(ver)
+            .version(cli_version)
             .arg(
                 Arg::new("config_path")
                     .short('c')
@@ -37,19 +72,24 @@ impl AxonCli {
             .subcommand(Command::new("run").about("Run axon process"));
 
         AxonCli {
-            version: Version::parse(matches.get_version().unwrap()).unwrap(),
+            version: axon_version,
             matches: matches.get_matches(),
         }
     }
 
-    pub fn start(&self) {
+    pub fn start(&self) -> Result<()> {
         self.start_with_custom_key_provider::<SecioKeyPair>(None)
     }
 
-    pub fn start_with_custom_key_provider<K: KeyProvider>(&self, key_provider: Option<K>) {
+    pub fn start_with_custom_key_provider<K: KeyProvider>(
+        &self,
+        key_provider: Option<K>,
+    ) -> Result<()> {
         let config_path = self.matches.get_one::<String>("config_path").unwrap();
-        let path = Path::new(&config_path).parent().unwrap();
-        let mut config: Config = parse_file(config_path, false).unwrap();
+        let path = Path::new(&config_path)
+            .parent()
+            .ok_or(Error::GettingParent)?;
+        let mut config: Config = parse_file(config_path, false).map_err(Error::ParsingConfig)?;
 
         if let Some(ref mut f) = config.rocksdb.options_file {
             *f = path.join(&f)
@@ -58,47 +98,53 @@ impl AxonCli {
             self.matches.get_one::<String>("genesis_path").unwrap(),
             true,
         )
-        .unwrap();
+        .map_err(Error::ParsingGenesis)?;
 
-        self.check_version(&config);
+        self.check_version(&config)?;
 
         register_log(&config);
 
-        Axon::new(config, genesis).run(key_provider).unwrap();
+        Axon::new(config, genesis)
+            .run(key_provider)
+            .map_err(Error::Running)?;
+        Ok(())
     }
 
-    fn check_version(&self, config: &Config) {
-        if !config.data_path.exists() {
-            std::fs::create_dir_all(&config.data_path).unwrap();
-        }
-
+    fn check_version(&self, config: &Config) -> Result<()> {
+        // Won't panic because parent of data_path_for_version() is data_path.
         check_version(
             &config.data_path_for_version(),
             &self.version,
-            &latest_compatible_version(),
-        );
+            latest_compatible_version(),
+        )
     }
 }
 
-fn check_version(p: &Path, current: &Version, least_compatible: &Version) {
+/// # Panics
+///
+/// If p.parent() is None.
+fn check_version(p: &Path, current: &Version, least_compatible: Version) -> Result<()> {
     let ver_str = match std::fs::read_to_string(p) {
         Ok(x) => x,
         Err(e) if e.kind() == io::ErrorKind::NotFound => "".into(),
-        Err(e) => panic!("failed to read version: {e}"),
+        Err(e) => return Err(Error::ReadingVersion(e)),
     };
 
     if ver_str.is_empty() {
-        return atomic_write(p, current.to_string().as_bytes()).unwrap();
+        atomic_write(p, current.to_string().as_bytes()).map_err(Error::WritingVersion)?;
+        return Ok(());
     }
 
     let prev_version = Version::parse(&ver_str).unwrap();
-    if prev_version < *least_compatible {
-        panic!(
-            "The previous version {} is not compatible with the current version {}",
-            prev_version, current
-        );
+    if prev_version < least_compatible {
+        return Err(Error::CheckingVersion(Box::new(CheckingVersionError {
+            least_compatible,
+            data: prev_version,
+            current: current.clone(),
+        })));
     }
-    atomic_write(p, current.to_string().as_bytes()).unwrap();
+    atomic_write(p, current.to_string().as_bytes()).map_err(Error::WritingVersion)?;
+    Ok(())
 }
 
 /// Write content to p atomically. Create the parent directory if it doesn't
@@ -107,7 +153,7 @@ fn check_version(p: &Path, current: &Version, least_compatible: &Version) {
 /// # Panics
 ///
 /// if p.parent() is None.
-fn atomic_write(p: &Path, content: &[u8]) -> std::io::Result<()> {
+fn atomic_write(p: &Path, content: &[u8]) -> io::Result<()> {
     let parent = p.parent().unwrap();
 
     std::fs::create_dir_all(parent)?;
@@ -147,28 +193,39 @@ mod tests {
     use super::*;
 
     #[test]
-    fn test_check_version() {
+    fn test_check_version() -> Result<()> {
         let tmp = NamedTempFile::new().unwrap();
         let p = tmp.path();
         // We just want NamedTempFile to delete the file on drop. We want to
         // start with the file not exist.
         std::fs::remove_file(p).unwrap();
 
-        let least_compatible = "0.1.0-alpha.9".parse().unwrap();
+        let latest_compatible: Version = "0.1.0-alpha.9".parse().unwrap();
 
-        check_version(p, &"0.1.15".parse().unwrap(), &least_compatible);
+        check_version(p, &"0.1.15".parse().unwrap(), latest_compatible.clone())?;
         assert_eq!(std::fs::read_to_string(p).unwrap(), "0.1.15");
 
-        check_version(p, &"0.2.0".parse().unwrap(), &least_compatible);
+        check_version(p, &"0.2.0".parse().unwrap(), latest_compatible)?;
         assert_eq!(std::fs::read_to_string(p).unwrap(), "0.2.0");
+
+        Ok(())
     }
 
-    #[should_panic = "The previous version"]
     #[test]
-    fn test_check_version_failure() {
+    fn test_check_version_failure() -> Result<()> {
         let tmp = NamedTempFile::new().unwrap();
         let p = tmp.path();
-        check_version(p, &"0.1.0".parse().unwrap(), &"0.1.0".parse().unwrap());
-        check_version(p, &"0.2.0".parse().unwrap(), &"0.2.0".parse().unwrap());
+        check_version(p, &"0.1.0".parse().unwrap(), "0.1.0".parse().unwrap())?;
+        let err =
+            check_version(p, &"0.2.2".parse().unwrap(), "0.2.0".parse().unwrap()).unwrap_err();
+        match err {
+            Error::CheckingVersion(e) => assert_eq!(*e, CheckingVersionError {
+                current:          "0.2.2".parse().unwrap(),
+                least_compatible: "0.2.0".parse().unwrap(),
+                data:             "0.1.0".parse().unwrap(),
+            }),
+            e => panic!("unexpected error {e}"),
+        }
+        Ok(())
     }
 }

core/cli/src/lib.rs

@@ -49,5 +49,9 @@ impl KeyProvider for CustomKey {
 }
 
 fn main() {
-    axon::run(CustomFeeAllocator::default(), CustomKey::default())
+    let result = axon::run(CustomFeeAllocator::default(), CustomKey::default(), "0.1.0");
+    if let Err(e) = result {
+        eprintln!("Error {e}");
+        std::process::exit(1);
+    }
 }

examples/custom_chain.rs

@@ -7,10 +7,15 @@ pub use protocol::{
 
 use std::sync::Arc;
 
-use core_cli::AxonCli;
+use core_cli::{AxonCli, Result};
 use core_executor::FEE_ALLOCATOR;
 
-pub fn run(fee_allocator: impl FeeAllocate + 'static, key_provider: impl KeyProvider) {
+pub fn run(
+    fee_allocator: impl FeeAllocate + 'static,
+    key_provider: impl KeyProvider,
+    cli_version: &'static str,
+) -> Result<()> {
     FEE_ALLOCATOR.swap(Arc::new(Box::new(fee_allocator)));
-    AxonCli::init(clap::crate_version!()).start_with_custom_key_provider(Some(key_provider));
+    AxonCli::init(clap::crate_version!().parse().unwrap(), cli_version)
+        .start_with_custom_key_provider(Some(key_provider))
 }

src/lib.rs

@@ -1,5 +1,13 @@
 use core_cli::AxonCli;
 
 fn main() {
-    AxonCli::init(clap::crate_version!()).start();
+    let result = AxonCli::init(
+        clap::crate_version!().parse().unwrap(),
+        concat!(clap::crate_version!(), env!("AXON_GIT_DESCRIPTION")),
+    )
+    .start();
+    if let Err(e) = result {
+        eprintln!("Error {e}");
+        std::process::exit(1);
+    }
 }