add command brew cask cleanup
#2759
Conversation
|
|
||
| def self.remove_old_downloads | ||
| ohai "Removing old downloads" | ||
| seven_days = Time.now - (60 * 60 * 24 * 7) |
There was a problem hiding this comment.
I don't agree with old being seven days. How about we raise this to at least 10 days? Thoughts @phinze @vitorgalvao @fanquake
There was a problem hiding this comment.
I have no strong opinion about a specific cutoff time and am happy to change this PR to 10 days.
But note, that cutoff only affects users who explicitly issue brew cask clean. Arguably, those users would find zero to be the cutoff of least surprise, which I also believe is more consistent with Homebrew. Though I stopped following the Homebrew source at a certain point.
My main purpose is to get the tracker symlinks in there, which then enables us to iterate on any type of cleanup behavior/interface going forward.
There was a problem hiding this comment.
Yeah I agree the least surprising thing to a user explicitly running brew cask clean would probably be removal of all downloads.
But no strong opinion on the cutoff length of the v1 solution here.
I would say that if we have a cutoff, that we should mention it in the output, like: "Removing cached downloads older than N days".
|
Similarly, I have no strong opinion regarding cutoff time (I always remove the cache directory after cleanup, through an alias). I agree with doing the same as homebrew (to match expectations, as @rolandwalker posited), but I'm fine either way. |
|
Then is only a question of interface. Let's provide an interface which accommodates all usecases: that's what interfaces are for. How about:
|
| def self.remove_dead_symlinks | ||
| ohai "Removing dead symlinks" | ||
| HOMEBREW_CACHE_CASKS.children.select(&:symlink?).each do |symlink| | ||
| unless symlink.exist? |
There was a problem hiding this comment.
Is Pathname#exist? what you want here?
I think you might want to resolve the symlink with Pathname#realpath then check if that exists:
unless symlink.realpath.exist?There was a problem hiding this comment.
It may be a weird interface, but symlink.exist? does correctly check for bad symlinks.
symlink.realpath raises an exception, because the real path isn't there.
symlink.readlink.exist? is even worse, as I learned in #2670, because if you don't wrap that in a Dir.chdir(symlink.dirname) do ... end, relative symlinks won't raise an exception, but will give the wrong answer.
There might be yet another method that works there if you want greater code clarity, but I don't know what it is.
There was a problem hiding this comment.
Hey cool!
If you checked that it does The Right Thing (tm), then .exist? actually does seem like the most natural interface to me.
Good on ya, ruby stdlib! 😄
|
Yeah I like the compromise with default immediate and |
plus new option --outdated
|
Revised to use the compromise interface with a 10-day cutoff, improved tests, and documented in the man page. |
add command `brew cask cleanup`
It came to my attention that Homebrew's
brew cleanupis not reliably cleaning up cached downloads from homebrew-cask.There is no easy way to solve that for old downloads (users must figure out that the cache directory is huge and clean it out manually).
This PR addresses downloads going forward, by making a tracker symlink to all files downloaded by homebrew-cask, and adding a new command
brew cask cleanupwhich chases the tracker symlinks and deletes cached downloads older than 7 days.