From e1b0027b51d8c8b7558513565c2baa45f1b1b984 Mon Sep 17 00:00:00 2001 From: Nathan Stocks Date: Thu, 30 Mar 2017 20:49:06 -0600 Subject: [PATCH 01/12] Refer to a subcommand as a subcommand. For some reason 'command' and 'subcommand' were intermixed to mean the same thing. Lets just call it the one thing that it is. --- src/bootstrap/flags.rs | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/src/bootstrap/flags.rs b/src/bootstrap/flags.rs index b55f3d710ca7b..ea0fc97e22a7b 100644 --- a/src/bootstrap/flags.rs +++ b/src/bootstrap/flags.rs @@ -90,12 +90,11 @@ impl Flags { 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] [...]", - command.unwrap_or("")); + let subcommand = args.get(0).map(|s| &**s); + let brief = format!("Usage: x.py [options] [...]"); println!("{}", opts.usage(&brief)); - match command { + match subcommand { Some("build") => { println!("\ Arguments: @@ -156,13 +155,13 @@ Arguments: _ => {} } - if let Some(command) = command { - if command == "build" || - command == "dist" || - command == "doc" || - command == "test" || - command == "bench" || - command == "clean" { + if let Some(subcommand) = subcommand { + if subcommand == "build" || + subcommand == "dist" || + subcommand == "doc" || + subcommand == "test" || + subcommand == "bench" || + subcommand == "clean" { println!("Available invocations:"); if args.iter().any(|a| a == "-v") { let flags = Flags::parse(&["build".to_string()]); @@ -170,10 +169,10 @@ Arguments: config.build = flags.build.clone(); let mut build = Build::new(flags, config); metadata::build(&mut build); - step::build_rules(&build).print_help(command); + step::build_rules(&build).print_help(subcommand); } else { println!(" ... elided, run `./x.py {} -h -v` to see", - command); + subcommand); } println!(""); @@ -189,13 +188,13 @@ Subcommands: clean Clean out build directories dist Build and/or install distribution artifacts -To learn more about a subcommand, run `./x.py -h` +To learn more about a subcommand, run `./x.py -h` "); process::exit(n); }; if args.len() == 0 { - println!("a command must be passed"); + println!("a subcommand must be passed"); usage(1, &opts); } let parse = |opts: &Options| { @@ -258,7 +257,7 @@ To learn more about a subcommand, run `./x.py -h` } "--help" => usage(0, &opts), cmd => { - println!("unknown command: {}", cmd); + println!("unknown subcommand: {}", cmd); usage(1, &opts); } }; From 8ad5c95e521f90897a6bd611956d476fcc51c042 Mon Sep 17 00:00:00 2001 From: Nathan Stocks Date: Thu, 30 Mar 2017 20:58:07 -0600 Subject: [PATCH 02/12] When dealing with the list of all possible subcommands, deal with them in the same order to ease comparing the sections of code in order. I chose the order that appears in the help text, because that is most likely to have been ordered with specific reasoning. --- src/bootstrap/flags.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/bootstrap/flags.rs b/src/bootstrap/flags.rs index ea0fc97e22a7b..556a362a8749a 100644 --- a/src/bootstrap/flags.rs +++ b/src/bootstrap/flags.rs @@ -157,11 +157,11 @@ Arguments: if let Some(subcommand) = subcommand { if subcommand == "build" || - subcommand == "dist" || - subcommand == "doc" || subcommand == "test" || subcommand == "bench" || - subcommand == "clean" { + subcommand == "doc" || + subcommand == "clean" || + subcommand == "dist" { println!("Available invocations:"); if args.iter().any(|a| a == "-v") { let flags = Flags::parse(&["build".to_string()]); @@ -219,10 +219,6 @@ To learn more about a subcommand, run `./x.py -h` m = parse(&opts); Subcommand::Build { paths: remaining_as_path(&m) } } - "doc" => { - m = parse(&opts); - Subcommand::Doc { paths: remaining_as_path(&m) } - } "test" => { opts.optmulti("", "test-args", "extra arguments", "ARGS"); m = parse(&opts); @@ -239,6 +235,10 @@ To learn more about a subcommand, run `./x.py -h` test_args: m.opt_strs("test-args"), } } + "doc" => { + m = parse(&opts); + Subcommand::Doc { paths: remaining_as_path(&m) } + } "clean" => { m = parse(&opts); if m.free.len() > 0 { From e1c1e09867e489a41170e726fe64281caaca087a Mon Sep 17 00:00:00 2001 From: Nathan Stocks Date: Thu, 30 Mar 2017 22:12:01 -0600 Subject: [PATCH 03/12] Don't print build statistics if we explicitly asked for the help message. --- src/bootstrap/bootstrap.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/bootstrap/bootstrap.py b/src/bootstrap/bootstrap.py index d5bc6127a1e7f..73f3b1d1cebab 100644 --- a/src/bootstrap/bootstrap.py +++ b/src/bootstrap/bootstrap.py @@ -593,14 +593,16 @@ def main(): start_time = time() try: bootstrap() - print("Build completed successfully in %s" % format_build_time(time() - start_time)) + if ('-h' not in sys.argv) and ('--help' not in sys.argv): + 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 ('-h' not in sys.argv) and ('--help' not in sys.argv): + print("Build completed unsuccessfully in %s" % format_build_time(time() - start_time)) sys.exit(exit_code) if __name__ == '__main__': From 584b40578d8ab999031da0855f319a94db06dc47 Mon Sep 17 00:00:00 2001 From: Nathan Stocks Date: Thu, 30 Mar 2017 22:18:27 -0600 Subject: [PATCH 04/12] Vastly improve the help output. - Don't print 'unknown subcommand' at the top of the help message. The help message now clearly instructs the user to provide a subcommand. - Clarify the usage line. Subcommand is required. Don't echo invalid input back out in the usage line (what the...???). args renamed to paths, because that's what all the args are referred to elsewhere. - List the available subcommands immediately following the usage line. It's the one required argument, after all. - Slightly improve the extra documentation for the build, test, and doc commands. - Don't print 'Available invocations:' at all. It occurred immediately before 'Available paths:'. - Clearly state that running with '-h -v' will produce a list of available paths. --- src/bootstrap/flags.rs | 53 +++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/src/bootstrap/flags.rs b/src/bootstrap/flags.rs index 556a362a8749a..1a260050a9422 100644 --- a/src/bootstrap/flags.rs +++ b/src/bootstrap/flags.rs @@ -90,19 +90,31 @@ impl Flags { opts.optflag("h", "help", "print this help message"); let usage = |n, opts: &Options| -> ! { - let subcommand = args.get(0).map(|s| &**s); - let brief = format!("Usage: x.py [options] [...]"); + let subcommand_help = format!("\ +Usage: x.py [options] [...] + +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 - println!("{}", opts.usage(&brief)); +To learn more about a subcommand, run `./x.py -h`"); + + println!("{}", opts.usage(&subcommand_help)); + + let subcommand = args.get(0).map(|s| &**s); match subcommand { Some("build") => { println!("\ 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 @@ -120,8 +132,8 @@ Arguments: Some("test") => { println!("\ 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 @@ -138,12 +150,12 @@ Arguments: Some("doc") => { println!("\ 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: @@ -155,6 +167,7 @@ Arguments: _ => {} } + if let Some(subcommand) = subcommand { if subcommand == "build" || subcommand == "test" || @@ -162,7 +175,6 @@ Arguments: subcommand == "doc" || subcommand == "clean" || subcommand == "dist" { - println!("Available invocations:"); if args.iter().any(|a| a == "-v") { let flags = Flags::parse(&["build".to_string()]); let mut config = Config::default(); @@ -171,7 +183,7 @@ Arguments: metadata::build(&mut build); step::build_rules(&build).print_help(subcommand); } else { - println!(" ... elided, run `./x.py {} -h -v` to see", + println!("Run `./x.py {} -h -v` to see a list of available paths.", subcommand); } @@ -179,18 +191,6 @@ Arguments: } } -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 -h` -"); - process::exit(n); }; if args.len() == 0 { @@ -256,8 +256,7 @@ To learn more about a subcommand, run `./x.py -h` } } "--help" => usage(0, &opts), - cmd => { - println!("unknown subcommand: {}", cmd); + _ => { usage(1, &opts); } }; From 992a59efc3e3fd976728fc19a1a854afcbb06adf Mon Sep 17 00:00:00 2001 From: Nathan Stocks Date: Thu, 30 Mar 2017 22:20:51 -0600 Subject: [PATCH 05/12] Using an untyped, one-letter variable binding as an argument to a function and then not using it until over 100 lines later is just mean. --- src/bootstrap/flags.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bootstrap/flags.rs b/src/bootstrap/flags.rs index 1a260050a9422..64b66d9c6bc34 100644 --- a/src/bootstrap/flags.rs +++ b/src/bootstrap/flags.rs @@ -89,7 +89,7 @@ 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 usage = |exit_code, opts: &Options| -> ! { let subcommand_help = format!("\ Usage: x.py [options] [...] @@ -191,7 +191,7 @@ Arguments: } } - process::exit(n); + process::exit(exit_code); }; if args.len() == 0 { println!("a subcommand must be passed"); From 5ba579e7f43e607315737899a779d8bd0f378f53 Mon Sep 17 00:00:00 2001 From: Nathan Stocks Date: Thu, 30 Mar 2017 22:35:55 -0600 Subject: [PATCH 06/12] Save my TODO's as comments, so I don't forget. --- src/bootstrap/flags.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/bootstrap/flags.rs b/src/bootstrap/flags.rs index 64b66d9c6bc34..684a39953e2e6 100644 --- a/src/bootstrap/flags.rs +++ b/src/bootstrap/flags.rs @@ -212,6 +212,8 @@ Arguments: let remaining_as_path = |m: &Matches| { m.free.iter().map(|p| cwd.join(p)).collect::>() }; + // TODO: Parse subcommand nicely up at top, so options can occur before the subcommand. + // TODO: Get the subcommand-specific options below into the help output let m: Matches; let cmd = match &args[0][..] { From aa4bd0ec0ea0e4edd2a4507c776f9979a05005d1 Mon Sep 17 00:00:00 2001 From: Nathan Stocks Date: Sat, 1 Apr 2017 15:48:03 -0600 Subject: [PATCH 07/12] Finish the improvements I planned. - No more manual args manipulation -- getopts used for everything. As a result, options can be in any position, now, even before the subcommand. - The additional options for test, bench, and dist now appear in the help output. - No more single-letter variable bindings used internally for large scopes. - Don't output the time measurement when just invoking 'x.py' - Logic is now much more linear. We build strings up, and then print them. --- src/bootstrap/bootstrap.py | 5 +- src/bootstrap/flags.rs | 222 ++++++++++++++++++------------------- src/bootstrap/step.rs | 11 +- 3 files changed, 113 insertions(+), 125 deletions(-) diff --git a/src/bootstrap/bootstrap.py b/src/bootstrap/bootstrap.py index 73f3b1d1cebab..0e5991a51bc80 100644 --- a/src/bootstrap/bootstrap.py +++ b/src/bootstrap/bootstrap.py @@ -591,9 +591,10 @@ 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() - if ('-h' not in sys.argv) and ('--help' not in sys.argv): + 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): @@ -601,7 +602,7 @@ def main(): else: exit_code = 1 print(e) - if ('-h' not in sys.argv) and ('--help' not in sys.argv): + if not help_triggered: print("Build completed unsuccessfully in %s" % format_build_time(time() - start_time)) sys.exit(exit_code) diff --git a/src/bootstrap/flags.rs b/src/bootstrap/flags.rs index 684a39953e2e6..e60e279876c37 100644 --- a/src/bootstrap/flags.rs +++ b/src/bootstrap/flags.rs @@ -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; @@ -75,7 +75,22 @@ pub enum Subcommand { impl Flags { pub fn parse(args: &[String]) -> Flags { + let mut extra_help = String::new(); + let mut subcommand_help = format!("\ +Usage: x.py [options] [...] + +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 -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"); @@ -89,26 +104,38 @@ impl Flags { opts.optopt("j", "jobs", "number of jobs to run in parallel", "JOBS"); opts.optflag("h", "help", "print this help message"); - let usage = |exit_code, opts: &Options| -> ! { - let subcommand_help = format!("\ -Usage: x.py [options] [...] - -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 + // 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); + }; -To learn more about a subcommand, run `./x.py -h`"); + // Get subcommand + let matches = opts.parse(&args[..]).unwrap_or_else(|e| { + // Invalid argument/option format + println!("\n{}\n", e); + usage(1, &opts, &subcommand_help, &extra_help); + }); + let subcommand = match matches.free.get(0) { + Some(s) => { s }, + None => { + // No subcommand -- lets only show the general usage and subcommand help in this case. + println!("{}\n", subcommand_help); + process::exit(0); + } + }; - println!("{}", opts.usage(&subcommand_help)); + // Get any optional paths which occur after the subcommand + let cwd = t!(env::current_dir()); + let paths = matches.free[1..].iter().map(|p| cwd.join(p)).collect::>(); - let subcommand = args.get(0).map(|s| &**s); - match subcommand { - 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 paths to directories to the crates and/or artifacts to compile. For example: @@ -125,12 +152,11 @@ 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 paths to directories to tests that should be compiled and run. For example: @@ -143,12 +169,13 @@ 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 paths to directories of documentation to build. For example: @@ -160,111 +187,74 @@ Arguments: 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(subcommand) = subcommand { - if subcommand == "build" || - subcommand == "test" || - subcommand == "bench" || - subcommand == "doc" || - subcommand == "clean" || - subcommand == "dist" { - 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(subcommand); - } else { - println!("Run `./x.py {} -h -v` to see a list of available paths.", - subcommand); - } - - println!(""); - } + "dist" => { + opts.optflag("", "install", "run installer as well"); } - - process::exit(exit_code); + _ => { } }; - if args.len() == 0 { - println!("a subcommand 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); + + // 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()); } - return m - }; + } else { + extra_help.push_str(format!("Run `./x.py {} -h -v` to see a list of available paths.", + subcommand).as_str()); + } - let cwd = t!(env::current_dir()); - let remaining_as_path = |m: &Matches| { - m.free.iter().map(|p| cwd.join(p)).collect::>() - }; - // TODO: Parse subcommand nicely up at top, so options can occur before the subcommand. - // TODO: Get the subcommand-specific options below into the help output + // 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) } + 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" => { - m = parse(&opts); - Subcommand::Doc { paths: remaining_as_path(&m) } + 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), _ => { - 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 { @@ -272,31 +262,29 @@ Arguments: } }); - 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"), } } } diff --git a/src/bootstrap/step.rs b/src/bootstrap/step.rs index 6eb12fed5abb2..5560b5b0333c8 100644 --- a/src/bootstrap/step.rs +++ b/src/bootstrap/step.rs @@ -978,26 +978,25 @@ invalid rule dependency graph detected, was a rule added and maybe typo'd? } } - pub fn print_help(&self, command: &str) { + pub fn get_help(&self, command: &str) -> Option { let kind = match command { "build" => Kind::Build, "doc" => Kind::Doc, "test" => Kind::Test, "bench" => Kind::Bench, "dist" => Kind::Dist, - _ => return, + _ => return None, }; let rules = self.rules.values().filter(|r| r.kind == kind); let rules = rules.filter(|r| !r.path.contains("nowhere")); let mut rules = rules.collect::>(); rules.sort_by_key(|r| r.path); - println!("Available paths:\n"); + let mut help_string = String::from("Available paths:\n"); for rule in rules { - print!(" ./x.py {} {}", command, rule.path); - - println!(""); + help_string.push_str(format!(" ./x.py {} {}\n", command, rule.path).as_str()); } + Some(help_string) } /// Construct the top-level build steps that we're going to be executing, From 2c9ae48149cb714e7cfd49c038af9b54e261da44 Mon Sep 17 00:00:00 2001 From: Nathan Stocks Date: Sat, 1 Apr 2017 23:16:46 -0600 Subject: [PATCH 08/12] Oops, we can't parse options until all options have been defined. Tiny bit of manual arg-parsing. Fixed tidy stuff too. --- src/bootstrap/flags.rs | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/src/bootstrap/flags.rs b/src/bootstrap/flags.rs index e60e279876c37..9b45246bc5002 100644 --- a/src/bootstrap/flags.rs +++ b/src/bootstrap/flags.rs @@ -113,31 +113,28 @@ To learn more about a subcommand, run `./x.py -h`"); process::exit(exit_code); }; - // Get subcommand - let matches = opts.parse(&args[..]).unwrap_or_else(|e| { - // Invalid argument/option format - println!("\n{}\n", e); - usage(1, &opts, &subcommand_help, &extra_help); - }); - let subcommand = match matches.free.get(0) { - Some(s) => { s }, - None => { - // No subcommand -- lets only show the general usage and subcommand help in this case. + // 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::>(); + possible_subcommands.retain(|&s| !s.starts_with('-')); + let subcommand = match possible_subcommands.first() { + Some(s) => s, + None => { + // No subcommand -- show the general usage and subcommand help println!("{}\n", subcommand_help); process::exit(0); } }; - // Get any optional paths which occur after the subcommand - let cwd = t!(env::current_dir()); - let paths = matches.free[1..].iter().map(|p| cwd.join(p)).collect::>(); - // Some subcommands have specific arguments help text match subcommand.as_str() { "build" => { subcommand_help.push_str("\n Arguments: - This subcommand accepts a number of paths to directories to the crates + This subcommand accepts a number of paths to directories to the crates and/or artifacts to compile. For example: ./x.py build src/libcore @@ -195,6 +192,17 @@ Arguments: _ => { } }; + // 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 paths = matches.free[1..].iter().map(|p| cwd.join(p)).collect::>(); + + // All subcommands can have an optional "Available paths" section if matches.opt_present("verbose") { let flags = Flags::parse(&["build".to_string()]); From 6b7258670f606e6951e3ad34c02a598d595dfa6e Mon Sep 17 00:00:00 2001 From: Nathan Stocks Date: Sun, 2 Apr 2017 10:28:36 -0600 Subject: [PATCH 09/12] Simplify a "use" statement as per @grunweg's feedback. --- src/bootstrap/flags.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bootstrap/flags.rs b/src/bootstrap/flags.rs index 9b45246bc5002..c374d27fe4f3e 100644 --- a/src/bootstrap/flags.rs +++ b/src/bootstrap/flags.rs @@ -18,7 +18,7 @@ use std::fs; use std::path::PathBuf; use std::process; -use getopts::{Options}; +use getopts::Options; use Build; use config::Config; From 1e5389853ca3a99a338f3a40cbb96150f711410c Mon Sep 17 00:00:00 2001 From: Nathan Stocks Date: Sun, 2 Apr 2017 13:11:53 -0600 Subject: [PATCH 10/12] Fix breaking the 'clean' subcommand caused replacing a single-letter variable with the same value in two contexts where it was used differently. That's why you don't use "m" as a variable for hundreds of lines in an outer function, and re-use it in closures several times in the same function. Sheesh. --- src/bootstrap/flags.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bootstrap/flags.rs b/src/bootstrap/flags.rs index c374d27fe4f3e..2e133664a05b3 100644 --- a/src/bootstrap/flags.rs +++ b/src/bootstrap/flags.rs @@ -244,7 +244,7 @@ Arguments: Subcommand::Doc { paths: paths } } "clean" => { - if matches.free.len() > 0 { + if paths.len() > 0 { println!("\nclean takes no arguments\n"); usage(1, &opts, &subcommand_help, &extra_help); } From 20cb7005b0c1e12383a4c2f9031c5c5f5c907efc Mon Sep 17 00:00:00 2001 From: Nathan Stocks Date: Mon, 3 Apr 2017 19:15:31 -0600 Subject: [PATCH 11/12] Handle options-with-arguments before subcommands such as './x.py -j 10 build' and detect pathological cases like './x.py --option-that-takes-argument clean build' --- src/bootstrap/flags.rs | 60 +++++++++++++++++++++++++++++++----------- 1 file changed, 44 insertions(+), 16 deletions(-) diff --git a/src/bootstrap/flags.rs b/src/bootstrap/flags.rs index 2e133664a05b3..3af3b76163c1e 100644 --- a/src/bootstrap/flags.rs +++ b/src/bootstrap/flags.rs @@ -119,7 +119,13 @@ To learn more about a subcommand, run `./x.py -h`"); // 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::>(); - possible_subcommands.retain(|&s| !s.starts_with('-')); + possible_subcommands.retain(|&s| + (s == "build") + || (s == "test") + || (s == "bench") + || (s == "doc") + || (s == "clean") + || (s == "dist")); let subcommand = match possible_subcommands.first() { Some(s) => s, None => { @@ -129,7 +135,43 @@ To learn more about a subcommand, run `./x.py -h`"); } }; - // Some subcommands have specific arguments help text + // Some subcommands get extra options + match subcommand.as_str() { + "test" => opts.optmulti("", "test-args", "extra arguments", "ARGS"), + "bench" => opts.optmulti("", "test-args", "extra arguments", "ARGS"), + "dist" => opts.optflag("", "install", "run installer as well"), + _ => { } + }; + + // 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); + }); + // Extra sanity check to make sure we didn't hit this crazy corner case: + // + // ./x.py --frobulate clean build + // ^-- option ^ ^- actual subcommand + // \_ arg to option could be mistaken as subcommand + let mut pass_sanity_check = true; + match matches.free.get(0) { + Some(check_subcommand) => { + if &check_subcommand != subcommand { + pass_sanity_check = false; + } + }, + None => { + pass_sanity_check = false; + } + } + if !pass_sanity_check { + println!("{}\n", subcommand_help); + println!("Sorry, I couldn't figure out which subcommand you were trying to specify.\n\ + You may need to move some options to after the subcommand.\n"); + process::exit(1); + } + // Extra help text for some commands match subcommand.as_str() { "build" => { subcommand_help.push_str("\n @@ -152,7 +194,6 @@ Arguments: ./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 paths to directories to tests that @@ -168,9 +209,6 @@ Arguments: ./x.py test ./x.py test --stage 1"); } - "bench" => { - opts.optmulti("", "test-args", "extra arguments", "ARGS"); - } "doc" => { subcommand_help.push_str("\n Arguments: @@ -186,18 +224,8 @@ Arguments: ./x.py doc ./x.py doc --stage 1"); } - "dist" => { - opts.optflag("", "install", "run installer as well"); - } _ => { } }; - - // 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 paths = matches.free[1..].iter().map(|p| cwd.join(p)).collect::>(); From ea2bfae8694221c92857a0b3dd96f63a8a255db2 Mon Sep 17 00:00:00 2001 From: Nathan Stocks Date: Tue, 4 Apr 2017 13:50:24 -0600 Subject: [PATCH 12/12] Branch arms need to match the return value even if it's not being assigned to anything --- src/bootstrap/flags.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/bootstrap/flags.rs b/src/bootstrap/flags.rs index 3af3b76163c1e..a1466d68a135a 100644 --- a/src/bootstrap/flags.rs +++ b/src/bootstrap/flags.rs @@ -137,10 +137,10 @@ To learn more about a subcommand, run `./x.py -h`"); // Some subcommands get extra options match subcommand.as_str() { - "test" => opts.optmulti("", "test-args", "extra arguments", "ARGS"), - "bench" => opts.optmulti("", "test-args", "extra arguments", "ARGS"), - "dist" => opts.optflag("", "install", "run installer as well"), - _ => { } + "test" => { opts.optmulti("", "test-args", "extra arguments", "ARGS"); }, + "bench" => { opts.optmulti("", "test-args", "extra arguments", "ARGS"); }, + "dist" => { opts.optflag("", "install", "run installer as well"); }, + _ => { }, }; // Done specifying what options are possible, so do the getopts parsing