-
Notifications
You must be signed in to change notification settings - Fork 877
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
Deal cleanly with malformed default-host #1578
Conversation
f7f4695
to
c50d1b0
Compare
At this point, I've added a My next steps will be to try and improve the situation by suggesting the build triple and the 'stable' channel if these errors come up. |
c50d1b0
to
b9f786a
Compare
6e26bae
to
4af0bd8
Compare
082440b
to
49a9cec
Compare
I've un-WIPped this because I finally added a commit which addresses the direct bad action in #745 I also rebased, so hopefully this'll be good. |
01e8877
to
a71cf31
Compare
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.
Thanks for the PR, there are some comments inline
e, | ||
dist::TargetTriple::from_host_or_build().as_ref() | ||
) | ||
})?; |
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 think we should move this checking to its own function - do_pre_install_sanity_checks
is about checking the user's system, rather than the installation options
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.
Okay, I've created a function called do_pre_install_options_sanity_checks()
and moved the content into there, though for now I'm calling it from the same spot as do_pre_install_sanity_checks()
unless I find something better.
src/rustup-dist/src/dist.rs
Outdated
@@ -210,6 210,12 @@ impl TargetTriple { | |||
} | |||
} | |||
|
|||
impl AsRef<str> for TargetTriple { |
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 don't think we should use AsRef
for this as it is doing a type conversion, not just a ref/deref
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.
Fair. Also there's already a Display so I've dropped this particular commit from the PR
"stable" | ||
} else { | ||
&opts.default_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.
This seems like it is duplicating behaviour defined elsewhere.
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 my knowledge it's not per-se. This is simply because using PartialToolchainDesc::resolve needs a valid toolchain name or it won't work (and none isn't valid IIRC).
It looks similar to other checks, but this time we're resolving from options and don't have a Cfg
struct so I can't just call set_default_host
et al, and deal with things that way. Also the debug!()
is meant to be useful to tell us that it's behaving right and picking the correct fully qualified toolchain name.
If I've missed something you know of, please do let me know, but I've had a fairly good grep around I think.
// Run some basic checks against the constructed configuration | ||
// For now, that means simply checking that 'stable' can resolve | ||
// for the current configuration. | ||
cfg.resolve_toolchain("stable") |
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 don't understand why we need this check. Could you explain please?
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.
Sure. By doing this check here we verify that later attempts to resolve the toolchain are more likely to succeed. It means we report the error about bad host types etc earlier in the sequence of operations. This leads to, in my opinion, a slightly better user experience.
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.
Thanks for the explanation. I wonder if this will ever give a false negatve? If we have some toolchain which is nightly-only perhaps?
src/rustup/config.rs
Outdated
@@ -470,7 478,20 @@ impl Cfg { | |||
} | |||
|
|||
pub fn set_default_host_triple(&self, host_triple: &str) -> Result<()> { | |||
if dist::PartialTargetTriple::from_str(host_triple).is_none() { | |||
if let Some(host) = dist::PartialTargetTriple::from_str(host_triple) { |
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 like these checks are duplicating the checks done in resolve
?
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.
Yeah, for some reason I was entirely blind to how to reuse PartialToolchainDesc::resolve()
when I wrote this check. I've reworked it now.
It is better to report errors cleanly rather than panic. If the default-host is set badly then we can end up panicking because we're unable to parse it. Since default-host is user input, it's better we don't panic.
In order to have useful early error messages when the configuration is unusably broken, allow the Cfg struct to smoke-test itself during `Cfg::from_env()`
It can be useful to debug rustup's operation. This comit adds a debug macro using BRIGHT_BLUE which only displays if RUSTUP_DEBUG is in the environment
When rustup-init is running, we need to verify that the provided host triple and toolchain channel can be used together to determine a full toolchain triple (quadruple?). If we can't do this during the sanity checks, error out early.
In order for the user to install rustup and no toolchain, we must support the 'none' toolchain which isn't actually a valid name. As such whitelist it in the santiy checks.
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.
Thanks for your review @nrc - I've responded to everything I think, and I'll push a new set of commits momentarily
src/rustup-dist/src/dist.rs
Outdated
@@ -210,6 210,12 @@ impl TargetTriple { | |||
} | |||
} | |||
|
|||
impl AsRef<str> for TargetTriple { |
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.
Fair. Also there's already a Display so I've dropped this particular commit from the PR
// Run some basic checks against the constructed configuration | ||
// For now, that means simply checking that 'stable' can resolve | ||
// for the current configuration. | ||
cfg.resolve_toolchain("stable") |
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.
Sure. By doing this check here we verify that later attempts to resolve the toolchain are more likely to succeed. It means we report the error about bad host types etc earlier in the sequence of operations. This leads to, in my opinion, a slightly better user experience.
src/rustup/config.rs
Outdated
@@ -470,7 478,20 @@ impl Cfg { | |||
} | |||
|
|||
pub fn set_default_host_triple(&self, host_triple: &str) -> Result<()> { | |||
if dist::PartialTargetTriple::from_str(host_triple).is_none() { | |||
if let Some(host) = dist::PartialTargetTriple::from_str(host_triple) { |
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.
Yeah, for some reason I was entirely blind to how to reuse PartialToolchainDesc::resolve()
when I wrote this check. I've reworked it now.
e, | ||
dist::TargetTriple::from_host_or_build().as_ref() | ||
) | ||
})?; |
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.
Okay, I've created a function called do_pre_install_options_sanity_checks()
and moved the content into there, though for now I'm calling it from the same spot as do_pre_install_sanity_checks()
unless I find something better.
"stable" | ||
} else { | ||
&opts.default_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.
To my knowledge it's not per-se. This is simply because using PartialToolchainDesc::resolve needs a valid toolchain name or it won't work (and none isn't valid IIRC).
It looks similar to other checks, but this time we're resolving from options and don't have a Cfg
struct so I can't just call set_default_host
et al, and deal with things that way. Also the debug!()
is meant to be useful to tell us that it's behaving right and picking the correct fully qualified toolchain name.
If I've missed something you know of, please do let me know, but I've had a fairly good grep around I think.
Perform similar checks to when resolving toolchains in order to reduce the possibility that the configuration will end up with an unusable default-host value. This finally addresses the initial point in rust-lang#745 and thus fixes it. Signed-off-by: Daniel Silverstone <[email protected]>
a71cf31
to
36ec58f
Compare
Looks good, thanks! |
It is better to report errors cleanly rather than panic. If the
default-host is set badly then we can end up panicking because
we're unable to parse it. Since default-host is user input, it's
better we don't panic.
This PR will, when complete, add in some appropriate checks for default-host
and hopefully lead the user to resolution of the problem resulting in fixing #745
So far, all I've done is to change the
.expect()
into.ok_or()
and propagate theResult<...>
properly. This removes the panic but means that we get some very odd reports from things
like
rustup show
, hence the plan to add some proper earlier checks which can report errors more usefully.If this approach seems sane, I'd appreciate any pointers possible on where to add appropriate checks, and in addition, where to add appropriate tests.