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.

Sign up for GitHub

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

bootstrap: consolidate subcommand parsing and matching #96003

Merged
merged 1 commit into from
Apr 21, 2022
Merged

bootstrap: consolidate subcommand parsing and matching #96003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
62 changes: 35 additions & 27 deletions src/bootstrap/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::cell::{Cell, RefCell};
use std::collections::BTreeSet;
use std::env;
use std::ffi::OsStr;
use std::fmt::Debug;
use std::fmt::{Debug, Write};
use std::fs;
use std::hash::Hash;
use std::ops::Deref;
Expand Down Expand Up @@ -125,7 +125,8 @@ impl TaskPath {
if found_kind.is_empty() {
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());
}
}
Expand Down Expand Up @@ -406,43 +407,53 @@ pub enum Kind {
Check,
Clippy,
Fix,
Format,
Test,
Bench,
Dist,
Doc,
Clean,
Dist,
Install,
Run,
Setup,
}

impl Kind {
fn parse(string: &str) -> Kind {
match string {
"build" => Kind::Build,
"check" => Kind::Check,
pub fn parse(string: &str) -> Option<Kind> {
// these strings, including the one-letter aliases, must match the x.py help text
Some(match string {
"build" | "b" => Kind::Build,
"check" | "c" => Kind::Check,
"clippy" => Kind::Clippy,
"fix" => Kind::Fix,
"test" => Kind::Test,
"fmt" => Kind::Format,
"test" | "t" => Kind::Test,
"bench" => Kind::Bench,
"doc" | "d" => Kind::Doc,
"clean" => Kind::Clean,
"dist" => Kind::Dist,
"doc" => Kind::Doc,
"install" => Kind::Install,
"run" => Kind::Run,
other => panic!("unknown kind: {}", other),
}
"run" | "r" => Kind::Run,
"setup" => Kind::Setup,
_ => return None,
})
}

fn as_str(&self) -> &'static str {
pub fn as_str(&self) -> &'static str {
match self {
Kind::Build => "build",
Kind::Check => "check",
Kind::Clippy => "clippy",
Kind::Fix => "fix",
Kind::Format => "fmt",
Kind::Test => "test",
Kind::Bench => "bench",
Kind::Dist => "dist",
Kind::Doc => "doc",
Kind::Clean => "clean",
Kind::Dist => "dist",
Kind::Install => "install",
Kind::Run => "run",
Kind::Setup => "setup",
}
}
}
Expand Down Expand Up @@ -486,7 +497,7 @@ impl<'a> Builder<'a> {
native::Lld,
native::CrtBeginEnd
),
Kind::Check | Kind::Clippy { .. } | Kind::Fix => describe!(
Kind::Check => describe!(
check::Std,
check::Rustc,
check::Rustdoc,
Expand Down Expand Up @@ -616,32 +627,29 @@ impl<'a> Builder<'a> {
install::Rustc
),
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> {
let kind = match subcommand {
"build" | "b" => Kind::Build,
"doc" | "d" => Kind::Doc,
"test" | "t" => Kind::Test,
"bench" => Kind::Bench,
"dist" => Kind::Dist,
"install" => Kind::Install,
_ => return None,
};
pub fn get_help(build: &Build, kind: Kind) -> Option<String> {
let step_descriptions = Builder::get_step_descriptions(kind);
if step_descriptions.is_empty() {
return None;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was part of the logic for printing the hint to run with -h -v to see available paths if and only if there are paths that we could list for that subcommand.

For example, in Rust now if you run ./x.py clippy -h it will suggest Run './x.py clippy -h -v' to see a list of available paths., but when you run ./x.py clippy -v -v it doesn't actually list any paths.

My assumption was that all of these commands could have useful paths to pass to them but I might be mistaken about that. It seems like check and clippy would take paths just like build does, but it kinda seems like clippy runs for "everything" in the build graph anyway and passing paths doesn't do much.

Is this specific list of commands which could print a list of paths for help text intentional and the others (clippy, check, fix) are supposed to be excluded?

Copy link
Member

@jyn514 jyn514 Apr 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption was that all of these commands could have useful paths to pass to them but I might be mistaken about that.

This is not currently true. There are several related issues:

  1. Not all commands accept paths. x clippy, as you noticed, runs on the whole tree. x clean does the same.
  2. Some commands that do not accept paths do not check whether a path was passed - x clippy src/bootstrap is exactly the same as an unconditional x clippy.
  3. Some commands the do accept paths do not go through StepDescription. For example x fmt src/bootstrap is perfectly valid and limits the changes to bootstrap itself, but there is no impl Step for Format description, because Step has no way to say "all paths".

Fixing those issues should wait for follow up PRs, I think. In the meantime, can you do the following:

  • Remove clippy and fix from the check arm and down to the vec![] arm. clippy is certainly not supported with paths, and fix has been broken for ages.
  • Change the code in usage that calls get_help to panic if -h -v is called for a subcommand that does not support paths. We should give the user an idea that something is broken rather than silently ignoring the flag.

}

let builder = Self::new_internal(build, kind, vec![]);
let builder = &builder;
// The "build" kind here is just a placeholder, it will be replaced with something else in
// the following statement.
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 = (desc.should_run)(should_run);
}
let mut help = String::from("Available paths:\n");
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 {
match pathset {
Expand Down
Loading