Allow .toml extension for rust-toolchain files

[phil-opp]

This pull request adds support for an optional .toml extension for rust-toolchain files, i.e. the file can now be also named rust-toolchain.toml. Only the TOML syntax is supported in rust-toolchain.toml files. The advantage of the .toml extension is that both users and IDEs directly recognize that the file is written in TOML.

If both rust-toolchain and rust-toolchain.toml files are present in a directory, rustup outputs a warning and chooses the rust-toolchain file. This way, this change is fully backwards compatible.

Resolves #2652

cc @kinnison

[phil-opp]

I'm opening this PR as a draft since there is still a small TODO:

rustup/src/config.rs

Lines 589 to 594 in e729cf5

	// TODO: is there a better way to print a warning?
	eprintln!(
	"warning: both `{0}` and `{1}` exist. Using `{0}`",
	path_rust_toolchain.display(),
	path_rust_toolchain_toml.display(),
	);

I'm not sure if that's the best way to print the warning. I saw that there is a warn! macro in the cli module, but it isn't available in config.rs.

[kinnison]

Firstly I'd like to apologise for how long it has taken me to look at this. I've been somewhat busy at work but my silence wasn't very polite and so I'm sorry.

In general this patch is excellent. You hilighted the issue with the warning yourself -- you'll need to add a notification kind, and thread that through so that we report it as a warning. There's plenty of examples in the code but if you want some help let me know.

I'm starting to become averse to boolean values being passed to functions like this though, and if you're up to it, I'd love to see that boolean replaced with a logical enumeration which makes it clear what the values mean.

[phil-opp]

Thanks a lot for the review, I fully agree with all your suggestions! No worries about the slight delay.

I just pushed a new version that should implement your suggestions. Let me know if I did everything right with the new notification kind.

[kinnison]

I think the only thing left to do is to confirm that the test is proving that the toolchain file was in use. If you can't come up with a way to do that, then ping me and I'll see what I can come up with.

[kinnison]

Also, if you're up for it, we should document this cleanly in the relevant section of the book.

[phil-opp]

@kinnison Could you clarify what you mean with "proving that the toolchain file was in use"? I already pushed a commit that checks that rustc --version errors before the toolchain file is created. So we can be quite sure that the rust-toolchain.toml file is considered by rustup because it's the only change between the two rustc --version calls.

I'll look into the book changes.

[kinnison]

Aha yes, that makes sense, sorry I missed that assertion because I was so focussed on thinking that there was a 'rustup default nightly' as there often is in these tests. No need to change the test again. Once you've made the doc tweaks (which I expect to be pretty easy for you) you can mark this ready for final review (i.e. stop it being draft) and then we can look to merge it. Excellent work.

[phil-opp]

Ok great, thanks!

[phil-opp]

I updated the Overrides chapter in 21f7228. Let me know if you have any suggestions for improvements.

[kinnison]

Almost there. Could you fix the one doc hiccough, and then rebase your commits so that we don't have 'do foo, fix foo' type commits, but rather a small number of clean commits. i.e. what you'd have done if you'd know what this would be right from the start?

If you can't rebase then don't worry, but I'd much prefer a clean commit series if possible.

We're nearly there 👍🏻

[phil-opp]

Rebased.

[kinnison]

👍 I'll merge assuming a green CI (which we can expect)