Let there be color #14465

Closed
wants to merge 1 commit into
from

Projects

None yet

9 participants

@samueljohn

Color and Highlighting!

Color-code the brew update and brew ls to quickly see which formulae, have changed and which are outdated.
Do this only on a Tty, so your scripts are fine! I think this highlighting and mentioning of brew upgrade also solves the flaw that people do think they are up-to-date when they did a brew update.

brew deps <formula> and brew uses <formula> now also marks installed dependencies and brew deps <formula additionally prints missing deps in red.
(Wich brew deps also indicating unlinked deps that are not keg_only).

I introduce a class Rack to simplify things. This has been discussed throughout the different versions of this PR. I tried to keep the changes at a minimum level. I will make another pull request in another branch where this new Rack class is used throughout the homebrew code-base to get rid of repeated code blocks dealing with verification of racks and kegs.

Wanna test/see in action?

cd $(brew --prefix)/Library/Homebrew/tests
ruby test_updater.rb -- --verbose
ruby test_utils.rb -- --verbose
ruby test_rack.rb

See commit message for details.

@samueljohn

Please review, if you have time. No hurry, it's cosmetics.

  • Are you ready to accept a few ms slower brew list but having color on the outdated ones?
  • Do you like the colors? (green => there is an update, blue => other formulae modifications)
  • First time I wrote a unit test. Check it out :-) (I needed to fix few things there)
@adamv

No colors on brew list, we need to keep it machine parsable.

@samueljohn

@adamv There are no colors if not tty?, so brew list >> mybrews.txt works.

What I do to test this:

  • Checkout into a branch
  • create a new other temp branch and switch to it
  • git reset --hard 12345 <- some older revision
  • Cherry pick the commit with the colored update
  • brew update --rebase
@samueljohn

brew list | grep wget also works.

@staticfloat staticfloat and 1 other commented on an outdated diff Aug 26, 2012
Library/Homebrew/cmd/update.rb
ohai title
- puts_columns formula
+ # Formulae which are in installed, too, get a * and color.
+ puts_columns formulae, installed, {:blue => installed}
+ end
+ # This is a hack, because git does not know about version updates
+ if key == :M
+ # Out of all modified formulae, highlight the outdated ones
+ outdated = outdated_brews.map { |b| b.name }
+ unless outdated.empty?
+ modified_outdated = formulae.select { |f| outdated.include?(f) }
+ unless modified_outdated.empty?
+ ohai title + " (with software update)"
@staticfloat
staticfloat Aug 26, 2012

I think saying "version update" is clearer than "software update".

@samueljohn
samueljohn Aug 26, 2012

That sounds better than my wording. Thanks.

@jacknagel jacknagel and 1 other commented on an outdated diff Aug 26, 2012
Library/Homebrew/utils.rb
@@ -96,6 +96,12 @@ def self.system cmd, *args
Process.wait
$?.success?
end
+
+ def list_installed_formulae
+ # Just a plain list of things in the Cellar. No caching here!
+ ENV['CLICOLOR'] = nil
+ `ls #{HOMEBREW_CELLAR}`.split if HOMEBREW_CELLAR.exist?
+ end
@jacknagel
jacknagel Aug 26, 2012

I think this already exists as Formula.installed.

@samueljohn
samueljohn Aug 26, 2012

Ah good. I searched for this and found it in the list command. Formula.installed is a much better name.

@mxcl
Homebrew member

Indeed, color is already removed if the stdout isn't a terminal.

@mxcl mxcl commented on an outdated diff Aug 28, 2012
Library/Homebrew/utils.rb
@@ -129,22 +135,68 @@ def curl *args
safe_system curl, *args
end
-def puts_columns items, star_items=[]
+# Print items in columns on tty or else just a list.
+# For items _also_ in star_items, a "*" is appended.
+# The color_items is a hash: {:green => ['item1', 'item2'], :red => ['item3']}
+# The keys are send to Tty.
+def puts_columns items, star_items=[], color_items={}, sort=false
@mxcl
mxcl Aug 28, 2012

I'm glad this is now in Ruby. Good work. I'd say though that the final three parameters should use an options-style hash:

puts_columns food, {:stars => stars, :colors => colors, :sort => true }

Also IMO sort doesn't need to be handled in the function ;) That is something the API-consumer can do if they choose to before passing items.

@mxcl
Homebrew member

A reasonably thorough overview makes me think this is great stuff.

All I'd say, (as I won't test it, it's tedious testing update code, and I have too many branches), is make sure it works with tap-updates!

@samueljohn

Hey all, thanks for the feed back!

The puts_columns (now pure ruby) is tested quite thoroughly in the unit test.

I'll make sure to test some more tap updates (but the update and git interaction code has not been touched - basically just the dump method got a lot of changes)

@samueljohn

@adamv has recently worked on the list command. I have to see how to get basic color in brew list now.

@samueljohn

Well, I'll only show color for brew update and not for brew list, because the latter accepts all flags, which ls accepts because it basically just executes ls -- and I can't rewrite that in ruby to add color-coding of outdated formulae.

@samueljohn

Almost.
Still taps are listed as homebrew/dupes/grep which is != the formula name grep.

@samueljohn

brew update works but color and stars are not yet shown for tapped formulae. I am on it.

@jacknagel

Would still like to see Formula.installed reused instead of a new utils method that does the same.

@samueljohn

@jacknagel I wasn't aware of that because I thought formula.installed? was the same thing. But a few lines further down in formula.rb there is indeed self.installed. Ok, will update for that :-)

Also I should use Formula::tap I guess. And I'd need Formula::tapped?, so I can color the updates for tapped formulae, easier.
Further, @jacknagel do you agree that Report::select_formula etc. should use Formula objects instead of Pathname and juggling around with these to find out if a formula is a tap or not? Look at Report::select_formula for example.

@jacknagel jacknagel commented on an outdated diff Sep 2, 2012
Library/Homebrew/test/test_utils.rb
class UtilTests < Test::Unit::TestCase
+ sio, stdout_tmp = nil
@jacknagel
jacknagel Sep 2, 2012

These locals are in a different scope than the ivars you reference below, so this declaration isn't really doing anything (ivars can be used without being initialized, and are nil by default).

@jacknagel jacknagel commented on an outdated diff Sep 2, 2012
Library/Homebrew/cmd/update.rb
@@ -125,14 +125,17 @@ def `(cmd)
class Report < Hash
+ hint = "" # will be shown in dump
@jacknagel
jacknagel Sep 2, 2012

My previous comment applies here also.

@jacknagel

@samueljohn re: select_formula, I think so.

Eventually I'd like to see the places in the code that are regex'ing strings replaced with a more OO solution. But that is neither here nor there.

@mxcl
Homebrew member

Well, I'll only show color for brew update and not for brew list, because the latter accepts all flags, which ls accepts because it basically just executes ls -- and I can't rewrite that in ruby to add color-coding of outdated formulae.

I consider the “feature” that brew ls does this to be just something I did “because”. I don't use it and doubt anyone does.

Colorizing brew ls in a useful way is a way better feature.

@samueljohn samueljohn commented on an outdated diff Sep 3, 2012
Library/Homebrew/cmd/list.rb
@@ -9,10 +11,13 @@ def list
return unless HOMEBREW_CELLAR.exist?
if ARGV.include? '--versions'
- list_versions
+ name_and_versions = list_versions
+ outdated = outdated_brews.map{ |b| b.name }
+ outdated_with_version = name_and_versions.select{ |f| outdated.include?(f[:name]) }
+ puts_columns name_and_versions.map{ |f| "#{f[:name]} #{f[:versions]*' '}" },
+ :colors=>{ :green=>outdated_with_version.map{ |f| "#{f[:name]} #{f[:versions]*' '}" } }
@samueljohn
samueljohn Sep 3, 2012

puts_columns just does string compare, so the items itself and the list of to-color-in-green items, both, need to have the same string. What would be coo but not yet possible:

outdated = Formula.outdated.map{ |f| "#{f.name} #{f.versions}" }
installed = Formula.installed.map{ |f| "#{f.name} #{f.versions}" }
puts_columns installed, :colors=>{:green=>outdated}
@samueljohn samueljohn commented on an outdated diff Sep 3, 2012
Library/Homebrew/cmd/list.rb
elsif ARGV.named.empty?
- ENV['CLICOLOR'] = nil
- exec 'ls', *ARGV.options_only << HOMEBREW_CELLAR
+ puts_columns Formula.installed, :colors=>{:green=>outdated_brews}
@samueljohn
samueljohn Sep 3, 2012

@jacknagel it reads so nice with Formula.installed :-)

@samueljohn samueljohn commented on an outdated diff Sep 3, 2012
Library/Homebrew/cmd/outdated.rb
end
end
+ # To be consistent with brew update and brew list, outdated are tinted green:
+ puts_columns formulae, :colors=>{:green=>formulae}
@samueljohn
samueljohn Sep 3, 2012

puts_columns handles the case when $stdout.tty?.

@samueljohn

I tested this now in with taps and without taps. It's stable now. Thanks for all your feedback - very welcome!

Please share your thoughts about:

  • The wording for: "Modified Formula (with new version)"
  • The color-sheme: blue=modified, green=with new version
  • Due to the color, the headings for the update-output do not stand out any more so strong as before. Perhaps introduce a new empty line before each heading (but the very first)?

@jacknagel I won't touch Report::select_formula since it must work on git's output (even for deleted formula - which is something Formula.factory can't).

@mxcl Okay brew ls is back in color (only color right now is "green" for "there is a new version"). brew outdated is colored the same way to be consistent, even if it does not make much sense here, since all outdated formulae are installed and therefore highlighted green.

@jacknagel

I'm not sold on coloring the brew outdated output since they will all be the same; also, I had it written so that it would output results as they were yielded by outdated_brews, but running it through puts_columns defeats that.

@jacknagel

We should probably add something like Keg.installed for use in brew list, as using Formula.installed will mean that kegs without a corresponding formulae are omitted.

@mxcl mxcl and 1 other commented on an outdated diff Sep 3, 2012
Library/Homebrew/cmd/update.rb
- puts_columns formula
+ # Formulae which are in installed, too, get a * and color.
+ puts_columns formulae, :star=>installed, :colors=>{:blue => installed}
+ if formulae.any? { |f| installed.include?(f) }
+ @hint << "*) Installed. See changes: #{Tty.blue}brew log <formula>#{Tty.reset} "
+ end
+ end
+ # This is a hack, because git does not know about version updates
+ if key == :M
+ # Out of all modified formulae, highlight the outdated ones
+ outdated = outdated_brews.map{ |f| f.tap == 'mxcl/master' ? f.name : "#{f.tap}/#{f.name}" }
+ unless outdated.empty?
+ modified_outdated = formulae.select { |f| outdated.include?(f) }
+ unless modified_outdated.empty?
+ ohai title + " (with new version)"
+ puts_columns modified_outdated, :star=>installed, :colors=>{:green => installed}
@mxcl
mxcl Sep 3, 2012

I'd prefer:

puts_columns modified_outdated, :star => installed, :green => installed
@mxcl
mxcl Sep 3, 2012

You don't have to. I'm just a stickler for prettiness in code. If it's hard or whatever it is not important.

@samueljohn
samueljohn Sep 3, 2012

Simplicity and beauty are always important.

@mxcl
Homebrew member

We should probably add something like Keg.installed for use in brew list, as using Formula.installed will mean that kegs without a corresponding formulae are omitted.

Yes, better be part of this patch.

I'm not sold on coloring the brew outdated output since they will all be the same; also, I had it written so that it would output results as they were yielded by outdated_brews, but running it through puts_columns defeats that.

I agree, and column output doesn't have much benefit here, since usually the list is short.

@samueljohn

I see. Okay, I'll try to add Keg.installed, change the signature of puts_columns to avoid :colors, and revert the outdated_brews thing. No color there.

@samueljohn

Ok, I found time to update according to your comments and rebase this.

  • brew outdated is reverted to a version without color.
  • The API call for puts_columns is now as @mxcl suggested.
  • brew deps and brew uses adds a star to the formulae installed and show a mini-legend.
  • All color and legend is only shown if writing to a Tty.
  • A new Keg.installed returns a list of names (strings) of software in there.
    • Must be a dictionary
    • The dictionary must contain other dictionaries (e.g. "bin")

@jacknagel the best would be that Keg.installed returns Keg instances but then it's not clear which version (always the newest?). Therefore, right now it returns just the names.

I tested all the commands on my system.
Testing brew update is possible by:

  • git reset --hard <some-older-ref> (but must be newer than c1cc99b)
  • brew install squid (or other things that got changes since then)
  • Cherry-picking my PR. I think brew pull 14465 is ok.
  • brew update
  • 👀 at the colors.
@jacknagel

I thought about Keg.installed a little more, and it seems to me that the problem is that we aren't really asking for kegs; what we are really asking for is a list of racks, i.e. the immediate subdirectories of HOMEBREW_CELLAR. But the concept of a "rack" is just a convention and hasn't been reified into a class [yet], so the best we can do at this time is probably:

class Keg
  def self.installed
    HOMEBREW_CELLAR.subdirs.map do |rack|
      rack.subdirs.map { |d| Keg.new(d) }.find { |keg| keg.linked? }
    end.compact
  end
end

I chose keg.linked? as the heuristic here, but I don't know that it is the best one. That there is even a question about the criteria shows me that I suggested the wrong name ("installed") for this. Alas, I can't think of a better one, because again, we aren't actually asking for kegs, but the names of kegs' parent directories.

So we could implement it this way now, but it will probably just get deprecated in the future because the name doesn't match what it is doing.

(Note also that this method can be very slow on a cold cache if you have a lot of kegs, as it involves a ton of filesystem activity. But there isn't really a way around that.)

Ideally, we would have something like

class Rack < Pathname
  # an initializer similar to Keg that prevents Racks from
  # being instantiated on "illegal" pathnames
  # ...

  def self.all
    HOMEBREW_CELLAR.subdirs.map { |d| Rack.new(d) }
  end

  def fname
    self.basename.to_s
  end
end

and then one could just say Rack.all.map(&:fname) to get the list we want. This can come as a later refactoring, and I do think it's worth doing; there are a number of places in the code where we are manually constructing this list.

@samueljohn

@jacknagel thanks for your long post. Much appreciated.

Really, your arguing speaks very clear that everything else than a Rack class is a fail. Keg.installed feels wrong.

Your code for the Rack could be a good first start. I just like to use this and you can improve the initializer (and other useful methods like returning the linked Keg, the versions etc.) later on.

Should I go for it and remove Keg.installed?

@jacknagel

If you're up to it, definitely!

@samueljohn

@jacknagel I think there is a bug in extend/pathname.rb that causes all children or subdirs to be the same extended type (e.g. Keg or Rack) and their initialize checks moan (correctly).

cairokeg = Keg.new('/homebrew/Cellar/cairo/1.12.2')
=> #<Keg:/homebrew/Cellar/cairo/1.12.2>
>> cairokeg.subdirs
RuntimeError: /homebrew/Cellar/cairo/1.12.2/.DS_Store is not a valid keg
    from /homebrew/Library/Homebrew/keg.rb:6:in `initialize'
    from /System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/pathname.rb:695:in `new'
    from /System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/pathname.rb:695:in `children'
    from /System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/pathname.rb:692:in `foreach'
    from /System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/pathname.rb:692:in `children'
    from /homebrew/Library/Homebrew/extend/pathname.rb:246:in `subdirs'
    from (irb):7

You see the checks in initialize fails. The children should be of the general type Pathname and not Keg.

@samueljohn

@jacknagel Perhaps, it's a feature of Ruby's Pathname that Pathname.children and some other methods creates new instances via self.class.new but self.class can be a Keg or in my case a Rack. The initialize of Keg then makes no sense.

At this point I don't know a good ruby-ish solution and could need a little hint.

@jacknagel

The guards in Keg's constructor are there to prevent instantiating a Keg on directories that aren't kegs, but unfortunately as you noticed this causes a lot of what should be harmless operations to raise exceptions. I had to work around this in Keg#basename by wrapping it in a Pathname: Pathname.new(self).basename. I don't know of any more comprehensive workaround, though.

Interestingly, Keg#parent returns a Pathname.

@samueljohn

Your workaround should be added Pathname.children in homebrew's extension to Pathname.

@samueljohn

Please note, now I implemented the Rack. That was actually the bigger task than implementing the color thing. Therefore, I renamed this PR and re-wrote the description text. The color stuff and puts_coloums is still in here because it uses the new classes and methods.

Consider this work-in-progress.

I intend to write all kind of tests to give you guys more trust in my changes - which are quite deep at some places.
Any idea of a good coverage tool for Ruby (similar to coverage.py html output )

@mistydemeo

We have coverage written into the rake test tasks - gem install rcov, then you can generate coverage in /usr/local/Library/Homebrew/tests via rake rcov.

@samueljohn

Cool, thanks @mistydemeo (also your new test_keg.rb tests do all pass with my branch).

Not so cool: On Xcode-only:

Error installing rcov:
ERROR: Failed to build gem native extension.
/System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/bin/ruby extconf.rb
mkmf.rb can't find header files for ruby at /System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/ruby.h

Looks like the same shit we had with python's build system (distutils/pip) a few months ago.

@jacknagel

Haven't looked very closely yet but I would suggest creating an alternate constructor rather than passing booleans; something like Keg.new!, the bang suggesting that it can raise exceptions.

@samueljohn
@jacknagel

Most people do, but it's not. For example, rails uses bangs to mean "this method will raise an exception if it doesn't do what it says it does" and there are non-bang equivalents that return nil (create/create!, save/save!).

Also there are many non-bang methods that mutate the receiver.

@MikeMcQuaid
Homebrew member

+1 what @jacknagel said.

@samueljohn
@jacknagel

I just want to make sure we make every effort to come up with a design that avoids passing around booleans; it's kind of an anti-pattern in an OO system.

@jacknagel

Perhaps a better approach would be to forgo inheritance altogether and use a combination of composition and delegation.

Something like

class Keg
  def initialize(path)
    @path = path
  end

  def self.new!(path)
    # do any validations that can raise errors here
    new(path)
  end

  # delegate unknown methods to @path
  def method_missing(name, *args, &block)
    @path.send(name, *args, &block)
  end
end

and similar for Rack.

@jacknagel

Just to add to the above, I think inheriting from a core or library type is also an anti-pattern, especially when said type has a very large API (which almost all Ruby classes do). You end up exposing a huge API but you only have control over a small subset of its behavior.

I think the strong preconditions that define a Keg make it impossible for it to be a true subtype of Pathname, but we can probably get closer by avoiding inheritance here. Basically the current Keg class is spitting in Barbara Liskov's face.

@jacknagel jacknagel closed this Oct 24, 2012
@jacknagel jacknagel reopened this Oct 24, 2012
@samueljohn

Hmmm ... don't blame me for extending Pathname that is how it is currently.

A Keg (and Rack) is a Pathname with certain extra methods and validation, so why not inheriting Pathname? But I am fine with a missing_method, too, if the others agree. Would that solve our current bug that Keg.children returns Kegs instead of Pathnames?

@jacknagel

Hmmm ... don't blame me for extending Pathname that is how it is currently.

Oh, I'm not! I'm just saying that the current design has probably been pushed to its limits.

A Keg (and Rack) is a Pathname with certain extra methods and validation, so why not inheriting Pathname?

I think I explained it pretty well in my previous post, but to summarize, the problems we are experiencing now (i.e. being unable to use some pathname methods because the Keg constructor raises exceptions) are a direct consequence of using inheritance: we don't have control over a large number of the methods that keg exposes because they are inherited but not overridden.

Would that solve our current bug that Keg.children returns Kegs instead of Pathnames?

Yep.

@samueljohn

Ok, ok ... I think I got your point. I think. :-)
However, I'm not going to change the Pathname extension. That would be too much for this PR. I will make MeaningfulPathname not inherit from Pathname but do the method_missing thing to delegate to Pathname - if you accept that.

@jacknagel

I'm not suggesting that we make any changes to the pathname extension; I just think that Keg and Rack shouldn't inherit from anything directly. If I were designing it from the ground up I would try to make them naked classes and just extract any common functionality into a module.

The most important thing to me is that we don't end up with API methods that take a boolean that changes what they do. At the very least that's an indication that you really have two methods, but probably also points to a wrong design decision at another level or incorrect assumptions about how the system should behave.

@samueljohn
@MikeMcQuaid
Homebrew member

I agree with @jacknagel again here.

@samueljohn

From the existing codebase I isolated that we need two different types of checking for kegs and racks: One is what I call valid? and the other is real?. A rack is valid if its parent is the HOMEBREW_CELLAR, has no spaces in name and so on. Its real? if it exists, is valid and is non-empty. So "real" is stronger.

When creating a new rack, only validity is checked on default, to be able to create a new instance and then call mkpath on it or something. But at some places we also create a new rack instance that has to be real. The same is true for keg.

For this reason I introduced the hated booleans raise_err, :validate and :realcheck that I am now trying to get rid off. I looks nicer without them (will push later) in almost all cases. However one question remains:

Do you consider making Rack#real? and Keg#real? return self instead of true bad design?
So basically they would raise an exception or return self.
It would enable me to do Rack.new('git').real?.children instead of r = Rack.new('git'); if r.real?; r.children end. And I do not need Rack.new!(x) any more, because its the same as Rack.new(x).real?
The following snipped aims to show the usefulness:

class << Formula
  def best_keg
    (keg.real? rescue nil) or
    (rack.newest_keg rescue nil)
  end
end

In compare to:

class << Formula
  def best_keg
    k = keg
    begin
      k.real?  # may raise an exception
      k = nil unless k.real?
    rescue
      k = nil
    end
    k or (rack.newest_keg rescue nil)
  end
end
@mistydemeo

I don't think interrogatory methods should ever return anything other than booleans. The latter code is longer but more readable and I think would be easier for someone not familiar with Rack and Keg to parse.

@samueljohn

@mistydemeo if it's the naming ... I can change that to realcheck or realchecked instead of real?
Rack.new(x).realchecked reads better.

@mistydemeo

So I say this without having looked at the code yet, but - is there a route here where we can avoid rescuing and muting exceptions this liberally? I understand it's a consequence of our inheriting from Pathname at this point of course. I'm concerned about using exceptions in basic logic. You incur a nontrivial performance hit that way (like in any language).

@samueljohn

So I say this without having looked at the code yet, but - is there a route here where we can avoid rescuing and muting exceptions this liberally?
[...]
You incur a nontrivial performance hit that way (like in any language).

Well I had the raise_err=true (its actually still the code of this PR at the moment, but I am locally going to change it) to avoid exactly this. It's independent from inheriting Pathname. I did what @jacknagel suggested, so no longer inheriting Pathname but delegating.
However, the return value of real? and that it raises exceptions that are sometimes muted is still to be questioned.
I am not sure if this performance hit is really so bad. I benchmarked a similar situation once in Python and it was negligible compared to file system operations like exist? or directory? (as long you don't do it in your innermost matrix-multiplication loop ^^)

@jacknagel

I suggest that we do this in smaller increments, i.e. start by simply introducing just a Rack class with the minimal API that is needed to complete the original goal (coloring the brew update output) and see where that gets us. I am 100% in favor of making our domain abstractions more concrete but TBH I don't even know where to start when looking at this code because there are so many new abstractions and named exceptions.

Once we are using that in production we can decide what the next steps should be.

@samueljohn

Yes, thats probably the best route to go, otherwise I fear you will reject the whole thing, that would be a pity.

I will introduce Rack, MeaningfulPathname and the and the exceptions for Rack - just so that brew update can be colored. Once we have that in shape, I am going to add the other changes to Keg, Formula and all the other places where the new puts_columns implementation is useful (e.g. to show the installed and unlinked formulae on brew deps x) in separate PRs.

@mxcl
Homebrew member

OK.

So, I know a lot of work has gone on here. But adding color to some commands and refactoring most of the underpinnings of brew are different things right?

I agree our codebase sucks, but I'm rightly nervous to refactor it considering our test coverage sucks.

I'd much rather fix everything and then start brew 2 properly. I already put in a proper test framework in the brew2 repo.

@mxcl
Homebrew member

Regarding the abstractions being discussed, seems good to me.

@mxcl
Homebrew member

PS I really want the color patch!

@samueljohn

I'll make it neat and clean: Only the color & highlight update plus the new Rack class that is only used in one place by my new code. Don't worry, I'll remove all other usages of the new class, so it cannot break stuff. Also I undo all the other changes and leave them for another PR or for discussion.

I didn't intent to change so much. When I coded this I couldn't resist to use the new Rack in order to try if the API is useful. And I found it useful and making sense.

@samueljohn

@jacknagel I am not so confident that composition and delegation via method_missing as you suggested is going to be a good solution. I implemented that but found that methods defined in Object are not delegated, which leads to bugs like:

  • to_s gives a different result than for Pathname
  • kind_of? has to be overwritten, too (and who knows what else? is_a? etc.?)
  • a == b returns false for the same reasons. Pathname overwrites this.

I think I am still in favor of inheritance.

@jacknagel

Overriding to_s and == is standard practice, and we don't want to be in the business of overriding kind_of?/is_a? anyway.

I still disagree that using inheritance is in any way preferable. The solution presented here uses a very complex API in order to overcome problems that are direct consequences of inheriting from Pathname. There are at least three essentially different paths through the system, but I suspect the cyclomatic complexity is actually much greater than that.

This pull request needs to pull its tentacles out of unrelated code. I just want us to be able to use Rack.all to get a list of Racks, and use those objects to get a list of their kegs. The rest is questionable at this point (is Formula#prefix really a keg if that version isn't installed?), and I think it would be a mistake to make far-reaching changes without experimenting with these abstractions on a smaller scale first.

@jacknagel

Let me suggest a second non-inheritance solution: expose "path" as a property of Keg. High-level keg operations should be implemented on Keg directly (basically the current Keg interface, plus comparisons and to_s), and code that absolutely needs to deal with pathnames can access (but not mutate!) the path property (though these should be kept to a minimum, and common ones given a higher-level abstraction on Keg directly).

@samueljohn

This pull request needs to pull its tentacles out of unrelated code.

Will do. Can't yet push, because it's inconsistent right now. Tomorrow is good.

Okay, if overwriting == and to_s is standard, I am fine with that.

@mxcl
Homebrew member

IMO inheritance is not correct.

Also I want us to move away from Pathname. I have experimented with this: https://github.com/mxcl/brew2/blob/master/lib/brew/string%2Bfile.rb

Code comes out very clean with string+file.

@samueljohn

Done. Check the commit message.

Rack does not inherit from Pathname - not even indirectly. I did what @jacknagel suggested and so Rack inherits from MeaningfulPathname only. Usage of Rack is very limited as you demanded and I removed my special Exceptions (they are just RuntimeErrors now) but I'd like to introduce them/discuss them in a separate PR.

@samueljohn

This is how brew update would now look like. (see Homebrew/tests/test_updater.rb)
brew update sneak-peek

And this is brew deps python (with an unlinked non-keg-only dep in red and a missing dep)
brew deps being more helpful

The hints are adaptive and only shown if needed.

@samueljohn

And the things I removed from this PR are moved to a branch samueljohn@colorupdate...racknroll

@jacknagel

This is moving in the right direction, but I still have some reservations.

I think MeaningfulPathname should go. There isn't any non-trivial behavior being inherited (i.e., subclasses are expected to re-implement its method interface).

The API that is being exposed is unwieldy. Some of it ends up being used in your racknroll branch, so it should be moved to that branch and be reviewed later.

But mostly I think there is too much code churn here for what we are gaining. Maybe it is my fault for overstating the importance of reification. It doesn't have to be done all at once, and doing it incrementally means we find places where we made mistakes and can fix them before they spread to other parts of the system.

This is the entirety of the behavior I needed in order to make your changes to brew update work:

https://gist.github.com/4041497

Note that I didn't test the other commands, and there may be some corner cases. But I wanted to give a POC for a simple solution.

@samueljohn

MeaningfulPathname should go -- @jacknagel

For this PR, sure, I could move things into Rack directly. Also I could remove Rack.for and one or two of the (right now) unused methods. No problem with that. In my opinion, Keg should not inherit from Pathname (because then we still have the bug that Keg#children sucks, because it returns instances of Keg) but it should inherit from MeaningfulPathname. Well, this is out of the scope for this PR.

There isn't any non-trivial behavior being inherited. [...]
Note that I didn't test the other commands, and there may be some corner cases.

Not so sure here. Only valid? and real? are overwritten by Rack. The other "trivial" methods are necessary to make it work like a Pathname (joining, comparison etc.) and these are not used by brew update, so you did not experience these problems in your test.
But of course this class makes no sense if Rack is the only MeaningfulPathname subclass.

@jacknagel

In my opinion, Keg should not inherit from Pathname

I agree, though my opinion is that neither Keg nor Rack should inherit from anything, and the long-term goal should be to fix the API so that external code doesn't have to do any raw pathname manipulations. My overall point is that implementation inheritance should only be used when there is real, tangible benefits to sharing that implementation. If the "meat" of the class is being overridden, there is no real benefit. I don't think sharing to_s and == is a reason to have a shared superclass. If the duplication is annoying, factor the duplicated methods out into a module and include it in both classes.

I guess I still don't know what problems real? and valid? are meant to solve, and why they can't just be rolled up into an alternate constructor. I get the allure of defensive programming, but at a certain point we're just making assertions based on a list of everything that could possibly go wrong. IMO is a mistake if any external code ever has to use these methods, so they should not be exposed. If a core assumption is violated, Homebrew should just abort and let the user file an issue, because there is probably a bug elsewhere that caused that assumption to be violated.

This also why I am not a fan of introducing a bunch of named exceptions here; the problems that they reify represent bugs in other parts of the system and not conditions that are expected to occur with some regularity. IOW, we expect the user to see things like UnsatisfiedRequirements, BuildError, and ChecksumMismatchError, so it makes sense for them to be real classes with special behavior.

@mxcl
Homebrew member

I have tried to think up sensible ways to represent this stuff in the past, several times. I think the amount of discussion here makes it clear that it is hard.

The abstractions you inevitably try to build hide important details and lead to complexity that doesn't really help anything. But not having abstractions leads to the kind of messy code we already have.

This kind of PR is useful for discussion, but ultimately what we need it branches of experimentation and iteration without pressure.

Please split the PR in two, the feaure that we desire and the refactoring suggestion that we can discuss without pressure to commit.

And we will not be committing a refactoring without much consideration, because we break Homebrew enough already, and because I'm not a fan of pushing refactors that turn out to be not better-enough, but you still had to learn all the new code and all the new side-effects and all the new gotchas.

@samueljohn

Ok, so I'll remove MeaningfulPathname and clean up Rack a bit (more). Hope this makes @jacknagel and @mxcl happy :-)

I think we agreed already that homebrew would benefit from the "Rack" abstraction - though a simplified one. Right now, we use Keg.installed when we really mean Rack.all and some other hacks (e.g. using formulae too often but not all racks need a corresponding formula or there could even be multiple formula (think taps) for the same rack.

Except a few corners, I don't think Homebrews codebase is broken. Perhaps a bit too much was put into the Pathname extension (uninstall_info for example.)

I guess I still don't know what problems real? and valid? are meant to solve

I think I can make new do the valid? checks and new! do the real? checks.
The reasoning is: "valid" is a Rack, if the place and name are right but it does possibly not yet exist.
A Rack is real? if it is valid and the directory exists and it has subdirs (kegs).
Currently, these checks are done at several places, several times and inconsistently.

[...] not a fan of introducing a bunch of named exceptions [...]

We can just use RuntimeError instead, but I thought it might be better style to rescue a specific exception than just to mute every RuntimeError (which might hide bugs).
Example: One wants to list all Racks but there is at least one empty directory. I would not want to bubble up the exception but just mute the empty (and therefore not real?) rack.

@mxcl I already split this PR into two (#14465 (comment)).
The feature that we desire gets a mess to implement without a good way to "talk" about a Rack, it's Kegs and which one is the "active", i.e. the keg that is currently linked. I think what you don't like right now is the MeaningfulPathname (and perhaps the Exceptions). I will remove that and slim down Rack to a minimum. Then, this PR is only about the coloring for update, list, deps and fixes two related bugs. Doing this without a Rack (as initially attempted) makes me go insane :-)

@jacknagel

We can just use RuntimeError instead, but I thought it might be better style to rescue a specific exception than just to mute every RuntimeError (which might hide bugs).

There are two good reasons to use named exceptions: (a) it is a condition that is meant to be exposed to the user, or (b) there is a sane way to recover from the error and continue.

For example, if we end up with something in the cellar that is not a directory, it's either because a formula wen't haywire or there is a bug somewhere in Homebrew. In either case, it is unlikely that an average user will be able to fix the problem without guidance. Contrast this with the exceptions that are defined in exceptions.rb: they largely represent things that are meant to be exposed to the user as part of normal usage, like attempting to install an already installed formula. This is (a).

(b) is things like CurlDownloadStrategyError. There is a sane way to recover from this: try a mirror.

Example: One wants to list all Racks but there is at least one empty directory. I would not want to bubble up the exception but just mute the empty (and therefore not real?) rack.

This is kind of a variation on (b), though in this case we don't need to name anything. This is what the <some action that might raise but we don't care> rescue nil construct is for. We're saying "yeah, there is a nonzero chance that this action will raise an error but I don't care because I can accomplish what I'm trying to do anyway".

So my point is that it doesn't have to be named at all: just raise a string. If the problem is that some entity is not a directory, just raise "foo is not a directory". This doesn't qualify for (a) because it is not expected to occur in normal usage, and it doesn't qualify for (b) because there isn't a sane way to recover from it. Callers that don't care will rescue nil, and callers that do care will let the exception propagate so that the bug can be reported and fixed.

@samueljohn

I am almost 99% with you @jacknagel.

rescue nil construct is for.

The 1% is here. If the the caught rescued exception is different from what you'd expect here to fail and it is indeed a serious bug, you won't notice. Using a more specific exception is good . At least this is what I learned in Python. Perhaps Ruby is different. I suspect people use <foo> rescue nil because it is so short compared to begin <foo>; rescue MyEx; nil end.

And I actually had this once recently: There was a bug in Pathname#subdirs but it did not bubble up. All I noticed at some point was that one rack was missing :-)

@samueljohn
  • Rebased and checked all tests that ship with homebrew (and my own tests for Rack).
  • Removed the MeaningfulPathname as @mxcl demands (separate PR maybe later, it's in my fork)
  • Removed all unused methods from Rack. So what remains is the minimum to make things work "like" a Pathname with the exception of active_keg, newest_keg and active_or_newest_keg which I find super-handy. But I can remove these three, too.

You may want to run ruby test_updater.rb -- --verbose in the test dir to see how the updater would look like.

@samueljohn

rebased on current master. No conflicts.

@samueljohn

what now?

@MikeMcQuaid
Homebrew member

@samueljohn Rebase this again and I'll merge unless @jacknagel objects.

@jacknagel

I'd like to review it again; I'll try to do it in the next day or so.

@jacknagel

Sorry for the delay here.

As I've said, I'm not sold on the design of the Rack class or the proposed API, so I'm not comfortable merging this in it's current state. I think it is important enough, and will eventually be relied on by enough other code, that it deserves a more careful design process.

Let's slim it down to the minimum needed to do the coloring in brew update, et. al.

This means that there isn't any need to worry about making Rack a drop-in replacement for a pathname at this time, it should be a regular class with only the methods needed to make the coloring work. There shouldn't be any diff to formula.rb, keg.rb, etc., for the time being.

@jacknagel

Also, let's not touch brew deps for the time being, there are still some lingering issues from the recent deps changes that might affect the semantics there. We can add the formatting when those are resolved.

@samueljohn

Alright.

@samueljohn samueljohn color in brew update & list. Core: Rack class
Check out `brew list --color`!

- New class Rack.
- Rack.all lists all racks installed to the cellar.
- brew update now uses color (only on tty)
- brew list uses color if --color is given (and on tty)
- brew update giving hint to run `brew upgrade` if needed
- brew update giving hint to run `brew log <formula>` if needed
- Color code for modified and outdated matching the hint.
- Installed formulae have a "*" in `brew {update,uses}.
- Rewrote the puts_column in pure ruby. Yay, no more /usr/bin/pr!
- Typo fix in testing_env "cellar" -> "Cellar".
- Added tests to test_utils.rb.
  Run with `ruby test_utils.rb -- --verbose` to see all the colors.
