notifications: log directly about non-fatal errors

Author: djc

src/config.rs

@@ -8,7 +8,7 @@ use anyhow::{Context, Result, anyhow, bail};
 use serde::Deserialize;
 use thiserror::Error as ThisError;
 use tokio_stream::StreamExt;
-use tracing::trace;
+use tracing::{error, trace};
 
 use crate::dist::AutoInstallMode;
 use crate::{
@@ -939,7 +939,7 @@ impl<'a> Cfg<'a> {
                 .update_extra(&[], &[], profile, force_update, false)
                 .await;
             if let Err(e) = &st {
-                (self.notify_handler)(Notification::NonFatalError(e));
+                error!("{e}");
             }
             (desc, st)
         });

src/dist/component/transaction.rs

@@ -13,12 +13,11 @@ use std::fs::File;
 use std::path::{Path, PathBuf};
 
 use anyhow::{Context, Result, anyhow};
-use tracing::info;
+use tracing::{error, info};
 
 use crate::dist::prefix::InstallPrefix;
 use crate::dist::temp;
 use crate::errors::*;
-use crate::notifications::Notification;
 use crate::process::Process;
 use crate::utils;
 
@@ -39,23 +38,16 @@ pub struct Transaction<'a> {
     prefix: InstallPrefix,
     changes: Vec<ChangedItem<'a>>,
     tmp_cx: &'a temp::Context,
-    notify_handler: &'a dyn Fn(Notification<'_>),
     committed: bool,
     process: &'a Process,
 }
 
 impl<'a> Transaction<'a> {
-    pub fn new(
-        prefix: InstallPrefix,
-        tmp_cx: &'a temp::Context,
-        notify_handler: &'a dyn Fn(Notification<'_>),
-        process: &'a Process,
-    ) -> Self {
+    pub fn new(prefix: InstallPrefix, tmp_cx: &'a temp::Context, process: &'a Process) -> Self {
         Transaction {
             prefix,
             changes: Vec::new(),
             tmp_cx,
-            notify_handler,
             committed: false,
             process,
         }
@@ -179,9 +171,7 @@ impl Drop for Transaction<'_> {
                 //          Notification::NonFatalError,
                 match item.roll_back(&self.prefix, self.process) {
                     Ok(()) => {}
-                    Err(e) => {
-                        (self.notify_handler)(Notification::NonFatalError(&e));
-                    }
+                    Err(e) => error!("{e}"),
                 }
             }
         }

src/dist/manifestation.rs

@@ -216,12 +216,7 @@ impl Manifestation {
         }
 
         // Begin transaction
-        let mut tx = Transaction::new(
-            prefix.clone(),
-            tmp_cx,
-            download_cfg.notify_handler,
-            download_cfg.process,
-        );
+        let mut tx = Transaction::new(prefix.clone(), tmp_cx, download_cfg.process);
 
         // If the previous installation was from a v1 manifest we need
         // to uninstall it first.
@@ -329,7 +324,7 @@ impl Manifestation {
     ) -> Result<()> {
         let prefix = self.installation.prefix();
 
-        let mut tx = Transaction::new(prefix.clone(), tmp_cx, notify_handler, process);
+        let mut tx = Transaction::new(prefix.clone(), tmp_cx, process);
 
         // Read configuration and delete it
         let rel_config_path = prefix.rel_manifest_file(CONFIG_FILE);
@@ -476,7 +471,7 @@ impl Manifestation {
         ));
 
         // Begin transaction
-        let mut tx = Transaction::new(prefix, tmp_cx, notify_handler, process);
+        let mut tx = Transaction::new(prefix, tmp_cx, process);
 
         // Uninstall components
         let components = self.installation.list()?;

src/notifications.rs

@@ -20,7 +20,6 @@ pub enum Notification<'a> {
     FileAlreadyDownloaded,
     CachedFileChecksumFailed,
     ExtensionNotInstalled(&'a str),
-    NonFatalError(&'a anyhow::Error),
     MissingInstalledComponent(&'a str),
     /// The URL of the download is passed as the last argument, to allow us to track concurrent downloads.
     DownloadingComponent(&'a str, &'a TargetTriple, Option<&'a TargetTriple>, &'a str),
@@ -108,7 +107,6 @@ impl Notification<'_> {
             | ComponentUnavailable(_, _)
             | ForcingUnavailableComponent(_)
             | StrayHash(_) => NotificationLevel::Warn,
-            NonFatalError(_) => NotificationLevel::Error,
             SignatureInvalid(_) => NotificationLevel::Warn,
             SetDefaultBufferSize(_) => NotificationLevel::Trace,
             DownloadingFile(_, _)
@@ -163,7 +161,6 @@ impl Display for Notification<'_> {
             FileAlreadyDownloaded => write!(f, "reusing previously downloaded file"),
             CachedFileChecksumFailed => write!(f, "bad checksum for cached download"),
             ExtensionNotInstalled(c) => write!(f, "extension '{c}' was not installed"),
-            NonFatalError(e) => write!(f, "{e}"),
             MissingInstalledComponent(c) => {
                 write!(f, "during uninstall component {c} was not found")
             }

src/test/dist.rs

@@ -22,7 +22,6 @@ use crate::dist::{
     prefix::InstallPrefix,
     temp,
 };
-use crate::notifications::Notification;
 use crate::process::TestProcess;
 
 pub struct DistContext {
@@ -67,12 +66,7 @@ impl DistContext {
     }
 
     pub fn transaction(&self) -> Transaction<'_> {
-        Transaction::new(
-            self.prefix.clone(),
-            &self.cx,
-            &|_: Notification<'_>| (),
-            &self.tp.process,
-        )
+        Transaction::new(self.prefix.clone(), &self.cx, &self.tp.process)
     }
 }
 

tests/suite/dist_install.rs

@@ -5,7 +5,6 @@ use rustup::dist::component::Components;
 use rustup::dist::component::Transaction;
 use rustup::dist::component::{DirectoryPackage, Package};
 use rustup::dist::prefix::InstallPrefix;
-use rustup::notifications::Notification;
 use rustup::test::{DistContext, MockComponentBuilder, MockFile, MockInstallerBuilder};
 use rustup::utils;
 
@@ -170,8 +169,7 @@ fn uninstall() {
     tx.commit();
 
     // Now uninstall
-    let notify = |_: Notification<'_>| ();
-    let mut tx = Transaction::new(cx.prefix.clone(), &cx.cx, &notify, &cx.tp.process);
+    let mut tx = Transaction::new(cx.prefix.clone(), &cx.cx, &cx.tp.process);
     for component in components.list().unwrap() {
         tx = component.uninstall(tx, &cx.tp.process).unwrap();
     }