Give dynamically generated instructions on how to replicate x.py errors #86022
Conversation
jyn514
added
T-bootstrap
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
|
|
Hmm, this doesn't work very well for unit tests: |
|
Ok, should be better now: |
|
UI tests are not super helpful though: |
This comment has been minimized.
This comment has been minimized.
src/bootstrap/flags.rs
Outdated
| @@ -27,6 +29,16 @@ impl Default for Color { | |||
| } | |||
| } | |||
|
|
|||
| impl From<Color> for ColorChoice { | |||
There was a problem hiding this comment.
Since I'm adding termcolor anyway, it probably makes sense to remove Color and just use ColorChoice directly.
|
FWIW I'd prefer to avoid coloring or otherwise formatting things in bootstrap. We can likely hide more of the failed command message (e.g., showing the full compile test invocation isn't necessary in non-verbose mode imo). |
Sure thing, done. I like it better with less output anyway :) It looks like this now:
|
This comment has been minimized.
This comment has been minimized.
|
I am unwilling to accept the atexit and/or similar primitives to facilitate this logic. I worry that the benefit it brings seems fairly low; in practice, the full build is likely to either take a while (if, for example, the compiler has been modified, it will need to be rebuilt) or not happen -- we already should sufficiently cache that we don't rerun tests that passed without cause. Plus, this sets precedent for us to try and fine tune the logic to allow even more precise diagnostic reporting; I'm not currently inclined to do that. I think an alternative to this, which may be more broadly useful, is to change the formatting of the "step start" messages we currently emit such that they contain a more close reference to the necessary command to invoke that step, should any paths be available. For example, for the errors shown in this PR, we could have a log something like this, which hopefully gives a clearer indication that to rerun you can simply copy/paste the most recent step. That info: line could even be auto generated (though perhaps not auto-emitted), similar to what you've done in this PR. |
jyn514
added
S-waiting-on-author
This is a great idea, thank you!
Hmm, I'm not sure I understand how these fit together? I'm fine with not using |
jyn514
added
S-waiting-on-review
|
I marked this as waiting-on-review since I'm not sure I'm on the right track - feel free to flip back to waiting-on-author if you want me to convert everything to use the new |
|
Just responding quickly to the last comment (haven't actually looked at code yet): changing to formatting step descriptions prior to running them in a way closely matching with their invocation is inherently high-level by virtue of steps being high level; for example, compile test invocation wouldn't/shouldn't specify a specific test in the info line, but the previous style / strategy would have been much more likely to take that tack. Doing so before the failure occurs similarly is a forcing function to print more limited information; again, this pushes the code in a less invasive direction hopefully. It also avoids dependence on primitives like atexit, which should be avoided if at all possible in my opinion. |
This comment has been minimized.
This comment has been minimized.
src/bootstrap/builder.rs
Outdated
| } | ||
| print!( | ||
| "{} {} --stage {}", | ||
| // TODO: this is wrong, e.g. `check --stage 1` runs build commands first |
There was a problem hiding this comment.
Not entirely sure how to fix this, but it seems critical to do so if we're to have some measure of success. I am hesitant to add additional annotations to the steps themselves, but perhaps that might be the way to go.
Mark-Simulacrum
added
S-waiting-on-author
…mulacrum
Make x.py less verbose on failures
- Don't print the exact command run by rustbuild unless `--verbose` is set.
This is almost always unhelpful, since it's just cargo with a lot of
arguments (and you can't replicate it anyway unless you have the environment variables, which aren't printed by default).
- Don't print "Build completed unsuccessfully" unless --verbose is set.
You can already tell the build failed by the errors above, and the
time isn't particularly helpful.
- Don't print the full path to bootstrap. This is useless to everyone,
even including when working on x.py itself. You can still opt-in to
this being shown with `--verbose`, since it will throw an exception.
Before:
```
error[E0432]: unresolved import `x`
--> library/std/src/lib.rs:343:5
|
343 | use x;
| ^ no external crate `x`
error: aborting due to previous error
For more information about this error, try `rustc --explain E0432`.
error: could not compile `std`
To learn more, run the command again with --verbose.
command did not execute successfully: "/home/joshua/rustc4/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "8" "--release" "--features" "panic-unwind backtrace" "--manifest-path" "/home/joshua/rustc4/library/test/Cargo.toml" "--message-format" "json-render-diagnostics"
expected success, got: exit status: 101
failed to run: /home/joshua/rustc4/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:00:13
```
After:
```
error[E0432]: unresolved import `x`
--> library/std/src/lib.rs:343:5
|
343 | use x;
| ^ no external crate `x`
error: aborting due to previous error
For more information about this error, try `rustc --explain E0432`.
error: could not compile `std`
To learn more, run the command again with --verbose.
```
cc rust-lang#86854, rust-lang#86022
r? `@Mark-Simulacrum`
Hmm, so my concern with this is very easily gets lost in cargo's output: I agree the atexit stuff is overkill, but I do think it's more useful to show at the end (showing it before seems fine to do in addition if you like). What do you think about making that part of the of destructor for |
Mark-Simulacrum
added
S-waiting-on-review
|
@Mark-Simulacrum I saw you switched the labels back and forth - did you intend for them to end up as |
…ring the build progress
- add stage override - switch more things to step_info
I implemented this. I'm feeling a little burned out on this PR, I'd like to land what I have so far if at all possible. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☔ The latest upstream changes (presumably #87509) made this pull request unmergeable. Please resolve the merge conflicts. |
crlf0710
added
S-waiting-on-author
camelid
added
S-waiting-on-author
|
Ping from triage: |
Example output:
Happy to take suggestions on how to improve the error message :) right
now it's getting lost in all the other output. Maybe it makes sense to
use bold and colors?
This works by storing the current step at all times in a global Mutex
and registering an
at_exithandler which looks up the current step.Fixes #84742.