- See the updater in color: `ruby test_updater.rb -- --verbose`
- Extended test_updater.rb to show the dump if `-- --verbose`.
- Fixed bug in extend/pathname.rb#subdirs (so that Keg.subdirs works)
b1abaa2
@samueljohn

I hope to have addressed your concerns, @jacknagel.

  • Rebased to master. Should apply cleanly.
  • No changes any more to formula.rb and deps.rb and keg.rb.
  • Rack class is minimal now.
  • Rack is no longer a Pathname. It has a @path though.
  • brew list --color (because coloring takes an extra second)
  • Check ruby test_rack.rb, ruby test_utils.rb -- --verbose and ruby test_updater.rb -- --verbose
  • All color and additional output is only displayed if on a Tty.
  • pathname.rb is only touched to fix a bug.
@MikeMcQuaid MikeMcQuaid commented on the diff Feb 28, 2013
Library/Contributions/manpages/brew.1.md
@@ -195,7 +195,7 @@ Note that these flags should only appear after a command.
be linked or which would be deleted by `brew link --overwrite`, but will not
actually link or delete any files.
- * `ls, list [--unbrewed] [--versions]` [<formulae>]:
+ * `ls, list [--unbrewed] [--versions] [--color]` [<formulae>]:
@MikeMcQuaid
MikeMcQuaid Feb 28, 2013

This can probably default on? Otherwise perhaps allow a env var to to enable it globally.

@samueljohn
samueljohn Mar 1, 2013

I am absolutely in favor to have this on by default. Could you and/or @jacknagel test this on a Mac with a HDD (no SSD), can you tell me the speed difference and if you a willing to accept that?

@MikeMcQuaid
MikeMcQuaid Mar 2, 2013

I only have SSDs I'm afraid.

@MikeMcQuaid
Homebrew member

Seems good to me. When @jacknagel is good, let's merge.

@jacknagel

Thanks for slimming this down. I will review/test and merge soon.

@camillol camillol commented on the diff Mar 1, 2013
Library/Contributions/manpages/brew.1.md
@@ -208,6 +208,9 @@ Note that these flags should only appear after a command.
If `--versions` is passed, show the version number for installed formulae,
or only the specified formulae if <formulae> are given.
+ If `--color` is passed, highlight, formulae that are outdated, force-
+ linked keg_only, or unlinked.
+
@camillol
camillol Mar 1, 2013

Bad grammar here.

@samueljohn
samueljohn Mar 1, 2013

If --color is passed, highlight outdated formulae, or force-linked keg_only formulae.

That better? Suggestions welcome.

@camillol

I get the Cellar/Rack/Keg hierarchy from a beer point of view. However, what we really have on disk is a Cellar/Formula/Version hierarchy, and we already have a Formula class. In fact, we end up with duplicated functionality between Rack and Formula. Furthermore, knowledge of the filesystem hierarchy is spread out over Rack, Formula and Keg now (and probably other places).

