feat(config): warn user if auto-install is enabled #4454
Conversation
| pub(crate) async fn from_local( | ||
| name: LocalToolchainName, | ||
| install_if_missing: bool, | ||
| install_if_missing: impl Fn() -> anyhow::Result<bool>, |
There was a problem hiding this comment.
This change is made so that the self.should_auto_install() call is postponed to L64, and that the false positive in L60 can be suppressed.
rami3l
force-pushed
the
feat/warn-auto-install
branch
7 times, most recently
from
b1d9725 to
8d25c01
Compare
rami3l
force-pushed
the
feat/warn-auto-install
branch
2 times, most recently
from
39db6e4 to
7ab47c5
Compare
rami3l
requested review from
djc and
ChrisDenton
and removed request for
djc
src/test/clitools.rs
Outdated
| // Disable auto installation of active toolchain unless explicitly requested | ||
| cmd.env("RUSTUP_AUTO_INSTALL", "0"); |
There was a problem hiding this comment.
Not sure about this. IMO we should keep the defaults in the tests the same way they are when running real-world rustup, otherwise things get confusing.
There was a problem hiding this comment.
@djc FYI this line should be a proof, not a requirement: the changes in this commit modulo this line will continue to work even if RUSTUP_AUTO_INSTALL is set to 1 or not set at all. How about unsetting it here?
There was a problem hiding this comment.
Unsetting it seems okay to me, as that should make it similar to the default behavior.
There was a problem hiding this comment.
@djc Hmmm technically those fixes did work but now we are creating a lot of noises in the snapshot tests 🤔
I will try to find a way out of this.
There was a problem hiding this comment.
@djc How about introducing another env var to remove this warning without disabling auto install?
Update: Okay, I think this means that the evaluation of self.should_auto_install() is not lazy enough. I'll try to fix that first.
There was a problem hiding this comment.
I just had another look, it turns out that even rustup target remove will call DistributableToolchain::from_partial() which in turn calls Cfg::local_toolchain() and triggers the warning added here, meaning if the active toolchain is not installed, even removing a target from it will cause the active toolchain be installed first.
There was a problem hiding this comment.
@djc The old behavior is so confusing to the point where I start to think that the test failures are actually exposing the inconsistencies in it rather than showing a flaw in my implementation...
If we ship it like this I am afraid that we will spam the users. How about making it less frequent by using a global flag to ensure it's only printed once per process, and providing an extra flag to disable these messages?
If you agree with that, we can potentially use that flag to make the snapshot tests pass, as I don't think the changes to be made to the snapshots are very useful in the scope of this PR.
There was a problem hiding this comment.
If we ship it like this I am afraid that we will spam the users. How about making it less frequent by using a global flag to ensure it's only printed once per process, and providing an extra flag to disable these messages?
I'm struggle to remember all the context here. Can you be more specific about what you're proposing? (Maybe with some "screenshots"/mock shell transcripts?)
rami3l
force-pushed
the
feat/warn-auto-install
branch
from
7ab47c5 to
2f50a0a
Compare
rami3l
force-pushed
the
feat/warn-auto-install
branch
from
2f50a0a to
e22e682
Compare
This commit ensures that every test case relying on the `RUSTUP_AUTO_INSTALL` config option is explicitly setting it, and every other test case will pass regardless of its value.
rami3l
force-pushed
the
feat/warn-auto-install
branch
from
e22e682 to
2e5dd94
Compare
Closes #4445.