Implemented logger indirection to avoid issues with running the knoll…

… command multiple times in the same process. Continued testing refinement.

src/indirect_logger.rs

@@ -0,0 +1,56 @@
+use log::{set_boxed_logger, set_max_level, Log, SetLoggerError};
+use simplelog::SharedLogger;
+use std::ops::Deref;
+use std::sync::{Arc, RwLock};
+
+/// An indirect logger is an implementation of log::Log that delegates
+/// all calls to a simplelog::SharedLogger.  This allows the logger to be
+/// updated at runtime.  
+///
+/// This is not really necessary for ordinary operation, but is useful in
+/// testing where we may invoke the knoll command multiple times in the
+/// same process lifetime.
+#[derive(Clone)]
+pub struct IndirectLogger {
+    logger: Arc<RwLock<Box<dyn SharedLogger>>>,
+}
+
+impl IndirectLogger {
+    /// Update this IndirectLogger to make use of the new logger in
+    /// subsequent calls to log::Log functions.
+    pub fn update(&self, logger: Box<dyn SharedLogger>) {
+        set_max_level(logger.level());
+        *self.logger.write().unwrap() = logger;
+    }
+
+    /// Construct a new IndirectLogger that delegates to the given logger.
+    pub fn new(logger: Box<dyn SharedLogger>) -> Self {
+        IndirectLogger {
+            logger: Arc::new(RwLock::new(logger)),
+        }
+    }
+
+    /// Initialize the global logger with the given logger wrapped by
+    /// an IndirectLogger.  This function returns a clone of IndirectLogger
+    /// that can be used to update the logger at a later time.
+    pub fn init(logger: Box<dyn SharedLogger>) -> Result<Self, SetLoggerError> {
+        set_max_level(logger.deref().level());
+        let indirect_logger = IndirectLogger::new(logger);
+        set_boxed_logger(Box::new(indirect_logger.clone()))?;
+        Ok(indirect_logger)
+    }
+}
+
+impl Log for IndirectLogger {
+    fn enabled(&self, metadata: &log::Metadata) -> bool {
+        self.logger.read().unwrap().deref().enabled(metadata)
+    }
+
+    fn log(&self, record: &log::Record) {
+        self.logger.read().unwrap().deref().log(record)
+    }
+
+    fn flush(&self) {
+        self.logger.read().unwrap().deref().flush()
+    }
+}

src/knoll.rs

@@ -2,18 +2,19 @@ use clap::{Arg, ArgAction, ArgMatches, Command};
 use humantime;
 use log::*;
 use serde::Serialize;
-use simplelog::{TermLogger, WriteLogger};
+use simplelog::{SharedLogger, TermLogger, WriteLogger};
 use std::collections::HashMap;
 use std::fmt::Formatter;
 use std::io::IsTerminal;
 use std::io::{BufReader, Read, Write};
 use std::path::PathBuf;
-use std::sync::{Condvar, Mutex};
+use std::sync::{Condvar, LazyLock, Mutex, RwLock};
 
 use crate::config::*;
 use crate::core_graphics;
 use crate::displays;
 use crate::displays::*;
+use crate::indirect_logger::IndirectLogger;
 use crate::serde::serialize_to_string;
 use crate::valid_config;
 use crate::valid_config::*;
@@ -167,29 +168,42 @@ fn verbosity_to_filter(verbosity: usize) -> LevelFilter {
     }
 }
 
+/// A handle to the current global IndirectLogger.
+static GLOBAL_LOGGER: LazyLock<RwLock<Option<IndirectLogger>>> =
+    LazyLock::new(|| RwLock::new(None));
+
 /// Helper to configure the logger by verbosity and depending on whether it
 /// is writing to a terminal or not.
 fn configure_logger<ERR: Write + IsTerminal + Send + 'static>(
     verbosity: usize,
     stderr: ERR,
