rustup should use the configured profile as fallback when the key is not present in rust-toolchain.toml

[polarathene]

Verification

• I searched for recent similar issues at https://github.com/rust-lang/rustup/issues?q=is%3Aissue+is%3Aopen%2Cclosed and found no duplicates.
• I am on the latest version of Rustup according to https://github.com/rust-lang/rustup/tags and am still able to reproduce my issue.

Problem

In 2020Q4 support for installing a toolchain defined in rust-toolchain.toml with a new profile key was added to rustup: #2586

While this works, when the profile key is not set, there is no fallback to the global profile setting for rustup. Thus if rust-toolchain.toml exists, despite rustup being configured for a minimal profile, the toolchain will be downloaded with the default profile bringing in over 600MB of HTML docs and additional components.

This took a while to trackdown/confirm why the rustup profile wasn't being respected 😅 While a little surprising, I tried searching for any existing issues and came up empty.

Steps

1. Install rustup and set the profile to minimal.
2. Add a rust-toolchain.toml file in an empty directory, with contents:

   [toolchain]
   channel = "1.70" # should be a toolchain version that is not installed

3. Run cargo --version, rustup will download the toolchain.
4. Check the toolchain install, and notice that there is over 600MB of docs installed despite the minimal profile:

   $ du --bytes -sh ~/.rustup/toolchains/1.77-x86_64-unknown-linux-gnu/share/doc/rust/html
   587M    /root/.rustup/toolchains/1.77-x86_64-unknown-linux-gnu/share/doc/rust/html

If you repeat these steps but add profile = "minimal" to the rust-toolchain.toml file with a different toolchain version, the HTML docs won't be present, the share/ directory itself will now be about 500KB.

Possible Solution(s)

• If profile is not defined, use the rustup configured profile setting to align with installing a toolchain via rustup explicitly.
• Document that a rust-toolchain.toml without a profile key will not fallback to the global rustup profile, which can result in excessive disk usage.

Notes

Past issues requesting the feature support prior to the 2020 PR:

• Installing a major.minor channel includes documentation even when profile is minimal #2593
• rust-toolchain should allow setting the equivalent of --profile minimal #2579
• Related historical issue (landed here first when trying to find why docs were installed): Making docs optional again #998

Discussion regarding interoperability across config layers, including better documenting precedence:

• Issue: Document (and perhaps change) interaction between rust-toolchain.toml and other override sources for components and targets #3483
• PR introducing related changes: Make the toolchain overriding config hierarchical #3492

Rustup version

$ rustup --version

rustup 1.27.0 (bbb9276d2 2024-03-08)
info: This is the version for the rustup toolchain manager, not the rustc compiler.
info: The currently active `rustc` version is `rustc 1.75.0 (82e1608df 2023-12-21)`

Installed toolchains

$ rustup show

Default host: x86_64-unknown-linux-gnu
rustup home:  /root/.rustup

installed toolchains
--------------------

1.75-x86_64-unknown-linux-gnu (default)
1.76-x86_64-unknown-linux-gnu
1.77-x86_64-unknown-linux-gnu
1.78-x86_64-unknown-linux-gnu

active toolchain
----------------

1.75-x86_64-unknown-linux-gnu (default)
rustc 1.75.0 (82e1608df 2023-12-21)

OS version

Windows 11 with WSL2, running rustup via a Docker container (Ubuntu 24.04):


$ cat /etc/os-release | grep PRETTY_NAME
PRETTY_NAME="Ubuntu 24.04 LTS"

$ uname -a
Linux 3d586f91b6a2 5.15.123.1-microsoft-standard-WSL2 #1 SMP Mon Aug 7 19:01:48 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

[polarathene]

The rustup docs imply that it should be using the configured minimal profile as fallback:

Note that if not specified, the default profile is not necessarily used, as a different default profile might have been set with rustup set profile.

[rami3l]

@polarathene Thanks for your in-depth investigation! I believe this is covered by #3483 as you've already found out, and no, that design is not intentional; I even believe my #3492 has fixed it.

Unfortunately, the inclusion of that patch hasn't been moving forward for a very specific reason: hierarchical config will introduce new attack vectors if one or serveral rust-toolchain.toml files can't be trusted. Due to the limited bandwidth of the Rustup team, all relevant work is suspended until we come up with a solution to that issue before pushing the hierarchical config proposal forward again. At this point I'm as sad as you are, sorry 🙇

[rami3l]

@polarathene OTOH I'm open to doc improvements, although I'm not sure if we're documenting a bug. Maybe calling it a limitation would be better.

If you can come up with a PR, I'll be glad to review it 🙏

[polarathene]

TL;DR: I could adjust the docs.. however fixing the regression introduced (specifically to profile support) instead may be fairly simple AFAIK? If that'd be acceptable, I don't mind contributing that instead?

---

Unfortunately, the inclusion of that patch hasn't been moving forward for a very specific reason: hierarchical config will introduce new attack vectors if one or serveral rust-toolchain.toml files can't be trusted

