Fix logger initialization on readonly filesystems

logger_core/src/lib.rs

@@ -2,9 +2,12 @@
  * Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0
  */
 use once_cell::sync::OnceCell;
-use std::sync::RwLock;
+use std::{
+    path::{Path, PathBuf},
+    sync::RwLock,
+};
 use tracing::{self, event};
-use tracing_appender::rolling::{RollingFileAppender, Rotation};
+use tracing_appender::rolling::{RollingFileAppender, RollingWriter, Rotation};
 use tracing_subscriber::{
     filter::Filtered,
     fmt::{
@@ -28,7 +31,7 @@ type InnerLayered = Layered<reload::Layer<InnerFiltered, Registry>, Registry>;
 // A reloadable layer of subscriber to a rolling file
 type FileReload = Handle<
     Filtered<
-        Layer<InnerLayered, DefaultFields, Format, RollingFileAppender>,
+        Layer<InnerLayered, DefaultFields, Format, LazyRollingFileAppender>,
         LevelFilter,
         InnerLayered,
     >,
@@ -48,6 +51,47 @@ pub static INITIATE_ONCE: InitiateOnce = InitiateOnce {
     init_once: OnceCell::new(),
 };
 
+const FILE_DIRECTORY: &str = "glide-logs";
+
+/// Wraps [RollingFileAppender] to defer initialization until logging is required,
+/// allowing [init] to disable file logging on read-only filesystems.
+/// This is needed because [RollingFileAppender] tries to create the log directory on initialization.
+struct LazyRollingFileAppender {
+    file_appender: OnceCell<RollingFileAppender>,
+    rotation: Rotation,
+    directory: PathBuf,
+    filename_prefix: PathBuf,
+}
+
+impl LazyRollingFileAppender {
+    fn new(
+        rotation: Rotation,
+        directory: impl AsRef<Path>,
+        filename_prefix: impl AsRef<Path>,
+    ) -> LazyRollingFileAppender {
+        LazyRollingFileAppender {
+            file_appender: OnceCell::new(),
+            rotation,
+            directory: directory.as_ref().to_path_buf(),
+            filename_prefix: filename_prefix.as_ref().to_path_buf(),
+        }
+    }
+}
+
+impl<'a> tracing_subscriber::fmt::writer::MakeWriter<'a> for LazyRollingFileAppender {
+    type Writer = RollingWriter<'a>;
+    fn make_writer(&'a self) -> Self::Writer {
+        let file_appender = self.file_appender.get_or_init(|| {
+            RollingFileAppender::new(
+                self.rotation.clone(),
+                self.directory.clone(),
+                self.filename_prefix.clone(),
+            )
+        });
+        file_appender.make_writer()
+    }
+}
+
 #[derive(Debug)]
 pub enum Level {
     Error = 0,
@@ -84,9 +128,9 @@ pub fn init(minimal_level: Option<Level>, file_name: Option<&str>) -> Level {
 
         let (stdout_layer, stdout_reload) = reload::Layer::new(stdout_fmt);
 
-        let file_appender = RollingFileAppender::new(
+        let file_appender = LazyRollingFileAppender::new(
             Rotation::HOURLY,
-            "glide-logs",
+            FILE_DIRECTORY,
             file_name.unwrap_or("output.log"),
         );
         let file_fmt = tracing_subscriber::fmt::layer()
@@ -130,7 +174,8 @@ pub fn init(minimal_level: Option<Level>, file_name: Option<&str>) -> Level {
                 });
         }
         Some(file) => {
-            let file_appender = RollingFileAppender::new(Rotation::HOURLY, "glide-logs", file);
+            let file_appender =
+                LazyRollingFileAppender::new(Rotation::HOURLY, FILE_DIRECTORY, file);
             let _ = reloads
                 .file_reload
                 .write()

logger_core/tests/test_logger.rs

@@ -9,7 +9,10 @@ use test_env_helpers::*;
 mod tests {
     use logger_core::{init, log_debug, log_trace};
     use rand::{distributions::Alphanumeric, Rng};
-    use std::fs::{read_dir, read_to_string, remove_dir_all};
+    use std::{
+        fs::{read_dir, read_to_string, remove_dir_all},
+        path::Path,
+    };
     const FILE_DIRECTORY: &str = "glide-logs";
 
     fn generate_random_string(length: usize) -> String {
@@ -38,6 +41,13 @@ mod tests {
         read_to_string(file.unwrap().path()).unwrap()
     }
 
+    #[test]
+    fn init_does_not_create_log_directory_when_console_init() {
+        init(Some(logger_core::Level::Trace), None);
+        let dir_exists = Path::new(FILE_DIRECTORY).is_dir();
+        assert!(!dir_exists);
+    }
+
     #[test]
     fn log_to_console_works_after_multiple_inits_diff_log_level() {
         let identifier = generate_random_string(10);
@@ -49,6 +59,16 @@ mod tests {
         log_trace(identifier, "boo");
     }
 
+    #[test]
+    fn log_to_console_does_not_create_log_directory_when_console_init() {
+        let identifier = generate_random_string(10);
+        init(Some(logger_core::Level::Trace), None);
+        // you should see in the console something like '2023-07-07T06:57:54.446236Z TRACE logger_core: e49NaJ5J41 - foo'
+        log_trace(identifier.clone(), "foo");
+        let dir_exists = Path::new(FILE_DIRECTORY).is_dir();
+        assert!(!dir_exists);
+    }
+
     #[test]
     fn log_to_file_works_after_multiple_inits() {
         let identifier = generate_random_string(10);
@@ -84,14 +104,20 @@ mod tests {
     }
 
     #[test]
-    fn log_to_file_disabled_when_console_init() {
+    fn log_to_file_disabled_after_console_init() {
         let identifier = generate_random_string(10);
         init(Some(logger_core::Level::Trace), Some(identifier.as_str()));
-        init(Some(logger_core::Level::Trace), None);
-        // you should see in the console something like '2023-07-07T06:57:54.446236Z TRACE logger_core: e49NaJ5J41 - foo'
         log_trace(identifier.clone(), "foo");
+        init(Some(logger_core::Level::Trace), None);
+        log_trace(identifier.clone(), "boo");
         let contents = get_file_contents(identifier.as_str());
-        assert!(contents.is_empty());
+        assert!(
+            contents.contains(identifier.as_str()),
+            "Contents: {}",
+            contents
+        );
+        assert!(contents.contains("foo"), "Contents: {}", contents);
+        assert!(!contents.contains("boo"), "Contents: {}", contents);
     }
 
     fn clean() -> Result<(), std::io::Error> {