I think the right way to look at this is that Formula represents a software specification, Keg represents an installed software directory (prefix), and the knowledge of how Kegs are laid out on disk is an implementation detail. Having a Rack class makes this implementation detail (the fact that there is currently one layer between the Cellar and Kegs) part of the API, while bizarrely ignoring as much as possible the fact that this middle layer represents formulas.

IMHO it would be better to avoid introducing a Rack concept, and instead concentrate the knowledge of how Kegs are laid out in the file system into a Cellar module. Or it may be possible to cleanly split it between Keg and Formula.

@samueljohn

Well, @camillol I disagree. Let me defend the Rack concept here.

At first sight, one might come to the conclusion that Rack and Formula should be the same class.
However, on a closer look and as you pointed out already, the Formula class is tied to a specific <formula>.rb file (which can come from different taps, so there is a m-to-n correspondence between Keg and Formula) and getting the currently installed and linked version is difficult and ambiguous. More in this direction: A rack that is installed, but the formula file has been removed. The idea to force "rack into formula" breaks down.

Looking at all the code snippets in the current code-base that do duplicate stuff around figuring out which keg to select if several are installed. Which is the linked one, etc.?
See for example:
https://github.com/mxcl/homebrew/blob/master/Library/Homebrew/extend/ARGV.rb#L16-L60
and how that could look when the Rack would know which is it's newest or "active" keg:
https://github.com/samueljohn/homebrew/blob/c9f334c2d3080f4a79b5db46c1d38e023b713498/Library/Homebrew/extend/ARGV.rb#L24-L46
(Note: That branch of mine is outdated but was mainly to test out the Rack concept and shows different places where the codebase could be simplified by Rack.)

