From 9f9c667629013118d00afa9d0c18174173037b00 Mon Sep 17 00:00:00 2001 From: Nick Cameron <nrc@ncameron.org> Date: Tue, 20 Nov 2018 15:00:20 +1300 Subject: [PATCH 1/8] Canonicalise component names before adding or removing --- src/rustup-dist/src/manifestation.rs | 4 ++-- src/rustup/toolchain.rs | 10 ++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/rustup-dist/src/manifestation.rs b/src/rustup-dist/src/manifestation.rs index 60312af7de..ec9621dd08 100644 --- a/src/rustup-dist/src/manifestation.rs +++ b/src/rustup-dist/src/manifestation.rs @@ -212,8 +212,8 @@ impl Manifestation { // If the package doesn't contain the component that the // manifest says it does the somebody must be playing a joke on us. - if !package.contains(name, Some(short_name)) { - return Err(ErrorKind::CorruptComponent(component.pkg.clone()).into()); + if !package.contains(name, Some(&short_name)) { + return Err(ErrorKind::CorruptComponent(short_name).into()); } tx = package.install(&self.installation, name, Some(short_name), tx)?; diff --git a/src/rustup/toolchain.rs b/src/rustup/toolchain.rs index 1539437e63..733b47f4c6 100644 --- a/src/rustup/toolchain.rs +++ b/src/rustup/toolchain.rs @@ -617,6 +617,11 @@ impl<'a> Toolchain<'a> { let manifestation = Manifestation::open(prefix, toolchain.target.clone())?; if let Some(manifest) = manifestation.load_manifest()? { + // Rename the component if necessary. + if let Some(to) = manifest.renames.get(&component.pkg) { + component.pkg = to.to_owned(); + } + // Validate the component name let rust_pkg = manifest .packages @@ -679,6 +684,11 @@ impl<'a> Toolchain<'a> { let manifestation = Manifestation::open(prefix, toolchain.target.clone())?; if let Some(manifest) = manifestation.load_manifest()? { + // Rename the component if necessary. + if let Some(to) = manifest.renames.get(&component.pkg) { + component.pkg = to.to_owned(); + } + // Validate the component name let rust_pkg = manifest .packages From f9d0d11d6797cabbdb4b3fd53c4c9fca867e6ce7 Mon Sep 17 00:00:00 2001 From: Nick Cameron <nrc@ncameron.org> Date: Tue, 20 Nov 2018 15:14:02 +1300 Subject: [PATCH 2/8] use the 'from' part of a rename when listing components --- src/rustup-dist/src/manifest.rs | 20 +++++++++++++------- src/rustup/toolchain.rs | 21 +++++++++++++++++---- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/rustup-dist/src/manifest.rs b/src/rustup-dist/src/manifest.rs index 81f85c781a..c5abc1b8e0 100644 --- a/src/rustup-dist/src/manifest.rs +++ b/src/rustup-dist/src/manifest.rs @@ -26,6 +26,7 @@ pub struct Manifest { pub date: String, pub packages: HashMap<String, Package>, pub renames: HashMap<String, String>, + pub reverse_renames: HashMap<String, String>, } #[derive(Clone, Debug, PartialEq)] @@ -78,11 +79,13 @@ impl Manifest { if !SUPPORTED_MANIFEST_VERSIONS.contains(&&*version) { return Err(ErrorKind::UnsupportedVersion(version).into()); } + let (renames, reverse_renames) = Self::table_to_renames(&mut table, path)?; Ok(Manifest { manifest_version: version, date: get_string(&mut table, "date", path)?, packages: Self::table_to_packages(&mut table, path)?, - renames: Self::table_to_renames(table, path)?, + renames, + reverse_renames, }) } pub fn to_toml(self) -> toml::value::Table { @@ -127,19 +130,22 @@ impl Manifest { } fn table_to_renames( - mut table: toml::value::Table, + table: &mut toml::value::Table, path: &str, - ) -> Result<HashMap<String, String>> { - let mut result = HashMap::new(); - let rename_table = get_table(&mut table, "rename", path)?; + ) -> Result<(HashMap<String, String>, HashMap<String, String>)> { + let mut renames = HashMap::new(); + let mut reverse_renames = HashMap::new(); + let rename_table = get_table(table, "rename", path)?; for (k, v) in rename_table { if let toml::Value::Table(mut t) = v { - result.insert(k.to_owned(), get_string(&mut t, "to", path)?); + let to = get_string(&mut t, "to", path)?; + renames.insert(k.to_owned(), to.clone()); + reverse_renames.insert(to, k.to_owned()); } } - Ok(result) + Ok((renames, reverse_renames)) } fn renames_to_table(renames: HashMap<String, String>) -> toml::value::Table { let mut result = toml::value::Table::new(); diff --git a/src/rustup/toolchain.rs b/src/rustup/toolchain.rs index 733b47f4c6..bc3ffd38b7 100644 --- a/src/rustup/toolchain.rs +++ b/src/rustup/toolchain.rs @@ -521,10 +521,18 @@ impl<'a> Toolchain<'a> { .get(&toolchain.target) .expect("component should have target toolchain"); + let mut component = component.clone(); + // Rename components to the 'old' name. This may seem wrong, but + // it is a hack to name -preview versions of components back to + // their unsuffixed names. + if let Some(from) = manifest.reverse_renames.get(&component.pkg) { + component.pkg = from.to_owned(); + } + res.push(ComponentStatus { - component: component.clone(), + component, required: true, - installed: installed, + installed, available: component_target_pkg.available(), }); } @@ -545,10 +553,15 @@ impl<'a> Toolchain<'a> { .get(&toolchain.target) .expect("extension should have target toolchain"); + let mut component = extension.clone(); + if let Some(from) = manifest.reverse_renames.get(&component.pkg) { + component.pkg = from.to_owned(); + } + res.push(ComponentStatus { - component: extension.clone(), + component, required: false, - installed: installed, + installed, available: extension_target_pkg.available(), }); } From 52acf2c165fd62a7b0e2a1ffac6236038ffbbec1 Mon Sep 17 00:00:00 2001 From: Nick Cameron <nrc@ncameron.org> Date: Tue, 20 Nov 2018 17:29:05 +1300 Subject: [PATCH 3/8] use the 'from' part of a rename for the binary --- src/rustup-cli/common.rs | 2 +- src/rustup-dist/src/errors.rs | 4 ++-- src/rustup-dist/src/manifest.rs | 25 +++++++++++++++++-------- src/rustup-dist/src/manifestation.rs | 24 ++++++++++++++++-------- src/rustup-dist/src/notifications.rs | 9 ++++----- src/rustup/errors.rs | 13 ++++++------- src/rustup/toolchain.rs | 20 ++++++++++++++++---- 7 files changed, 62 insertions(+), 35 deletions(-) diff --git a/src/rustup-cli/common.rs b/src/rustup-cli/common.rs index 37f3d3eb1e..f17afd111f 100644 --- a/src/rustup-cli/common.rs +++ b/src/rustup-cli/common.rs @@ -310,7 +310,7 @@ pub fn list_targets(toolchain: &Toolchain) -> Result<()> { pub fn list_components(toolchain: &Toolchain) -> Result<()> { let mut t = term2::stdout(); for component in toolchain.list_components()? { - let name = component.component.name(); + let name = component.component.pkg; if component.required { let _ = t.attr(term2::Attr::Bold); let _ = writeln!(t, "{} (default)", name); diff --git a/src/rustup-dist/src/errors.rs b/src/rustup-dist/src/errors.rs index f193c4288f..53c7c4d5db 100644 --- a/src/rustup-dist/src/errors.rs +++ b/src/rustup-dist/src/errors.rs @@ -99,9 +99,9 @@ error_chain! { description("unsupported manifest version") display("manifest version '{}' is not supported", v) } - MissingPackageForComponent(c: Component) { + MissingPackageForComponent(name: String) { description("missing package for component") - display("server sent a broken manifest: missing package for component {}", c.name()) + display("server sent a broken manifest: missing package for component {}", name) } MissingPackageForRename(name: String) { description("missing package for the target of a rename") diff --git a/src/rustup-dist/src/manifest.rs b/src/rustup-dist/src/manifest.rs index c5abc1b8e0..880e7bdd8e 100644 --- a/src/rustup-dist/src/manifest.rs +++ b/src/rustup-dist/src/manifest.rs @@ -170,9 +170,9 @@ impl Manifest { fn validate_targeted_package(&self, tpkg: &TargetedPackage) -> Result<()> { for c in tpkg.components.iter().chain(tpkg.extensions.iter()) { let cpkg = self.get_package(&c.pkg) - .chain_err(|| ErrorKind::MissingPackageForComponent(c.clone()))?; + .chain_err(|| ErrorKind::MissingPackageForComponent(c.short_name(self)))?; let _ctpkg = cpkg.get_target(c.target.as_ref()) - .chain_err(|| ErrorKind::MissingPackageForComponent(c.clone()))?; + .chain_err(|| ErrorKind::MissingPackageForComponent(c.short_name(self)))?; } Ok(()) } @@ -390,18 +390,27 @@ impl Component { result.insert("pkg".to_owned(), toml::Value::String(self.pkg)); result } - pub fn name(&self) -> String { + pub fn name(&self, manifest: &Manifest) -> String { + let pkg = self.short_name(manifest); if let Some(ref t) = self.target { - format!("{}-{}", self.pkg, t) + format!("{}-{}", pkg, t) } else { - format!("{}", self.pkg) + format!("{}", pkg) } } - pub fn description(&self) -> String { + pub fn short_name(&self, manifest: &Manifest) -> String { + let mut pkg = self.pkg.clone(); + if let Some(from) = manifest.reverse_renames.get(&pkg) { + pkg = from.to_owned(); + } + pkg + } + pub fn description(&self, manifest: &Manifest) -> String { + let pkg = self.short_name(manifest); if let Some(ref t) = self.target { - format!("'{}' for target '{}'", self.pkg, t) + format!("'{}' for target '{}'", pkg, t) } else { - format!("'{}'", self.pkg) + format!("'{}'", pkg) } } } diff --git a/src/rustup-dist/src/manifestation.rs b/src/rustup-dist/src/manifestation.rs index ec9621dd08..821a952931 100644 --- a/src/rustup-dist/src/manifestation.rs +++ b/src/rustup-dist/src/manifestation.rs @@ -179,7 +179,7 @@ impl Manifestation { component.target.as_ref(), )); - tx = self.uninstall_component(&component, tx, notify_handler.clone())?; + tx = self.uninstall_component(&component, new_manifest, tx, notify_handler.clone())?; } // Install components @@ -207,8 +207,8 @@ impl Manifestation { // names are not the same as the dist manifest component // names. Some are just the component name some are the // component name plus the target triple. - let ref name = component.name(); - let ref short_name = format!("{}", component.pkg); + let ref name = component.name(new_manifest); + let short_name = component.short_name(new_manifest); // If the package doesn't contain the component that the // manifest says it does the somebody must be playing a joke on us. @@ -216,7 +216,7 @@ impl Manifestation { return Err(ErrorKind::CorruptComponent(short_name).into()); } - tx = package.install(&self.installation, name, Some(short_name), tx)?; + tx = package.install(&self.installation, name, Some(&short_name), tx)?; } // Install new distribution manifest @@ -246,7 +246,12 @@ impl Manifestation { Ok(UpdateStatus::Changed) } - pub fn uninstall(&self, temp_cfg: &temp::Cfg, notify_handler: &Fn(Notification)) -> Result<()> { + pub fn uninstall( + &self, + manifest: &Manifest, + temp_cfg: &temp::Cfg, + notify_handler: &Fn(Notification), + ) -> Result<()> { let prefix = self.installation.prefix(); let mut tx = Transaction::new(prefix.clone(), temp_cfg, notify_handler); @@ -259,7 +264,7 @@ impl Manifestation { tx.remove_file("dist config", rel_config_path)?; for component in config.components { - tx = self.uninstall_component(&component, tx, notify_handler)?; + tx = self.uninstall_component(&component, manifest, tx, notify_handler)?; } tx.commit(); @@ -269,6 +274,7 @@ impl Manifestation { fn uninstall_component<'a>( &self, component: &Component, + manifest: &Manifest, mut tx: Transaction<'a>, notify_handler: &Fn(Notification), ) -> Result<Transaction<'a>> { @@ -276,7 +282,7 @@ impl Manifestation { // names are not the same as the dist manifest component // names. Some are just the component name some are the // component name plus the target triple. - let ref name = component.name(); + let ref name = component.name(manifest); let ref short_name = format!("{}", component.pkg); if let Some(c) = self.installation.find(&name)? { tx = c.uninstall(tx)?; @@ -490,7 +496,9 @@ impl Update { result.components_to_install.push(component.clone()); } else { if changes.add_extensions.contains(&component) { - notify_handler(Notification::ComponentAlreadyInstalled(&component)); + notify_handler(Notification::ComponentAlreadyInstalled( + &component.description(new_manifest) + )); } } } diff --git a/src/rustup-dist/src/notifications.rs b/src/rustup-dist/src/notifications.rs index b69d203f0b..693d888db4 100644 --- a/src/rustup-dist/src/notifications.rs +++ b/src/rustup-dist/src/notifications.rs @@ -3,7 +3,6 @@ use std::fmt::{self, Display}; use temp; use rustup_utils; use rustup_utils::notify::NotificationLevel; -use manifest::Component; use dist::TargetTriple; use errors::*; @@ -13,7 +12,7 @@ pub enum Notification<'a> { Temp(temp::Notification<'a>), Extracting(&'a Path, &'a Path), - ComponentAlreadyInstalled(&'a Component), + ComponentAlreadyInstalled(&'a str), CantReadUpdateHash(&'a Path), NoUpdateHash(&'a Path), ChecksumValid(&'a str), @@ -21,7 +20,7 @@ pub enum Notification<'a> { FileAlreadyDownloaded, CachedFileChecksumFailed, RollingBack, - ExtensionNotInstalled(&'a Component), + ExtensionNotInstalled(&'a str), NonFatalError(&'a Error), MissingInstalledComponent(&'a str), DownloadingComponent(&'a str, &'a TargetTriple, Option<&'a TargetTriple>), @@ -84,7 +83,7 @@ impl<'a> Display for Notification<'a> { Utils(ref n) => n.fmt(f), Extracting(_, _) => write!(f, "extracting..."), ComponentAlreadyInstalled(ref c) => { - write!(f, "component {} is up to date", c.description()) + write!(f, "component {} is up to date", c) } CantReadUpdateHash(path) => write!( f, @@ -97,7 +96,7 @@ impl<'a> Display for Notification<'a> { 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 '{}' was not installed", c.name()), + ExtensionNotInstalled(c) => write!(f, "extension '{}' was not installed", c), NonFatalError(e) => write!(f, "{}", e), MissingInstalledComponent(c) => { write!(f, "during uninstall component {} was not found", c) diff --git a/src/rustup/errors.rs b/src/rustup/errors.rs index 6b8eb64166..1ae5cf2c06 100644 --- a/src/rustup/errors.rs +++ b/src/rustup/errors.rs @@ -1,6 +1,5 @@ use rustup_dist::{self, temp}; use rustup_utils; -use rustup_dist::manifest::Component; use toml; error_chain! { @@ -44,22 +43,22 @@ error_chain! { description("toolchain does not support components") display("toolchain '{}' does not support components", t) } - UnknownComponent(t: String, c: Component) { + UnknownComponent(t: String, c: String) { description("toolchain does not contain component") - display("toolchain '{}' does not contain component {}", t, c.description()) + display("toolchain '{}' does not contain component {}", t, c) } - AddingRequiredComponent(t: String, c: Component) { + AddingRequiredComponent(t: String, c: String) { description("required component cannot be added") display("component {} was automatically added because it is required for toolchain '{}'", - c.description(), t) + c, t) } ParsingSettings(e: toml::de::Error) { description("error parsing settings") } - RemovingRequiredComponent(t: String, c: Component) { + RemovingRequiredComponent(t: String, c: String) { description("required component cannot be removed") display("component {} is required for toolchain '{}' and cannot be removed", - c.description(), t) + c, t) } NoExeName { description("couldn't determine self executable name") diff --git a/src/rustup/toolchain.rs b/src/rustup/toolchain.rs index bc3ffd38b7..a0adb81ed0 100644 --- a/src/rustup/toolchain.rs +++ b/src/rustup/toolchain.rs @@ -647,7 +647,10 @@ impl<'a> Toolchain<'a> { if targ_pkg.components.contains(&component) { return Err( - ErrorKind::AddingRequiredComponent(self.name.to_string(), component).into(), + ErrorKind::AddingRequiredComponent( + self.name.to_string(), + component.description(&manifest), + ).into(), ); } @@ -660,7 +663,10 @@ impl<'a> Toolchain<'a> { component = wildcard_component; } else { return Err( - ErrorKind::UnknownComponent(self.name.to_string(), component).into(), + ErrorKind::UnknownComponent( + self.name.to_string(), + component.description(&manifest), + ).into(), ); } } @@ -714,7 +720,10 @@ impl<'a> Toolchain<'a> { if targ_pkg.components.contains(&component) { return Err( - ErrorKind::RemovingRequiredComponent(self.name.to_string(), component).into(), + ErrorKind::RemovingRequiredComponent( + self.name.to_string(), + component.description(&manifest), + ).into(), ); } @@ -728,7 +737,10 @@ impl<'a> Toolchain<'a> { component = wildcard_component; } else { return Err( - ErrorKind::UnknownComponent(self.name.to_string(), component).into(), + ErrorKind::UnknownComponent( + self.name.to_string(), + component.description(&manifest), + ).into(), ); } } From 35e1a9e28c85b8c06ff07bf656a2f072657a7a3e Mon Sep 17 00:00:00 2001 From: Nick Cameron <nrc@ncameron.org> Date: Wed, 21 Nov 2018 08:54:36 +1300 Subject: [PATCH 4/8] Fix tests --- src/rustup-mock/src/clitools.rs | 25 ++++++++++++------------- tests/cli-misc.rs | 6 +++--- tests/cli-self-upd.rs | 2 +- tests/cli-v2.rs | 2 +- 4 files changed, 17 insertions(+), 18 deletions(-) diff --git a/src/rustup-mock/src/clitools.rs b/src/rustup-mock/src/clitools.rs index d12c7f9c8c..0b45ee1d62 100644 --- a/src/rustup-mock/src/clitools.rs +++ b/src/rustup-mock/src/clitools.rs @@ -485,11 +485,11 @@ fn build_mock_channel( ]; if rename_rls { + let rls = build_mock_rls_installer(version, version_hash, true); + all.push(("rls-preview", vec![(rls, host_triple.clone())])); + } else { let rls = build_mock_rls_installer(version, version_hash, false); all.push(("rls", vec![(rls, host_triple.clone())])); - } else { - let rls_preview = build_mock_rls_installer(version, version_hash, true); - all.push(("rls-preview", vec![(rls_preview, host_triple.clone())])); } let more = vec![ @@ -526,11 +526,11 @@ fn build_mock_channel( all.extend(more); if rename_rls { + let rls = build_mock_rls_installer(version, version_hash, true); + all.push(("rls-preview", vec![(rls, triple.clone())])); + } else { let rls = build_mock_rls_installer(version, version_hash, false); all.push(("rls", vec![(rls, triple.clone())])); - } else { - let rls_preview = build_mock_rls_installer(version, version_hash, true); - all.push(("rls-preview", vec![(rls_preview, triple.clone())])); } let more = vec![ @@ -584,12 +584,12 @@ fn build_mock_channel( }); if rename_rls { target_pkg.extensions.push(MockComponent { - name: "rls".to_string(), + name: "rls-preview".to_string(), target: target.to_string(), }); } else { target_pkg.extensions.push(MockComponent { - name: "rls-preview".to_string(), + name: "rls".to_string(), target: target.to_string(), }); } @@ -614,7 +614,7 @@ fn build_mock_channel( let mut renames = HashMap::new(); if rename_rls { - renames.insert("rls-preview".to_owned(), "rls".to_owned()); + renames.insert("rls".to_owned(), "rls-preview".to_owned()); } MockChannel { @@ -634,7 +634,7 @@ fn build_mock_unavailable_channel(channel: &str, date: &str, version: &'static s "rust-docs", "rust-std", "rustc", - "rls-preview", + "rls", "rust-analysis", ]; let packages = packages @@ -764,13 +764,12 @@ fn build_mock_cargo_installer(version: &str, version_hash: &str) -> MockInstalle fn build_mock_rls_installer( version: &str, version_hash: &str, - preview: bool, + _preview: bool, ) -> MockInstallerBuilder { - let name = if preview { "rls-preview" } else { "rls" }; MockInstallerBuilder { components: vec![ MockComponentBuilder { - name: name.to_string(), + name: "rls".to_string(), files: mock_bin("rls", version, version_hash), }, ], diff --git a/tests/cli-misc.rs b/tests/cli-misc.rs index 06c76f6270..d47767e74c 100644 --- a/tests/cli-misc.rs +++ b/tests/cli-misc.rs @@ -533,7 +533,7 @@ fn telemetry_cleanup_removes_old_files() { fn rls_exists_in_toolchain() { setup(&|config| { expect_ok(config, &["rustup", "default", "stable"]); - expect_ok(config, &["rustup", "component", "add", "rls-preview"]); + expect_ok(config, &["rustup", "component", "add", "rls"]); assert!(config.exedir.join(format!("rls{}", EXE_SUFFIX)).exists()); expect_ok(config, &["rls", "--version"]); @@ -563,7 +563,7 @@ fn rename_rls_before() { clitools::setup(Scenario::ArchivesV2, &|config| { set_current_dist_date(config, "2015-01-01"); expect_ok(config, &["rustup", "default", "nightly"]); - expect_ok(config, &["rustup", "component", "add", "rls-preview"]); + expect_ok(config, &["rustup", "component", "add", "rls"]); set_current_dist_date(config, "2015-01-02"); expect_ok(config, &["rustup", "update", "--no-self-update"]); @@ -581,7 +581,7 @@ fn rename_rls_after() { set_current_dist_date(config, "2015-01-02"); expect_ok(config, &["rustup", "update", "--no-self-update"]); - expect_ok(config, &["rustup", "component", "add", "rls"]); + expect_ok(config, &["rustup", "component", "add", "rls-preview"]); assert!(config.exedir.join(format!("rls{}", EXE_SUFFIX)).exists()); expect_ok(config, &["rls", "--version"]); diff --git a/tests/cli-self-upd.rs b/tests/cli-self-upd.rs index 4aca349093..bbc30b5b9a 100644 --- a/tests/cli-self-upd.rs +++ b/tests/cli-self-upd.rs @@ -1311,7 +1311,7 @@ fn rls_proxy_set_up_after_install() { EXE_SUFFIX ), ); - expect_ok(config, &["rustup", "component", "add", "rls-preview"]); + expect_ok(config, &["rustup", "component", "add", "rls"]); expect_ok(config, &["rls", "--version"]); }); } diff --git a/tests/cli-v2.rs b/tests/cli-v2.rs index 440c1ae8cd..0c2af955a7 100644 --- a/tests/cli-v2.rs +++ b/tests/cli-v2.rs @@ -827,7 +827,7 @@ fn update_unavailable_std() { config, &["rustup", "update", "nightly"], &format!( - "component 'rust-std' for '{}' is unavailable for download", + "component 'rust-std' for target '{}' is unavailable for download", trip ), ); From 320a0e6c1cfd8f4c856659b2491ac1c3f421d55f Mon Sep 17 00:00:00 2001 From: Nick Cameron <nrc@ncameron.org> Date: Wed, 21 Nov 2018 08:55:17 +1300 Subject: [PATCH 5/8] Be more thorough about how components are named --- src/rustup-cli/common.rs | 4 +- src/rustup-cli/rustup_mode.rs | 22 +++-------- src/rustup-dist/src/errors.rs | 32 +++++----------- src/rustup-dist/src/manifest.rs | 32 +++++++++++++--- src/rustup-dist/src/manifestation.rs | 56 +++++++++++++--------------- src/rustup/toolchain.rs | 46 ++++++++--------------- 6 files changed, 83 insertions(+), 109 deletions(-) diff --git a/src/rustup-cli/common.rs b/src/rustup-cli/common.rs index f17afd111f..b5e435c1df 100644 --- a/src/rustup-cli/common.rs +++ b/src/rustup-cli/common.rs @@ -284,7 +284,7 @@ pub fn rustc_version(toolchain: &Toolchain) -> String { pub fn list_targets(toolchain: &Toolchain) -> Result<()> { let mut t = term2::stdout(); for component in toolchain.list_components()? { - if component.component.pkg == "rust-std" { + if component.component.name_in_manifest() == "rust-std" { let target = component .component .target @@ -310,7 +310,7 @@ pub fn list_targets(toolchain: &Toolchain) -> Result<()> { pub fn list_components(toolchain: &Toolchain) -> Result<()> { let mut t = term2::stdout(); for component in toolchain.list_components()? { - let name = component.component.pkg; + let name = component.name; if component.required { let _ = t.attr(term2::Attr::Bold); let _ = writeln!(t, "{} (default)", name); diff --git a/src/rustup-cli/rustup_mode.rs b/src/rustup-cli/rustup_mode.rs index 28737879bf..040a62700a 100644 --- a/src/rustup-cli/rustup_mode.rs +++ b/src/rustup-cli/rustup_mode.rs @@ -650,7 +650,7 @@ fn show(cfg: &Cfg) -> Result<()> { match t.list_components() { Ok(cs_vec) => cs_vec .into_iter() - .filter(|c| c.component.pkg == "rust-std") + .filter(|c| c.component.name_in_manifest() == "rust-std") .filter(|c| c.installed) .collect(), Err(_) => vec![], @@ -778,10 +778,7 @@ fn target_add(cfg: &Cfg, m: &ArgMatches) -> Result<()> { let toolchain = explicit_or_dir_toolchain(cfg, m)?; for target in m.values_of("target").expect("") { - let new_component = Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str(target)), - }; + let new_component = Component::new("rust-std".to_string(), Some(TargetTriple::from_str(target))); toolchain.add_component(new_component)?; } @@ -793,10 +790,7 @@ fn target_remove(cfg: &Cfg, m: &ArgMatches) -> Result<()> { let toolchain = explicit_or_dir_toolchain(cfg, m)?; for target in m.values_of("target").expect("") { - let new_component = Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str(target)), - }; + let new_component = Component::new("rust-std".to_string(), Some(TargetTriple::from_str(target))); toolchain.remove_component(new_component)?; } @@ -823,10 +817,7 @@ fn component_add(cfg: &Cfg, m: &ArgMatches) -> Result<()> { }); for component in m.values_of("component").expect("") { - let new_component = Component { - pkg: component.to_string(), - target: target.clone(), - }; + let new_component = Component::new(component.to_string(), target.clone()); toolchain.add_component(new_component)?; } @@ -847,10 +838,7 @@ fn component_remove(cfg: &Cfg, m: &ArgMatches) -> Result<()> { }); for component in m.values_of("component").expect("") { - let new_component = Component { - pkg: component.to_string(), - target: target.clone(), - }; + let new_component = Component::new(component.to_string(), target.clone()); toolchain.remove_component(new_component)?; } diff --git a/src/rustup-dist/src/errors.rs b/src/rustup-dist/src/errors.rs index 53c7c4d5db..7e5882be3f 100644 --- a/src/rustup-dist/src/errors.rs +++ b/src/rustup-dist/src/errors.rs @@ -3,7 +3,7 @@ use std::io::{self, Write}; use temp; use toml; use rustup_utils; -use manifest::Component; +use manifest::{Component, Manifest}; error_chain! { links { @@ -82,15 +82,9 @@ error_chain! { ComponentFilePermissionsFailed { description("error setting file permissions during install") } - ComponentDownloadFailed(c: Component) { + ComponentDownloadFailed(c: String) { description("component download failed") - display("component download failed for {}{}", c.pkg, { - if let Some(ref t) = c.target { - format!("-{}", t) - } else { - "".to_owned() - } - }) + display("component download failed for {}", c) } Parsing(e: toml::de::Error) { description("error parsing manifest") @@ -107,42 +101,34 @@ error_chain! { description("missing package for the target of a rename") display("server sent a broken manifest: missing package for the target of a rename {}", name) } - RequestedComponentsUnavailable(c: Vec<Component>) { + RequestedComponentsUnavailable(c: Vec<Component>, manifest: Manifest) { description("some requested components are unavailable to download") - display("{}", component_unavailable_msg(&c)) + display("{}", component_unavailable_msg(&c, &manifest)) } } } -fn component_unavailable_msg(cs: &[Component]) -> String { +fn component_unavailable_msg(cs: &[Component], manifest: &Manifest) -> String { assert!(!cs.is_empty()); let mut buf = vec![]; - fn format_component(c: &Component) -> String { - if let Some(ref t) = c.target { - format!("'{}' for '{}'", c.pkg, t) - } else { - format!("'{}'", c.pkg) - } - } - if cs.len() == 1 { let _ = write!( buf, "component {} is unavailable for download", - format_component(&cs[0]) + &cs[0].description(manifest) ); } else { use itertools::Itertools; let same_target = cs.iter() .all(|c| c.target == cs[0].target || c.target.is_none()); if same_target { - let mut cs_strs = cs.iter().map(|c| format!("'{}'", c.pkg)); + let mut cs_strs = cs.iter().map(|c| format!("'{}'", c.short_name(manifest))); let cs_str = cs_strs.join(", "); let _ = write!(buf, "some components unavailable for download: {}", cs_str); } else { - let mut cs_strs = cs.iter().map(format_component); + let mut cs_strs = cs.iter().map(|c| c.description(manifest)); let cs_str = cs_strs.join(", "); let _ = write!(buf, "some components unavailable for download: {}", cs_str); } diff --git a/src/rustup-dist/src/manifest.rs b/src/rustup-dist/src/manifest.rs index 880e7bdd8e..a54069537c 100644 --- a/src/rustup-dist/src/manifest.rs +++ b/src/rustup-dist/src/manifest.rs @@ -58,7 +58,7 @@ pub struct PackageBins { #[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)] pub struct Component { - pub pkg: String, + pkg: String, pub target: Option<TargetTriple>, } @@ -200,6 +200,16 @@ impl Manifest { Ok(()) } + + // If the component should be renamed by this manifest, then return a new + // component with the new name. If not, return `None`. + pub fn rename_component(&self, component: &Component) -> Option<Component> { + self.renames.get(&component.pkg).map(|r| { + let mut c = component.clone(); + c.pkg = r.clone(); + c + }) + } } impl Package { @@ -365,6 +375,15 @@ impl TargetedPackage { } impl Component { + pub fn new(pkg: String, target: Option<TargetTriple>) -> Component { + Component { pkg, target } + } + pub fn wildcard(&self) -> Component { + Component { + pkg: self.pkg.clone(), + target: None, + } + } pub fn from_toml(mut table: toml::value::Table, path: &str) -> Result<Self> { Ok(Component { pkg: get_string(&mut table, "pkg", path)?, @@ -399,11 +418,11 @@ impl Component { } } pub fn short_name(&self, manifest: &Manifest) -> String { - let mut pkg = self.pkg.clone(); - if let Some(from) = manifest.reverse_renames.get(&pkg) { - pkg = from.to_owned(); + if let Some(from) = manifest.reverse_renames.get(&self.pkg) { + from.to_owned() + } else { + self.pkg.clone() } - pkg } pub fn description(&self, manifest: &Manifest) -> String { let pkg = self.short_name(manifest); @@ -413,4 +432,7 @@ impl Component { format!("'{}'", pkg) } } + pub fn name_in_manifest(&self) -> &String { + &self.pkg + } } diff --git a/src/rustup-dist/src/manifestation.rs b/src/rustup-dist/src/manifestation.rs index 821a952931..c7b3b7f9b2 100644 --- a/src/rustup-dist/src/manifestation.rs +++ b/src/rustup-dist/src/manifestation.rs @@ -126,7 +126,7 @@ impl Manifestation { } // Make sure we don't accidentally uninstall the essential components! (see #1297) - update.missing_essential_components(&self.target_triple)?; + update.missing_essential_components(&self.target_triple, new_manifest)?; // Validate that the requested components are available match update.unavailable_components(new_manifest) { @@ -142,7 +142,7 @@ impl Manifestation { let mut things_downloaded: Vec<String> = Vec::new(); for (component, format, url, hash) in update.components_urls_and_hashes(new_manifest)? { notify_handler(Notification::DownloadingComponent( - &component.pkg, + &component.short_name(new_manifest), &self.target_triple, component.target.as_ref(), )); @@ -154,13 +154,13 @@ impl Manifestation { let url_url = utils::parse_url(&url)?; - let dowloaded_file = download_cfg + let downloaded_file = download_cfg .download(&url_url, &hash) - .chain_err(|| ErrorKind::ComponentDownloadFailed(component.clone()))?; + .chain_err(|| ErrorKind::ComponentDownloadFailed(component.name(new_manifest)))?; things_downloaded.push(hash); - things_to_install.push((component, format, dowloaded_file)); + things_to_install.push((component, format, downloaded_file)); } // Begin transaction @@ -174,7 +174,7 @@ impl Manifestation { // Uninstall components for component in update.components_to_uninstall { notify_handler(Notification::RemovingComponent( - &component.pkg, + &component.short_name(new_manifest), &self.target_triple, component.target.as_ref(), )); @@ -184,8 +184,15 @@ impl Manifestation { // Install components for (component, format, installer_file) in things_to_install { + // For historical reasons, the rust-installer component + // names are not the same as the dist manifest component + // names. Some are just the component name some are the + // component name plus the target triple. + let ref name = component.name(new_manifest); + let short_name = component.short_name(new_manifest); + notify_handler(Notification::InstallingComponent( - &component.pkg, + &short_name, &self.target_triple, component.target.as_ref(), )); @@ -203,15 +210,8 @@ impl Manifestation { } }; - // For historical reasons, the rust-installer component - // names are not the same as the dist manifest component - // names. Some are just the component name some are the - // component name plus the target triple. - let ref name = component.name(new_manifest); - let short_name = component.short_name(new_manifest); - // If the package doesn't contain the component that the - // manifest says it does the somebody must be playing a joke on us. + // manifest says it does then somebody must be playing a joke on us. if !package.contains(name, Some(&short_name)) { return Err(ErrorKind::CorruptComponent(short_name).into()); } @@ -283,7 +283,7 @@ impl Manifestation { // names. Some are just the component name some are the // component name plus the target triple. let ref name = component.name(manifest); - let ref short_name = format!("{}", component.pkg); + let ref short_name = component.short_name(manifest); if let Some(c) = self.installation.find(&name)? { tx = c.uninstall(tx)?; } else if let Some(c) = self.installation.find(&short_name)? { @@ -537,10 +537,7 @@ impl Update { 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(); + if let Some(renamed_component) = new_manifest.rename_component(&existing_component) { let is_already_included = self.final_component_list.contains(&renamed_component); if !is_already_included { @@ -563,7 +560,7 @@ impl Update { self.components_to_uninstall .push(existing_component.clone()); notify_handler(Notification::ComponentUnavailable( - &existing_component.pkg, + &existing_component.short_name(new_manifest), existing_component.target.as_ref(), )); } @@ -577,24 +574,21 @@ impl Update { self.components_to_uninstall.is_empty() && self.components_to_install.is_empty() } - fn missing_essential_components(&self, target_triple: &TargetTriple) -> Result<()> { + fn missing_essential_components(&self, target_triple: &TargetTriple, manifest: &Manifest) -> Result<()> { let missing_essential_components = ["rustc", "cargo"] .iter() .filter_map(|pkg| { - if self.final_component_list.iter().any(|c| &c.pkg == pkg) { + if self.final_component_list.iter().any(|c| &c.name_in_manifest() == pkg) { None } else { - Some(Component { - pkg: pkg.to_string(), - target: Some(target_triple.clone()), - }) + Some(Component::new(pkg.to_string(), Some(target_triple.clone()))) } }) .collect::<Vec<_>>(); if !missing_essential_components.is_empty() { return Err( - ErrorKind::RequestedComponentsUnavailable(missing_essential_components).into(), + ErrorKind::RequestedComponentsUnavailable(missing_essential_components, manifest.clone()).into(), ); } @@ -606,7 +600,7 @@ impl Update { .iter() .filter(|c| { use manifest::*; - let pkg: Option<&Package> = new_manifest.get_package(&c.pkg).ok(); + let pkg: Option<&Package> = new_manifest.get_package(&c.name_in_manifest()).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) @@ -617,7 +611,7 @@ impl Update { unavailable_components.extend_from_slice(&self.missing_components); if !unavailable_components.is_empty() { - return Err(ErrorKind::RequestedComponentsUnavailable(unavailable_components).into()); + return Err(ErrorKind::RequestedComponentsUnavailable(unavailable_components, new_manifest.clone()).into()); } Ok(()) @@ -630,7 +624,7 @@ impl Update { ) -> Result<Vec<(Component, Format, String, String)>> { let mut components_urls_and_hashes = Vec::new(); for component in &self.components_to_install { - let package = new_manifest.get_package(&component.pkg)?; + let package = new_manifest.get_package(&component.name_in_manifest())?; let target_package = package.get_target(component.target.as_ref())?; let bins = target_package.bins.as_ref().expect("components available"); diff --git a/src/rustup/toolchain.rs b/src/rustup/toolchain.rs index a0adb81ed0..c7e595503c 100644 --- a/src/rustup/toolchain.rs +++ b/src/rustup/toolchain.rs @@ -34,6 +34,7 @@ pub struct Toolchain<'a> { /// Used by the `list_component` function pub struct ComponentStatus { pub component: Component, + pub name: String, pub required: bool, pub installed: bool, pub available: bool, @@ -512,25 +513,18 @@ impl<'a> Toolchain<'a> { .unwrap_or(false); // Get the component so we can check if it is available - let component_pkg = manifest.get_package(&component.pkg).expect(&format!( + let component_pkg = manifest.get_package(&component.name_in_manifest()).expect(&format!( "manifest should contain component {}", - &component.pkg + &component.short_name(&manifest) )); let component_target_pkg = component_pkg .targets .get(&toolchain.target) .expect("component should have target toolchain"); - let mut component = component.clone(); - // Rename components to the 'old' name. This may seem wrong, but - // it is a hack to name -preview versions of components back to - // their unsuffixed names. - if let Some(from) = manifest.reverse_renames.get(&component.pkg) { - component.pkg = from.to_owned(); - } - res.push(ComponentStatus { - component, + component: component.clone(), + name: component.name(&manifest), required: true, installed, available: component_target_pkg.available(), @@ -544,22 +538,18 @@ impl<'a> Toolchain<'a> { .unwrap_or(false); // Get the component so we can check if it is available - let extension_pkg = manifest.get_package(&extension.pkg).expect(&format!( + let extension_pkg = manifest.get_package(&extension.name_in_manifest()).expect(&format!( "manifest should contain extension {}", - &extension.pkg + &extension.short_name(&manifest) )); let extension_target_pkg = extension_pkg .targets .get(&toolchain.target) .expect("extension should have target toolchain"); - let mut component = extension.clone(); - if let Some(from) = manifest.reverse_renames.get(&component.pkg) { - component.pkg = from.to_owned(); - } - res.push(ComponentStatus { - component, + component: extension.clone(), + name: extension.name(&manifest), required: false, installed, available: extension_target_pkg.available(), @@ -631,8 +621,8 @@ impl<'a> Toolchain<'a> { if let Some(manifest) = manifestation.load_manifest()? { // Rename the component if necessary. - if let Some(to) = manifest.renames.get(&component.pkg) { - component.pkg = to.to_owned(); + if let Some(c) = manifest.rename_component(&component) { + component = c; } // Validate the component name @@ -655,10 +645,7 @@ impl<'a> Toolchain<'a> { } if !targ_pkg.extensions.contains(&component) { - let wildcard_component = Component { - target: None, - ..component.clone() - }; + let wildcard_component = component.wildcard(); if targ_pkg.extensions.contains(&wildcard_component) { component = wildcard_component; } else { @@ -704,8 +691,8 @@ impl<'a> Toolchain<'a> { if let Some(manifest) = manifestation.load_manifest()? { // Rename the component if necessary. - if let Some(to) = manifest.renames.get(&component.pkg) { - component.pkg = to.to_owned(); + if let Some(c) = manifest.rename_component(&component) { + component = c; } // Validate the component name @@ -729,10 +716,7 @@ impl<'a> Toolchain<'a> { let dist_config = manifestation.read_config()?.unwrap(); if !dist_config.components.contains(&component) { - let wildcard_component = Component { - target: None, - ..component.clone() - }; + let wildcard_component = component.wildcard(); if dist_config.components.contains(&wildcard_component) { component = wildcard_component; } else { From db517e48d9a7f3ba71ae5b3accb9898a4aa51ed8 Mon Sep 17 00:00:00 2001 From: Nick Cameron <nrc@ncameron.org> Date: Wed, 21 Nov 2018 11:22:06 +1300 Subject: [PATCH 6/8] Add tests --- tests/cli-misc.rs | 80 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/tests/cli-misc.rs b/tests/cli-misc.rs index d47767e74c..7da8c84670 100644 --- a/tests/cli-misc.rs +++ b/tests/cli-misc.rs @@ -588,6 +588,86 @@ fn rename_rls_after() { }); } +#[test] +fn rename_rls_add_old_name() { + clitools::setup(Scenario::ArchivesV2, &|config| { + set_current_dist_date(config, "2015-01-01"); + expect_ok(config, &["rustup", "default", "nightly"]); + + set_current_dist_date(config, "2015-01-02"); + expect_ok(config, &["rustup", "update", "--no-self-update"]); + expect_ok(config, &["rustup", "component", "add", "rls"]); + + assert!(config.exedir.join(format!("rls{}", EXE_SUFFIX)).exists()); + expect_ok(config, &["rls", "--version"]); + }); +} + +#[test] +fn rename_rls_list() { + clitools::setup(Scenario::ArchivesV2, &|config| { + set_current_dist_date(config, "2015-01-01"); + expect_ok(config, &["rustup", "default", "nightly"]); + + set_current_dist_date(config, "2015-01-02"); + expect_ok(config, &["rustup", "update", "--no-self-update"]); + expect_ok(config, &["rustup", "component", "add", "rls"]); + + let out = run( + config, + "rustup", + &["component", "list"], + &[], + ); + assert!(out.ok); + assert!(out.stdout.contains(&format!("rls-{}", this_host_triple())) + ); + }); +} + +#[test] +fn rename_rls_preview_list() { + clitools::setup(Scenario::ArchivesV2, &|config| { + set_current_dist_date(config, "2015-01-01"); + expect_ok(config, &["rustup", "default", "nightly"]); + + set_current_dist_date(config, "2015-01-02"); + expect_ok(config, &["rustup", "update", "--no-self-update"]); + expect_ok(config, &["rustup", "component", "add", "rls-preview"]); + + let out = run( + config, + "rustup", + &["component", "list"], + &[], + ); + assert!(out.ok); + assert!(out.stdout.contains(&format!("rls-{}", this_host_triple())) + ); + }); +} + +#[test] +fn rename_rls_remove() { + clitools::setup(Scenario::ArchivesV2, &|config| { + set_current_dist_date(config, "2015-01-01"); + expect_ok(config, &["rustup", "default", "nightly"]); + + set_current_dist_date(config, "2015-01-02"); + expect_ok(config, &["rustup", "update", "--no-self-update"]); + + expect_ok(config, &["rustup", "component", "add", "rls"]); + expect_ok(config, &["rls", "--version"]); + expect_ok(config, &["rustup", "component", "remove", "rls"]); + expect_err(config, &["rls", "--version"], "does not have the binary `rls`"); + + expect_ok(config, &["rustup", "component", "add", "rls"]); + expect_ok(config, &["rls", "--version"]); + expect_ok(config, &["rustup", "component", "remove", "rls-preview"]); + expect_err(config, &["rls", "--version"], "does not have the binary `rls`"); + }); +} + #[test] fn install_stops_if_rustc_exists() { let temp_dir = TempDir::new("fakebin").unwrap(); From 5645cf547abd04dd08f9ee02b242c00ada045856 Mon Sep 17 00:00:00 2001 From: Nick Cameron <nrc@ncameron.org> Date: Wed, 21 Nov 2018 13:05:12 +1300 Subject: [PATCH 7/8] Give a better error message if a component is not installed --- src/rustup-cli/self_update.rs | 9 +-------- src/rustup/errors.rs | 10 +++++++++- src/rustup/lib.rs | 23 +++++++++++++++++++++++ tests/cli-misc.rs | 10 +++++----- tests/cli-self-upd.rs | 4 ++-- 5 files changed, 40 insertions(+), 16 deletions(-) diff --git a/src/rustup-cli/self_update.rs b/src/rustup-cli/self_update.rs index 3817fd4a31..b2c7ae14a6 100644 --- a/src/rustup-cli/self_update.rs +++ b/src/rustup-cli/self_update.rs @@ -34,6 +34,7 @@ use common::{self, Confirm}; use errors::*; use rustup_dist::dist; use rustup_utils::utils; +use rustup::{TOOLS, DUP_TOOLS}; use same_file::Handle; use std::env; use std::env::consts::EXE_SUFFIX; @@ -192,14 +193,6 @@ doing then it is fine to continue installation without the build tools, but otherwise, install the C++ build tools before proceeding. "#; -static TOOLS: &'static [&'static str] = - &["rustc", "rustdoc", "cargo", "rust-lldb", "rust-gdb", "rls", "cargo-clippy"]; - -// Tools which are commonly installed by Cargo as well as rustup. We take a bit -// more care with these to ensure we don't overwrite the user's previous -// installation. -static DUP_TOOLS: &'static [&'static str] = &["rustfmt", "cargo-fmt"]; - static UPDATE_ROOT: &'static str = "https://static.rust-lang.org/rustup"; /// `CARGO_HOME` suitable for display, possibly with $HOME diff --git a/src/rustup/errors.rs b/src/rustup/errors.rs index 1ae5cf2c06..69ad1ee79a 100644 --- a/src/rustup/errors.rs +++ b/src/rustup/errors.rs @@ -1,5 +1,6 @@ use rustup_dist::{self, temp}; use rustup_utils; +use component_for_bin; use toml; error_chain! { @@ -27,7 +28,7 @@ error_chain! { } BinaryNotFound(t: String, bin: String) { description("toolchain does not contain binary") - display("toolchain '{}' does not have the binary `{}`", t, bin) + display("'{}' is not installed for the toolchain '{}'{}", bin, t, install_msg(bin)) } NeedMetadataUpgrade { description("rustup's metadata is out of date. run `rustup self upgrade-data`") @@ -71,3 +72,10 @@ error_chain! { } } } + +fn install_msg(bin: &str) -> String { + match component_for_bin(bin) { + Some(c) => format!("\nTo install, run `rustup component add {}`", c), + None => String::new(), + } +} diff --git a/src/rustup/lib.rs b/src/rustup/lib.rs index 0e70055394..629497f701 100644 --- a/src/rustup/lib.rs +++ b/src/rustup/lib.rs @@ -22,6 +22,29 @@ pub use config::*; pub use toolchain::*; pub use rustup_utils::{notify, toml_utils, utils}; + +// A list of all binaries which Rustup will proxy. +pub static TOOLS: &'static [&'static str] = + &["rustc", "rustdoc", "cargo", "rust-lldb", "rust-gdb", "rls", "cargo-clippy"]; + +// Tools which are commonly installed by Cargo as well as rustup. We take a bit +// more care with these to ensure we don't overwrite the user's previous +// installation. +pub static DUP_TOOLS: &'static [&'static str] = &["rustfmt", "cargo-fmt"]; + +fn component_for_bin(binary: &str) -> Option<&'static str> { + match binary { + "rustc" | "rustdoc" => Some("rustc"), + "cargo" => Some("cargo"), + "rust-lldb" => Some("lldb-preview"), + "rust-gdb" => Some("gdb-preview"), + "rls" => Some("rls"), + "cargo-clippy" => Some("clippy"), + "rustfmt" | "cargo-fmt" => Some("rustfmt"), + _ => None, + } +} + mod errors; mod notifications; mod toolchain; diff --git a/tests/cli-misc.rs b/tests/cli-misc.rs index 7da8c84670..b4ffd9b09b 100644 --- a/tests/cli-misc.rs +++ b/tests/cli-misc.rs @@ -377,7 +377,7 @@ fn rustup_failed_path_search() { config, broken, &format!( - "toolchain 'custom' does not have the binary `fake_proxy{}`", + "'fake_proxy{}' is not installed for the toolchain 'custom'", EXE_SUFFIX ), ); @@ -550,9 +550,9 @@ fn rls_does_not_exist_in_toolchain() { config, &["rls", "--version"], &format!( - "toolchain 'stable-{}' does not have the binary `rls{}`", + "'rls{}' is not installed for the toolchain 'stable-{}'", + EXE_SUFFIX, this_host_triple(), - EXE_SUFFIX ), ); }); @@ -659,12 +659,12 @@ fn rename_rls_remove() { expect_ok(config, &["rustup", "component", "add", "rls"]); expect_ok(config, &["rls", "--version"]); expect_ok(config, &["rustup", "component", "remove", "rls"]); - expect_err(config, &["rls", "--version"], "does not have the binary `rls`"); + expect_err(config, &["rls", "--version"], "'rls' is not installed"); expect_ok(config, &["rustup", "component", "add", "rls"]); expect_ok(config, &["rls", "--version"]); expect_ok(config, &["rustup", "component", "remove", "rls-preview"]); - expect_err(config, &["rls", "--version"], "does not have the binary `rls`"); + expect_err(config, &["rls", "--version"], "'rls' is not installed"); }); } diff --git a/tests/cli-self-upd.rs b/tests/cli-self-upd.rs index bbc30b5b9a..e8e499fa63 100644 --- a/tests/cli-self-upd.rs +++ b/tests/cli-self-upd.rs @@ -1306,9 +1306,9 @@ fn rls_proxy_set_up_after_install() { config, &["rls", "--version"], &format!( - "toolchain 'stable-{}' does not have the binary `rls{}`", + "'rls{}' is not installed for the toolchain 'stable-{}'", + EXE_SUFFIX, this_host_triple(), - EXE_SUFFIX ), ); expect_ok(config, &["rustup", "component", "add", "rls"]); From 06af140083e3f593ca779f916dbe6cafa6d16aa1 Mon Sep 17 00:00:00 2001 From: Nick Cameron <nrc@ncameron.org> Date: Tue, 27 Nov 2018 13:52:54 +1300 Subject: [PATCH 8/8] fix unit tests --- src/rustup-dist/tests/dist.rs | 336 +++++++++++++----------------- src/rustup-dist/tests/manifest.rs | 4 +- 2 files changed, 146 insertions(+), 194 deletions(-) diff --git a/src/rustup-dist/tests/dist.rs b/src/rustup-dist/tests/dist.rs index eccf1dbeae..ce73ba3aff 100644 --- a/src/rustup-dist/tests/dist.rs +++ b/src/rustup-dist/tests/dist.rs @@ -233,8 +233,8 @@ fn bonus_component(name: &'static str, contents: Arc<Vec<u8>>) -> MockPackage { installer: MockInstallerBuilder { components: vec![ MockComponentBuilder { - name: format!("{}-x86_64-apple-darwin", name), - files: vec![MockFile::new_arc(&*format!("bin/{}", name), contents)], + name: "bonus-x86_64-apple-darwin".to_owned(), + files: vec![MockFile::new_arc("bin/bonus", contents)], }, ], }, @@ -337,57 +337,8 @@ fn rename_component() { assert!(!utils::path_exists(&prefix.path().join("bin/bobo"))); change_channel_date(url, "nightly", "2016-02-02"); update_from_dist(url, toolchain, prefix, &[], &[], download_cfg, temp_cfg).unwrap(); - assert!(!utils::path_exists(&prefix.path().join("bin/bonus"))); - assert!(utils::path_exists(&prefix.path().join("bin/bobo"))); - }, - ); -} - -// Test that a rename is ignored if the component with the new name is already installed. -#[test] -fn rename_component_ignore() { - let dist_tempdir = TempDir::new("rustup").unwrap(); - let ref url = Url::parse(&format!("file://{}", dist_tempdir.path().to_string_lossy())).unwrap(); - - let edit = &|_: &str, pkgs: &mut [MockPackage]| { - let tpkg = pkgs[0] - .targets - .iter_mut() - .find(|p| p.target == "x86_64-apple-darwin") - .unwrap(); - tpkg.components.push(MockComponent { - name: "bobo".to_string(), - target: "x86_64-apple-darwin".to_string(), - }); - }; - - let date_1 = "2016-02-01"; - let mut channel_1 = create_mock_channel("nightly", date_1, Some(edit)); - channel_1.packages[4] = bonus_component("bobo", Arc::new(date_1.as_bytes().to_vec())); - let date_2 = "2016-02-02"; - let mut channel_2 = create_mock_channel("nightly", date_2, Some(edit)); - channel_2.packages[4] = bonus_component("bobo", Arc::new(date_2.as_bytes().to_vec())); - channel_2 - .renames - .insert("bonus".to_owned(), "bobo".to_owned()); - let mock_dist_server = MockDistServer { - path: dist_tempdir.path().to_owned(), - channels: vec![channel_1, channel_2], - }; - - setup_from_dist_server( - mock_dist_server, - url, - false, - &|url, toolchain, prefix, download_cfg, temp_cfg| { - change_channel_date(url, "nightly", "2016-02-01"); - update_from_dist(url, toolchain, prefix, &[], &[], download_cfg, temp_cfg).unwrap(); - assert!(!utils::path_exists(&prefix.path().join("bin/bonus"))); - assert!(utils::path_exists(&prefix.path().join("bin/bobo"))); - change_channel_date(url, "nightly", "2016-02-02"); - update_from_dist(url, toolchain, prefix, &[], &[], download_cfg, temp_cfg).unwrap(); - assert!(!utils::path_exists(&prefix.path().join("bin/bonus"))); - assert!(utils::path_exists(&prefix.path().join("bin/bobo"))); + assert!(utils::path_exists(&prefix.path().join("bin/bonus"))); + assert!(!utils::path_exists(&prefix.path().join("bin/bobo"))); }, ); } @@ -435,8 +386,8 @@ fn rename_component_new() { assert!(!utils::path_exists(&prefix.path().join("bin/bobo"))); change_channel_date(url, "nightly", "2016-02-02"); update_from_dist(url, toolchain, prefix, &[], &[], download_cfg, temp_cfg).unwrap(); - assert!(!utils::path_exists(&prefix.path().join("bin/bonus"))); - assert!(utils::path_exists(&prefix.path().join("bin/bobo"))); + assert!(!utils::path_exists(&prefix.path().join("bin/bobo"))); + assert!(utils::path_exists(&prefix.path().join("bin/bonus"))); }, ); } @@ -518,8 +469,9 @@ fn uninstall( ) -> Result<()> { let trip = toolchain.target.clone(); let manifestation = Manifestation::open(prefix.clone(), trip)?; + let manifest = manifestation.load_manifest()?.unwrap(); - manifestation.uninstall(temp_cfg, notify_handler.clone())?; + manifestation.uninstall(&manifest, temp_cfg, notify_handler.clone())?; Ok(()) } @@ -748,14 +700,14 @@ fn update_preserves_extensions() { download_cfg, temp_cfg| { let ref adds = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-apple-darwin")), - }, - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-unknown-linux-gnu")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-apple-darwin")), + ), + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-unknown-linux-gnu")), + ), ]; change_channel_date(url, "nightly", "2016-02-01"); @@ -811,10 +763,10 @@ fn update_preserves_extensions_that_became_components() { false, &|url, toolchain, prefix, download_cfg, temp_cfg| { let ref adds = vec![ - Component { - pkg: "bonus".to_string(), - target: Some(TargetTriple::from_str("x86_64-apple-darwin")), - }, + Component::new( + "bonus".to_string(), + Some(TargetTriple::from_str("x86_64-apple-darwin")), + ), ]; change_channel_date(url, "nightly", "2016-02-01"); @@ -893,14 +845,14 @@ fn add_extensions_for_initial_install() { download_cfg, temp_cfg| { let ref adds = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-apple-darwin")), - }, - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-unknown-linux-gnu")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-apple-darwin")), + ), + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-unknown-linux-gnu")), + ), ]; update_from_dist(url, toolchain, prefix, adds, &[], download_cfg, temp_cfg).unwrap(); @@ -923,14 +875,14 @@ fn add_extensions_for_same_manifest() { update_from_dist(url, toolchain, prefix, &[], &[], download_cfg, temp_cfg).unwrap(); let ref adds = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-apple-darwin")), - }, - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-unknown-linux-gnu")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-apple-darwin")), + ), + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-unknown-linux-gnu")), + ), ]; update_from_dist(url, toolchain, prefix, adds, &[], download_cfg, temp_cfg).unwrap(); @@ -958,14 +910,14 @@ fn add_extensions_for_upgrade() { change_channel_date(url, "nightly", "2016-02-02"); let ref adds = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-apple-darwin")), - }, - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-unknown-linux-gnu")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-apple-darwin")), + ), + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-unknown-linux-gnu")), + ), ]; update_from_dist(url, toolchain, prefix, adds, &[], download_cfg, temp_cfg).unwrap(); @@ -988,10 +940,10 @@ fn add_extension_not_in_manifest() { download_cfg, temp_cfg| { let ref adds = vec![ - Component { - pkg: "rust-bogus".to_string(), - target: Some(TargetTriple::from_str("i686-apple-darwin")), - }, + Component::new( + "rust-bogus".to_string(), + Some(TargetTriple::from_str("i686-apple-darwin")), + ), ]; update_from_dist(url, toolchain, prefix, adds, &[], download_cfg, temp_cfg).unwrap(); @@ -1007,10 +959,10 @@ fn add_extension_that_is_required_component() { download_cfg, temp_cfg| { let ref adds = vec![ - Component { - pkg: "rustc".to_string(), - target: Some(TargetTriple::from_str("x86_64-apple-darwin")), - }, + Component::new( + "rustc".to_string(), + Some(TargetTriple::from_str("x86_64-apple-darwin")), + ), ]; update_from_dist(url, toolchain, prefix, adds, &[], download_cfg, temp_cfg).unwrap(); @@ -1035,10 +987,10 @@ fn add_extensions_does_not_remove_other_components() { update_from_dist(url, toolchain, prefix, &[], &[], download_cfg, temp_cfg).unwrap(); let ref adds = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-apple-darwin")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-apple-darwin")), + ), ]; update_from_dist(url, toolchain, prefix, adds, &[], download_cfg, temp_cfg).unwrap(); @@ -1057,10 +1009,10 @@ fn remove_extensions_for_initial_install() { download_cfg, temp_cfg| { let ref removes = vec![ - Component { - pkg: "rustc".to_string(), - target: Some(TargetTriple::from_str("x86_64-apple-darwin")), - }, + Component::new( + "rustc".to_string(), + Some(TargetTriple::from_str("x86_64-apple-darwin")), + ), ]; update_from_dist(url, toolchain, prefix, &[], removes, download_cfg, temp_cfg).unwrap(); @@ -1075,23 +1027,23 @@ fn remove_extensions_for_same_manifest() { download_cfg, temp_cfg| { let ref adds = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-apple-darwin")), - }, - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-unknown-linux-gnu")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-apple-darwin")), + ), + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-unknown-linux-gnu")), + ), ]; update_from_dist(url, toolchain, prefix, adds, &[], download_cfg, temp_cfg).unwrap(); let ref removes = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-apple-darwin")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-apple-darwin")), + ), ]; update_from_dist(url, toolchain, prefix, &[], removes, download_cfg, temp_cfg).unwrap(); @@ -1115,14 +1067,14 @@ fn remove_extensions_for_upgrade() { change_channel_date(url, "nightly", "2016-02-01"); let ref adds = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-apple-darwin")), - }, - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-unknown-linux-gnu")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-apple-darwin")), + ), + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-unknown-linux-gnu")), + ), ]; update_from_dist(url, toolchain, prefix, adds, &[], download_cfg, temp_cfg).unwrap(); @@ -1130,10 +1082,10 @@ fn remove_extensions_for_upgrade() { change_channel_date(url, "nightly", "2016-02-02"); let ref removes = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-apple-darwin")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-apple-darwin")), + ), ]; update_from_dist(url, toolchain, prefix, &[], removes, download_cfg, temp_cfg).unwrap(); @@ -1162,10 +1114,10 @@ fn remove_extension_not_in_manifest() { change_channel_date(url, "nightly", "2016-02-02"); let ref removes = vec![ - Component { - pkg: "rust-bogus".to_string(), - target: Some(TargetTriple::from_str("i686-apple-darwin")), - }, + Component::new( + "rust-bogus".to_string(), + Some(TargetTriple::from_str("i686-apple-darwin")), + ), ]; update_from_dist(url, toolchain, prefix, &[], removes, download_cfg, temp_cfg).unwrap(); @@ -1198,10 +1150,10 @@ fn remove_extension_not_in_manifest_but_is_already_installed() { change_channel_date(url, "nightly", "2016-02-01"); let ref adds = vec![ - Component { - pkg: "bonus".to_string(), - target: Some(TargetTriple::from_str("x86_64-apple-darwin")), - }, + Component::new( + "bonus".to_string(), + Some(TargetTriple::from_str("x86_64-apple-darwin")), + ), ]; update_from_dist(url, toolchain, prefix, adds, &[], download_cfg, temp_cfg).unwrap(); assert!(utils::path_exists(&prefix.path().join("bin/bonus"))); @@ -1209,10 +1161,10 @@ fn remove_extension_not_in_manifest_but_is_already_installed() { change_channel_date(url, "nightly", "2016-02-02"); let ref removes = vec![ - Component { - pkg: "bonus".to_string(), - target: Some(TargetTriple::from_str("x86_64-apple-darwin")), - }, + Component::new( + "bonus".to_string(), + Some(TargetTriple::from_str("x86_64-apple-darwin")), + ), ]; update_from_dist(url, toolchain, prefix, &[], removes, download_cfg, temp_cfg).unwrap(); }, @@ -1230,10 +1182,10 @@ fn remove_extension_that_is_required_component() { update_from_dist(url, toolchain, prefix, &[], &[], download_cfg, temp_cfg).unwrap(); let ref removes = vec![ - Component { - pkg: "rustc".to_string(), - target: Some(TargetTriple::from_str("x86_64-apple-darwin")), - }, + Component::new( + "rustc".to_string(), + Some(TargetTriple::from_str("x86_64-apple-darwin")), + ), ]; update_from_dist(url, toolchain, prefix, &[], removes, download_cfg, temp_cfg).unwrap(); @@ -1251,10 +1203,10 @@ fn remove_extension_not_installed() { update_from_dist(url, toolchain, prefix, &[], &[], download_cfg, temp_cfg).unwrap(); let ref removes = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-apple-darwin")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-apple-darwin")), + ), ]; update_from_dist(url, toolchain, prefix, &[], removes, download_cfg, temp_cfg).unwrap(); @@ -1273,19 +1225,19 @@ fn remove_extensions_does_not_remove_other_components() { download_cfg, temp_cfg| { let ref adds = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-apple-darwin")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-apple-darwin")), + ), ]; update_from_dist(url, toolchain, prefix, adds, &[], download_cfg, temp_cfg).unwrap(); let ref removes = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-apple-darwin")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-apple-darwin")), + ), ]; update_from_dist(url, toolchain, prefix, &[], removes, download_cfg, temp_cfg).unwrap(); @@ -1304,10 +1256,10 @@ fn add_and_remove_for_upgrade() { change_channel_date(url, "nightly", "2016-02-01"); let ref adds = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-unknown-linux-gnu")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-unknown-linux-gnu")), + ), ]; update_from_dist(url, toolchain, prefix, adds, &[], download_cfg, temp_cfg).unwrap(); @@ -1315,17 +1267,17 @@ fn add_and_remove_for_upgrade() { change_channel_date(url, "nightly", "2016-02-02"); let ref adds = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-apple-darwin")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-apple-darwin")), + ), ]; let ref removes = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-unknown-linux-gnu")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-unknown-linux-gnu")), + ), ]; update_from_dist( @@ -1355,26 +1307,26 @@ fn add_and_remove() { download_cfg, temp_cfg| { let ref adds = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-unknown-linux-gnu")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-unknown-linux-gnu")), + ), ]; update_from_dist(url, toolchain, prefix, adds, &[], download_cfg, temp_cfg).unwrap(); let ref adds = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-apple-darwin")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-apple-darwin")), + ), ]; let ref removes = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-unknown-linux-gnu")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-unknown-linux-gnu")), + ), ]; update_from_dist( @@ -1407,17 +1359,17 @@ fn add_and_remove_same_component() { update_from_dist(url, toolchain, prefix, &[], &[], download_cfg, temp_cfg).unwrap(); let ref adds = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-apple-darwin")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-apple-darwin")), + ), ]; let ref removes = vec![ - Component { - pkg: "rust-std".to_string(), - target: Some(TargetTriple::from_str("i686-apple_darwin")), - }, + Component::new( + "rust-std".to_string(), + Some(TargetTriple::from_str("i686-apple_darwin")), + ), ]; update_from_dist( diff --git a/src/rustup-dist/tests/manifest.rs b/src/rustup-dist/tests/manifest.rs index 1a91630398..341400f929 100644 --- a/src/rustup-dist/tests/manifest.rs +++ b/src/rustup-dist/tests/manifest.rs @@ -33,11 +33,11 @@ fn parse_smoke_test() { assert_eq!(rust_target_pkg.bins.clone().unwrap().hash, "..."); let ref component = rust_target_pkg.components[0]; - assert_eq!(component.pkg, "rustc"); + assert_eq!(component.name_in_manifest(), "rustc"); assert_eq!(component.target.as_ref(), Some(&x86_64_unknown_linux_gnu)); let ref component = rust_target_pkg.extensions[0]; - assert_eq!(component.pkg, "rust-std"); + assert_eq!(component.name_in_manifest(), "rust-std"); assert_eq!(component.target.as_ref(), Some(&x86_64_unknown_linux_musl)); let docs_pkg = pkg.get_package("rust-docs").unwrap();