WIP - Address assorted Rust comments

src/rust/engine/logging/src/lib.rs

@@ -44,3 +44,66 @@ extern crate simplelog;
 extern crate ui;
 
 pub type Logger = logger::Logger;
+
+// TODO My intuition is that these belog here, because putting them outside of the `logging`
+// lcrate would expose concepts like `Level` and `LevelFilter` to the outside.
+use num_enum::CustomTryInto;
+
+// This is a hard-coding of constants in the standard logging python package.
+// TODO: Switch from CustomTryInto to TryFromPrimitive when try_from is stable.
+#[derive(Debug, Eq, PartialEq, CustomTryInto, Clone, Copy)]
+#[repr(u64)]
+enum PythonLogLevel {
+  NotSet = 0,
+  // Trace doesn't exist in a Python world, so set it to "a bit lower than Debug".
+  Trace = 5,
+  Debug = 10,
+  Info = 20,
+  Warn = 30,
+  Error = 40,
+  Critical = 50,
+}
+
+impl From<log::Level> for PythonLogLevel {
+  fn from(level: log::Level) -> Self {
+    match level {
+      log::Level::Error => PythonLogLevel::Error,
+      log::Level::Warn => PythonLogLevel::Warn,
+      log::Level::Info => PythonLogLevel::Info,
+      log::Level::Debug => PythonLogLevel::Debug,
+      log::Level::Trace => PythonLogLevel::Trace,
+    }
+  }
+}
+
+impl From<PythonLogLevel> for log::LevelFilter {
+  fn from(level: PythonLogLevel) -> Self {
+    match level {
+      PythonLogLevel::NotSet => log::LevelFilter::Off,
+      PythonLogLevel::Trace => log::LevelFilter::Trace,
+      PythonLogLevel::Debug => log::LevelFilter::Debug,
+      PythonLogLevel::Info => log::LevelFilter::Info,
+      PythonLogLevel::Warn => log::LevelFilter::Warn,
+      PythonLogLevel::Error => log::LevelFilter::Error,
+      // Rust doesn't have a Critical, so treat them like Errors.
+      PythonLogLevel::Critical => log::LevelFilter::Error,
+    }
+  }
+}
+
+impl From<PythonLogLevel> for log::Level {
+  fn from(level: PythonLogLevel) -> Self {
+    match level {
+      PythonLogLevel::NotSet => {
+        panic!("PythonLogLevel::NotSet doesn't have a translation to Level")
+      }
+      PythonLogLevel::Trace => log::Level::Trace,
+      PythonLogLevel::Debug => log::Level::Debug,
+      PythonLogLevel::Info => log::Level::Info,
+      PythonLogLevel::Warn => log::Level::Warn,
+      PythonLogLevel::Error => log::Level::Error,
+      // Rust doesn't have a Critical, so treat them like Errors.
+      PythonLogLevel::Critical => log::Level::Error,
+    }
+  }
+}

src/rust/engine/logging/src/logger.rs

@@ -1,77 +1,18 @@
 use lazy_static::lazy_static;
 use log::{log, set_logger, set_max_level, LevelFilter, Log, Metadata, Record};
-use num_enum::CustomTryInto;
 use parking_lot::{Mutex, RwLock};
 use simplelog::Config;
 use simplelog::WriteLogger;
 use std::fs::File;
 use std::fs::OpenOptions;
 use std::io::{stderr, Stderr, Write};
 use std::path::PathBuf;
+use TryIntoPythonLogLevel;
 
 lazy_static! {
   pub static ref LOGGER: Logger = Logger::new(LevelFilter::Off);
 }
 