Having said that, I think Rack would even be the more versatile and cleaner concept than Keg, which is the root of the difficulties we have: We often need the keg that is currently linked, or if none, the keg defined by the name and version of the <formula>.rb or the newest version of all kegs for one Rack. Doing all this just in-place plus handling of empty dirs and .DS_Strore is how we do it now.
For example Formula#rack currently constructs the prefix by using the version as defined in the corresponding .rb. This makes it hard to reason about the currently installed Rack, if it is outdated, linked etc.:
https://github.com/mxcl/homebrew/blob/master/Library/Homebrew/build.rb#L159-L172
https://github.com/mxcl/homebrew/blob/master/Library/Homebrew/cmd/missing.rb#L5-L15
and so forth...

To conclude: Rack would indeed hide and centralize a lot of implementation details (like handling DIY installations, .DS_Store files, Keg, validation and getting the "linked or newest" keg). Though not in the version of this pull request.

We don't have a Cellar class right now. So not sure if introducing Cellar instead of Rack gains much. But I am not opposed to that idea.

@samueljohn samueljohn referenced this pull request Mar 22, 2013
Closed

Freeze installed formulae. #18515

6 of 6 tasks complete
@jacknagel

FYI I am working on integrating this. Lots of conflicts in the rebase but I'll deal with them since I've been stubborn about getting it merged ;)

