squiid/rust
1
0
Fork 0
Code Issues Pull requests Projects Activity

Auto merge of #96003 - aswild:pr/bootstrap-subcommands-cleanup, r=jyn514

Browse source 
bootstrap: consolidate subcommand parsing and matching

There's several places where the x.py command names are matched as
strings, leading to some inconsistencies and opportunities for cleanup.

* Add Format, Clean, and Setup variants to builder::Kind.
* Use Kind to parse the x.py subcommand name (including aliases)
* Match on the subcommand Kind rather than strings when handling
  options and help text.
* Several subcommands don't display any paths when run with `-h -v` even
  though the help text indicates that they should. Fix this and refactor
  so that manually keeping matches in sync isn't necessary.

Fixes #95937
This commit is contained in:
bors 2022-04-21 10:38:43 +00:00
parent 3d3dafb771 870cb8ef92
commit 1dec35a1b0
2 changed files with 85 additions and 103 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

62
src/bootstrap/builder.rs
View file

@ -3,7 +3,7 @@ use std::cell::{Cell, RefCell};
use std::collections::BTreeSet; use std::collections::BTreeSet;
use std::env; use std::env;
use std::ffi::OsStr; use std::ffi::OsStr;
use std::fmt::Debug; use std::fmt::{Debug, Write};
use std::fs; use std::fs;
use std::hash::Hash; use std::hash::Hash;
use std::ops::Deref; use std::ops::Deref;
@ -125,7 +125,8 @@ impl TaskPath {
if found_kind.is_empty() { if found_kind.is_empty() {
panic!("empty kind in task path {}", path.display()); panic!("empty kind in task path {}", path.display());
} }
kind = Some(Kind::parse(found_kind)); kind = Kind::parse(found_kind);
assert!(kind.is_some());
path = Path::new(found_prefix).join(components.as_path()); path = Path::new(found_prefix).join(components.as_path());
} }
} }
@ -431,43 +432,53 @@ pub enum Kind {
Check, Check,
Clippy, Clippy,
Fix, Fix,
Format,
Test, Test,
Bench, Bench,
Dist,
Doc, Doc,
Clean,
Dist,
Install, Install,
Run, Run,
Setup,
} }
impl Kind { impl Kind {
fn parse(string: &str) -> Kind { pub fn parse(string: &str) -> Option<Kind> {
match string { // these strings, including the one-letter aliases, must match the x.py help text
"build" => Kind::Build, Some(match string {
"check" => Kind::Check, "build" | "b" => Kind::Build,
"check" | "c" => Kind::Check,
"clippy" => Kind::Clippy, "clippy" => Kind::Clippy,
"fix" => Kind::Fix, "fix" => Kind::Fix,
"test" => Kind::Test, "fmt" => Kind::Format,
"test" | "t" => Kind::Test,
"bench" => Kind::Bench, "bench" => Kind::Bench,
"doc" | "d" => Kind::Doc,
"clean" => Kind::Clean,
"dist" => Kind::Dist, "dist" => Kind::Dist,
"doc" => Kind::Doc,
"install" => Kind::Install, "install" => Kind::Install,
"run" => Kind::Run, "run" | "r" => Kind::Run,
other => panic!("unknown kind: {}", other), "setup" => Kind::Setup,
} _ => return None,
})
} }
fn as_str(&self) -> &'static str { pub fn as_str(&self) -> &'static str {
match self { match self {
Kind::Build => "build", Kind::Build => "build",
Kind::Check => "check", Kind::Check => "check",
Kind::Clippy => "clippy", Kind::Clippy => "clippy",
Kind::Fix => "fix", Kind::Fix => "fix",
Kind::Format => "fmt",
Kind::Test => "test", Kind::Test => "test",
Kind::Bench => "bench", Kind::Bench => "bench",
Kind::Dist => "dist",
Kind::Doc => "doc", Kind::Doc => "doc",
Kind::Clean => "clean",
Kind::Dist => "dist",
Kind::Install => "install", Kind::Install => "install",
Kind::Run => "run", Kind::Run => "run",
Kind::Setup => "setup",
} }
} }
} }
@ -511,7 +522,7 @@ impl<'a> Builder<'a> {
native::Lld, native::Lld,
native::CrtBeginEnd native::CrtBeginEnd
), ),
Kind::Check | Kind::Clippy { .. } | Kind::Fix => describe!( Kind::Check => describe!(
check::Std, check::Std,
check::Rustc, check::Rustc,
check::Rustdoc, check::Rustdoc,
@ -641,32 +652,29 @@ impl<'a> Builder<'a> {
install::Rustc install::Rustc
), ),
Kind::Run => describe!(run::ExpandYamlAnchors, run::BuildManifest, run::BumpStage0), Kind::Run => describe!(run::ExpandYamlAnchors, run::BuildManifest, run::BumpStage0),
// These commands either don't use paths, or they're special-cased in Build::build()
Kind::Clean | Kind::Clippy | Kind::Fix | Kind::Format | Kind::Setup => vec![],
} }
} }
pub fn get_help(build: &Build, subcommand: &str) -> Option<String> { pub fn get_help(build: &Build, kind: Kind) -> Option<String> {
let kind = match subcommand { let step_descriptions = Builder::get_step_descriptions(kind);
"build" | "b" => Kind::Build, if step_descriptions.is_empty() {
"doc" | "d" => Kind::Doc, return None;
"test" | "t" => Kind::Test, }
"bench" => Kind::Bench,
"dist" => Kind::Dist,
"install" => Kind::Install,
_ => return None,
};
let builder = Self::new_internal(build, kind, vec![]); let builder = Self::new_internal(build, kind, vec![]);
let builder = &builder; let builder = &builder;
// The "build" kind here is just a placeholder, it will be replaced with something else in // The "build" kind here is just a placeholder, it will be replaced with something else in
// the following statement. // the following statement.
let mut should_run = ShouldRun::new(builder, Kind::Build); let mut should_run = ShouldRun::new(builder, Kind::Build);
for desc in Builder::get_step_descriptions(builder.kind) { for desc in step_descriptions {
should_run.kind = desc.kind; should_run.kind = desc.kind;
should_run = (desc.should_run)(should_run); should_run = (desc.should_run)(should_run);
} }
let mut help = String::from("Available paths:\n"); let mut help = String::from("Available paths:\n");
let mut add_path = |path: &Path| { let mut add_path = |path: &Path| {
help.push_str(&format!(" ./x.py {} {}\n", subcommand, path.display())); t!(write!(help, " ./x.py {} {}\n", kind.as_str(), path.display()));
}; };
for pathset in should_run.paths { for pathset in should_run.paths {
match pathset { match pathset {

122
src/bootstrap/flags.rs
View file

@ -8,7 +8,7 @@ use std::process;
use getopts::Options; use getopts::Options;
use crate::builder::Builder; use crate::builder::{Builder, Kind};
use crate::config::{Config, TargetSelection}; use crate::config::{Config, TargetSelection};
use crate::setup::Profile; use crate::setup::Profile;
use crate::util::t; use crate::util::t;
@ -243,27 +243,7 @@ To learn more about a subcommand, run `./x.py <subcommand> -h`",
// the subcommand. Therefore we must manually identify the subcommand first, so that we can // the subcommand. Therefore we must manually identify the subcommand first, so that we can
// complete the definition of the options. Then we can use the getopt::Matches object from // complete the definition of the options. Then we can use the getopt::Matches object from
// there on out. // there on out.
let subcommand = args.iter().find(|&s| { let subcommand = match args.iter().find_map(|s| Kind::parse(&s)) {
(s == "build")
|| (s == "b")
|| (s == "check")
|| (s == "c")
|| (s == "clippy")
|| (s == "fix")
|| (s == "fmt")
|| (s == "test")
|| (s == "t")
|| (s == "bench")
|| (s == "doc")
|| (s == "d")
|| (s == "clean")
|| (s == "dist")
|| (s == "install")
|| (s == "run")
|| (s == "r")
|| (s == "setup")
});
let subcommand = match subcommand {
Some(s) => s, Some(s) => s,
None => { None => {
// No or an invalid subcommand -- show the general usage and subcommand help // No or an invalid subcommand -- show the general usage and subcommand help
@ -276,8 +256,8 @@ To learn more about a subcommand, run `./x.py <subcommand> -h`",
}; };
// Some subcommands get extra options // Some subcommands get extra options
match subcommand.as_str() { match subcommand {
"test" | "t" => { Kind::Test => {
opts.optflag("", "no-fail-fast", "Run all tests regardless of failure"); opts.optflag("", "no-fail-fast", "Run all tests regardless of failure");
opts.optmulti( opts.optmulti(
"", "",
@ -316,22 +296,22 @@ To learn more about a subcommand, run `./x.py <subcommand> -h`",
`/<build_base>/rustfix_missing_coverage.txt`", `/<build_base>/rustfix_missing_coverage.txt`",
); );
} }
"check" | "c" => { Kind::Check => {
opts.optflag("", "all-targets", "Check all targets"); opts.optflag("", "all-targets", "Check all targets");
} }
"bench" => { Kind::Bench => {
opts.optmulti("", "test-args", "extra arguments", "ARGS"); opts.optmulti("", "test-args", "extra arguments", "ARGS");
} }
"clippy" => { Kind::Clippy => {
opts.optflag("", "fix", "automatically apply lint suggestions"); opts.optflag("", "fix", "automatically apply lint suggestions");
} }
"doc" | "d" => { Kind::Doc => {
opts.optflag("", "open", "open the docs in a browser"); opts.optflag("", "open", "open the docs in a browser");
} }
"clean" => { Kind::Clean => {
opts.optflag("", "all", "clean all build artifacts"); opts.optflag("", "all", "clean all build artifacts");
} }
"fmt" => { Kind::Format => {
opts.optflag("", "check", "check formatting instead of applying."); opts.optflag("", "check", "check formatting instead of applying.");
} }
_ => {} _ => {}
@ -339,25 +319,22 @@ To learn more about a subcommand, run `./x.py <subcommand> -h`",
// fn usage() // fn usage()
let usage = |exit_code: i32, opts: &Options, verbose: bool, subcommand_help: &str| -> ! { let usage = |exit_code: i32, opts: &Options, verbose: bool, subcommand_help: &str| -> ! {
let mut extra_help = String::new();
// All subcommands except `clean` can have an optional "Available paths" section
if verbose {
let config = Config::parse(&["build".to_string()]); let config = Config::parse(&["build".to_string()]);
let build = Build::new(config); let build = Build::new(config);
let paths = Builder::get_help(&build, subcommand);
let maybe_rules_help = Builder::get_help(&build, subcommand.as_str());
extra_help.push_str(maybe_rules_help.unwrap_or_default().as_str());
} else if !(subcommand.as_str() == "clean" || subcommand.as_str() == "fmt") {
extra_help.push_str(
format!("Run `./x.py {} -h -v` to see a list of available paths.", subcommand)
.as_str(),
);
}
println!("{}", opts.usage(subcommand_help)); println!("{}", opts.usage(subcommand_help));
if !extra_help.is_empty() { if let Some(s) = paths {
println!("{}", extra_help); if verbose {
println!("{}", s);
} else {
println!(
"Run `./x.py {} -h -v` to see a list of available paths.",
subcommand.as_str()
);
}
} else if verbose {
panic!("No paths available for subcommand `{}`", subcommand.as_str());
} }
process::exit(exit_code); process::exit(exit_code);
}; };
@ -375,7 +352,7 @@ To learn more about a subcommand, run `./x.py <subcommand> -h`",
// ^-- option ^ ^- actual subcommand // ^-- option ^ ^- actual subcommand
// \_ arg to option could be mistaken as subcommand // \_ arg to option could be mistaken as subcommand
let mut pass_sanity_check = true; let mut pass_sanity_check = true;
match matches.free.get(0) { match matches.free.get(0).and_then(|s| Kind::parse(&s)) {
Some(check_subcommand) => { Some(check_subcommand) => {
if check_subcommand != subcommand { if check_subcommand != subcommand {
pass_sanity_check = false; pass_sanity_check = false;
@ -394,8 +371,8 @@ To learn more about a subcommand, run `./x.py <subcommand> -h`",
process::exit(1); process::exit(1);
} }
// Extra help text for some commands // Extra help text for some commands
match subcommand.as_str() { match subcommand {
"build" | "b" => { Kind::Build => {
subcommand_help.push_str( subcommand_help.push_str(
"\n "\n
Arguments: Arguments:
@ -415,7 +392,7 @@ Arguments:
./x.py build ", ./x.py build ",
); );
} }
"check" | "c" => { Kind::Check => {
subcommand_help.push_str( subcommand_help.push_str(
"\n "\n
Arguments: Arguments:
@ -427,7 +404,7 @@ Arguments:
If no arguments are passed then many artifacts are checked.", If no arguments are passed then many artifacts are checked.",
); );
} }
"clippy" => { Kind::Clippy => {
subcommand_help.push_str( subcommand_help.push_str(
"\n "\n
Arguments: Arguments:
@ -438,7 +415,7 @@ Arguments:
./x.py clippy library/core library/proc_macro", ./x.py clippy library/core library/proc_macro",
); );
} }
"fix" => { Kind::Fix => {
subcommand_help.push_str( subcommand_help.push_str(
"\n "\n
Arguments: Arguments:
@ -449,7 +426,7 @@ Arguments:
./x.py fix library/core library/proc_macro", ./x.py fix library/core library/proc_macro",
); );
} }
"fmt" => { Kind::Format => {
subcommand_help.push_str( subcommand_help.push_str(
"\n "\n
Arguments: Arguments:
@ -460,7 +437,7 @@ Arguments:
./x.py fmt --check", ./x.py fmt --check",
); );
} }
"test" | "t" => { Kind::Test => {
subcommand_help.push_str( subcommand_help.push_str(
"\n "\n
Arguments: Arguments:
@ -488,7 +465,7 @@ Arguments:
./x.py test --stage 1", ./x.py test --stage 1",
); );
} }
"doc" | "d" => { Kind::Doc => {
subcommand_help.push_str( subcommand_help.push_str(
"\n "\n
Arguments: Arguments:
@ -506,7 +483,7 @@ Arguments:
./x.py doc --stage 1", ./x.py doc --stage 1",
); );
} }
"run" | "r" => { Kind::Run => {
subcommand_help.push_str( subcommand_help.push_str(
"\n "\n
Arguments: Arguments:
@ -518,7 +495,7 @@ Arguments:
At least a tool needs to be called.", At least a tool needs to be called.",
); );
} }
"setup" => { Kind::Setup => {
subcommand_help.push_str(&format!( subcommand_help.push_str(&format!(
"\n "\n
x.py setup creates a `config.toml` which changes the defaults for x.py itself. x.py setup creates a `config.toml` which changes the defaults for x.py itself.
@ -535,7 +512,7 @@ Arguments:
Profile::all_for_help(" ").trim_end() Profile::all_for_help(" ").trim_end()
)); ));
} }
_ => {} Kind::Bench | Kind::Clean | Kind::Dist | Kind::Install => {}
}; };
// Get any optional paths which occur after the subcommand // Get any optional paths which occur after the subcommand
let mut paths = matches.free[1..].iter().map(|p| p.into()).collect::<Vec<PathBuf>>(); let mut paths = matches.free[1..].iter().map(|p| p.into()).collect::<Vec<PathBuf>>();
@ -547,9 +524,9 @@ Arguments:
usage(0, &opts, verbose, &subcommand_help); usage(0, &opts, verbose, &subcommand_help);
} }
let cmd = match subcommand.as_str() { let cmd = match subcommand {
"build" | "b" => Subcommand::Build { paths }, Kind::Build => Subcommand::Build { paths },
"check" | "c" => { Kind::Check => {
if matches.opt_present("all-targets") { if matches.opt_present("all-targets") {
eprintln!( eprintln!(
"Warning: --all-targets is now on by default and does not need to be passed explicitly." "Warning: --all-targets is now on by default and does not need to be passed explicitly."
@ -557,9 +534,9 @@ Arguments:
} }
Subcommand::Check { paths } Subcommand::Check { paths }
} }
"clippy" => Subcommand::Clippy { paths, fix: matches.opt_present("fix") }, Kind::Clippy => Subcommand::Clippy { paths, fix: matches.opt_present("fix") },
"fix" => Subcommand::Fix { paths }, Kind::Fix => Subcommand::Fix { paths },
"test" | "t" => Subcommand::Test { Kind::Test => Subcommand::Test {
paths, paths,
bless: matches.opt_present("bless"), bless: matches.opt_present("bless"),
force_rerun: matches.opt_present("force-rerun"), force_rerun: matches.opt_present("force-rerun"),
@ -578,9 +555,9 @@ Arguments:
DocTests::Yes DocTests::Yes
}, },
}, },
"bench" => Subcommand::Bench { paths, test_args: matches.opt_strs("test-args") }, Kind::Bench => Subcommand::Bench { paths, test_args: matches.opt_strs("test-args") },
"doc" | "d" => Subcommand::Doc { paths, open: matches.opt_present("open") }, Kind::Doc => Subcommand::Doc { paths, open: matches.opt_present("open") },
"clean" => { Kind::Clean => {
if !paths.is_empty() { if !paths.is_empty() {
println!("\nclean does not take a path argument\n"); println!("\nclean does not take a path argument\n");
usage(1, &opts, verbose, &subcommand_help); usage(1, &opts, verbose, &subcommand_help);
@ -588,17 +565,17 @@ Arguments:
Subcommand::Clean { all: matches.opt_present("all") } Subcommand::Clean { all: matches.opt_present("all") }
} }
"fmt" => Subcommand::Format { check: matches.opt_present("check"), paths }, Kind::Format => Subcommand::Format { check: matches.opt_present("check"), paths },
"dist" => Subcommand::Dist { paths }, Kind::Dist => Subcommand::Dist { paths },
"install" => Subcommand::Install { paths }, Kind::Install => Subcommand::Install { paths },
"run" | "r" => { Kind::Run => {
if paths.is_empty() { if paths.is_empty() {
println!("\nrun requires at least a path!\n"); println!("\nrun requires at least a path!\n");
usage(1, &opts, verbose, &subcommand_help); usage(1, &opts, verbose, &subcommand_help);
} }
Subcommand::Run { paths } Subcommand::Run { paths }
} }
"setup" => { Kind::Setup => {
let profile = if paths.len() > 1 { let profile = if paths.len() > 1 {
println!("\nat most one profile can be passed to setup\n"); println!("\nat most one profile can be passed to setup\n");
usage(1, &opts, verbose, &subcommand_help) usage(1, &opts, verbose, &subcommand_help)
@ -618,9 +595,6 @@ Arguments:
}; };
Subcommand::Setup { profile } Subcommand::Setup { profile }
} }
_ => {
usage(1, &opts, verbose, &subcommand_help);
}
}; };
if let Subcommand::Check { .. } = &cmd { if let Subcommand::Check { .. } = &cmd {