Delete content of downloads dir of user home in update subcommand

[BeniCheni]

Resolves #2038.

P.S.: can use some mentoring from review comments to:

• polish code pattern
• improve test to actually download a binary, run updateand assert the deleted content of downloads folder of user home.

I'll keep thinking, and will push edit(s), if I could figure out a better test. But thought that I'd start the PR to collect some feedback.

[BeniCheni]

OR, even simpler ...

@timw4mail, good 👁. format! is definitely more elegant. Updated.

[kinnison]

Thank you for trying to solve this particular issue. However there are a couple of fundamental things which need changing.

One is that you need to properly manage the download path by means of the Cfg instance which exists already, and not keep casting paths into strings. String guarantees utf8 correctness, but UNIX paths are NOT guaranteed to be valid UTF8, hence Rust has Pathbuf and Path.

The second is that I really don't want to see the downloads dir cleaned up unless the user has just completed a full rustup update because that's the only time where we can be sure that the partials are no longer needed.

There is a call to common::update_all_channels() in update() in rustup_mode.rs which is where you should be adding your call to delete the downloads contents. i.e. in update() after common::update_all_channels().

If you want additional help with making these changes, just let me know.

Thanks again,

D.

[kinnison]

It may also make sense to solve #1252 at the same time, by adding support for cleaning up the content of the temp directory in dist/temp.rs's impl Cfg. Perhaps you could split your cleanup function into utils/utils.rs and then use it from update() to clean up cfg.download_dir and add cfg.temp_cfg.clean() to ask dist::temp::Cfg to clean up too? If that's too much for you then just ignore this suggestion and I'll add it once your PR is mergeable.

[BeniCheni]

Perhaps you could split your cleanup function into utils/utils.rs and then use it from update() to clean up cfg.download_dir and add cfg.temp_cfg.clean() to ask dist::temp::Cfg to clean up too?

@kinnison, thanks for the insightful comments. Last commit attempted to follow the suggestion, hopefully, make the calls to clean up "downloads/" and "tmp/" dirs at the right place where after a full rustup update.

PTAL at convenience? Will promptly edit under guidance, if this version still is calling the cleanup functions at the right place.

[kinnison]

The change looks good, but I don't understand why there's a Cargo.lock change here since you don't touch Cargo.toml -- could you please remove that from the diff, and rename the variable as per my comment, then I think we're almost good to merge.

[BeniCheni]

but I don't understand why there's a Cargo.lock change here

Reverted the overlook of Cargo.lock file in the PR.

just call this dir_contents?

Sounds great. Last commit renamed to dir_contents. Thanks, @kinnison

[kinnison]

One little tweak, but I can do that on merge if you don't want to.

[BeniCheni]

One little tweak, but I can do that on merge if you don't want to.

@kinnison, pushed another update for another tweak. Thanks for catching the unnecessary borrow. 👍