Skip to content

Overhaul Bootstrap (x.py) Command-Line-Parsing & Help Output #41011

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

Merged
merged 12 commits into from
Apr 6, 2017
Merged

Overhaul Bootstrap (x.py) Command-Line-Parsing & Help Output #41011

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e1b0027
Refer to a subcommand as a subcommand.
CleanCut Mar 31, 2017
8ad5c95
When dealing with the list of all possible subcommands, deal with the…
CleanCut Mar 31, 2017
e1c1e09
Don't print build statistics if we explicitly asked for the help mess…
CleanCut Mar 31, 2017
584b405
Vastly improve the help output.
CleanCut Mar 31, 2017
992a59e
Using an untyped, one-letter variable binding as an argument to a fun…
CleanCut Mar 31, 2017
5ba579e
Save my TODO's as comments, so I don't forget.
CleanCut Mar 31, 2017
aa4bd0e
Finish the improvements I planned.
CleanCut Apr 1, 2017
2c9ae48
Oops, we can't parse options until all options have been defined. Ti…
CleanCut Apr 2, 2017
6b72586
Simplify a "use" statement as per @grunweg's feedback.
CleanCut Apr 2, 2017
1e53898
Fix breaking the 'clean' subcommand caused replacing a single-letter …
CleanCut Apr 2, 2017
20cb700
Handle options-with-arguments before subcommands such as './x.py -j 1…
CleanCut Apr 4, 2017
ea2bfae
Branch arms need to match the return value even if it's not being ass…
CleanCut Apr 4, 2017
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
7 changes: 5 additions & 2 deletions src/bootstrap/bootstrap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,16 +591,19 @@ def bootstrap():

def main():
start_time = time()
help_triggered = ('-h' in sys.argv) or ('--help' in sys.argv) or (len(sys.argv) == 1)
try:
bootstrap()
print("Build completed successfully in %s" % format_build_time(time() - start_time))
if not help_triggered:
print("Build completed successfully in %s" % format_build_time(time() - start_time))
except (SystemExit, KeyboardInterrupt) as e:
if hasattr(e, 'code') and isinstance(e.code, int):
exit_code = e.code
else:
exit_code = 1
print(e)
print("Build completed unsuccessfully in %s" % format_build_time(time() - start_time))
if not help_triggered:
print("Build completed unsuccessfully in %s" % format_build_time(time() - start_time))
sys.exit(exit_code)

if __name__ == '__main__':
Expand Down
256 changes: 126 additions & 130 deletions src/bootstrap/flags.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use std::fs;
use std::path::PathBuf;
use std::process;

use getopts::{Matches, Options};
use getopts::Options;

