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

warn toolchain on different platform #2534

Merged
merged 1 commit into from
Nov 17, 2020
Merged

Conversation

frbimo
Copy link
Contributor

@frbimo frbimo commented Oct 22, 2020

Fixes
#2520

Testing
cargo test

@frbimo
Copy link
Contributor Author

frbimo commented Oct 27, 2020

@kinnison kindly reminder*

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.

There's only one tiny niggle around what I feel might be a wasted clone() otherwise this looks excellent. I'm especially grateful you added a test for this.

src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
@frbimo
Copy link
Contributor Author

frbimo commented Oct 28, 2020

@kinnison ptal

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.

This looks good to me. One final chance for @rbtcollins to have a look in case the approach or wording isn't to his taste, otherwise I'll look to merge it fairly soon.

@rbtcollins
Copy link
Contributor

rbtcollins commented Oct 30, 2020 via email

@kinnison
Copy link
Contributor

I'm loathe to add an interactive prompt to a part of rustup which has never been interactive before now. If we think interactivity makes sense then we have to detect if there's a tty on stdin and stuff like that, otherwise it'd be a pain for anyone who has automated this in CI or similar. Also we simply don't know for sure so warning non-interactively is probably enough.

if host toolchain's arch is different with target.

Signed-off-by: frbimo <[email protected]>
@kinnison
Copy link
Contributor

I rebased to clean up the conflict and am minded to merge this as I remain of the opinion that interactive prompting doesn't seem ideal to me.

@kinnison
Copy link
Contributor

The CI badness is snapcraft being odd.

@kinnison kinnison merged commit 5674652 into rust-lang:master Nov 17, 2020
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.

3 participants