So to clarify:

• bug: Despite the current documentation for rust-toolchain.toml support with rustup, the setting profile configured for rustup is ignored when this file is present and a toolchain is installed?
• This rust-toolchain.toml feature appears to have broken the global behaviour (as described in the original feature request), so this is technically a regression.
• Fixing this bug requires a more complicated PR with wider impact to be approved and merged, but is blocked due to security concerns that imposes, and the limited bandwidth available from maintainers for the foreseeable future to tackle that PR?

Is it not possible to fix the regression introduced specifically with profile?

• If my rustup has a profile configured globally as minimal, it should respect that when it's not present in rust-toolchain.toml, as it previously did.
• Since the referenced PR with it's improvements that could fix this concern introduces too much friction, perhaps this specific use-case can be resolved separately?

I'd rather address that than correct the docs with the existing gotcha. If the contribution doesn't sound too troublesome and would get approval, I can give it a shot?

• Presumably when profile is None after parsing rust-toolchain.toml, it just needs to additionally check for a profile key in ~/.rustup/settings.toml as a fallback?
• This would prevent the mishap of excessive disk usage (and network bandwidth wasted for both parties involved) which sounds like a win. I don't see any security concerns with allowing to fallback to the expected profile configured for rustup?

[rami3l]

If the contribution doesn't sound too troublesome and would get approval, I can give it a shot?

@polarathene Wait, you reminded me of something. Maybe my #3492 can be carefully cherry-picked into two parts. I do remember some commits are related to rust-toolchain.toml while others aren't.

So in theory you can use (or get inspired from) half of my PR (so without the rust-toolchain.toml filesystem walk hierarchy part), make sure that the default toolchain (incl. profile, channel, etc.) is working as you'd expect, add a bunch of smoke tests, adjust the docs accordingly (mentioning the limitations) and call it a day. The rest of that PR will be merged later...

As long as @rbtcollins agrees with that plan.

Anyway, that PR is on our v1.28 milestone (meaning we have to at least react to it in some way), so I'm not going anywhere.

[lucacasonato]

Seemingly this has been fixed on main. I have added a regression test here: #4040.

[rami3l]

Seemingly this has been fixed on main. I have added a regression test here: #4040.

@lucacasonato Thanks for the heads up! This looks interesting. Maybe that's a side effect from the serde introduction in #3864 (cc @djc)? If so, I'd be glad to close this very soon.

[djc]

Seemingly this has been fixed on main. I have added a regression test here: #4040.

@lucacasonato Thanks for the heads up! This looks interesting. Maybe that's a side effect from the serde introduction in #3864 (cc @djc)? If so, I'd be glad to close this very soon.

Hmm, it's not obvious to me why/how that PR changed it. Would be interesting to bisect this...

[rami3l]

Seemingly this has been fixed on main. I have added a regression test here: #4040.

@lucacasonato Thanks for the heads up! This looks interesting. Maybe that's a side effect from the serde introduction in #3864 (cc @djc)? If so, I'd be glad to close this very soon.

Hmm, it's not obvious to me why/how that PR changed it. Would be interesting to bisect this...

@djc Actually there was a semantic change regarding Settings::profile's default value (in 04005f8, Some -> None):

rustup/src/settings.rs

Lines 88 to 100 in 5442ade

	impl Default for Settings {
	fn default() -> Self {
	Self {
	version: MetadataVersion::default(),
	default_host_triple: None,
	default_toolchain: None,
	profile: Some(Profile::default()),
	overrides: BTreeMap::new(),
	pgp_keys: None,
	auto_self_update: None,
	}
	}
	}

The relevant default() call was unfortunately written as Default::default() so my full-repo grip has missed it:

rustup/src/settings.rs

Lines 46 to 55 in e774ac0

	*self.cache.borrow_mut() = Some(if utils::is_file(&self.path) {
	let content = utils::read_file("settings", &self.path)?;
	Settings::parse(&content).with_context(|| RustupError::ParsingFile {
	name: "settings",
	path: self.path.clone(),
	})?
	} else {
	needs_save = true;
	Default::default()
	});

I'm still not sure if this has influenced anything. Will investigate further...

[djc]

Given that Cfg::get_profile() should yield the default profile anyway, it shouldn't be a substantial change? It does feel more correct to initialize with None than Some(Profile::default()).

[rami3l]

Given that Cfg::get_profile() should yield the default profile anyway, it shouldn't be a substantial change? It does feel more correct to initialize with None than Some(Profile::default()).

@djc You're right. This is a regression from #3340, and fix is actually in ae71bde. Obviously I failed to recognize one my own commits XD

TLDR: The Cfg::get_profile() function was not wrong. It's just that wasn't not used in .ensure_installed() until #3980 for unknown reasons. So I failed to bisect according to rustup show profile since it relies on the former.

[djc]

Hah, that makes sense.