diff --git a/build.rs b/build.rs index 1b4f18920..0ddc3c45e 100644 --- a/build.rs +++ b/build.rs @@ -122,6 +122,15 @@ const fn verify_public_api_symbols() { const _: fn(&cli::Cli, &ArgMatches) -> bool = cli::resolve_merged_diag_json; const _: fn(&cli::Cli, &ArgMatches) -> ortho_config::OrthoResult = cli::merge_with_config; + const _: usize = std::mem::size_of::(); + const _: fn(&cli::Cli) -> cli::ConfigFileLayers = cli::ConfigFileLayers::load; + const _: fn(&cli::Cli, &ArgMatches, &cli::ConfigFileLayers) -> bool = + cli::resolve_merged_diag_json_with_layers; + const _: fn( + &cli::Cli, + &ArgMatches, + &cli::ConfigFileLayers, + ) -> ortho_config::OrthoResult = cli::merge_with_config_layers; const _: LocalizedParseFn = cli::parse_with_localizer_from; const _: fn(&cli::Cli) -> Option = cli::Cli::no_emoji_override; const _: fn(&cli::Cli) -> bool = cli::Cli::progress_enabled; diff --git a/src/cli/diag.rs b/src/cli/diag.rs index 204317d1f..fe9d59c2c 100644 --- a/src/cli/diag.rs +++ b/src/cli/diag.rs @@ -7,12 +7,11 @@ use clap::ArgMatches; use clap::parser::ValueSource; -use ortho_config::OrthoResult; use ortho_config::figment::Figment; use ortho_config::uncased::Uncased; use serde_json::Value; -use super::discovery::collect_diag_file_layers; +use super::discovery::ConfigFileLayers; use super::merge::env_provider; use super::parser::Cli; @@ -23,8 +22,22 @@ use super::parser::Cli; /// environment. #[must_use] pub fn resolve_merged_diag_json(cli: &Cli, matches: &ArgMatches) -> bool { - let mut diag_json = - diag_json_from_file_layers(cli).unwrap_or_else(|_| Cli::default().diag_json); + resolve_merged_diag_json_with_layers(cli, matches, &ConfigFileLayers::load(cli)) +} + +/// Resolve the diagnostic JSON preference using pre-loaded file layers. +/// +/// Callers that also run the full configuration merge should load the file +/// layers once via [`ConfigFileLayers::load`] and share them with +/// [`super::merge_with_config_layers`] so config files are read from disk +/// only once per invocation. +#[must_use] +pub fn resolve_merged_diag_json_with_layers( + cli: &Cli, + matches: &ArgMatches, + file_layers: &ConfigFileLayers, +) -> bool { + let mut diag_json = diag_json_from_file_layers(file_layers); diag_json = diag_json_from_env(diag_json); diag_json_from_matches(cli, matches, diag_json) } @@ -46,16 +59,18 @@ fn diag_json_from_matches(cli: &Cli, matches: &ArgMatches, discovered: bool) -> } } -fn diag_json_from_file_layers(cli: &Cli) -> OrthoResult { +fn diag_json_from_file_layers(file_layers: &ConfigFileLayers) -> bool { let default = Cli::default().diag_json; - let layers = collect_diag_file_layers(cli)?; + let Ok(layers) = file_layers.as_result() else { + return default; + }; let mut diag_json = default; for layer in layers { - if let Some(layer_diag_json) = diag_json_from_layer(&layer.into_value()) { + if let Some(layer_diag_json) = diag_json_from_layer(&layer.clone().into_value()) { diag_json = layer_diag_json; } } - Ok(diag_json) + diag_json } fn diag_json_from_env(fallback: bool) -> bool { diff --git a/src/cli/discovery.rs b/src/cli/discovery.rs index 1dd4002e0..0db585a3d 100644 --- a/src/cli/discovery.rs +++ b/src/cli/discovery.rs @@ -17,32 +17,63 @@ use super::parser::Cli; const CONFIG_ENV_VAR: &str = "NETSUKE_CONFIG"; const CONFIG_ENV_VAR_LEGACY: &str = "NETSUKE_CONFIG_PATH"; +/// Configuration file layers discovered and loaded once per invocation. +/// +/// Both the diagnostic-mode pre-pass (`resolve_merged_diag_json_with_layers`) +/// and the full merge (`merge_with_config_layers`) consume the same file +/// layers. Loading them once and +/// sharing the result avoids opening, reading, and deserialising every +/// config file twice on startup. +#[derive(Debug, Clone)] +pub struct ConfigFileLayers(OrthoResult>>); + +impl ConfigFileLayers { + /// Discover and load the configuration file layers for `cli`. + /// + /// Honours the explicit `--config` flag and the `NETSUKE_CONFIG` / + /// `NETSUKE_CONFIG_PATH` environment variables before falling back to + /// scope discovery. + #[must_use] + pub fn load(cli: &Cli) -> Self { + let explicit_path = explicit_config_path(cli); + if let Some(path) = &explicit_path { + tracing::debug!(layer = "file", path = %path.display(), "loading explicit configuration file"); + } else { + tracing::debug!(layer = "file", "discovering configuration files"); + } + Self(explicit_path.map_or_else( + || collect_file_layers(cli.directory.as_deref()), + |path| load_layers_from_path(&path), + )) + } + + /// Borrow the discovery outcome. + pub(crate) fn as_result( + &self, + ) -> Result<&[MergeLayer<'static>], &Arc> { + match &self.0 { + Ok(layers) => Ok(layers), + Err(err) => Err(err), + } + } +} + pub(crate) fn push_file_layers( - cli: &Cli, + file_layers: &ConfigFileLayers, composer: &mut MergeComposer, errors: &mut Vec>, ) { - let explicit_path = explicit_config_path(cli); - if let Some(path) = &explicit_path { - tracing::debug!(layer = "file", path = %path.display(), "loading explicit configuration file"); - } else { - tracing::debug!(layer = "file", "discovering configuration files"); - } - let layers_result = explicit_path.map_or_else( - || collect_file_layers(cli.directory.as_deref()), - |path| load_layers_from_path(&path), - ); - match layers_result { + match file_layers.as_result() { Ok(layers) => push_discovered_layers(composer, layers), Err(err) => { tracing::debug!(layer = "file", error = %err, "configuration file discovery failed"); - errors.push(err); + errors.push(Arc::clone(err)); } } } /// Push discovered file layers onto the composer, logging each path. -fn push_discovered_layers(composer: &mut MergeComposer, layers: Vec>) { +fn push_discovered_layers(composer: &mut MergeComposer, layers: &[MergeLayer<'static>]) { if layers.is_empty() { tracing::debug!(layer = "file", "no configuration file layers found"); } @@ -52,7 +83,7 @@ fn push_discovered_layers(composer: &mut MergeComposer, layers: Vec Err(err), } } - -pub(crate) fn collect_diag_file_layers(cli: &Cli) -> OrthoResult>> { - explicit_config_path(cli).map_or_else( - || collect_file_layers(cli.directory.as_deref()), - |path| load_layers_from_path(&path), - ) -} diff --git a/src/cli/merge.rs b/src/cli/merge.rs index fbe00011a..5b8932f4f 100644 --- a/src/cli/merge.rs +++ b/src/cli/merge.rs @@ -31,7 +31,7 @@ use serde::Serialize; use serde_json::{Map, Value, json}; use super::config::{BuildConfig, CliConfig, Theme}; -use super::discovery::push_file_layers; +use super::discovery::{ConfigFileLayers, push_file_layers}; use super::parser::{BuildArgs, Cli, Commands}; use super::validation_error; use crate::theme::ThemePreference; @@ -48,11 +48,30 @@ type MergeError = std::sync::Arc; /// Returns an [`ortho_config::OrthoError`] if layer composition or merging /// fails. pub fn merge_with_config(cli: &Cli, matches: &ArgMatches) -> OrthoResult { + merge_with_config_layers(cli, matches, &ConfigFileLayers::load(cli)) +} + +/// Merge configuration layers over parsed CLI input, reusing pre-loaded +/// file layers. +/// +/// Callers that also run the diagnostic pre-pass should load the file layers +/// once via [`ConfigFileLayers::load`] and pass them to both functions so +/// config files are read from disk only once per invocation. +/// +/// # Errors +/// +/// Returns an [`ortho_config::OrthoError`] if layer composition or merging +/// fails. +pub fn merge_with_config_layers( + cli: &Cli, + matches: &ArgMatches, + file_layers: &ConfigFileLayers, +) -> OrthoResult { let mut errors = Vec::new(); let mut composer = MergeComposer::with_capacity(4); push_defaults_layer(&mut composer, &mut errors); - push_file_layers(cli, &mut composer, &mut errors); + push_file_layers(file_layers, &mut composer, &mut errors); push_environment_layer(&mut composer, &mut errors); push_cli_layer(cli, matches, &mut composer, &mut errors); diff --git a/src/cli/mod.rs b/src/cli/mod.rs index c7d2d3b5d..9b00875bf 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -16,8 +16,9 @@ mod parser; mod parsing; pub use config::{CliConfig, ColourPolicy, OutputFormat, SpinnerMode, Theme}; -pub use diag::resolve_merged_diag_json; -pub use merge::merge_with_config; +pub use diag::{resolve_merged_diag_json, resolve_merged_diag_json_with_layers}; +pub use discovery::ConfigFileLayers; +pub use merge::{merge_with_config, merge_with_config_layers}; pub use parser::{ BuildArgs, Cli, Commands, GraphArgs, diag_json_hint_from_args, locale_hint_from_args, parse_with_localizer_from, diff --git a/src/main.rs b/src/main.rs index 0c2a53958..ddbc27fd8 100644 --- a/src/main.rs +++ b/src/main.rs @@ -53,7 +53,14 @@ fn run_with_args( Ok(parsed) => parsed, Err(code) => return code, }; - let mode = DiagMode::from_json_enabled(cli::resolve_merged_diag_json(&parsed_cli, &matches)); + // Load config file layers once; the diagnostic pre-pass and the full + // merge share the result, avoiding duplicate filesystem I/O on startup. + let config_layers = cli::ConfigFileLayers::load(&parsed_cli); + let mode = DiagMode::from_json_enabled(cli::resolve_merged_diag_json_with_layers( + &parsed_cli, + &matches, + &config_layers, + )); if !mode.is_json() && parsed_cli.verbose { // Initialise tracing before the merge so the configuration @@ -64,7 +71,7 @@ fn run_with_args( init_tracing(Level::DEBUG); } - let merged_cli = match merge_cli_or_exit(&parsed_cli, &matches, mode) { + let merged_cli = match merge_cli_or_exit(&parsed_cli, &matches, mode, &config_layers) { Ok(merged) => merged, Err(code) => return code, }; @@ -133,8 +140,9 @@ fn merge_cli_or_exit( parsed_cli: &cli::Cli, matches: &ArgMatches, mode: DiagMode, + config_layers: &cli::ConfigFileLayers, ) -> Result { - match cli::merge_with_config(parsed_cli, matches) { + match cli::merge_with_config_layers(parsed_cli, matches, config_layers) { Ok(merged) => Ok(merged.with_default_command()), Err(err) => { if mode.is_json() { diff --git a/tests/cli_tests/config_discovery.rs b/tests/cli_tests/config_discovery.rs index 887a35548..f80b1c3da 100644 --- a/tests/cli_tests/config_discovery.rs +++ b/tests/cli_tests/config_discovery.rs @@ -637,3 +637,35 @@ fetch_allow_scheme = ["https"] drop(cwd_guard); result } + +#[rstest] +fn config_layers_are_loaded_once_and_shared() -> Result<()> { + // Deleting the config file after `ConfigFileLayers::load` proves both + // consumers reuse the cached layers instead of re-reading the disk. + let _env_lock = test_support::env_lock::EnvLock::acquire(); + let temp_dir = tempfile::tempdir().context("create temporary config directory")?; + let config_path = temp_dir.path().join("netsuke.toml"); + std::fs::write(&config_path, "diag_json = true\njobs = 7\n").context("write netsuke.toml")?; + let _config_guard = + test_support::EnvVarGuard::set("NETSUKE_CONFIG_PATH", config_path.as_os_str()); + + let localizer = std::sync::Arc::from(netsuke::cli_localization::build_localizer(None)); + let (cli, matches) = netsuke::cli::parse_with_localizer_from(["netsuke"], &localizer) + .context("parse CLI args")?; + let layers = netsuke::cli::ConfigFileLayers::load(&cli); + + std::fs::remove_file(&config_path).context("delete config file after load")?; + + ensure!( + netsuke::cli::resolve_merged_diag_json_with_layers(&cli, &matches, &layers), + "diag pre-pass should honour the cached file layer after deletion" + ); + let merged = netsuke::cli::merge_with_config_layers(&cli, &matches, &layers) + .context("merge with cached layers")?; + ensure!( + merged.jobs == Some(7), + "merge should honour the cached file layer after deletion, got {:?}", + merged.jobs + ); + Ok(()) +}