diff --git a/src/rust/engine/logging/src/lib.rs b/src/rust/engine/logging/src/lib.rs index c08e452ec1d6..3fcc195c81fb 100644 --- a/src/rust/engine/logging/src/lib.rs +++ b/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 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 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 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, + } + } +} diff --git a/src/rust/engine/logging/src/logger.rs b/src/rust/engine/logging/src/logger.rs index 554e9b0b52f6..d72ef9f9fc99 100644 --- a/src/rust/engine/logging/src/logger.rs +++ b/src/rust/engine/logging/src/logger.rs @@ -1,6 +1,5 @@ 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; @@ -8,70 +7,12 @@ 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 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 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 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, pantsd_log: Mutex>, @@ -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 MaybeWriteLogger { MaybeWriteLogger { level: level, inner: Some(WriteLogger::new( - LevelFilter::Trace, + LevelFilter::max(), Config::default(), writable, )), @@ -182,10 +124,6 @@ impl MaybeWriteLogger { pub fn level(&self) -> LevelFilter { self.level } - - pub fn set_level(&mut self, new_level: LevelFilter) { - self.level = new_level - } } impl Log for MaybeWriteLogger { @@ -203,6 +141,8 @@ impl Log for MaybeWriteLogger { } fn flush(&self) { - unimplemented!() + if let Some(ref logger) = self.inner { + logger.flush(); + } } } diff --git a/src/rust/engine/src/externs.rs b/src/rust/engine/src/externs.rs index 09bc3175f409..655e5292d9fe 100644 --- a/src/rust/engine/src/externs.rs +++ b/src/rust/engine/src/externs.rs @@ -243,17 +243,6 @@ lazy_static! { static ref INTERNS: RwLock = 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) {} -//} diff --git a/src/rust/engine/src/lib.rs b/src/rust/engine/src/lib.rs index 2b013eb5eea8..628dbd3df95c 100644 --- a/src/rust/engine/src/lib.rs +++ b/src/rust/engine/src/lib.rs @@ -29,10 +29,6 @@ )] // Arc 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"); }