-
Notifications
You must be signed in to change notification settings - Fork 974
feat(config): warn user if auto-install is enabled #4454
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
base: master
Are you sure you want to change the base?
Changes from all commits
7c4e663
50e98fb
61650b9
7ab47c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ use anyhow::{Context, Result, anyhow, bail}; | |
use serde::Deserialize; | ||
use thiserror::Error as ThisError; | ||
use tokio_stream::StreamExt; | ||
use tracing::trace; | ||
use tracing::{info, trace, warn}; | ||
|
||
use crate::dist::AutoInstallMode; | ||
use crate::{ | ||
|
@@ -387,12 +387,26 @@ impl<'a> Cfg<'a> { | |
} | ||
|
||
pub(crate) fn should_auto_install(&self) -> Result<bool> { | ||
if let Ok(mode) = self.process.var("RUSTUP_AUTO_INSTALL") { | ||
Ok(mode != "0") | ||
} else { | ||
self.settings_file | ||
.with(|s| Ok(s.auto_install != Some(AutoInstallMode::Disable))) | ||
let res = match self.process.var("RUSTUP_AUTO_INSTALL") { | ||
Ok(mode) => mode != "0", | ||
Err(_) => self | ||
.settings_file | ||
.with(|s| Ok(s.auto_install != Some(AutoInstallMode::Disable)))?, | ||
}; | ||
if res | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggest making this more linear to avoid the messy layout: if !res {
return Ok(res);
}
let recursing = self.process.var("RUST_RECURSION_COUNT");
if matches!(recursing.as_deref(), Ok("1")) {
return Ok(res);
}
warn!(..);
Ok(res) |
||
// We also need to suppress this warning if we're deep inside a recursive call. | ||
&& matches!( | ||
self.process.var("RUST_RECURSION_COUNT").as_deref(), | ||
Err(_) | Ok("0"), | ||
) | ||
{ | ||
warn!("auto-install is enabled, active toolchain will be installed if absent"); | ||
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is too strong until we have a clearer plan about how/when we are actually going to disable it by default. So I would only want to show a message when the active toolchain is actually absent, and make it more informational. I do like showing the suggestion on how to disable the behavior! |
||
warn!("this behavior is deprecated and will be removed in a future version"); | ||
info!( | ||
"you may opt out now with `RUSTUP_AUTO_INSTALL=0` or `rustup set auto-install disable`" | ||
); | ||
} | ||
Ok(res) | ||
} | ||
|
||
// Returns a profile, if one exists in the settings file. | ||
|
@@ -746,10 +760,7 @@ impl<'a> Cfg<'a> { | |
|
||
async fn local_toolchain(&self, name: Option<LocalToolchainName>) -> Result<Toolchain<'_>> { | ||
match name { | ||
Some(tc) => { | ||
let install_if_missing = self.should_auto_install()?; | ||
Toolchain::from_local(tc, install_if_missing, self).await | ||
} | ||
Some(tc) => Toolchain::from_local(tc, || self.should_auto_install(), self).await, | ||
None => { | ||
let tc = self | ||
.maybe_ensure_active_toolchain(None) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -277,6 +277,12 @@ impl Config { | |
"/bogus-config-file.toml", | ||
); | ||
|
||
// Clear current recursion count to avoid messing up related logic | ||
cmd.env("RUST_RECURSION_COUNT", ""); | ||
|
||
// Disable auto installation of active toolchain unless explicitly requested | ||
cmd.env("RUSTUP_AUTO_INSTALL", "0"); | ||
Comment on lines
+283
to
+284
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about this. IMO we should keep the defaults in the tests the same way they are when running real-world |
||
|
||
// Pass `RUSTUP_CI` over to the test process in case it is required downstream | ||
if let Some(ci) = env::var_os("RUSTUP_CI") { | ||
cmd.env("RUSTUP_CI", ci); | ||
|
@@ -291,7 +297,7 @@ impl Config { | |
/// specified by `args` under the default environment. | ||
#[must_use] | ||
pub async fn expect<S: AsRef<OsStr> + Clone + Debug>(&self, args: impl AsRef<[S]>) -> Assert { | ||
self.expect_with_env(args, &[]).await | ||
self.expect_with_env(args, []).await | ||
} | ||
|
||
/// Returns an [`Assert`] object to check the output of running the command | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,15 +53,15 @@ pub(crate) struct Toolchain<'a> { | |
impl<'a> Toolchain<'a> { | ||
pub(crate) async fn from_local( | ||
name: LocalToolchainName, | ||
install_if_missing: bool, | ||
install_if_missing: impl Fn() -> anyhow::Result<bool>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is made so that the |
||
cfg: &'a Cfg<'a>, | ||
) -> anyhow::Result<Toolchain<'a>> { | ||
match Self::new(cfg, name) { | ||
Ok(tc) => Ok(tc), | ||
Err(RustupError::ToolchainNotInstalled { | ||
name: ToolchainName::Official(desc), | ||
.. | ||
}) if install_if_missing => { | ||
}) if install_if_missing()? => { | ||
Ok( | ||
DistributableToolchain::install(cfg, &desc, &[], &[], cfg.get_profile()?, true) | ||
.await? | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest not naming this
res
(which I usually read as short forresult
and expect to be of typeResult
) butauto_install
(or maybeauto
).