Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 tasks done
polarathene opened this issue May 4, 2024 · 12 comments · Fixed by #4040
Closed
2 tasks done
Labels
Milestone

Comments

@polarathene
Copy link

Verification

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:

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

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
Copy link
Author

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
Copy link
Member

rami3l commented May 4, 2024

@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
Copy link
Member

rami3l commented May 4, 2024

@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
Copy link
Author

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
Copy link
Member

rami3l commented May 4, 2024

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.

@rami3l rami3l added this to the 1.28.0 milestone May 4, 2024
@rami3l rami3l modified the milestones: 1.28.0, 1.29.0 Aug 9, 2024
@lucacasonato
Copy link
Contributor

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

@rami3l
Copy link
Member

rami3l commented Oct 3, 2024

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.

@rami3l rami3l modified the milestones: 1.29.0, 1.28.0 Oct 3, 2024
@djc
Copy link
Contributor

djc commented Oct 3, 2024

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 djc closed this as completed in #4040 Oct 3, 2024
@rami3l
Copy link
Member

rami3l commented Oct 4, 2024

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
Copy link
Contributor

djc commented Oct 4, 2024

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
Copy link
Member

rami3l commented Oct 4, 2024

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
Copy link
Contributor

djc commented Oct 4, 2024

Hah, that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants