From 03ee7b59991a107b08d4a2da50b1654dba48e225 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Fri, 9 Aug 2024 16:19:19 +0200 Subject: [PATCH 1/2] Move verbose help parsing to `main` To remove a side effect (process exit) when parsing config. --- src/bootstrap/src/bin/main.rs | 6 +++++- src/bootstrap/src/core/config/flags.rs | 26 ++++++++++++++++++-------- src/bootstrap/src/core/config/mod.rs | 2 +- src/bootstrap/src/lib.rs | 2 +- 4 files changed, 25 insertions(+), 11 deletions(-) diff --git a/src/bootstrap/src/bin/main.rs b/src/bootstrap/src/bin/main.rs index dc8b5487a61..48f0159486a 100644 --- a/src/bootstrap/src/bin/main.rs +++ b/src/bootstrap/src/bin/main.rs @@ -11,7 +11,7 @@ use std::{env, process}; use bootstrap::{ - find_recent_config_change_ids, human_readable_changes, t, Build, Config, Subcommand, + find_recent_config_change_ids, human_readable_changes, t, Build, Config, Flags, Subcommand, CONFIG_CHANGE_HISTORY, }; @@ -19,6 +19,10 @@ fn main() { let args = env::args().skip(1).collect::>(); let config = Config::parse(&args); + if Flags::try_parse_verbose_help(&args) { + return; + } + let mut build_lock; let _build_lock_guard; diff --git a/src/bootstrap/src/core/config/flags.rs b/src/bootstrap/src/core/config/flags.rs index 19f752da81c..3948fe0d98c 100644 --- a/src/bootstrap/src/core/config/flags.rs +++ b/src/bootstrap/src/core/config/flags.rs @@ -183,9 +183,9 @@ pub struct Flags { } impl Flags { - pub fn parse(args: &[String]) -> Self { - let first = String::from("x.py"); - let it = std::iter::once(&first).chain(args.iter()); + /// Check if ` -h -v` was passed. + /// If yes, print the available paths and return `true`. + pub fn try_parse_verbose_help(args: &[String]) -> bool { // We need to check for ` -h -v`, in which case we list the paths #[derive(Parser)] #[command(disable_help_flag(true))] @@ -198,10 +198,10 @@ struct HelpVerboseOnly { cmd: Kind, } if let Ok(HelpVerboseOnly { help: true, verbose: 1.., cmd: subcommand }) = - HelpVerboseOnly::try_parse_from(it.clone()) + HelpVerboseOnly::try_parse_from(normalize_args(args)) { println!("NOTE: updating submodules before printing available paths"); - let config = Config::parse(&[String::from("build")]); + let config = Config::parse(Self::parse(&[String::from("build")])); let build = Build::new(config); let paths = Builder::get_help(&build, subcommand); if let Some(s) = paths { @@ -209,11 +209,21 @@ struct HelpVerboseOnly { } else { panic!("No paths available for subcommand `{}`", subcommand.as_str()); } - crate::exit!(0); + true + } else { + false } - - Flags::parse_from(it) } + + pub fn parse(args: &[String]) -> Self { + Flags::parse_from(normalize_args(args)) + } +} + +fn normalize_args(args: &[String]) -> Vec { + let first = String::from("x.py"); + let it = std::iter::once(first).chain(args.iter().cloned()); + it.collect() } #[derive(Debug, Clone, Default, clap::Subcommand)] diff --git a/src/bootstrap/src/core/config/mod.rs b/src/bootstrap/src/core/config/mod.rs index 23556e8bc5d..9f09dd13f29 100644 --- a/src/bootstrap/src/core/config/mod.rs +++ b/src/bootstrap/src/core/config/mod.rs @@ -1,6 +1,6 @@ #[allow(clippy::module_inception)] mod config; -pub(crate) mod flags; +pub mod flags; #[cfg(test)] mod tests; diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index 453fb39327d..6edfc2ccdaf 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -43,7 +43,7 @@ mod utils; pub use core::builder::PathSet; -pub use core::config::flags::Subcommand; +pub use core::config::flags::{Flags, Subcommand}; pub use core::config::Config; pub use utils::change_tracker::{ From 5431a93ddc1c8482d9a801d1627991d16c2318d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Fri, 9 Aug 2024 16:19:25 +0200 Subject: [PATCH 2/2] Pass `Flags` to `Config::parse` explicitly --- src/bootstrap/src/bin/main.rs | 4 +++- src/bootstrap/src/core/builder/tests.rs | 5 +++-- src/bootstrap/src/core/config/config.rs | 7 +++---- src/bootstrap/src/core/config/tests.rs | 19 ++++++++++--------- src/bootstrap/src/utils/helpers/tests.rs | 8 +++++--- 5 files changed, 24 insertions(+), 19 deletions(-) diff --git a/src/bootstrap/src/bin/main.rs b/src/bootstrap/src/bin/main.rs index 48f0159486a..f03f03e2d93 100644 --- a/src/bootstrap/src/bin/main.rs +++ b/src/bootstrap/src/bin/main.rs @@ -17,12 +17,14 @@ fn main() { let args = env::args().skip(1).collect::>(); - let config = Config::parse(&args); if Flags::try_parse_verbose_help(&args) { return; } + let flags = Flags::parse(&args); + let config = Config::parse(flags); + let mut build_lock; let _build_lock_guard; diff --git a/src/bootstrap/src/core/builder/tests.rs b/src/bootstrap/src/core/builder/tests.rs index f19a4dd6d49..e06df65c1bc 100644 --- a/src/bootstrap/src/core/builder/tests.rs +++ b/src/bootstrap/src/core/builder/tests.rs @@ -3,13 +3,14 @@ use super::*; use crate::core::build_steps::doc::DocumentationFormat; use crate::core::config::Config; +use crate::Flags; fn configure(cmd: &str, host: &[&str], target: &[&str]) -> Config { configure_with_args(&[cmd.to_owned()], host, target) } fn configure_with_args(cmd: &[String], host: &[&str], target: &[&str]) -> Config { - let mut config = Config::parse(cmd); + let mut config = Config::parse(Flags::parse(cmd)); // don't save toolstates config.save_toolstates = None; config.dry_run = DryRun::SelfCheck; @@ -23,7 +24,7 @@ fn configure_with_args(cmd: &[String], host: &[&str], target: &[&str]) -> Config let submodule_build = Build::new(Config { // don't include LLVM, so CI doesn't require ninja/cmake to be installed rust_codegen_backends: vec![], - ..Config::parse(&["check".to_owned()]) + ..Config::parse(Flags::parse(&["check".to_owned()])) }); submodule_build.require_submodule("src/doc/book", None); config.submodules = Some(false); diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index 14beef20bad..bc7025de916 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -1188,7 +1188,7 @@ pub fn default_opts() -> Config { } } - pub fn parse(args: &[String]) -> Config { + pub fn parse(flags: Flags) -> Config { #[cfg(test)] fn get_toml(_: &Path) -> TomlConfig { TomlConfig::default() @@ -1218,11 +1218,10 @@ fn get_toml(file: &Path) -> TomlConfig { exit!(2); }) } - Self::parse_inner(args, get_toml) + Self::parse_inner(flags, get_toml) } - pub(crate) fn parse_inner(args: &[String], get_toml: impl Fn(&Path) -> TomlConfig) -> Config { - let mut flags = Flags::parse(args); + pub(crate) fn parse_inner(mut flags: Flags, get_toml: impl Fn(&Path) -> TomlConfig) -> Config { let mut config = Config::default_opts(); // Set flags. diff --git a/src/bootstrap/src/core/config/tests.rs b/src/bootstrap/src/core/config/tests.rs index 6e695d269cf..40f3e5e7222 100644 --- a/src/bootstrap/src/core/config/tests.rs +++ b/src/bootstrap/src/core/config/tests.rs @@ -12,9 +12,10 @@ use crate::core::config::{LldMode, Target, TargetSelection, TomlConfig}; fn parse(config: &str) -> Config { - Config::parse_inner(&["check".to_string(), "--config=/does/not/exist".to_string()], |&_| { - toml::from_str(&config).unwrap() - }) + Config::parse_inner( + Flags::parse(&["check".to_string(), "--config=/does/not/exist".to_string()]), + |&_| toml::from_str(&config).unwrap(), + ) } #[test] @@ -108,7 +109,7 @@ fn clap_verify() { #[test] fn override_toml() { let config = Config::parse_inner( - &[ + Flags::parse(&[ "check".to_owned(), "--config=/does/not/exist".to_owned(), "--set=change-id=1".to_owned(), @@ -121,7 +122,7 @@ fn override_toml() { "--set=target.x86_64-unknown-linux-gnu.rpath=false".to_owned(), "--set=target.aarch64-unknown-linux-gnu.sanitizers=false".to_owned(), "--set=target.aarch64-apple-darwin.runner=apple".to_owned(), - ], + ]), |&_| { toml::from_str( r#" @@ -201,12 +202,12 @@ fn override_toml() { #[should_panic] fn override_toml_duplicate() { Config::parse_inner( - &[ + Flags::parse(&[ "check".to_owned(), "--config=/does/not/exist".to_string(), "--set=change-id=1".to_owned(), "--set=change-id=2".to_owned(), - ], + ]), |&_| toml::from_str("change-id = 0").unwrap(), ); } @@ -226,7 +227,7 @@ fn get_toml(file: &Path) -> TomlConfig { .and_then(|table: toml::Value| TomlConfig::deserialize(table)) .unwrap() } - Config::parse_inner(&["check".to_owned()], get_toml); + Config::parse_inner(Flags::parse(&["check".to_owned()]), get_toml); } #[test] @@ -301,7 +302,7 @@ fn order_of_clippy_rules() { "-Aclippy::foo1".to_string(), "-Aclippy::foo2".to_string(), ]; - let config = Config::parse(&args); + let config = Config::parse(Flags::parse(&args)); let actual = match &config.cmd { crate::Subcommand::Clippy { allow, deny, warn, forbid, .. } => { diff --git a/src/bootstrap/src/utils/helpers/tests.rs b/src/bootstrap/src/utils/helpers/tests.rs index f0cb324674f..103c4d26a18 100644 --- a/src/bootstrap/src/utils/helpers/tests.rs +++ b/src/bootstrap/src/utils/helpers/tests.rs @@ -5,7 +5,7 @@ use crate::utils::helpers::{ check_cfg_arg, extract_beta_rev, hex_encode, make, program_out_of_date, symlink_dir, }; -use crate::Config; +use crate::{Config, Flags}; #[test] fn test_make() { @@ -58,7 +58,8 @@ fn test_check_cfg_arg() { #[test] fn test_program_out_of_date() { - let config = Config::parse(&["check".to_owned(), "--config=/does/not/exist".to_owned()]); + let config = + Config::parse(Flags::parse(&["check".to_owned(), "--config=/does/not/exist".to_owned()])); let tempfile = config.tempdir().join(".tmp-stamp-file"); File::create(&tempfile).unwrap().write_all(b"dummy value").unwrap(); assert!(tempfile.exists()); @@ -73,7 +74,8 @@ fn test_program_out_of_date() { #[test] fn test_symlink_dir() { - let config = Config::parse(&["check".to_owned(), "--config=/does/not/exist".to_owned()]); + let config = + Config::parse(Flags::parse(&["check".to_owned(), "--config=/does/not/exist".to_owned()])); let tempdir = config.tempdir().join(".tmp-dir"); let link_path = config.tempdir().join(".tmp-link");