From 90978b65538c92297e244655e68573eeba0109c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20Vinyals=20Valdepe=C3=B1as?= Date: Sat, 8 Jul 2023 20:10:44 +0200 Subject: [PATCH 1/2] fix & boyscout: ensure loader caches correct log file Log file can be customized with `--log` argument, the `open-log` command should respect that. As part of this work, also done some boy-scouting around initialisation of directories, cleaning up and making sure they happen in the same place. --- helix-loader/src/lib.rs | 39 +++++++++++++++++++++++++++------------ helix-term/src/main.rs | 25 +++++++------------------ 2 files changed, 34 insertions(+), 30 deletions(-) diff --git a/helix-loader/src/lib.rs b/helix-loader/src/lib.rs index ad4ad899db67..c11a97cab37c 100644 --- a/helix-loader/src/lib.rs +++ b/helix-loader/src/lib.rs @@ -11,19 +11,23 @@ static RUNTIME_DIRS: once_cell::sync::Lazy> = static CONFIG_FILE: once_cell::sync::OnceCell = once_cell::sync::OnceCell::new(); -pub fn initialize_config_file(specified_file: Option) { - let config_file = specified_file.unwrap_or_else(|| { - let config_dir = config_dir(); +static LOG_FILE: once_cell::sync::OnceCell = once_cell::sync::OnceCell::new(); - if !config_dir.exists() { - std::fs::create_dir_all(&config_dir).ok(); - } +pub fn initialize_config_file(specified_file: Option) { + let config_file = specified_file.unwrap_or_else(default_config_file); + CONFIG_FILE.set(config_file).ok(); +} - config_dir.join("config.toml") - }); +pub fn initialize_log_file(specified_file: Option) { + let log_file = specified_file.unwrap_or_else(default_log_file); + LOG_FILE.set(log_file).ok(); +} - // We should only initialize this value once. - CONFIG_FILE.set(config_file).ok(); +pub fn ensure_config_dir() { + let config_dir = config_dir(); + if !config_dir.exists() { + std::fs::create_dir_all(&config_dir).ok(); + } } /// A list of runtime directories from highest to lowest priority @@ -121,11 +125,15 @@ pub fn cache_dir() -> PathBuf { path } +pub fn default_config_file() -> PathBuf { + config_dir().join("config.toml") +} + pub fn config_file() -> PathBuf { CONFIG_FILE .get() .map(|path| path.to_path_buf()) - .unwrap_or_else(|| config_dir().join("config.toml")) + .unwrap_or_else(default_config_file) } pub fn workspace_config_file() -> PathBuf { @@ -136,10 +144,17 @@ pub fn lang_config_file() -> PathBuf { config_dir().join("languages.toml") } -pub fn log_file() -> PathBuf { +pub fn default_log_file() -> PathBuf { cache_dir().join("helix.log") } +pub fn log_file() -> PathBuf { + LOG_FILE + .get() + .map(|path| path.to_path_buf()) + .unwrap_or_else(default_log_file) +} + /// Merge two TOML documents, merging values from `right` onto `left` /// /// When an array exists in both `left` and `right`, `right`'s array is diff --git a/helix-term/src/main.rs b/helix-term/src/main.rs index 73513bf392ce..53d8829cd7d5 100644 --- a/helix-term/src/main.rs +++ b/helix-term/src/main.rs @@ -4,9 +4,8 @@ use helix_loader::VERSION_AND_GIT_HASH; use helix_term::application::Application; use helix_term::args::Args; use helix_term::config::{Config, ConfigLoadError}; -use std::path::PathBuf; -fn setup_logging(logpath: PathBuf, verbosity: u64) -> Result<()> { +fn setup_logging(verbosity: u64) -> Result<()> { let mut base_config = fern::Dispatch::new(); base_config = match verbosity { @@ -27,7 +26,7 @@ fn setup_logging(logpath: PathBuf, verbosity: u64) -> Result<()> { message )) }) - .chain(fern::log_file(logpath)?); + .chain(fern::log_file(helix_loader::log_file())?); base_config.chain(file_config).apply()?; @@ -41,12 +40,6 @@ fn main() -> Result<()> { #[tokio::main] async fn main_impl() -> Result { - let logpath = helix_loader::log_file(); - let parent = logpath.parent().unwrap(); - if !parent.exists() { - std::fs::create_dir_all(parent).ok(); - } - let help = format!( "\ {} {} @@ -78,7 +71,7 @@ FLAGS: VERSION_AND_GIT_HASH, env!("CARGO_PKG_AUTHORS"), env!("CARGO_PKG_DESCRIPTION"), - logpath.display(), + helix_loader::default_log_file().display(), ); let args = Args::parse_args().context("could not parse arguments")?; @@ -116,15 +109,11 @@ FLAGS: return Ok(0); } - let logpath = args.log_file.as_ref().cloned().unwrap_or(logpath); - setup_logging(logpath, args.verbosity).context("failed to initialize logging")?; - - let config_dir = helix_loader::config_dir(); - if !config_dir.exists() { - std::fs::create_dir_all(&config_dir).ok(); - } - + helix_loader::ensure_config_dir(); helix_loader::initialize_config_file(args.config_file.clone()); + helix_loader::initialize_log_file(args.log_file.clone()); + + setup_logging(args.verbosity).context("failed to initialize logging")?; let config = match Config::load_default() { Ok(config) => config, From c6c06442b4c59c364b2011588bdd7bf3244ed3ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20Vinyals=20Valdepe=C3=B1as?= Date: Sat, 8 Jul 2023 23:23:50 +0200 Subject: [PATCH 2/2] Ensure parent dirs when initialising log and config files Also, updated the getters to `unwrap()` as it seems silly to default again in case the cell is not initialised. If the cell is not initialised it would be a big programming error, as the first thing we do when starting the editor is precisely that. --- helix-loader/src/lib.rs | 39 ++++++++++++++++++--------------------- helix-term/src/main.rs | 1 - 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/helix-loader/src/lib.rs b/helix-loader/src/lib.rs index c11a97cab37c..c51c9c7de2cf 100644 --- a/helix-loader/src/lib.rs +++ b/helix-loader/src/lib.rs @@ -15,21 +15,16 @@ static LOG_FILE: once_cell::sync::OnceCell = once_cell::sync::OnceCell: pub fn initialize_config_file(specified_file: Option) { let config_file = specified_file.unwrap_or_else(default_config_file); + ensure_parent_dir(&config_file); CONFIG_FILE.set(config_file).ok(); } pub fn initialize_log_file(specified_file: Option) { let log_file = specified_file.unwrap_or_else(default_log_file); + ensure_parent_dir(&log_file); LOG_FILE.set(log_file).ok(); } -pub fn ensure_config_dir() { - let config_dir = config_dir(); - if !config_dir.exists() { - std::fs::create_dir_all(&config_dir).ok(); - } -} - /// A list of runtime directories from highest to lowest priority /// /// The priority is: @@ -125,15 +120,12 @@ pub fn cache_dir() -> PathBuf { path } -pub fn default_config_file() -> PathBuf { - config_dir().join("config.toml") +pub fn config_file() -> PathBuf { + CONFIG_FILE.get().map(|path| path.to_path_buf()).unwrap() } -pub fn config_file() -> PathBuf { - CONFIG_FILE - .get() - .map(|path| path.to_path_buf()) - .unwrap_or_else(default_config_file) +pub fn log_file() -> PathBuf { + LOG_FILE.get().map(|path| path.to_path_buf()).unwrap() } pub fn workspace_config_file() -> PathBuf { @@ -148,13 +140,6 @@ pub fn default_log_file() -> PathBuf { cache_dir().join("helix.log") } -pub fn log_file() -> PathBuf { - LOG_FILE - .get() - .map(|path| path.to_path_buf()) - .unwrap_or_else(default_log_file) -} - /// Merge two TOML documents, merging values from `right` onto `left` /// /// When an array exists in both `left` and `right`, `right`'s array is @@ -242,6 +227,18 @@ pub fn find_workspace() -> (PathBuf, bool) { (current_dir, true) } +fn default_config_file() -> PathBuf { + config_dir().join("config.toml") +} + +fn ensure_parent_dir(path: &Path) { + if let Some(parent) = path.parent() { + if !parent.exists() { + std::fs::create_dir_all(parent).ok(); + } + } +} + #[cfg(test)] mod merge_toml_tests { use std::str; diff --git a/helix-term/src/main.rs b/helix-term/src/main.rs index 53d8829cd7d5..97e962e0d149 100644 --- a/helix-term/src/main.rs +++ b/helix-term/src/main.rs @@ -109,7 +109,6 @@ FLAGS: return Ok(0); } - helix_loader::ensure_config_dir(); helix_loader::initialize_config_file(args.config_file.clone()); helix_loader::initialize_log_file(args.log_file.clone());