-
Notifications
You must be signed in to change notification settings - Fork 985
Change toolchain override message from info to warn #4581
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
base: main
Are you sure you want to change the base?
Conversation
|
I don't think this PR makes sense. Since the behavior is intended, displaying this as a warning violates a common expectation of what a warning means in most logging. |
I don't think that's the case here. This should be a warning because it's easy to miss that when you set the default toochain, but have an override then setting the default actually has no effect. This message is warning the user user that this configuration is nonsense and won't do what you expect from running commands that alter the default toolchain. This is a very real problem that came from thinking that we were running some tasks against the beta toolchain, but actually weren't. Warnings in logging are often used to mark places like this that are possibly a problem but may not be, hence this is not an error. |
From my perspective, this message is informing the user that rustup is executing on its configuration. I don't think rustup can decide whether that configuration is nonsense or not. |
|
Strong opinion, weakly held disagreement with your perspective here. |
What part do you disagree with? How would we make the warning not annoying to people who have intentionally set up an override? |
|
I believe this is a footgun. It's not an error, but it's an easy misconfiguring that rustup can and should highlight. The ability to install a default rust toolchain when you're in a context where that default will not be used should be an annoying warning even when this is intentional as using a tool like dtolnay/rust-toolchain I can see how having any warnings that you can't disable might be also a problem. But it's not an error, and it will never fail a ci process to see this warning. The ability to explicitly opt in to warnings as errors for compilation is something to bear in mind here. Warnings that help ensure that things are ok are good warnings. Making them info level makes them bad warnings. As mentioned, I'm happy to defer to your wider context on this though as it's unlikely that I'll personally make this mistake again and it may be me over indexing on prevention of something that happens rarely. (That said, I just submitted a PR at work where I would have hit this exact problem had I not been aware of it). |
|
@rami3l @ChrisDenton thoughts? |
Problem:
rustup currently logs an info-level message when a rust-toolchain.toml file overrides the active toolchain. This message is easy to miss in CI logs, leading to confusion when the expected toolchain (e.g., beta) isn’t being used.
Fix:
Promote the message about toolchain override (rust-toolchain.toml) from info to warn level, making it more visible during builds and CI runs.
Fix #4504