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

polarathene commented 4 months ago

Verification

[X] I searched for recent similar issues at https://github.com/rust-lang/rustup/issues?q=is%3Aissue+is%3Aopen%2Cclosed and found no duplicates.
[X] 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: https://github.com/rust-lang/rustup/pull/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

Install rustup and set the profile to minimal.

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

Run cargo --version, rustup will download the toolchain.

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:

https://github.com/rust-lang/rustup/issues/2593
https://github.com/rust-lang/rustup/issues/2579
Related historical issue (landed here first when trying to find why docs were installed): https://github.com/rust-lang/rustup/issues/998

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

Issue: https://github.com/rust-lang/rustup/issues/3483
PR introducing related changes: https://github.com/rust-lang/rustup/pull/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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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.

rust-lang / rustup