Create proxy classes for "partial" X11 dependencies

When a formula's dependency tree contains more than one X11 dependency,
they are de-duplicated by comparing the min_version attribute. However,
this can result in broken dependency trees if one of the X11Dependency
objects was actually specified as e.g. `:libpng`.

In practice, this only matters when one or more of the dependencies has
additional metadata that makes it distinct from the rest, i.e. an
:optional or :recommended tag.

To combat this, make these special, "partial" X11 dependencies instances
of different classes so that they are not de-duped.

It will still be necessary, at the time when requirements are expanded
by the installer, to de-duplicate any remaining X11 dependencies after
applying the optional/recommended filters in order to avoid duplicated
modifications to the environment (as ENV.x11 is not idempotent).

c.f. Homebrew/homebrew#17369.

Library/Homebrew/dependency_collector.rb

@@ -77,24 +77,18 @@ private
     when :autoconf, :automake, :bsdmake, :libtool
       # Xcode no longer provides autotools or some other build tools
       Dependency.new(spec.to_s, tag) unless MacOS::Xcode.provides_autotools?
-    when :libpng, :freetype, :pixman, :fontconfig, :cairo
+    when *X11Dependency::Proxy::PACKAGES
       if MacOS.version >= :mountain_lion
         Dependency.new(spec.to_s, tag)
       else
-        X11Dependency.new(spec.to_s, tag)
+        X11Dependency::Proxy.for(spec.to_s, tag)
       end
-    when :x11
-      X11Dependency.new(spec.to_s, tag)
-    when :xcode
-      XcodeDependency.new(tag)
-    when :mysql
-      MysqlInstalled.new(tag)
-    when :postgresql
-      PostgresqlInstalled.new(tag)
-    when :tex
-      TeXInstalled.new(tag)
-    when :clt
-      CLTDependency.new(tag)
+    when :x11        then X11Dependency.new(spec.to_s, tag)
+    when :xcode      then XcodeDependency.new(tag)
+    when :mysql      then MysqlInstalled.new(tag)
+    when :postgresql then PostgresqlInstalled.new(tag)
+    when :tex        then TeXInstalled.new(tag)
+    when :clt        then CLTDependency.new(tag)
     else
       raise "Unsupported special dependency #{spec}"
     end

Library/Homebrew/requirement.rb

@@ -46,11 +46,11 @@ class Requirement
   end
 
   def eql?(other)
-    other.is_a?(self.class) && hash == other.hash
+    instance_of?(other.class) && hash == other.hash
   end
 
   def hash
-    message.hash
+    [name, *tags].hash
   end
 
   private

Library/Homebrew/requirements.rb

@@ -83,7 +83,9 @@ class X11Dependency < Requirement
       raise TypeError, "expected X11Dependency"
     end
 
-    if other.min_version.nil?
+    if min_version.nil? && other.min_version.nil?
+      0
+    elsif other.min_version.nil?
       1
     elsif @min_version.nil?
       -1
@@ -92,6 +94,35 @@ class X11Dependency < Requirement
     end
   end
 
+  # When X11Dependency is subclassed, the new class should
+  # also inherit the information specified in the DSL above.
+  def self.inherited(mod)
+    instance_variables.each do |ivar|
+      mod.instance_variable_set(ivar, instance_variable_get(ivar))
+    end
+  end
+
+  # X11Dependency::Proxy is a base class for the X11 pseudo-deps.
+  # Rather than instantiate it directly, a separate class is built
+  # for each of the packages that we proxy to X11Dependency.
+  class Proxy < self
+    PACKAGES = [:libpng, :freetype, :pixman, :fontconfig]
+
+    def self.for(name, *tags)
+      constant = name.capitalize
+
+      if const_defined?(constant)
+        klass = const_get(constant)
+      else
+        klass = Class.new(self) do
+          def initialize(name, *tags) super end
+        end
+
+        const_set(constant, klass)
+      end
+      klass.new(name, *tags)
+    end
+  end
 end
 
 

Library/Homebrew/test/test_x11_deps.rb

@@ -0,0 +1,67 @@
+require 'testing_env'
+require 'requirements'
+require 'extend/set'
+
+class X11DependencyTests < Test::Unit::TestCase
+  def test_eql_instances_are_eql
+    x = X11Dependency.new
+    y = X11Dependency.new
+    assert x.eql?(y)
+    assert y.eql?(x)
+    assert x.hash == y.hash
+  end
+
+  def test_not_eql_when_hashes_differ
+    x = X11Dependency.new("foo")
+    y = X11Dependency.new
+    assert x.hash != y.hash
+    assert !x.eql?(y)
+    assert !y.eql?(x)
+  end
+
+  def test_proxy_for
+    x = X11Dependency::Proxy.for("libpng")
+    assert_instance_of X11Dependency::Proxy::Libpng, x
+    assert_kind_of X11Dependency, x
+  end
+
+  def test_proxy_eql_instances_are_eql
+    x = X11Dependency::Proxy.for("libpng")
+    y = X11Dependency::Proxy.for("libpng")
+    assert x.eql?(y)
+    assert y.eql?(x)
+    assert x.hash == y.hash
+  end
+
+  def test_proxy_not_eql_when_hashes_differ
+    x = X11Dependency::Proxy.for("libpng")
+    y = X11Dependency::Proxy.for("fontconfig")
+    assert x.hash != y.hash
+    assert !x.eql?(y)
+    assert !y.eql?(x)
+  end
+
+  def test_x_never_eql_to_proxy_x11_dep
+    x = X11Dependency.new("libpng")
+    p = X11Dependency::Proxy.for("libpng")
+    assert !x.eql?(p)
+    assert !p.eql?(x)
+  end
+end
+
+class X11DepCollectionTests < Test::Unit::TestCase
+  def setup
+    @set = ComparableSet.new
+  end
+
+  def test_x_can_coxist_with_proxy
+    @set << X11Dependency.new << X11Dependency::Proxy.for("libpng")
+    assert_equal 2, @set.count
+  end
+
+  def test_multiple_proxies_can_coexist
+    @set << X11Dependency::Proxy.for("libpng")
+    @set << X11Dependency::Proxy.for("fontconfig")
+    assert_equal 2, @set.count
+  end
+end