use Build;
use config::Config;
Expand Down Expand Up @@ -75,7 +75,22 @@ pub enum Subcommand {

impl Flags {
pub fn parse(args: &[String]) -> Flags {
let mut extra_help = String::new();
Copy link

Choose a reason for hiding this comment

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

Referencing your commit message, how about leaving a mention that options can be used in any position. I'm only reading the code, so apologies if this is actually present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries. It is mentioned in both the commit message where the change was made (but not on the first line of the message) and in the PR text. I also stated in the PR text that this resolves #38373, which is a request to accept positional arguments after options.

If there is something I can do to make it more clear, I'd be happy to. Just let me know. Should I amend the commit it was in to get that info into the first commit line?

Copy link

Choose a reason for hiding this comment

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

I see; I did notice this in the PR text and commit messages. I was rather wondering if you could adjust the usage string to mention this - after all, users of the script might not always look up the file's history.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see. What do you suggest would make it clearer? I haven't observed a standard help message that indicates such a capability, rather it seems to be generally assumed. My first inclination would be to annotate the "Options:" header in some way, but that header is part of the generated help string by getopts in this instance.

let mut subcommand_help = format!("\
Usage: x.py <subcommand> [options] [<paths>...]

Subcommands:
build Compile either the compiler or libraries
test Build and run some test suites
bench Build and run some benchmarks
doc Build documentation
clean Clean out build directories
dist Build and/or install distribution artifacts

To learn more about a subcommand, run `./x.py <subcommand> -h`");

let mut opts = Options::new();
// Options common to all subcommands
opts.optflagmulti("v", "verbose", "use verbose output (-vv for very verbose)");
opts.optflag("i", "incremental", "use incremental compilation");
opts.optopt("", "config", "TOML configuration file for build", "FILE");
Expand All @@ -89,21 +104,41 @@ impl Flags {
opts.optopt("j", "jobs", "number of jobs to run in parallel", "JOBS");
opts.optflag("h", "help", "print this help message");

let usage = |n, opts: &Options| -> ! {
let command = args.get(0).map(|s| &**s);
let brief = format!("Usage: x.py {} [options] [<args>...]",
command.unwrap_or("<command>"));
// fn usage()
let usage = |exit_code: i32, opts: &Options, subcommand_help: &str, extra_help: &str| -> ! {
println!("{}", opts.usage(subcommand_help));
if !extra_help.is_empty() {
println!("{}", extra_help);
}
process::exit(exit_code);
};

// We can't use getopt to parse the options until we have completed specifying which
// options are valid, but under the current implementation, some options are conditional on
// 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
// there on out.
let mut possible_subcommands = args.iter().collect::<Vec<_>>();
possible_subcommands.retain(|&s| !s.starts_with('-'));
let subcommand = match possible_subcommands.first() {
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately I don't think this is quite right, for example ./x.py --stage 0 build would not parse correctly as it would assume the command is 0, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Yet another reason getopts should have some parse() variant that will not error out if an invalid option is encountered. Then we could just parse once for the subcommand, add more options, and then parse again.

Hmmm...options with their own arguments. That's actually a tough nut to crack without rewriting a lot of our own getopt-like logic.

What if we cheated and took the first argument not starting with - AND that is one of our valid subcommands? Is there any overlap between arguments to options and subcommands?

I'll see what I can do to fix this later tonight.

Copy link
Member

Choose a reason for hiding this comment

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

I unfortunately don't really know a great solution here, I've reached the same conclusion as you have which is that most of the option are relatively unfortunate.

This is why the "bug" exists today where you can't say ./x.py -j1 build, I didn't feel that there was a good enough solution to implement to actually allow that. I'd always be more on board, however, with simply having a much nicer error message!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I devised a pretty good solution.

  • Manually look for the first occurence of an argument that matches a known subcommand. Assume it is the subcommand.
  • Add any subcommand-specific options.
  • Parse using getopts
  • Do a sanity check using getopts to make sure that the subcommand we found was correct. If not, exit with the usage and a nice message advising you to move options after subcommands for this case.

This should work. The only time you would ever hit the error condition would be the pathological condition where you put an option with an argument which could have been a subcommand before the actual subcommand. Like this:

./x.py --option clean build

Copy link
Member

Choose a reason for hiding this comment

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

I'm personally ok with a better error message, but if you're willing to implement it that sounds reasonable to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sweet. I just pushed my latest commit. Hopefully I've managed to make it all work without breaking anything...

Some(s) => s,
None => {
// No subcommand -- show the general usage and subcommand help
println!("{}\n", subcommand_help);
process::exit(0);
}
};

println!("{}", opts.usage(&brief));
match command {
Some("build") => {
println!("\
// Some subcommands have specific arguments help text
match subcommand.as_str() {
"build" => {
subcommand_help.push_str("\n
Arguments:
This subcommand accepts a number of positional arguments of directories to
the crates and/or artifacts to compile. For example:
This subcommand accepts a number of paths to directories to the crates
and/or artifacts to compile. For example:

./x.py build src/libcore
./x.py build src/libproc_macro
./x.py build src/libcore src/libproc_macro
./x.py build src/libstd --stage 1

If no arguments are passed then the complete artifacts for that stage are
Expand All @@ -114,15 +149,14 @@ Arguments:

For a quick build with a usable compile, you can pass:

./x.py build --stage 1 src/libtest
");
}

Some("test") => {
println!("\
./x.py build --stage 1 src/libtest");
}
"test" => {
opts.optmulti("", "test-args", "extra arguments", "ARGS");
subcommand_help.push_str("\n
Arguments:
This subcommand accepts a number of positional arguments of directories to
tests that should be compiled and run. For example:
This subcommand accepts a number of paths to directories to tests that
should be compiled and run. For example:

./x.py test src/test/run-pass
./x.py test src/libstd --test-args hash_map
Expand All @@ -132,171 +166,133 @@ Arguments:
compiled and tested.

./x.py test
./x.py test --stage 1
");
}

Some("doc") => {
println!("\
./x.py test --stage 1");
}
"bench" => {
opts.optmulti("", "test-args", "extra arguments", "ARGS");
}
"doc" => {
subcommand_help.push_str("\n
Arguments:
This subcommand accepts a number of positional arguments of directories of
documentation to build. For example:
This subcommand accepts a number of paths to directories of documentation
to build. For example:

./x.py doc src/doc/book
./x.py doc src/doc/nomicon
./x.py doc src/libstd
./x.py doc src/doc/book src/libstd

If no arguments are passed then everything is documented:

./x.py doc
./x.py doc --stage 1
");
}

_ => {}
./x.py doc --stage 1");
}

if let Some(command) = command {
if command == "build" ||
command == "dist" ||
command == "doc" ||
command == "test" ||
command == "bench" ||
command == "clean" {
println!("Available invocations:");
if args.iter().any(|a| a == "-v") {
let flags = Flags::parse(&["build".to_string()]);
let mut config = Config::default();
config.build = flags.build.clone();
let mut build = Build::new(flags, config);
metadata::build(&mut build);
step::build_rules(&build).print_help(command);
} else {
println!(" ... elided, run `./x.py {} -h -v` to see",
command);
}

println!("");
}
}

println!("\
Subcommands:
build Compile either the compiler or libraries
test Build and run some test suites
bench Build and run some benchmarks
doc Build documentation
clean Clean out build directories
dist Build and/or install distribution artifacts

To learn more about a subcommand, run `./x.py <command> -h`
");

process::exit(n);
};
if args.len() == 0 {
println!("a command must be passed");
usage(1, &opts);
}
let parse = |opts: &Options| {
let m = opts.parse(&args[1..]).unwrap_or_else(|e| {
println!("failed to parse options: {}", e);
usage(1, opts);
});
if m.opt_present("h") {
usage(0, opts);
"dist" => {
opts.optflag("", "install", "run installer as well");
}
return m
_ => { }
};

// Done specifying what options are possible, so do the getopts parsing
let matches = opts.parse(&args[..]).unwrap_or_else(|e| {
// Invalid argument/option format
println!("\n{}\n", e);
usage(1, &opts, &subcommand_help, &extra_help);
});
// Get any optional paths which occur after the subcommand
let cwd = t!(env::current_dir());
let remaining_as_path = |m: &Matches| {
m.free.iter().map(|p| cwd.join(p)).collect::<Vec<_>>()
};
let paths = matches.free[1..].iter().map(|p| cwd.join(p)).collect::<Vec<_>>();


// All subcommands can have an optional "Available paths" section
if matches.opt_present("verbose") {
let flags = Flags::parse(&["build".to_string()]);
let mut config = Config::default();
config.build = flags.build.clone();
let mut build = Build::new(flags, config);
metadata::build(&mut build);
let maybe_rules_help = step::build_rules(&build).get_help(subcommand);
if maybe_rules_help.is_some() {
extra_help.push_str(maybe_rules_help.unwrap().as_str());
}
} else {
extra_help.push_str(format!("Run `./x.py {} -h -v` to see a list of available paths.",
subcommand).as_str());
}

// User passed in -h/--help?
if matches.opt_present("help") {
usage(0, &opts, &subcommand_help, &extra_help);
}

let m: Matches;
let cmd = match &args[0][..] {
let cmd = match subcommand.as_str() {
"build" => {
m = parse(&opts);
Subcommand::Build { paths: remaining_as_path(&m) }
}
"doc" => {
m = parse(&opts);
Subcommand::Doc { paths: remaining_as_path(&m) }
Subcommand::Build { paths: paths }
}
"test" => {
opts.optmulti("", "test-args", "extra arguments", "ARGS");
m = parse(&opts);
Subcommand::Test {
paths: remaining_as_path(&m),
test_args: m.opt_strs("test-args"),
paths: paths,
test_args: matches.opt_strs("test-args"),
}
}
"bench" => {
opts.optmulti("", "test-args", "extra arguments", "ARGS");
m = parse(&opts);
Subcommand::Bench {
paths: remaining_as_path(&m),
test_args: m.opt_strs("test-args"),
paths: paths,
test_args: matches.opt_strs("test-args"),
}
}
"doc" => {
Subcommand::Doc { paths: paths }
}
"clean" => {
m = parse(&opts);
if m.free.len() > 0 {
println!("clean takes no arguments");
usage(1, &opts);
if matches.free.len() > 0 {
println!("\nclean takes no arguments\n");
usage(1, &opts, &subcommand_help, &extra_help);
}
Subcommand::Clean
}
"dist" => {
opts.optflag("", "install", "run installer as well");
m = parse(&opts);
Subcommand::Dist {
paths: remaining_as_path(&m),
install: m.opt_present("install"),
paths: paths,
install: matches.opt_present("install"),
}
}
"--help" => usage(0, &opts),
cmd => {
println!("unknown command: {}", cmd);
usage(1, &opts);
_ => {
usage(1, &opts, &subcommand_help, &extra_help);
}
};


let cfg_file = m.opt_str("config").map(PathBuf::from).or_else(|| {
let cfg_file = matches.opt_str("config").map(PathBuf::from).or_else(|| {
if fs::metadata("config.toml").is_ok() {
Some(PathBuf::from("config.toml"))
} else {
None
}
});

let mut stage = m.opt_str("stage").map(|j| j.parse().unwrap());

let incremental = m.opt_present("i");
let mut stage = matches.opt_str("stage").map(|j| j.parse().unwrap());

if incremental {
if matches.opt_present("incremental") {
if stage.is_none() {
stage = Some(1);
}
}

Flags {
verbose: m.opt_count("v"),
verbose: matches.opt_count("verbose"),
stage: stage,
on_fail: m.opt_str("on-fail"),
keep_stage: m.opt_str("keep-stage").map(|j| j.parse().unwrap()),
build: m.opt_str("build").unwrap_or_else(|| {
on_fail: matches.opt_str("on-fail"),
keep_stage: matches.opt_str("keep-stage").map(|j| j.parse().unwrap()),
build: matches.opt_str("build").unwrap_or_else(|| {
env::var("BUILD").unwrap()
}),
host: split(m.opt_strs("host")),
target: split(m.opt_strs("target")),
host: split(matches.opt_strs("host")),
target: split(matches.opt_strs("target")),
config: cfg_file,
src: m.opt_str("src").map(PathBuf::from),
jobs: m.opt_str("jobs").map(|j| j.parse().unwrap()),
src: matches.opt_str("src").map(PathBuf::from),
jobs: matches.opt_str("jobs").map(|j| j.parse().unwrap()),
cmd: cmd,
incremental: incremental,
incremental: matches.opt_present("incremental"),
}
}
}
Expand Down
Loading