Make default output less noisy #81

marcospb19 · 2022-12-25T18:34:02Z

Fixes #74.

cargo-sweep logs every deleted file, this makes the output
very noisy and hard to follow (outputs approximately a thousand
lines per cleaned project).

This PR changes the default output to omit deleted files, and
only show them with the --verbose flag.

marcospb19 · 2022-12-25T18:36:31Z

PR is a draft because this is the remaining output after the changes:

[INFO] Cleaned 5.34 GiB
[INFO] Using all installed toolchains: ["stable-2022-09-22-x86_64-unknown-linux-
gnu", "stable-x86_64-unknown-linux-gnu", "nightly-2022-06-20-x86_64-unknown-linu
x-gnu", "nightly-2022-08-06-x86_64-unknown-linux-gnu", "nightly-x86_64-unknown-l
inux-gnu", "1.63.0-x86_64-unknown-linux-gnu", "1.64.0-x86_64-unknown-linux-gnu"]

So now you don't know what project was cleaned.

marcospb19 · 2022-12-25T18:41:26Z

The list of installed toolchains is shown for every project it sweeps, but I think it is always the same.

So we should probably just show it once at the start.

This is the function that loads the toolchains:

cargo-sweep/src/fingerprint.rs

Lines 373 to 374 in 75414ca

    
           fn rustup_toolchain_list() -> Option<Vec<String>> { 
        
               let out = Command::new("rustup").args(["toolchain", "list"]).output();

It's always constant, because it calls the rustup binary, I think we can assume the binary doesn't change (also, cargo sweep nevers cd into other directory).

marcospb19 · 2022-12-25T20:03:27Z

Should be ready for review now, fixing the list of toolchains being outputted repeatedly will require a little bit of refactoring, so I prefer doing it in another PR.

marcospb19 · 2022-12-25T20:08:34Z

Sorry I had to force push again, cargo clippy --fix broke a test.

tests/integration.rs

jyn514 · 2022-12-25T20:15:01Z

I just fixed the clippy warnings in #80 (and made a couple other changes that should make the errors easier to read) - do you mind rebasing over master?

Also, please revert the changes to the assertion message, like the error says they don't do what you expect in 2018 edition and I would prefer to switch the edition in a separate PR (edit: done #83).

marcospb19 · 2022-12-25T20:19:49Z

Oh ok, I'll do that, I was having a lot of trouble with the assertion message, so I force pushed like 5 times, let me fix it.

by logging the list of deleted files on the 'Debug' log level, requiring the user to provide the --verbose flag to see it previously, this list was logged to the 'Info' log level

tests/integration.rs

jyn514 · 2022-12-25T20:28:40Z

Do you mind adding a test showing the new output? Comparing run(sweep(&["--maxsize", "0"])).get_output().stdout to some string would be ideal; I guess it's annoying because it hashes in the name but maybe you can normalize those?

tests/integration.rs

now it requires a target directory to which cargo-sweep will try to perform the cleaning

marcospb19 · 2022-12-26T17:13:31Z

tests/integration.rs

+    let pattern = unindent(&format!(
+        r#"\[DEBUG\] cleaning: "/tmp/{regex_skip}" with remove_older_until_fits
+        \[DEBUG\] size_to_remove: {regex_skip}
+        \[DEBUG\] Sizing: "/tmp/{regex_skip}/debug" with total_disk_space_in_a_profile
+        \[DEBUG\] Hashs by time: \[
+            \(
+                {regex_skip},
+                "{regex_skip}",
+            \),
+        \]
+        \[DEBUG\] cleaning: "/tmp/{regex_skip}/debug" with remove_not_built_with_in_a_profile
+        \[DEBUG\] Successfully removed: "/tmp/{regex_skip}/debug/deps/libsample_project-{regex_skip}.rlib"
+        \[DEBUG\] Successfully removed: "/tmp/{regex_skip}/debug/deps/libsample_project-{regex_skip}.rmeta"
+        \[DEBUG\] Successfully removed: "/tmp/{regex_skip}/debug/deps/sample_project-{regex_skip}.d"
+        \[DEBUG\] Successfully removed: "/tmp/{regex_skip}/debug/.fingerprint/sample-project-{regex_skip}"
+        \[INFO\] Cleaned {regex_skip} from "/tmp/{regex_skip}""#,
+    ));


This does work, but if the test fails, it doesn't give any tips on what changed, I'm not sure if it's a great approach.

For reference, here's the output we are matching:

[DEBUG] cleaning: "/tmp/.tmpXhoLjT" with remove_older_until_fits [DEBUG] size_to_remove: 46978 [DEBUG] Sizing: "/tmp/.tmpXhoLjT/debug" with total_disk_space_in_a_profile [DEBUG] Hashs by time: [ ( 13.926894ms, "261b05c2354104eb", ), ] [DEBUG] cleaning: "/tmp/.tmpXhoLjT/debug" with remove_not_built_with_in_a_profile [DEBUG] Successfully removed: "/tmp/.tmpXhoLjT/debug/deps/libsample_project-261b05c2354104eb.rlib" [DEBUG] Successfully removed: "/tmp/.tmpXhoLjT/debug/deps/libsample_project-261b05c2354104eb.rmeta" [DEBUG] Successfully removed: "/tmp/.tmpXhoLjT/debug/deps/sample_project-261b05c2354104eb.d" [DEBUG] Successfully removed: "/tmp/.tmpXhoLjT/debug/.fingerprint/sample-project-261b05c2354104eb" [INFO] Cleaned 9.43 KiB from "/tmp/.tmpXhoLjT"

I think it's ok; if you run the test with --nocapture it will show the original output.

jyn514 · 2022-12-26T17:33:23Z

tests/integration.rs

+
+    let output = std::str::from_utf8(&assert.get_output().stdout).unwrap();
+
+    let regex_skip = r#".+?"#;


this can just be .* instead of needing to make it optional; and at that point I think it would be simpler to inline it instead of using format.

Aha! A regex Gotcha.

Fun fact: Inserting an ? after a quantifier (*, +, ? or {N..M}) switches
to non-greedy matching, which can be good for performance as it decreases
excessive backtracking from the usual greedy eater.

(It's the same symbol to make stuff optional... why...)

Well, that's a tiny text to match so I'll switch to .+ anyways 👍.

I do think this is a bit less readable from the editor, but the GitHub diff doesn't highlight the format placeholder.

jyn514

Thanks! Happy to merge this with the last nit fixed :)

tests/integration.rs

marcospb19 · 2022-12-26T21:39:06Z

Now it's failing because of Windows paths

"C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\.tmpBWHDFH\\debug\\.fingerprint\\

marcospb19 · 2022-12-26T21:43:45Z

Fixed, if you want I can squash the last (one or two) commits.

jyn514 · 2022-12-26T21:53:33Z

Thanks for sticking with this!

marcospb19 force-pushed the make-default-output-less-noisy branch 2 times, most recently from 68271d3 to ccebf4b Compare December 25, 2022 20:01

marcospb19 marked this pull request as ready for review December 25, 2022 20:01

marcospb19 commented Dec 25, 2022

View reviewed changes

tests/integration.rs Show resolved Hide resolved

marcospb19 force-pushed the make-default-output-less-noisy branch from ccebf4b to af05edb Compare December 25, 2022 20:18

marcospb19 force-pushed the make-default-output-less-noisy branch from af05edb to 701561e Compare December 25, 2022 20:20

marcospb19 added 2 commits December 25, 2022 17:23

make default output less noisy

b5cc645

by logging the list of deleted files on the 'Debug' log level, requiring the user to provide the --verbose flag to see it previously, this list was logged to the 'Info' log level

Show the path of the cleaned project

0e63272

marcospb19 force-pushed the make-default-output-less-noisy branch from 701561e to 48b4d8c Compare December 25, 2022 20:25

marcospb19 commented Dec 25, 2022

View reviewed changes

tests/integration.rs Outdated Show resolved Hide resolved

solve Clippy warnings

9120c29

marcospb19 force-pushed the make-default-output-less-noisy branch from 48b4d8c to 9120c29 Compare December 25, 2022 22:14

marcospb19 marked this pull request as draft December 26, 2022 01:44

marcospb19 commented Dec 26, 2022

View reviewed changes

tests/integration.rs Outdated Show resolved Hide resolved

marcospb19 added 2 commits December 26, 2022 13:50

refactor test function run

f25ec8b

now it requires a target directory to which cargo-sweep will try to perform the cleaning

add test to compare new default output

d78a8eb

marcospb19 force-pushed the make-default-output-less-noisy branch from dcf7008 to d78a8eb Compare December 26, 2022 17:11

marcospb19 marked this pull request as ready for review December 26, 2022 17:12

marcospb19 commented Dec 26, 2022

View reviewed changes

jyn514 reviewed Dec 26, 2022

View reviewed changes

jyn514 approved these changes Dec 26, 2022

View reviewed changes

jyn514 reviewed Dec 26, 2022

View reviewed changes

tests/integration.rs Outdated Show resolved Hide resolved

fix test path regex

5f6cf75

marcospb19 force-pushed the make-default-output-less-noisy branch from 5749886 to 5f6cf75 Compare December 26, 2022 21:11

fix test regex to match windows paths

b6f256b

marcospb19 requested a review from jyn514 December 26, 2022 21:42

jyn514 merged commit fd4dc55 into holmgr:master Dec 26, 2022

marcospb19 deleted the make-default-output-less-noisy branch December 26, 2022 21:54

marcospb19 mentioned this pull request Feb 4, 2023

Update CHANGELOG.md #96

Merged

marcospb19 mentioned this pull request Sep 12, 2023

bump version to 0.7.0 #109

Closed

marcospb19 mentioned this pull request Oct 5, 2023

Bump version to 0.7.0 #121

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make default output less noisy #81

Make default output less noisy #81

marcospb19 commented Dec 25, 2022 •

edited

Loading

marcospb19 commented Dec 25, 2022

marcospb19 commented Dec 25, 2022

marcospb19 commented Dec 25, 2022

marcospb19 commented Dec 25, 2022

jyn514 commented Dec 25, 2022 •

edited

Loading

marcospb19 commented Dec 25, 2022

jyn514 commented Dec 25, 2022

marcospb19 Dec 26, 2022

marcospb19 Dec 26, 2022 •

edited

Loading

jyn514 Dec 26, 2022

jyn514 Dec 26, 2022

marcospb19 Dec 26, 2022

marcospb19 Dec 26, 2022

jyn514 left a comment

marcospb19 commented Dec 26, 2022

marcospb19 commented Dec 26, 2022

jyn514 commented Dec 26, 2022


		let output = std::str::from_utf8(&assert.get_output().stdout).unwrap();

		let regex_skip = r#".+?"#;

Make default output less noisy #81

Make default output less noisy #81

Conversation

marcospb19 commented Dec 25, 2022 • edited Loading

marcospb19 commented Dec 25, 2022

marcospb19 commented Dec 25, 2022

marcospb19 commented Dec 25, 2022

marcospb19 commented Dec 25, 2022

jyn514 commented Dec 25, 2022 • edited Loading

marcospb19 commented Dec 25, 2022

jyn514 commented Dec 25, 2022

marcospb19 Dec 26, 2022

Choose a reason for hiding this comment

marcospb19 Dec 26, 2022 • edited Loading

Choose a reason for hiding this comment

jyn514 Dec 26, 2022

Choose a reason for hiding this comment

jyn514 Dec 26, 2022

Choose a reason for hiding this comment

marcospb19 Dec 26, 2022

Choose a reason for hiding this comment

marcospb19 Dec 26, 2022

Choose a reason for hiding this comment

jyn514 left a comment

Choose a reason for hiding this comment

marcospb19 commented Dec 26, 2022

marcospb19 commented Dec 26, 2022

jyn514 commented Dec 26, 2022

marcospb19 commented Dec 25, 2022 •

edited

Loading

jyn514 commented Dec 25, 2022 •

edited

Loading

marcospb19 Dec 26, 2022 •

edited

Loading