feat(cli/rustup-mode): warn about auto-installation in some subcommands#4895
feat(cli/rustup-mode): warn about auto-installation in some subcommands#4895rami3l wants to merge 3 commits into
Conversation
rami3l
added
the
backport
|
Hmmm the blast radius is too large. Lemme think: maybe we should just print the warning as an additional step for |
rami3l
force-pushed
the
feat/backport-auto-install-rustup-warn
branch
3 times, most recently
from
41ff1f3 to
191ea78
Compare
rami3l
changed the title
rustup subcommands
rami3l
force-pushed
the
feat/backport-auto-install-rustup-warn
branch
from
191ea78 to
7375543
Compare
rami3l
force-pushed
the
feat/backport-auto-install-rustup-warn
branch
from
7375543 to
5f10e42
Compare
rami3l
force-pushed
the
feat/backport-auto-install-rustup-warn
branch
from
5f10e42 to
fb400fe
Compare
rami3l
force-pushed
the
feat/backport-auto-install-rustup-warn
branch
from
fb400fe to
69a3107
Compare
| // NOTE: Special behavior for rustup v1.29 | ||
| warn!( | ||
| "this is deprecated for most `rustup` commands and may not work in a future release" | ||
| ); | ||
| warn!("see <https://github.com/rust-lang/rustup/issues/4836> for more info"); | ||
| return; |
There was a problem hiding this comment.
Okay, now I am confused about this (sorry). The way I read this, this will issue a warning for subcommands for which allow_auto_install() yields false exactly because they don't use the active toolchain anyway.
So am I misunderstanding this or is this pure noise?
There was a problem hiding this comment.
@djc Your understanding looks correct to me.
Looking back I'd argue that allow_auto_install might no longer be the best name here anymore because it now stands for "allows auto install in v1.30" or "should not emit a deprecation warning in v1.29" which is making the code harder for you to review. Is that you are trying to say?
I can offer to do a cleanup commit upfront in which I'll rename this variable, if that makes it easier for you 🙏
One downside for that is that it might make future backports more difficult if they happen to touch this part 🤔
There was a problem hiding this comment.
I'm suggesting that it does not make sense to warn about this because automatic installation is not actually affecting the running command anyway.
There was a problem hiding this comment.
@djc It doesn't mean anything in the context of the current command indeed, but I've seen people using this (e.g. rustup show) to install the active toolchain, because there was a time when rustup toolchain install wasn't available (v1.27 and earlier) and that was the only way to install the active toolchain. Maybe it makes more sense to you now?
There was a problem hiding this comment.
Hmmm maybe for the sake of clarity I should have added "to install the active toolchain, use rustup toolchain install instead" or something like that.
2 participants
Adapted from
c037408(#4840); closes #4894.