rust-lang
fix(toolchain): improve error message for '+toolchain' prefix
When users accidentally use +stable with rustup run, we now provide a message instructing them to remove the + prefix.
closes #4571
@djc Just confirming, currently it's far from common but we do allow toolchain names with the + prefixes right? I don't see anything about that in rustup toolchain link --help so I assume it's still valid.
If that is the case, then it's possible that we need to raise this error later in the pipeline, only when a toolchain with the said name is not found?
As a user used to
+toolchain, it can be easy to accidentally do that withcargo runbut the error you get doesn't communicate that problem:$ rustup run +stable cargo check error: toolchain '+stable' is not installed #4571
From what I'm seeing + prefixes are only specifically supported at the top level (in both rustup and proxy mode). I definitely don't recall any details but I think I'm comfortable with the risk of these changes.
From what I'm seeing
+prefixes are only specifically supported at the top level (in both rustup and proxy mode). I definitely don't recall any details but I think I'm comfortable with the risk of these changes.
@djc Sorry for not being clear enough previously, but what I wanted to say is about the edge case of a toolchain name starting with +, say +foo, so that the calls would be written as cargo ++foo ....
I know this sounds crazy and should be banned in https://github.com/rust-lang/rustup/issues/4059, but IIRC for now such names are still valid 😓
> rustup run +foo cargo
error: toolchain '+foo' is not installed
> rustup ++foo show
Default host: aarch64-apple-darwin
[...]
error: toolchain '+foo' is not installed
... totally coherent 😱
forbid toolchain names starting with
+
Does this also steer from the original issue at hand?
Hmmm the original error message was confusing but made sense exactly because + has been permitted at the beginning of the toolchain names... So I won't call this a deviation.
Also, given the current error message, it should be easy to deduce that a toolchain name is expected as the positional argument here, rather than the +-prefixed form.
Besides, I think this is also an opportunity for us to test out what toolchain names are actually used so at least we'd have some expectation of what can be broken.
Finally, this could allow smarter guesses to happen, since we are sure that the + prefix here is not conveying any particular intention.
- For example, when
stableis installed we can eventually issue something like: "+stableis not a valid toolchain name, did you meanstable?" which IMHO would be a great follow-up to do.
As I already said, this looks good to me as long as @djc feels the same. However we could potentially use more prudence in merging yet another breaking change between beta and stable... I'd currently prefer playing the safe card and postpone this to the upcoming 2.0 👀
However we could potentially use more prudence in merging yet another breaking change between beta and stable... I'd currently prefer playing the safe card and postpone this to the upcoming 2.0 👀
I mean, do you have a concrete plan for 2.0? When will that be released? I still think the version number is not going to make much difference in how much testing/caution people exercise before/when upgrading.
I mean, do you have a concrete plan for 2.0? When will that be released? I still think the version number is not going to make much difference in how much testing/caution people exercise before/when upgrading.
@djc Thank you for this constructive question.
I'm sorry but I'm not sure if we are on the same page here; I think my stance with breaking changes will not change significantly whether the next non-patch release will be 1.30 or 2.0, as it is in theory more dangerous after a beta release has been cut.
Of course, in terms of actual impact this should be considered a minor breakage, so I'm also okay with merging this patch immediately, as long as enough attention has been drawn. Your call!
Given the amount of response on internals I have no reason to feel the beta has resulted in a substantial amount of testing. Maybe we should do a post to the Inside Rust blog?
Given the amount of response on internals I have no reason to feel the beta has resulted in a substantial amount of testing. Maybe we should do a post to the Inside Rust blog?
@djc Yes, that's a good idea! I'll take care of it.
Besides, after giving this PR another look, I think I'm probably worrying too much again over a small change that can be easily reverted in the next patch release. My apologies!