@camillol

@samueljohn re: formula, the rack stuff would not go anywhere in Library/Formula, it would be shared in Library/Homebrew/formula.rb. It could be implemented using class methods, or you could have a factory method that instantiates a generic MissingFormula subclass if the formula file is missing. It depends on the API we want to have.

I agree with collecting all the stuff that deals with the Cellar directory layout in one place. The problem with Rack is that it turns the on-disk layout from an implementation detail into an API concept, so that while some code may be consolidate, knowledge of the layout gets spread to even more places (because it's now part of the API).

The API is also bad in other ways. We now have to require keg, rack and formula, and it's not clear which part should do what. Things like linked_but_keg_only should really be a one-line select call, but since the functionality is split between Rack and Formula you have to write a multi-line ugliness to pull it out, which is then wrapped into a method with a strangely narrow purpose. In a language like Ruby, having to have methods like linked_but_keg_only or unlinked_but_not_keg_only is a code smell. valid? is a very strange method to have: why would you create an invalid object in the first place? You should raise an exception from initialize instead. And you do, but then why expose a valid? method? And why call it again in real?? And why in the world would that method return true when the rack is real, but raise an exception instead of returning false when it's not real? A question mark method than can only ever return true is another obvious code smell.

Basically, gathering the cellar layout handling code in one place is good. The methods that you've written are good, I assume, in terms of correctly doing what they try to do. But the API is bad. And that's not a strange result if you proceed by mopping up some hairy code from all over the place (which, again, is a noble goal!), instead of designing the API first.

Normally, a commit that adds color in the terminal and revises a fundamental API of the project would be requested to be split into two commits. And you seem to have done that, when @mxcl requested it. But still, if we put Rack in there, I'm afraid it's going to quickly get adopted de facto. Now, I see that @jacknagel had his own misgivings about the API, so he may be making more changes than he's letting on. But, even the if the design is changed, the colorization should be truly split from the API change.

@jacknagel

My first cut at this is: https://github.com/jacknagel/homebrew/compare/color

I split up the original commit so it should be easier to follow.

@camillol, I share your concerns about the API. I think a lot of the confusing parts are the result of trying to anticipate and overcompensating for future problems. However I do want some kind of abstraction here. I ended up replacing the Rack implementation with my own simpler version from some months ago.

There is an empty commit at the tip with my TODOs.

@samueljohn

@jacknagel I am fine with you taking the best of this PR and making the worst of it better. And surely, you code looks better than mine - no doubt about that. I especially like the mapper thing you did in list.rb and the Columnizer class.
Glad we agree on the Rack class.

@camillol Thanks for your thoughts. You will perhaps be surprised - but we agree (except for the Rack class)!
I basically collected the the code that was done ad-hoc several times in different places and put it into methods (therefore the valid? and real?). However, now I think that would need a larger refactoring afford to clean the code base.

@jacknagel

Note that I haven't stopped working on this, it's pretty much ready to go, but I want to spend a little more time addressing the performance issues that surround loading all formulae.

@samueljohn

@jacknagel let me know how I may assist you (testing or something).

@jacknagel

I've kept my branch rebased: https://github.com/jacknagel/homebrew/compare/color

The patches themselves are fine, but I'm still very concerned about performance. The specific performance issues are not caused by any code in this pull request, but it did make them much more obvious. I've made a lot of small optimizations but there's a lot of "dead air" while all formulae are loaded. I'm hoping that cleaning up Formula.factory will open up some doors for speeding this up.

@jacknagel

For instance, running brew readall with a cold cache often takes 10x as long as with a warm cache, and while for us the Homebrew tree is usually in cache, I suspect for most users it is not, and running brew update will suddenly become dog slow.

@samueljohn

Alright, I will start to use your branch on my production system to see how it goes. Don't hurry. I am fine waiting and was just checking my back-log. So glad you took over here. I will patiently wait for Formula.factory rework.

@samueljohn

I tested your branch and it works for me. A minor glitch is perhaps that brew list --color --pinned shows the legend as if brew list --color is used, so it may in fact show that unlinked are red but there is no --pinned unlinked keg.

Any idea about the speed of reading all formulae? Perhaps caching the formula versions and writing them together with a file timestamp into a single temp file.

@adamv adamv commented on the diff Nov 5, 2013
Library/Homebrew/utils.rb
else
+ # Don't fuck up scripts and shell piping:
@adamv
adamv Nov 5, 2013

Eh, let's not add more swears

@samueljohn

If at all, we will use the code Jack refactored. I guess it's without swears. But I can't swear.

@MikeMcQuaid
Homebrew member

@jacknagel Any thoughts/news on this?

@samueljohn samueljohn closed this Feb 9, 2014
@samueljohn samueljohn deleted the samueljohn:colorupdate branch Mar 27, 2014
@xu-cheng xu-cheng locked and limited conversation to collaborators Feb 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.