From f46f1afeabe9068a8c330db98fd17051de623142 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Sat, 15 Jun 2024 13:05:07 +0200 Subject: [PATCH 1/2] Let argument parser handle Profile conversion --- src/cli/rustup_mode.rs | 17 +++++--------- src/cli/self_update.rs | 20 +++++++++------- src/cli/setup_mode.rs | 12 ++++------ src/config.rs | 30 ++++++++---------------- src/dist/dist.rs | 53 +++++++++++++++++++++++------------------- 5 files changed, 60 insertions(+), 72 deletions(-) diff --git a/src/cli/rustup_mode.rs b/src/cli/rustup_mode.rs index 2add7bc982..638d1ee440 100644 --- a/src/cli/rustup_mode.rs +++ b/src/cli/rustup_mode.rs @@ -3,7 +3,6 @@ use std::fmt; use std::io::Write; use std::path::{Path, PathBuf}; use std::process::ExitStatus; -use std::str::FromStr; use anyhow::{anyhow, Error, Result}; use clap::{ @@ -333,8 +332,8 @@ struct UpdateOpts { )] toolchain: Vec, - #[arg(long, value_parser = PossibleValuesParser::new(Profile::names()))] - profile: Option, + #[arg(long, value_enum)] + profile: Option, /// Add specific components on installation #[arg(short, long, value_delimiter = ',', num_args = 1..)] @@ -518,11 +517,8 @@ enum SetSubcmd { /// The default components installed with a toolchain Profile { - #[arg( - default_value = Profile::default_name(), - value_parser = PossibleValuesParser::new(Profile::names()), - )] - profile_name: String, + #[arg(value_enum, default_value_t)] + profile_name: Profile, }, /// The rustup auto self update mode @@ -710,7 +706,7 @@ pub async fn main(current_dir: PathBuf) -> Result { .set_default_host_triple(host_triple) .map(|_| utils::ExitCode(0)), SetSubcmd::Profile { profile_name } => { - cfg.set_profile(&profile_name).map(|_| utils::ExitCode(0)) + cfg.set_profile(profile_name).map(|_| utils::ExitCode(0)) } SetSubcmd::AutoSelfUpdate { auto_self_update_mode, @@ -819,8 +815,7 @@ async fn update(cfg: &mut Cfg, opts: UpdateOpts) -> Result { && self_update_mode == SelfUpdateMode::Enable && !opts.no_self_update; let forced = opts.force_non_host; - if let Some(p) = &opts.profile { - let p = Profile::from_str(p)?; + if let Some(p) = opts.profile { cfg.set_profile_override(p); } let cfg = &cfg; diff --git a/src/cli/self_update.rs b/src/cli/self_update.rs index eecb79e3f5..b6c2e44bc2 100644 --- a/src/cli/self_update.rs +++ b/src/cli/self_update.rs @@ -56,6 +56,8 @@ use std::{env, fmt}; use anyhow::{anyhow, Context, Result}; use cfg_if::cfg_if; +use clap::ValueEnum; +use itertools::Itertools; use same_file::Handle; use serde::{Deserialize, Serialize}; @@ -88,7 +90,7 @@ pub use windows::complete_windows_uninstall; pub(crate) struct InstallOpts<'a> { pub default_host_triple: Option, pub default_toolchain: Option, - pub profile: String, + pub profile: Profile, pub no_modify_path: bool, pub no_update_toolchain: bool, pub components: &'a [&'a str], @@ -107,7 +109,7 @@ impl<'a> InstallOpts<'a> { targets, } = self; - cfg.set_profile(&profile)?; + cfg.set_profile(profile)?; if let Some(default_host_triple) = &default_host_triple { // Set host triple now as it will affect resolution of toolchain_str @@ -202,13 +204,13 @@ impl<'a> InstallOpts<'a> { .unwrap_or("stable".into()), )?)?); - self.profile = common::question_str( + self.profile = ::from_str(&common::question_str( &format!( "Profile (which tools and data to install)? ({})", - Profile::names().join("/") + Profile::value_variants().iter().join("/"), ), - &self.profile, - )?; + self.profile.as_str(), + )?)?; self.no_modify_path = !common::question_bool("Modify PATH variable?", !self.no_modify_path)?; @@ -1313,7 +1315,7 @@ mod tests { use crate::cli::common; use crate::cli::self_update::InstallOpts; - use crate::dist::dist::PartialToolchainDesc; + use crate::dist::dist::{PartialToolchainDesc, Profile}; use crate::test::{test_dir, with_rustup_home, Env}; use crate::{currentprocess, for_host}; @@ -1329,8 +1331,8 @@ mod tests { let opts = InstallOpts { default_host_triple: None, - default_toolchain: None, // No toolchain specified - profile: "default".to_owned(), // default profile + default_toolchain: None, // No toolchain specified + profile: Profile::Default, // default profile no_modify_path: false, components: &[], targets: &[], diff --git a/src/cli/setup_mode.rs b/src/cli/setup_mode.rs index 49dcbb3bb3..c30112ac5b 100644 --- a/src/cli/setup_mode.rs +++ b/src/cli/setup_mode.rs @@ -1,7 +1,7 @@ use std::path::PathBuf; use anyhow::Result; -use clap::{builder::PossibleValuesParser, Parser}; +use clap::Parser; use crate::{ cli::{ @@ -43,12 +43,8 @@ struct RustupInit { #[arg(long)] default_toolchain: Option, - #[arg( - long, - value_parser = PossibleValuesParser::new(Profile::names()), - default_value = Profile::default_name(), - )] - profile: String, + #[arg(long, value_enum, default_value_t)] + profile: Profile, /// Component name to also install #[arg(short, long, value_delimiter = ',', num_args = 1..)] @@ -110,7 +106,7 @@ pub async fn main(current_dir: PathBuf) -> Result { return Ok(utils::ExitCode(0)); } - if &profile == "complete" { + if profile == Profile::Complete { warn!("{}", common::WARN_COMPLETE_PROFILE); } diff --git a/src/config.rs b/src/config.rs index 45ad21380a..62d93e625f 100644 --- a/src/config.rs +++ b/src/config.rs @@ -370,19 +370,14 @@ impl Cfg { Ok(()) } - pub(crate) fn set_profile(&mut self, profile: &str) -> Result<()> { - match Profile::from_str(profile) { - Ok(p) => { - self.profile_override = None; - self.settings_file.with_mut(|s| { - s.profile = Some(p); - Ok(()) - })?; - (self.notify_handler)(Notification::SetProfile(profile)); - Ok(()) - } - Err(err) => Err(err), - } + pub(crate) fn set_profile(&mut self, profile: Profile) -> Result<()> { + self.profile_override = None; + self.settings_file.with_mut(|s| { + s.profile = Some(profile); + Ok(()) + })?; + (self.notify_handler)(Notification::SetProfile(profile.as_str())); + Ok(()) } pub(crate) fn set_auto_self_update(&mut self, mode: &str) -> Result<()> { @@ -414,13 +409,8 @@ impl Cfg { if let Some(p) = self.profile_override { return Ok(p); } - self.settings_file.with(|s| { - let p = match s.profile { - Some(p) => p, - None => Profile::Default, - }; - Ok(p) - }) + self.settings_file + .with(|s| Ok(s.profile.unwrap_or_default())) } pub(crate) fn get_self_update_mode(&self) -> Result { diff --git a/src/dist/dist.rs b/src/dist/dist.rs index ee38dbd8c4..76b856a79f 100644 --- a/src/dist/dist.rs +++ b/src/dist/dist.rs @@ -8,6 +8,9 @@ use std::str::FromStr; use anyhow::{anyhow, bail, Context, Result}; use chrono::NaiveDate; +use clap::builder::PossibleValue; +use clap::ValueEnum; +use itertools::Itertools; use once_cell::sync::Lazy; use regex::Regex; use serde::{Deserialize, Serialize}; @@ -611,6 +614,30 @@ pub enum Profile { Complete, } +impl Profile { + pub(crate) fn as_str(&self) -> &'static str { + match self { + Self::Minimal => "minimal", + Self::Default => "default", + Self::Complete => "complete", + } + } +} + +impl ValueEnum for Profile { + fn value_variants<'a>() -> &'a [Self] { + &[Profile::Minimal, Profile::Default, Profile::Complete] + } + + fn to_possible_value(&self) -> Option { + Some(PossibleValue::new(self.as_str())) + } + + fn from_str(input: &str, _: bool) -> Result { + ::from_str(input).map_err(|e| e.to_string()) + } +} + impl FromStr for Profile { type Err = anyhow::Error; @@ -622,22 +649,12 @@ impl FromStr for Profile { _ => Err(anyhow!(format!( "unknown profile name: '{}'; valid profile names are: {}", name, - valid_profile_names() + Self::value_variants().iter().join(", ") ))), } } } -impl Profile { - pub(crate) fn names() -> &'static [&'static str] { - &["minimal", "default", "complete"] - } - - pub(crate) fn default_name() -> &'static str { - "default" - } -} - impl fmt::Display for TargetTriple { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { self.0.fmt(f) @@ -680,22 +697,10 @@ impl fmt::Display for ToolchainDesc { impl fmt::Display for Profile { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match *self { - Self::Minimal => write!(f, "minimal"), - Self::Default => write!(f, "default"), - Self::Complete => write!(f, "complete"), - } + write!(f, "{}", self.as_str()) } } -pub(crate) fn valid_profile_names() -> String { - Profile::names() - .iter() - .map(|s| format!("'{s}'")) - .collect::>() - .join(", ") -} - // Installs or updates a toolchain from a dist server. If an initial // install then it will be installed with the default components. If // an upgrade then all the existing components will be upgraded. From 618cc061e5109365347bba4d2349c7ac25f47158 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Sat, 15 Jun 2024 13:53:58 +0200 Subject: [PATCH 2/2] Let argument parser handle SelfUpdateMode conversion --- src/cli/rustup_mode.rs | 19 ++++++++----------- src/cli/self_update.rs | 34 +++++++++++++++++++++++----------- src/config.rs | 26 ++++++++++---------------- 3 files changed, 41 insertions(+), 38 deletions(-) diff --git a/src/cli/rustup_mode.rs b/src/cli/rustup_mode.rs index 638d1ee440..d7abbae46c 100644 --- a/src/cli/rustup_mode.rs +++ b/src/cli/rustup_mode.rs @@ -5,10 +5,7 @@ use std::path::{Path, PathBuf}; use std::process::ExitStatus; use anyhow::{anyhow, Error, Result}; -use clap::{ - builder::{PossibleValue, PossibleValuesParser}, - Args, CommandFactory, Parser, Subcommand, ValueEnum, -}; +use clap::{builder::PossibleValue, Args, CommandFactory, Parser, Subcommand, ValueEnum}; use clap_complete::Shell; use itertools::Itertools; @@ -523,11 +520,8 @@ enum SetSubcmd { /// The rustup auto self update mode AutoSelfUpdate { - #[arg( - default_value = SelfUpdateMode::default_mode(), - value_parser = PossibleValuesParser::new(SelfUpdateMode::modes()), - )] - auto_self_update_mode: String, + #[arg(value_enum, default_value_t)] + auto_self_update_mode: SelfUpdateMode, }, } @@ -710,7 +704,7 @@ pub async fn main(current_dir: PathBuf) -> Result { } SetSubcmd::AutoSelfUpdate { auto_self_update_mode, - } => set_auto_self_update(cfg, &auto_self_update_mode), + } => set_auto_self_update(cfg, auto_self_update_mode), }, RustupSubcmd::Completions { shell, command } => output_completion_script(shell, command), } @@ -1510,7 +1504,10 @@ async fn man( Ok(utils::ExitCode(0)) } -fn set_auto_self_update(cfg: &mut Cfg, auto_self_update_mode: &str) -> Result { +fn set_auto_self_update( + cfg: &mut Cfg, + auto_self_update_mode: SelfUpdateMode, +) -> Result { if self_update::NEVER_SELF_UPDATE { let process = process(); let mut args = process.args_os(); diff --git a/src/cli/self_update.rs b/src/cli/self_update.rs index b6c2e44bc2..3763181ab7 100644 --- a/src/cli/self_update.rs +++ b/src/cli/self_update.rs @@ -56,6 +56,7 @@ use std::{env, fmt}; use anyhow::{anyhow, Context, Result}; use cfg_if::cfg_if; +use clap::builder::PossibleValue; use clap::ValueEnum; use itertools::Itertools; use same_file::Handle; @@ -243,21 +244,36 @@ pub(crate) const NEVER_SELF_UPDATE: bool = true; #[cfg(not(feature = "no-self-update"))] pub(crate) const NEVER_SELF_UPDATE: bool = false; -#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +#[derive(Clone, Copy, Debug, Default, Deserialize, Eq, PartialEq, Serialize)] #[serde(rename_all = "kebab-case")] pub enum SelfUpdateMode { + #[default] Enable, Disable, CheckOnly, } impl SelfUpdateMode { - pub(crate) fn modes() -> &'static [&'static str] { - &["enable", "disable", "check-only"] + pub(crate) fn as_str(&self) -> &'static str { + match self { + Self::Enable => "enable", + Self::Disable => "disable", + Self::CheckOnly => "check-only", + } + } +} + +impl ValueEnum for SelfUpdateMode { + fn value_variants<'a>() -> &'a [Self] { + &[Self::Enable, Self::Disable, Self::CheckOnly] } - pub(crate) fn default_mode() -> &'static str { - "enable" + fn to_possible_value(&self) -> Option { + Some(PossibleValue::new(self.as_str())) + } + + fn from_str(input: &str, _: bool) -> Result { + ::from_str(input).map_err(|e| e.to_string()) } } @@ -272,7 +288,7 @@ impl FromStr for SelfUpdateMode { _ => Err(anyhow!(format!( "unknown self update mode: '{}'; valid modes are {}", mode, - Self::modes().join(", "), + Self::value_variants().iter().join(", ") ))), } } @@ -280,11 +296,7 @@ impl FromStr for SelfUpdateMode { impl std::fmt::Display for SelfUpdateMode { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str(match self { - SelfUpdateMode::Enable => "enable", - SelfUpdateMode::Disable => "disable", - SelfUpdateMode::CheckOnly => "check-only", - }) + f.write_str(self.as_str()) } } diff --git a/src/config.rs b/src/config.rs index 62d93e625f..50eeba2277 100644 --- a/src/config.rs +++ b/src/config.rs @@ -380,18 +380,13 @@ impl Cfg { Ok(()) } - pub(crate) fn set_auto_self_update(&mut self, mode: &str) -> Result<()> { - match SelfUpdateMode::from_str(mode) { - Ok(update_mode) => { - self.settings_file.with_mut(|s| { - s.auto_self_update = Some(update_mode); - Ok(()) - })?; - (self.notify_handler)(Notification::SetSelfUpdate(mode)); - Ok(()) - } - Err(err) => Err(err), - } + pub(crate) fn set_auto_self_update(&mut self, mode: SelfUpdateMode) -> Result<()> { + self.settings_file.with_mut(|s| { + s.auto_self_update = Some(mode); + Ok(()) + })?; + (self.notify_handler)(Notification::SetSelfUpdate(mode.as_str())); + Ok(()) } pub(crate) fn set_toolchain_override(&mut self, toolchain_override: &ResolvableToolchainName) { @@ -415,11 +410,10 @@ impl Cfg { pub(crate) fn get_self_update_mode(&self) -> Result { self.settings_file.with(|s| { - let mode = match &s.auto_self_update { - Some(mode) => mode.clone(), + Ok(match s.auto_self_update { + Some(mode) => mode, None => SelfUpdateMode::Enable, - }; - Ok(mode) + }) }) }