Skip to content

bootstrap: emit hint if a config key is used in the wrong section #146611

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 17, 2025
Merged

bootstrap: emit hint if a config key is used in the wrong section #146611

merged 1 commit into from
Sep 17, 2025

Conversation

@lolbinarycat
Copy link
Contributor

@lolbinarycat lolbinarycat commented Sep 15, 2025 •
edited by rustbot
Loading

based on discussion on #146591

now, if the user puts build.download-rustc in bootstrap.toml, they'll get a diagnostic:
hint: try moving `download-rustc` to the `rust` section

and if they nest things too much (rust.rust.download-rustc):
hint: section name `rust` used as a key within a section

if they specify a top-level key within a section (rust.profile):
hint: try using `profile` as a top level key

r? @Kobzol

@rustbot
Copy link
Collaborator

rustbot commented Sep 15, 2025

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Sep 15, 2025
@lolbinarycat lolbinarycat force-pushed the bootstrap-toml-wrong-section-diagnostic branch from e85f09f to 2593762 Compare September 15, 2025 21:56
Kobzol
Kobzol reviewed Sep 16, 2025
View reviewed changes
Copy link
Member

@Kobzol Kobzol left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation is so hacky I don't even want to look at it 😆 But since this is just a best effort hint, and doesn't affect the rest of the codebase, I'm fine with it. Left one nit. Thank you!

View changes since this review

src/bootstrap/src/core/config/config.rs Outdated
}

fn find_correct_section_for_field(field_name: &str) -> Vec<&'static str> {
let sections = ["<top level>", "build", "install", "llvm", "gcc", "rust", "dist"];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually have a field named build, so for that this heuristic wouldn't work, but that's not so important.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code could easily be generalized to detect when a section name is also valid as a key in a given section, but the problem is I have no idea what a good diagnostic to print in that case would be.

@lolbinarycat
Copy link
Contributor Author

alright, made the code more robust with an enum, and also made keys like build get a much nicer diagnostic:

hint: `build` would be valid in section `build`, or at top level

@lolbinarycat lolbinarycat force-pushed the bootstrap-toml-wrong-section-diagnostic branch from 2593762 to fed721c Compare September 16, 2025 15:59
@Kobzol
Copy link
Member

Kobzol commented Sep 16, 2025

Some manual testing:

[rust]
full-bootstrap = true # hint: try moving `full-bootstrap` to the `build` section => awesome hint! really magical
[rust]
build.full-bootstrap = true # hint: `build` would be valid in section `build`, or at top level => a bit confusing, as it doesn't mention full-bootstrap`, which is the more important part
[build]
build.full-bootstrap = true # ERROR: Failed to parse '/projects/personal/rust/rust/bootstrap.toml': invalid type: map, expected a string for key `build.build` # I wonder why this wasn't causing this error in the original issue that motivated this PR?

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@lolbinarycat
Copy link
Contributor Author

unfortunately for the last two there's not much that can be easily done without majorly increasing complexity, as the error we get from the toml deserializer doesn't contain enough info to identify them robustly. I guess technically we could just also look for the invalid type: map error, but I'm really not sure it's worth it.

@lolbinarycat lolbinarycat force-pushed the bootstrap-toml-wrong-section-diagnostic branch from fed721c to 9c42379 Compare September 16, 2025 17:39
@Kobzol
Copy link
Member

Kobzol commented Sep 16, 2025

Yeah, agreed! Thank you.

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Sep 16, 2025

📌 Commit 9c42379 has been approved by Kobzol

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 16, 2025
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 16, 2025
Merged
bors added a commit that referenced this pull request Sep 16, 2025
@bors
Auto merge of #146650 - matthiaskrgr:rollup-rjrklz9, r=matthiaskrgr
3f1552a 
Rollup of 5 pull requests

Successful merges:

 - #146442 (Display ?Sized, const, and lifetime parameters in trait item suggestions across a crate boundary)
 - #146474 (Improve `core::ascii` coverage)
 - #146605 (Bump rustfix 0.8.1 -> 0.8.7)
 - #146611 (bootstrap: emit hint if a config key is used in the wrong section)
 - #146618 (Do not run ui test if options specific to LLVM are used when another codegen backend is used)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d1ed4f4 into rust-lang:master Sep 17, 2025
10 checks passed
@rustbot rustbot added this to the 1.92.0 milestone Sep 17, 2025
rust-timer added a commit that referenced this pull request Sep 17, 2025
@rust-timer
Unrolled build for #146611
ce87ef1 
Rollup merge of #146611 - lolbinarycat:bootstrap-toml-wrong-section-diagnostic, r=Kobzol

bootstrap: emit hint if a config key is used in the wrong section

based on discussion on #146591

now, if the user puts `build.download-rustc` in `bootstrap.toml`, they'll get a diagnostic:
``hint: try moving `download-rustc` to the `rust` section``

and if they nest things too much (`rust.rust.download-rustc`):
``hint: section name `rust` used as a key within a section``

if they specify a top-level key within a section (`rust.profile`):
``hint: try using `profile` as a top level key``

r? `@Kobzol`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kobzol Kobzol Kobzol left review comments

Assignees

@Kobzol Kobzol

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Projects

None yet

Milestone

1.92.0

Development

Successfully merging this pull request may close these issues.

5 participants

@lolbinarycat @rustbot @Kobzol @rust-log-analyzer @bors