Skip to content
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

clean: add --with-downloads option #10070

Closed
wants to merge 5 commits into from
Closed

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Nov 12, 2021

This patch adds the --with-downloads flag to cargo clean, which
allows the command to also remove related artifacts from CARGO_HOME
such as downloaded .crate files in ~/.cargo/cache/ and extracted
source directories in ~/.cargo/src/.

Note that this feature is not intended to replace the cargo-cache
command which does smart cache management. Instead, this command simply
blows away whatever it's asked to delete with no smarts whatsoever.

Fixes #3289.

/cc @matthiaskrgr since it seems likely you might have thoughts.

I realize the exact name of this argument is likely going to be subject to some bikeshedding. Alex suggested --global in #3289 (comment), but I didn't go with that since this specifically hits CARGO_HOME, which isn't necessarily "global". I initially used --include-cache, which seemed sufficiently unambiguous, but landed on --with-downloads following Matthias' suggestion.

This PR is also my first encounter with the Filesystem type, so observations about my (potential mis-)use of that would be helpful! Same thing with whether I'm dealing correctly with the package cache lock (specifically in the no -p case).

This patch adds the `--include-cache` flag to `cargo clean`, which
allows the command to also remove related artifacts from `CARGO_HOME`
such as downloaded `.crate` files in `~/.cargo/cache/` and extracted
source directories in `~/.cargo/src/`.

Note that this feature is not intended to replace the `cargo-cache`
command which does smart cache management. Instead, this command simply
blows away whatever it's asked to delete with no smarts whatsoever.

Fixes rust-lang#3289.
@rust-highfive
Copy link

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 12, 2021
@matthiaskrgr
Copy link
Member

matthiaskrgr commented Nov 13, 2021

Hmm I also have the feeling that the arg name could be improved.
If I am on a train (offline) learning rust and have no idea that there is a build artifacts cache and download cache, I may accidentally just blow away everything and no longer be able to build afterwards.

I think something like cargo clean --downloads or --with-downloads would be clearer, what do you think?

@jonhoo
Copy link
Contributor Author

jonhoo commented Nov 13, 2021

Oooh, yeah, I like --with-downloads (or --include-downloads?). It's not completely clear that it includes the extracted source too, but I think it's close enough. I'll make the adjustment on Monday unless other suggestions arise by then.

@bors
Copy link
Contributor

bors commented Nov 15, 2021

☔ The latest upstream changes (presumably #10080) made this pull request unmergeable. Please resolve the merge conflicts.

@jonhoo jonhoo changed the title clean: add --include-cache option clean: add --with-downloads option Nov 16, 2021
@alexcrichton
Copy link
Member

I'm going to switch this over to @ehuss who I believe has worked a lot more with paths/caching than I have

r? @ehuss

@rust-highfive rust-highfive assigned ehuss and unassigned alexcrichton Nov 17, 2021
@bors
Copy link
Contributor

bors commented Nov 17, 2021

☔ The latest upstream changes (presumably #10072) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss
Copy link
Contributor

ehuss commented Nov 17, 2021

Can you say more about what your use case is? I would not expect anyone to ever need to run such a command.

BTW, I've been working on some experiments with automating cleaning of stale cache files. Related to that, I think we will likely want to separate out cache-related actions to a separate subcommand (probably as a subcommand to clean, though I'm not 100% certain on that).

@jonhoo
Copy link
Contributor Author

jonhoo commented Nov 18, 2021

For cargo clean --with-downloads (so no -p), I could envision myself running that occasionally as I've definitely accumulated a bunch of old versions of things up through the years. Sure, I could do something fancy based on time using cargo-cache, but I don't think I would actually save that much compared to just blowing it all away. And I could do it without installing anything :p

As for --with-downloads -p ..., that one is definitely more niche. The use-case for me is specifically around #10071 where I happen to know that whatever Cargo may have cached for a given crate version is now incorrect. Separate from the concerns around what the correct behavior is in that case, and whether it should even be legal, I would much rather have Cargo clean its own caches than have some external program go poking in Cargo's directories to remove things.

There's also the completeness argument: if we have --with-downloads for non--p, we should also support it combined with -p.

BTW, I've been working on some experiments with automating cleaning of stale cache files. Related to that, I think we will likely want to separate out cache-related actions to a separate subcommand (probably as a subcommand to clean, though I'm not 100% certain on that).

What does "stale" mean in this context? I definitely like the sound of that though!

@ehuss
Copy link
Contributor

ehuss commented Nov 20, 2021

What does "stale" mean in this context? I definitely like the sound of that though!

The idea is to track the last time something was used, and remove unused things after some period of time. The trick is doing that efficiently, which so far has been difficult.

When I get a chance, I'll try to follow back with some ideas about the interface for manual cleaning.

@bors
Copy link
Contributor

bors commented Dec 6, 2021

☔ The latest upstream changes (presumably #10168) made this pull request unmergeable. Please resolve the merge conflicts.

@abhizer
Copy link

abhizer commented May 17, 2023

This would be a useful feature, what is blocking this from getting merged?

@epage
Copy link
Contributor

epage commented May 17, 2023

I'm going to go ahead and close this as this is fairly stale and there is more design work to be done before we get back to a PR.

@epage epage mentioned this pull request May 17, 2023
@matthiaskrgr
Copy link
Member

@abhizer you can look if https://crates.io/crates/cargo-cache suits your needs

@jonhoo
Copy link
Contributor Author

jonhoo commented Jul 30, 2023

I'm going to go ahead and close this as this is fairly stale and there is more design work to be done before we get back to a PR.

Closing this now as I assume you just hit the wrong button 😅

I still think a flag like this might be useful, but don't have a direct need for it myself any more.

@jonhoo jonhoo closed this Jul 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo clean ~/.cargo
8 participants