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

Deal cleanly with malformed default-host #1578

Merged
merged 7 commits into from Jan 22, 2019
Merged

Conversation

@kinnison
Copy link
Collaborator

kinnison commented Dec 14, 2018

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 the Result<...>
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.

@kinnison kinnison force-pushed the kinnison:kinnison/fix-745 branch from f7f4695 to c50d1b0 Dec 16, 2018
@kinnison

This comment has been minimized.

Copy link
Collaborator Author

kinnison commented Dec 16, 2018

At this point, I've added a debug!() macro, fixed a minor typo in one of the sanity check warnings, and added an explicit sanity check on the host and toolchain inputs to the installer. This combo should handle the situation mentioned in #874 by @alexcrichton too.

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.

@kinnison kinnison force-pushed the kinnison:kinnison/fix-745 branch from c50d1b0 to b9f786a Dec 16, 2018
@kinnison

This comment has been minimized.

Copy link
Collaborator Author

kinnison commented Dec 19, 2018

This PR will need further work as #1583 has been merged and #1585 may get merged soon. Also there'll be a PR about clippy lint fixes which might happen before this too. I'll get back to it, I promise.

@kinnison kinnison force-pushed the kinnison:kinnison/fix-745 branch 3 times, most recently from 6e26bae to 4af0bd8 Jan 8, 2019
@kinnison kinnison changed the title WIP: Deal cleanly with malformed default-host Deal cleanly with malformed default-host Jan 14, 2019
@kinnison kinnison force-pushed the kinnison:kinnison/fix-745 branch from 082440b to 49a9cec Jan 14, 2019
@kinnison

This comment has been minimized.

Copy link
Collaborator Author

kinnison commented Jan 14, 2019

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.

@kinnison kinnison force-pushed the kinnison:kinnison/fix-745 branch 2 times, most recently from 01e8877 to a71cf31 Jan 14, 2019
Copy link
Member

nrc left a comment

Thanks for the PR, there are some comments inline

e,
dist::TargetTriple::from_host_or_build().as_ref()
)
})?;

This comment has been minimized.

Copy link
@nrc

nrc Jan 16, 2019

Member

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

This comment has been minimized.

Copy link
@kinnison

kinnison Jan 16, 2019

Author Collaborator

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.

@@ -210,6 +210,12 @@ impl TargetTriple {
}
}

impl AsRef<str> for TargetTriple {

This comment has been minimized.

Copy link
@nrc

nrc Jan 16, 2019

Member

I don't think we should use AsRef for this as it is doing a type conversion, not just a ref/deref

This comment has been minimized.

Copy link
@kinnison

kinnison Jan 16, 2019

Author Collaborator

Fair. Also there's already a Display so I've dropped this particular commit from the PR

"stable"
} else {
&opts.default_toolchain
};

This comment has been minimized.

Copy link
@nrc

nrc Jan 16, 2019

Member

This seems like it is duplicating behaviour defined elsewhere.

This comment has been minimized.

Copy link
@kinnison

kinnison Jan 16, 2019

Author Collaborator

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")

This comment has been minimized.

Copy link
@nrc

nrc Jan 16, 2019

Member

I don't understand why we need this check. Could you explain please?

This comment has been minimized.

Copy link
@kinnison

kinnison Jan 16, 2019

Author Collaborator

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.

This comment has been minimized.

Copy link
@nrc

nrc Jan 17, 2019

Member

Thanks for the explanation. I wonder if this will ever give a false negatve? If we have some toolchain which is nightly-only perhaps?

@@ -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) {

This comment has been minimized.

Copy link
@nrc

nrc Jan 16, 2019

Member

Seems like these checks are duplicating the checks done in resolve?

This comment has been minimized.

Copy link
@kinnison

kinnison Jan 16, 2019

Author Collaborator

Yeah, for some reason I was entirely blind to how to reuse PartialToolchainDesc::resolve() when I wrote this check. I've reworked it now.

kinnison added 6 commits Dec 14, 2018
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.
Copy link
Collaborator Author

kinnison left a comment

Thanks for your review @nrc - I've responded to everything I think, and I'll push a new set of commits momentarily

@@ -210,6 +210,12 @@ impl TargetTriple {
}
}

impl AsRef<str> for TargetTriple {

This comment has been minimized.

Copy link
@kinnison

kinnison Jan 16, 2019

Author Collaborator

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")

This comment has been minimized.

Copy link
@kinnison

kinnison Jan 16, 2019

Author Collaborator

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.

@@ -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) {

This comment has been minimized.

Copy link
@kinnison

kinnison Jan 16, 2019

Author Collaborator

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()
)
})?;

This comment has been minimized.

Copy link
@kinnison

kinnison Jan 16, 2019

Author Collaborator

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
};

This comment has been minimized.

Copy link
@kinnison

kinnison Jan 16, 2019

Author Collaborator

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 #745 and thus fixes it.

Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
@kinnison kinnison force-pushed the kinnison:kinnison/fix-745 branch from a71cf31 to 36ec58f Jan 16, 2019
@nrc

This comment has been minimized.

Copy link
Member

nrc commented Jan 22, 2019

Looks good, thanks!

@nrc nrc merged commit b697df1 into rust-lang:master Jan 22, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@kinnison kinnison deleted the kinnison:kinnison/fix-745 branch Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.