-
Notifications
You must be signed in to change notification settings - Fork 926
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
fix(errors)!: improve error messages for RustupError::ToolchainNotInstalled
#4258
Conversation
bc1dd09
to
4b961f6
Compare
6231595
to
76816ea
Compare
}; | ||
|
||
Ok(Some(((&toolchain).into(), reason))) | ||
pub(crate) fn active_toolchain(&self) -> Result<Option<(LocalToolchainName, ActiveReason)>> { |
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.
We have to extract a sync function here because v1.28.1 has made the Cfg::find_active_toolchain()
async again, but we really need to find the active toolchain in a sync context in the commits that follow... (sigh)
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.
Also, I suggest renaming Cfg::find_active_toolchain()
to something else. Any ideas?
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.
To be honest I'm struggling to think of a good name.
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.
@ChrisDenton I'm currently thinking about something like:
Before | After | Remarks |
---|---|---|
find_active_toolchain() | maybe_ensure_active_toolchain() | Dispatches to one of the following (async) |
find_active_toolchain() (old implementation in 1.28.0) | active_toolchain() | Queries the active toolchain (sync) |
find_or_install_active_toolchain() | ensure_active_toolchain() | Ensures the installation of the active toolchain (async) |
... how does that sound?
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.
That sounds good to me. I think those names do make the relationship between them clearer to me at least.
1cd3ec8
to
2a91a8b
Compare
…sure_active_toolchain()` & adjust variable names
…o `ensure_active_toolchain()`
2a91a8b
to
c2ff01d
Compare
@@ -763,7 +763,7 @@ async fn default_( | |||
} | |||
}; | |||
|
|||
if let Some((toolchain, reason)) = cfg.maybe_ensure_active_toolchain(None).await? { | |||
if let Some((toolchain, reason)) = cfg.active_toolchain()? { |
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.
Since we're just emitting an info!()
, there's no need to install anything.
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.
lgtm, thanks!
if !should_install_active { | ||
return Ok(Some(((&toolchain).into(), reason))); | ||
return self.active_toolchain(); |
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.
If should_install_active
is set to false
, we return self.active_toolchain()
which was the old behavior of find_active_toolchain()
in v1.28.0, corresponding to the last 2 early-returning branch of the previous match
that gets deleted in this change:
Lines 517 to 528 in e2d9e7e
pub(crate) fn find_active_toolchain( | |
&self, | |
) -> Result<Option<(LocalToolchainName, ActiveReason)>> { | |
Ok( | |
if let Some((override_config, reason)) = self.find_override_config()? { | |
Some((override_config.into_local_toolchain_name(), reason)) | |
} else { | |
self.get_default()? | |
.map(|x| (x.into(), ActiveReason::Default)) | |
}, | |
) | |
} |
} | ||
|
||
let components = components.iter().map(AsRef::as_ref).collect::<Vec<_>>(); |
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.
If should_install
is set to true
, we delegate the installation to find_or_install_active_toolchain()
which has the same logic:
Lines 758 to 765 in e2d9e7e
if let OverrideCfg::Official { | |
toolchain, | |
components, | |
targets, | |
profile, | |
} = override_config | |
{ | |
self.ensure_installed( |
Lines 813 to 844 in e2d9e7e
let components: Vec<_> = components.iter().map(AsRef::as_ref).collect(); | |
let targets: Vec<_> = targets.iter().map(AsRef::as_ref).collect(); | |
let profile = match profile { | |
Some(profile) => profile, | |
None => self.get_profile()?, | |
}; | |
let (status, toolchain) = match DistributableToolchain::new(self, toolchain.clone()) { | |
Err(RustupError::ToolchainNotInstalled { .. }) => { | |
DistributableToolchain::install( | |
self, | |
toolchain, | |
&components, | |
&targets, | |
profile, | |
false, | |
) | |
.await? | |
} | |
Ok(mut distributable) => { | |
if verbose { | |
(self.notify_handler)(Notification::UsingExistingToolchain(toolchain)); | |
} | |
let status = if !distributable.components_exist(&components, &targets)? { | |
distributable.update(&components, &targets, profile).await? | |
} else { | |
UpdateStatus::Unchanged | |
}; | |
(status, distributable) | |
} | |
Err(e) => return Err(e.into()), | |
}; | |
Ok((status, toolchain.into())) |
@@ -675,17 +675,13 @@ impl<'a> Cfg<'a> { | |||
|
|||
// XXX: this awkwardness deals with settings file being locked already | |||
let toolchain_name = toolchain_name.resolve(&default_host_triple)?; | |||
match Toolchain::new(self, (&toolchain_name).into()) { |
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.
I have manually inlined Toolchain::new()
here and removed the statement causing an infinite recursion, because it was not needed; in fact, to allow any RustupError::ToolchainNotInstalled
to happen with this call, the toolchain must not exist:
Lines 106 to 115 in f9edccd
pub(crate) fn new(cfg: &'a Cfg<'a>, name: LocalToolchainName) -> Result<Self, RustupError> { | |
let path = cfg.toolchain_path(&name); | |
if !Toolchain::exists(cfg, &name)? { | |
return Err(match name { | |
LocalToolchainName::Named(name) => RustupError::ToolchainNotInstalled { name }, | |
LocalToolchainName::Path(name) => RustupError::PathToolchainNotInstalled(name), | |
}); | |
} | |
Ok(Self { cfg, name, path }) | |
} |
Closes #4212.