diff --git a/src/trx-client/src/main.rs b/src/trx-client/src/main.rs index c2266af1..d2f67717 100644 --- a/src/trx-client/src/main.rs +++ b/src/trx-client/src/main.rs @@ -20,6 +20,7 @@ use tokio::task::JoinHandle; use tracing::{error, info}; use trx_app::{init_logging, normalize_name}; +use trx_config::ConfigFile; use trx_core::audio::AudioStreamInfo; use trx_core::decode::DecodedMessage; @@ -80,7 +81,7 @@ struct Cli { /// rigctl frontend listen address #[arg(long = "rigctl-listen")] rigctl_listen: Option, - /// rigctl frontend listen port + /// Deprecated: ignored, use [frontends.rigctl].rig_ports #[arg(long = "rigctl-port")] rigctl_port: Option, /// JSON TCP frontend listen address @@ -127,6 +128,7 @@ fn check_config(loaded: &trx_config::ConfigLoad) -> DynResult<()> } let mut warnings: Vec = loaded.unknown_keys.iter().map(|k| k.to_string()).collect(); + warnings.extend(ClientConfig::deprecations(&loaded.present_keys)); let mut cfg = loaded.config.clone(); let mut errors = Vec::new(); @@ -223,6 +225,13 @@ async fn async_init() -> DynResult { info!("Loaded configuration from {}", path.display()); } loaded.report_unknown_keys(cli.strict_config)?; + loaded.report_deprecations(); + if cli.rigctl_port.is_some() { + tracing::warn!( + "--rigctl-port is ignored; give each rig its own listener via \ + [frontends.rigctl].rig_ports" + ); + } let mut cfg = loaded.config; // Secrets configured as *_file are read before validation, so everything diff --git a/src/trx-config/src/client.rs b/src/trx-config/src/client.rs index 7d2f32de..d7a34e5c 100644 --- a/src/trx-config/src/client.rs +++ b/src/trx-config/src/client.rs @@ -935,6 +935,51 @@ impl ConfigFile for ClientConfig { "trx-client" } + /// Keys that no longer do what they look like they do. + fn deprecations(present: &std::collections::BTreeSet) -> Vec { + let mut warnings = Vec::new(); + + if present.contains("remote") { + if present.contains("remotes") { + warnings.push( + "[remote] is ignored because [[remotes]] is set; delete it".to_string(), + ); + } else { + warnings.push( + "[remote] is superseded by [[remotes]], which supports several rigs; \ + it still works but will be removed in a future release" + .to_string(), + ); + } + } + + if present.contains("frontends.rigctl.port") { + warnings.push( + "[frontends.rigctl].port is ignored; give each rig its own listener via \ + [frontends.rigctl].rig_ports" + .to_string(), + ); + } + + if present.contains("frontends.audio.rig_ports") { + warnings.push( + "[frontends.audio].rig_ports is superseded by [frontends.audio].rig_urls, \ + which can also point at a different host" + .to_string(), + ); + } + + if present.contains("frontends.http.default_rig_id") { + warnings.push( + "[frontends.http].default_rig_id has been renamed to default_rig_name; \ + the old name still works but will be removed in a future release" + .to_string(), + ); + } + + warnings + } + /// Include one `[[remotes]]` entry so unknown keys nested inside a remote /// entry still get a suggestion. fn reference_value() -> toml::Value { @@ -955,6 +1000,7 @@ impl ConfigFile for ClientConfig { #[cfg(test)] mod tests { use super::*; + use crate::ConfigFile; #[test] fn test_default_config() { @@ -1404,6 +1450,51 @@ url = "remote.example.com:4530" .contains("poll_interval_ms must be > 0")); } + // --- Deprecation warnings --- + + fn present(paths: &[&str]) -> std::collections::BTreeSet { + paths.iter().map(|p| p.to_string()).collect() + } + + #[test] + fn test_deprecation_flags_shadowed_remote_section() { + let warnings = ClientConfig::deprecations(&present(&["remote", "remotes"])); + assert!( + warnings.iter().any(|w| w.contains("[remote] is ignored")), + "unexpected warnings: {warnings:?}" + ); + } + + #[test] + fn test_deprecation_nudges_lone_remote_section() { + let warnings = ClientConfig::deprecations(&present(&["remote"])); + assert_eq!(warnings.len(), 1); + assert!(warnings[0].contains("superseded by [[remotes]]")); + } + + #[test] + fn test_deprecation_flags_ignored_rigctl_port() { + let warnings = ClientConfig::deprecations(&present(&["frontends.rigctl.port"])); + assert!(warnings.iter().any(|w| w.contains("rig_ports"))); + } + + #[test] + fn test_deprecation_flags_renamed_default_rig_id() { + let warnings = ClientConfig::deprecations(&present(&["frontends.http.default_rig_id"])); + assert!(warnings.iter().any(|w| w.contains("default_rig_name"))); + } + + #[test] + fn test_no_deprecations_for_a_current_config() { + let warnings = ClientConfig::deprecations(&present(&[ + "remotes", + "frontends.http.port", + "frontends.rigctl.rig_ports", + "frontends.audio.rig_urls", + ])); + assert!(warnings.is_empty(), "unexpected warnings: {warnings:?}"); + } + // --- Secret indirection --- fn secret_file(content: &str) -> tempfile::NamedTempFile { diff --git a/src/trx-config/src/file.rs b/src/trx-config/src/file.rs index 7e1211a9..9b119326 100644 --- a/src/trx-config/src/file.rs +++ b/src/trx-config/src/file.rs @@ -22,7 +22,9 @@ use serde::Serialize; use std::path::{Path, PathBuf}; use thiserror::Error; -use crate::unknown::{describe, UnknownKey}; +use std::collections::BTreeSet; + +use crate::unknown::{describe, flatten_paths, UnknownKey}; /// Every section key that may appear at the root of a combined config file. pub const SECTION_KEYS: &[&str] = &["trx-server", "trx-client"]; @@ -37,6 +39,18 @@ pub struct ConfigLoad { pub path: Option, /// Keys present in the file that no config field claimed. pub unknown_keys: Vec, + /// Every key path the file actually set. Defaults are indistinguishable + /// from explicit values once deserialized, so deprecation checks need this. + pub present_keys: BTreeSet, +} + +impl ConfigLoad { + /// Log a warning for every deprecated key the file sets. + pub fn report_deprecations(&self) { + for message in T::deprecations(&self.present_keys) { + tracing::warn!("{}", message); + } + } } impl ConfigLoad { @@ -97,10 +111,12 @@ fn select_section(table: &toml::Table, key: &str) -> Option { /// Returns `Ok(Some((cfg, unknown_keys)))` when the section is present and /// parses cleanly, `Ok(None)` when the section is absent, or `Err` on I/O / /// parse failure. +type LoadedSection = (T, Vec, BTreeSet); + fn load_section_from_file( path: &Path, key: &str, -) -> Result)>, ConfigError> { +) -> Result>, ConfigError> { let content = std::fs::read_to_string(path) .map_err(|e| ConfigError::ReadError(path.to_path_buf(), e.to_string()))?; @@ -116,13 +132,19 @@ fn load_section_from_file( crate::secrets::expand_env_vars(&mut section) .map_err(|e| ConfigError::ParseError(path.to_path_buf(), e))?; + let present_keys = flatten_paths(§ion); + // Deserialize straight from the TOML value so serde applies every default, // recording any key no field claimed. let mut ignored: Vec = Vec::new(); let cfg: T = serde_ignored::deserialize(section, |path| ignored.push(path.to_string())) .map_err(|e| ConfigError::ParseError(path.to_path_buf(), e.to_string()))?; - Ok(Some((cfg, describe(&ignored, &T::reference_value())))) + Ok(Some(( + cfg, + describe(&ignored, &T::reference_value()), + present_keys, + ))) } /// Trait for loading configuration from a `trx-rs.toml` section. @@ -130,6 +152,12 @@ pub trait ConfigFile: Sized + Default + DeserializeOwned + Serialize { /// Section key in `trx-rs.toml` (e.g. `"trx-server"` or `"trx-client"`). fn section_key() -> &'static str; + /// Warnings for deprecated keys the file sets, given every key path present + /// in it. Defaults to none. + fn deprecations(_present_keys: &BTreeSet) -> Vec { + Vec::new() + } + /// A TOML rendering of a populated config, used to suggest corrections for /// unknown keys. Implementations should fill in list-valued sections such /// as `[[rigs]]` so keys nested inside them can be suggested too. @@ -145,8 +173,8 @@ pub trait ConfigFile: Sized + Default + DeserializeOwned + Serialize { /// cannot be read, is not valid TOML, or is sectioned for some other /// component only. fn load_from_file(path: &Path) -> Result, ConfigError> { - let (config, unknown_keys) = load_section_from_file::(path, Self::section_key())? - .ok_or_else(|| { + let (config, unknown_keys, present_keys) = + load_section_from_file::(path, Self::section_key())?.ok_or_else(|| { ConfigError::ParseError( path.to_path_buf(), format!("missing [{}] section", Self::section_key()), @@ -156,6 +184,7 @@ pub trait ConfigFile: Sized + Default + DeserializeOwned + Serialize { config, path: Some(path.to_path_buf()), unknown_keys, + present_keys, }) } @@ -166,13 +195,14 @@ pub trait ConfigFile: Sized + Default + DeserializeOwned + Serialize { fn load_from_default_paths() -> Result, ConfigError> { for path in config_search_paths() { if path.exists() { - if let Some((config, unknown_keys)) = + if let Some((config, unknown_keys, present_keys)) = load_section_from_file::(&path, Self::section_key())? { return Ok(ConfigLoad { config, path: Some(path), unknown_keys, + present_keys, }); } } @@ -181,6 +211,7 @@ pub trait ConfigFile: Sized + Default + DeserializeOwned + Serialize { config: Self::default(), path: None, unknown_keys: Vec::new(), + present_keys: BTreeSet::new(), }) } } diff --git a/src/trx-config/src/server.rs b/src/trx-config/src/server.rs index 586f83a2..72c9c888 100644 --- a/src/trx-config/src/server.rs +++ b/src/trx-config/src/server.rs @@ -1092,6 +1092,33 @@ impl ConfigFile for ServerConfig { "trx-server" } + /// The flat per-rig sections are silently ignored once `[[rigs]]` exists, + /// which is worth saying out loud. + fn deprecations(present: &std::collections::BTreeSet) -> Vec { + if !present.contains("rigs") { + return Vec::new(); + } + [ + "rig", + "behavior", + "audio", + "sdr", + "pskreporter", + "aprsfi", + "decode_logs", + "decoders", + ] + .iter() + .filter(|section| present.contains(**section)) + .map(|section| { + format!( + "[{section}] is ignored because [[rigs]] is set; move it to the \ + matching [rigs.{section}] entry" + ) + }) + .collect() + } + /// Include one `[[rigs]]` entry so unknown keys nested inside a rig entry /// still get a suggestion. fn reference_value() -> toml::Value { @@ -1106,6 +1133,7 @@ impl ConfigFile for ServerConfig { #[cfg(test)] mod tests { use super::*; + use crate::ConfigFile; #[test] fn test_default_config() { @@ -1751,6 +1779,28 @@ port = 4531 ); } + // --- Deprecation warnings --- + + fn present(paths: &[&str]) -> std::collections::BTreeSet { + paths.iter().map(|p| p.to_string()).collect() + } + + #[test] + fn test_deprecation_flags_flat_sections_shadowed_by_rigs() { + let warnings = ServerConfig::deprecations(&present(&["rigs", "rig", "audio"])); + assert_eq!(warnings.len(), 2, "unexpected warnings: {warnings:?}"); + assert!(warnings.iter().any(|w| w.contains("[rig] is ignored"))); + assert!(warnings.iter().any(|w| w.contains("[audio] is ignored"))); + } + + #[test] + fn test_no_deprecation_for_single_rig_layout() { + // The flat layout is the documented simple form; only warn when it is + // being silently ignored. + let warnings = ServerConfig::deprecations(&present(&["rig", "audio", "listen"])); + assert!(warnings.is_empty(), "unexpected warnings: {warnings:?}"); + } + // --- Secret indirection --- #[test] diff --git a/src/trx-server/src/main.rs b/src/trx-server/src/main.rs index b670b03e..7f7b9d93 100644 --- a/src/trx-server/src/main.rs +++ b/src/trx-server/src/main.rs @@ -30,6 +30,7 @@ use trx_core::audio::AudioStreamInfo; use trx_app::{init_logging, normalize_name}; use trx_config::shared::BoundSocket; +use trx_config::ConfigFile; use trx_backend::{register_builtin_backends_on, RegistrationContext, RigAccess}; use trx_core::rig::controller::{AdaptivePolling, ExponentialBackoff}; use trx_core::rig::request::RigRequest; @@ -940,8 +941,10 @@ fn check_config(cli: &Cli, loaded: &trx_config::ConfigLoad) -> Dyn None => println!("(no config file found; checking built-in defaults)"), } - for key in &loaded.unknown_keys { - println!(" warning: {}", key); + let mut warnings: Vec = loaded.unknown_keys.iter().map(|k| k.to_string()).collect(); + warnings.extend(ServerConfig::deprecations(&loaded.present_keys)); + for warning in &warnings { + println!(" warning: {}", warning); } let mut cfg = loaded.config.clone(); @@ -966,17 +969,12 @@ fn check_config(cli: &Cli, loaded: &trx_config::ConfigLoad) -> Dyn rigs.len(), rigs.iter().map(|r| r.id.as_str()).collect::>().join(", ") ); - if !loaded.unknown_keys.is_empty() { - println!(" {} warning(s)", loaded.unknown_keys.len()); + if !warnings.is_empty() { + println!(" {} warning(s)", warnings.len()); } Ok(()) } else { - Err(format!( - "{} error(s), {} warning(s)", - errors.len(), - loaded.unknown_keys.len() - ) - .into()) + Err(format!("{} error(s), {} warning(s)", errors.len(), warnings.len()).into()) } } @@ -1010,6 +1008,7 @@ async fn main() -> DynResult<()> { info!("Loaded configuration from {}", path.display()); } loaded.report_unknown_keys(cli.strict_config)?; + loaded.report_deprecations(); let mut cfg = loaded.config; // Secrets configured as *_file are read before validation, so everything