-
Notifications
You must be signed in to change notification settings - Fork 762
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
Add a garbage collection mechanism to the CLI #1217
Conversation
crates/puffin-cache/src/lib.rs
Outdated
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This phase is pretty bad (tightly coupled to the cache, but awkwardly so in that it doesn't read the manifest files that point to the latest entry, it just looks for the most recent directory and deletes all the others).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to not read the manifest files?
I think being tightly coupled here is okay, since this is the puffin-cache
crate after-all. :)
(I do think our cache representation is a little leaky, but that's a problem for another day.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One reason is that the manifest files and readers are all in puffin-distribution
(which depends on cache crate), but this code is in the cache crate itself :(
It might be necessary though. Removing the "wrong" directories here will break the cache, which is really bad.
1b2531c
to
b4525b8
Compare
I'll also add some tests for this. Parts of it are really tedious to test, but it's probably important. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I can see how testing this would be annoying. Maybe one way is to just explicitly build cache directories and then run prune
and assert the expected result. But then your tests are tightly coupled with the cache representation.
crates/puffin-cache/src/lib.rs
Outdated
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to not read the manifest files?
I think being tightly coupled here is okay, since this is the puffin-cache
crate after-all. :)
(I do think our cache representation is a little leaky, but that's a problem for another day.)
5707e88
to
61d2e4c
Compare
61d2e4c
to
f426b3c
Compare
I'm merging this for now without "Old source distribution builds (i.e., we have a more recent version)." That's more complex, and was delaying it, but this is already useful. |
I'm not sure why |
Summary
Detects unused cache entries, which can come in a few forms:
archive-v0
, but not symlinked-to from anywhere in the cache).Closes #1059.