Add sample defaults for config.toml

[jyn514]

• Allow including defaults in src/bootstrap/defaults using profile = "...".
• Add default config files, with a README noting they're experimental and asking you to open an issue if you run into trouble. The config files have comments explaining why the defaults are set.
• Combine config files using the merge dependency.

This introduces a new dependency on merge that hasn't yet been vetted.

I want to improve the output when include = "x" isn't found:

thread 'main' panicked at 'fs::read_to_string(&file) failed with No such file or directory (os error 2) ("configuration file did not exist")', src/bootstrap/config.rs:522:28
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failed to run: /home/joshua/rustc/build/bootstrap/debug/bootstrap test tidy
Build completed unsuccessfully in 0:00:00

However that seems like it could be fixed in a follow-up.

Closes #76619

[jyn514]

r? @Mark-Simulacrum

[jyn514]

Hmm, I think I'll change include = to profile, since it can only include defaults we, the rust maintainers, have decided on.

[Mark-Simulacrum]

I am feeling like we should at least split the actual inclusion of defaults into a separate PR, and perhaps one PR per default file. It would help me at least if they were documented in those files (at least with links) as to rationale why we think these are the right defaults. That would also likely help people figure out which one to use.

I am fine with that documentation being on rustc-dev-guide, but I personally think it would be better suited to being in tree. I don't feel strongly on that point though.

I am not sure how I feel about the merge crate dependency or the effect that has here. It seems a bit worrisome that we're going to end up ignoring the merged-in parts in bootstrap.py with the current approach. Do you know if simple concatenation of the various file contents will perhaps work well enough for us?

[jyn514]

Do you know if simple concatenation of the various file contents will perhaps work well enough for us?

Well, the include shows up as part of config.toml. So for that to work we'd have to parse it twice - I guess that's not so bad, performance doesn't matter for x.py after all. I'll see about implementing that.

It would help me at least if they were documented in those files (at least with links) as to rationale why we think these are the right defaults. That would also likely help people figure out which one to use.

+1, I'll add that.

I am feeling like we should at least split the actual inclusion of defaults into a separate PR, and perhaps one PR per default file.

Hmm, I'm ok with this but I'm not sure how to decide on defaults in the first place ... maybe I could post a thread on Zulip and solicit opinions? Ideally these would be options people actually use, so me picking random things doesn't help as much as hearing what people are using.

Fortunately these defaults don't impact workflows unless you opt-in to the changes, so we can always change things after the fact.

[jyn514]

Do you know if simple concatenation of the various file contents will perhaps work well enough for us?

This didn't work :(

failed to parse TOML configuration '/home/joshua/rustc/src/bootstrap/defaults/config.toml.library': redefinition of table `build` for key `build` at line 125 column 1

[Mark-Simulacrum]

Hm. I wonder if https://docs.rs/toml/0.5.6/toml/de/struct.Deserializer.html#method.set_allow_duplicate_after_longer_table would help there, though that could get removed at some point.

The reason I'd prefer not to use something like merge is because it seems quite error prone to use, or at least, there are interactions that I wouldn't want to leave up to proc macro I think (e.g., debug and debuginfo-std overlap in a subtle way, and it's not entirely clear how to merge those). Concatenation of files wouldn't necessarily be better, here, to be clear, but it would feel at least mildly more transparent how things get set, I think.

The precise error we're seeing is because [build] is being defined twice, right? One thought I've had historically is that I'm pretty sure the various config.toml sections, other than the target-specific opts, don't really buy us anything -- and indeed I can never be sure which one (build/rust/llvm) and option should go into. Would the parser error out if we removed those sections in favor of a flat namespace?

That is, is this an error? If so then there's probably nothing we can do, but if not perhaps there's room for us to modify things into a flat namespace and go for concatenation...

debug = true
debug = false

[jyn514]

Unfortunately that's an error:

failed to parse TOML configuration 'config.toml': unknown field `debug`, expected one of `build`, `install`, `llvm`, `rust`, `target`, `dist` at line 555 column 1

there are interactions that I wouldn't want to leave up to proc macro I think (e.g., debug and debuginfo-std overlap in a subtle way, and it's not entirely clear how to merge those)

Hmm, I'm not sure how relevant that is ... this is merging the parsed structs, not Config. So if you had debug = true in the defaults and debuginfo-std = false in config.toml, it would be exactly as if you had both in config.toml.

Would the parser error out if we removed those sections in favor of a flat namespace?

I'm not opposed to a flat namespace ... maybe we could use serde(flatten)? https://serde.rs/attr-flatten.html I have to double check whether that would be backwards compatible but it seems like the right approach.

[jyn514]

Oh - one issue with doing simple concatenation is that it will give errors if there's a setting in the defaults that you override yourself:

failed to parse TOML configuration 'config.toml': duplicate field `debug` for key `rust` at line 556 column 1

Maybe that's a good thing, so you know if the defaults conflict? I'd prefer it to give precedence to config.toml though.

[jyn514]

I'm not opposed to a flat namespace ... maybe we could use serde(flatten)? https://serde.rs/attr-flatten.html I have to double check whether that would be backwards compatible but it seems like the right approach.

It is not backwards compatible :(

failed to parse TOML configuration 'config.toml': unknown field `llvm` at line 555 column 1

[jyn514]

The reason I'd prefer not to use something like merge is because it seems quite error prone to use, or at least, there are interactions that I wouldn't want to leave up to proc macro I think (e.g., debug and debuginfo-std overlap in a subtle way, and it's not entirely clear how to merge those).

Hmm, I'm not sure how relevant that is ... this is merging the parsed structs, not Config. So if you had debug = true in the defaults and debuginfo-std = false in config.toml, it would be exactly as if you had both in config.toml.

Would you be opposed to using merge if I added lots of tests for the behavior? I don't see another good way forward, other than using a different library like https://docs.rs/config/0.10.1/config/index.html or writing out several hundred lines of the code merge would generate.

[Mark-Simulacrum]

I think tests would be unhelpful. Let's use merge for now, and see where that gets us -- I worry that it will not behave like we want, but perhaps that worry is unfounded. If we run into trouble we can explore alternatives down the line.

If you can update the PR description, that would be great, and then r=me. I am not sure that the defaults here are 100% what we want, but we can explore modifying them in future PRs, perhaps before recommending this in rustc-dev-guide. It may also make sense to stick headers on the "include" to suggest filing an issue if you ended up adjusting any additional settings or changing defaults.

[jyn514]

I updated the PR description and pushed some changes fixing bugs, it turns out you were right the derive didn't behave as expected. I emailed the maintainer asking them to make the behavior in c5925ba the default.

[Mark-Simulacrum]

Please also squash the commits -- generally speaking on PRs that modify ~100 lines or less I don't think not squashing commits as you go is helpful.

[Mark-Simulacrum]

It looks like the PR description still talks about a poor error on non-existent includes, but I think the code handles that better now?

[jyn514]

It still has a panic:

thread 'main' panicked at 'fs::read_to_string(&file) failed with No such file or directory (os error 2) ("configuration file did not exist")', src/bootstrap/config.rs:553:28

It's not the worst thing in the world, but it would be nice to at least print the file it didn't find.

[Mark-Simulacrum]

Ah, okay, we handle the parsing error nicely but not the non-existence. That's okay for now, it doesn't appear to be a regression.

I am happy with a future patch to merge target configs, especially because I'm not even sure we want to: it's rather unclear that doing so will benefit us because having target-specific defaults seems potentially confusing. (That said, implicitly enabling the download-ci-llvm option on Linux might be a good idea).

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit c9c8fb8 has been approved by Mark-Simulacrum