better error handling for rust.codegen-backends on deserialization
#114278
Conversation
|
r? @clubby789 (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
onur-ozkan
changed the title
rust.codegen-backends on deserialization
onur-ozkan
force-pushed
the
validate-codegen-backend-config
branch
from
2d1b9bc to
3ea7fed
Compare
src/bootstrap/config.rs
Outdated
| let available_backends = vec!["llvm", "cranelift", "gcc"]; | ||
|
|
||
| config.rust_codegen_backends = backends.iter().map(|s| { | ||
| if !available_backends.contains(&s.as_str()) { |
There was a problem hiding this comment.
Applying validation risks making life harder for people who use out-of-tree codegen backends such as rustc_codegen_spirv, and in general, for making new codegen backends (if any ever exist) get off the ground. In order to apply the asked-for help, it should reverse this logic: check for approximate names to the ones we already know and recommend they use the other option instead.
There was a problem hiding this comment.
Alternatively, we can handle these names without getting too worried. An example: when we combine rustc_codegen_ with the backend name, it fails if we use rustc_codegen_gcc, resulting in rustc_codegen_rustc_codegen_gcc. However, we can fix this problem by not adding the rustc_codegen_ prefix if it's already included in the name. So, out-of-tree backends won't be a problem in this situation. What do you think on this approach?
There was a problem hiding this comment.
I think that sounds like a good solution
There was a problem hiding this comment.
I'm always a little wary of canonicalizing stuff instead of either simply accepting or rejecting, but it seems plausibly fine in this case.
There was a problem hiding this comment.
So with the latest change, now we panic if in-tree backends are used with rustc_codegen_ prefix, and print a help message if out-of-tree backend is used with rustc_codegen_ prefix. Each message includes the suggestion for fixing the problem.
There was a problem hiding this comment.
Alternatively, we can handle these names without getting too worried. An example: when we combine rustc_codegen_ with the backend name, it fails if we use rustc_codegen_gcc, resulting in rustc_codegen_rustc_codegen_gcc. However, we can fix this problem by not adding the rustc_codegen_ prefix if it's already included in the name. So, out-of-tree backends won't be a problem in this situation. What do you think on this approach?
The reason I didn't follow this solution was because it was going to be too hacky(it didn't work out of the box, codegen backends were failing on the compilation)
onur-ozkan
force-pushed
the
validate-codegen-backend-config
branch
from
3ea7fed to
ccefc97
Compare
onur-ozkan
changed the title
rust.codegen-backends on deserializationrust.codegen-backends on deserialization
Signed-off-by: ozkanonur <work@onurozkan.dev>
onur-ozkan
force-pushed
the
validate-codegen-backend-config
branch
from
ccefc97 to
b602cf5
Compare
|
@bors r+ |
bors
added
S-waiting-on-bors
|
@bors rollup |
…-config, r=clubby789 better error handling for `rust.codegen-backends` on deserialization Fixes rust-lang#109315
…-config, r=clubby789 better error handling for `rust.codegen-backends` on deserialization Fixes rust-lang#109315
…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#114278 (better error handling for `rust.codegen-backends` on deserialization) - rust-lang#114674 (Add clubby789 to `users_on_vacation`) - rust-lang#114678 (`Expr::can_have_side_effects()` is incorrect for struct/enum/array/tuple literals) - rust-lang#114681 (doc (unstable-book): fix a typo) - rust-lang#114684 (Remove redundant calls to `resolve_vars_with_obligations`) r? `@ghost` `@rustbot` modify labels: rollup
Fixes #109315