-
Notifications
You must be signed in to change notification settings - Fork 1k
feat(cli/rustup-mode): warn about auto-installation in some subcommands #4895
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: release/1.29
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -142,10 +142,10 @@ impl<T> EnsureInstalled<T> { | |
| } | ||
|
|
||
| impl<T: Display> EnsureInstalled<T> { | ||
| fn warn_auto_install(&self, process: &Process) { | ||
| fn warn_auto_install(&self, cfg: &Cfg<'_>) { | ||
| // If we're already in a recursion, or we haven't just installed the active toolchain, then | ||
| // don't print the warning. | ||
| let recursions = process.var("RUST_RECURSION_COUNT"); | ||
| let recursions = cfg.process.var("RUST_RECURSION_COUNT"); | ||
| if recursions.is_ok_and(|it| it != "0") || !matches!(self.status, UpdateStatus::Installed) { | ||
| return; | ||
| } | ||
|
|
@@ -156,6 +156,15 @@ impl<T: Display> EnsureInstalled<T> { | |
| ); | ||
| warn!("this might cause rustup commands to take longer time to finish than expected"); | ||
| info!("you may opt out with `RUSTUP_AUTO_INSTALL=0` or `rustup set auto-install disable`"); | ||
|
|
||
| if cfg.allow_auto_install { | ||
| return; | ||
| } | ||
|
|
||
| // NOTE: Special behavior for rustup v1.29 | ||
| warn!("auto-installation of the active toolchain is enabled when running `rustup`"); | ||
| warn!("this behavior may change in an upcoming release for most `rustup` subcommands"); | ||
| warn!("see <https://github.com/rust-lang/rustup/issues/4836> for more info") | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -423,10 +432,6 @@ impl<'a> Cfg<'a> { | |
| } | ||
|
|
||
| pub(crate) fn should_auto_install(&self) -> Result<bool> { | ||
| if !self.allow_auto_install { | ||
|
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. What is this trying to achieve?
Member
Author
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. @djc Thanks for asking! In However, since it is decided that in 1.29 we should only warn instead of rejecting directly, this commit replaces it with a warning. It should be noted that the warning should have existed in this function but since this will cause too many false positives (we should only show it when an auto-installation is actually triggered, not whenever we are in an auto-install deprecated subcommand), it is moved to another module.
Member
Author
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. Please don't hesitate to suggest any changes if you think that will make your review easier. |
||
| return Ok(false); | ||
| } | ||
|
|
||
| if let Ok(mode) = self.process.var("RUSTUP_AUTO_INSTALL") { | ||
| Ok(mode != "0") | ||
| } else { | ||
|
|
@@ -562,7 +567,7 @@ impl<'a> Cfg<'a> { | |
| match self.ensure_active_toolchain(true, false).await { | ||
| Ok(r) => { | ||
| let (tc, source) = r; | ||
| tc.warn_auto_install(self.process); | ||
| tc.warn_auto_install(self); | ||
| Ok(Some((tc.inner, source))) | ||
| } | ||
| Err(e) => match e.downcast_ref::<RustupError>() { | ||
|
|
@@ -768,7 +773,7 @@ impl<'a> Cfg<'a> { | |
| let install_if_missing = self.should_auto_install()?; | ||
| let EnsureInstalled { inner: tc, status } = | ||
| Toolchain::from_local(tc, install_if_missing, self).await?; | ||
| EnsureInstalled::new(tc.name(), status).warn_auto_install(self.process); | ||
| EnsureInstalled::new(tc.name(), status).warn_auto_install(self); | ||
| Ok((tc, source)) | ||
| } | ||
| None => { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Seems weird to have both the warning above and this warning shown at the same time?
View changes since the review
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.
@djc Good catch!
How about showing the existing warning only when the deprecation notice is not shown?