-) -> Result<(), Error> {
+) -> Result<(), SetLoggerError> {
     let mut config_builder = simplelog::ConfigBuilder::new();
     config_builder.set_time_format_rfc3339();
 
     let level_filter = verbosity_to_filter(verbosity);
-    if stderr.is_terminal() {
+    let session_logger: Box<dyn SharedLogger> = if stderr.is_terminal() {
         // If the destination is a terminal, use the `Termlogger`.
-        TermLogger::init(
+        TermLogger::new(
             level_filter,
             config_builder.build(),
             simplelog::TerminalMode::Stderr,
             simplelog::ColorChoice::Auto,
-        )?
+        )
     } else {
         // Otherwise just use a plain `WriteLogger`.
-        WriteLogger::init(level_filter, config_builder.build(), stderr)?
+        WriteLogger::new(level_filter, config_builder.build(), stderr)
     };
 
+    // Update or initialize the global logger.
+    let mut opt_logger = GLOBAL_LOGGER.write().unwrap();
+    match opt_logger.as_mut() {
+        Some(logger) => logger.update(session_logger),
+        None => {
+            *opt_logger = Some(IndirectLogger::init(session_logger)?);
+        }
+    }
+
     Ok(())
 }
 
@@ -571,7 +585,10 @@ fn configure_displays<DS: DisplayState>(
         }
     }
 
-    Ok(cfgtxn.commit()?)
+    cfgtxn.commit()?;
+    info!("Configuration complete.");
+
+    Ok(())
 }
 
 ////////////////////////////////////////////////////////////////////////////////

src/lib.rs

@@ -9,6 +9,7 @@ pub mod config;
 pub mod core_graphics;
 pub mod displays;
 pub mod fake_displays;
+pub mod indirect_logger;
 pub mod knoll;
 pub mod real_displays;
 mod serde;

src/main.rs

@@ -10,6 +10,7 @@ mod config;
 mod core_graphics;
 mod displays;
 mod fake_displays;
+pub mod indirect_logger;
 mod knoll;
 mod real_displays;
 mod serde;

tests/knoll_tests.rs

@@ -4,80 +4,125 @@ extern crate knoll;
 use coverage_helper::test;
 use knoll::displays::DisplayState;
 use knoll::fake_displays::FakeDisplayState;
-use knoll::knoll::run;
+use knoll::knoll::{run, Error};
 use knoll::real_displays::*;
-use std::io::Read;
+use std::io::{Read, Write};
 use tempfile::tempdir;
 
 #[cfg_attr(all(coverage_nightly, test), coverage(off))]
-/// Run the knoll command with the given arguments and return the output to
-/// stdout and stderr.
+/// Run the knoll command with the given arguments an optional input and
+/// returns whether the invoked resulted in an error and the text output
+/// to stdout and stderr.
+///
 /// The only caveat is that it is currently not possible to override output
 /// destinations for the clap argument parser when using `--help` or
