From 2d7ae28742662eca86774216e1b43ac8b87a01f5 Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Thu, 23 Jul 2020 23:33:38 +0200 Subject: [PATCH 1/6] Accept TOML format in rust-toolchain --- README.md | 18 ++- src/cli/common.rs | 12 +- src/cli/rustup_mode.rs | 2 +- src/cli/setup_mode.rs | 2 +- src/config.rs | 260 +++++++++++++++++++++++++++++++++++++---- src/errors.rs | 3 + tests/cli-rustup.rs | 168 ++++++++++++++++++++++++++ 7 files changed, 434 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index b24f4f0219..8eb7efcb54 100644 --- a/README.md +++ b/README.md @@ -404,9 +404,21 @@ source repository. This is most often the case for nightly-only software that pins to a revision from the release archives. In these cases the toolchain can be named in the project's directory -in a file called `rust-toolchain`, the content of which is the name of -a single `rustup` toolchain, and which is suitable to check in to -source control. This file has to be encoded in US-ASCII (if you are on +in a file called `rust-toolchain`, the content of which is either the name of +a single `rustup` toolchain, or a TOML file with the following layout: + +``` toml +[toolchain] +channel = "nightly-2020-07-10" +components = [ "rustfmt", "rustc-dev" ] +targets = [ "wasm32-unknown-unknown", "thumbv2-none-eabi" ] +``` + +If the TOML format is used, the `[toolchain]` section and the `channel` entry +are mandatory. + +The `rust-toolchain` file is suitable to check in to source control. +This file has to be encoded in US-ASCII (if you are on Windows, check the encoding and that it does not starts with a BOM). The toolchains named in this file have a more restricted form than diff --git a/src/cli/common.rs b/src/cli/common.rs index 6ecbaf4e65..11fce94bbb 100644 --- a/src/cli/common.rs +++ b/src/cli/common.rs @@ -480,12 +480,12 @@ pub fn list_toolchains(cfg: &Cfg, verbose: bool) -> Result { String::new() }; let cwd = utils::current_dir()?; - let ovr_toolchain_name = if let Ok(Some((ovr_toolchain, _reason))) = cfg.find_override(&cwd) - { - ovr_toolchain.name().to_string() - } else { - String::new() - }; + let ovr_toolchain_name = + if let Ok(Some((toolchain, _override_cfg, _reason))) = cfg.find_override(&cwd) { + toolchain.name().to_string() + } else { + String::new() + }; for toolchain in toolchains { let if_default = if def_toolchain_name == &*toolchain { " (default)" diff --git a/src/cli/rustup_mode.rs b/src/cli/rustup_mode.rs index f0d7169a05..4add233080 100644 --- a/src/cli/rustup_mode.rs +++ b/src/cli/rustup_mode.rs @@ -786,7 +786,7 @@ fn default_(cfg: &Cfg, m: &ArgMatches<'_>) -> Result { } let cwd = utils::current_dir()?; - if let Some((toolchain, reason)) = cfg.find_override(&cwd)? { + if let Some((toolchain, _override_cfg, reason)) = cfg.find_override(&cwd)? { info!( "note that the toolchain '{}' is currently in use ({})", toolchain.name(), diff --git a/src/cli/setup_mode.rs b/src/cli/setup_mode.rs index 9bebcff3d2..a22f54df87 100644 --- a/src/cli/setup_mode.rs +++ b/src/cli/setup_mode.rs @@ -16,7 +16,7 @@ pub fn main() -> Result { return self_update::self_replace(); } - // Internal testament dump used during CI. Not for users. + // Internal testament dump used during CI. Not for users. if arg1 == Some("--dump-testament") { common::dump_testament()?; return Ok(utils::ExitCode(0)); diff --git a/src/config.rs b/src/config.rs index 4e82c5bbb6..515a610b6f 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,12 +1,14 @@ use std::borrow::Cow; use std::fmt::{self, Display}; use std::io; +use std::mem; use std::path::{Path, PathBuf}; use std::process::Command; use std::str::FromStr; use std::sync::Arc; use pgp::{Deserializable, SignedPublicKey}; +use serde::Deserialize; use crate::dist::download::DownloadCfg; use crate::dist::{dist, temp}; @@ -18,6 +20,27 @@ use crate::settings::{Settings, SettingsFile, DEFAULT_METADATA_VERSION}; use crate::toolchain::{DistributableToolchain, Toolchain, UpdateStatus}; use crate::utils::utils; +#[derive(Debug, Default, Deserialize, PartialEq, Eq)] +struct OverrideFile { + toolchain: ToolchainSection, +} + +#[derive(Debug, Default, Deserialize, PartialEq, Eq)] +struct ToolchainSection { + channel: String, + components: Option>, + targets: Option>, +} + +impl> From for ToolchainSection { + fn from(channel: T) -> Self { + Self { + channel: channel.into(), + ..Default::default() + } + } +} + #[derive(Debug)] pub enum OverrideReason { Environment, @@ -37,6 +60,21 @@ impl Display for OverrideReason { } } +#[derive(Debug, Default)] +pub struct OverrideCfg { + components: Vec, + targets: Vec, +} + +impl From for OverrideCfg { + fn from(file: OverrideFile) -> Self { + Self { + components: file.toolchain.components.unwrap_or_default(), + targets: file.toolchain.targets.unwrap_or_default(), + } + } +} + lazy_static::lazy_static! { static ref BUILTIN_PGP_KEY: SignedPublicKey = pgp::SignedPublicKey::from_armor_single( io::Cursor::new(&include_bytes!("rust-key.pgp.ascii")[..]) @@ -401,17 +439,28 @@ impl Cfg { } } - pub fn find_override(&self, path: &Path) -> Result, OverrideReason)>> { + pub fn find_override( + &self, + path: &Path, + ) -> Result, OverrideCfg, OverrideReason)>> { let mut override_ = None; // First check toolchain override from command if let Some(ref name) = self.toolchain_override { - override_ = Some((name.to_string(), OverrideReason::CommandLine)); + override_ = Some(( + name.to_string(), + OverrideCfg::default(), + OverrideReason::CommandLine, + )); } // Check RUSTUP_TOOLCHAIN if let Some(ref name) = self.env_override { - override_ = Some((name.to_string(), OverrideReason::Environment)); + override_ = Some(( + name.to_string(), + OverrideCfg::default(), + OverrideReason::Environment, + )); } // Then walk up the directory tree from 'path' looking for either the @@ -424,7 +473,7 @@ impl Cfg { })?; } - if let Some((name, reason)) = override_ { + if let Some((name, override_cfg, reason)) = override_ { // This is hackishly using the error chain to provide a bit of // extra context about what went wrong. The CLI will display it // on a line after the proximate error. @@ -457,9 +506,9 @@ impl Cfg { // Strip the confusing NotADirectory error and only mention that the // override toolchain is not installed. Err(Error::from(reason_err)) - .chain_err(|| ErrorKind::OverrideToolchainNotInstalled(name.to_string())) + .chain_err(|| ErrorKind::OverrideToolchainNotInstalled(toolchain.name().into())) } else { - Ok(Some((toolchain, reason))) + Ok(Some((toolchain, override_cfg, reason))) } } else { Ok(None) @@ -470,7 +519,7 @@ impl Cfg { &self, dir: &Path, settings: &Settings, - ) -> Result> { + ) -> Result> { let notify = self.notify_handler.as_ref(); let dir = utils::canonicalize_path(dir, notify); let mut dir = Some(&*dir); @@ -479,16 +528,16 @@ impl Cfg { // First check the override database if let Some(name) = settings.dir_override(d, notify) { let reason = OverrideReason::OverrideDB(d.to_owned()); - return Ok(Some((name, reason))); + return Ok(Some((name, OverrideCfg::default(), reason))); } // Then look for 'rust-toolchain' let toolchain_file = d.join("rust-toolchain"); - if let Ok(s) = utils::read_file("toolchain file", &toolchain_file) { - if let Some(s) = s.lines().next() { - let toolchain_name = s.trim(); + if let Ok(contents) = utils::read_file("toolchain file", &toolchain_file) { + if let Some(mut override_file) = Cfg::parse_override_file(contents)? { + let toolchain_name = mem::take(&mut override_file.toolchain.channel); let all_toolchains = self.list_toolchains()?; - if !all_toolchains.iter().any(|s| s == toolchain_name) { + if !all_toolchains.iter().any(|s| s == &toolchain_name) { // The given name is not resolvable as a toolchain, so // instead check it's plausible for installation later dist::validate_channel_name(&toolchain_name).chain_err(|| { @@ -500,7 +549,7 @@ impl Cfg { })?; } let reason = OverrideReason::ToolchainFile(toolchain_file); - return Ok(Some((toolchain_name.to_string(), reason))); + return Ok(Some((toolchain_name, override_file.into(), reason))); } } @@ -510,26 +559,74 @@ impl Cfg { Ok(None) } + fn parse_override_file>(contents: S) -> Result> { + let contents = contents.as_ref(); + + if contents.lines().any(|line| line.starts_with("[toolchain]")) { + toml::from_str::(contents) + .map(|file| Some(file)) + .map_err(|e| ErrorKind::ParsingOverride(e).into()) + } else { + Ok(contents.lines().next().map(|line| OverrideFile { + toolchain: line.trim().into(), + })) + } + } + pub fn find_or_install_override_toolchain_or_default( &self, path: &Path, ) -> Result<(Toolchain<'_>, Option)> { - if let Some((toolchain, reason)) = - if let Some((toolchain, reason)) = self.find_override(path)? { - Some((toolchain, Some(reason))) + fn components_exist( + distributable: &DistributableToolchain<'_>, + components: &[&str], + targets: &[&str], + ) -> Result { + let components_requested = !components.is_empty() || !targets.is_empty(); + + match (distributable.list_components(), components_requested) { + // If the toolchain does not support components but there were components requested, bubble up the error + (Err(e), true) => Err(e), + (Ok(installed_components), _) => { + Ok(components.iter().chain(targets.iter()).all(|name| { + installed_components.iter().any(|status| { + status.component.short_name_in_manifest() == name && status.installed + }) + })) + } + _ => Ok(true), + } + } + + if let Some((toolchain, override_cfg, reason)) = + if let Some((toolchain, override_cfg, reason)) = self.find_override(path)? { + Some((toolchain, Some(override_cfg), Some(reason))) } else { - self.find_default()?.map(|toolchain| (toolchain, None)) + self.find_default()? + .map(|toolchain| (toolchain, None, None)) } { - if !toolchain.exists() { - if toolchain.is_custom() { + if toolchain.is_custom() { + if !toolchain.exists() { return Err( ErrorKind::ToolchainNotInstalled(toolchain.name().to_string()).into(), ); } + } else { + let components = override_cfg.as_ref().map_or(Vec::new(), |cfg| { + cfg.components.iter().map(AsRef::as_ref).collect() + }); + let targets = override_cfg.as_ref().map_or(Vec::new(), |cfg| { + cfg.targets.iter().map(AsRef::as_ref).collect() + }); + let distributable = DistributableToolchain::new(&toolchain)?; - distributable.install_from_dist(true, false, &[], &[])?; + if !toolchain.exists() || !components_exist(&distributable, &components, &targets)? + { + distributable.install_from_dist(true, false, &components, &targets)?; + } } + Ok((toolchain, reason)) } else { // No override and no default set @@ -722,3 +819,126 @@ impl Cfg { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parse_legacy_toolchain_file() { + let contents = "nightly-2020-07-10"; + + let result = Cfg::parse_override_file(contents); + assert_eq!( + result.unwrap(), + Some(OverrideFile { + toolchain: ToolchainSection::from(contents) + }) + ); + } + + #[test] + fn parse_toml_toolchain_file() { + let contents = r#" +[toolchain] +channel = "nightly-2020-07-10" +components = [ "rustfmt", "rustc-dev" ] +targets = [ "wasm32-unknown-unknown", "thumbv2-none-eabi" ] +"#; + + let result = Cfg::parse_override_file(contents); + assert_eq!( + result.unwrap(), + Some(OverrideFile { + toolchain: ToolchainSection { + channel: "nightly-2020-07-10".into(), + components: Some(vec!["rustfmt".into(), "rustc-dev".into()]), + targets: Some(vec![ + "wasm32-unknown-unknown".into(), + "thumbv2-none-eabi".into() + ]), + } + }) + ); + } + + #[test] + fn parse_toml_toolchain_file_only_channel() { + let contents = r#" +[toolchain] +channel = "nightly-2020-07-10" +"#; + + let result = Cfg::parse_override_file(contents); + assert_eq!( + result.unwrap(), + Some(OverrideFile { + toolchain: ToolchainSection { + channel: "nightly-2020-07-10".into(), + components: None, + targets: None, + } + }) + ); + } + + #[test] + fn parse_toml_toolchain_file_empty_components() { + let contents = r#" +[toolchain] +channel = "nightly-2020-07-10" +components = [] +"#; + + let result = Cfg::parse_override_file(contents); + assert_eq!( + result.unwrap(), + Some(OverrideFile { + toolchain: ToolchainSection { + channel: "nightly-2020-07-10".into(), + components: Some(vec![]), + targets: None, + } + }) + ); + } + + #[test] + fn parse_toml_toolchain_file_empty_targets() { + let contents = r#" +[toolchain] +channel = "nightly-2020-07-10" +targets = [] +"#; + + let result = Cfg::parse_override_file(contents); + assert_eq!( + result.unwrap(), + Some(OverrideFile { + toolchain: ToolchainSection { + channel: "nightly-2020-07-10".into(), + components: None, + targets: Some(vec![]), + } + }) + ); + } + + #[test] + fn parse_empty_toml_toolchain_file() { + let contents = r#" +[toolchain] +"#; + + let result = Cfg::parse_override_file(contents); + assert!(result.is_err()); + } + + #[test] + fn parse_empty_toolchain_file() { + let contents = ""; + + let result = Cfg::parse_override_file(contents); + assert_eq!(result.unwrap(), None); + } +} diff --git a/src/errors.rs b/src/errors.rs index 71d24a3442..f67c63b1ab 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -356,6 +356,9 @@ error_chain! { ParsingSettings(e: toml::de::Error) { description("error parsing settings") } + ParsingOverride(e: toml::de::Error) { + description("error parsing override file") + } NoExeName { description("couldn't determine self executable name") } diff --git a/tests/cli-rustup.rs b/tests/cli-rustup.rs index 2e393a9c6c..59ea45be74 100644 --- a/tests/cli-rustup.rs +++ b/tests/cli-rustup.rs @@ -1402,6 +1402,174 @@ fn file_override_with_archive() { }); } +#[test] +fn file_override_toml_format() { + setup(&|config| { + expect_ok(config, &["rustup", "default", "stable"]); + expect_ok( + config, + &[ + "rustup", + "toolchain", + "install", + "nightly-2015-01-01", + "--no-self-update", + ], + ); + + expect_stdout_ok(config, &["rustc", "--version"], "hash-stable-1.1.0"); + + let cwd = config.current_dir(); + let toolchain_file = cwd.join("rust-toolchain"); + raw::write_file( + &toolchain_file, + r#" +[toolchain] +channel = "nightly-2015-01-01" +"#, + ) + .unwrap(); + + expect_stdout_ok(config, &["rustc", "--version"], "hash-nightly-1"); + }); +} + +#[test] +fn file_override_toml_format_add_components_toolchain_not_installed() { + setup(&|config| { + expect_ok(config, &["rustup", "default", "stable"]); + + let cwd = config.current_dir(); + let toolchain_file = cwd.join("rust-toolchain"); + raw::write_file( + &toolchain_file, + r#" +[toolchain] +channel = "nightly-2015-01-01" +components = [ "rust-src" ] +"#, + ) + .unwrap(); + + expect_stdout_ok(config, &["rustc", "--version"], "hash-nightly-1"); + expect_stdout_ok( + config, + &["rustup", "component", "list"], + "rust-src (installed)", + ); + }); +} + +#[test] +fn file_override_toml_format_add_components_toolchain_misses_components() { + setup(&|config| { + expect_ok(config, &["rustup", "default", "stable"]); + expect_ok( + config, + &[ + "rustup", + "toolchain", + "install", + "nightly-2015-01-01", + "--no-self-update", + ], + ); + + expect_stdout_ok(config, &["rustc", "--version"], "hash-stable-1.1.0"); + + let cwd = config.current_dir(); + let toolchain_file = cwd.join("rust-toolchain"); + raw::write_file( + &toolchain_file, + r#" +[toolchain] +channel = "nightly-2015-01-01" +components = [ "rust-src" ] +"#, + ) + .unwrap(); + + expect_stdout_ok(config, &["rustc", "--version"], "hash-nightly-1"); + expect_stdout_ok( + config, + &["rustup", "component", "list"], + "rust-src (installed)", + ); + }); +} + +#[test] +fn file_override_toml_format_add_components_invalid_component() { + setup(&|config| { + expect_ok(config, &["rustup", "default", "stable"]); + expect_ok( + config, + &[ + "rustup", + "toolchain", + "install", + "nightly-2015-01-01", + "--no-self-update", + ], + ); + + expect_stdout_ok(config, &["rustc", "--version"], "hash-stable-1.1.0"); + + let cwd = config.current_dir(); + let toolchain_file = cwd.join("rust-toolchain"); + raw::write_file( + &toolchain_file, + r#" +[toolchain] +channel = "nightly-2015-01-01" +components = [ "rust-bongo" ] +"#, + ) + .unwrap(); + + // The toolchain should be installed and the invalid component skipped + expect_stdout_ok(config, &["rustc", "--version"], "hash-nightly-1"); + }); +} + +#[test] +fn file_override_toml_format_add_components_toolchain_misses_target() { + setup(&|config| { + expect_ok(config, &["rustup", "default", "stable"]); + expect_ok( + config, + &[ + "rustup", + "toolchain", + "install", + "nightly-2015-01-01", + "--no-self-update", + ], + ); + + expect_stdout_ok(config, &["rustc", "--version"], "hash-stable-1.1.0"); + + let cwd = config.current_dir(); + let toolchain_file = cwd.join("rust-toolchain"); + raw::write_file( + &toolchain_file, + r#" +[toolchain] +channel = "nightly-2015-01-01" +targets = [ "arm-linux-androideabi" ] +"#, + ) + .unwrap(); + + expect_stdout_ok(config, &["rustc", "--version"], "hash-nightly-1"); + expect_stdout_ok( + config, + &["rustup", "component", "list"], + "arm-linux-androideabi (installed)", + ); + }); +} + #[test] fn directory_override_beats_file_override() { setup(&|config| { From c01846ed303c03472997918a21f0eb94cbf55b9d Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Mon, 27 Jul 2020 23:15:56 +0200 Subject: [PATCH 2/6] Apply suggestions from PR review - Make channel optional in the TOML toolchain file. - Avoid exposing OverrideCfg outside the `config` module. - Rework tests to avoid installing multiple toolchains unnecessarily and improve test naming to express better the intent. - Restore unrelated extra whitespace. --- src/cli/common.rs | 11 +-- src/cli/rustup_mode.rs | 2 +- src/cli/setup_mode.rs | 2 +- src/config.rs | 208 +++++++++++++++++++++++++++-------------- tests/cli-rustup.rs | 80 ++++++---------- 5 files changed, 174 insertions(+), 129 deletions(-) diff --git a/src/cli/common.rs b/src/cli/common.rs index 11fce94bbb..619b5da07a 100644 --- a/src/cli/common.rs +++ b/src/cli/common.rs @@ -480,12 +480,11 @@ pub fn list_toolchains(cfg: &Cfg, verbose: bool) -> Result { String::new() }; let cwd = utils::current_dir()?; - let ovr_toolchain_name = - if let Ok(Some((toolchain, _override_cfg, _reason))) = cfg.find_override(&cwd) { - toolchain.name().to_string() - } else { - String::new() - }; + let ovr_toolchain_name = if let Ok(Some((toolchain, _reason))) = cfg.find_override(&cwd) { + toolchain.name().to_string() + } else { + String::new() + }; for toolchain in toolchains { let if_default = if def_toolchain_name == &*toolchain { " (default)" diff --git a/src/cli/rustup_mode.rs b/src/cli/rustup_mode.rs index 4add233080..f0d7169a05 100644 --- a/src/cli/rustup_mode.rs +++ b/src/cli/rustup_mode.rs @@ -786,7 +786,7 @@ fn default_(cfg: &Cfg, m: &ArgMatches<'_>) -> Result { } let cwd = utils::current_dir()?; - if let Some((toolchain, _override_cfg, reason)) = cfg.find_override(&cwd)? { + if let Some((toolchain, reason)) = cfg.find_override(&cwd)? { info!( "note that the toolchain '{}' is currently in use ({})", toolchain.name(), diff --git a/src/cli/setup_mode.rs b/src/cli/setup_mode.rs index a22f54df87..9bebcff3d2 100644 --- a/src/cli/setup_mode.rs +++ b/src/cli/setup_mode.rs @@ -16,7 +16,7 @@ pub fn main() -> Result { return self_update::self_replace(); } - // Internal testament dump used during CI. Not for users. + // Internal testament dump used during CI. Not for users. if arg1 == Some("--dump-testament") { common::dump_testament()?; return Ok(utils::ExitCode(0)); diff --git a/src/config.rs b/src/config.rs index 515a610b6f..6c67d719f0 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,7 +1,6 @@ use std::borrow::Cow; use std::fmt::{self, Display}; use std::io; -use std::mem; use std::path::{Path, PathBuf}; use std::process::Command; use std::str::FromStr; @@ -25,22 +24,30 @@ struct OverrideFile { toolchain: ToolchainSection, } +impl OverrideFile { + fn is_empty(&self) -> bool { + self.toolchain.is_empty() + } +} + #[derive(Debug, Default, Deserialize, PartialEq, Eq)] struct ToolchainSection { - channel: String, + channel: Option, components: Option>, targets: Option>, } -impl> From for ToolchainSection { - fn from(channel: T) -> Self { - Self { - channel: channel.into(), - ..Default::default() - } +impl ToolchainSection { + fn is_empty(&self) -> bool { + self.channel.is_none() && self.components.is_none() && self.targets.is_none() } } +enum OverrideKind { + Name(String), + File(OverrideFile), +} + #[derive(Debug)] pub enum OverrideReason { Environment, @@ -60,17 +67,28 @@ impl Display for OverrideReason { } } -#[derive(Debug, Default)] -pub struct OverrideCfg { +#[derive(Default)] +struct OverrideCfg<'a> { + toolchain: Option>, components: Vec, targets: Vec, } -impl From for OverrideCfg { - fn from(file: OverrideFile) -> Self { - Self { - components: file.toolchain.components.unwrap_or_default(), - targets: file.toolchain.targets.unwrap_or_default(), +impl<'a> OverrideCfg<'a> { + fn from_kind(cfg: &'a Cfg, kind: OverrideKind) -> Result { + match kind { + OverrideKind::Name(name) => Ok(Self { + toolchain: Some(Toolchain::from(cfg, &name)?), + ..Default::default() + }), + OverrideKind::File(file) => Ok(Self { + toolchain: match file.toolchain.channel { + Some(name) => Some(Toolchain::from(cfg, &name)?), + None => None, + }, + components: file.toolchain.components.unwrap_or_default(), + targets: file.toolchain.targets.unwrap_or_default(), + }), } } } @@ -439,17 +457,24 @@ impl Cfg { } } - pub fn find_override( + pub fn find_override(&self, path: &Path) -> Result, OverrideReason)>> { + self.find_override_config(path).map(|opt| { + opt.and_then(|(override_cfg, reason)| { + override_cfg.toolchain.map(|toolchain| (toolchain, reason)) + }) + }) + } + + fn find_override_config( &self, path: &Path, - ) -> Result, OverrideCfg, OverrideReason)>> { + ) -> Result, OverrideReason)>> { let mut override_ = None; // First check toolchain override from command if let Some(ref name) = self.toolchain_override { override_ = Some(( - name.to_string(), - OverrideCfg::default(), + OverrideKind::Name(name.to_string()), OverrideReason::CommandLine, )); } @@ -457,8 +482,7 @@ impl Cfg { // Check RUSTUP_TOOLCHAIN if let Some(ref name) = self.env_override { override_ = Some(( - name.to_string(), - OverrideCfg::default(), + OverrideKind::Name(name.to_string()), OverrideReason::Environment, )); } @@ -473,7 +497,7 @@ impl Cfg { })?; } - if let Some((name, override_cfg, reason)) = override_ { + if let Some((kind, reason)) = override_ { // This is hackishly using the error chain to provide a bit of // extra context about what went wrong. The CLI will display it // on a line after the proximate error. @@ -497,19 +521,22 @@ impl Cfg { ), }; - let toolchain = Toolchain::from(self, &name)?; - // Overridden toolchains can be literally any string, but only - // distributable toolchains will be auto-installed by the wrapping - // code; provide a nice error for this common case. (default could - // be set badly too, but that is much less common). - if !toolchain.exists() && toolchain.is_custom() { - // Strip the confusing NotADirectory error and only mention that the - // override toolchain is not installed. - Err(Error::from(reason_err)) - .chain_err(|| ErrorKind::OverrideToolchainNotInstalled(toolchain.name().into())) - } else { - Ok(Some((toolchain, override_cfg, reason))) + let override_cfg = OverrideCfg::from_kind(self, kind)?; + if let Some(toolchain) = &override_cfg.toolchain { + // Overridden toolchains can be literally any string, but only + // distributable toolchains will be auto-installed by the wrapping + // code; provide a nice error for this common case. (default could + // be set badly too, but that is much less common). + if !toolchain.exists() && toolchain.is_custom() { + // Strip the confusing NotADirectory error and only mention that the + // override toolchain is not installed. + return Err(Error::from(reason_err)).chain_err(|| { + ErrorKind::OverrideToolchainNotInstalled(toolchain.name().into()) + }); + } } + + Ok(Some((override_cfg, reason))) } else { Ok(None) } @@ -519,7 +546,7 @@ impl Cfg { &self, dir: &Path, settings: &Settings, - ) -> Result> { + ) -> Result> { let notify = self.notify_handler.as_ref(); let dir = utils::canonicalize_path(dir, notify); let mut dir = Some(&*dir); @@ -528,28 +555,30 @@ impl Cfg { // First check the override database if let Some(name) = settings.dir_override(d, notify) { let reason = OverrideReason::OverrideDB(d.to_owned()); - return Ok(Some((name, OverrideCfg::default(), reason))); + return Ok(Some((OverrideKind::Name(name), reason))); } // Then look for 'rust-toolchain' let toolchain_file = d.join("rust-toolchain"); if let Ok(contents) = utils::read_file("toolchain file", &toolchain_file) { - if let Some(mut override_file) = Cfg::parse_override_file(contents)? { - let toolchain_name = mem::take(&mut override_file.toolchain.channel); - let all_toolchains = self.list_toolchains()?; - if !all_toolchains.iter().any(|s| s == &toolchain_name) { - // The given name is not resolvable as a toolchain, so - // instead check it's plausible for installation later - dist::validate_channel_name(&toolchain_name).chain_err(|| { - format!( - "invalid channel name '{}' in '{}'", - toolchain_name, - toolchain_file.display() - ) - })?; + if let Some(override_file) = Cfg::parse_override_file(contents)? { + if let Some(toolchain_name) = &override_file.toolchain.channel { + let all_toolchains = self.list_toolchains()?; + if !all_toolchains.iter().any(|s| s == toolchain_name) { + // The given name is not resolvable as a toolchain, so + // instead check it's plausible for installation later + dist::validate_channel_name(&toolchain_name).chain_err(|| { + format!( + "invalid channel name '{}' in '{}'", + toolchain_name, + toolchain_file.display() + ) + })?; + } } + let reason = OverrideReason::ToolchainFile(toolchain_file); - return Ok(Some((toolchain_name, override_file.into(), reason))); + return Ok(Some((OverrideKind::File(override_file), reason))); } } @@ -564,11 +593,14 @@ impl Cfg { if contents.lines().any(|line| line.starts_with("[toolchain]")) { toml::from_str::(contents) - .map(|file| Some(file)) + .map(|file| if file.is_empty() { None } else { Some(file) }) .map_err(|e| ErrorKind::ParsingOverride(e).into()) } else { Ok(contents.lines().next().map(|line| OverrideFile { - toolchain: line.trim().into(), + toolchain: ToolchainSection { + channel: Some(line.trim().into()), + ..Default::default() + }, })) } } @@ -598,12 +630,29 @@ impl Cfg { } } - if let Some((toolchain, override_cfg, reason)) = - if let Some((toolchain, override_cfg, reason)) = self.find_override(path)? { - Some((toolchain, Some(override_cfg), Some(reason))) - } else { - self.find_default()? - .map(|toolchain| (toolchain, None, None)) + if let Some((toolchain, components, targets, reason)) = + match self.find_override_config(path)? { + Some(( + OverrideCfg { + toolchain, + components, + targets, + }, + reason, + )) => { + let default = if toolchain.is_none() { + self.find_default()? + } else { + None + }; + + toolchain + .or(default) + .map(|toolchain| (toolchain, components, targets, Some(reason))) + } + None => self + .find_default()? + .map(|toolchain| (toolchain, vec![], vec![], None)), } { if toolchain.is_custom() { @@ -613,12 +662,8 @@ impl Cfg { ); } } else { - let components = override_cfg.as_ref().map_or(Vec::new(), |cfg| { - cfg.components.iter().map(AsRef::as_ref).collect() - }); - let targets = override_cfg.as_ref().map_or(Vec::new(), |cfg| { - cfg.targets.iter().map(AsRef::as_ref).collect() - }); + let components: Vec<_> = components.iter().map(AsRef::as_ref).collect(); + let targets: Vec<_> = targets.iter().map(AsRef::as_ref).collect(); let distributable = DistributableToolchain::new(&toolchain)?; if !toolchain.exists() || !components_exist(&distributable, &components, &targets)? @@ -832,7 +877,10 @@ mod tests { assert_eq!( result.unwrap(), Some(OverrideFile { - toolchain: ToolchainSection::from(contents) + toolchain: ToolchainSection { + channel: Some(contents.into()), + ..Default::default() + } }) ); } @@ -851,7 +899,7 @@ targets = [ "wasm32-unknown-unknown", "thumbv2-none-eabi" ] result.unwrap(), Some(OverrideFile { toolchain: ToolchainSection { - channel: "nightly-2020-07-10".into(), + channel: Some("nightly-2020-07-10".into()), components: Some(vec!["rustfmt".into(), "rustc-dev".into()]), targets: Some(vec![ "wasm32-unknown-unknown".into(), @@ -874,7 +922,7 @@ channel = "nightly-2020-07-10" result.unwrap(), Some(OverrideFile { toolchain: ToolchainSection { - channel: "nightly-2020-07-10".into(), + channel: Some("nightly-2020-07-10".into()), components: None, targets: None, } @@ -895,7 +943,7 @@ components = [] result.unwrap(), Some(OverrideFile { toolchain: ToolchainSection { - channel: "nightly-2020-07-10".into(), + channel: Some("nightly-2020-07-10".into()), components: Some(vec![]), targets: None, } @@ -916,7 +964,7 @@ targets = [] result.unwrap(), Some(OverrideFile { toolchain: ToolchainSection { - channel: "nightly-2020-07-10".into(), + channel: Some("nightly-2020-07-10".into()), components: None, targets: Some(vec![]), } @@ -924,6 +972,26 @@ targets = [] ); } + #[test] + fn parse_toml_toolchain_file_no_channel() { + let contents = r#" +[toolchain] +components = [ "rustfmt" ] +"#; + + let result = Cfg::parse_override_file(contents); + assert_eq!( + result.unwrap(), + Some(OverrideFile { + toolchain: ToolchainSection { + channel: None, + components: Some(vec!["rustfmt".into()]), + targets: None, + } + }) + ); + } + #[test] fn parse_empty_toml_toolchain_file() { let contents = r#" @@ -931,7 +999,7 @@ targets = [] "#; let result = Cfg::parse_override_file(contents); - assert!(result.is_err()); + assert_eq!(result.unwrap(), None); } #[test] diff --git a/tests/cli-rustup.rs b/tests/cli-rustup.rs index 59ea45be74..28def93ff8 100644 --- a/tests/cli-rustup.rs +++ b/tests/cli-rustup.rs @@ -11,8 +11,8 @@ use rustup::test::this_host_triple; use rustup::utils::raw; use crate::mock::clitools::{ - self, expect_err, expect_ok, expect_ok_ex, expect_stderr_ok, expect_stdout_ok, run, - set_current_dist_date, Config, Scenario, + self, expect_err, expect_not_stdout_ok, expect_ok, expect_ok_ex, expect_stderr_ok, + expect_stdout_ok, run, set_current_dist_date, Config, Scenario, }; macro_rules! for_host_and_home { @@ -1403,7 +1403,7 @@ fn file_override_with_archive() { } #[test] -fn file_override_toml_format() { +fn file_override_toml_format_select_installed_toolchain() { setup(&|config| { expect_ok(config, &["rustup", "default", "stable"]); expect_ok( @@ -1435,9 +1435,15 @@ channel = "nightly-2015-01-01" } #[test] -fn file_override_toml_format_add_components_toolchain_not_installed() { +fn file_override_toml_format_install_both_toolchain_and_components() { setup(&|config| { expect_ok(config, &["rustup", "default", "stable"]); + expect_stdout_ok(config, &["rustc", "--version"], "hash-stable-1.1.0"); + expect_not_stdout_ok( + config, + &["rustup", "component", "list"], + "rust-src (installed)", + ); let cwd = config.current_dir(); let toolchain_file = cwd.join("rust-toolchain"); @@ -1461,35 +1467,26 @@ components = [ "rust-src" ] } #[test] -fn file_override_toml_format_add_components_toolchain_misses_components() { +fn file_override_toml_format_add_missing_components() { setup(&|config| { expect_ok(config, &["rustup", "default", "stable"]); - expect_ok( + expect_not_stdout_ok( config, - &[ - "rustup", - "toolchain", - "install", - "nightly-2015-01-01", - "--no-self-update", - ], + &["rustup", "component", "list"], + "rust-src (installed)", ); - expect_stdout_ok(config, &["rustc", "--version"], "hash-stable-1.1.0"); - let cwd = config.current_dir(); let toolchain_file = cwd.join("rust-toolchain"); raw::write_file( &toolchain_file, r#" [toolchain] -channel = "nightly-2015-01-01" components = [ "rust-src" ] "#, ) .unwrap(); - expect_stdout_ok(config, &["rustc", "--version"], "hash-nightly-1"); expect_stdout_ok( config, &["rustup", "component", "list"], @@ -1499,55 +1496,38 @@ components = [ "rust-src" ] } #[test] -fn file_override_toml_format_add_components_invalid_component() { +fn file_override_toml_format_add_missing_targets() { setup(&|config| { expect_ok(config, &["rustup", "default", "stable"]); - expect_ok( + expect_not_stdout_ok( config, - &[ - "rustup", - "toolchain", - "install", - "nightly-2015-01-01", - "--no-self-update", - ], + &["rustup", "component", "list"], + "arm-linux-androideabi (installed)", ); - expect_stdout_ok(config, &["rustc", "--version"], "hash-stable-1.1.0"); - let cwd = config.current_dir(); let toolchain_file = cwd.join("rust-toolchain"); raw::write_file( &toolchain_file, r#" [toolchain] -channel = "nightly-2015-01-01" -components = [ "rust-bongo" ] +targets = [ "arm-linux-androideabi" ] "#, ) .unwrap(); - // The toolchain should be installed and the invalid component skipped - expect_stdout_ok(config, &["rustc", "--version"], "hash-nightly-1"); + expect_stdout_ok( + config, + &["rustup", "component", "list"], + "arm-linux-androideabi (installed)", + ); }); } #[test] -fn file_override_toml_format_add_components_toolchain_misses_target() { +fn file_override_toml_format_skip_invalid_component() { setup(&|config| { expect_ok(config, &["rustup", "default", "stable"]); - expect_ok( - config, - &[ - "rustup", - "toolchain", - "install", - "nightly-2015-01-01", - "--no-self-update", - ], - ); - - expect_stdout_ok(config, &["rustc", "--version"], "hash-stable-1.1.0"); let cwd = config.current_dir(); let toolchain_file = cwd.join("rust-toolchain"); @@ -1555,17 +1535,15 @@ fn file_override_toml_format_add_components_toolchain_misses_target() { &toolchain_file, r#" [toolchain] -channel = "nightly-2015-01-01" -targets = [ "arm-linux-androideabi" ] +components = [ "rust-bongo" ] "#, ) .unwrap(); - expect_stdout_ok(config, &["rustc", "--version"], "hash-nightly-1"); - expect_stdout_ok( + expect_stderr_ok( config, - &["rustup", "component", "list"], - "arm-linux-androideabi (installed)", + &["rustc", "--version"], + "warning: Force-skipping unavailable component 'rust-bongo", ); }); } From 7b88517d4377fb3b35540e57bf926c503003e30a Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Tue, 28 Jul 2020 22:57:40 +0200 Subject: [PATCH 3/6] Remove `OverrideKind` enum --- src/config.rs | 63 +++++++++++++++++++++------------------------------ 1 file changed, 26 insertions(+), 37 deletions(-) diff --git a/src/config.rs b/src/config.rs index 6c67d719f0..eeddfa75f5 100644 --- a/src/config.rs +++ b/src/config.rs @@ -43,9 +43,15 @@ impl ToolchainSection { } } -enum OverrideKind { - Name(String), - File(OverrideFile), +impl> From for OverrideFile { + fn from(channel: T) -> Self { + Self { + toolchain: ToolchainSection { + channel: Some(channel.into()), + ..Default::default() + }, + } + } } #[derive(Debug)] @@ -75,21 +81,15 @@ struct OverrideCfg<'a> { } impl<'a> OverrideCfg<'a> { - fn from_kind(cfg: &'a Cfg, kind: OverrideKind) -> Result { - match kind { - OverrideKind::Name(name) => Ok(Self { - toolchain: Some(Toolchain::from(cfg, &name)?), - ..Default::default() - }), - OverrideKind::File(file) => Ok(Self { - toolchain: match file.toolchain.channel { - Some(name) => Some(Toolchain::from(cfg, &name)?), - None => None, - }, - components: file.toolchain.components.unwrap_or_default(), - targets: file.toolchain.targets.unwrap_or_default(), - }), - } + fn from_file(cfg: &'a Cfg, file: OverrideFile) -> Result { + Ok(Self { + toolchain: match file.toolchain.channel { + Some(name) => Some(Toolchain::from(cfg, &name)?), + None => None, + }, + components: file.toolchain.components.unwrap_or_default(), + targets: file.toolchain.targets.unwrap_or_default(), + }) } } @@ -473,18 +473,12 @@ impl Cfg { // First check toolchain override from command if let Some(ref name) = self.toolchain_override { - override_ = Some(( - OverrideKind::Name(name.to_string()), - OverrideReason::CommandLine, - )); + override_ = Some((name.into(), OverrideReason::CommandLine)); } // Check RUSTUP_TOOLCHAIN if let Some(ref name) = self.env_override { - override_ = Some(( - OverrideKind::Name(name.to_string()), - OverrideReason::Environment, - )); + override_ = Some((name.into(), OverrideReason::Environment)); } // Then walk up the directory tree from 'path' looking for either the @@ -497,7 +491,7 @@ impl Cfg { })?; } - if let Some((kind, reason)) = override_ { + if let Some((file, reason)) = override_ { // This is hackishly using the error chain to provide a bit of // extra context about what went wrong. The CLI will display it // on a line after the proximate error. @@ -521,7 +515,7 @@ impl Cfg { ), }; - let override_cfg = OverrideCfg::from_kind(self, kind)?; + let override_cfg = OverrideCfg::from_file(self, file)?; if let Some(toolchain) = &override_cfg.toolchain { // Overridden toolchains can be literally any string, but only // distributable toolchains will be auto-installed by the wrapping @@ -546,7 +540,7 @@ impl Cfg { &self, dir: &Path, settings: &Settings, - ) -> Result> { + ) -> Result> { let notify = self.notify_handler.as_ref(); let dir = utils::canonicalize_path(dir, notify); let mut dir = Some(&*dir); @@ -555,7 +549,7 @@ impl Cfg { // First check the override database if let Some(name) = settings.dir_override(d, notify) { let reason = OverrideReason::OverrideDB(d.to_owned()); - return Ok(Some((OverrideKind::Name(name), reason))); + return Ok(Some((name.into(), reason))); } // Then look for 'rust-toolchain' @@ -578,7 +572,7 @@ impl Cfg { } let reason = OverrideReason::ToolchainFile(toolchain_file); - return Ok(Some((OverrideKind::File(override_file), reason))); + return Ok(Some((override_file, reason))); } } @@ -596,12 +590,7 @@ impl Cfg { .map(|file| if file.is_empty() { None } else { Some(file) }) .map_err(|e| ErrorKind::ParsingOverride(e).into()) } else { - Ok(contents.lines().next().map(|line| OverrideFile { - toolchain: ToolchainSection { - channel: Some(line.trim().into()), - ..Default::default() - }, - })) + Ok(contents.lines().next().map(|line| line.trim().into())) } } From 846c9a5a5fb4e9dab325f983abb76d0fdcb5988b Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Thu, 30 Jul 2020 23:14:33 +0200 Subject: [PATCH 4/6] Avoid parsing TOML manually When trying to detect the format of the toolchain override file, let serde do all the work. If the file does not parse as TOML, fallback to the legacy format, but keep the TOML error. If the first line does not happen to be a valid toolchain, show also the TOML error so that TOML syntax errors are reported. --- src/config.rs | 108 +++++++++++++++++++++++++++++++------------- src/errors.rs | 2 +- tests/cli-rustup.rs | 7 ++- 3 files changed, 84 insertions(+), 33 deletions(-) diff --git a/src/config.rs b/src/config.rs index eeddfa75f5..037f2da933 100644 --- a/src/config.rs +++ b/src/config.rs @@ -555,19 +555,26 @@ impl Cfg { // Then look for 'rust-toolchain' let toolchain_file = d.join("rust-toolchain"); if let Ok(contents) = utils::read_file("toolchain file", &toolchain_file) { - if let Some(override_file) = Cfg::parse_override_file(contents)? { + if let (Some(override_file), toml_err) = Cfg::parse_override_file(contents) { if let Some(toolchain_name) = &override_file.toolchain.channel { let all_toolchains = self.list_toolchains()?; if !all_toolchains.iter().any(|s| s == toolchain_name) { // The given name is not resolvable as a toolchain, so // instead check it's plausible for installation later - dist::validate_channel_name(&toolchain_name).chain_err(|| { - format!( - "invalid channel name '{}' in '{}'", - toolchain_name, - toolchain_file.display() - ) - })?; + let mut validation = dist::validate_channel_name(&toolchain_name) + .chain_err(|| { + format!( + "error parsing override file as legacy format: invalid channel name '{}' in '{}'", + toolchain_name, + toolchain_file.display() + ) + }); + // If there was an error parsing the toolchain file as TOML, report it now. + // This can help if the toolchain file is actually TOML but there was a syntax error. + if let Some(err) = toml_err { + validation = validation.chain_err(|| err); + } + validation?; } } @@ -582,15 +589,27 @@ impl Cfg { Ok(None) } - fn parse_override_file>(contents: S) -> Result> { + fn parse_override_file>( + contents: S, + ) -> (Option, Option) { let contents = contents.as_ref(); - if contents.lines().any(|line| line.starts_with("[toolchain]")) { - toml::from_str::(contents) - .map(|file| if file.is_empty() { None } else { Some(file) }) - .map_err(|e| ErrorKind::ParsingOverride(e).into()) - } else { - Ok(contents.lines().next().map(|line| line.trim().into())) + // Avoid TOML parsing error for backwards compatibility with the legacy format + if contents.is_empty() { + return (None, None); + } + + // Try to parse as TOML ... + match toml::from_str::(contents) + .map(|file| if file.is_empty() { None } else { Some(file) }) + .map_err(ErrorKind::ParsingOverride) + { + Ok(override_file) => (override_file, None), + // ... in case of error, try to parse as the legacy format + Err(toml_err) => ( + contents.lines().next().map(|line| line.trim().into()), + Some(toml_err), + ), } } @@ -862,9 +881,9 @@ mod tests { fn parse_legacy_toolchain_file() { let contents = "nightly-2020-07-10"; - let result = Cfg::parse_override_file(contents); + let (override_file, toml_err) = Cfg::parse_override_file(contents); assert_eq!( - result.unwrap(), + override_file, Some(OverrideFile { toolchain: ToolchainSection { channel: Some(contents.into()), @@ -872,6 +891,7 @@ mod tests { } }) ); + assert!(toml_err.is_some()); } #[test] @@ -883,9 +903,9 @@ components = [ "rustfmt", "rustc-dev" ] targets = [ "wasm32-unknown-unknown", "thumbv2-none-eabi" ] "#; - let result = Cfg::parse_override_file(contents); + let (override_file, toml_err) = Cfg::parse_override_file(contents); assert_eq!( - result.unwrap(), + override_file, Some(OverrideFile { toolchain: ToolchainSection { channel: Some("nightly-2020-07-10".into()), @@ -897,6 +917,7 @@ targets = [ "wasm32-unknown-unknown", "thumbv2-none-eabi" ] } }) ); + assert!(toml_err.is_none()); } #[test] @@ -906,9 +927,9 @@ targets = [ "wasm32-unknown-unknown", "thumbv2-none-eabi" ] channel = "nightly-2020-07-10" "#; - let result = Cfg::parse_override_file(contents); + let (override_file, toml_err) = Cfg::parse_override_file(contents); assert_eq!( - result.unwrap(), + override_file, Some(OverrideFile { toolchain: ToolchainSection { channel: Some("nightly-2020-07-10".into()), @@ -917,6 +938,7 @@ channel = "nightly-2020-07-10" } }) ); + assert!(toml_err.is_none()); } #[test] @@ -927,9 +949,9 @@ channel = "nightly-2020-07-10" components = [] "#; - let result = Cfg::parse_override_file(contents); + let (override_file, toml_err) = Cfg::parse_override_file(contents); assert_eq!( - result.unwrap(), + override_file, Some(OverrideFile { toolchain: ToolchainSection { channel: Some("nightly-2020-07-10".into()), @@ -938,6 +960,7 @@ components = [] } }) ); + assert!(toml_err.is_none()); } #[test] @@ -948,9 +971,9 @@ channel = "nightly-2020-07-10" targets = [] "#; - let result = Cfg::parse_override_file(contents); + let (override_file, toml_err) = Cfg::parse_override_file(contents); assert_eq!( - result.unwrap(), + override_file, Some(OverrideFile { toolchain: ToolchainSection { channel: Some("nightly-2020-07-10".into()), @@ -959,6 +982,7 @@ targets = [] } }) ); + assert!(toml_err.is_none()); } #[test] @@ -968,9 +992,9 @@ targets = [] components = [ "rustfmt" ] "#; - let result = Cfg::parse_override_file(contents); + let (override_file, toml_err) = Cfg::parse_override_file(contents); assert_eq!( - result.unwrap(), + override_file, Some(OverrideFile { toolchain: ToolchainSection { channel: None, @@ -979,6 +1003,7 @@ components = [ "rustfmt" ] } }) ); + assert!(toml_err.is_none()); } #[test] @@ -987,15 +1012,36 @@ components = [ "rustfmt" ] [toolchain] "#; - let result = Cfg::parse_override_file(contents); - assert_eq!(result.unwrap(), None); + let (override_file, toml_err) = Cfg::parse_override_file(contents); + assert!(override_file.is_none()); + assert!(toml_err.is_none()); } #[test] fn parse_empty_toolchain_file() { let contents = ""; - let result = Cfg::parse_override_file(contents); - assert_eq!(result.unwrap(), None); + let (override_file, toml_err) = Cfg::parse_override_file(contents); + assert!(override_file.is_none()); + assert!(toml_err.is_none()); + } + + #[test] + fn parse_toml_syntax_error() { + let contents = r#"[toolchain] +channel = nightly +"#; + + let (override_file, toml_err) = Cfg::parse_override_file(contents); + assert_eq!( + override_file, + Some(OverrideFile { + toolchain: ToolchainSection { + channel: Some("[toolchain]".into()), + ..Default::default() + } + }) + ); + assert!(toml_err.is_some()); } } diff --git a/src/errors.rs b/src/errors.rs index f67c63b1ab..748dfae69c 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -357,7 +357,7 @@ error_chain! { description("error parsing settings") } ParsingOverride(e: toml::de::Error) { - description("error parsing override file") + description("error parsing override file as TOML") } NoExeName { description("couldn't determine self executable name") diff --git a/tests/cli-rustup.rs b/tests/cli-rustup.rs index 28def93ff8..fbab20ff68 100644 --- a/tests/cli-rustup.rs +++ b/tests/cli-rustup.rs @@ -1706,7 +1706,12 @@ fn bad_file_override() { expect_err( config, &["rustc", "--version"], - "invalid channel name 'gumbo' in", + "error parsing override file as legacy format: invalid channel name 'gumbo' in", + ); + expect_err( + config, + &["rustc", "--version"], + "error parsing override file as TOML", ); }); } From 15642feaa91514856b5dcd2d13a88944c7374665 Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Thu, 30 Jul 2020 23:17:28 +0200 Subject: [PATCH 5/6] Update README: channel not mandatory in override --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 8eb7efcb54..509d2584ff 100644 --- a/README.md +++ b/README.md @@ -414,8 +414,8 @@ components = [ "rustfmt", "rustc-dev" ] targets = [ "wasm32-unknown-unknown", "thumbv2-none-eabi" ] ``` -If the TOML format is used, the `[toolchain]` section and the `channel` entry -are mandatory. +If the TOML format is used, the `[toolchain]` section is mandatory, +but all its entries are optional. The `rust-toolchain` file is suitable to check in to source control. This file has to be encoded in US-ASCII (if you are on From c2db7dac6b38c99538eec472db9d23d18f918409 Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Fri, 31 Jul 2020 22:05:05 +0200 Subject: [PATCH 6/6] Rework toolchain file parsing --- README.md | 2 +- src/config.rs | 194 +++++++++++++++++++++----------------------- src/errors.rs | 12 ++- tests/cli-rustup.rs | 7 +- 4 files changed, 105 insertions(+), 110 deletions(-) diff --git a/README.md b/README.md index 509d2584ff..e6a5c5f71e 100644 --- a/README.md +++ b/README.md @@ -415,7 +415,7 @@ targets = [ "wasm32-unknown-unknown", "thumbv2-none-eabi" ] ``` If the TOML format is used, the `[toolchain]` section is mandatory, -but all its entries are optional. +and at least one property must be specified. The `rust-toolchain` file is suitable to check in to source control. This file has to be encoded in US-ASCII (if you are on diff --git a/src/config.rs b/src/config.rs index 037f2da933..de0e6e00a4 100644 --- a/src/config.rs +++ b/src/config.rs @@ -555,32 +555,24 @@ impl Cfg { // Then look for 'rust-toolchain' let toolchain_file = d.join("rust-toolchain"); if let Ok(contents) = utils::read_file("toolchain file", &toolchain_file) { - if let (Some(override_file), toml_err) = Cfg::parse_override_file(contents) { - if let Some(toolchain_name) = &override_file.toolchain.channel { - let all_toolchains = self.list_toolchains()?; - if !all_toolchains.iter().any(|s| s == toolchain_name) { - // The given name is not resolvable as a toolchain, so - // instead check it's plausible for installation later - let mut validation = dist::validate_channel_name(&toolchain_name) - .chain_err(|| { - format!( - "error parsing override file as legacy format: invalid channel name '{}' in '{}'", - toolchain_name, - toolchain_file.display() - ) - }); - // If there was an error parsing the toolchain file as TOML, report it now. - // This can help if the toolchain file is actually TOML but there was a syntax error. - if let Some(err) = toml_err { - validation = validation.chain_err(|| err); - } - validation?; - } + let override_file = Cfg::parse_override_file(contents)?; + if let Some(toolchain_name) = &override_file.toolchain.channel { + let all_toolchains = self.list_toolchains()?; + if !all_toolchains.iter().any(|s| s == toolchain_name) { + // The given name is not resolvable as a toolchain, so + // instead check it's plausible for installation later + dist::validate_channel_name(&toolchain_name).chain_err(|| { + format!( + "invalid channel name '{}' in '{}'", + toolchain_name, + toolchain_file.display() + ) + })?; } - - let reason = OverrideReason::ToolchainFile(toolchain_file); - return Ok(Some((override_file, reason))); } + + let reason = OverrideReason::ToolchainFile(toolchain_file); + return Ok(Some((override_file, reason))); } dir = d.parent(); @@ -589,27 +581,30 @@ impl Cfg { Ok(None) } - fn parse_override_file>( - contents: S, - ) -> (Option, Option) { + fn parse_override_file>(contents: S) -> Result { let contents = contents.as_ref(); - // Avoid TOML parsing error for backwards compatibility with the legacy format - if contents.is_empty() { - return (None, None); - } + match contents.lines().count() { + 0 => return Err(ErrorKind::EmptyOverrideFile.into()), + 1 => { + let channel = contents.trim(); - // Try to parse as TOML ... - match toml::from_str::(contents) - .map(|file| if file.is_empty() { None } else { Some(file) }) - .map_err(ErrorKind::ParsingOverride) - { - Ok(override_file) => (override_file, None), - // ... in case of error, try to parse as the legacy format - Err(toml_err) => ( - contents.lines().next().map(|line| line.trim().into()), - Some(toml_err), - ), + if channel.is_empty() { + Err(ErrorKind::EmptyOverrideFile.into()) + } else { + Ok(channel.into()) + } + } + _ => { + let override_file = toml::from_str::(contents) + .map_err(ErrorKind::ParsingOverrideFile)?; + + if override_file.is_empty() { + Err(ErrorKind::InvalidOverrideFile.into()) + } else { + Ok(override_file) + } + } } } @@ -881,32 +876,31 @@ mod tests { fn parse_legacy_toolchain_file() { let contents = "nightly-2020-07-10"; - let (override_file, toml_err) = Cfg::parse_override_file(contents); + let result = Cfg::parse_override_file(contents); assert_eq!( - override_file, - Some(OverrideFile { + result.unwrap(), + OverrideFile { toolchain: ToolchainSection { channel: Some(contents.into()), - ..Default::default() + components: None, + targets: None, } - }) + } ); - assert!(toml_err.is_some()); } #[test] fn parse_toml_toolchain_file() { - let contents = r#" -[toolchain] + let contents = r#"[toolchain] channel = "nightly-2020-07-10" components = [ "rustfmt", "rustc-dev" ] targets = [ "wasm32-unknown-unknown", "thumbv2-none-eabi" ] "#; - let (override_file, toml_err) = Cfg::parse_override_file(contents); + let result = Cfg::parse_override_file(contents); assert_eq!( - override_file, - Some(OverrideFile { + result.unwrap(), + OverrideFile { toolchain: ToolchainSection { channel: Some("nightly-2020-07-10".into()), components: Some(vec!["rustfmt".into(), "rustc-dev".into()]), @@ -915,95 +909,86 @@ targets = [ "wasm32-unknown-unknown", "thumbv2-none-eabi" ] "thumbv2-none-eabi".into() ]), } - }) + } ); - assert!(toml_err.is_none()); } #[test] fn parse_toml_toolchain_file_only_channel() { - let contents = r#" -[toolchain] + let contents = r#"[toolchain] channel = "nightly-2020-07-10" "#; - let (override_file, toml_err) = Cfg::parse_override_file(contents); + let result = Cfg::parse_override_file(contents); assert_eq!( - override_file, - Some(OverrideFile { + result.unwrap(), + OverrideFile { toolchain: ToolchainSection { channel: Some("nightly-2020-07-10".into()), components: None, targets: None, } - }) + } ); - assert!(toml_err.is_none()); } #[test] fn parse_toml_toolchain_file_empty_components() { - let contents = r#" -[toolchain] + let contents = r#"[toolchain] channel = "nightly-2020-07-10" components = [] "#; - let (override_file, toml_err) = Cfg::parse_override_file(contents); + let result = Cfg::parse_override_file(contents); assert_eq!( - override_file, - Some(OverrideFile { + result.unwrap(), + OverrideFile { toolchain: ToolchainSection { channel: Some("nightly-2020-07-10".into()), components: Some(vec![]), targets: None, } - }) + } ); - assert!(toml_err.is_none()); } #[test] fn parse_toml_toolchain_file_empty_targets() { - let contents = r#" -[toolchain] + let contents = r#"[toolchain] channel = "nightly-2020-07-10" targets = [] "#; - let (override_file, toml_err) = Cfg::parse_override_file(contents); + let result = Cfg::parse_override_file(contents); assert_eq!( - override_file, - Some(OverrideFile { + result.unwrap(), + OverrideFile { toolchain: ToolchainSection { channel: Some("nightly-2020-07-10".into()), components: None, targets: Some(vec![]), } - }) + } ); - assert!(toml_err.is_none()); } #[test] fn parse_toml_toolchain_file_no_channel() { - let contents = r#" -[toolchain] + let contents = r#"[toolchain] components = [ "rustfmt" ] "#; - let (override_file, toml_err) = Cfg::parse_override_file(contents); + let result = Cfg::parse_override_file(contents); assert_eq!( - override_file, - Some(OverrideFile { + result.unwrap(), + OverrideFile { toolchain: ToolchainSection { channel: None, components: Some(vec!["rustfmt".into()]), targets: None, } - }) + } ); - assert!(toml_err.is_none()); } #[test] @@ -1012,18 +997,33 @@ components = [ "rustfmt" ] [toolchain] "#; - let (override_file, toml_err) = Cfg::parse_override_file(contents); - assert!(override_file.is_none()); - assert!(toml_err.is_none()); + let result = Cfg::parse_override_file(contents); + assert!(matches!( + result.unwrap_err().kind(), + ErrorKind::InvalidOverrideFile + )); } #[test] fn parse_empty_toolchain_file() { let contents = ""; - let (override_file, toml_err) = Cfg::parse_override_file(contents); - assert!(override_file.is_none()); - assert!(toml_err.is_none()); + let result = Cfg::parse_override_file(contents); + assert!(matches!( + result.unwrap_err().kind(), + ErrorKind::EmptyOverrideFile + )); + } + + #[test] + fn parse_whitespace_toolchain_file() { + let contents = " "; + + let result = Cfg::parse_override_file(contents); + assert!(matches!( + result.unwrap_err().kind(), + ErrorKind::EmptyOverrideFile + )); } #[test] @@ -1032,16 +1032,10 @@ components = [ "rustfmt" ] channel = nightly "#; - let (override_file, toml_err) = Cfg::parse_override_file(contents); - assert_eq!( - override_file, - Some(OverrideFile { - toolchain: ToolchainSection { - channel: Some("[toolchain]".into()), - ..Default::default() - } - }) - ); - assert!(toml_err.is_some()); + let result = Cfg::parse_override_file(contents); + assert!(matches!( + result.unwrap_err().kind(), + ErrorKind::ParsingOverrideFile(..) + )); } } diff --git a/src/errors.rs b/src/errors.rs index 748dfae69c..eb411de371 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -356,9 +356,6 @@ error_chain! { ParsingSettings(e: toml::de::Error) { description("error parsing settings") } - ParsingOverride(e: toml::de::Error) { - description("error parsing override file as TOML") - } NoExeName { description("couldn't determine self executable name") } @@ -377,6 +374,15 @@ error_chain! { BrokenPartialFile { description("partially downloaded file may have been damaged and was removed, please try again") } + EmptyOverrideFile { + description("empty toolchain override file detected. Please remove it, or else specify the desired toolchain properties in the file") + } + InvalidOverrideFile { + description("missing toolchain properties in toolchain override file") + } + ParsingOverrideFile(e: toml::de::Error) { + description("error parsing override file") + } } } diff --git a/tests/cli-rustup.rs b/tests/cli-rustup.rs index fbab20ff68..28def93ff8 100644 --- a/tests/cli-rustup.rs +++ b/tests/cli-rustup.rs @@ -1706,12 +1706,7 @@ fn bad_file_override() { expect_err( config, &["rustc", "--version"], - "error parsing override file as legacy format: invalid channel name 'gumbo' in", - ); - expect_err( - config, - &["rustc", "--version"], - "error parsing override file as TOML", + "invalid channel name 'gumbo' in", ); }); }