Rollup merge of #128878 - Kobzol:refactor-flags, r=onur-ozkan

Slightly refactor `Flags` in bootstrap

The next step for https://github.com/rust-lang/rust/issues/126819 is to track commands executed inside `Config::parse`. This is quite challenging, because (tracked) command execution needs to access some state that is stored inside `Config`, which creates a sort of a chicken-and-egg problem.

I would like to first untangle `Config::parse` a little bit, which is what this PR starts with.

Tracking issue: https://github.com/rust-lang/rust/issues/126819

r? `@onur-ozkan`

src/bootstrap/src/bin/main.rs

@@ -11,13 +11,19 @@ use std::str::FromStr;
 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,
 };
 
 fn main() {
     let args = env::args().skip(1).collect::<Vec<_>>();
-    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;

src/bootstrap/src/core/builder/tests.rs

@@ -3,13 +3,14 @@ use std::thread;
 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);

src/bootstrap/src/core/config/config.rs

@@ -1191,7 +1191,7 @@ impl Config {
         }
     }
 
-    pub fn parse(args: &[String]) -> Config {
+    pub fn parse(flags: Flags) -> Config {
         #[cfg(test)]
         fn get_toml(_: &Path) -> TomlConfig {
             TomlConfig::default()
@@ -1221,11 +1221,10 @@ impl Config {
                     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 {
+    pub(crate) fn parse_inner(mut flags: Flags, get_toml: impl Fn(&Path) -> TomlConfig) -> Config {
-        let mut flags = Flags::parse(args);
         let mut config = Config::default_opts();
 
         // Set flags.

src/bootstrap/src/core/config/flags.rs

@@ -183,9 +183,9 @@ pub struct Flags {
 }
 
 impl Flags {
-    pub fn parse(args: &[String]) -> Self {
+    /// Check if `<cmd> -h -v` was passed.
-        let first = String::from("x.py");
+    /// If yes, print the available paths and return `true`.
-        let it = std::iter::once(&first).chain(args.iter());
+    pub fn try_parse_verbose_help(args: &[String]) -> bool {
         // We need to check for `<cmd> -h -v`, in which case we list the paths
         #[derive(Parser)]
         #[command(disable_help_flag(true))]
@@ -198,10 +198,10 @@ impl Flags {
             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 @@ impl Flags {
             } 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<String> {
+    let first = String::from("x.py");
+    let it = std::iter::once(first).chain(args.iter().cloned());
+    it.collect()
 }
 
 #[derive(Debug, Clone, Default, clap::Subcommand)]

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;
 

src/bootstrap/src/core/config/tests.rs

@@ -12,9 +12,10 @@ use crate::core::build_steps::clippy::get_clippy_rules_in_order;
 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()], |&_| {
+    Config::parse_inner(
-        toml::from_str(&config).unwrap()
+        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 @@ runner = "x86_64-runner"
 #[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 profile_user_dist() {
             .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, .. } => {

src/bootstrap/src/lib.rs

@@ -43,7 +43,7 @@ mod core;
 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::{

src/bootstrap/src/utils/helpers/tests.rs

@@ -5,7 +5,7 @@ use std::path::PathBuf;
 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");