Merge branch 'config-sources-api' into 'main'

Tidy up ConfigurationSource a bit

See merge request tpo/core/arti!495

Cargo.lock

@@ -166,6 +166,7 @@ dependencies = [
  "arti-client",
  "config",
  "derive_builder_fork_arti",
+ "fs-mistrust",
  "once_cell",
  "regex",
  "serde",

crates/arti-bench/src/main.rs

@@ -354,7 +354,7 @@ fn main() -> Result<()> {
         )
         .get_matches();
     info!("Parsing Arti configuration...");
-    let mut config_sources = arti_config::ConfigurationSources::new();
+    let mut config_sources = arti_config::ConfigurationSources::new_empty();
     matches
         .values_of_os("arti-config")
         .unwrap_or_default()

crates/arti-client/src/config.rs

@@ -18,7 +18,7 @@ use std::collections::HashMap;
 use std::path::Path;
 use std::path::PathBuf;
 use std::time::Duration;
-pub use tor_config::{CfgPath, ConfigBuildError, Reconfigure};
+pub use tor_config::{CfgPath, CfgPathError, ConfigBuildError, Reconfigure};
 
 /// Types for configuring how Tor circuits are built.
 pub mod circ {
@@ -370,6 +370,11 @@ pub(crate) fn default_fs_mistrust() -> fs_mistrust::Mistrust {
     mistrust
 }
 
+/// Return a filename for the default user configuration file.
+pub fn default_config_file() -> Result<PathBuf, CfgPathError> {
+    CfgPath::new("${ARTI_CONFIG}/arti.toml".into()).path()
+}
+
 #[cfg(test)]
 mod test {
     #![allow(clippy::unwrap_used)]

crates/arti-config/Cargo.toml

@@ -17,6 +17,7 @@ tor-circmgr = { package = "tor-circmgr", path = "../tor-circmgr", version = "0.3
 tor-config = { package = "tor-config", path = "../tor-config", version = "0.3.0", features = [
     "expand-paths",
 ] }
+fs-mistrust = { path = "../fs-mistrust", version = "0.1.0" }
 config = { version = "0.13", default-features = false, features = ["toml"] }
 once_cell = "1"
 serde = { version = "1.0.103", features = ["derive"] }

crates/arti-config/src/lib.rs

@@ -52,7 +52,6 @@ mod options;
 
 pub use cmdline::CmdLine;
 pub use options::ARTI_DEFAULTS;
-use tor_config::CfgPath;
 
 /// The synchronous configuration builder type we use.
 ///
@@ -86,25 +85,71 @@ enum MustRead {
 
 impl ConfigurationSources {
     /// Create a new empty [`ConfigurationSources`].
-    pub fn new() -> Self {
+    pub fn new_empty() -> Self {
         Self::default()
     }
 
+    /// Establish a [`ConfigurationSources`] the usual way from a command line and defaults
+    ///
+    /// The caller should have parsed the program's command line, and extracted (inter alia)
+    ///
+    ///  * `config_files_options`: Paths of config file(s)
+    ///  * `cmdline_toml_override_options`: Overrides ("key=value")
+    ///
+    /// The caller should also provide `default_config_file`, the default location of the
+    /// configuration file.  This is used if no file(s) are specified on the command line.
+    ///
+    /// `mistrust` is used to check whether the configuration files have appropriate permissions.
+    pub fn from_cmdline<F, O>(
+        default_config_file: impl Into<PathBuf>,
+        config_files_options: impl IntoIterator<Item = F>,
+        cmdline_toml_override_options: impl IntoIterator<Item = O>,
+        mistrust: &fs_mistrust::Mistrust,
+    ) -> Result<Self, fs_mistrust::Error>
+    where
+        F: Into<PathBuf>,
+        O: Into<String>,
+    {
+        let mut cfg_sources = ConfigurationSources::new_empty();
+
+        let mut any_files = false;
+        for f in config_files_options {
+            let f = f.into();
+            mistrust.verifier().require_file().check(&f)?;
+            cfg_sources.push_file(f);
+            any_files = true;
+        }
+        if !any_files {
+            let default = default_config_file.into();
+            match mistrust.verifier().require_file().check(&default) {
+                Ok(()) => {}
+                Err(fs_mistrust::Error::NotFound(_)) => {}
+                Err(e) => return Err(e),
+            }
+            cfg_sources.push_optional_file(default);
+        }
+
+        for s in cmdline_toml_override_options {
+            cfg_sources.push_option(s);
+        }
+
+        Ok(cfg_sources)
+    }
+
     /// Add `p` to the list of files that we want to read configuration from.
     ///
     /// Configuration files are loaded and applied in the order that they are
     /// added to this object.
     ///
     /// If the listed file is absent, loading the configuration won't succeed.
-    pub fn push_file<P: AsRef<Path>>(&mut self, p: P) {
-        self.files.push((p.as_ref().to_owned(), MustRead::MustRead));
+    pub fn push_file(&mut self, p: impl Into<PathBuf>) {
+        self.files.push((p.into(), MustRead::MustRead));
     }
 
     /// As `push_file`, but if the listed file can't be loaded, loading the
     /// configuration can still succeed.
-    pub fn push_optional_file<P: AsRef<Path>>(&mut self, p: P) {
-        self.files
-            .push((p.as_ref().to_owned(), MustRead::TolerateAbsence));
+    pub fn push_optional_file(&mut self, p: impl Into<PathBuf>) {
+        self.files.push((p.into(), MustRead::TolerateAbsence));
     }
 
     /// Add `s` to the list of overridden options to apply to our configuration.
@@ -113,8 +158,8 @@ impl ConfigurationSources {
     /// order that they are added to this object.
     ///
     /// The format for `s` is as in [`CmdLine`].
-    pub fn push_option<S: ToOwned<Owned = String> + ?Sized>(&mut self, option: &S) {
-        self.options.push(option.to_owned());
+    pub fn push_option(&mut self, option: impl Into<String>) {
+        self.options.push(option.into());
     }
 
     /// Return an iterator over the files that we care about.
@@ -161,15 +206,11 @@ where
     builder
 }
 
-/// Return a filename for the default user configuration file.
-pub fn default_config_file() -> Option<PathBuf> {
-    CfgPath::new("${ARTI_CONFIG}/arti.toml".into()).path().ok()
-}
-
 #[cfg(test)]
 mod test {
     #![allow(clippy::unwrap_used)]
     use super::*;
+    use arti_client::config::default_config_file;
     use tempfile::tempdir;
 
     static EX_TOML: &str = "

crates/arti-testing/src/config.rs

@@ -122,7 +122,7 @@ pub(crate) fn parse_cmdline() -> Result<Job> {
 
     let config = {
         // TODO: this is mostly duplicate code.
-        let mut cfg_sources = arti_config::ConfigurationSources::new();
+        let mut cfg_sources = arti_config::ConfigurationSources::new_empty();
 
         let config_files = matches.values_of_os("config-files").unwrap_or_default();
 

crates/arti/src/lib.rs

@@ -128,8 +128,8 @@ pub use cfg::{
 };
 pub use logging::{LoggingConfig, LoggingConfigBuilder};
 
+use arti_client::config::default_config_file;
 use arti_client::{TorClient, TorClientConfig};
-use arti_config::default_config_file;
 use safelog::with_safe_logging_suppressed;
 use tor_rtcompat::{BlockOn, Runtime};
 
@@ -263,7 +263,10 @@ pub fn main_main() -> Result<()> {
     // correct behavior is different depending on whether the filename is given
     // explicitly or not.
     let mut config_file_help = "Specify which config file(s) to read.".to_string();
-    if let Some(default) = arti_config::default_config_file() {
+    if let Ok(default) = default_config_file() {
+        // If we couldn't resolve the default config file, then too bad.  If something
+        // actually tries to use it, it will produce an error, but don't fail here
+        // just for that reason.
         config_file_help.push_str(&format!(" Defaults to {:?}", default));
     }
 
@@ -368,33 +371,12 @@ pub fn main_main() -> Result<()> {
         mistrust
     };
 
-    let cfg_sources = {
-        let mut cfg_sources = arti_config::ConfigurationSources::new();
-
-        let config_files = matches.values_of_os("config-files").unwrap_or_default();
-        if config_files.len() == 0 {
-            if let Some(default) = default_config_file() {
-                match mistrust.verifier().require_file().check(&default) {
-                    Ok(()) => {}
-                    Err(fs_mistrust::Error::NotFound(_)) => {}
-                    Err(e) => return Err(e.into()),
-                }
-                cfg_sources.push_optional_file(default);
-            }
-        } else {
-            for f in config_files {
-                mistrust.verifier().require_file().check(f)?;
-                cfg_sources.push_file(f);
-            }
-        }
-
-        matches
-            .values_of("option")
-            .unwrap_or_default()
-            .for_each(|s| cfg_sources.push_option(s));
-
-        cfg_sources
-    };
+    let cfg_sources = arti_config::ConfigurationSources::from_cmdline(
+        default_config_file()?,
+        matches.values_of_os("config-files").unwrap_or_default(),
+        matches.values_of("option").unwrap_or_default(),
+        &mistrust,
+    )?;
 
     let cfg = cfg_sources.load()?;
 

crates/tor-config/src/lib.rs

@@ -55,7 +55,7 @@ pub use educe;
 pub use err::{ConfigBuildError, ReconfigureError};
 pub use mut_cfg::MutCfg;
 pub use paste::paste;
-pub use path::CfgPath;
+pub use path::{CfgPath, CfgPathError};
 pub use serde;
 
 pub use tor_basic_utils::macro_first_nonempty;

doc/semver_status.md

@@ -43,3 +43,8 @@ BREAKING: Routerstatus::nickname() now returns &str, not &String.
 
 BREAKING: Runtime now requires the Debug trait to be implemented.
 
+### arti-config
+
+BREAKING: default_config_file moved to arti_client, and changed to return Result
+GREAKING: ConfigurationSource::new_empty renamed from ::new
+BREAKING: ConfigurationSource methods take Into<String> and Into<PathBuf> now