holmgr / cargo-sweep

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

Improve performance and accuracy by baking in `targets` structure knowledge? #6

Closed epage closed 5 years ago

epage commented 5 years ago

targets has a basic structure. Once you have the directory for a target, a lot of dependencies are kept in hashed folders. I suspect those hashes include the version and feature flags.

The problem is this becomes more brittle because it becomes dependent on how cargo works.

holmgr commented 5 years ago

This would be a nice change, but it would perhaps also involve some restructuring since we would have to defer the actual deletion until we are certain that all files are old enough and then delete the entire directory.

I also do get the issue of being sensitive to changes of how cargo/rustc stores the files in the target directory, although I would guess that it is pretty stable by this point.

Eh2406 commented 5 years ago

I would guess that it is pretty stable by this point.

It is pretty stable, in that It has not changed in a very long time. It is an implementation detail, in that the Cargo team would be comfortable changing it. There has been some buzz about adding a folder to make it clear what is from crates.io (and unlikely to change) vs other sources (more likely to change), or a folder for each compiler version. If there are changes to the structure that would make it easier for cargo-sweep to work well, please let us know at the Cargo repo.

Eh2406 commented 5 years ago

While investigating https://github.com/holmgr/cargo-sweep/issues/2#issuecomment-447607139, I learned some things about how the folder is structured. As I understand it, the folders come in sets. For example, if there is a folder atty-5ea146758e503e24 in /target/debug/.fingerprint then there may be a corresponding folder in /build, /incremental, /native and files with that name in /deps and /. I think it would be most reliable to keep/remove all of these files as a set. I believe that cargo looks at the files in /.fingerprint for every transitive dependency on every bild. So the atime from there may be the most reliable. I also think that it will not be a big PR to have cargo add a timestamp file while it is looking at the fingerprint. Edit: Verified. compare_old_fingerprint reads the /.fingerprint/lib-... file, and is called in prepare_target, witch is called in compile, which calls itself recursively on the deps.

Meanwhile, the fingerprint already includes the version of rust needed for the files, sow next time I have a sec I will work on a version that lets you cargo +stable build && cargo +nightly build && rustup toolchain remove nightly && cargo sweep --installed and it will remove all the files related to nightly leaving you with the same files as if you had only run cargo +stable build. (Witch a lot of peepal need to clean the cache folder on CI when nightly updates. like https://github.com/rust-lang/crates.io/pull/1578)

holmgr commented 5 years ago

Hmm, looks interesting. However, are we sure that .fingerprint is actually touched when doing a compile every time, i.e even if we compile twice in a row?

If you have time on work on that feature that cleans the cache folder on CI on nightly updates I would really appreciate it!

Eh2406 commented 5 years ago

I have a branch that builds on #14 to remove sets of files based on the fingerprints atime. I have been testing it with

cargo +nightly build // to make sure deps are around
cargo +stable build // to make sure deps are around
cargo sweep -s
cargo +stable build  // make sure no deps are rebuilt
cargo sweep -f
cargo +stable build  // make sure no deps are rebuilt
cargo +nightly build  // make sure deps are rebuilt, I.E. we cleaned something above. 

and so far it has worked on everything I have tried. (well I am on a system that does not support atime. so I had to use a local copy of Cargo with https://github.com/rust-lang/cargo/pull/6477, but that just touches a file instead of reading it.)

Eh2406 commented 5 years ago

Move to close?

holmgr commented 5 years ago

👍