bootstrap: Don't use panics to report user errors

[jyn514]

Currently, bootstrap uses assert! interchangeably between "we expect this command to always succeed" and "you, the user, did something wrong and need to fix it". See

rust/src/bootstrap/test.rs

Line 553 in d28ef9d

assert!(out.status.success(), "`cargo miri setup` returned with non-0 exit code");

for an example of the former and

rust/src/bootstrap/test.rs

Line 1611 in d28ef9d

assert!(llvm_bin_path.is_dir());

for an example of the latter. We should have more helpful error reporting for the latter, to make it more clear that it's not a bug.

The background motivation is that people are used to seeing the latter panics, and so don't report bugs when they see the former. This makes it hard to fix bugs because we don't know things are broken.

[GentBinaku]

Can I take this ?

[jyn514]

@GentBinaku go for it!

[est31]

I think it's easy to find things that are clear user errors and to print that to the user. But in general, even a failing cargo miri setup invocation could have originated in an user error, so I'd advise against something like the ICE error message that links to the issue tracker.

[jyn514]

@est31 in that case, I would expect cargo-miri to have a reasonable message that explains what's going on. but honestly at that point even if miri anticipated the error the whole thing becomes quite murky: is it a "user error" if bootstrap and miri both work fine individually but not in combination? Is it a "user error" if someone runs x fix expecting it to work and do something useful (it's been broken for years)?

FWIW rustc prints the ICE message when you rebase over master and try to reuse the incremental cache and that was very helpful for finding #76720.

[est31]

is it a "user error" if bootstrap and miri both work fine individually but not in combination? Is it a "user error" if someone runs x fix expecting it to work and do something useful (it's been broken for years)?

Yeah it is a really hard question to answer. The problem is rooted in bootstrap's nature of being a system integration tool, with some of the components of the system even provided by the user (or optionally provided, say the stage 0 compiler).

The components called by bootstrap have different concepts of "user" as bootstrap (for them, bootstrap is another user), they receive different input data (it's been preprocessed by bootstrap), etc. In rustc on the other hand, everyone is on the same page about who the user is, what user input is, etc.

and that was very helpful for finding

Interesting, I didn't know about that. rustc's ICE messages might not be 100% accurate but they have a very low false positive rate which is very helpful.

[jyn514]

Interesting, I didn't know about that. rustc's ICE messages might not be 100% accurate but they have a very low false positive rate which is very helpful.

Yup, and this was one case where it was a false positive: the bug was in bootstrap because it was forcing two different compilers to reuse the same incremental cache.

In general I would rather not tell people to ignore errors; the worst that happens is we get multiple reports of the same bug, whereas if we tell people to ignore errors we don't know about real bugs people are running into.

[jyn514]

Copying my comment from #107417:

As a general rule:

• If the panic can only happen because bootstrap's code itself was changed, it's an internal error (e.g. the Step checking in builder.rs)
• If the panic can only happen because of a config.toml option or flags passed to bootstrap, before any commands are run, it's a user error
• If the panic happens because of an external command it's ambiguous, but I would prefer to default to panicking so people report the bug.
• If the panic happens because of an I/O error it's likely an internal error.
• If the panic should never happen (e.g. "path traversal attack") it's an internal error