From a1afc0331c888d1fc80e55d8d9d552636a206d72 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Tue, 9 Apr 2024 17:02:08 +0200 Subject: [PATCH 01/10] Extract struct from InstallMethods::Dist variant --- src/dist/mod.rs | 69 ++++++++++++++++++++++------------ src/install.rs | 68 +++++++-------------------------- src/toolchain/distributable.rs | 10 ++--- 3 files changed, 64 insertions(+), 83 deletions(-) diff --git a/src/dist/mod.rs b/src/dist/mod.rs index 1e12ffa020..b4b5adbcda 100644 --- a/src/dist/mod.rs +++ b/src/dist/mod.rs @@ -11,7 +11,10 @@ use regex::Regex; use serde::{Deserialize, Serialize}; use thiserror::Error as ThisError; -use crate::{currentprocess::Process, errors::RustupError, toolchain::ToolchainName, utils::utils}; +use crate::{ + config::Cfg, currentprocess::Process, errors::RustupError, toolchain::ToolchainName, + utils::utils, +}; pub mod component; pub(crate) mod config; @@ -707,51 +710,69 @@ impl fmt::Display for Profile { } } +#[derive(Clone)] +pub(crate) struct DistOptions<'a> { + pub(crate) cfg: &'a Cfg<'a>, + pub(crate) desc: &'a ToolchainDesc, + pub(crate) profile: Profile, + pub(crate) update_hash: Option<&'a Path>, + pub(crate) dl_cfg: DownloadCfg<'a>, + /// --force bool is whether to force an update/install + pub(crate) force: bool, + /// --allow-downgrade + pub(crate) allow_downgrade: bool, + /// toolchain already exists + pub(crate) exists: bool, + /// currently installed date and version + pub(crate) old_date_version: Option<(String, String)>, + /// Extra components to install from dist + pub(crate) components: &'a [&'a str], + /// Extra targets to install from dist + pub(crate) targets: &'a [&'a str], +} + // Installs or updates a toolchain from a dist server. If an initial // install then it will be installed with the default components. If // an upgrade then all the existing components will be upgraded. // // Returns the manifest's hash if anything changed. -#[cfg_attr(feature = "otel", tracing::instrument(err, skip_all, fields(profile=format!("{profile:?}"), prefix=prefix.path().to_string_lossy().to_string())))] +#[cfg_attr(feature = "otel", tracing::instrument(err, skip_all, fields(profile=format!("{:?}", opts.profile), prefix=prefix.path().to_string_lossy().to_string())))] pub(crate) async fn update_from_dist( - download: DownloadCfg<'_>, - update_hash: Option<&Path>, - toolchain: &ToolchainDesc, - profile: Option, prefix: &InstallPrefix, - force_update: bool, - allow_downgrade: bool, - old_date: Option<&str>, - components: &[&str], - targets: &[&str], + opts: &DistOptions<'_>, ) -> Result> { let fresh_install = !prefix.path().exists(); - let hash_exists = update_hash.map(Path::exists).unwrap_or(false); + let hash_exists = opts.update_hash.map(Path::exists).unwrap_or(false); // fresh_install means the toolchain isn't present, but hash_exists means there is a stray hash file if fresh_install && hash_exists { // It's ok to unwrap, because hash have to exist at this point - (download.notify_handler)(Notification::StrayHash(update_hash.unwrap())); - std::fs::remove_file(update_hash.unwrap())?; + (opts.dl_cfg.notify_handler)(Notification::StrayHash(opts.update_hash.unwrap())); + std::fs::remove_file(opts.update_hash.unwrap())?; } let res = update_from_dist_( - download, - update_hash, - toolchain, - profile, + opts.dl_cfg, + opts.update_hash, + opts.desc, + match opts.exists { + true => None, + false => Some(opts.profile), + }, prefix, - force_update, - allow_downgrade, - old_date, - components, - targets, + opts.force, + opts.allow_downgrade, + opts.old_date_version + .as_ref() + .map(|(date, _)| date.as_str()), + opts.components, + opts.targets, ) .await; // 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(), download.notify_handler); + let _ = utils::remove_dir("toolchain", prefix.path(), opts.dl_cfg.notify_handler); } res diff --git a/src/install.rs b/src/install.rs index 7c400f1903..37c714bfbb 100644 --- a/src/install.rs +++ b/src/install.rs @@ -6,7 +6,7 @@ use anyhow::Result; use crate::{ config::Cfg, - dist::{self, download::DownloadCfg, prefix::InstallPrefix, Notification}, + dist::{self, prefix::InstallPrefix, DistOptions, Notification}, errors::RustupError, notifications::Notification as RootNotification, toolchain::{CustomToolchainName, LocalToolchainName, Toolchain}, @@ -32,25 +32,7 @@ pub(crate) enum InstallMethod<'a> { dest: &'a CustomToolchainName, cfg: &'a Cfg<'a>, }, - Dist { - cfg: &'a Cfg<'a>, - desc: &'a dist::ToolchainDesc, - profile: dist::Profile, - update_hash: Option<&'a Path>, - dl_cfg: DownloadCfg<'a>, - /// --force bool is whether to force an update/install - force: bool, - /// --allow-downgrade - allow_downgrade: bool, - /// toolchain already exists - exists: bool, - /// currently installed date and version - old_date_version: Option<(String, String)>, - /// Extra components to install from dist - components: &'a [&'a str], - /// Extra targets to install from dist - targets: &'a [&'a str], - }, + Dist(DistOptions<'a>), } impl<'a> InstallMethod<'a> { @@ -61,10 +43,10 @@ impl<'a> InstallMethod<'a> { match self { InstallMethod::Copy { .. } | InstallMethod::Link { .. } - | InstallMethod::Dist { + | InstallMethod::Dist(DistOptions { old_date_version: None, .. - } => (nh)(RootNotification::InstallingToolchain(&self.dest_basename())), + }) => (nh)(RootNotification::InstallingToolchain(&self.dest_basename())), _ => (nh)(RootNotification::UpdatingToolchain(&self.dest_basename())), } @@ -83,10 +65,10 @@ impl<'a> InstallMethod<'a> { true => { (nh)(RootNotification::InstalledToolchain(&self.dest_basename())); match self { - InstallMethod::Dist { + InstallMethod::Dist(DistOptions { old_date_version: Some((_, v)), .. - } => UpdateStatus::Updated(v.clone()), + }) => UpdateStatus::Updated(v.clone()), InstallMethod::Copy { .. } | InstallMethod::Link { .. } | InstallMethod::Dist { .. } => UpdateStatus::Installed, @@ -121,36 +103,12 @@ impl<'a> InstallMethod<'a> { utils::symlink_dir(src, path, notify_handler)?; Ok(true) } - InstallMethod::Dist { - desc, - profile, - update_hash, - dl_cfg, - force: force_update, - allow_downgrade, - exists, - old_date_version, - components, - targets, - .. - } => { + InstallMethod::Dist(opts) => { let prefix = &InstallPrefix::from(path.to_owned()); - let maybe_new_hash = dist::update_from_dist( - *dl_cfg, - update_hash.as_deref(), - desc, - if *exists { None } else { Some(*profile) }, - prefix, - *force_update, - *allow_downgrade, - old_date_version.as_ref().map(|dv| dv.0.as_str()), - components, - targets, - ) - .await?; + let maybe_new_hash = dist::update_from_dist(prefix, opts).await?; if let Some(hash) = maybe_new_hash { - if let Some(hash_file) = update_hash { + if let Some(hash_file) = opts.update_hash { utils::write_file("update hash", hash_file, &hash)?; } @@ -166,7 +124,7 @@ impl<'a> InstallMethod<'a> { match self { InstallMethod::Copy { cfg, .. } => cfg, InstallMethod::Link { cfg, .. } => cfg, - InstallMethod::Dist { cfg, .. } => cfg, + InstallMethod::Dist(DistOptions { cfg, .. }) => cfg, } } @@ -174,7 +132,7 @@ impl<'a> InstallMethod<'a> { match self { InstallMethod::Copy { dest, .. } => (*dest).into(), InstallMethod::Link { dest, .. } => (*dest).into(), - InstallMethod::Dist { desc, .. } => (*desc).into(), + InstallMethod::Dist(DistOptions { desc, .. }) => (*desc).into(), } } @@ -186,7 +144,9 @@ impl<'a> InstallMethod<'a> { match self { InstallMethod::Copy { cfg, dest, .. } => cfg.toolchain_path(&(*dest).into()), InstallMethod::Link { cfg, dest, .. } => cfg.toolchain_path(&(*dest).into()), - InstallMethod::Dist { cfg, desc, .. } => cfg.toolchain_path(&(*desc).into()), + InstallMethod::Dist(DistOptions { cfg, desc, .. }) => { + cfg.toolchain_path(&(*desc).into()) + } } } } diff --git a/src/toolchain/distributable.rs b/src/toolchain/distributable.rs index 83d871e00c..3463305e81 100644 --- a/src/toolchain/distributable.rs +++ b/src/toolchain/distributable.rs @@ -12,7 +12,7 @@ use crate::{ manifest::{Component, ComponentStatus, Manifest}, manifestation::{Changes, Manifestation}, prefix::InstallPrefix, - PartialToolchainDesc, Profile, ToolchainDesc, + DistOptions, PartialToolchainDesc, Profile, ToolchainDesc, }, install::{InstallMethod, UpdateStatus}, notifications::Notification, @@ -336,7 +336,7 @@ impl<'a> DistributableToolchain<'a> { let hash_path = cfg.get_hash_file(desc, true)?; let update_hash = Some(&hash_path as &Path); - let status = InstallMethod::Dist { + let status = InstallMethod::Dist(DistOptions { cfg, desc, profile, @@ -348,7 +348,7 @@ impl<'a> DistributableToolchain<'a> { old_date_version: None, components, targets, - } + }) .install() .await?; Ok((status, Self::new(cfg, desc.clone())?)) @@ -412,7 +412,7 @@ impl<'a> DistributableToolchain<'a> { let hash_path = cfg.get_hash_file(&self.desc, true)?; let update_hash = Some(&hash_path as &Path); - InstallMethod::Dist { + InstallMethod::Dist(DistOptions { cfg, desc: &self.desc, profile, @@ -424,7 +424,7 @@ impl<'a> DistributableToolchain<'a> { old_date_version, components, targets, - } + }) .install() .await } From a959a1588f0a3d32451f56f7c1822fa7e0ba86fb Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Tue, 18 Jun 2024 11:38:28 +0200 Subject: [PATCH 02/10] Avoid unnecessary unwrapping --- src/dist/mod.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/dist/mod.rs b/src/dist/mod.rs index b4b5adbcda..f4bd4fc13b 100644 --- a/src/dist/mod.rs +++ b/src/dist/mod.rs @@ -742,12 +742,12 @@ pub(crate) async fn update_from_dist( opts: &DistOptions<'_>, ) -> Result> { let fresh_install = !prefix.path().exists(); - let hash_exists = opts.update_hash.map(Path::exists).unwrap_or(false); - // fresh_install means the toolchain isn't present, but hash_exists means there is a stray hash file - if fresh_install && hash_exists { - // It's ok to unwrap, because hash have to exist at this point - (opts.dl_cfg.notify_handler)(Notification::StrayHash(opts.update_hash.unwrap())); - std::fs::remove_file(opts.update_hash.unwrap())?; + if let Some(hash) = opts.update_hash { + // fresh_install means the toolchain isn't present, but hash_exists means there is a stray hash file + if fresh_install && Path::exists(hash) { + (opts.dl_cfg.notify_handler)(Notification::StrayHash(hash)); + std::fs::remove_file(hash)?; + } } let res = update_from_dist_( From 39740ed03e0f841025bc877e35e57c9269777615 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Tue, 18 Jun 2024 11:46:29 +0200 Subject: [PATCH 03/10] Propagate use of DistOptions --- src/dist/mod.rs | 76 +++++++++++++++++++------------------------------ 1 file changed, 29 insertions(+), 47 deletions(-) diff --git a/src/dist/mod.rs b/src/dist/mod.rs index f4bd4fc13b..40c2a66dee 100644 --- a/src/dist/mod.rs +++ b/src/dist/mod.rs @@ -750,25 +750,7 @@ pub(crate) async fn update_from_dist( } } - let res = update_from_dist_( - opts.dl_cfg, - opts.update_hash, - opts.desc, - match opts.exists { - true => None, - false => Some(opts.profile), - }, - prefix, - opts.force, - opts.allow_downgrade, - opts.old_date_version - .as_ref() - .map(|(date, _)| date.as_str()), - opts.components, - opts.targets, - ) - .await; - + let res = update_from_dist_(prefix, opts).await; // Don't leave behind an empty / broken installation directory if res.is_err() && fresh_install { // FIXME Ignoring cascading errors @@ -779,29 +761,21 @@ pub(crate) async fn update_from_dist( } async fn update_from_dist_( - download: DownloadCfg<'_>, - update_hash: Option<&Path>, - toolchain: &ToolchainDesc, - profile: Option, prefix: &InstallPrefix, - force_update: bool, - allow_downgrade: bool, - old_date: Option<&str>, - components: &[&str], - targets: &[&str], + opts: &DistOptions<'_>, ) -> Result> { - let mut toolchain = toolchain.clone(); let mut fetched = String::new(); let mut first_err = None; - let backtrack = toolchain.channel == "nightly" && toolchain.date.is_none(); + let backtrack = opts.desc.channel == "nightly" && opts.desc.date.is_none(); // We want to limit backtracking if we do not already have a toolchain - let mut backtrack_limit: Option = if toolchain.date.is_some() { + let mut backtrack_limit: Option = if opts.desc.date.is_some() { None } else { // We limit the backtracking to 21 days by default (half a release cycle). // The limit of 21 days is an arbitrary selection, so we let the user override it. const BACKTRACK_LIMIT_DEFAULT: i32 = 21; - let provided = download + let provided = opts + .dl_cfg .process .var("RUSTUP_BACKTRACK_LIMIT") .ok() @@ -820,30 +794,36 @@ async fn update_from_dist_( // We could arguably use the date of the first rustup release here, but that would break a // bunch of the tests, which (inexplicably) use 2015-01-01 as their manifest dates. let first_manifest = date_from_manifest_date("2014-12-20").unwrap(); - let old_manifest = old_date - .and_then(date_from_manifest_date) + let old_manifest = opts + .old_date_version + .as_ref() + .and_then(|(d, _)| date_from_manifest_date(d)) .unwrap_or(first_manifest); - let last_manifest = if allow_downgrade { + let last_manifest = if opts.allow_downgrade { first_manifest } else { old_manifest }; let current_manifest = { - let manifestation = Manifestation::open(prefix.clone(), toolchain.target.clone())?; + let manifestation = Manifestation::open(prefix.clone(), opts.desc.target.clone())?; manifestation.load_manifest()? }; + let mut toolchain = opts.desc.clone(); loop { match try_update_from_dist_( - download, - update_hash, + opts.dl_cfg, + opts.update_hash, &toolchain, - profile, + match opts.exists { + false => Some(opts.profile), + true => None, + }, prefix, - force_update, - components, - targets, + opts.force, + opts.components, + opts.targets, &mut fetched, ) .await @@ -857,11 +837,13 @@ async fn update_from_dist_( let cause = e.downcast_ref::(); match cause { Some(DistError::ToolchainComponentsMissing(components, manifest, ..)) => { - (download.notify_handler)(Notification::SkippingNightlyMissingComponent( - &toolchain, - current_manifest.as_ref().unwrap_or(manifest), - components, - )); + (opts.dl_cfg.notify_handler)( + Notification::SkippingNightlyMissingComponent( + &toolchain, + current_manifest.as_ref().unwrap_or(manifest), + components, + ), + ); if first_err.is_none() { first_err = Some(e); From fe27e91d895027490b39ea8a279afe81339cddc4 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Tue, 18 Jun 2024 11:52:15 +0200 Subject: [PATCH 04/10] Reduce rightward drift --- src/dist/mod.rs | 124 +++++++++++++++++++++++------------------------- 1 file changed, 60 insertions(+), 64 deletions(-) diff --git a/src/dist/mod.rs b/src/dist/mod.rs index 40c2a66dee..6a7bc64eaa 100644 --- a/src/dist/mod.rs +++ b/src/dist/mod.rs @@ -812,7 +812,7 @@ async fn update_from_dist_( let mut toolchain = opts.desc.clone(); loop { - match try_update_from_dist_( + let result = try_update_from_dist_( opts.dl_cfg, opts.update_hash, &toolchain, @@ -826,76 +826,72 @@ async fn update_from_dist_( opts.targets, &mut fetched, ) - .await - { - Ok(v) => break Ok(v), - Err(e) => { - if !backtrack { - break Err(e); - } - - let cause = e.downcast_ref::(); - match cause { - Some(DistError::ToolchainComponentsMissing(components, manifest, ..)) => { - (opts.dl_cfg.notify_handler)( - Notification::SkippingNightlyMissingComponent( - &toolchain, - current_manifest.as_ref().unwrap_or(manifest), - components, - ), - ); - - if first_err.is_none() { - first_err = Some(e); - } - // We decrement the backtrack count only on unavailable component errors - // so that the limit only applies to nightlies that were indeed available, - // and ignores missing ones. - backtrack_limit = backtrack_limit.map(|n| n - 1); - } + .await; - Some(DistError::MissingReleaseForToolchain(..)) => { - // no need to even print anything for missing nightlies, - // since we don't really "skip" them - } - _ => { - // All other errors break the loop - break Err(e); - } - }; + let e = match result { + Ok(v) => return Ok(v), + Err(e) if !backtrack => return Err(e), + Err(e) => e, + }; - if let Some(backtrack_limit) = backtrack_limit { - if backtrack_limit < 1 { - // This unwrap is safe because we can only hit this if we've - // had a chance to set first_err - break Err(first_err.unwrap()); - } + let cause = e.downcast_ref::(); + match cause { + Some(DistError::ToolchainComponentsMissing(components, manifest, ..)) => { + (opts.dl_cfg.notify_handler)(Notification::SkippingNightlyMissingComponent( + &toolchain, + current_manifest.as_ref().unwrap_or(manifest), + components, + )); + + if first_err.is_none() { + first_err = Some(e); } + // We decrement the backtrack count only on unavailable component errors + // so that the limit only applies to nightlies that were indeed available, + // and ignores missing ones. + backtrack_limit = backtrack_limit.map(|n| n - 1); + } - // The user asked to update their nightly, but the latest nightly does not have all - // the components that the user currently has installed. Let's try the previous - // nightlies in reverse chronological order until we find a nightly that does, - // starting at one date earlier than the current manifest's date. - let toolchain_date = toolchain.date.as_ref().unwrap_or(&fetched); - let try_next = date_from_manifest_date(toolchain_date) - .unwrap_or_else(|| panic!("Malformed manifest date: {toolchain_date:?}")) - .pred_opt() - .unwrap(); - - if try_next < last_manifest { - // Wouldn't be an update if we go further back than the user's current nightly. - if let Some(e) = first_err { - break Err(e); - } else { - // In this case, all newer nightlies are missing, which means there are no - // updates, so the user is already at the latest nightly. - break Ok(None); - } - } + Some(DistError::MissingReleaseForToolchain(..)) => { + // no need to even print anything for missing nightlies, + // since we don't really "skip" them + } + _ => { + // All other errors break the loop + break Err(e); + } + }; - toolchain.date = Some(try_next.format("%Y-%m-%d").to_string()); + if let Some(backtrack_limit) = backtrack_limit { + if backtrack_limit < 1 { + // This unwrap is safe because we can only hit this if we've + // had a chance to set first_err + break Err(first_err.unwrap()); } } + + // The user asked to update their nightly, but the latest nightly does not have all + // the components that the user currently has installed. Let's try the previous + // nightlies in reverse chronological order until we find a nightly that does, + // starting at one date earlier than the current manifest's date. + let toolchain_date = toolchain.date.as_ref().unwrap_or(&fetched); + let try_next = date_from_manifest_date(toolchain_date) + .unwrap_or_else(|| panic!("Malformed manifest date: {toolchain_date:?}")) + .pred_opt() + .unwrap(); + + if try_next < last_manifest { + // Wouldn't be an update if we go further back than the user's current nightly. + if let Some(e) = first_err { + break Err(e); + } else { + // In this case, all newer nightlies are missing, which means there are no + // updates, so the user is already at the latest nightly. + break Ok(None); + } + } + + toolchain.date = Some(try_next.format("%Y-%m-%d").to_string()); } } From 427de9f4f3d126b4d1da67f5366eff3dd77ac01e Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Tue, 18 Jun 2024 11:55:55 +0200 Subject: [PATCH 05/10] Inline wrapper function --- src/dist/mod.rs | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/src/dist/mod.rs b/src/dist/mod.rs index 6a7bc64eaa..dbeee90962 100644 --- a/src/dist/mod.rs +++ b/src/dist/mod.rs @@ -750,20 +750,6 @@ pub(crate) async fn update_from_dist( } } - let res = update_from_dist_(prefix, opts).await; - // 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); - } - - res -} - -async fn update_from_dist_( - prefix: &InstallPrefix, - opts: &DistOptions<'_>, -) -> Result> { let mut fetched = String::new(); let mut first_err = None; let backtrack = opts.desc.channel == "nightly" && opts.desc.date.is_none(); @@ -811,7 +797,7 @@ async fn update_from_dist_( }; let mut toolchain = opts.desc.clone(); - loop { + let res = loop { let result = try_update_from_dist_( opts.dl_cfg, opts.update_hash, @@ -829,8 +815,8 @@ async fn update_from_dist_( .await; let e = match result { - Ok(v) => return Ok(v), - Err(e) if !backtrack => return Err(e), + Ok(v) => break Ok(v), + Err(e) if !backtrack => break Err(e), Err(e) => e, }; @@ -892,7 +878,15 @@ async fn update_from_dist_( } toolchain.date = Some(try_next.format("%Y-%m-%d").to_string()); + }; + + // 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); } + + res } async fn try_update_from_dist_( From 4d6d8d870534a85a9cc650ca939616ee40a32bb5 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Tue, 18 Jun 2024 11:59:52 +0200 Subject: [PATCH 06/10] Remove indirection in update error handling --- src/dist/mod.rs | 61 ++++++++++++++++--------------------------------- 1 file changed, 20 insertions(+), 41 deletions(-) diff --git a/src/dist/mod.rs b/src/dist/mod.rs index dbeee90962..9e84047358 100644 --- a/src/dist/mod.rs +++ b/src/dist/mod.rs @@ -1001,24 +1001,14 @@ async fn try_update_from_dist_( }; } Ok(None) => return Ok(None), - Err(any) => { - enum Cases { - DNE, - CF, - Other, - } - let case = match any.downcast_ref::() { - Some(RustupError::ChecksumFailed { .. }) => Cases::CF, - Some(RustupError::DownloadNotExists { .. }) => Cases::DNE, - _ => Cases::Other, - }; - match case { - Cases::CF => return Ok(None), - Cases::DNE => { + Err(err) => { + match err.downcast_ref::() { + Some(RustupError::ChecksumFailed { .. }) => return Ok(None), + Some(RustupError::DownloadNotExists { .. }) => { // Proceed to try v1 as a fallback - (download.notify_handler)(Notification::DownloadingLegacyManifest); + (download.notify_handler)(Notification::DownloadingLegacyManifest) } - Cases::Other => return Err(any), + _ => return Err(err), } } } @@ -1026,35 +1016,24 @@ async fn try_update_from_dist_( // If the v2 manifest is not found then try v1 let manifest = match dl_v1_manifest(download, toolchain).await { Ok(m) => m, - Err(any) => { - enum Cases { - DNE, - CF, - Other, + Err(err) => match err.downcast_ref::() { + Some(RustupError::ChecksumFailed { .. }) => return Err(err), + Some(RustupError::DownloadNotExists { .. }) => { + bail!(DistError::MissingReleaseForToolchain( + toolchain.manifest_name() + )); } - let case = match any.downcast_ref::() { - Some(RustupError::ChecksumFailed { .. }) => Cases::CF, - Some(RustupError::DownloadNotExists { .. }) => Cases::DNE, - _ => Cases::Other, - }; - match case { - Cases::DNE => { - bail!(DistError::MissingReleaseForToolchain( + _ => { + return Err(err).with_context(|| { + format!( + "failed to download manifest for '{}'", toolchain.manifest_name() - )); - } - Cases::CF => return Err(any), - Cases::Other => { - return Err(any).with_context(|| { - format!( - "failed to download manifest for '{}'", - toolchain.manifest_name() - ) - }); - } + ) + }); } - } + }, }; + let result = manifestation .update_v1( &manifest, From 6262ae1d6f180036dbd742ff101368eb5fb8d0f4 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Tue, 18 Jun 2024 12:01:47 +0200 Subject: [PATCH 07/10] Remove intermediate state from error handling --- src/dist/mod.rs | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/dist/mod.rs b/src/dist/mod.rs index 9e84047358..1fe92acef1 100644 --- a/src/dist/mod.rs +++ b/src/dist/mod.rs @@ -1043,23 +1043,17 @@ async fn try_update_from_dist_( download.process, ) .await; + // inspect, determine what context to add, then process afterwards. - let mut download_not_exists = false; - match &result { - Ok(_) => (), - Err(e) => { - if let Some(RustupError::DownloadNotExists { .. }) = e.downcast_ref::() { - download_not_exists = true - } + if let Err(e) = &result { + if let Some(RustupError::DownloadNotExists { .. }) = e.downcast_ref::() { + return result.with_context(|| { + format!("could not download nonexistent rust version `{toolchain_str}`") + }); } } - if download_not_exists { - result.with_context(|| { - format!("could not download nonexistent rust version `{toolchain_str}`") - }) - } else { - result - } + + result } pub(crate) async fn dl_v2_manifest( From f3caa39645d5784dbc4ac7bfb511a867efe5c55a Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Tue, 18 Jun 2024 12:05:02 +0200 Subject: [PATCH 08/10] Rename desc fields to toolchain --- src/dist/mod.rs | 10 +++++----- src/install.rs | 12 ++++++++---- src/toolchain/distributable.rs | 10 +++++----- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/dist/mod.rs b/src/dist/mod.rs index 1fe92acef1..535313cabe 100644 --- a/src/dist/mod.rs +++ b/src/dist/mod.rs @@ -713,7 +713,7 @@ impl fmt::Display for Profile { #[derive(Clone)] pub(crate) struct DistOptions<'a> { pub(crate) cfg: &'a Cfg<'a>, - pub(crate) desc: &'a ToolchainDesc, + pub(crate) toolchain: &'a ToolchainDesc, pub(crate) profile: Profile, pub(crate) update_hash: Option<&'a Path>, pub(crate) dl_cfg: DownloadCfg<'a>, @@ -752,9 +752,9 @@ pub(crate) async fn update_from_dist( let mut fetched = String::new(); let mut first_err = None; - let backtrack = opts.desc.channel == "nightly" && opts.desc.date.is_none(); + let backtrack = opts.toolchain.channel == "nightly" && opts.toolchain.date.is_none(); // We want to limit backtracking if we do not already have a toolchain - let mut backtrack_limit: Option = if opts.desc.date.is_some() { + let mut backtrack_limit: Option = if opts.toolchain.date.is_some() { None } else { // We limit the backtracking to 21 days by default (half a release cycle). @@ -792,11 +792,11 @@ pub(crate) async fn update_from_dist( }; let current_manifest = { - let manifestation = Manifestation::open(prefix.clone(), opts.desc.target.clone())?; + let manifestation = Manifestation::open(prefix.clone(), opts.toolchain.target.clone())?; manifestation.load_manifest()? }; - let mut toolchain = opts.desc.clone(); + let mut toolchain = opts.toolchain.clone(); let res = loop { let result = try_update_from_dist_( opts.dl_cfg, diff --git a/src/install.rs b/src/install.rs index 37c714bfbb..f968b4da37 100644 --- a/src/install.rs +++ b/src/install.rs @@ -132,7 +132,9 @@ impl<'a> InstallMethod<'a> { match self { InstallMethod::Copy { dest, .. } => (*dest).into(), InstallMethod::Link { dest, .. } => (*dest).into(), - InstallMethod::Dist(DistOptions { desc, .. }) => (*desc).into(), + InstallMethod::Dist(DistOptions { + toolchain: desc, .. + }) => (*desc).into(), } } @@ -144,9 +146,11 @@ impl<'a> InstallMethod<'a> { match self { InstallMethod::Copy { cfg, dest, .. } => cfg.toolchain_path(&(*dest).into()), InstallMethod::Link { cfg, dest, .. } => cfg.toolchain_path(&(*dest).into()), - InstallMethod::Dist(DistOptions { cfg, desc, .. }) => { - cfg.toolchain_path(&(*desc).into()) - } + InstallMethod::Dist(DistOptions { + cfg, + toolchain: desc, + .. + }) => cfg.toolchain_path(&(*desc).into()), } } } diff --git a/src/toolchain/distributable.rs b/src/toolchain/distributable.rs index 3463305e81..618bb67b71 100644 --- a/src/toolchain/distributable.rs +++ b/src/toolchain/distributable.rs @@ -327,18 +327,18 @@ impl<'a> DistributableToolchain<'a> { #[cfg_attr(feature = "otel", tracing::instrument(err, skip_all))] pub(crate) async fn install( cfg: &'a Cfg<'a>, - desc: &ToolchainDesc, + toolchain: &ToolchainDesc, components: &[&str], targets: &[&str], profile: Profile, force: bool, ) -> anyhow::Result<(UpdateStatus, DistributableToolchain<'a>)> { - let hash_path = cfg.get_hash_file(desc, true)?; + let hash_path = cfg.get_hash_file(toolchain, true)?; let update_hash = Some(&hash_path as &Path); let status = InstallMethod::Dist(DistOptions { cfg, - desc, + toolchain, profile, update_hash, dl_cfg: cfg.download_cfg(&|n| (cfg.notify_handler)(n.into())), @@ -351,7 +351,7 @@ impl<'a> DistributableToolchain<'a> { }) .install() .await?; - Ok((status, Self::new(cfg, desc.clone())?)) + Ok((status, Self::new(cfg, toolchain.clone())?)) } #[cfg_attr(feature = "otel", tracing::instrument(err, skip_all))] @@ -414,7 +414,7 @@ impl<'a> DistributableToolchain<'a> { InstallMethod::Dist(DistOptions { cfg, - desc: &self.desc, + toolchain: &self.desc, profile, update_hash, dl_cfg: cfg.download_cfg(&|n| (cfg.notify_handler)(n.into())), From 73e550838e78b9399121c92d68769adca65436e6 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Tue, 18 Jun 2024 12:08:45 +0200 Subject: [PATCH 09/10] Use local suppression for clippy::too_many_arguments --- src/dist/mod.rs | 1 + src/lib.rs | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dist/mod.rs b/src/dist/mod.rs index 535313cabe..2969f70066 100644 --- a/src/dist/mod.rs +++ b/src/dist/mod.rs @@ -889,6 +889,7 @@ pub(crate) async fn update_from_dist( res } +#[allow(clippy::too_many_arguments)] async fn try_update_from_dist_( download: DownloadCfg<'_>, update_hash: Option<&Path>, diff --git a/src/lib.rs b/src/lib.rs index c28048cc21..de034e5758 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,6 +1,5 @@ #![deny(rust_2018_idioms)] #![allow( - clippy::too_many_arguments, clippy::type_complexity, clippy::upper_case_acronyms, // see https://github.com/rust-lang/rust-clippy/issues/6974 clippy::vec_init_then_push, // uses two different styles of initialization From 0453e1fe46e72f791dbb3c4a2656a86348f0cf85 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Tue, 18 Jun 2024 12:12:21 +0200 Subject: [PATCH 10/10] Remove unnecessary lint suppressions --- src/lib.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index de034e5758..ad507a930c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,9 +1,6 @@ #![deny(rust_2018_idioms)] #![allow( clippy::type_complexity, - clippy::upper_case_acronyms, // see https://github.com/rust-lang/rust-clippy/issues/6974 - clippy::vec_init_then_push, // uses two different styles of initialization - clippy::box_default, // its ugly and outside of inner loops irrelevant clippy::result_large_err, // 288 bytes is our 'large' variant today, which is unlikely to be a performance problem clippy::arc_with_non_send_sync, // will get resolved as we move further into async )]