Cleanup: Continue on error removing keg #2374

joshka · 2017-03-21T09:17:53Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew tests with your changes locally?

MikeMcQuaid · 2017-03-22T09:24:03Z

Library/Homebrew/cleanup.rb

-    def self.cleanup_cellar
-      Formula.installed.each do |formula|
-        cleanup_formula formula
+    def self.cleanup_cellar(formulae = Formula.installed)


I don't think we need a default argument here.

The use case of this is to have the command not duplicate the continue on fail logic. See https://github.com/Homebrew/brew/pull/2374/files#diff-8cc42f61c29a8f960250394cbc3319c4L24

Gotcha, I misread, thanks.

MikeMcQuaid · 2017-03-22T09:24:44Z

Library/Homebrew/cleanup.rb

+    def self.cleanup_cellar(formulae = Formula.installed)
+      unremovable_kegs = []
+      formulae.each do |formula|
+        cleanup_formula formula, unremovable_kegs


Feels weird to pass unremovable_kegs as an argument; what about an instance variable instead?

I agree, I wasn't sure whether we needed to maintain the previous behavior of failing when cleanup_formula fails for individual calls, so I did it this way. I'll change this to use an instance variable.

Using an instance variable also allows us to move the failure messaging to the command rather than the library functions. We can also continue onto the cache, logs, lockfiles etc when formula cleanup fails.

MikeMcQuaid · 2017-03-22T09:25:26Z

Library/Homebrew/cleanup.rb

+
+      return if unremovable_kegs.empty?
+      ofail <<-EOS.undent
+        Could not cleanup old kegs


Could not cleanup old kegs! Fix your permissions on:

MikeMcQuaid · 2017-03-22T09:26:01Z

Library/Homebrew/cleanup.rb

+      return if unremovable_kegs.empty?
+      ofail <<-EOS.undent
+        Could not cleanup old kegs
+        Check you have permission to remove the following folders in the cellar:


Can roll this line into the previous one.

MikeMcQuaid · 2017-03-22T09:26:16Z

Library/Homebrew/cleanup.rb

+      ofail <<-EOS.undent
+        Could not cleanup old kegs
+        Check you have permission to remove the following folders in the cellar:
+          #{unremovable_kegs.join " "}


.join "\n "

joshka · 2017-03-23T04:32:39Z

Broken test seems like it's a connection issue. Not sure if this will work for me or if it's locked to maintainers, but here goes nothing:
@BrewTestBot test this please

joshka · 2017-03-23T05:03:03Z

Thanks @ilovezfs

ilovezfs · 2017-03-23T05:03:33Z

yw

MikeMcQuaid

One final nit then good to 🚢. Nice work!

MikeMcQuaid · 2017-03-23T08:56:05Z

Library/Homebrew/cleanup.rb

+
+    def self.unremovable_kegs
+      @unremovable_kegs
+    end


I don't think these helper methods are needed; just write directly to the instance variable.

It's used externally from the Cleanup command. I wrote it this way for consistency with the two disk_cleanup_size methods rather than a knowledge of idiomatic ruby (I'm a ruby n00b coming from a mostly C# background). I could reasonably drop the add_unremovable_keg method, but I think the accessor needs to remain.

Is the following more idiomatic in ruby?

def self.unremovable_kegs @unremovable_kegs ||= [] end def cleanup_keg(keg) # ... unremovable_kegs << keg

or:

@unremovable_kegs = [] attr_accessor :unremovable_kegs def cleanup_keg(keg) # ... unremovable_kegs << keg

I went with the first option based on not finding any instances of attr_reader through Homebrew where the class only has class methods.

joshka · 2017-03-23T10:34:59Z

Library/Homebrew/cmd/cleanup.rb

+  def report_unremovable_kegs
+    ofail <<-EOS.undent
+      Could not cleanup old kegs! Fix your permissions on:
+        #{unremovable_kegs.join "\n  "}


This needs a Cleanup. in front

joshka · 2017-03-23T11:04:16Z

A couple of quick questions:
Do you prefer commits to be rebased prior to merge?
Is your preferred style guide the github one?

It might be best to update https://github.com/Homebrew/brew/blob/master/CONTRIBUTING.md with the answers to these questions as it would help others, instead of just me.

