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

bes · 2022-12-17T16:42:38Z

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 #66

jyn514

Can you please add a test for the new behavior? You may need to add the project from #66 into tests, see tests/integration.rs for some existing code you can use as an example.

Otherwise this looks fine to me :) the CI failures are unrelated and should be fixed if you rebase over master.

bes · 2023-01-28T22:05:02Z

@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 · 2023-01-28T22:17:53Z

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 · 2023-01-28T22:19:28Z

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 · 2023-01-28T22:41:52Z

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}

tests/integration.rs

marcospb19

LGTM, sorry my last PR gave you some conflicts xD.

These should be easy to fix because you only changed one line at main.rs, and there aren't many changes on integration.rs, here's the diff, just in case it helps.

EDIT: oh and overwrite your PR's regex pattern over mine please.

bes · 2023-02-01T05:47:54Z

Updated

bes · 2023-02-01T22:07:42Z

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".

jyn514

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.

tests/integration.rs

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 holmgr#66

The empty_project_output test doesn't have the same traversing/logging order on macos as it does on whatever os/machine it was written for. This commit tries to improve the situation somewhat by making the test runnable on macos as well, while sacrificing readability and test accuracy. An alternative fix would be to not use regex for the test at all.

There is an edge case where the `cargo test` invocation can be run with `CARGO_TARGET_DIR` set, which needs to be handled by the recursive_multiple_root_workspaces test since it assumes that `CARGO_TARGET_DIR` isn't set.

This commit improves the recursive_multiple_root_workspaces by applying some review comments: * Deduplicating the cargo command by making the cargo function take an absolute path instead of a relative one. * Removing an unnecessary CARGO_INCREMENTAL=0 environment variable * Renaming the temporary workspace directory variable

…spaces This commit changes the recursive_multiple_root_workspaces test by adding assertions to make sure that the actions of cargo-sweep were actually effective in cleaning nested root workspaces.

bes · 2023-02-04T20:48:45Z

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

Thanks! This looks really good :) I just have a couple nits.

tests/integration.rs

* Remove a few comments that are no longer valid. * Improve the understanding of the final assertions by adding a more detailed explanation.

jyn514 · 2023-02-05T22:20:27Z

Thanks for sticking with this!

jyn514 reviewed Dec 25, 2022

View reviewed changes

jyn514 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 25, 2022

bes force-pushed the cargo-sweep-actually-recursive branch 2 times, most recently from 2dd47ac to 1ff2a84 Compare January 28, 2023 22:00

bes force-pushed the cargo-sweep-actually-recursive branch from 1ff2a84 to 84fc0f9 Compare January 28, 2023 22:06

bes force-pushed the cargo-sweep-actually-recursive branch from 603d59c to 8c4ca2b Compare January 28, 2023 22:48

marcospb19 reviewed Jan 30, 2023

View reviewed changes

tests/integration.rs Show resolved Hide resolved

marcospb19 reviewed Jan 30, 2023

View reviewed changes

tests/integration.rs Outdated Show resolved Hide resolved

bes force-pushed the cargo-sweep-actually-recursive branch from ba43ad6 to 8b75297 Compare January 30, 2023 08:44

marcospb19 approved these changes Feb 1, 2023

View reviewed changes

bes force-pushed the cargo-sweep-actually-recursive branch 2 times, most recently from e542c06 to 48e462e Compare February 1, 2023 05:47

bes force-pushed the cargo-sweep-actually-recursive branch from 48e462e to bf1b8a2 Compare February 1, 2023 07:46

jyn514 added S-waiting-on-review Status: Awaiting review from a maintainer and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 2, 2023

jyn514 self-assigned this Feb 2, 2023

jyn514 reviewed Feb 2, 2023

View reviewed changes

tests/integration.rs Show resolved Hide resolved

tests/integration.rs Outdated Show resolved Hide resolved

tests/integration.rs Outdated Show resolved Hide resolved

tests/integration.rs Outdated Show resolved Hide resolved

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer labels Feb 2, 2023

bes added 5 commits February 2, 2023 08:49

Add clarifying comments to the recursive_multiple_root_workspaces test

e94aaec

Make cargo-sweep output parselable by human-size

a13db04

bes force-pushed the cargo-sweep-actually-recursive branch from 25a73db to c88fa94 Compare February 2, 2023 07:49

marcospb19 mentioned this pull request Feb 4, 2023

Only show toolchain list once when using --installed #88

Merged

Assert that cargo-sweep was effective in recursive_multiple_root_work…

442d75b

…spaces This commit changes the recursive_multiple_root_workspaces test by adding assertions to make sure that the actions of cargo-sweep were actually effective in cleaning nested root workspaces.

jyn514 approved these changes Feb 5, 2023

View reviewed changes

tests/integration.rs Outdated Show resolved Hide resolved

tests/integration.rs Outdated Show resolved Hide resolved

tests/integration.rs Outdated Show resolved Hide resolved

tests/integration.rs Show resolved Hide resolved

Clean up recursive_multiple_root_workspaces test documentation

d5a72d6

* Remove a few comments that are no longer valid. * Improve the understanding of the final assertions by adding a more detailed explanation.

jyn514 merged commit dbb2133 into holmgr:master Feb 5, 2023

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 the recursive option of cargo-sweep recurse past Cargo directories #78

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

bes commented Dec 17, 2022

jyn514 left a comment

bes commented Jan 28, 2023

bes commented Jan 28, 2023

bes commented Jan 28, 2023

bes commented Jan 28, 2023

marcospb19 left a comment •

edited

Loading

bes commented Feb 1, 2023

bes commented Feb 1, 2023

jyn514 left a comment

bes commented Feb 4, 2023

jyn514 left a comment

jyn514 commented Feb 5, 2023 •

edited

Loading

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

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

Conversation

bes commented Dec 17, 2022

jyn514 left a comment

Choose a reason for hiding this comment

bes commented Jan 28, 2023

bes commented Jan 28, 2023

bes commented Jan 28, 2023

bes commented Jan 28, 2023

marcospb19 left a comment • edited Loading

Choose a reason for hiding this comment

bes commented Feb 1, 2023

bes commented Feb 1, 2023

jyn514 left a comment

Choose a reason for hiding this comment

bes commented Feb 4, 2023

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 commented Feb 5, 2023 • edited Loading

marcospb19 left a comment •

edited

Loading

jyn514 commented Feb 5, 2023 •

edited

Loading