cleanup: fix go_cache cleanup #4778
Conversation
ghost
assigned 
Library/Homebrew/cleanup.rb
Outdated
| @@ -237,6 +237,10 @@ def cleanup_path(path) | |||
| if dry_run? | |||
| puts "Would remove: #{path} (#{path.abv})" | |||
| else | |||
| # Go makes its cache contents read-only to ensure cache integrity, | |||
| # which makes sense but is something we need to undo for cleanup. | |||
| FileUtils.chmod_R 0755, path if path.to_s.include?(cache/"go_cache") | |||
There was a problem hiding this comment.
I think this would fit better above where we check for path.nested_cache?.
|
Made some changes to the way this was written. |
Library/Homebrew/cleanup.rb
Outdated
| @@ -213,6 +219,7 @@ def cleanup_cache(entries = nil) | |||
| entries ||= [cache, cache/"Cask"].select(&:directory?).flat_map(&:children) | |||
|
|
|||
| entries.each do |path| | |||
| FileUtils.chmod_R 0755, path if path.needs_chowning? && !dry_run? | |||
There was a problem hiding this comment.
Does this need to be recursive? Could +r be used instead? Otherwise this may overwrite a decision to e.g. make this group writable.
There was a problem hiding this comment.
The cache wouldn't remove locally unless I ran recursively through everything; you end up with the build cache removed but the module cache being persistent. Which isn't the worst thing in the world, a longer-lasting cache is the most effective way for the Go changes to be useful, but I was planning to achieve that a little more legitimately than simply making it impossible for cleanup to remove without user intervention, heh.
Otherwise this may overwrite a decision to e.g. make this group writable.
Where would that decision come from? I didn't think we especially supported user intervention in cache management?
Library/Homebrew/cleanup.rb
Outdated
| def needs_chowning? | ||
| # Go makes its cache contents read-only to ensure cache integrity, | ||
| # which makes sense but is something we need to undo for cleanup. | ||
| directory? && %w[go_cache].include?(basename.to_s) |
There was a problem hiding this comment.
This doesn't check if it needs chowning or not? Maybe worth checking if it's readable?
There was a problem hiding this comment.
It should always need chowning if we're going to remove it, and the user hasn't manually manipulated the go_cache. With the module cache especially Go will refuse to reuse it if it has been "tampered" with, as far as I can see.
Beyond the Go usage though I guess it's possible a check is worth doing. I'm happy to add one if you think it'll be useful?
There was a problem hiding this comment.
Could just tweak the name; is_go_cache_directory? or similar I think signals what it does a bit more?
There was a problem hiding this comment.
Sure, if you're happy with that it works for me.
I generalised it in case we needed to expand it to other tools/formulae/etc for whatever reason in the future, but we can always handle that in the future rather than me trying to peer into a 🔮, heh.
There was a problem hiding this comment.
Aye, can refactor if and when we need it for more 👍
|
Change pushed. |
|
Perfect, thanks @DomT4! |
|
Thanks Mike! |
brew stylewith your changes locally?brew testswith your changes locally?Oops 🙃. It's possible people want me to write this a different way, which I'm open to.