joshka · 2017-03-23T11:20:45Z

One more question. This class only has class methods, should it be a module? Some of the ruby style guides I'm reading suggest this approach, others do not.

MikeMcQuaid · 2017-03-26T18:55:28Z

Do you prefer commits to be rebased prior to merge?

We don't mind either way on Homebrew/brew as we use merge commits anyway.

Is your preferred style guide the github one?

Yeh although our Rubocop does most of the enforcing too.

It might be best to update https://github.com/Homebrew/brew/blob/master/CONTRIBUTING.md with the answers to these questions as it would help others, instead of just me.

We generally rely on tooling and CI rather than documentation as it's easier to keep up to date.

This class only has class methods, should it be a module?

Yeh, that may make sense and could use module_function on it. Feel free to add to this PR?

MikeMcQuaid · 2017-03-30T19:10:12Z

@joshka Travis CI got upset. Can you force push here to kick it off again? Thanks!

Fixes Homebrew#2355 Create unremovable_kegs instance var Check cellar cleanup failure after full cleanup completes Use module_function in Homebrew::Cleanup as we never instantiate the class

joshka · 2017-03-31T06:51:36Z

I wasn't sure if there was an easy way to force push without making a change, so instead I squashed the commits and rewrote the commit messages. I made no other changes other than this.

MikeMcQuaid · 2017-03-31T08:43:53Z

Great work here (again) @joshka!

MikeMcQuaid requested changes Mar 22, 2017

View reviewed changes

joshka force-pushed the cleanup-command-continue-on-error branch 2 times, most recently from ca19913 to 7181bd4 Compare March 23, 2017 03:06

ilovezfs closed this Mar 23, 2017

ilovezfs reopened this Mar 23, 2017

MikeMcQuaid requested changes Mar 23, 2017

View reviewed changes

joshka commented Mar 23, 2017

View reviewed changes

MikeMcQuaid approved these changes Mar 26, 2017

View reviewed changes

Brew cleanup: Continue on error removing keg

48fdd16

Fixes Homebrew#2355 Create unremovable_kegs instance var Check cellar cleanup failure after full cleanup completes Use module_function in Homebrew::Cleanup as we never instantiate the class

joshka force-pushed the cleanup-command-continue-on-error branch from 18bba9f to 48fdd16 Compare March 31, 2017 06:48

MikeMcQuaid merged commit 80b39bb into Homebrew:master Mar 31, 2017

Homebrew locked and limited conversation to collaborators May 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup: Continue on error removing keg #2374

Cleanup: Continue on error removing keg #2374

joshka commented Mar 21, 2017 •

edited

MikeMcQuaid Mar 22, 2017

joshka Mar 23, 2017

MikeMcQuaid Mar 23, 2017

MikeMcQuaid Mar 22, 2017

joshka Mar 23, 2017

joshka Mar 23, 2017

MikeMcQuaid Mar 22, 2017

MikeMcQuaid Mar 22, 2017

MikeMcQuaid Mar 22, 2017

joshka commented Mar 23, 2017

joshka commented Mar 23, 2017

ilovezfs commented Mar 23, 2017

MikeMcQuaid left a comment

MikeMcQuaid Mar 23, 2017

joshka Mar 23, 2017

joshka Mar 23, 2017 •

edited

joshka Mar 23, 2017

joshka commented Mar 23, 2017 •

edited

joshka commented Mar 23, 2017

MikeMcQuaid commented Mar 26, 2017

MikeMcQuaid commented Mar 30, 2017

joshka commented Mar 31, 2017

MikeMcQuaid commented Mar 31, 2017

Cleanup: Continue on error removing keg #2374

Cleanup: Continue on error removing keg #2374

Conversation

joshka commented Mar 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshka commented Mar 23, 2017

joshka commented Mar 23, 2017

ilovezfs commented Mar 23, 2017

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshka Mar 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshka commented Mar 23, 2017 • edited

joshka commented Mar 23, 2017

MikeMcQuaid commented Mar 26, 2017

MikeMcQuaid commented Mar 30, 2017

joshka commented Mar 31, 2017

MikeMcQuaid commented Mar 31, 2017

joshka commented Mar 21, 2017 •

edited

joshka Mar 23, 2017 •

edited

joshka commented Mar 23, 2017 •

edited