-/// `--version`.  So at present we cannot capture the output of these options.
-fn run_knoll<DS: DisplayState>(args: Vec<&str>) -> (String, String) {
+/// `--version`.  So at present we cannot capture the output of these
+/// options.
+fn run_knoll<DS: DisplayState>(
+    args: Vec<&str>,
+    input: Option<String>,
+) -> (Option<Error>, String, String) {
     let mut vec_out: Vec<u8> = Vec::new();
     let dir = tempdir().expect("Failed to create temporary directory.");
-    let path = dir.path().join("stderr");
-    let file_err = std::fs::File::create(path.clone()).expect("Failed to open temporary file.");
-    let _ = run::<DS, std::io::Stdin, &mut Vec<u8>, std::fs::File>(
-        &args.into_iter().map(|s| String::from(s)).collect(),
-        std::io::stdin(),
-        &mut vec_out,
-        file_err.try_clone().expect("Clone failed"),
-    );
+    let err_path = dir.path().join("stderr");
+    let file_err =
+        std::fs::File::create(err_path.clone()).expect("Failed to open temporary file for stderr.");
+    let file_err_clone = file_err.try_clone().expect("Cloning stderr file failed");
+    let res = if let Some(input_str) = input {
+        let in_path = dir.path().join("stdin");
+        let mut file_in = std::fs::File::create(in_path.clone())
+            .expect("Failed to open temporary file for stdin.");
+        file_in
+            .write(input_str.as_bytes())
+            .expect("Failed to write to file.");
+        file_in.flush().expect("Failed to flush file.");
+        // Need to reopen the file for knoll to read from it.
+        let file_in =
+            std::fs::File::open(in_path).expect("Failed to open temporary file for stdin.");
+        run::<DS, std::fs::File, &mut Vec<u8>, std::fs::File>(
+            &args.into_iter().map(|s| String::from(s)).collect(),
+            file_in,
+            &mut vec_out,
+            file_err_clone,
+        )
+    } else {
+        run::<DS, std::io::Stdin, &mut Vec<u8>, std::fs::File>(
+            &args.into_iter().map(|s| String::from(s)).collect(),
+            std::io::stdin(),
+            &mut vec_out,
+            file_err_clone,
+        )
+    };
+    // Convert the result to an option
+    let opt_error = match res {
+        Ok(_) => None,
+        Err(e) => Some(e),
+    };
 
-    let mut file_err = std::fs::File::open(path).expect("Failed to open temporary file.");
+    let mut file_err =
+        std::fs::File::open(err_path).expect("Failed to open temporary file for stderr.");
     let mut string_err = String::new();
     file_err.read_to_string(&mut string_err).unwrap();
 
-    (String::from_utf8(vec_out).unwrap(), string_err)
+    (opt_error, String::from_utf8(vec_out).unwrap(), string_err)
 }
 
 #[cfg_attr(all(coverage_nightly, test), coverage(off))]
-/// Run the knoll commmand with the given arguments using real displays.
-fn run_knoll_real(args: Vec<&str>) -> (String, String) {
-    run_knoll::<RealDisplayState>(args)
+/// Run the knoll command with the given arguments using real displays.
+fn run_knoll_real(args: Vec<&str>, input: Option<String>) -> (Option<Error>, String, String) {
+    run_knoll::<RealDisplayState>(args, input)
 }
 
 #[cfg_attr(all(coverage_nightly, test), coverage(off))]
-/// Run the knoll commmand with the given arguments using fake displays.
-fn run_knoll_fake(args: Vec<&str>) -> (String, String) {
-    run_knoll::<FakeDisplayState>(args)
+/// Run the knoll command with the given arguments using fake displays.
+fn run_knoll_fake(args: Vec<&str>, input: Option<String>) -> (Option<Error>, String, String) {
+    run_knoll::<FakeDisplayState>(args, input)
 }
 
 #[test]
 /// Test the knoll --help command
 fn test_help() {
-    run_knoll_real(vec!["knoll", "--help"]);
+    run_knoll_real(vec!["knoll", "--help"], None);
 }
 
 #[test]
 /// Test the knoll --version command
 fn test_version() {
-    run_knoll_real(vec!["knoll", "--version"]);
+    run_knoll_real(vec!["knoll", "--version"], None);
 }
 
 #[test]
 /// Test the default knoll command behavior with real displays.
 fn test_real_default() {
-    run_knoll_real(vec!["knoll", "-vvv"]);
+    let (opt_err, stdout, stderr) = run_knoll_real(vec!["knoll", "-vvv"], None);
+    // Verify that no error occurred.
+    assert!(opt_err.is_none());
+    // Verify that no display configuration took place.
+    assert!(!stderr.contains("Configuration complete."));
+    // Run idempotency test.
+    let (opt_err, stdout_new, stderr) = run_knoll_real(vec!["knoll", "-vvv"], Some(stdout.clone()));
+    // Verify that no error occurred.
+    assert!(opt_err.is_none());
+    // Verify that display configuration did happen.
+    assert!(stderr.contains("Configuration complete."));
+    // Results should be unchanged.
+    assert_eq!(stdout, stdout_new);
 }
 
 #[test]
 /// Test the default knoll list command behavior with real displays.
 fn test_real_list() {
-    run_knoll_real(vec!["knoll", "list"]);
+    run_knoll_real(vec!["knoll", "list"], None);
 }
 
 #[test]
 /// Test the default knoll command behavior with fake displays.
 fn test_fake_default() {
-    run_knoll_fake(vec!["knoll", "-vvv"]);
+    run_knoll_fake(vec!["knoll", "-vvv"], None);
 }
 
 #[test]
 /// Test the default knoll list command behavior with fake displays.
 fn test_fake_list() {
-    run_knoll_fake(vec!["knoll", "list"]);
+    run_knoll_fake(vec!["knoll", "list"], None);
 }