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

CleanCut · 2017-04-02T05:19:33Z

While working on #40417, I got frustrated with the behavior of x.py and the bootstrap binary it wraps, so I decided to do something about it. This PR should improve documentation, make the command-line-parsing more flexible, and clean up some of the internals. No command that worked before should stop working. At least that's the theory. :-)

This should resolve at least #40920 and #38373.

Changes:

No more manual args manipulation -- getopts used everywhere except the one place it's not possible. 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 or explicitly passing -h or --help
Logic is now much more linear. We build strings up, and then print them.
Refer to subcommands as subcommands everywhere (some places we were saying "command")
Other minor stuff.

@alexcrichton This is my first PR. Do I need to do something specific to request reviewers or anything?

For some reason 'command' and 'subcommand' were intermixed to mean the same thing. Lets just call it the one thing that it is.

…m 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.

…age.

rust-highfive · 2017-04-02T05:19:39Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

grunweg

drive-by nit: typo "documentatino"

grunweg

Some minor drive-by comments by a non-expert; nice work!

grunweg · 2017-04-02T09:39:28Z

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};


Now that this line is just a single import, |use getopts::Options;| should also work.

grunweg · 2017-04-02T09:41:52Z

src/bootstrap/flags.rs

@@ -75,7 +75,22 @@ pub enum Subcommand {

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


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.

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?

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.

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.

CleanCut · 2017-04-02T16:28:01Z

@grunweg I can't find the "documentatino" typo that you indicated. Could you point me where to find it so that I can fix it?

grunweg · 2017-04-02T17:08:28Z

@CleanCut Sure, it's in the commit message of a7bf371 (sorry for being imprecise here).

CleanCut · 2017-04-02T18:17:30Z

@grunweg I can't find a way to amend a commit message other than the latest commit in the branch. I think in this instance I will simply have to leave the typo for future generations to enjoy. :-/

grunweg · 2017-04-02T18:50:00Z

Using |git rebase --interactive| you can rewrite the entire history, including the commit messages (see e.g. https://robots.thoughtbot.com/git-interactive-rebase-squash-amend-rewriting-history or various other sources on the internet). In any case, not a big deal.

- 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.

…ction and then not using it until over 100 lines later is just mean.

- 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.

…y bit of manual arg-parsing. Fixed tidy stuff too.

CleanCut · 2017-04-02T19:01:33Z

Fancy! Git never ceases to amaze me. I used that command to reword the one commit, which naturally resulted in the 5 commits following it to be re-hashed. So I had to force-push the branch back up after that.

Typo fixed!

Now I feel like an achievement ought to flash across my screen: "Achievement: Apprentice Manipulator of Time and Space"

…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.

CleanCut · 2017-04-03T19:19:03Z

So, @aturon you're the assignee. Does that mean you'll be doing the "official" review?

alexcrichton · 2017-04-03T22:35:14Z

src/bootstrap/flags.rs

+        // there on out.
+        let mut possible_subcommands = args.iter().collect::<Vec<_>>();
+        possible_subcommands.retain(|&s| !s.starts_with('-'));
+        let subcommand = match possible_subcommands.first() {


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?

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.

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!

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

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

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

…0 build' and detect pathological cases like './x.py --option-that-takes-argument clean build'

…igned to anything

aturon · 2017-04-04T21:18:33Z

@CleanCut Hey, sorry for the late response, I'm swamped in all-week meetings.

I don't know the rustbuild code at all, so I'm going to bring in @alexcrichton for the review. Hopefully we can expand the set of reviewers for this code over time!

r? @alexcrichton

CleanCut · 2017-04-04T23:01:26Z

@aturon I'm so sorry to hear that! The worst weeks of my life were the ones spent in meetings. ;-)

@alexcrichton Looks like the builds are passing. What thinkest thou? Does it pass muster?

alexcrichton · 2017-04-04T23:14:50Z

@bors: r+

Looks great to me, thanks @CleanCut!

bors · 2017-04-04T23:14:51Z

📌 Commit ea2bfae has been approved by alexcrichton

CleanCut · 2017-04-05T01:00:53Z

Sweet! That's two of my PR's accepted, now. Feels nice. Wish I could make a career out of it.

Feels much more meaningful than dealing with arbitrary business logic all day.

@alexcrichton

…hton Overhaul Bootstrap (x.py) Command-Line-Parsing & Help Output While working on rust-lang#40417, I got frustrated with the behavior of x.py and the bootstrap binary it wraps, so I decided to do something about it. This PR should improve documentation, make the command-line-parsing more flexible, and clean up some of the internals. No command that worked before should stop working. At least that's the theory. :-) This should resolve at least rust-lang#40920 and rust-lang#38373. Changes: - No more manual args manipulation -- getopts used everywhere except the one place it's not possible. 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` or explicitly passing `-h` or `--help` - Logic is now much more linear. We build strings up, and then print them. - Refer to subcommands as subcommands everywhere (some places we were saying "command") - Other minor stuff. @alexcrichton This is my first PR. Do I need to do something specific to request reviewers or anything?

@alexcrichton

…hton Overhaul Bootstrap (x.py) Command-Line-Parsing & Help Output While working on rust-lang#40417, I got frustrated with the behavior of x.py and the bootstrap binary it wraps, so I decided to do something about it. This PR should improve documentation, make the command-line-parsing more flexible, and clean up some of the internals. No command that worked before should stop working. At least that's the theory. :-) This should resolve at least rust-lang#40920 and rust-lang#38373. Changes: - No more manual args manipulation -- getopts used everywhere except the one place it's not possible. 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` or explicitly passing `-h` or `--help` - Logic is now much more linear. We build strings up, and then print them. - Refer to subcommands as subcommands everywhere (some places we were saying "command") - Other minor stuff. @alexcrichton This is my first PR. Do I need to do something specific to request reviewers or anything?

Rollup of 5 pull requests - Successful merges: #40908, #41011, #41026, #41037, #41050 - Failed merges:

CleanCut added 3 commits March 30, 2017 20:49

Refer to a subcommand as a subcommand.

e1b0027

For some reason 'command' and 'subcommand' were intermixed to mean the same thing. Lets just call it the one thing that it is.

When dealing with the list of all possible subcommands, deal with the…

8ad5c95

…m 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.

Don't print build statistics if we explicitly asked for the help mess…

e1c1e09

…age.

rust-highfive assigned aturon Apr 2, 2017

frewsxcv requested review from alexcrichton and removed request for alexcrichton April 2, 2017 05:35

grunweg reviewed Apr 2, 2017

View reviewed changes

CleanCut added 6 commits April 2, 2017 12:56

Using an untyped, one-letter variable binding as an argument to a fun…

992a59e

…ction and then not using it until over 100 lines later is just mean.

Save my TODO's as comments, so I don't forget.

5ba579e

Oops, we can't parse options until all options have been defined. Tin…

2c9ae48

…y bit of manual arg-parsing. Fixed tidy stuff too.

Simplify a "use" statement as per @grunweg's feedback.

6b72586

alexcrichton reviewed Apr 3, 2017

View reviewed changes

CleanCut added 2 commits April 3, 2017 19:15

Handle options-with-arguments before subcommands such as './x.py -j 1…

20cb700

…0 build' and detect pathological cases like './x.py --option-that-takes-argument clean build'

Branch arms need to match the return value even if it's not being ass…

ea2bfae

…igned to anything

rust-highfive assigned alexcrichton and unassigned aturon Apr 4, 2017

frewsxcv mentioned this pull request Apr 6, 2017

Rollup of 8 pull requests #41100

Closed

frewsxcv mentioned this pull request Apr 6, 2017

Rollup of 7 pull requests #41101

Closed

frewsxcv mentioned this pull request Apr 6, 2017

Rollup of 5 pull requests #41102

Merged

bors added a commit that referenced this pull request Apr 6, 2017

Auto merge of #41102 - frewsxcv:rollup, r=frewsxcv

e5e9275

Rollup of 5 pull requests - Successful merges: #40908, #41011, #41026, #41037, #41050 - Failed merges:

bors merged commit ea2bfae into rust-lang:master Apr 6, 2017

CleanCut deleted the bootstrap-help branch February 22, 2019 00:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

CleanCut commented Apr 2, 2017 •

edited

Loading

rust-highfive commented Apr 2, 2017

grunweg left a comment

grunweg left a comment

grunweg Apr 2, 2017

CleanCut Apr 2, 2017

grunweg Apr 2, 2017

CleanCut Apr 2, 2017

grunweg Apr 2, 2017

CleanCut Apr 2, 2017

CleanCut commented Apr 2, 2017

grunweg commented Apr 2, 2017

CleanCut commented Apr 2, 2017

grunweg commented Apr 2, 2017

CleanCut commented Apr 2, 2017

CleanCut commented Apr 3, 2017

alexcrichton Apr 3, 2017

CleanCut Apr 3, 2017

alexcrichton Apr 3, 2017

CleanCut Apr 4, 2017

alexcrichton Apr 4, 2017

CleanCut Apr 4, 2017

aturon commented Apr 4, 2017

CleanCut commented Apr 4, 2017

alexcrichton commented Apr 4, 2017

bors commented Apr 4, 2017

CleanCut commented Apr 5, 2017

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

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

Conversation

CleanCut commented Apr 2, 2017 • edited Loading

rust-highfive commented Apr 2, 2017

grunweg left a comment

Choose a reason for hiding this comment

grunweg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CleanCut commented Apr 2, 2017

grunweg commented Apr 2, 2017

CleanCut commented Apr 2, 2017

grunweg commented Apr 2, 2017

CleanCut commented Apr 2, 2017

CleanCut commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aturon commented Apr 4, 2017

CleanCut commented Apr 4, 2017

alexcrichton commented Apr 4, 2017

bors commented Apr 4, 2017

CleanCut commented Apr 5, 2017

CleanCut commented Apr 2, 2017 •

edited

Loading