-
Notifications
You must be signed in to change notification settings - Fork 892
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
uninstalls toolchains prior to deleting the rustup home folder #2864
base: master
Are you sure you want to change the base?
uninstalls toolchains prior to deleting the rustup home folder #2864
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see at least one test validating that this uninstall is happening. Probably it ought to be a new test in tests/cli-self-upd.rs
something like an fn uninstall_removes_installed_toolchains()
sure thing, work has gotten a bit more stressful in the last couple weeks, I will try to do something this weekend |
@Darunada Do you still have time to advance it? If you don't have time I would be happy to help add that test. But it would be great if you could move forward. Thanks! |
10e3130
to
5a9b7a0
Compare
5a9b7a0
to
4abc90a
Compare
2e7c6c9
to
6745cd9
Compare
Sorry for the delay, I went ahead and added a test. Let me know if you like the approach! |
let no_prompt = m.get_flag("no-prompt"); | ||
|
||
self_update::uninstall(no_prompt) | ||
fn self_uninstall(cfg: &Cfg, m: &ArgMatches<'_>) -> Result<utils::ExitCode> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't have a lifetime argument here.
Fixes #2789
I was able to reproduce the issue, and with this change the command now succeeds with the following output.