Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Slightly refactor Flags in bootstrap #128878

Merged
merged 2 commits into from
Aug 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/bootstrap/src/bin/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 3 additions & 2 deletions src/bootstrap/src/core/builder/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand Down
7 changes: 3 additions & 4 deletions src/bootstrap/src/core/config/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1188,7 +1188,7 @@ impl Config {
}
}

pub fn parse(args: &[String]) -> Config {
pub fn parse(flags: Flags) -> Config {
#[cfg(test)]
fn get_toml(_: &Path) -> TomlConfig {
TomlConfig::default()
Expand Down Expand Up @@ -1218,11 +1218,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 {
let mut flags = Flags::parse(args);
pub(crate) fn parse_inner(mut flags: Flags, get_toml: impl Fn(&Path) -> TomlConfig) -> Config {
onur-ozkan marked this conversation as resolved.
Show resolved Hide resolved
let mut config = Config::default_opts();

// Set flags.
Expand Down
24 changes: 17 additions & 7 deletions src/bootstrap/src/core/config/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<cmd> -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 `<cmd> -h -v`, in which case we list the paths
#[derive(Parser)]
#[command(disable_help_flag(true))]
Expand All @@ -198,24 +198,34 @@ 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 {
println!("{s}");
} 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))
}
Kobzol marked this conversation as resolved.
Show resolved Hide resolved
}

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)]
pub enum Subcommand {
#[command(aliases = ["b"], long_about = "\n
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/src/core/config/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#[allow(clippy::module_inception)]
mod config;
pub(crate) mod flags;
pub mod flags;
#[cfg(test)]
mod tests;

Expand Down
19 changes: 10 additions & 9 deletions src/bootstrap/src/core/config/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()], |&_| {
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]
Expand Down Expand Up @@ -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(),
Expand All @@ -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#"
Expand Down Expand Up @@ -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(),
);
}
Expand All @@ -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]
Expand Down Expand Up @@ -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, .. } => {
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down
8 changes: 5 additions & 3 deletions src/bootstrap/src/utils/helpers/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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());
Expand All @@ -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");

Expand Down
Loading