From 185ec25b9e388bb76b21c212b4a4123f5533f71d Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Sat, 27 Apr 2019 01:05:09 +0200 Subject: [PATCH 1/5] Rewrite logging config deserialization This patch rewrites the logging config deserialization. It removes the manual traversing of the config toml structure and replaces it with types which implement `Deserialize`, which is way more convenient and easy to read (and extend). Signed-off-by: Matthias Beyer --- lib/core/libimagrt/Cargo.toml | 2 + lib/core/libimagrt/src/lib.rs | 2 + lib/core/libimagrt/src/logger.rs | 143 ++++++++++++++++--------------- 3 files changed, 78 insertions(+), 69 deletions(-) diff --git a/lib/core/libimagrt/Cargo.toml b/lib/core/libimagrt/Cargo.toml index 63e97ee2..31a04a8e 100644 --- a/lib/core/libimagrt/Cargo.toml +++ b/lib/core/libimagrt/Cargo.toml @@ -29,6 +29,8 @@ toml-query = "0.9" atty = "0.2" failure = "0.1" failure_derive = "0.1" +serde_derive = "1" +serde = "1" libimagstore = { version = "0.10.0", path = "../../../lib/core/libimagstore" } libimagerror = { version = "0.10.0", path = "../../../lib/core/libimagerror" } diff --git a/lib/core/libimagrt/src/lib.rs b/lib/core/libimagrt/src/lib.rs index 825b49e0..4247ff6c 100644 --- a/lib/core/libimagrt/src/lib.rs +++ b/lib/core/libimagrt/src/lib.rs @@ -41,6 +41,8 @@ extern crate itertools; extern crate ansi_term; extern crate handlebars; +extern crate serde; +#[macro_use] extern crate serde_derive; #[macro_use] extern crate failure; extern crate clap; diff --git a/lib/core/libimagrt/src/logger.rs b/lib/core/libimagrt/src/logger.rs index ef586bbb..8aabcb5e 100644 --- a/lib/core/libimagrt/src/logger.rs +++ b/lib/core/libimagrt/src/logger.rs @@ -349,79 +349,84 @@ mod log_lvl_aggregate { fn aggregate_module_settings(_matches: &ArgMatches, config: Option<&Value>) -> Result> { - // Helper macro to return the error from Some(Err(_)) and map everything else to an - // Option<_> - macro_rules! inner_try { - ($v:expr) => { - match $v { - Some(Ok(v)) => Some(v), - Some(Err(e)) => return Err(e), - None => None, + use toml_query::read::Partial; + use std::convert::TryInto; + + #[derive(Serialize, Deserialize, Debug)] + struct LoggingModuleConfig { + pub destinations: Option>, + pub level: Option, + pub enabled: bool, + } + + #[derive(Serialize, Deserialize, Debug)] + enum LogLevel { + #[serde(rename = "trace")] + Trace, + + #[serde(rename = "debug")] + Debug, + + #[serde(rename = "info")] + Info, + + #[serde(rename = "warn")] + Warn, + + #[serde(rename = "error")] + Error, + } + + impl Into for LogLevel { + fn into(self) -> Level { + match self { + LogLevel::Trace => Level::Trace, + LogLevel::Debug => Level::Debug, + LogLevel::Info => Level::Info, + LogLevel::Warn => Level::Warn, + LogLevel::Error => Level::Error, } } - }; + } + + #[derive(Serialize, Deserialize, Debug)] + struct LoggingModuleConfigMap(BTreeMap); + + impl<'a> Partial<'a> for LoggingModuleConfigMap { + const LOCATION: &'static str = "imag.logging.modules"; + type Output = Self; + } + + impl TryInto> for LoggingModuleConfigMap { + type Error = Error; + + fn try_into(self) -> Result> { + let mut map = BTreeMap::new(); + + for (key, value) in self.0.into_iter() { + map.insert(key, ModuleSettings { + enabled: value.enabled, + level: value.level.map(Into::into), + destinations: match value.destinations { + None => None, + Some(ds) => Some(ds + .iter() + .map(Deref::deref) + .map(translate_destination) + .collect::>>()?) + }, + }); + } + + Ok(map) + } + } match config { - Some(cfg) => match cfg.read("imag.logging.modules").map_err(Error::from) { - Ok(Some(&Value::Table(ref t))) => { - // translate the module settings from the table `t` - let mut settings = BTreeMap::new(); - - for (module_name, v) in t { - let destinations = inner_try! { - v.read("destinations") - .context("Failed reading header 'destinations'") - .map_err(Error::from) - .context(EM::TomlQueryError)? - .map(|val| { - val.as_array() - .ok_or_else(|| { - let msg = "Type error at 'imag.logging.modules..destinations', expected 'Array'"; - Error::from(err_msg(msg)) - }) - .and_then(translate_destinations) - }) - }; - - let level = inner_try! { - v.read_string("level") - .context("Failed reading header 'level'") - .map_err(Error::from) - .context(EM::TomlQueryError)? - .map(|s| match_log_level_str(&s)) - }; - - let enabled = v.read("enabled") - .map_err(Error::from) - .context(EM::TomlQueryError)? - .map(|v| v.as_bool().unwrap_or(false)) - .ok_or_else(|| { - let msg = "Type error at 'imag.logging.modules..enabled', expected 'Boolean'"; - Error::from(err_msg(msg)) - })?; - - let module_settings = ModuleSettings { - enabled: enabled, - level: level, - destinations: destinations, - }; - - // We don't care whether there was a value, we override it. - let _ = settings.insert(module_name.to_owned(), module_settings); - } - - Ok(settings) - }, - Ok(Some(_)) => { - let msg = "Type error at 'imag.logging.modules', expected 'Table'"; - Err(Error::from(err_msg(msg))) - }, - Ok(None) => { - // No modules configured. This is okay! - Ok(BTreeMap::new()) - }, - Err(e) => Err(e).context(EM::TomlQueryError).map_err(Error::from), - }, + Some(cfg) => cfg.read_partial::()? + .ok_or_else(|| err_msg("Logging configuration missing"))? + .try_into() + .map_err(Error::from), None => { write!(stderr(), "No Configuration.").ok(); write!(stderr(), "cannot find module-settings for logging.").ok(); From fb8b50fa9a75b50becf9576c22f0041f9b551fea Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Sat, 27 Apr 2019 01:17:25 +0200 Subject: [PATCH 2/5] Remove boilerplate by enabling serde in "log" dependency This patch adds the "serde" feature to the "log" dependency, so we can deserialize logging levels directly into "log" types. Signed-off-by: Matthias Beyer --- lib/core/libimagrt/Cargo.toml | 2 +- lib/core/libimagrt/src/logger.rs | 32 +------------------------------- 2 files changed, 2 insertions(+), 32 deletions(-) diff --git a/lib/core/libimagrt/Cargo.toml b/lib/core/libimagrt/Cargo.toml index 31a04a8e..f9b83932 100644 --- a/lib/core/libimagrt/Cargo.toml +++ b/lib/core/libimagrt/Cargo.toml @@ -45,7 +45,7 @@ features = ["suggestions", "color", "wrap_help"] [dependencies.log] version = "0.4" default-features = false -features = ["std"] +features = ["std", "serde"] [dependencies.handlebars] version = "^1.0.5" diff --git a/lib/core/libimagrt/src/logger.rs b/lib/core/libimagrt/src/logger.rs index 8aabcb5e..956d251d 100644 --- a/lib/core/libimagrt/src/logger.rs +++ b/lib/core/libimagrt/src/logger.rs @@ -355,40 +355,10 @@ fn aggregate_module_settings(_matches: &ArgMatches, config: Option<&Value>) #[derive(Serialize, Deserialize, Debug)] struct LoggingModuleConfig { pub destinations: Option>, - pub level: Option, + pub level: Option, pub enabled: bool, } - #[derive(Serialize, Deserialize, Debug)] - enum LogLevel { - #[serde(rename = "trace")] - Trace, - - #[serde(rename = "debug")] - Debug, - - #[serde(rename = "info")] - Info, - - #[serde(rename = "warn")] - Warn, - - #[serde(rename = "error")] - Error, - } - - impl Into for LogLevel { - fn into(self) -> Level { - match self { - LogLevel::Trace => Level::Trace, - LogLevel::Debug => Level::Debug, - LogLevel::Info => Level::Info, - LogLevel::Warn => Level::Warn, - LogLevel::Error => Level::Error, - } - } - } - #[derive(Serialize, Deserialize, Debug)] struct LoggingModuleConfigMap(BTreeMap); From a3f771ca65be082331fa69e8e7fcb97d9b1b33fb Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Sat, 27 Apr 2019 01:21:48 +0200 Subject: [PATCH 3/5] Add comment why we do this Signed-off-by: Matthias Beyer --- lib/core/libimagrt/src/logger.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/core/libimagrt/src/logger.rs b/lib/core/libimagrt/src/logger.rs index 956d251d..e7c03315 100644 --- a/lib/core/libimagrt/src/logger.rs +++ b/lib/core/libimagrt/src/logger.rs @@ -352,6 +352,16 @@ fn aggregate_module_settings(_matches: &ArgMatches, config: Option<&Value>) use toml_query::read::Partial; use std::convert::TryInto; + // + // We define helper types here for deserializing easily using typed toml-query functionality. + // + // We need the helper types because we cannot deserialize in the target types directly, because + // of the `File(Arc>)` variant in `LogDestination`, which would + // technically possible to deserialize the toml into the type, but it might be a bad idea. + // + // This code is idomatic enough for the conversions, so it is not a big painpoint. + // + #[derive(Serialize, Deserialize, Debug)] struct LoggingModuleConfig { pub destinations: Option>, @@ -382,7 +392,7 @@ fn aggregate_module_settings(_matches: &ArgMatches, config: Option<&Value>) Some(ds) => Some(ds .iter() .map(Deref::deref) - .map(translate_destination) + .map(translate_destination) // This is why we do this whole thing .collect::>>()?) }, }); From d5eb9185232c0ff015e44106701fcf26b00a18bd Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Sat, 27 Apr 2019 02:06:24 +0200 Subject: [PATCH 4/5] travis: Update rustc Because we depend on try_into in these patches, we have to bump the rustc version to 1.34.0 minimum here. And because we do support the last three rust stable compilers, this has to wait. Signed-off-by: Matthias Beyer --- .travis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 66a3540b..f76bda92 100644 --- a/.travis.yml +++ b/.travis.yml @@ -16,7 +16,7 @@ matrix: - bash ./scripts/branch-contains-no-tmp-commits - bash ./scripts/version-updated - language: rust - rust: 1.33.0 + rust: 1.34.0 cache: directories: - /home/travis/.cargo @@ -26,7 +26,7 @@ matrix: - cargo build --all --all-features -j 1 || exit 1 - cargo test --all --all-features -j 1 || exit 1 - language: rust - rust: 1.34.2 + rust: 1.35.0 cache: directories: - /home/travis/.cargo From 5f5ce54edb90005fedb072356cd5053c9ac5deea Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Sat, 11 May 2019 13:37:14 +0200 Subject: [PATCH 5/5] Rewrite to use derive macro Signed-off-by: Matthias Beyer --- lib/core/libimagrt/src/lib.rs | 4 ++-- lib/core/libimagrt/src/logger.rs | 9 ++------- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/lib/core/libimagrt/src/lib.rs b/lib/core/libimagrt/src/lib.rs index 4247ff6c..ebe8f4fc 100644 --- a/lib/core/libimagrt/src/lib.rs +++ b/lib/core/libimagrt/src/lib.rs @@ -37,17 +37,17 @@ while_true, )] -#[macro_use] extern crate log; +extern crate log; extern crate itertools; extern crate ansi_term; extern crate handlebars; extern crate serde; #[macro_use] extern crate serde_derive; #[macro_use] extern crate failure; +#[macro_use] extern crate toml_query; extern crate clap; extern crate toml; -extern crate toml_query; extern crate atty; extern crate libimagstore; diff --git a/lib/core/libimagrt/src/logger.rs b/lib/core/libimagrt/src/logger.rs index e7c03315..ebb9ec49 100644 --- a/lib/core/libimagrt/src/logger.rs +++ b/lib/core/libimagrt/src/logger.rs @@ -349,7 +349,6 @@ mod log_lvl_aggregate { fn aggregate_module_settings(_matches: &ArgMatches, config: Option<&Value>) -> Result> { - use toml_query::read::Partial; use std::convert::TryInto; // @@ -369,14 +368,10 @@ fn aggregate_module_settings(_matches: &ArgMatches, config: Option<&Value>) pub enabled: bool, } - #[derive(Serialize, Deserialize, Debug)] + #[derive(Partial, Serialize, Deserialize, Debug)] + #[location = "imag.logging.modules"] struct LoggingModuleConfigMap(BTreeMap); - impl<'a> Partial<'a> for LoggingModuleConfigMap { - const LOCATION: &'static str = "imag.logging.modules"; - type Output = Self; - } - impl TryInto> for LoggingModuleConfigMap { type Error = Error;