-// This is a hard-coding of constants in the standard logging python package.
-// TODO: Switch from CustomTryInto to TryFromPrimitive when try_from is stable.
-#[derive(Debug, Eq, PartialEq, CustomTryInto, Clone, Copy)]
-#[repr(u64)]
-enum PythonLogLevel {
-  NotSet = 0,
-  // Trace doesn't exist in a Python world, so set it to "a bit lower than Debug".
-  Trace = 5,
-  Debug = 10,
-  Info = 20,
-  Warn = 30,
-  Error = 40,
-  Critical = 50,
-}
-
-impl From<log::Level> for PythonLogLevel {
-  fn from(level: log::Level) -> Self {
-    match level {
-      log::Level::Error => PythonLogLevel::Error,
-      log::Level::Warn => PythonLogLevel::Warn,
-      log::Level::Info => PythonLogLevel::Info,
-      log::Level::Debug => PythonLogLevel::Debug,
-      log::Level::Trace => PythonLogLevel::Trace,
-    }
-  }
-}
-
-impl From<PythonLogLevel> for log::LevelFilter {
-  fn from(level: PythonLogLevel) -> Self {
-    match level {
-      PythonLogLevel::NotSet => log::LevelFilter::Off,
-      PythonLogLevel::Trace => log::LevelFilter::Trace,
-      PythonLogLevel::Debug => log::LevelFilter::Debug,
-      PythonLogLevel::Info => log::LevelFilter::Info,
-      PythonLogLevel::Warn => log::LevelFilter::Warn,
-      PythonLogLevel::Error => log::LevelFilter::Error,
-      // Rust doesn't have a Critical, so treat them like Errors.
-      PythonLogLevel::Critical => log::LevelFilter::Error,
-    }
-  }
-}
-
-impl From<PythonLogLevel> for log::Level {
-  fn from(level: PythonLogLevel) -> Self {
-    match level {
-      PythonLogLevel::NotSet => {
-        panic!("PythonLogLevel::NotSet doesn't have a translation to Level")
-      }
-      PythonLogLevel::Trace => log::Level::Trace,
-      PythonLogLevel::Debug => log::Level::Debug,
-      PythonLogLevel::Info => log::Level::Info,
-      PythonLogLevel::Warn => log::Level::Warn,
-      PythonLogLevel::Error => log::Level::Error,
-      // Rust doesn't have a Critical, so treat them like Errors.
-      PythonLogLevel::Critical => log::Level::Error,
-    }
-  }
-}
-
 pub struct Logger {
   level: RwLock<LevelFilter>,
   pantsd_log: Mutex<MaybeWriteLogger<File>>,
@@ -92,7 +33,7 @@ impl Logger {
     match max_python_level {
       Ok(python_level) => {
         let level: log::LevelFilter = python_level.into();
-        (*LOGGER).set_level(level);
+        LOGGER.set_level(level);
         set_logger(&*LOGGER).expect("Error setting up global logger.");
       }
       Err(err) => panic!("Unrecognised log level from python: {}: {}", max_level, err),
@@ -125,7 +66,7 @@ impl Logger {
         .open(log_file_path)
         .map(|file| {
           *self.pantsd_log.lock() = MaybeWriteLogger::new(file, level.into());
-        }).map_err(|err| format!("{}", err))
+        }).map_err(|err| format!("Error opening pantsd logfile: {}", err))
     })
   }
 
@@ -151,7 +92,8 @@ impl Log for Logger {
   }
 
   fn flush(&self) {
-    unimplemented!()
+    self.stderr_log.lock().flush();
+    self.pantsd_log.lock().flush();
   }
 }
 
@@ -172,7 +114,7 @@ impl<W: Write + Send + 'static> MaybeWriteLogger<W> {
     MaybeWriteLogger {
       level: level,
       inner: Some(WriteLogger::new(
-        LevelFilter::Trace,
+        LevelFilter::max(),
         Config::default(),
         writable,
       )),
@@ -182,10 +124,6 @@ impl<W: Write + Send + 'static> MaybeWriteLogger<W> {
   pub fn level(&self) -> LevelFilter {
     self.level
   }
-
-  pub fn set_level(&mut self, new_level: LevelFilter) {
-    self.level = new_level
-  }
 }
 
 impl<W: Write + Send + 'static> Log for MaybeWriteLogger<W> {
@@ -203,6 +141,8 @@ impl<W: Write + Send + 'static> Log for MaybeWriteLogger<W> {
   }
 
   fn flush(&self) {
-    unimplemented!()
+    if let Some(ref logger) = self.inner {
+      logger.flush();
+    }
   }
 }

