Auto-install toolchains more consistently #2252
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rbtcollins
force-pushed
the
more-auto-installs
branch
from
7e0a9ac
to
af1f031
Compare
kinnison
reviewed
Mar 4, 2020
rbtcollins
force-pushed
the
more-auto-installs
branch
4 times, most recently
from
c0a8e03
to
c579abe
Compare
kinnison
approved these changes
Mar 5, 2020
Comment on lines
-165
to
+172
| expect_err( | ||
| expect_stderr_ok( | ||
| config, | ||
| &["rustc"], | ||
| for_host!("toolchain 'nightly-{0}' is not installed"), | ||
| &["rustc", "--version"], | ||
| "info: installing component", |
There was a problem hiding this comment.
With this change, I feel the function name for the test isn't brilliant. If you can't be bothered to fix it I don't mind, otherwise it'd be nice if it were renamed something like remove_default_toolchain_auto_reinstalls or somesuch
There was a problem hiding this comment.
remove_default_toolchain_autoinstalls is what my fingers put put :P. I noticed after doing it that the very next test, for overridden toolchains, had the same naming bug, but ce la vie.
Comment on lines
-252
to
+259
| expect_err( | ||
| expect_stderr_ok( | ||
| config, | ||
| &["rustc"], | ||
| for_host!("toolchain 'nightly-{0}' is not installed"), | ||
| &["rustc", "--version"], | ||
| "info: installing component", |
This combines all auto-installation logic to the root of the codepaths leading to running a binary, which should lead to auto-installation happening for all cases, as long as a toolchain has been selected. Config.find_default no longer verifies the toolchain in order to permit this auto-installation to take place in one location; the callers of find_default do not generally need verified toolchains, so this should be fine.
rbtcollins
force-pushed
the
more-auto-installs
branch
from
c579abe
to
0c5178e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This combines all auto-installation logic to the root of the codepaths
leading to running a binary, which should lead to auto-installation
happening for all cases, as long as a toolchain has been selected.
Config.find_default no longer verifies the toolchain in order to permit
this auto-installation to take place in one location; the callers of
find_default do not generally need verified toolchains, so this should
be fine.