Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix :log-open when --log is specified #7573

Merged
merged 2 commits into from
Jul 9, 2023
Merged

Fix :log-open when --log is specified #7573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90978b6
fix & boyscout: ensure loader caches correct log file
alevinval Jul 8, 2023
c6c0644
Ensure parent dirs when initialising log and config files
alevinval Jul 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 28 additions & 16 deletions helix-loader/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,20 @@ static RUNTIME_DIRS: once_cell::sync::Lazy<Vec<PathBuf>> =

static CONFIG_FILE: once_cell::sync::OnceCell<PathBuf> = once_cell::sync::OnceCell::new();

pub fn initialize_config_file(specified_file: Option<PathBuf>) {
let config_file = specified_file.unwrap_or_else(|| {
let config_dir = config_dir();

if !config_dir.exists() {
std::fs::create_dir_all(&config_dir).ok();
}
static LOG_FILE: once_cell::sync::OnceCell<PathBuf> = once_cell::sync::OnceCell::new();

config_dir.join("config.toml")
});

// We should only initialize this value once.
pub fn initialize_config_file(specified_file: Option<PathBuf>) {
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<PathBuf>) {
let log_file = specified_file.unwrap_or_else(default_log_file);
ensure_parent_dir(&log_file);
LOG_FILE.set(log_file).ok();
}

/// A list of runtime directories from highest to lowest priority
///
/// The priority is:
Expand Down Expand Up @@ -122,10 +121,11 @@ pub fn cache_dir() -> PathBuf {
}

pub fn config_file() -> PathBuf {
CONFIG_FILE
.get()
.map(|path| path.to_path_buf())
.unwrap_or_else(|| config_dir().join("config.toml"))
CONFIG_FILE.get().map(|path| path.to_path_buf()).unwrap()
}

pub fn log_file() -> PathBuf {
LOG_FILE.get().map(|path| path.to_path_buf()).unwrap()
Comment on lines 123 to +128
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to write the following here (and the equivalent for the config_file())

unwrap_or_else(|| {
   initialize_log_file(None)
   log_file()
})

But then I really think we should unwrap() and potentially fail. A failure here means we have seriously broken the initialization steps in the main.rs, it seems less misleading than re-replicating the initialization of the cell in two places.

}

pub fn workspace_config_file() -> PathBuf {
Expand All @@ -136,7 +136,7 @@ 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")
}

Expand Down Expand Up @@ -227,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;
Expand Down
24 changes: 6 additions & 18 deletions helix-term/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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()?;

Expand All @@ -41,12 +40,6 @@ fn main() -> Result<()> {

#[tokio::main]
async fn main_impl() -> Result<i32> {
let logpath = helix_loader::log_file();
let parent = logpath.parent().unwrap();
if !parent.exists() {
std::fs::create_dir_all(parent).ok();
}

let help = format!(
"\
{} {}
Expand Down Expand Up @@ -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")?;
Expand Down Expand Up @@ -116,15 +109,10 @@ 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::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,
Expand Down