src/rust/engine/src/externs.rs

@@ -243,17 +243,6 @@ lazy_static! {
   static ref INTERNS: RwLock<Interns> = RwLock::new(Interns::new());
 }
 
-// This is mut so that the max level can be set via set_externs.
-// It should only be set exactly once, and nothing should ever read it (it is only defined to
-// prevent the FfiLogger from being dropped).
-// In order to avoid a performance hit, there is no lock guarding it (because if it had a lock, it
-// would need to be acquired for every single logging statement).
-// Please don't mutate it.
-// Please.
-//static mut LOGGER: FfiLogger = FfiLogger {
-//  level_filter: log::LevelFilter::Off,
-//};
-
 ///
 /// Set the static Externs for this process. All other methods of this module will fail
 /// until this has been called.
@@ -602,55 +591,3 @@ where
   mem::forget(cs);
   output
 }
-
-/////
-///// FfiLogger is an implementation of log::Log which asks the Python logging system to log via cffi.
-/////
-//struct FfiLogger {
-//  level_filter: log::LevelFilter,
-//}
-//
-//impl FfiLogger {
-//  // init must only be called once in the lifetime of the program. No other loggers may be init'd.
-//  // If either of the above are violated, expect a panic.
-//  pub fn init(&'static mut self, max_level: u8) {
-//    let max_python_level = max_level.try_into_PythonLogLevel();
-//    self.level_filter = {
-//      match max_python_level {
-//        Ok(python_level) => {
-//          let level: log::LevelFilter = python_level.into();
-//          level
-//        }
-//        Err(err) => panic!("Unrecognised log level from python: {}: {}", max_level, err),
-//      }
-//    };
-//
-//    log::set_max_level(self.level_filter);
-//    log::set_logger(self)
-//      .expect("Failed to set logger (maybe you tried to call init multiple times?)");
-//  }
-//}
-//
-//impl log::Log for FfiLogger {
-//  fn enabled(&self, metadata: &log::Metadata) -> bool {
-//    metadata.level() <= self.level_filter
-//  }
-//
-//  fn log(&self, record: &log::Record) {
-//    if !self.enabled(record.metadata()) {
-//      return;
-//    }
-//    let level: PythonLogLevel = record.level().into();
-//    let message = format!("{}", record.args());
-//    with_externs(|e| {
-//      (e.log)(
-//        e.context,
-//        level as u8,
-//        message.as_ptr(),
-//        message.len() as u64,
-//      )
-//    })
-//  }
-//
-//  fn flush(&self) {}
-//}

src/rust/engine/src/lib.rs

@@ -29,10 +29,6 @@
 )]
 // Arc<Mutex> can be more clear than needing to grok Orderings:
 #![cfg_attr(feature = "cargo-clippy", allow(mutex_atomic))]
-// We only use unsafe pointer derefrences in our no_mangle exposed API, but it is nicer to list
-// just the one minor call as unsafe, than to mark the whole function as unsafe which may hide
-// other unsafeness.
-#![cfg_attr(feature = "cargo-clippy", allow(not_unsafe_ptr_arg_deref))]
 
 pub mod cffi_externs;
 mod context;
@@ -71,6 +67,7 @@ extern crate tokio;
 extern crate ui;
 extern crate url;
 
+use std::borrow::Borrow;
 use std::ffi::CStr;
 use std::fs::File;
 use std::io;
@@ -801,9 +798,9 @@ pub extern "C" fn setup_stderr_logger(level: u64) {
 
 #[no_mangle]
 pub extern "C" fn write_log(msg: *const raw::c_char, level: u64) {
-  let message_str = unsafe { CStr::from_ptr(msg).to_string_lossy().into_owned() };
+  let message_str = unsafe { CStr::from_ptr(msg).to_string_lossy() };
   LOGGER
-    .log_from_python(&message_str, level)
+    .log_from_python(message_str.borrow(), level)
     .expect("Error logging message");
 }