From f944d9d65dbf9a5bc037383a4cf77520eb0e9aae Mon Sep 17 00:00:00 2001 From: Ellis Haker <66984057+petrovvvv@users.noreply.github.com> Date: Wed, 29 May 2024 13:50:26 -0700 Subject: [PATCH 1/2] changed default log file to ~/.odilia.log (#151) * changed default log file to ~/.odilia.log * updated to use env instead of dirs home_dir, need to create odilia directory in .local/share * changed log file to XDG_DATA * removed TODOs * changed log path --------- Co-authored-by: Ellis Haker --- Cargo.lock | 2 +- common/Cargo.toml | 1 + common/src/settings/log.rs | 14 +++++++++----- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 089551d5..f66fef1b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1601,7 +1601,6 @@ dependencies = [ name = "odilia-cache" version = "0.3.0" dependencies = [ - "async-trait", "atspi", "atspi-client", "atspi-common 0.5.0", @@ -1633,6 +1632,7 @@ dependencies = [ "serde_plain", "smartstring", "thiserror", + "xdg", "zbus", ] diff --git a/common/Cargo.toml b/common/Cargo.toml index 65ca163f..ffdbd648 100644 --- a/common/Cargo.toml +++ b/common/Cargo.toml @@ -21,3 +21,4 @@ thiserror = "1.0.37" zbus.workspace = true serde_plain.workspace = true figment = "0.10.15" +xdg = "2.4.1" diff --git a/common/src/settings/log.rs b/common/src/settings/log.rs index d687f976..b717440f 100644 --- a/common/src/settings/log.rs +++ b/common/src/settings/log.rs @@ -1,6 +1,6 @@ +use serde::{Deserialize, Serialize}; use std::path::PathBuf; -use serde::{Deserialize, Serialize}; ///structure used for all the configurable options related to logging #[derive(Debug, Serialize, Deserialize)] #[allow(clippy::module_name_repetitions)] @@ -16,10 +16,14 @@ pub struct LogSettings { } impl Default for LogSettings { fn default() -> Self { - Self { - level: "info".to_owned(), - logger: LoggingKind::File("/var/log/odilia.log".into()), - } + let xdg_dirs = xdg::BaseDirectories::with_prefix("odilia").expect( + "unable to find the odilia config directory according to the xdg dirs specification", + ); + let log_path = xdg_dirs + .place_state_file("odilia.log") + .expect("unable to place log file"); + + Self { level: "info".to_owned(), logger: LoggingKind::File(log_path) } } } From a31331ec095e0e06c0c2b81fd307f6d903585102 Mon Sep 17 00:00:00 2001 From: albertotirla <77588370+albertotirla@users.noreply.github.com> Date: Thu, 30 May 2024 00:23:50 +0300 Subject: [PATCH 2/2] some long awaited fixes (#152) * logging: remove printing to stderr if environment variables for filter errors for other reasons than not found, since we give the values provided in the configuration anyway * logging: use simplified `file::create` instead of providing all the options needlessly * logging: make output of logs not be sent in two places, both stderr and the logging method described in the configuration file * workspace: make xdg a workspace dependency * config: make default logging path be in $XDG_STATE_HOME * config: make system have priority before user when a user value is not specified and make the toml writer to write all the layers before the user one in the configuration it writes to disk in case of missing configuration * state: remove the ret when it's not required for us to know the value, to reduce log complexity * events: remove some unnecesary use of inlines and remove ret, err attributes when their output in the logs would be redundant --- Cargo.toml | 1 + common/Cargo.toml | 2 +- odilia/Cargo.toml | 2 +- odilia/src/events/object.rs | 34 ++++++++------------ odilia/src/logging.rs | 63 +++++++++++++++---------------------- odilia/src/main.rs | 19 +++++------ odilia/src/state.rs | 10 +++--- 7 files changed, 56 insertions(+), 75 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 726d5c16..3541c739 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -53,3 +53,4 @@ tracing-tree = "^0.2.2" zbus = { version = "3.14.1", features = ["tokio"] } serde_plain = "1.0.1" +xdg = "2.5.2" diff --git a/common/Cargo.toml b/common/Cargo.toml index ffdbd648..f234421f 100644 --- a/common/Cargo.toml +++ b/common/Cargo.toml @@ -21,4 +21,4 @@ thiserror = "1.0.37" zbus.workspace = true serde_plain.workspace = true figment = "0.10.15" -xdg = "2.4.1" +xdg.workspace=true diff --git a/odilia/Cargo.toml b/odilia/Cargo.toml index 794d7a4e..2ce5eb49 100644 --- a/odilia/Cargo.toml +++ b/odilia/Cargo.toml @@ -51,7 +51,7 @@ tracing-log.workspace = true tracing-subscriber.workspace = true tracing-tree.workspace = true tracing.workspace = true -xdg = "2.4.1" +xdg.workspace=true zbus.workspace = true odilia-notify = { version = "0.1.0", path = "../odilia-notify" } clap = { version = "4.5.1", features = ["derive"] } diff --git a/odilia/src/events/object.rs b/odilia/src/events/object.rs index 0a8dc7e4..61b28b40 100644 --- a/odilia/src/events/object.rs +++ b/odilia/src/events/object.rs @@ -1,7 +1,7 @@ use crate::state::ScreenReaderState; use atspi_common::events::object::ObjectEvents; -#[tracing::instrument(level = "debug", skip(state), ret, err)] +#[tracing::instrument(level = "debug", skip(state), err)] pub async fn dispatch(state: &ScreenReaderState, event: &ObjectEvents) -> eyre::Result<()> { // Dispatch based on member match event { @@ -36,7 +36,6 @@ mod text_changed { use ssip_client_async::Priority; use std::collections::HashMap; - #[inline] #[tracing::instrument(level = "trace")] pub fn update_string_insert( start_pos: usize, @@ -74,21 +73,18 @@ mod text_changed { } } - #[inline] pub fn append_to_object(original: &str, to_append: &str) -> String { let mut new_text = original.chars().collect::>(); new_text.extend(to_append.chars()); new_text.into_iter().collect() } - #[inline] pub fn insert_at_index(original: &str, to_splice: &str, index: usize) -> String { let mut new_text = original.chars().collect::>(); new_text.splice(index.., to_splice.chars()); new_text.into_iter().collect() } - #[inline] pub fn insert_at_range( original: &str, to_splice: &str, @@ -100,10 +96,9 @@ mod text_changed { new_text.into_iter().collect() } - #[inline] /// Get the live state of a set of attributes. /// Although the function only currently tests one attribute, in the future it may be important to inspect many attributes, compare them, or do additional logic. - #[tracing::instrument(level = "trace", ret, err)] + #[tracing::instrument(level = "trace", ret)] pub fn get_live_state(attributes: &HashMap) -> OdiliaResult { match attributes.get("live") { None => Err(OdiliaError::NoAttributeError("live".to_string())), @@ -111,7 +106,6 @@ mod text_changed { } } - #[inline] /// if the aria-live attribute is set to "polite", then set the priority of the message to speak once all other messages are done /// if the aria-live attribute is set to "assertive", then set the priority of the message to speak immediately, stop all other messages, and do not interrupt that piece of speech /// otherwise, do not continue @@ -123,8 +117,7 @@ mod text_changed { } } - #[inline] - #[tracing::instrument(level = "trace", ret, err)] + #[tracing::instrument(level = "trace", ret)] pub fn get_atomic_state(attributes: &HashMap) -> OdiliaResult { match attributes.get("atomic") { None => Err(OdiliaError::NoAttributeError("atomic".to_string())), @@ -162,7 +155,7 @@ mod text_changed { } } - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state), err)] pub async fn dispatch( state: &ScreenReaderState, event: &TextChangedEvent, @@ -180,7 +173,7 @@ mod text_changed { Ok(()) } - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state))] pub async fn speak_insertion( state: &ScreenReaderState, event: &TextChangedEvent, @@ -203,7 +196,7 @@ mod text_changed { /// The `insert` boolean, if set to true, will update the text in the cache. /// If it is set to false, the selection will be removed. /// The [`TextChangedEvent::operation`] value will *NOT* be checked by this function. - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state), err)] pub async fn insert_or_delete( state: &ScreenReaderState, event: &TextChangedEvent, @@ -258,7 +251,7 @@ mod children_changed { use odilia_common::result::OdiliaResult; use std::sync::Arc; - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state), err)] pub async fn dispatch( state: &ScreenReaderState, event: &ChildrenChangedEvent, @@ -271,7 +264,7 @@ mod children_changed { } Ok(()) } - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state), err)] pub async fn add( state: &ScreenReaderState, event: &ChildrenChangedEvent, @@ -286,7 +279,6 @@ mod children_changed { tracing::debug!("Add a single item to cache."); Ok(()) } - #[inline] fn get_child_primitive(event: &ChildrenChangedEvent) -> AccessiblePrimitive { event.child.clone().into() } @@ -395,7 +387,7 @@ mod text_caret_moved { } // TODO: left/right vs. up/down, and use generated speech - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state), err)] pub async fn text_cursor_moved( state: &ScreenReaderState, event: &TextCaretMovedEvent, @@ -431,7 +423,7 @@ mod text_caret_moved { Ok(()) } - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state), err)] pub async fn dispatch( state: &ScreenReaderState, event: &TextCaretMovedEvent, @@ -472,7 +464,7 @@ mod state_changed { } } - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state), err)] pub async fn dispatch( state: &ScreenReaderState, event: &StateChangedEvent, @@ -481,7 +473,7 @@ mod state_changed { // update cache with state of item let a11y_prim = AccessiblePrimitive::from_event(event)?; if update_state(state, &a11y_prim, event.state, state_value)? { - tracing::debug!("Updating of the state was not succesful! The item with id {:?} was not found in the cache.", a11y_prim.id); + tracing::trace!("Updating of the state was not successful! The item with id {:?} was not found in the cache.", a11y_prim.id); } else { tracing::trace!("Updated the state of accessible with ID {:?}, and state {:?} to {state_value}.", a11y_prim.id, event.state); } @@ -497,7 +489,7 @@ mod state_changed { Ok(()) } - #[tracing::instrument(level = "debug", skip(state), ret, err)] + #[tracing::instrument(level = "debug", skip(state), err)] pub async fn focused( state: &ScreenReaderState, event: &StateChangedEvent, diff --git a/odilia/src/logging.rs b/odilia/src/logging.rs index 278b7387..c79e53c2 100644 --- a/odilia/src/logging.rs +++ b/odilia/src/logging.rs @@ -3,59 +3,46 @@ //! Not much here yet, but this will get more complex if we decide to add other layers for error //! reporting, tokio-console, etc. -use std::env; +use std::{env, io}; use eyre::Context; use odilia_common::settings::{log::LoggingKind, ApplicationConfig}; use tracing_error::ErrorLayer; -use tracing_log::LogTracer; use tracing_subscriber::{prelude::*, EnvFilter}; use tracing_tree::HierarchicalLayer; /// Initialise the logging stack /// this requires an application configuration structure, so configuration must be initialized before logging is pub fn init(config: &ApplicationConfig) -> eyre::Result<()> { - let env_filter = - match env::var("APP_LOG").or_else(|_| env::var("RUST_LOG")) { - Ok(s) => EnvFilter::from(s), - Err(env::VarError::NotPresent) => EnvFilter::from(&config.log.level), - Err(e) => { - eprintln!("Warning: Failed to read log filter from APP_LOG or RUST_LOG: {e}"); - EnvFilter::from(&config.log.level) - } - }; - //this requires boxing because the types returned by this match block would be incompatible otherwise, since we return different layers depending on what we get from the configuration. It is possible to do it otherwise, hopefully, but for now this and a forced dereference at the end would do - let output_layer = match &config.log.logger { + let env_filter = match env::var("APP_LOG").or_else(|_| env::var("RUST_LOG")) { + Ok(s) => EnvFilter::from(s), + _ => EnvFilter::from(&config.log.level), + }; + let tree = HierarchicalLayer::new(4) + .with_bracketed_fields(true) + .with_targets(true) + .with_deferred_spans(true) + .with_span_retrace(true) + .with_indent_lines(true) + .with_ansi(false) + .with_wraparound(4); + //this requires boxing because the types returned by this match block would be incompatible otherwise, since we return different layers, or modifications to a layer depending on what we get from the configuration. It is possible to do it otherwise, hopefully, but for now this would do + let final_layer = match &config.log.logger { LoggingKind::File(path) => { - let file = std::fs::File::options() - .create_new(true) - .write(true) - .open(path) - .with_context(|| { - format!("creating log file '{}'", path.display()) - })?; - let fmt = - tracing_subscriber::fmt::layer().with_ansi(false).with_writer(file); - fmt.boxed() + let file = std::fs::File::create(path).with_context(|| { + format!("creating log file '{}'", path.display()) + })?; + tree.with_writer(file).boxed() } - LoggingKind::Tty => tracing_subscriber::fmt::layer() - .with_ansi(true) - .with_target(true) + LoggingKind::Tty => tree.with_writer(io::stdout).with_ansi(true).boxed(), + LoggingKind::Syslog => tracing_journald::Layer::new()? + .with_syslog_identifier("odilia".to_owned()) .boxed(), - LoggingKind::Syslog => tracing_journald::layer()?.boxed(), }; - let subscriber = tracing_subscriber::Registry::default() + tracing_subscriber::Registry::default() .with(env_filter) - .with(output_layer) .with(ErrorLayer::default()) - .with(HierarchicalLayer::new(4) - .with_bracketed_fields(true) - .with_targets(true) - .with_deferred_spans(true) - .with_span_retrace(true) - .with_indent_lines(true)); - tracing::subscriber::set_global_default(subscriber) - .wrap_err("unable to init default logging layer")?; - LogTracer::init().wrap_err("unable to init tracing log layer")?; + .with(final_layer) + .init(); Ok(()) } diff --git a/odilia/src/main.rs b/odilia/src/main.rs index 427b568f..632675fa 100644 --- a/odilia/src/main.rs +++ b/odilia/src/main.rs @@ -165,7 +165,7 @@ async fn main() -> eyre::Result<()> { fn load_configuration(cli_overide: Option) -> Result { // In order, do a configuration file specified via cli, XDG_CONFIG_HOME, the usual location for system wide configuration(/etc/odilia/config.toml) - // If XDG_CONFIG_HOME based configuration wasn't found, create one with default values for the user to alter, for the next run of odilia + // If XDG_CONFIG_HOME based configuration wasn't found, create one by combining default values with the system provided ones, if available, for the user to alter, for the next run of odilia //default configuration first, because that doesn't affect the priority outlined above let figment = Figment::from(Serialized::defaults(ApplicationConfig::default())); //cli override, if applicable @@ -180,15 +180,16 @@ fn load_configuration(cli_overide: Option) -> Result>( &self, event: &T, @@ -307,7 +307,7 @@ impl ScreenReaderState { .get_or_create(&accessible_proxy, Arc::downgrade(&self.cache)) .await } - #[tracing::instrument(skip_all, ret, err)] + #[tracing::instrument(skip_all, err)] pub async fn new_accessible<'a, T: GenericEvent<'a>>( &self, event: &T, @@ -321,7 +321,7 @@ impl ScreenReaderState { .build() .await?) } - #[tracing::instrument(skip_all, ret, err)] + #[tracing::instrument(skip_all, err)] pub async fn add_cache_match_rule(&self) -> OdiliaResult<()> { let cache_rule = MatchRule::builder() .msg_type(MessageType::Signal)