From 6ab0843ec378aa400e7bc09220295a5a714b6b11 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Fri, 5 Jan 2018 15:55:23 +1300 Subject: [PATCH 1/3] Catch some missing components that weren't warned on There is a lot of refactoring here too. There was a check to ensure that if there are components that we plan to install (or update) that are missing from the manifest, then we error out. However, this only worked for components that had an entry in the manifest under the Rust package, but were missing components of their own. For tools like the RLS, when they are missing, they are completely missing from the manifest. They therefore didn't get picked up by the check. This patch adds an extra Vec to track such components and then warns about them. --- src/rustup-dist/src/manifestation.rs | 348 ++++++++++++++++----------- 1 file changed, 202 insertions(+), 146 deletions(-) diff --git a/src/rustup-dist/src/manifestation.rs b/src/rustup-dist/src/manifestation.rs index d48e13e299..44c21ec5fe 100644 --- a/src/rustup-dist/src/manifestation.rs +++ b/src/rustup-dist/src/manifestation.rs @@ -40,6 +40,22 @@ impl Changes { remove_extensions: Vec::new(), } } + + fn check_invariants(&self, rust_target_package: &TargetedPackage, config: &Option) { + for component_to_add in &self.add_extensions { + assert!(rust_target_package.extensions.contains(component_to_add), + "package must contain extension to add"); + assert!(!self.remove_extensions.contains(component_to_add), + "can't both add and remove extensions"); + } + for component_to_remove in &self.remove_extensions { + assert!(rust_target_package.extensions.contains(component_to_remove), + "package must contain extension to remove"); + let config = config.as_ref().expect("removing extension on fresh install?"); + assert!(config.components.contains(component_to_remove), + "removing package that isn't installed"); + } + } } #[derive(PartialEq, Debug)] @@ -86,78 +102,31 @@ impl Manifestation { let prefix = self.installation.prefix(); let ref rel_installed_manifest_path = prefix.rel_manifest_file(DIST_MANIFEST); let ref installed_manifest_path = prefix.path().join(rel_installed_manifest_path); - let rust_package = try!(new_manifest.get_package("rust")); - let rust_target_package = try!(rust_package.get_target(Some(&self.target_triple))); - - // Load the previous dist manifest - let ref old_manifest = try!(self.load_manifest()); - - // Load the configuration and list of installed components. - let ref config = try!(self.read_config()); // Create the lists of components needed for installation - let component_lists = try!(build_update_component_lists(new_manifest, old_manifest, config, - changes, &rust_target_package, - notify_handler)); - let (components_to_uninstall, - components_to_install, - final_component_list) = component_lists; - - if components_to_uninstall.is_empty() && components_to_install.is_empty() { + let update = try!(Update::build_update( + self, + new_manifest, + changes, + notify_handler, + )); + + if update.nothing_changes() { return Ok(UpdateStatus::Unchanged); } // Make sure we don't accidentally uninstall the essential components! (see #1297) - let missing_essential_components = ["rustc", "cargo"] - .iter() - .filter_map(|pkg| if final_component_list.iter().any(|c| &c.pkg == pkg) { - None - } else { - Some(Component { - pkg: pkg.to_string(), - target: Some(self.target_triple.clone()), - }) - }) - .collect::>(); - if !missing_essential_components.is_empty() { - return Err(ErrorKind::RequestedComponentsUnavailable(missing_essential_components).into()); - } + update.missing_essential_components(&self.target_triple)?; // Validate that the requested components are available - let unavailable_components: Vec = components_to_install.iter().filter(|c| { - use manifest::*; - let pkg: Option<&Package> = new_manifest.get_package(&c.pkg).ok(); - let target_pkg: Option<&TargetedPackage> = pkg.and_then(|p| p.get_target(c.target.as_ref()).ok()); - target_pkg.map(|tp| tp.available()) != Some(true) - }).cloned().collect(); - - if !unavailable_components.is_empty() { - return Err(ErrorKind::RequestedComponentsUnavailable(unavailable_components).into()); - } - - // Map components to urls and hashes - let mut components_urls_and_hashes: Vec<(Component, Format, String, String)> = Vec::new(); - for component in components_to_install { - let package = try!(new_manifest.get_package(&component.pkg)); - let target_package = try!(package.get_target(component.target.as_ref())); - - let bins = target_package.bins.as_ref().expect("components available"); - let c_u_h = - if let (Some(url), Some(hash)) = (bins.xz_url.clone(), - bins.xz_hash.clone()) { - (component, Format::Xz, url, hash) - } else { - (component, Format::Gz, bins.url.clone(), bins.hash.clone()) - }; - components_urls_and_hashes.push(c_u_h); - } + update.unavailable_components(new_manifest)?; let altered = temp_cfg.dist_server != DEFAULT_DIST_SERVER; // Download component packages and validate hashes let mut things_to_install: Vec<(Component, Format, File)> = Vec::new(); let mut things_downloaded: Vec = Vec::new(); - for (component, format, url, hash) in components_urls_and_hashes { + for (component, format, url, hash) in update.components_urls_and_hashes(new_manifest)? { notify_handler(Notification::DownloadingComponent(&component.pkg, &self.target_triple, @@ -183,10 +152,11 @@ impl Manifestation { // If the previous installation was from a v1 manifest we need // to uninstall it first. + let ref config = try!(self.read_config()); tx = try!(self.maybe_handle_v2_upgrade(config, tx)); // Uninstall components - for component in components_to_uninstall { + for component in update.components_to_uninstall { notify_handler(Notification::RemovingComponent(&component.pkg, &self.target_triple, @@ -245,7 +215,7 @@ impl Manifestation { // `Components` *also* tracks what is installed, but it only tracks names, not // name/target. Needs to be fixed in rust-installer. let mut config = Config::new(); - config.components = final_component_list; + config.components = update.final_component_list; let ref config_str = config.stringify(); let ref rel_config_path = prefix.rel_manifest_file(CONFIG_FILE); let ref config_path = prefix.path().join(rel_config_path); @@ -410,108 +380,194 @@ impl Manifestation { } } -/// Returns components to uninstall, install, and the list of all -/// components that will be up to date after the update. -fn build_update_component_lists( - new_manifest: &Manifest, - old_manifest: &Option, - config: &Option, - changes: Changes, - rust_target_package: &TargetedPackage, - notify_handler: &Fn(Notification), - ) -> Result<(Vec, Vec, Vec)> { - - // Check some invariantns - for component_to_add in &changes.add_extensions { - assert!(rust_target_package.extensions.contains(component_to_add), - "package must contain extension to add"); - assert!(!changes.remove_extensions.contains(component_to_add), - "can't both add and remove extensions"); - } - for component_to_remove in &changes.remove_extensions { - assert!(rust_target_package.extensions.contains(component_to_remove), - "package must contain extension to remove"); - let config = config.as_ref().expect("removing extension on fresh install?"); - assert!(config.components.contains(component_to_remove), - "removing package that isn't installed"); - } +struct Update { + components_to_uninstall: Vec, + components_to_install: Vec, + final_component_list: Vec, + missing_components: Vec, +} - // The list of components already installed, empty if a new install - let starting_list = config.as_ref().map(|c| c.components.clone()).unwrap_or(Vec::new()); +impl Update { + /// Returns components to uninstall, install, and the list of all + /// components that will be up to date after the update. + fn build_update( + manifestation: &Manifestation, + new_manifest: &Manifest, + changes: Changes, + notify_handler: &Fn(Notification), + ) -> Result { + // Load the configuration and list of installed components. + let config = try!(manifestation.read_config()); - // The list of components we'll have installed at the end - let mut final_component_list = Vec::new(); + // The package to install. + let rust_package = try!(new_manifest.get_package("rust")); + let rust_target_package = try!(rust_package.get_target(Some(&manifestation.target_triple))); - // Find the final list of components we want to be left with when - // we're done: required components, added extensions, and existing - // installed extensions. + changes.check_invariants(rust_target_package, &config); - // Add components required by the package, according to the - // manifest - for required_component in &rust_target_package.components { - final_component_list.push(required_component.clone()); - } + // The list of components already installed, empty if a new install + let starting_list = config.as_ref().map(|c| c.components.clone()).unwrap_or(Vec::new()); - // Add requested extension components - for extension in &changes.add_extensions { - final_component_list.push(extension.clone()); - } + let mut result = Update { + components_to_uninstall: vec![], + components_to_install: vec![], + final_component_list: vec![], + missing_components: vec![], + }; - // Add extensions that are already installed - for existing_component in &starting_list { - let is_removed = changes.remove_extensions.contains(existing_component); - - if !is_removed { - // If there is a rename in the (new) manifest, then we uninstall the component with the - // old name and install a component with the new name - if new_manifest.renames.contains_key(&existing_component.pkg) { - let mut renamed_component = existing_component.clone(); - renamed_component.pkg = new_manifest.renames[&existing_component.pkg].to_owned(); - let is_already_included = final_component_list.contains(&renamed_component); - if !is_already_included { - final_component_list.push(renamed_component); + // Find the final list of components we want to be left with when + // we're done: required components, added extensions, and existing + // installed extensions. + result.build_final_component_list( + &starting_list, + rust_target_package, + new_manifest, + &changes, + ); + + // If this is a full upgrade then the list of components to + // uninstall is all that are currently installed, and those + // to install the final list. It's a complete reinstall. + // + // If it's a modification then the components to uninstall are + // those that are currently installed but not in the final list. + // To install are those on the final list but not already + // installed. + let old_manifest = try!(manifestation.load_manifest()); + let just_modifying_existing_install = old_manifest.as_ref() == Some(new_manifest); + + if just_modifying_existing_install { + for existing_component in &starting_list { + if !result.final_component_list.contains(existing_component) { + result.components_to_uninstall.push(existing_component.clone()) } - } else { - let is_extension = rust_target_package.extensions.contains(existing_component); - let is_already_included = final_component_list.contains(existing_component); - if is_extension && !is_already_included { - final_component_list.push(existing_component.clone()); + } + for component in &result.final_component_list { + if !starting_list.contains(component) { + result.components_to_install.push(component.clone()); + } else { + if changes.add_extensions.contains(&component) { + notify_handler(Notification::ComponentAlreadyInstalled(&component)); + } } } + } else { + result.components_to_uninstall = starting_list.clone(); + result.components_to_install = result.final_component_list.clone(); } + + Ok(result) } - let mut components_to_uninstall = Vec::new(); - let mut components_to_install = Vec::new(); - - // If this is a full upgrade then the list of components to - // uninstall is all that are currently installed, and those - // to install the final list. It's a complete reinstall. - // - // If it's a modification then the components to uninstall are - // those that are currently installed but not in the final list. - // To install are those on the final list but not already - // installed. - let just_modifying_existing_install = old_manifest.as_ref() == Some(new_manifest); - if !just_modifying_existing_install { - components_to_uninstall = starting_list.clone(); - components_to_install = final_component_list.clone(); - } else { - for existing_component in &starting_list { - if !final_component_list.contains(existing_component) { - components_to_uninstall.push(existing_component.clone()) - } + /// Build the list of components we'll have installed at the end + fn build_final_component_list( + &mut self, + starting_list: &[Component], + rust_target_package: &TargetedPackage, + new_manifest: &Manifest, + changes: &Changes + ) { + // Add components required by the package, according to the + // manifest + for required_component in &rust_target_package.components { + self.final_component_list.push(required_component.clone()); } - for component in &final_component_list { - if !starting_list.contains(component) { - components_to_install.push(component.clone()); - } else { - if changes.add_extensions.contains(&component) { - notify_handler(Notification::ComponentAlreadyInstalled(&component)); + + // Add requested extension components + for extension in &changes.add_extensions { + self.final_component_list.push(extension.clone()); + } + + // Add extensions that are already installed + for existing_component in starting_list { + let is_removed = changes.remove_extensions.contains(existing_component); + + if !is_removed { + // If there is a rename in the (new) manifest, then we uninstall the component with the + // old name and install a component with the new name + if new_manifest.renames.contains_key(&existing_component.pkg) { + let mut renamed_component = existing_component.clone(); + renamed_component.pkg = new_manifest.renames[&existing_component.pkg].to_owned(); + let is_already_included = self.final_component_list.contains(&renamed_component); + if !is_already_included { + self.final_component_list.push(renamed_component); + } + } else { + let is_already_included = self.final_component_list.contains(existing_component); + if !is_already_included { + let component_is_present = rust_target_package.extensions.contains(existing_component) + || rust_target_package.components.contains(existing_component); + + if component_is_present { + self.final_component_list.push(existing_component.clone()); + } else { + self.missing_components.push(existing_component.clone()); + } + } } } } } - Ok((components_to_uninstall, components_to_install, final_component_list)) + fn nothing_changes(&self) -> bool { + self.components_to_uninstall.is_empty() && self.components_to_install.is_empty() + } + + fn missing_essential_components(&self, target_triple: &TargetTriple) -> Result<()> { + let missing_essential_components = ["rustc", "cargo"] + .iter() + .filter_map(|pkg| if self.final_component_list.iter().any(|c| &c.pkg == pkg) { + None + } else { + Some(Component { + pkg: pkg.to_string(), + target: Some(target_triple.clone()), + }) + }) + .collect::>(); + + if !missing_essential_components.is_empty() { + return Err(ErrorKind::RequestedComponentsUnavailable(missing_essential_components).into()); + } + + Ok(()) + } + + fn unavailable_components(&self, new_manifest: &Manifest) -> Result<()> { + let mut unavailable_components: Vec = self.components_to_install.iter().filter(|c| { + use manifest::*; + let pkg: Option<&Package> = new_manifest.get_package(&c.pkg).ok(); + let target_pkg: Option<&TargetedPackage> = pkg.and_then(|p| p.get_target(c.target.as_ref()).ok()); + target_pkg.map(|tp| tp.available()) != Some(true) + }).cloned().collect(); + + unavailable_components.extend_from_slice(&self.missing_components); + + if !unavailable_components.is_empty() { + return Err(ErrorKind::RequestedComponentsUnavailable(unavailable_components).into()); + } + + Ok(()) + } + + /// Map components to urls and hashes + fn components_urls_and_hashes(&self, new_manifest: &Manifest) -> Result> { + let mut components_urls_and_hashes = Vec::new(); + for component in &self.components_to_install { + let package = try!(new_manifest.get_package(&component.pkg)); + let target_package = try!(package.get_target(component.target.as_ref())); + + let bins = target_package.bins.as_ref().expect("components available"); + let c_u_h = + if let (Some(url), Some(hash)) = (bins.xz_url.clone(), + bins.xz_hash.clone()) { + (component.clone(), Format::Xz, url, hash) + } else { + (component.clone(), Format::Gz, bins.url.clone(), bins.hash.clone()) + }; + components_urls_and_hashes.push(c_u_h); + } + + Ok(components_urls_and_hashes) + } } From 554488024f176cb588fa94b6722117b19f0059f2 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Thu, 11 Jan 2018 16:25:59 +1300 Subject: [PATCH 2/3] Support `--force` for `rustup update` --- src/rustup-cli/common.rs | 4 ++-- src/rustup-cli/rustup_mode.rs | 14 +++++++++++--- src/rustup-cli/self_update.rs | 2 +- src/rustup-dist/src/dist.rs | 20 ++++++++++++-------- src/rustup-dist/src/manifestation.rs | 19 +++++++++++++------ src/rustup-dist/tests/dist.rs | 8 +++++++- src/rustup/config.rs | 10 ++++------ src/rustup/install.rs | 14 +++++++++----- src/rustup/toolchain.rs | 24 ++++++++++++++---------- 9 files changed, 73 insertions(+), 42 deletions(-) diff --git a/src/rustup-cli/common.rs b/src/rustup-cli/common.rs index 9fc5d62857..2b12906452 100644 --- a/src/rustup-cli/common.rs +++ b/src/rustup-cli/common.rs @@ -189,9 +189,9 @@ fn show_channel_updates(cfg: &Cfg, toolchains: Vec<(String, rustup::Result Result<()> { +pub fn update_all_channels(cfg: &Cfg, self_update: bool, force_update: bool) -> Result<()> { - let toolchains = try!(cfg.update_all_channels()); + let toolchains = try!(cfg.update_all_channels(force_update)); if toolchains.is_empty() { info!("no updatable toolchains installed"); diff --git a/src/rustup-cli/rustup_mode.rs b/src/rustup-cli/rustup_mode.rs index 21524e1b5a..05c932772c 100644 --- a/src/rustup-cli/rustup_mode.rs +++ b/src/rustup-cli/rustup_mode.rs @@ -144,7 +144,11 @@ pub fn cli() -> App<'static, 'static> { .help("Don't perform self update when running the `rustup` command") .long("no-self-update") .takes_value(false) - .hidden(true))) + .hidden(true)) + .arg(Arg::with_name("force") + .help("Force an update, even if some components are missing") + .long("force") + .takes_value(false))) .subcommand(SubCommand::with_name("default") .about("Set the default toolchain") .after_help(DEFAULT_HELP) @@ -462,7 +466,7 @@ fn update(cfg: &Cfg, m: &ArgMatches) -> Result<()> { let toolchain = try!(cfg.get_toolchain(name, false)); let status = if !toolchain.is_custom() { - Some(try!(toolchain.install_from_dist())) + Some(try!(toolchain.install_from_dist(m.is_present("force")))) } else if !toolchain.exists() { return Err(ErrorKind::ToolchainNotInstalled(toolchain.name().to_string()).into()); } else { @@ -475,7 +479,11 @@ fn update(cfg: &Cfg, m: &ArgMatches) -> Result<()> { } } } else { - try!(common::update_all_channels(cfg, !m.is_present("no-self-update") && !self_update::NEVER_SELF_UPDATE)); + try!(common::update_all_channels( + cfg, + !m.is_present("no-self-update") && !self_update::NEVER_SELF_UPDATE, + m.is_present("force"), + )); } Ok(()) diff --git a/src/rustup-cli/self_update.rs b/src/rustup-cli/self_update.rs index e35bcc2fdd..6721182fa4 100644 --- a/src/rustup-cli/self_update.rs +++ b/src/rustup-cli/self_update.rs @@ -747,7 +747,7 @@ fn maybe_install_rust(toolchain_str: &str, default_host_triple: &str, verbose: b // Set host triple first as it will affect resolution of toolchain_str try!(cfg.set_default_host_triple(default_host_triple)); let toolchain = try!(cfg.get_toolchain(toolchain_str, false)); - let status = try!(toolchain.install_from_dist()); + let status = try!(toolchain.install_from_dist(false)); try!(cfg.set_default(toolchain_str)); println!(""); try!(common::show_channel_update(cfg, toolchain_str, Ok(status))); diff --git a/src/rustup-dist/src/dist.rs b/src/rustup-dist/src/dist.rs index 20ded6d63f..cfa0d13bd3 100644 --- a/src/rustup-dist/src/dist.rs +++ b/src/rustup-dist/src/dist.rs @@ -462,7 +462,8 @@ pub fn update_from_dist<'a>(download: DownloadCfg<'a>, toolchain: &ToolchainDesc, prefix: &InstallPrefix, add: &[Component], - remove: &[Component]) + remove: &[Component], + force_update: bool) -> Result> { let fresh_install = !prefix.path().exists(); @@ -472,7 +473,8 @@ pub fn update_from_dist<'a>(download: DownloadCfg<'a>, toolchain, prefix, add, - remove); + remove, + force_update); // Don't leave behind an empty / broken installation directory if res.is_err() && fresh_install { @@ -485,12 +487,13 @@ pub fn update_from_dist<'a>(download: DownloadCfg<'a>, } pub fn update_from_dist_<'a>(download: DownloadCfg<'a>, - update_hash: Option<&Path>, - toolchain: &ToolchainDesc, - prefix: &InstallPrefix, - add: &[Component], - remove: &[Component]) - -> Result> { + update_hash: Option<&Path>, + toolchain: &ToolchainDesc, + prefix: &InstallPrefix, + add: &[Component], + remove: &[Component], + force_update: bool) + -> Result> { let toolchain_str = toolchain.to_string(); let manifestation = try!(Manifestation::open(prefix.clone(), toolchain.target.clone())); @@ -507,6 +510,7 @@ pub fn update_from_dist_<'a>(download: DownloadCfg<'a>, (download.notify_handler)(Notification::DownloadedManifest(&m.date, m.get_rust_version().ok())); return match try!(manifestation.update(&m, changes, + force_update, &download, download.notify_handler.clone())) { UpdateStatus::Unchanged => Ok(None), diff --git a/src/rustup-dist/src/manifestation.rs b/src/rustup-dist/src/manifestation.rs index 44c21ec5fe..0765cf0af1 100644 --- a/src/rustup-dist/src/manifestation.rs +++ b/src/rustup-dist/src/manifestation.rs @@ -91,11 +91,14 @@ impl Manifestation { /// distribution manifest to "rustlib/rustup-dist.toml" and a /// configuration containing the component name-target pairs to /// "rustlib/rustup-config.toml". - pub fn update(&self, - new_manifest: &Manifest, - changes: Changes, - download_cfg: &DownloadCfg, - notify_handler: &Fn(Notification)) -> Result { + pub fn update( + &self, + new_manifest: &Manifest, + changes: Changes, + force_update: bool, + download_cfg: &DownloadCfg, + notify_handler: &Fn(Notification), + ) -> Result { // Some vars we're going to need a few times let temp_cfg = download_cfg.temp_cfg; @@ -119,7 +122,11 @@ impl Manifestation { update.missing_essential_components(&self.target_triple)?; // Validate that the requested components are available - update.unavailable_components(new_manifest)?; + match update.unavailable_components(new_manifest) { + Ok(_) => {}, + _ if force_update => {}, + Err(e) => return Err(e), + } let altered = temp_cfg.dist_server != DEFAULT_DIST_SERVER; diff --git a/src/rustup-dist/tests/dist.rs b/src/rustup-dist/tests/dist.rs index c66ad1dd52..61a4893390 100644 --- a/src/rustup-dist/tests/dist.rs +++ b/src/rustup-dist/tests/dist.rs @@ -409,7 +409,13 @@ fn update_from_dist(dist_server: &Url, remove_extensions: remove.to_owned(), }; - manifestation.update(&manifest, changes, download_cfg, download_cfg.notify_handler.clone()) + manifestation.update( + &manifest, + changes, + false, + download_cfg, + download_cfg.notify_handler.clone(), + ) } fn make_manifest_url(dist_server: &Url, toolchain: &ToolchainDesc) -> Result { diff --git a/src/rustup/config.rs b/src/rustup/config.rs index e915dacbc9..1bd9f4265e 100644 --- a/src/rustup/config.rs +++ b/src/rustup/config.rs @@ -269,7 +269,7 @@ impl Cfg { Err(Error::from(reason_err)) .chain_err(|| ErrorKind::OverrideToolchainNotInstalled(name.to_string())) } else { - try!(toolchain.install_from_dist()); + try!(toolchain.install_from_dist(false)); Ok(Some((toolchain, reason))) } } @@ -336,8 +336,6 @@ impl Cfg { }) } - - pub fn list_toolchains(&self) -> Result> { if utils::is_directory(&self.toolchains_dir) { let mut toolchains: Vec<_> = try!(utils::read_dir("toolchains", &self.toolchains_dir)) @@ -354,7 +352,7 @@ impl Cfg { } } - pub fn update_all_channels(&self) -> Result)>> { + pub fn update_all_channels(&self, force_update: bool) -> Result)>> { let toolchains = try!(self.list_toolchains()); // Convert the toolchain strings to Toolchain values @@ -369,7 +367,7 @@ impl Cfg { // Update toolchains and collect the results let toolchains = toolchains.map(|(n, t)| { let t = t.and_then(|t| { - let t = t.install_from_dist(); + let t = t.install_from_dist(force_update); if let Err(ref e) = t { (self.notify_handler)(Notification::NonFatalError(e)); } @@ -414,7 +412,7 @@ impl Cfg { binary: &str) -> Result { let ref toolchain = try!(self.get_toolchain(toolchain, false)); if install_if_missing && !toolchain.exists() { - try!(toolchain.install_from_dist()); + try!(toolchain.install_from_dist(false)); } if let Some(cmd) = try!(self.maybe_do_cargo_fallback(toolchain, binary)) { diff --git a/src/rustup/install.rs b/src/rustup/install.rs index 49c5814c41..9d6270e505 100644 --- a/src/rustup/install.rs +++ b/src/rustup/install.rs @@ -16,7 +16,8 @@ pub enum InstallMethod<'a> { Copy(&'a Path), Link(&'a Path), Installer(&'a Path, &'a temp::Cfg), - Dist(&'a dist::ToolchainDesc, Option<&'a Path>, DownloadCfg<'a>), + // bool is whether to force an update + Dist(&'a dist::ToolchainDesc, Option<&'a Path>, DownloadCfg<'a>, bool), } impl<'a> InstallMethod<'a> { @@ -24,8 +25,8 @@ impl<'a> InstallMethod<'a> { if path.exists() { // Don't uninstall first for Dist method match self { - InstallMethod::Dist(_, _, _) | - InstallMethod::Installer(_, _) => {} + InstallMethod::Dist(..) | + InstallMethod::Installer(..) => {} _ => { try!(uninstall(path, notify_handler)); } @@ -45,7 +46,7 @@ impl<'a> InstallMethod<'a> { try!(InstallMethod::tar_gz(src, path, &temp_cfg, notify_handler)); Ok(true) } - InstallMethod::Dist(toolchain, update_hash, dl_cfg) => { + InstallMethod::Dist(toolchain, update_hash, dl_cfg, force_update) => { let prefix = &InstallPrefix::from(path.to_owned()); let maybe_new_hash = try!(dist::update_from_dist( @@ -53,7 +54,10 @@ impl<'a> InstallMethod<'a> { update_hash, toolchain, prefix, - &[], &[])); + &[], + &[], + force_update, + )); if let Some(hash) = maybe_new_hash { if let Some(hash_file) = update_hash { diff --git a/src/rustup/toolchain.rs b/src/rustup/toolchain.rs index 38edd8d4f2..898b645461 100644 --- a/src/rustup/toolchain.rs +++ b/src/rustup/toolchain.rs @@ -145,8 +145,8 @@ impl<'a> Toolchain<'a> { match install_method { InstallMethod::Copy(_) | InstallMethod::Link(_) | - InstallMethod::Installer(_, _) => self.is_custom(), - InstallMethod::Dist(_, _, _) => !self.is_custom(), + InstallMethod::Installer(..) => self.is_custom(), + InstallMethod::Dist(..) => !self.is_custom(), } } fn update_hash(&self) -> Result> { @@ -166,22 +166,23 @@ impl<'a> Toolchain<'a> { } } - pub fn install_from_dist(&self) -> Result { + pub fn install_from_dist(&self, force_update: bool) -> Result { if try!(self.cfg.telemetry_enabled()) { - return self.install_from_dist_with_telemetry(); + return self.install_from_dist_with_telemetry(force_update); } - self.install_from_dist_inner() + self.install_from_dist_inner(force_update) } - pub fn install_from_dist_inner(&self) -> Result { + pub fn install_from_dist_inner(&self, force_update: bool) -> Result { let update_hash = try!(self.update_hash()); self.install(InstallMethod::Dist(&try!(self.desc()), update_hash.as_ref().map(|p| &**p), - self.download_cfg())) + self.download_cfg(), + force_update)) } - pub fn install_from_dist_with_telemetry(&self) -> Result { - let result = self.install_from_dist_inner(); + pub fn install_from_dist_with_telemetry(&self, force_update: bool) -> Result { + let result = self.install_from_dist_inner(force_update); match result { Ok(us) => { @@ -210,7 +211,8 @@ impl<'a> Toolchain<'a> { let update_hash = try!(self.update_hash()); self.install_if_not_installed(InstallMethod::Dist(&try!(self.desc()), update_hash.as_ref().map(|p| &**p), - self.download_cfg())) + self.download_cfg(), + false)) } pub fn is_custom(&self) -> bool { ToolchainDesc::from_str(&self.name).is_err() @@ -603,6 +605,7 @@ impl<'a> Toolchain<'a> { try!(manifestation.update(&manifest, changes, + false, &self.download_cfg(), self.download_cfg().notify_handler.clone())); @@ -652,6 +655,7 @@ impl<'a> Toolchain<'a> { try!(manifestation.update(&manifest, changes, + false, &self.download_cfg(), self.download_cfg().notify_handler.clone())); From dcf0f9d001123773aadf5e85141235ffc4dc2cb0 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Thu, 11 Jan 2018 16:59:32 +1300 Subject: [PATCH 3/3] tests This adds a new test and removes test `update_removes_components_that_dont_exist` Historically, this test was added before the check that aborts update on missing components (which is tested by the new test in the previous commit, as well as elsewhere), so I think this test was only testing a bug. Note that the new behaviour is the same as the ancient behviour when the user uses `--force` --- src/rustup-dist/tests/dist.rs | 39 +++++++++++++++++++++++++++++++---- tests/cli-rustup.rs | 2 +- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/src/rustup-dist/tests/dist.rs b/src/rustup-dist/tests/dist.rs index 61a4893390..6933f94fe5 100644 --- a/src/rustup-dist/tests/dist.rs +++ b/src/rustup-dist/tests/dist.rs @@ -393,6 +393,27 @@ fn update_from_dist(dist_server: &Url, download_cfg: &DownloadCfg, temp_cfg: &temp::Cfg) -> Result { + update_from_dist_( + dist_server, + toolchain, + prefix, + add, + remove, + download_cfg, + temp_cfg, + false, + ) +} + +fn update_from_dist_(dist_server: &Url, + toolchain: &ToolchainDesc, + prefix: &InstallPrefix, + add: &[Component], + remove: &[Component], + download_cfg: &DownloadCfg, + temp_cfg: &temp::Cfg, + force_update: bool) -> Result { + // Download the dist manifest and place it into the installation prefix let ref manifest_url = try!(make_manifest_url(dist_server, toolchain)); let manifest_file = try!(temp_cfg.new_file()); @@ -412,7 +433,7 @@ fn update_from_dist(dist_server: &Url, manifestation.update( &manifest, changes, - false, + force_update, download_cfg, download_cfg.notify_handler.clone(), ) @@ -520,8 +541,8 @@ fn upgrade() { } #[test] -fn update_removes_components_that_dont_exist() { - // On day 1 install the 'bonus' component, on day 2 its no londer a component +fn force_update() { + // On day 1 install the 'bonus' component, on day 2 its no longer a component let edit = &|date: &str, pkg: &mut MockPackage| { if date == "2016-02-01" { let mut tpkg = pkg.targets.iter_mut().find(|p| p.target == "x86_64-apple-darwin").unwrap(); @@ -531,12 +552,22 @@ fn update_removes_components_that_dont_exist() { }); } }; + setup(Some(edit), false, &|url, toolchain, prefix, download_cfg, temp_cfg| { change_channel_date(url, "nightly", "2016-02-01"); + // Update with bonus. update_from_dist(url, toolchain, prefix, &[], &[], download_cfg, temp_cfg).unwrap(); assert!(utils::path_exists(&prefix.path().join("bin/bonus"))); change_channel_date(url, "nightly", "2016-02-02"); - update_from_dist(url, toolchain, prefix, &[], &[], download_cfg, temp_cfg).unwrap(); + + // Update without bonus, should fail. + let err = update_from_dist(url, toolchain, prefix, &[], &[], download_cfg, temp_cfg).unwrap_err(); + match *err.kind() { + ErrorKind::RequestedComponentsUnavailable(..) => {}, + _ => panic!() + } + // Force update without bonus, should succeed, but bonus binary will be missing. + update_from_dist_(url, toolchain, prefix, &[], &[], download_cfg, temp_cfg, true).unwrap(); assert!(!utils::path_exists(&prefix.path().join("bin/bonus"))); }); } diff --git a/tests/cli-rustup.rs b/tests/cli-rustup.rs index ad88e4bb38..58d18fa760 100644 --- a/tests/cli-rustup.rs +++ b/tests/cli-rustup.rs @@ -752,7 +752,7 @@ fn toolchain_update_is_like_update_except_that_bare_install_is_an_error() { fn proxy_toolchain_shorthand() { setup(&|config| { expect_ok(config, &["rustup", "default", "stable"]); - expect_ok(config, &["rustup", "toolchain", "update" , "nightly"]); + expect_ok(config, &["rustup", "toolchain", "update", "nightly"]); expect_stdout_ok(config, &["rustc", "--version"], "hash-s-2"); expect_stdout_ok(config, &["rustc", "+stable", "--version"], "hash-s-2"); expect_stdout_ok(config, &["rustc", "+nightly", "--version"], "hash-n-2");