search

holmgr / cargo-sweep

A cargo subcommand for cleaning up unused build files generated by Cargo
MIT License
693 stars 31 forks source link

Make the recursive option of cargo-sweep recurse past Cargo directories #78

Closed bes closed 1 year ago

bes commented 1 year ago

When passing the -r (recursive) option to cargo-sweep, it is supposed to be recursive over the subdirectories of the root path it is given (whether implicit or explicit). cargo-sweep erronously assumed that a Cargo project could not contain unrelated subprojects.

This patch lets cargo-sweep keep recursing past cargo roots.

The drawback of this patch is that cargo-sweep will need to recurse more, and as such becomes a bit slower to run.

Fixes https://github.com/holmgr/cargo-sweep/issues/66

bes commented 1 year ago

@jyn514 I finally got some time to write an integration test, and I think it came out OK.

But I have to say, the state of the integration.rs file is not very good. I tried to improve it, but I have some... feelings.

Tests should be easy to follow step-by-step by reading the test function, but folks have tried to be too clever with the code. Deduplication, DRY, and utility functions that assume too much about how a test works will end up hurting tests as much as they help improve regular code.

I had to start by documenting a lot of the code that I didn't understand and then patch my integration test on top of that.

Furthermore, the empty_project_output didn't run locally for me. Either I broke it somehow, or the regexp was written by someone with another type of machine than me where the file system outputs files and directories in a different order. We'll see once someone approves my PR for testing. My recommendation would be to fix that test to be platform agnostic (unless I broke it, of course).

bes commented 1 year ago

Yeah I just confirmed that the test empty_project_output is broken on the master branch on my machine as well (2019 intel mac)

bes commented 1 year ago

The regex requires this order

        \[DEBUG\] Successfully removed: ".+libsample_project-.+\.rlib"
        \[DEBUG\] Successfully removed: ".+libsample_project-.+\.rmeta"
        \[DEBUG\] Successfully removed: ".+sample_project-.+\.d"
        \[DEBUG\] Successfully removed: ".+.fingerprint.+sample-project-.+"

But my machine outputs this order

[DEBUG] Successfully removed: "/var/folders/g8/qgn3gbf514b_p34n_jc_1_lc0000gn/T/.tmpKdJQnA/debug/deps/sample_project-f27e05aa5c13ed9b.d"
[DEBUG] Successfully removed: "/var/folders/g8/qgn3gbf514b_p34n_jc_1_lc0000gn/T/.tmpKdJQnA/debug/deps/libsample_project-f27e05aa5c13ed9b.rlib"
[DEBUG] Successfully removed: "/var/folders/g8/qgn3gbf514b_p34n_jc_1_lc0000gn/T/.tmpKdJQnA/debug/deps/libsample_project-f27e05aa5c13ed9b.rmeta"
[DEBUG] Successfully removed: "/var/folders/g8/qgn3gbf514b_p34n_jc_1_lc0000gn/T/.tmpKdJQnA/debug/.fingerprint/sample-project-f27e05aa5c13ed9b"
bes commented 1 year ago

I managed to fix it by changing the regexp to this monstrosity. My guess is that regex isn't the right tool for the job, though.

        (\[DEBUG\] Successfully removed: (".+libsample_project-.+\.rlib"|".+libsample_project-.+\.rmeta"|".+sample_project-.+\.d"|".+.fingerprint.+sample-project-.+")\n{0,1}){4}
bes commented 1 year ago

Updated

bes commented 1 year ago

Looks like there is a "bug" in format_bytes(bytes: u64), it doesn't agree with the values from the human-size crate?

human-size expects bytes to be suffixed B but format_bytes outputs the string "bytes".

bes commented 1 year ago

The source changes seem ok, but I had a few comments about the test changes - most of them are non-blocking except the fact that the test isn't actually effective.

Fixed 👍

jyn514 commented 1 year ago

Thanks for sticking with this!