Make the toolchain overriding config hierarchical

[rami3l]

Fixes #3483.

Quoting https://rust-lang.github.io/rustup/overrides.html:

rustup automatically determines which [toolchain] to use when one of the
installed commands like rustc is executed. There are several ways to control
and override which toolchain is used:

1. A [toolchain override shorthand] used on the command-line, such as cargo +beta.
2. The RUSTUP_TOOLCHAIN environment variable.
3. A [directory override], set with the rustup override command.
4. The [rust-toolchain.toml] file.
5. The [default toolchain].

Please note that the "hierarchical config" we are talking about here is different from what cargo does right now:
rustup won't do file system-based config hierarchy, which means 3. and 4. will be merged if and only if they are from the same directory, otherwise only the one that comes first in terms of proximity from the current working directory will be merged.

Concerns

• Figure out the interactions with Cargo and Rustup safe file discovery. rfcs#3279.
  • This PR has been blocked on its implementation.
• Clarify the way in whichrustup override should interact with rust-toolchain.toml.
  • Currently, in find_override_from_dir_walk, we find the first directory that has either one of those, and return immediately. There is no merging even at the same level.
    • Solution: We do the merging at the same level and keep the early return.
  • To be consistent with cargo we need to check what we have from both sides at each level (rustup override first), and dir_walk all the way up, merging all the rustup override + rust-toolchain.toml overrides in the process. However, will this cause significant performance penalties?
    • We won't support that kind of file system-based config hierarchy.
• Add some new test cases to the test suite.
• Provide an escape hatch, for the case where someone has configured defaults that are incompatible with the channel they are overriding.
  • One possible escape hatch is just to document that installation defaults are ignored if the toolchain is already installed.
• Adjust the current docs accordingly.

[rbtcollins]

I think this approach to the code conflates two things - and this is why I said I don't think we want hierarchical config : we can can generate arbitrarily confusing outcomes for users when we merge data on a per-key basis..

What I was proposing is that:

• the channel selection is unaltered from today
• we search separately for installation defaults other than channel, along a subset of the same search path, to find the installation default the user has specified. The subset being the sources that are able to specify installation defaults. Which e.g. environment variables cannot.

And writing this has me realise that we also need to provide an escape hatch, for the case where someone has configured defaults that are incompatible with the channel they are overriding. One possible escape hatch is just to document that installation defaults are ignored if the toolchain is already installed (I believe this is the case).

Thank you for writing tests. I think that these tests would be best as unit tests; the UI is not being changed, and we should be able to produce much more compact tests closer to the core. They are ok if you wish to stay with these external layer tests, but I have a very strong preference to see close-to-the-unit tests where possible!

(I haven't read the code in detail, just did a quick skim in a couple of spare minutes I had available; apologies if I've misunderstood something)

[rami3l]

@rbtcollins Thanks for your clarification!

What I was proposing is that:

• the channel selection is unaltered from today

It is still the case that once the toolchain field is set, it will not be overridden again. I believe I did special-case the toolchain selection in the code to ensure this behavior remains unchanged:

rustup/src/config.rs

Lines 94 to 98 in 2c87185

	if !self.has_toolchain() {
	// `channel` and `path` are mutually exclusive, so they need to be updated together,
	// and this will happen only if `self` doesn't have a toolchain yet.
	(self.channel, self.path) = (other.channel, other.path);
	}

After all, the original issue in #3483 is that rustup currently doesn't even bother to check other sources once the toolchain field becomes available. Here I just made it possible to merge other fields.

• we search separately for installation defaults other than channel, along a subset of the same search path, to find the installation default the user has specified. The subset being the sources that are able to specify installation defaults. Which e.g. environment variables cannot.

If I'm not mistaken, all 4 sources can specify a toolchain, but only rust-toolchain.toml can specify other fields (this is also stated in #3483), so I believe my patch currently does what you expect it to do already.

And writing this has me realise that we also need to provide an escape hatch, for the case where someone has configured defaults that are incompatible with the channel they are overriding. One possible escape hatch is just to document that installation defaults are ignored if the toolchain is already installed (I believe this is the case).

Would you mind clarifying on this one? In what way they might become incompatible?

Thank you for writing tests. I think that these tests would be best as unit tests; the UI is not being changed, and we should be able to produce much more compact tests closer to the core. They are ok if you wish to stay with these external layer tests, but I have a very strong preference to see close-to-the-unit tests where possible!

I agree. I'll definitely look into that when I have time!
These black box tests are rather for making sure that we are on the same page in terms of what should be implemented in this PR. Now I believe that I have correctly understood your intentions.

[rami3l]

And writing this has me realise that we also need to provide an escape hatch, for the case where someone has configured defaults that are incompatible with the channel they are overriding. One possible escape hatch is just to document that installation defaults are ignored if the toolchain is already installed (I believe this is the case).

Would you mind clarifying on this one? In what way they might become incompatible?

Did you mean, e.g. requiring nightly + rls when nightly is already installed without rls?

@rbtcollins replied:

Current situation:

1. channel is found first from a list of locations
2. components are specified by first rustup-toolchain.toml IF AND ONLY IF channel is source from rustup-toolchain; and then default-profile
3. targets are specified only by rustup-toolchain.toml IF AND ONLY IF channel is source from rustup-toolchain

.. and (from memory) installed toolchains are never modified

the proposed change here is to drop the IF AND ONLY IF condition in the bottom two cases.

So what happens if the following sequence takes place.

1. rustup installs beta
2. an override is set for beta for a particular path
3. cargo or some other command that triggers implicit toolchain installation is run, finds beta and chooses that, and finds the other components and targets using the changed lookup successfully... but as beta is installed, takes no action.

The user will be filing a nearly identical bug report again.

Checking all components and targets every call is a lot of overhead, as a bunch of file IO is required to do that. I don't think thats a good path forward in the short term.

Now, in the scenario above (which we could test btw) - I strongly suspect that rustup override set ... is one of the commands that does implicit installation today.

cc #1397 (comment) #2686 (comment)

[rami3l]

It seems that both the missing components and the missing targets will be installed once an auto-install-invoking command is executed:

rustup/tests/suite/cli_rustup.rs

Lines 2050 to 2061 in f32f91e

	raw::write_file(
	&toolchain_file,
	&format!(
	r#"
	[toolchain]
	channel = "beta"
	components = [ "rls" ]
	targets = [ "{MULTI_ARCH1}" ]
	"#,
	),
	)
	.unwrap();

rustup/tests/suite/cli_rustup.rs

Lines 2063 to 2068 in f32f91e

	// Implicitly install the missing components and targets.
	config.expect_stdout_ok(&["rustc", "--version"], beta);
	let list_installed = &["rustup", "component", "list", "--installed"];
	config.expect_stdout_ok(list_installed, "rls-");
	config.expect_stdout_ok(list_installed, &format!("rust-std-{MULTI_ARCH1}"));

So I guess the concern about incompatible config has been addressed.

[rami3l]

@rbtcollins is concerned about this change's possibility of making environment isolation in systems like bazel more difficult (cc bazelbuild/rules_rust#2285).

Will #2793 become an option in terms of mitigating this issue? (Thanks for the hint, @djc!)

[rbtcollins]

I've converted this to draft for now - its not in 'ready to review' state.