-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Save x.py's help text for saving output time #146692
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.
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
Conversation
8fedea1
to
2e25304
Compare
This comment has been minimized.
This comment has been minimized.
2e25304
to
6c09bbb
Compare
This PR modifies If appropriate, please update |
rustbot has assigned @Mark-Simulacrum. Use |
I'm on the fence on this. I'm slightly leaning towards "I don't feel like the extra complexity and having to remember another point of potential outdatedness pulls its weight". But if another bootstrap reviewer feels okay about this, that is fine with me too. |
I also wasn't a super big fan of this, tbh. But while testing this PR, I realized that this approach actually cannot work in the way it was designated in the original issue, or at least not generally. Because it assumes that bootstrap options and flags are unified, but that is no longer the case. Since we switched to clap, some subcommands actually have their own arguments, and thus also different help. For example, here is the output of
But with this PR, when I do So for this to work generically, the Python wrapper would have to:
Even 1) is annoying, as the Python wrapper would have to reimplement CLI parsing logic, to recognize e.g. things like Sorry for not catching that earlier, but I think that this is not going to work. CC @jyn514 if you have other opinions. |
Thanks, I didn't catch And
I agree, this is inevitable if all subcommands' helps are also loaded from files. I don't think this is a good way. I think we can make only root help (x help|--help|-h) use a saved file by changing help_triggered in bootstrap.py. In that case, only the root speeds up to print the help but other subcommands still use helps from the binary. |
Per above I think we can probably close this? One thought is we could plausibly generate e.g. the man pages that clap supports and check those in, but it all seems fairly complicated for not as much value. |
I think if we only do this for the very top-level |
This comment has been minimized.
This comment has been minimized.
--skip-std-check-if-no-download-rustc | ||
Skip checking the standard library if `rust.download-rustc` isn't available. This is mostly for RA as building the stage1 compiler to check the library tree on each code change might be too much for some computers | ||
-h, --help | ||
Print help (see more with '--help') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not relevant to this PR, but I wonder if we should really be showing all of this in help... e.g., the PGO options don't feel relevant to 99% of x.py users, and there's enough options here that we end up scrolling off screen for the actually-useful collection of subcommands test/doc/check/etc.
It might mean that we want a verbose and non-verbose help for the top level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to me for both > not relevant to this PR and > want a verbose and non-verbose help for the top level
Although I don't know if Clap supports such detailed/brief help, can I make an issue ticket for it (Check Clap's features, then modify the top-level help if we can)
This comment has been minimized.
This comment has been minimized.
@rustbot review Note: I will clean up the commit history after the review |
|
||
# If the user is asking for help, let them know that the whole download-and-build | ||
# Root help (e.g., x.py --help) prints help from the saved file to save the time | ||
if len(sys.argv) == 1 or sys.argv[1] in ["-h", "--help"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second part of this seems like it's not quite right, it needs (sys.argv[1] in ["-h", "--help"] and len(sys.argv) == 2)
, right? Otherwise x.py --help check
wouldn't do the right thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently x.py --help check
prints the top level help, not check
's help. So I think we don't need len(sys.argv) == 2
for it.
Or, do you think x.py --help check
should be identical with x.py check --help
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, do you think~
I guess if we dispatch x.py --help check
to bootstrap binary. Clap returns the top level help for it unless we modify the behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, do you think x.py --help check should be identical with x.py check --help?
Yeah, that's what I'd expect. Printing the top-level output for x.py --help check
seems quite odd to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if clap has this behavior, then I'm OK sticking with that decision (would be interesting to see if that's intentional). It's probably a rare case either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I checked that:
- clap checks arguments from left to right
- If an argument is help (--help or -h), it prints help of the current level (e.g., ./x --help is for top level, and ./x check --help is for check level) and exit immediately
So x.py --help check
prints the top level help, and argument check
is just ignored.
r=me with commits squashed |
2464757
to
c923756
Compare
This comment has been minimized.
This comment has been minimized.
Currently x.py help (or x.py --help) builds bootstrap binary everytime, but it delays printing help. This change saves the current top level help text into a file. x.py help prints the file and doesn't touch bootstrap binary. x.py test bootstrap checks if the file is up to date. Note that subcommand level helps (e.g., x.py check --help) aren't saved.
c923756
to
340702c
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
@bors r+ |
…lacrum Save x.py's help text for saving output time Fix rust-lang#141903 Currently x.py help (--help) builds bootstrap binary everytime, so it takes some seconds to print help. This PR does: - Saves current help text into a file (x.py run generate-help) - Changes bootstrap.py to print the help in the saved file and to exit without touching bootstrap binary - Modifies x.py test bootstrap to check if the help file is up-to-date
Rollup of 12 pull requests Successful merges: - #138799 (core: simplify `Extend` for tuples) - #146692 (Save x.py's help text for saving output time) - #147168 (Don't unconditionally build alloc for `no-std` targets) - #147178 ([DebugInfo] Improve formatting of MSVC enum struct variants) - #147240 (Add an ACP list item to the library tracking issue template) - #147246 (Explain not existed key in BTreeMap::split_off) - #147393 (Extract most code from `define_feedable!`) - #147495 (Update wasm-component-ld to 0.5.18) - #147503 (Fix documentation of Instant::now on mac) - #147541 (Change int-to-ptr transmute lowering back to inttoptr) - #147549 (Replace `LLVMRustContextCreate` with normal LLVM-C API calls) - #147596 (Adjust the Arm targets in CI to reflect latest changes) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 8 pull requests Successful merges: - #138799 (core: simplify `Extend` for tuples) - #145897 (Rehome 30 `tests/ui/issues/` tests to other subdirectories under `tests/ui/` [#4 of Batch #2]) - #146692 (Save x.py's help text for saving output time) - #147240 (Add an ACP list item to the library tracking issue template) - #147246 (Explain not existed key in BTreeMap::split_off) - #147393 (Extract most code from `define_feedable!`) - #147503 (Fix documentation of Instant::now on mac) - #147549 (Replace `LLVMRustContextCreate` with normal LLVM-C API calls) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #146692 - Shunpoco:issue-141903, r=Mark-Simulacrum Save x.py's help text for saving output time Fix #141903 Currently x.py help (--help) builds bootstrap binary everytime, so it takes some seconds to print help. This PR does: - Saves current help text into a file (x.py run generate-help) - Changes bootstrap.py to print the help in the saved file and to exit without touching bootstrap binary - Modifies x.py test bootstrap to check if the help file is up-to-date
Fix #141903
Currently x.py help (--help) builds bootstrap binary everytime, so it takes some seconds to print help.
This PR does: