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

do_msvc_check: set default choice to no #2529

Merged
merged 1 commit into from
Feb 25, 2021
Merged

do_msvc_check: set default choice to no #2529

merged 1 commit into from
Feb 25, 2021

Conversation

creekorful
Copy link
Contributor

Before that the behavior on Windows when running rustup-init
without having build tools installed was to continue.

Now the user need to explicitly choose to continue installation,
and thus we are making sure they are carefully reading the warning
message.

Closes: #2514

Before that the behavior on Windows when running rustup-init
without having build tools installed was to continue.

Now the user need to explicitly choose to continue installation,
and thus we are making sure they are carefully reading the warning
message.

Closes: #2514
@kinnison
Copy link
Contributor

Before I can accept this (and I really do want to) we should take a moment to reflect on whether this might break anyone's CI. Is it plausible anyone would be programmatically responding to this prompt because they know they're using mingw, but without passing -y to the init process?

@creekorful
Copy link
Contributor Author

I guess that's plausible. But I think that users should be passing -y flag to ensure skipping behavior rather than relying on interactive prompt (especially in CI).

So this could be a breaking change for existing CI.

What should we do?

@kinnison
Copy link
Contributor

We make a call, my vote is with you - that if you want to deal with prompts like this you should be passing -y but I'd like @rbtcollins to weigh in. Robert, if you agree with us, you can merge this rather than metoo'ing :D

@rbtcollins
Copy link
Contributor

I think this needs a release note and perhaps even a doc update - I haven't reviewed the doc instructions but I have a suspicion that we have something in there that overlaps this.

@rbtcollins
Copy link
Contributor

Why does this skip test and find no cache?

You'll need to be more specific about your question; the builds look fine to me.

@kinnison
Copy link
Contributor

The caches aren't always executed cleanly because of limits on github actions' side. Don't worry about them.

@kinnison
Copy link
Contributor

I think that @rbtcollins is right that we need to double check the documentation. Can you go through the installation instructions for Windows and see if we need to tweak that a bit to explain this. If we decide to merge then I'm happy to write the release note as part of the announce / changelog when we do the release, I'll just have to make an issue to track that.

@creekorful
Copy link
Contributor Author

Hello, I have reviewed the Windows install instructions here and haven't found something to update. The documentation looks valid and up-to-date AFAIC.
Can someone else check this to make sure I haven't forgot anything?

@kinnison
Copy link
Contributor

I agree that the docs don't mention anything to the effect of the questions, nor their defaults.

All I'm wondering is if there's value in mentioning something since those who use rustup but expect to be using the GNU toolchain may find the change in default surprising. Then again people who use muscle-memory to install rustup are probably few and far between.

@rbtcollins If you agree, I suggest we merge this and ensure that a release note mentions the change.

@creekorful
Copy link
Contributor Author

Hello there!

Any news on this changes? :)

@rbtcollins rbtcollins merged commit 96107c4 into rust-lang:master Feb 25, 2021
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.

Default choice when running rustup-init without build tools installed is to continue
3 participants