Do not allow none as custom toolchain name#3174
Do not allow none as custom toolchain name#3174ashutoshvarma wants to merge 1 commit intorust-lang:masterfrom
none as custom toolchain name#3174Conversation
| impl<'a> CustomToolchain<'a> { | ||
| pub fn new(toolchain: &'a Toolchain<'a>) -> Result<CustomToolchain<'a>> { | ||
| if toolchain.is_custom() { | ||
| if toolchain.is_custom() && toolchain.name() != "none" { |
There was a problem hiding this comment.
@rbtcollins Do you think we need to move this special name into is_custom_name fn? Or we can just determine it here?
There was a problem hiding this comment.
I think it's not a custom name, right? the whole point is that we're not allowing it as a name for custom toolchains.
There was a problem hiding this comment.
I think it's not a custom name, right?
Actually, in the current is_custom_name fn we treat it as a custom name.
Line 194 in 060b8f9
So it's more like a special case and probably we can put it into is_custom_name.
There was a problem hiding this comment.
I agree with @hi-rustin that this is perhaps the wrong place.
A quick look at the code shows that we haven't integrated the "none" value of a toolchain systematically into the code. We shouldn't need to patch it here and there and everywhere.
We have things like resolve_toolchain(&name) that might be a decent place to flat out prohibit 'none' as a toolchain name, irrespective of custom-or-not
jyn514
left a comment
jyn514
left a comment
There was a problem hiding this comment.
This seems fine to me; I think if you prevent creating a toolchain with this name that's enough and you don't need to also change other parts of rustup. In fact I would prefer not to for compatibility with people who happened to already have this name in an old version of rustup and then upgraded their install.
| expect_ok(config, &["rustup", "update", "nightly"]); | ||
| expect_ok(config, &["rustup", "default", "nightly"]); | ||
| expect_stdout_ok(config, &["rustup", "show"], "custom"); | ||
| // should not allow 'none' for custom toolchain name |
There was a problem hiding this comment.
Could you please make this a separate test? Thanks!
| impl<'a> CustomToolchain<'a> { | ||
| pub fn new(toolchain: &'a Toolchain<'a>) -> Result<CustomToolchain<'a>> { | ||
| if toolchain.is_custom() { | ||
| if toolchain.is_custom() && toolchain.name() != "none" { |
There was a problem hiding this comment.
I agree with @hi-rustin that this is perhaps the wrong place.
A quick look at the code shows that we haven't integrated the "none" value of a toolchain systematically into the code. We shouldn't need to patch it here and there and everywhere.
We have things like resolve_toolchain(&name) that might be a decent place to flat out prohibit 'none' as a toolchain name, irrespective of custom-or-not
|
I have a large refactoring of our toolchain name handling that will obsolete this. |
|
See #3340 |
4 participants
Fix #3130
Throws when
noneis used for custom toolchain name.