Add a progress indicator for `cargo clean`#10236

theo-lw · 2021-12-28T02:18:39Z

Closes #9981.

Cleaning the entire target directory:

Cleaning some packages:

rust-highfive · 2021-12-28T02:18:41Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

weihanglo

The progress bar seems disappear when --verbose is specified with -p. I guess it is replaced by the output of Removing .... We can tick the progress bar after each rm_rf, but if a removal is stuck the progress bar still disappears for a while.

weihanglo · 2021-12-28T06:08:30Z

src/cargo/ops/cargo_clean.rs

What if there is a package with lots of artifacts? I feel like it would still look like "get stuck"?

It would. I added a message listing of the # of files/folders cleaned so users can see that it isn't stuck.

weihanglo · 2021-12-28T06:14:57Z

src/cargo/ops/cargo_clean.rs

Not sure but maybe keep the verbose Removing ... as what fn rm_rf does? Though it only displays Removing /path/to/target-dir

Oops, updated so the verbose Removing ... is kept.

weihanglo · 2021-12-28T06:34:28Z

src/cargo/ops/cargo_clean.rs

NIT: Feel like progress can be a part of function body instead of a parameter.

theo-lw · 2021-12-29T04:48:06Z

Fixed the issue of Removing... not being shown in verbose mode:

Fixed the issue of the progress bar disappearing in verbose mode:

weihanglo

Awesome. I think it's good to move forward and wait the review from maintainers. 🎉

src/cargo/ops/cargo_clean.rs

alexcrichton

Seems reasonable to me, thanks for this!

alexcrichton · 2022-01-04T15:37:41Z

src/cargo/ops/cargo_clean.rs

 }
+
+fn clean_entire_folder(path: &Path, config: &Config) -> CargoResult<()> {
+    let num_paths = walkdir::WalkDir::new(path).into_iter().count();


This naively seems like it would have a large impact and slow things down because we have to walk everything before actually removing everything. I don't have a great sense for the practical impact of this though.

An alternative to this I think would be to perform the walk once, collect a list of all paths to remove, then remove them all in a loop. That would mean we only have to walk once and should be similar in performance I think.

alexcrichton · 2022-01-04T15:39:20Z

src/cargo/ops/cargo_clean.rs

I think it'd be ok to change this from impl Cleaning... to dyn Cleaning... since we don't need the monomorphization aspect here per se

ehuss · 2022-01-04T17:32:04Z

Just to echo some of Alex's comments, can you do some benchmarking and report if there is a performance hit with this? I think it would be good to test with some medium-ish sized projects, and on different systems (windows in particular has some slow fs behaviors). A small hit is fine, but if this makes cargo clean twice as slow, that might be concerning.

theo-lw · 2022-01-04T23:10:27Z

I'll do some benchmarking :) didn't realize that Windows had some peculiarities

Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>

theo-lw · 2022-01-05T18:05:42Z

Running cargo clean on rust-analyzer on Windows:

Before these changes:

After these changes:

So it looks like there is a performance regression of ~12% on Windows.

Running cargo clean on rust-analyzer on my mac:

Before these changes:

After these changes:

Performance doesn't have seemed to change much on my mac.

I'll experiment with collecting the paths into a vector.

ehuss · 2022-03-30T23:43:58Z

I did some testing on various systems and different project sizes, and got roughly the same performance hit that you mentioned (ranging from 0 to 10%). The small performance hit is a bit unfortunate, but overall I don't think will be noticed much. I'm a little concerned for using cargo over a slow network connection, as I think this could tick quite frequently. I think there can possibly be followup in the future if that turns into a serious concern. Another possibility is to explore deleting files in parallel.

@rust-lang/cargo I'm going to just do a quick fcp check since this changes the output of a stable command, and there is a small performance hit.

@rfcbot fcp merge

rfcbot · 2022-03-30T23:43:58Z

Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot · 2022-03-31T00:29:52Z

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot · 2022-04-10T01:10:52Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

weihanglo · 2022-04-10T07:06:26Z

Just posted the possible extensions as a new issue (#10552). I am going to merge this PR.

@bors r+

bors · 2022-04-10T07:06:28Z

📌 Commit 60cfe7e has been approved by weihanglo

bors · 2022-04-10T07:06:36Z

⌛ Testing commit 60cfe7e with merge f51e799...

bors · 2022-04-10T07:42:21Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing f51e799 to master...

Update cargo 11 commits in e2e2dddebe66dfc1403a312653557e332445308b..dba5baf4345858c591517b24801902a062c399f8 2022-04-05 17:04:53 +0000 to 2022-04-13 21:58:27 +0000 - Part 6 of RFC2906 - Switch the inheritance source from `workspace` to… (rust-lang/cargo#10564) - Part 5 of RFC2906 - Add support for inheriting `rust-version` (rust-lang/cargo#10563) - Add support for rustc --check-cfg well known names and values (rust-lang/cargo#10486) - Reserve filename `Cargo.toml.orig` in `cargo package` (rust-lang/cargo#10551) - Retry command invocation with argfile (rust-lang/cargo#10546) - Add a progress indicator for `cargo clean` (rust-lang/cargo#10236) - Ensure host units don't depend on Docscrape units, fixes rust-lang/cargo#10545 (rust-lang/cargo#10549) - Fix docs: Bindeps env vars are passed to build script at runtime (rust-lang/cargo#10550) - Part 4 of RFC2906 - Add support for inheriting `readme` (rust-lang/cargo#10548) - Part 3 of RFC2906 - Add support for inheriting `license-path`, and `depednency.path` (rust-lang/cargo#10538) - Bump to 0.63.0, update changelog (rust-lang/cargo#10544)

Add a progress indicator for cargo clean

4d46ffb

rust-highfive assigned ehuss Dec 28, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 28, 2021

weihanglo reviewed Dec 28, 2021

View reviewed changes

Print the bar in verbose mode, keep count of the number of cleaned files

9ab86fc

weihanglo approved these changes Jan 3, 2022

View reviewed changes

src/cargo/ops/cargo_clean.rs Outdated Show resolved Hide resolved

src/cargo/ops/cargo_clean.rs Outdated Show resolved Hide resolved

alexcrichton reviewed Jan 4, 2022

View reviewed changes

theo-lw and others added 3 commits January 5, 2022 10:32

Remove borow

3e1a8ba

Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>

Remove borrow

9b39df9

Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>

Dyn instead of impl

60cfe7e

ehuss added the T-cargo Team: Cargo label Mar 30, 2022

rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Apr 10, 2022

weihanglo mentioned this pull request Apr 10, 2022

Performance enhancement for cargo clean #10552

Open

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 10, 2022

bors merged commit f51e799 into rust-lang:master Apr 10, 2022

ehuss mentioned this pull request Apr 14, 2022

Update cargo rust-lang/rust#96031

Merged

ehuss added this to the 1.62.0 milestone Apr 22, 2022

ehuss mentioned this pull request Aug 10, 2023

cargo clean with target as a symlink fails #12473

Closed

Conversation

theo-lw commented Dec 28, 2021

Uh oh!

rust-highfive commented Dec 28, 2021

Uh oh!

weihanglo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theo-lw commented Dec 29, 2021

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ehuss commented Jan 4, 2022

Uh oh!

theo-lw commented Jan 4, 2022

Uh oh!

theo-lw commented Jan 5, 2022

Uh oh!

ehuss commented Mar 30, 2022

Uh oh!

rfcbot commented Mar 30, 2022 • edited by epage Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rfcbot commented Mar 31, 2022

Uh oh!

rfcbot commented Apr 10, 2022

Uh oh!

weihanglo commented Apr 10, 2022

Uh oh!

bors commented Apr 10, 2022

Uh oh!

bors commented Apr 10, 2022

Uh oh!

bors commented Apr 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Comments

weihanglo left a comment •

edited

Loading

rfcbot commented Mar 30, 2022 •

edited by epage

Loading