diff --git a/src/cli/rustup_mode.rs b/src/cli/rustup_mode.rs index 44fe79e63b..20cbacd571 100644 --- a/src/cli/rustup_mode.rs +++ b/src/cli/rustup_mode.rs @@ -1557,10 +1557,7 @@ fn override_remove( }; for p in &paths { - if cfg - .settings_file - .with_mut(|s| Ok(s.remove_override(p, cfg.notify_handler.as_ref())))? - { + if cfg.settings_file.with_mut(|s| Ok(s.remove_override(p)))? { info!("override toolchain for '{}' removed", p.display()); } else { info!("no override toolchain for '{}'", p.display()); diff --git a/src/cli/self_update.rs b/src/cli/self_update.rs index be8f87c4e2..f3a5ab133b 100644 --- a/src/cli/self_update.rs +++ b/src/cli/self_update.rs @@ -61,7 +61,6 @@ use crate::{ dist::{self, PartialToolchainDesc, Profile, TargetTriple, ToolchainDesc}, errors::RustupError, install::UpdateStatus, - notifications::Notification, process::{Process, terminalsource}, toolchain::{ DistributableToolchain, MaybeOfficialToolchainName, ResolvableToolchainName, Toolchain, @@ -770,7 +769,7 @@ fn install_bins(process: &Process) -> Result<()> { let this_exe_path = utils::current_exe()?; let rustup_path = bin_path.join(format!("rustup{EXE_SUFFIX}")); - utils::ensure_dir_exists("bin", &bin_path, &|_: Notification<'_>| {})?; + utils::ensure_dir_exists("bin", &bin_path)?; // NB: Even on Linux we can't just copy the new binary over the (running) // old binary; we must unlink it first. if rustup_path.exists() { @@ -1004,7 +1003,7 @@ pub(crate) fn uninstall(no_prompt: bool, process: &Process) -> Result| {})?; + utils::remove_dir("rustup_home", &rustup_dir)?; } info!("removing cargo home"); @@ -1026,7 +1025,7 @@ pub(crate) fn uninstall(no_prompt: bool, process: &Process) -> Result| {})?; + utils::remove_dir("cargo_home", &dirent.path())?; } else { utils::remove_file("cargo_home", &dirent.path())?; } @@ -1054,7 +1053,7 @@ pub(crate) fn uninstall(no_prompt: bool, process: &Process) -> Result| {})?; + utils::remove_dir("cargo_home", &dirent.path())?; } else { utils::remove_file("cargo_home", &dirent.path())?; } diff --git a/src/cli/self_update/unix.rs b/src/cli/self_update/unix.rs index 99aa6f806c..706553e323 100644 --- a/src/cli/self_update/unix.rs +++ b/src/cli/self_update/unix.rs @@ -6,7 +6,6 @@ use tracing::{error, warn}; use super::install_bins; use super::shell::{self, Posix, UnixShell}; -use crate::notifications::Notification; use crate::process::Process; use crate::utils; @@ -50,7 +49,7 @@ pub(crate) fn do_anti_sudo_check(no_prompt: bool, process: &Process) -> Result Result<()> { let cargo_home = process.cargo_home()?; - utils::remove_dir("cargo_home", &cargo_home, &|_: Notification<'_>| ()) + utils::remove_dir("cargo_home", &cargo_home) } pub(crate) fn do_remove_from_path(process: &Process) -> Result<()> { @@ -98,7 +97,7 @@ pub(crate) fn do_add_to_path(process: &Process) -> Result<()> { rc.display() ) })?; - utils::ensure_dir_exists("rcfile dir", rc_dir, &|_: Notification<'_>| ())?; + utils::ensure_dir_exists("rcfile dir", rc_dir)?; utils::append_file("rcfile", &rc, cmd_to_write) .with_context(|| format!("could not amend shell profile: '{}'", rc.display()))?; } diff --git a/src/cli/self_update/windows.rs b/src/cli/self_update/windows.rs index 100e0bae39..c247cc8b25 100644 --- a/src/cli/self_update/windows.rs +++ b/src/cli/self_update/windows.rs @@ -23,7 +23,6 @@ use super::{InstallOpts, install_bins, report_error}; use crate::cli::{download_tracker::DownloadTracker, markdown::md}; use crate::dist::TargetTriple; use crate::download::download_file; -use crate::notifications::Notification; use crate::process::{Process, terminalsource::ColorableTerminal}; use crate::utils; @@ -373,7 +372,7 @@ pub fn complete_windows_uninstall(process: &Process) -> Result // Now that the parent has exited there are hopefully no more files open in CARGO_HOME let cargo_home = process.cargo_home()?; - utils::remove_dir("cargo_home", &cargo_home, &|_: Notification<'_>| ())?; + utils::remove_dir("cargo_home", &cargo_home)?; // Now, run a *system* binary to inherit the DELETE_ON_CLOSE // handle to *this* process, then exit. The OS will delete the gc diff --git a/src/config.rs b/src/config.rs index fbb173d0cb..fb75b6a9ae 100644 --- a/src/config.rs +++ b/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::{ @@ -247,7 +247,7 @@ impl<'a> Cfg<'a> { // Set up the rustup home directory let rustup_dir = process.rustup_home()?; - utils::ensure_dir_exists("home", &rustup_dir, notify_handler.as_ref())?; + utils::ensure_dir_exists("home", &rustup_dir)?; let settings_file = SettingsFile::new(rustup_dir.join("settings.toml")); settings_file.with(|s| { @@ -290,13 +290,7 @@ impl<'a> Cfg<'a> { }; let dist_root_server = dist_root_server(process)?; - - let notify_clone = notify_handler.clone(); - let tmp_cx = temp::Context::new( - rustup_dir.join("tmp"), - dist_root_server.as_str(), - Box::new(move |n| (notify_clone)(n)), - ); + let tmp_cx = temp::Context::new(rustup_dir.join("tmp"), dist_root_server.as_str()); let dist_root = dist_root_server + "/dist"; let cfg = Self { @@ -411,9 +405,7 @@ impl<'a> Cfg<'a> { } pub(crate) fn ensure_toolchains_dir(&self) -> Result<(), anyhow::Error> { - utils::ensure_dir_exists("toolchains", &self.toolchains_dir, &|n| { - (self.notify_handler)(n) - })?; + utils::ensure_dir_exists("toolchains", &self.toolchains_dir)?; Ok(()) } @@ -437,11 +429,7 @@ impl<'a> Cfg<'a> { create_parent: bool, ) -> Result { if create_parent { - utils::ensure_dir_exists( - "update-hash", - &self.update_hash_dir, - self.notify_handler.as_ref(), - )?; + utils::ensure_dir_exists("update-hash", &self.update_hash_dir)?; } Ok(self.update_hash_dir.join(toolchain.to_string())) @@ -468,7 +456,7 @@ impl<'a> Cfg<'a> { let dirs = utils::read_dir("toolchains", &self.toolchains_dir)?; for dir in dirs { let dir = dir.context("IO Error reading toolchains")?; - utils::remove_dir("toolchain", &dir.path(), self.notify_handler.as_ref())?; + utils::remove_dir("toolchain", &dir.path())?; } // Also delete the update hashes @@ -582,7 +570,7 @@ impl<'a> Cfg<'a> { while let Some(d) = dir { // First check the override database - if let Some(name) = settings.dir_override(d, notify) { + if let Some(name) = settings.dir_override(d) { let reason = ActiveReason::OverrideDB(d.to_owned()); // Note that `rustup override set` fully resolves it's input // before writing to settings.toml, so resolving here may not @@ -945,7 +933,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) }); diff --git a/src/dist/component/components.rs b/src/dist/component/components.rs index db9a7805c1..bdc92fefa3 100644 --- a/src/dist/component/components.rs +++ b/src/dist/component/components.rs @@ -266,7 +266,7 @@ impl Component { let temp = tx.temp().new_file()?; utils::filter_file("components", &abs_path, &temp, |l| l != self.name)?; tx.modify_file(path)?; - utils::rename("components", &temp, &abs_path, tx.notify_handler(), process)?; + utils::rename("components", &temp, &abs_path, process)?; // TODO: If this is the last component remove the components file // and the version file. diff --git a/src/dist/component/package.rs b/src/dist/component/package.rs index c89dbe1ccc..eb17588464 100644 --- a/src/dist/component/package.rs +++ b/src/dist/component/package.rs @@ -138,10 +138,10 @@ impl Package for DirectoryPackage { #[derive(Debug)] #[allow(dead_code)] // temp::Dir is held for drop. -pub(crate) struct TarPackage<'a>(DirectoryPackage, temp::Dir<'a>); +pub(crate) struct TarPackage(DirectoryPackage, temp::Dir); -impl<'a> TarPackage<'a> { - pub(crate) fn new(stream: R, cx: &PackageContext<'a>) -> Result { +impl TarPackage { + pub(crate) fn new(stream: R, cx: &PackageContext<'_>) -> Result { let temp_dir = cx.tmp_cx.new_directory()?; let mut archive = tar::Archive::new(stream); // The rust-installer packages unpack to a directory called @@ -528,7 +528,7 @@ fn unpack_without_first_dir( Ok(()) } -impl Package for TarPackage<'_> { +impl Package for TarPackage { fn contains(&self, component: &str, short_name: Option<&str>) -> bool { self.0.contains(component, short_name) } @@ -547,16 +547,16 @@ impl Package for TarPackage<'_> { } #[derive(Debug)] -pub(crate) struct TarGzPackage<'a>(TarPackage<'a>); +pub(crate) struct TarGzPackage(TarPackage); -impl<'a> TarGzPackage<'a> { - pub(crate) fn new(stream: R, cx: &PackageContext<'a>) -> Result { +impl TarGzPackage { + pub(crate) fn new(stream: R, cx: &PackageContext<'_>) -> Result { let stream = flate2::read::GzDecoder::new(stream); Ok(TarGzPackage(TarPackage::new(stream, cx)?)) } } -impl Package for TarGzPackage<'_> { +impl Package for TarGzPackage { fn contains(&self, component: &str, short_name: Option<&str>) -> bool { self.0.contains(component, short_name) } @@ -575,16 +575,16 @@ impl Package for TarGzPackage<'_> { } #[derive(Debug)] -pub(crate) struct TarXzPackage<'a>(TarPackage<'a>); +pub(crate) struct TarXzPackage(TarPackage); -impl<'a> TarXzPackage<'a> { - pub(crate) fn new(stream: R, cx: &PackageContext<'a>) -> Result { +impl TarXzPackage { + pub(crate) fn new(stream: R, cx: &PackageContext<'_>) -> Result { let stream = xz2::read::XzDecoder::new(stream); Ok(TarXzPackage(TarPackage::new(stream, cx)?)) } } -impl Package for TarXzPackage<'_> { +impl Package for TarXzPackage { fn contains(&self, component: &str, short_name: Option<&str>) -> bool { self.0.contains(component, short_name) } @@ -603,16 +603,16 @@ impl Package for TarXzPackage<'_> { } #[derive(Debug)] -pub(crate) struct TarZStdPackage<'a>(TarPackage<'a>); +pub(crate) struct TarZStdPackage(TarPackage); -impl<'a> TarZStdPackage<'a> { - pub(crate) fn new(stream: R, cx: &PackageContext<'a>) -> Result { +impl TarZStdPackage { + pub(crate) fn new(stream: R, cx: &PackageContext<'_>) -> Result { let stream = zstd::stream::read::Decoder::new(stream)?; Ok(TarZStdPackage(TarPackage::new(stream, cx)?)) } } -impl Package for TarZStdPackage<'_> { +impl Package for TarZStdPackage { fn contains(&self, component: &str, short_name: Option<&str>) -> bool { self.0.contains(component, short_name) } diff --git a/src/dist/component/transaction.rs b/src/dist/component/transaction.rs index 4252de8a02..14858fbfcb 100644 --- a/src/dist/component/transaction.rs +++ b/src/dist/component/transaction.rs @@ -13,11 +13,11 @@ use std::fs::File; use std::path::{Path, PathBuf}; use anyhow::{Context, Result, anyhow}; +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; @@ -36,25 +36,18 @@ use crate::utils; /// already exists. pub struct Transaction<'a> { prefix: InstallPrefix, - changes: Vec>, + changes: Vec, 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, } @@ -66,7 +59,7 @@ impl<'a> Transaction<'a> { self.committed = true; } - fn change(&mut self, item: ChangedItem<'a>) { + fn change(&mut self, item: ChangedItem) { self.changes.push(item); } @@ -99,14 +92,8 @@ impl<'a> Transaction<'a> { /// Remove a file from a relative path to the install prefix. pub fn remove_file(&mut self, component: &str, relpath: PathBuf) -> Result<()> { assert!(relpath.is_relative()); - let item = ChangedItem::remove_file( - &self.prefix, - component, - relpath, - self.tmp_cx, - self.notify_handler(), - self.process, - )?; + let item = + ChangedItem::remove_file(&self.prefix, component, relpath, self.tmp_cx, self.process)?; self.change(item); Ok(()) } @@ -115,14 +102,8 @@ impl<'a> Transaction<'a> { /// install prefix. pub fn remove_dir(&mut self, component: &str, relpath: PathBuf) -> Result<()> { assert!(relpath.is_relative()); - let item = ChangedItem::remove_dir( - &self.prefix, - component, - relpath, - self.tmp_cx, - self.notify_handler(), - self.process, - )?; + let item = + ChangedItem::remove_dir(&self.prefix, component, relpath, self.tmp_cx, self.process)?; self.change(item); Ok(()) } @@ -161,14 +142,7 @@ impl<'a> Transaction<'a> { src: &Path, ) -> Result<()> { assert!(relpath.is_relative()); - let item = ChangedItem::move_file( - &self.prefix, - component, - relpath, - src, - self.notify_handler(), - self.process, - )?; + let item = ChangedItem::move_file(&self.prefix, component, relpath, src, self.process)?; self.change(item); Ok(()) } @@ -176,14 +150,7 @@ impl<'a> Transaction<'a> { /// Recursively move a directory to a relative path of the install prefix. pub(crate) fn move_dir(&mut self, component: &str, relpath: PathBuf, src: &Path) -> Result<()> { assert!(relpath.is_relative()); - let item = ChangedItem::move_dir( - &self.prefix, - component, - relpath, - src, - self.notify_handler(), - self.process, - )?; + let item = ChangedItem::move_dir(&self.prefix, component, relpath, src, self.process)?; self.change(item); Ok(()) } @@ -191,9 +158,6 @@ impl<'a> Transaction<'a> { pub(crate) fn temp(&self) -> &'a temp::Context { self.tmp_cx } - pub(crate) fn notify_handler(&self) -> &'a dyn Fn(Notification<'_>) { - self.notify_handler - } } /// If a Transaction is dropped without being committed, the changes @@ -201,15 +165,13 @@ impl<'a> Transaction<'a> { impl Drop for Transaction<'_> { fn drop(&mut self) { if !self.committed { - (self.notify_handler)(Notification::RollingBack); + info!("rolling back changes"); for item in self.changes.iter().rev() { // ok_ntfy!(self.notify_handler, // Notification::NonFatalError, - match item.roll_back(&self.prefix, self.notify_handler(), self.process) { + match item.roll_back(&self.prefix, self.process) { Ok(()) => {} - Err(e) => { - (self.notify_handler)(Notification::NonFatalError(&e)); - } + Err(e) => error!("{e}"), } } } @@ -221,33 +183,27 @@ impl Drop for Transaction<'_> { /// package, or updating a component, distill down into a series of /// these primitives. #[derive(Debug)] -enum ChangedItem<'a> { +enum ChangedItem { AddedFile(PathBuf), AddedDir(PathBuf), - RemovedFile(PathBuf, temp::File<'a>), - RemovedDir(PathBuf, temp::Dir<'a>), - ModifiedFile(PathBuf, Option>), + RemovedFile(PathBuf, temp::File), + RemovedDir(PathBuf, temp::Dir), + ModifiedFile(PathBuf, Option), } -impl<'a> ChangedItem<'a> { - fn roll_back( - &self, - prefix: &InstallPrefix, - notify: &'a dyn Fn(Notification<'_>), - process: &Process, - ) -> Result<()> { +impl ChangedItem { + fn roll_back(&self, prefix: &InstallPrefix, process: &Process) -> Result<()> { use self::ChangedItem::*; match self { AddedFile(path) => utils::remove_file("component", &prefix.abs_path(path))?, - AddedDir(path) => utils::remove_dir("component", &prefix.abs_path(path), notify)?, + AddedDir(path) => utils::remove_dir("component", &prefix.abs_path(path))?, RemovedFile(path, tmp) | ModifiedFile(path, Some(tmp)) => { - utils::rename("component", tmp, &prefix.abs_path(path), notify, process)? + utils::rename("component", tmp, &prefix.abs_path(path), process)? } RemovedDir(path, tmp) => utils::rename( "component", &tmp.join("bk"), &prefix.abs_path(path), - notify, process, )?, ModifiedFile(path, None) => { @@ -268,7 +224,7 @@ impl<'a> ChangedItem<'a> { })) } else { if let Some(p) = abs_path.parent() { - utils::ensure_dir_exists("component", p, &|_: Notification<'_>| ())?; + utils::ensure_dir_exists("component", p)?; } Ok(abs_path) } @@ -296,15 +252,14 @@ impl<'a> ChangedItem<'a> { src: &Path, ) -> Result { let abs_path = ChangedItem::dest_abs_path(prefix, component, &relpath)?; - utils::copy_dir(src, &abs_path, &|_: Notification<'_>| ())?; + utils::copy_dir(src, &abs_path)?; Ok(ChangedItem::AddedDir(relpath)) } fn remove_file( prefix: &InstallPrefix, component: &str, relpath: PathBuf, - tmp_cx: &'a temp::Context, - notify: &'a dyn Fn(Notification<'_>), + tmp_cx: &temp::Context, process: &Process, ) -> Result { let abs_path = prefix.abs_path(&relpath); @@ -316,7 +271,7 @@ impl<'a> ChangedItem<'a> { } .into()) } else { - utils::rename("component", &abs_path, &backup, notify, process)?; + utils::rename("component", &abs_path, &backup, process)?; Ok(ChangedItem::RemovedFile(relpath, backup)) } } @@ -324,8 +279,7 @@ impl<'a> ChangedItem<'a> { prefix: &InstallPrefix, component: &str, relpath: PathBuf, - tmp_cx: &'a temp::Context, - notify: &'a dyn Fn(Notification<'_>), + tmp_cx: &temp::Context, process: &Process, ) -> Result { let abs_path = prefix.abs_path(&relpath); @@ -337,14 +291,14 @@ impl<'a> ChangedItem<'a> { } .into()) } else { - utils::rename("component", &abs_path, &backup.join("bk"), notify, process)?; + utils::rename("component", &abs_path, &backup.join("bk"), process)?; Ok(ChangedItem::RemovedDir(relpath, backup)) } } fn modify_file( prefix: &InstallPrefix, relpath: PathBuf, - tmp_cx: &'a temp::Context, + tmp_cx: &temp::Context, ) -> Result { let abs_path = prefix.abs_path(&relpath); @@ -354,7 +308,7 @@ impl<'a> ChangedItem<'a> { Ok(ChangedItem::ModifiedFile(relpath, Some(backup))) } else { if let Some(p) = abs_path.parent() { - utils::ensure_dir_exists("component", p, &|_: Notification<'_>| {})?; + utils::ensure_dir_exists("component", p)?; } Ok(ChangedItem::ModifiedFile(relpath, None)) } @@ -364,11 +318,10 @@ impl<'a> ChangedItem<'a> { component: &str, relpath: PathBuf, src: &Path, - notify: &'a dyn Fn(Notification<'_>), process: &Process, ) -> Result { let abs_path = ChangedItem::dest_abs_path(prefix, component, &relpath)?; - utils::rename("component", src, &abs_path, notify, process)?; + utils::rename("component", src, &abs_path, process)?; Ok(ChangedItem::AddedFile(relpath)) } fn move_dir( @@ -376,11 +329,10 @@ impl<'a> ChangedItem<'a> { component: &str, relpath: PathBuf, src: &Path, - notify: &'a dyn Fn(Notification<'_>), process: &Process, ) -> Result { let abs_path = ChangedItem::dest_abs_path(prefix, component, &relpath)?; - utils::rename("component", src, &abs_path, notify, process)?; + utils::rename("component", src, &abs_path, process)?; Ok(ChangedItem::AddedDir(relpath)) } } diff --git a/src/dist/download.rs b/src/dist/download.rs index d7755a57f5..3106fbcfbb 100644 --- a/src/dist/download.rs +++ b/src/dist/download.rs @@ -21,7 +21,7 @@ pub struct DownloadCfg<'a> { pub dist_root: &'a str, pub tmp_cx: &'a temp::Context, pub download_dir: &'a PathBuf, - pub notify_handler: &'a dyn Fn(Notification<'_>), + pub(crate) notify_handler: &'a dyn Fn(Notification<'_>), pub process: &'a Process, } @@ -43,11 +43,7 @@ impl<'a> DownloadCfg<'a> { /// target file already exists, then the hash is checked and it is returned /// immediately without re-downloading. pub(crate) async fn download(&self, url: &Url, hash: &str) -> Result { - utils::ensure_dir_exists( - "Download Directory", - self.download_dir, - &self.notify_handler, - )?; + utils::ensure_dir_exists("Download Directory", self.download_dir)?; let target_file = self.download_dir.join(Path::new(hash)); if target_file.exists() { @@ -111,13 +107,7 @@ impl<'a> DownloadCfg<'a> { } else { (self.notify_handler)(Notification::ChecksumValid(url.as_ref())); - utils::rename( - "downloaded", - &partial_file_path, - &target_file, - self.notify_handler, - self.process, - )?; + utils::rename("downloaded", &partial_file_path, &target_file, self.process)?; Ok(File { path: target_file }) } } @@ -158,7 +148,7 @@ impl<'a> DownloadCfg<'a> { url_str: &str, update_hash: Option<&Path>, ext: &str, - ) -> Result, String)>> { + ) -> Result> { let hash = self.download_hash(url_str).await?; let partial_hash: String = hash.chars().take(UPDATE_HASH_LEN).collect(); diff --git a/src/dist/manifestation.rs b/src/dist/manifestation.rs index e5a23cb8cb..1319e70eb4 100644 --- a/src/dist/manifestation.rs +++ b/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. @@ -320,7 +315,7 @@ impl Manifestation { } #[cfg(test)] - pub fn uninstall( + pub(crate) fn uninstall( &self, manifest: &Manifest, tmp_cx: &temp::Context, @@ -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()?; diff --git a/src/dist/manifestation/tests.rs b/src/dist/manifestation/tests.rs index c8564f6a39..83689fd5ed 100644 --- a/src/dist/manifestation/tests.rs +++ b/src/dist/manifestation/tests.rs @@ -454,11 +454,7 @@ impl TestContext { let prefix_tempdir = tempfile::Builder::new().prefix("rustup").tempdir().unwrap(); let work_tempdir = tempfile::Builder::new().prefix("rustup").tempdir().unwrap(); - let tmp_cx = temp::Context::new( - work_tempdir.path().to_owned(), - DEFAULT_DIST_SERVER, - Box::new(|_| ()), - ); + let tmp_cx = temp::Context::new(work_tempdir.path().to_owned(), DEFAULT_DIST_SERVER); let toolchain = ToolchainDesc::from_str("nightly-x86_64-apple-darwin").unwrap(); let prefix = InstallPrefix::from(prefix_tempdir.path()); @@ -1471,12 +1467,7 @@ async fn unable_to_download_component() { } fn prevent_installation(prefix: &InstallPrefix) { - utils::ensure_dir_exists( - "installation path", - &prefix.path().join("lib"), - &|_: Notification<'_>| {}, - ) - .unwrap(); + utils::ensure_dir_exists("installation path", &prefix.path().join("lib")).unwrap(); let install_blocker = prefix.path().join("lib").join("rustlib"); utils::write_file("install-blocker", &install_blocker, "fail-installation").unwrap(); } @@ -1526,7 +1517,7 @@ async fn checks_files_hashes_before_reuse() { .unwrap()[..64] .to_owned(); let prev_download = cx.download_dir.join(target_hash); - utils::ensure_dir_exists("download dir", &cx.download_dir, &|_: Notification<'_>| {}).unwrap(); + utils::ensure_dir_exists("download dir", &cx.download_dir).unwrap(); utils::write_file("bad previous download", &prev_download, "bad content").unwrap(); println!("wrote previous download to {}", prev_download.display()); @@ -1560,7 +1551,7 @@ async fn handle_corrupt_partial_downloads() { .unwrap()[..SHA256_HASH_LEN] .to_owned(); - utils::ensure_dir_exists("download dir", &cx.download_dir, &|_: Notification<'_>| {}).unwrap(); + utils::ensure_dir_exists("download dir", &cx.download_dir).unwrap(); let partial_path = cx.download_dir.join(format!("{target_hash}.partial")); utils_raw::write_file( &partial_path, diff --git a/src/dist/mod.rs b/src/dist/mod.rs index faef0a0360..57bc1b91f0 100644 --- a/src/dist/mod.rs +++ b/src/dist/mod.rs @@ -1050,7 +1050,7 @@ pub(crate) async fn update_from_dist( // Don't leave behind an empty / broken installation directory if res.is_err() && fresh_install { // FIXME Ignoring cascading errors - let _ = utils::remove_dir("toolchain", prefix.path(), opts.dl_cfg.notify_handler); + let _ = utils::remove_dir("toolchain", prefix.path()); } res diff --git a/src/dist/temp.rs b/src/dist/temp.rs index d18f6d3005..66ab6995fc 100644 --- a/src/dist/temp.rs +++ b/src/dist/temp.rs @@ -3,8 +3,8 @@ use std::{fmt, fs, ops}; pub(crate) use anyhow::{Context as _, Result}; use thiserror::Error as ThisError; +use tracing::{debug, warn}; -use crate::notifications::Notification; use crate::utils::{self, raw}; #[derive(Debug, ThisError)] @@ -18,12 +18,11 @@ pub(crate) enum CreatingError { } #[derive(Debug)] -pub(crate) struct Dir<'a> { - cfg: &'a Context, +pub(crate) struct Dir { path: PathBuf, } -impl ops::Deref for Dir<'_> { +impl ops::Deref for Dir { type Target = Path; fn deref(&self) -> &Path { @@ -31,25 +30,25 @@ impl ops::Deref for Dir<'_> { } } -impl Drop for Dir<'_> { +impl Drop for Dir { fn drop(&mut self) { if raw::is_directory(&self.path) { - let n = Notification::DirectoryDeletion( - &self.path, - remove_dir_all::remove_dir_all(&self.path), - ); - (self.cfg.notify_handler)(n); + match remove_dir_all::remove_dir_all(&self.path) { + Ok(()) => debug!(path = %self.path.display(), "deleted temp directory"), + Err(e) => { + warn!(path = %self.path.display(), error = %e, "could not delete temp directory") + } + } } } } #[derive(Debug)] -pub struct File<'a> { - cfg: &'a Context, +pub struct File { path: PathBuf, } -impl ops::Deref for File<'_> { +impl ops::Deref for File { type Target = Path; fn deref(&self) -> &Path { @@ -57,11 +56,15 @@ impl ops::Deref for File<'_> { } } -impl Drop for File<'_> { +impl Drop for File { fn drop(&mut self) { if raw::is_file(&self.path) { - let n = Notification::FileDeletion(&self.path, fs::remove_file(&self.path)); - (self.cfg.notify_handler)(n); + match fs::remove_file(&self.path) { + Ok(()) => debug!(path = %self.path.display(), "deleted temp file"), + Err(e) => { + warn!(path = %self.path.display(), error = %e, "could not delete temp file") + } + } } } } @@ -69,30 +72,24 @@ impl Drop for File<'_> { pub struct Context { root_directory: PathBuf, pub dist_server: String, - notify_handler: Box)>, } impl Context { - pub fn new( - root_directory: PathBuf, - dist_server: &str, - notify_handler: Box)>, - ) -> Self { + pub fn new(root_directory: PathBuf, dist_server: &str) -> Self { Self { root_directory, dist_server: dist_server.to_owned(), - notify_handler, } } pub(crate) fn create_root(&self) -> Result { raw::ensure_dir_exists(&self.root_directory, |p| { - (self.notify_handler)(Notification::CreatingRoot(p)); + debug!(path = %p.display(), "creating temp root"); }) .with_context(|| CreatingError::Root(PathBuf::from(&self.root_directory))) } - pub(crate) fn new_directory(&self) -> Result> { + pub(crate) fn new_directory(&self) -> Result { self.create_root()?; loop { @@ -103,22 +100,19 @@ impl Context { // This is technically racey, but the probability of getting the same // random names at exactly the same time is... low. if !raw::path_exists(&temp_dir) { - (self.notify_handler)(Notification::CreatingDirectory("temp", &temp_dir)); + debug!(name = "temp", path = %temp_dir.display(), "creating directory"); fs::create_dir(&temp_dir) .with_context(|| CreatingError::Directory(PathBuf::from(&temp_dir)))?; - return Ok(Dir { - cfg: self, - path: temp_dir, - }); + return Ok(Dir { path: temp_dir }); } } } - pub fn new_file(&self) -> Result> { + pub fn new_file(&self) -> Result { self.new_file_with_ext("", "") } - pub(crate) fn new_file_with_ext(&self, prefix: &str, ext: &str) -> Result> { + pub(crate) fn new_file_with_ext(&self, prefix: &str, ext: &str) -> Result { self.create_root()?; loop { @@ -129,13 +123,10 @@ impl Context { // This is technically racey, but the probability of getting the same // random names at exactly the same time is... low. if !raw::path_exists(&temp_file) { - (self.notify_handler)(Notification::CreatingFile(&temp_file)); + debug!(path = %temp_file.display(), "creating temp file"); fs::File::create(&temp_file) .with_context(|| CreatingError::File(PathBuf::from(&temp_file)))?; - return Ok(File { - cfg: self, - path: temp_file, - }); + return Ok(File { path: temp_file }); } } } diff --git a/src/download/mod.rs b/src/download/mod.rs index 755c1229d5..41912390b6 100644 --- a/src/download/mod.rs +++ b/src/download/mod.rs @@ -96,7 +96,7 @@ async fn download_file_( use sha2::Digest; use std::cell::RefCell; - notify_handler(Notification::DownloadingFile(url, path)); + notify_handler(Notification::DownloadingFile(url)); let hasher = RefCell::new(hasher); diff --git a/src/install.rs b/src/install.rs index 22d52fecbf..2ab59b7d4b 100644 --- a/src/install.rs +++ b/src/install.rs @@ -56,7 +56,7 @@ impl InstallMethod<'_> { } nh(Notification::ToolchainDirectory(&self.dest_path())); - let updated = self.run(&self.dest_path(), &|n| nh(n)).await?; + let updated = self.run(&self.dest_path()).await?; let status = match updated { false => { @@ -84,24 +84,24 @@ impl InstallMethod<'_> { } } - async fn run(&self, path: &Path, notify_handler: &dyn Fn(Notification<'_>)) -> Result { + async fn run(&self, path: &Path) -> Result { if path.exists() { // Don't uninstall first for Dist method match self { InstallMethod::Dist { .. } => {} _ => { - uninstall(path, notify_handler)?; + uninstall(path)?; } } } match self { InstallMethod::Copy { src, .. } => { - utils::copy_dir(src, path, notify_handler)?; + utils::copy_dir(src, path)?; Ok(true) } InstallMethod::Link { src, .. } => { - utils::symlink_dir(src, path, notify_handler)?; + utils::symlink_dir(src, path)?; Ok(true) } InstallMethod::Dist(opts) => { @@ -156,6 +156,6 @@ impl InstallMethod<'_> { } } -pub(crate) fn uninstall(path: &Path, notify_handler: &dyn Fn(Notification<'_>)) -> Result<()> { - utils::remove_dir("install", path, notify_handler) +pub(crate) fn uninstall(path: &Path) -> Result<()> { + utils::remove_dir("install", path) } diff --git a/src/notifications.rs b/src/notifications.rs index 8669d12663..f011414efb 100644 --- a/src/notifications.rs +++ b/src/notifications.rs @@ -1,5 +1,4 @@ use std::fmt::{self, Display}; -use std::io; use std::path::{Path, PathBuf}; use url::Url; @@ -11,17 +10,13 @@ use crate::utils::units; use crate::{dist::ToolchainDesc, toolchain::ToolchainName, utils::notify::NotificationLevel}; #[derive(Debug)] -pub enum Notification<'a> { - Extracting(&'a Path, &'a Path), +pub(crate) enum Notification<'a> { ComponentAlreadyInstalled(&'a str), CantReadUpdateHash(&'a Path), NoUpdateHash(&'a Path), ChecksumValid(&'a str), FileAlreadyDownloaded, CachedFileChecksumFailed, - RollingBack, - 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), @@ -33,15 +28,9 @@ pub enum Notification<'a> { DownloadingLegacyManifest, SkippingNightlyMissingComponent(&'a ToolchainDesc, &'a Manifest, &'a [Component]), ForcingUnavailableComponent(&'a str), - ComponentUnavailable(&'a str, Option<&'a TargetTriple>), StrayHash(&'a Path), - SignatureInvalid(&'a str), RetryingDownload(&'a str), - CreatingDirectory(&'a str, &'a Path), - LinkingDirectory(&'a Path, &'a Path), - CopyingDirectory(&'a Path, &'a Path), - RemovingDirectory(&'a str, &'a Path), - DownloadingFile(&'a Url, &'a Path), + DownloadingFile(&'a Url), /// Received the Content-Length of the to-be downloaded data with /// the respective URL of the download (for tracking concurrent downloads). DownloadContentLengthReceived(u64, Option<&'a str>), @@ -51,25 +40,15 @@ pub enum Notification<'a> { DownloadFinished(Option<&'a str>), /// Download has failed. DownloadFailed(&'a str), - NoCanonicalPath(&'a Path), ResumingPartialDownload, /// This would make more sense as a crate::notifications::Notification /// member, but the notification callback is already narrowed to /// utils::notifications by the time tar unpacking is called. SetDefaultBufferSize(usize), Error(String), + #[cfg(feature = "curl-backend")] UsingCurl, UsingReqwest, - /// Renaming encountered a file in use error and is retrying. - /// The InUse aspect is a heuristic - the OS specifies - /// Permission denied, but as we work in users home dirs and - /// running programs like virus scanner are known to cause this - /// the heuristic is quite good. - RenameInUse(&'a Path, &'a Path), - CreatingRoot(&'a Path), - CreatingFile(&'a Path), - FileDeletion(&'a Path, io::Result<()>), - DirectoryDeletion(&'a Path, io::Result<()>), SetAutoInstall(&'a str), SetDefaultToolchain(Option<&'a ToolchainName>), SetOverrideToolchain(&'a Path, &'a str), @@ -103,47 +82,31 @@ impl Notification<'_> { | NoUpdateHash(_) | FileAlreadyDownloaded | DownloadingLegacyManifest => NotificationLevel::Debug, - Extracting(_, _) - | DownloadingComponent(_, _, _, _) + DownloadingComponent(_, _, _, _) | InstallingComponent(_, _, _) | RemovingComponent(_, _, _) | RemovingOldComponent(_, _, _) | ComponentAlreadyInstalled(_) - | RollingBack | DownloadingManifest(_) | SkippingNightlyMissingComponent(_, _, _) | RetryingDownload(_) | DownloadedManifest(_, _) => NotificationLevel::Info, CantReadUpdateHash(_) - | ExtensionNotInstalled(_) | MissingInstalledComponent(_) | CachedFileChecksumFailed - | ComponentUnavailable(_, _) | ForcingUnavailableComponent(_) | StrayHash(_) => NotificationLevel::Warn, - NonFatalError(_) => NotificationLevel::Error, - SignatureInvalid(_) => NotificationLevel::Warn, SetDefaultBufferSize(_) => NotificationLevel::Trace, - CreatingDirectory(_, _) - | RemovingDirectory(_, _) - | LinkingDirectory(_, _) - | CopyingDirectory(_, _) - | DownloadingFile(_, _) + DownloadingFile(_) | DownloadContentLengthReceived(_, _) | DownloadDataReceived(_, _) | DownloadFinished(_) | DownloadFailed(_) | ResumingPartialDownload - | UsingCurl | UsingReqwest => NotificationLevel::Debug, - RenameInUse(_, _) => NotificationLevel::Info, - NoCanonicalPath(_) => NotificationLevel::Warn, + #[cfg(feature = "curl-backend")] + UsingCurl => NotificationLevel::Debug, Error(_) => NotificationLevel::Error, - CreatingRoot(_) | CreatingFile(_) => NotificationLevel::Debug, - FileDeletion(_, result) | DirectoryDeletion(_, result) => match result { - Ok(_) => NotificationLevel::Debug, - Err(_) => NotificationLevel::Warn, - }, ToolchainDirectory(_) | LookingForToolchain(_) | InstallingToolchain(_) @@ -170,7 +133,6 @@ impl Display for Notification<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> std::result::Result<(), fmt::Error> { use self::Notification::*; match self { - Extracting(_, _) => write!(f, "extracting..."), ComponentAlreadyInstalled(c) => write!(f, "component {c} is up to date"), CantReadUpdateHash(path) => write!( f, @@ -178,12 +140,9 @@ impl Display for Notification<'_> { path.display() ), NoUpdateHash(path) => write!(f, "no update hash at: '{}'", path.display()), - ChecksumValid(_) => write!(f, "checksum passed"), + ChecksumValid(url) => write!(f, "checksum passed for {url}"), FileAlreadyDownloaded => write!(f, "reusing previously downloaded file"), CachedFileChecksumFailed => write!(f, "bad checksum for cached download"), - RollingBack => write!(f, "rolling back changes"), - 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") } @@ -228,13 +187,6 @@ impl Display for Notification<'_> { write!(f, "latest update on {date}, no rust version") } DownloadingLegacyManifest => write!(f, "manifest not found. trying legacy manifest"), - ComponentUnavailable(pkg, toolchain) => { - if let Some(tc) = toolchain { - write!(f, "component '{pkg}' is not available on target '{tc}'") - } else { - write!(f, "component '{pkg}' is not available") - } - } StrayHash(path) => write!( f, "removing stray hash found at '{}' in order to continue", @@ -259,53 +211,22 @@ impl Display for Notification<'_> { ForcingUnavailableComponent(component) => { write!(f, "Force-skipping unavailable component '{component}'") } - SignatureInvalid(url) => write!(f, "Signature verification failed for '{url}'"), RetryingDownload(url) => write!(f, "retrying download for '{url}'"), - CreatingDirectory(name, path) => { - write!(f, "creating {} directory: '{}'", name, path.display()) - } Error(e) => write!(f, "error: '{e}'"), - LinkingDirectory(_, dest) => write!(f, "linking directory from: '{}'", dest.display()), - CopyingDirectory(src, _) => write!(f, "copying directory from: '{}'", src.display()), - RemovingDirectory(name, path) => { - write!(f, "removing {} directory: '{}'", name, path.display()) - } - RenameInUse(src, dest) => write!( - f, - "retrying renaming '{}' to '{}'", - src.display(), - dest.display() - ), SetDefaultBufferSize(size) => write!( f, "using up to {} of RAM to unpack components", units::Size::new(*size) ), - DownloadingFile(url, _) => write!(f, "downloading file from: '{url}'"), + DownloadingFile(url) => write!(f, "downloading file from: '{url}'"), DownloadContentLengthReceived(len, _) => write!(f, "download size is: '{len}'"), DownloadDataReceived(data, _) => write!(f, "received some data of size {}", data.len()), DownloadFinished(_) => write!(f, "download finished"), DownloadFailed(_) => write!(f, "download failed"), - NoCanonicalPath(path) => write!(f, "could not canonicalize path: '{}'", path.display()), ResumingPartialDownload => write!(f, "resuming partial download"), + #[cfg(feature = "curl-backend")] UsingCurl => write!(f, "downloading with curl"), UsingReqwest => write!(f, "downloading with reqwest"), - CreatingRoot(path) => write!(f, "creating temp root: {}", path.display()), - CreatingFile(path) => write!(f, "creating temp file: {}", path.display()), - FileDeletion(path, result) => { - if result.is_ok() { - write!(f, "deleted temp file: {}", path.display()) - } else { - write!(f, "could not delete temp file: {}", path.display()) - } - } - DirectoryDeletion(path, result) => { - if result.is_ok() { - write!(f, "deleted temp directory: {}", path.display()) - } else { - write!(f, "could not delete temp directory: {}", path.display()) - } - } SetAutoInstall(auto) => write!(f, "auto install set to '{auto}'"), SetDefaultToolchain(None) => write!(f, "default toolchain unset"), SetDefaultToolchain(Some(name)) => write!(f, "default toolchain set to '{name}'"), diff --git a/src/settings.rs b/src/settings.rs index 44305d727a..efbc39da0b 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -98,22 +98,16 @@ pub struct Settings { } impl Settings { - fn path_to_key(path: &Path, notify_handler: &dyn Fn(Notification<'_>)) -> String { + fn path_to_key(path: &Path) -> String { if path.exists() { - utils::canonicalize_path(path, notify_handler) - .display() - .to_string() + utils::canonicalize_path(path).display().to_string() } else { path.display().to_string() } } - pub(crate) fn remove_override( - &mut self, - path: &Path, - notify_handler: &dyn Fn(Notification<'_>), - ) -> bool { - let key = Self::path_to_key(path, notify_handler); + pub(crate) fn remove_override(&mut self, path: &Path) -> bool { + let key = Self::path_to_key(path); self.overrides.remove(&key).is_some() } @@ -123,17 +117,13 @@ impl Settings { toolchain: String, notify_handler: &dyn Fn(Notification<'_>), ) { - let key = Self::path_to_key(path, notify_handler); + let key = Self::path_to_key(path); notify_handler(Notification::SetOverrideToolchain(path, &toolchain)); self.overrides.insert(key, toolchain); } - pub(crate) fn dir_override( - &self, - dir: &Path, - notify_handler: &dyn Fn(Notification<'_>), - ) -> Option { - let key = Self::path_to_key(dir, notify_handler); + pub(crate) fn dir_override(&self, dir: &Path) -> Option { + let key = Self::path_to_key(dir); self.overrides.get(&key).cloned() } diff --git a/src/test/dist.rs b/src/test/dist.rs index f5164c3d23..66031c6143 100644 --- a/src/test/dist.rs +++ b/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 { @@ -49,11 +48,7 @@ impl DistContext { pkg_dir, inst_dir, prefix, - cx: temp::Context::new( - tmp_dir.path().to_owned(), - DEFAULT_DIST_SERVER, - Box::new(|_| ()), - ), + cx: temp::Context::new(tmp_dir.path().to_owned(), DEFAULT_DIST_SERVER), tp: TestProcess::default(), _tmp_dir: tmp_dir, }) @@ -67,12 +62,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) } } diff --git a/src/toolchain.rs b/src/toolchain.rs index e905918db1..ad7183cfce 100644 --- a/src/toolchain.rs +++ b/src/toolchain.rs @@ -97,11 +97,11 @@ impl<'a> Toolchain<'a> { } ActiveReason::OverrideDB(path) => format!( "the directory override for '{}' specifies an uninstalled toolchain", - utils::canonicalize_path(path, cfg.notify_handler.as_ref()).display(), + utils::canonicalize_path(path).display(), ), ActiveReason::ToolchainFile(path) => format!( "the toolchain file at '{}' specifies an uninstalled toolchain", - utils::canonicalize_path(path, cfg.notify_handler.as_ref()).display(), + utils::canonicalize_path(path).display(), ), ActiveReason::Default => { "the default toolchain does not describe an installed toolchain".to_string() @@ -554,9 +554,7 @@ impl<'a> Toolchain<'a> { InstalledPath::File { name, path } => { utils::ensure_file_removed(name, &path)? } - InstalledPath::Dir { path } => { - install::uninstall(path, &|n| (cfg.notify_handler)(n))? - } + InstalledPath::Dir { path } => install::uninstall(path)?, } } true diff --git a/src/utils/mod.rs b/src/utils/mod.rs index d1c9ae4c0b..6c04454565 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -11,6 +11,7 @@ use std::process::ExitStatus; use anyhow::{Context, Result, anyhow, bail}; use retry::delay::{Fibonacci, jitter}; use retry::{OperationResult, retry}; +use tracing::{debug, info, warn}; use url::Url; use crate::errors::*; @@ -60,16 +61,9 @@ impl From for ExitCode { } } -pub fn ensure_dir_exists<'a, N>( - name: &'static str, - path: &'a Path, - notify_handler: &'a dyn Fn(N), -) -> Result -where - N: From>, -{ +pub fn ensure_dir_exists(name: &'static str, path: &Path) -> Result { raw::ensure_dir_exists(path, |_| { - notify_handler(Notification::CreatingDirectory(name, path).into()) + debug!(name, path = %path.display(), "creating directory"); }) .with_context(|| RustupError::CreatingDirectory { name, @@ -133,12 +127,9 @@ pub(crate) fn filter_file bool>( }) } -pub(crate) fn canonicalize_path<'a, N>(path: &'a Path, notify_handler: &dyn Fn(N)) -> PathBuf -where - N: From>, -{ +pub(crate) fn canonicalize_path(path: &Path) -> PathBuf { fs::canonicalize(path).unwrap_or_else(|_| { - notify_handler(Notification::NoCanonicalPath(path).into()); + warn!(path = %path.display(), "could not canonicalize path"); PathBuf::from(path) }) } @@ -163,15 +154,8 @@ pub(crate) fn assert_is_directory(path: &Path) -> Result<()> { } } -pub(crate) fn symlink_dir<'a, N>( - src: &'a Path, - dest: &'a Path, - notify_handler: &dyn Fn(N), -) -> Result<()> -where - N: From>, -{ - notify_handler(Notification::LinkingDirectory(src, dest).into()); +pub(crate) fn symlink_dir(src: &Path, dest: &Path) -> Result<()> { + debug!(source = %src.display(), destination = %dest.display(), "linking directory"); raw::symlink_dir(src, dest).with_context(|| { format!( "could not create link from '{}' to '{}'", @@ -233,15 +217,8 @@ fn symlink_file(src: &Path, dest: &Path) -> Result<()> { }) } -pub(crate) fn copy_dir<'a, N>( - src: &'a Path, - dest: &'a Path, - notify_handler: &dyn Fn(N), -) -> Result<()> -where - N: From>, -{ - notify_handler(Notification::CopyingDirectory(src, dest).into()); +pub(crate) fn copy_dir(src: &Path, dest: &Path) -> Result<()> { + debug!(source = %src.display(), destination = %dest.display(), "copying directory"); raw::copy_dir(src, dest).with_context(|| { format!( "could not copy directory from '{}' to '{}'", @@ -271,15 +248,8 @@ pub(crate) fn copy_file(src: &Path, dest: &Path) -> Result<()> { } } -pub(crate) fn remove_dir<'a, N>( - name: &'static str, - path: &'a Path, - notify_handler: &dyn Fn(N), -) -> Result<()> -where - N: From>, -{ - notify_handler(Notification::RemovingDirectory(name, path).into()); +pub(crate) fn remove_dir(name: &'static str, path: &Path) -> Result<()> { + debug!(name, path = %path.display(), "removing directory"); raw::remove_dir(path).with_context(|| RustupError::RemovingDirectory { name, path: PathBuf::from(path), @@ -397,41 +367,29 @@ pub(crate) fn format_path_for_display(path: &str) -> String { } #[cfg(target_os = "linux")] -fn copy_and_delete<'a, N>( - name: &'static str, - src: &'a Path, - dest: &'a Path, - notify_handler: &'a dyn Fn(N), -) -> Result<()> -where - N: From>, -{ +fn copy_and_delete(name: &'static str, src: &Path, dest: &Path) -> Result<()> { // https://github.com/rust-lang/rustup/issues/1239 // This uses std::fs::copy() instead of the faster std::fs::rename() to // avoid cross-device link errors. if src.is_dir() { - copy_dir(src, dest, notify_handler).and(remove_dir_all::remove_dir_all(src).with_context( - || RustupError::RemovingDirectory { + copy_dir(src, dest).and(remove_dir_all::remove_dir_all(src).with_context(|| { + RustupError::RemovingDirectory { name, path: PathBuf::from(src), - }, - )) + } + })) } else { copy_file(src, dest).and(remove_file(name, src)) } } -pub fn rename<'a, N>( +pub fn rename( name: &'static str, - src: &'a Path, - dest: &'a Path, - notify_handler: &'a dyn Fn(N), + src: &Path, + dest: &Path, #[allow(unused_variables)] // Only used on Linux process: &Process, -) -> Result<()> -where - N: From>, -{ +) -> Result<()> { // https://github.com/rust-lang/rustup/issues/1870 // 21 fib steps from 1 sums to ~28 seconds, hopefully more than enough // for our previous poor performance that avoided the race condition with @@ -444,14 +402,19 @@ where Ok(()) => OperationResult::Ok(()), Err(e) => match e.kind() { io::ErrorKind::PermissionDenied => { - notify_handler(Notification::RenameInUse(src, dest).into()); + // Renaming encountered a file in use error and is retrying. + // The InUse aspect is a heuristic - the OS specifies + // Permission denied, but as we work in users home dirs and + // running programs like virus scanner are known to cause this + // the heuristic is quite good. + info!(source = %src.display(), destination = %dest.display(), "renaming file in use, retrying"); OperationResult::Retry(e) } #[cfg(target_os = "linux")] _ if process.var_os("RUSTUP_PERMIT_COPY_RENAME").is_some() && Some(EXDEV) == e.raw_os_error() => { - match copy_and_delete(name, src, dest, notify_handler) { + match copy_and_delete(name, src, dest) { Ok(()) => OperationResult::Ok(()), Err(_) => OperationResult::Err(e), } diff --git a/tests/suite/dist_install.rs b/tests/suite/dist_install.rs index 00ff32165f..3090f06503 100644 --- a/tests/suite/dist_install.rs +++ b/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, ¬ify, &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(); }