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

Fixed panic when no toolchain was configured #1787

Merged
merged 2 commits into from
Apr 23, 2019
Merged

Fixed panic when no toolchain was configured #1787

merged 2 commits into from
Apr 23, 2019

Conversation

Glamhoth
Copy link
Contributor

Fixed panic when no toolchain was configured and user run
rustup default.

@kinnison
Copy link
Contributor

Can you describe the situation in which this happens, and would it be possible to write a test to demonstrate it no longer does? If the testing is too hard I won't complain too much, though with the introduction of a message, it'd be good to have a test which explicitly trips it.

@Glamhoth
Copy link
Contributor Author

It happens when there are no toolchain configured in .rustup/settings.toml file, i.e. just after rustup was installed. I think it would be possible to test if running rustup default returns, in that case, an error.

@kinnison
Copy link
Contributor

OK, yes I can now reproduce the issue so I agree it needs fixing. That's good.

Now we need to devise a test for this.

Perhaps, in test/cli-misc.rs you might try adding:

#[test]
fn no_panic_on_default_missing() {
    setup(&|config| {
        expect_err(
            config,
            &["rustup", "default"],
            "no default toolchain configured",
        );
    })
}

And see if that helps. I can confirm that the above test would panic on current master.

@Glamhoth
Copy link
Contributor Author

Test passes after my fix.

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kinnison kinnison merged commit cdc1489 into rust-lang:master Apr 23, 2019
@Glamhoth Glamhoth deleted the default_fix branch April 25, 2019 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants