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

Added dx clean stats #2934

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

Andrew15-5
Copy link
Contributor

@Andrew15-5 Andrew15-5 commented Sep 6, 2024

#2788 (comment):

dx clean doesn't show any progress or how many GiB it has removed.

Currently, I just naively parsed the output of cargo clean and then summed it with Dioxus-specific cleanings.

# dx=$(realpath ./target/debug/dx); template=$(realpath ../dioxus-template); cd /tmp/name; mkdir -p target dist; dd if=/dev/zero of=/dev/stdout bs=1 count=5520 2> /dev/null | tee target/file > dist/file; "$dx" clean
[packages/cli/src/cli/clean.rs:60:9] summary_line = "     Removed 2 files, 7.1KiB total"
[packages/cli/src/cli/clean.rs:155:5] value = "7.1"
[packages/cli/src/cli/clean.rs:155:5] unit = "KiB"
Removed 7270 bytes
[packages/cli/src/cli/clean.rs:80:9] &out_dir = "/tmp/name/dist"
out_dir exists
Removed 5.39 KiB (Dioxus-specific)
Removed 12.49 KiB total
  1. DioxusCrate::new() uses Internet connection when using dx clean
  2. DioxusCrate::new() also creates a 1.7 KiB JSON file every single time, can't we eliminate that? This will probably fail if there's no available space. clean subcommand must remove, not add.
  3. Does cargo clean only remove the ${CARGO_TARGET_DIR:-target}? If so, then I can directly grab its size, which will eliminate string parsing and the call to cargo clean (optionally).
  4. Currently, can only print the total removed size, but not the total number of files removed.
  5. Need to create 2 static hashmaps by using once_cell (can be removed if MSRV is 1.80, IIRC).

Copy link
Contributor

@matthunz matthunz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! 👍

Something I'm curious about here is maybe directly checking the folder sizes before and after a clean instead of parsing? 🤔 Just in case cargo changes its format or etc

Just a couple ideas I had poking around:

packages/cli/src/cli/clean.rs Outdated Show resolved Hide resolved
Comment on lines +143 to 156
// Maybe add optional LockOptions argument?
pub fn new(target: &TargetArgs) -> Result<Self, CrateConfigError> {
// Use this struct in new/init subcommands?
let mut cmd = Cmd::new();
cmd.lock_opts(LockOptions {
offline: true, // Don't access Internet when `dx clean` is run.
frozen: false,
locked: false,
});
cmd.features(target.features.clone());
let builder = krates::Builder::new();
// Tries to access Internet when offline.
let krates = builder.build(cmd, |_| {})?;
let package = find_main_package(target.package.clone(), &krates)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary here? I'm not sure cargo clean will attempt to access the internet, but I'm definitely not sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn't have been there if I didn't try calling dx clean on a place with no Internet connection. It was failing for some reason. That was the reason. I was thinking that this should be a separate issue. But I'm not sure. I think a global --offline (and others) flag should be added, and it should be enabled by default for clean subcommand.

So it's necessary if you are offline and want to just clean up the project dir. But an additional if statement is missing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh ok that does sound like a great idea 👍 I’ll see if I can reproduce that error, that definitely sounds like the right fix

@Andrew15-5
Copy link
Contributor Author

Something I'm curious about here is maybe directly checking the folder sizes before and after a clean instead of parsing?

Yes, but if you consider that the project can have a giant directory or is in a workspace or something, this can lead to a longer clean up time. But in the happy case, this is definitely one of the easiest things to do.

Directly checking this makes sure that we don't spend any extra time evaluating the size that will be removed. Since Rust apps are supposed to be fast, I think we can trade a bit of more verbose clean up code for maximum performance.

But again, if only ${CARGO_TARGET_DIR:-target} is removed, then I can remove parsing cargo clean's output. I definitely want this instead of what I'm doing now. I might have to ask this in rustaceans' chat or look at it's implementation (and maybe even copy something from it).

@Andrew15-5
Copy link
Contributor Author

It looks like it is indeed only ever removes the target dir and nothing else. But you can configure in more ways than I thought: https://doc.rust-lang.org/cargo/guide/build-cache.html. I'm not sure if looking for config.toml is a good idea, especially since there could be a few of those. I will mention it so that maybe in the future this can be implemented if someone is indeed using it.

@Andrew15-5
Copy link
Contributor Author

Andrew15-5 commented Sep 6, 2024

Now the command shows this:

[packages/cli/src/cli/clean.rs:28:9] target_dir = "/tmp/name/target"
[packages/cli/src/cli/clean.rs:38:9] target_dir_file_count = 1
[packages/cli/src/cli/clean.rs:38:9] target_dir_size = 1733
[packages/cli/src/cli/clean.rs:41:9] &out_dir = "/tmp/name/dist"
[packages/cli/src/cli/clean.rs:51:9] out_dir_file_count = 0
[packages/cli/src/cli/clean.rs:51:9] out_dir_size = 0
Removed 1 files, 1.69 KiB total

Because of that same file that is being added at the top of the function call. You can't get 0 files and bytes.

But at least now everything is counted manually and precisely. But cargo clean is now substituted with walkdir.

I noticed that there is also 5 spaces on the left (or maybe a tab) in the cargo clean output. And the first word is bold. Don't know if this should be also copied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants