Enable new patch implementation with compatibility layer

Library/Homebrew/formula.rb

@@ -3,7 +3,6 @@ require 'formula_lock'
 require 'formula_pin'
 require 'hardware'
 require 'bottles'
-require 'patches'
 require 'compilers'
 require 'build_environment'
 require 'build_options'
@@ -224,16 +223,7 @@ class Formula
   # any e.g. configure options for this package
   def options; [] end
 
-  # patches are automatically applied after extracting the tarball
-  # return an array of strings, or if you need a patch level other than -p1
-  # return a Hash eg.
-  #   {
-  #     :p0 => ['http://foo.com/patch1', 'http://foo.com/patch2'],
-  #     :p1 =>  'http://bar.com/patch2'
-  #   }
-  # The final option is to return DATA, then put a diff after __END__. You
-  # can still return a Hash with DATA as the value for a patch level key.
-  def patches; end
+  def patches; {} end
 
   # rarely, you don't want your library symlinked into the main prefix
   # see gettext.rb for an example
@@ -633,23 +623,15 @@ class Formula
   end
 
   def patch
-    patch_list = Patches.new(patches)
-    return if patch_list.empty?
+    active_spec.add_legacy_patches(patches)
+    return if patchlist.empty?
 
-    if patch_list.external_patches?
-      ohai "Downloading patches"
-      patch_list.download!
+    active_spec.patches.select(&:external?).each do |patch|
+      patch.verify_download_integrity(patch.fetch)
     end
 
     ohai "Patching"
-    patch_list.each do |p|
-      case p.compression
-        when :gzip  then with_system_path { safe_system "gunzip",  p.compressed_filename }
-        when :bzip2 then with_system_path { safe_system "bunzip2", p.compressed_filename }
-      end
-      # -f means don't prompt the user if there are errors; just exit with non-zero status
-      safe_system '/usr/bin/patch', '-g', '0', '-f', *(p.patch_args)
-    end
+    active_spec.patches.each(&:apply)
   end
 
   # Explicitly request changing C++ standard library compatibility check

Library/Homebrew/patch.rb

@@ -1,5 +1,6 @@
 require 'resource'
 require 'stringio'
+require 'erb'
 
 class Patch
   def self.create(strip, io=nil, &block)
@@ -22,6 +23,32 @@ class Patch
     end
   end
 
+  def self.normalize_legacy_patches(list)
+    patches = []
+
+    case list
+    when Hash
+      list
+    when Array, String, IO, StringIO
+      { :p1 => list }
+    else
+      {}
+    end.each_pair do |strip, urls|
+      urls = [urls] unless Array === urls
+      urls.each do |url|
+        case url
+        when IO, StringIO
+          patch = IOPatch.new(url, strip)
+        else
+          patch = LegacyPatch.new(strip, url)
+        end
+        patches << patch
+      end
+    end
+
+    patches
+  end
+
   attr_reader :whence
 
   def external?
@@ -80,3 +107,31 @@ class ExternalPatch < Patch
     end
   end
 end
+
+# Legacy patches have no checksum and are not cached
+class LegacyPatch < ExternalPatch
+  def initialize(strip, url)
+    super(strip)
+    resource.url = url
+  end
+
+  def owner= owner
+    super
+    resource.name = "patch-#{ERB::Util.url_encode(resource.url)}"
+  end
+
+  def fetch
+    resource.clear_cache
+    super
+  end
+
+  def verify_download_integrity(fn)
+    # no-op
+  end
+
+  def apply
+    super
+  ensure
+    resource.clear_cache
+  end
+end

Library/Homebrew/patches.rb

@@ -1,143 +0,0 @@
-require 'stringio'
-
-class Patches
-  include Enumerable
-
-  # The patches defined in a formula and the DATA from that file
-  def initialize patches
-    @patches = []
-    return if patches.nil?
-    n = 0
-    normalize_patches(patches).each do |patch_p, urls|
-      # Wrap the urls list in an array if it isn't already;
-      # DATA.each does each line, which doesn't work so great
-      urls = [urls] unless urls.kind_of? Array
-      urls.each do |url|
-        @patches << Patch.new(patch_p, '%03d-homebrew.diff' % n, url)
-        n += 1
-      end
-    end
-  end
-
-  def external_patches?
-    not external_curl_args.empty?
-  end
-
-  def each(&blk)
-    @patches.each(&blk)
-  end
-  def empty?
-    @patches.empty?
-  end
-
-  def download!
-    return unless external_patches?
-
-    # downloading all at once is much more efficient, especially for FTP
-    curl(*external_curl_args)
-
-    external_patches.each{|p| p.stage!}
-  end
-
-  private
-
-  def external_patches
-    @patches.select{|p| p.external?}
-  end
-
-  # Collects the urls and output names of all external patches
-  def external_curl_args
-    external_patches.collect{|p| p.curl_args}.flatten
-  end
-
-  def normalize_patches patches
-    if patches.kind_of? Hash
-      patches
-    else
-      { :p1 => patches } # We assume -p1
-    end
-  end
-
-end
-
-class Patch
-  # Used by formula to unpack after downloading
-  attr_reader :compression
-  attr_reader :compressed_filename
-  # Used by audit
-  attr_reader :url
-
-  def initialize patch_p, filename, url
-    @patch_p = patch_p
-    @patch_filename = filename
-    @compressed_filename = nil
-    @compression = nil
-    @url = nil
-
-    if url.kind_of? IO or url.kind_of? StringIO
-      # File-like objects. Most common when DATA is passed.
-      write_data url
-    elsif looks_like_url(url)
-      @url = url # Save URL to mark this as an external patch
-    else
-      # it's a file on the local filesystem
-      # use URL as the filename for patch
-      @patch_filename = url
-    end
-  end
-
-  # rename the downloaded file to take compression into account
-  def stage!
-    return unless external?
-    detect_compression!
-    case @compression
-    when :gzip
-      @compressed_filename = @patch_filename + '.gz'
-      FileUtils.mv @patch_filename, @compressed_filename
-    when :bzip2
-      @compressed_filename = @patch_filename + '.bz2'
-      FileUtils.mv @patch_filename, @compressed_filename
-    end
-  end
-
-  def external?
-    not @url.nil?
-  end
-
-  def patch_args
-    ["-#{@patch_p}", '-i', @patch_filename]
-  end
-
-  def curl_args
-    [@url, '-o', @patch_filename]
-  end
-
-  private
-
-  # Detect compression type from the downloaded patch.
-  def detect_compression!
-    # If nil we have not tried to detect yet
-    if @compression.nil?
-      path = Pathname.new(@patch_filename)
-      if path.exist?
-        @compression = path.compression_type
-        @compression ||= :none # If nil, convert to :none
-      end
-    end
-  end
-
-  # Write the given file object (DATA) out to a local file for patch
-  def write_data f
-    pn = Pathname.new @patch_filename
-    pn.write(brew_var_substitution(f.read.to_s))
-  end
-
-  # Do any supported substitutions of HOMEBREW vars in a DATA patch
-  def brew_var_substitution s
-    s.gsub("HOMEBREW_PREFIX", HOMEBREW_PREFIX)
-  end
-
-  def looks_like_url str
-    str =~ %r[^\w+\://]
-  end
-end

Library/Homebrew/software_spec.rb

@@ -91,6 +91,12 @@ class SoftwareSpec
   def patch strip=:p1, io=nil, &block
     patches << Patch.create(strip, io, &block)
   end
+
+  def add_legacy_patches(list)
+    list = Patch.normalize_legacy_patches(list)
+    list.each { |p| p.owner = self }
+    patches.concat(list)
+  end
 end
 
 class HeadSoftwareSpec < SoftwareSpec

Library/Homebrew/test/test_patch.rb

@@ -50,3 +50,66 @@ class PatchTests < Test::Unit::TestCase
     assert_raises(ArgumentError) { Patch.create(Object.new) }
   end
 end
+
+class LegacyPatchTests < Test::Unit::TestCase
+  def test_patch_single_string
+    patches = Patch.normalize_legacy_patches("http://example.com/patch.diff")
+    assert_equal 1, patches.length
+    assert_equal :p1, patches.first.strip
+  end
+
+  def test_patch_array
+    patches = Patch.normalize_legacy_patches(
+      %w{http://example.com/patch1.diff http://example.com/patch2.diff}
+    )
+
+    assert_equal 2, patches.length
+    assert_equal :p1, patches[0].strip
+    assert_equal :p1, patches[1].strip
+  end
+
+  def test_p0_hash_to_string
+    patches = Patch.normalize_legacy_patches(
+      :p0 => "http://example.com/patch.diff"
+    )
+
+    assert_equal 1, patches.length
+    assert_equal :p0, patches.first.strip
+  end
+
+  def test_p1_hash_to_string
+    patches = Patch.normalize_legacy_patches(
+      :p1 => "http://example.com/patch.diff"
+    )
+
+    assert_equal 1, patches.length
+    assert_equal :p1, patches.first.strip
+  end
+
+  def test_mixed_hash_to_strings
+    patches = Patch.normalize_legacy_patches(
+      :p1 => "http://example.com/patch1.diff",
+      :p0 => "http://example.com/patch0.diff"
+    )
+    assert_equal 2, patches.length
+    assert_equal 1, patches.select { |p| p.strip == :p0 }.length
+    assert_equal 1, patches.select { |p| p.strip == :p1 }.length
+  end
+
+  def test_mixed_hash_to_arrays
+    patches = Patch.normalize_legacy_patches(
+      :p1 => ["http://example.com/patch10.diff",
+              "http://example.com/patch11.diff"],
+      :p0 => ["http://example.com/patch00.diff",
+              "http://example.com/patch01.diff"]
+    )
+
+    assert_equal 4, patches.length
+    assert_equal 2, patches.select { |p| p.strip == :p0 }.length
+    assert_equal 2, patches.select { |p| p.strip == :p1 }.length
+  end
+
+  def test_nil
+    assert_empty Patch.normalize_legacy_patches(nil)
+  end
+end

Library/Homebrew/test/test_patches.rb

@@ -1,88 +0,0 @@
-require 'testing_env'
-require 'test/testball'
-require 'set'
-
-# Expose some internals
-class Patches
-  attr_reader :patches
-end
-
-class Patch
-  attr_reader :patch_p
-  attr_reader :patch_filename
-end
-
-
-class PatchingTests < Test::Unit::TestCase
-  def test_patchSingleString
-    patches = Patches.new("http://example.com/patch.diff")
-    assert_equal 1, patches.patches.length
-    p = patches.patches[0]
-    assert_equal :p1, p.patch_p
-  end
-
-  def test_patchArray
-    patches = Patches.new(["http://example.com/patch1.diff", "http://example.com/patch2.diff"])
-    assert_equal 2, patches.patches.length
-
-    p1 = patches.patches[0]
-    assert_equal :p1, p1.patch_p
-
-    p2 = patches.patches[0]
-    assert_equal :p1, p2.patch_p
-  end
-
-  def test_p0_hash_to_string
-    patches = Patches.new({
-      :p0 => "http://example.com/patch.diff"
-    })
-    assert_equal 1, patches.patches.length
-
-    p = patches.patches[0]
-    assert_equal :p0, p.patch_p
-  end
-
-  def test_p1_hash_to_string
-    patches = Patches.new({
-      :p1 => "http://example.com/patch.diff"
-    })
-    assert_equal 1, patches.patches.length
-
-    p = patches.patches[0]
-    assert_equal :p1, p.patch_p
-  end
-
-  def test_mixed_hash_to_strings
-    expected = {
-      :p1 => "http://example.com/patch1.diff",
-      :p0 => "http://example.com/patch0.diff"
-    }
-    patches = Patches.new(expected)
-    assert_equal 2, patches.patches.length
-
-    # Make sure unique filenames were assigned
-    filenames = Set.new
-    patches.each do |p|
-      filenames << p.patch_filename
-    end
-
-    assert_equal 2, filenames.size
-  end
-
-  def test_mixed_hash_to_arrays
-    expected = {
-      :p1 => ["http://example.com/patch10.diff","http://example.com/patch11.diff"],
-      :p0 => ["http://example.com/patch00.diff","http://example.com/patch01.diff"]
-    }
-    patches = Patches.new(expected)
-    assert_equal 4, patches.patches.length
-
-    # Make sure unique filenames were assigned
-    filenames = Set.new
-    patches.each do |p|
-      filenames << p.patch_filename
-    end
-
-    assert_equal 4, filenames.size
-  end
-end

Library/Homebrew/test/test_patching.rb

@@ -1,64 +1,153 @@
 require 'testing_env'
-require 'test/testball'
+require 'formula'
+require 'testball'
 
 class PatchingTests < Test::Unit::TestCase
-  def read_file path
-    File.open(path, 'r') { |f| f.read }
+  def formula(&block)
+    super do
+      url "file:///#{TEST_FOLDER}/tarballs/testball-0.1.tbz"
+      sha1 "482e737739d946b7c8cbaf127d9ee9c148b999f5"
+      class_eval(&block)
+    end
+  end
+
+  def assert_patched(path)
+    s = File.read(path)
+    assert !s.include?("NOOP"), "File was unpatched."
+    assert s.include?("ABCD"), "File was not patched as expected."
   end
 
   def test_single_patch
     shutup do
-      Class.new(TestBall) do
+      formula do
         def patches
           "file:///#{TEST_FOLDER}/patches/noop-a.diff"
         end
-      end.new("test_single_patch").brew do
-        s = read_file 'libexec/NOOP'
-        assert !s.include?("NOOP"), "File was unpatched."
-        assert s.include?("ABCD"), "File was not patched as expected."
+      end.brew { assert_patched 'libexec/NOOP' }
+    end
+  end
+
+  def test_single_patch_dsl
+    shutup do
+      formula do
+        patch do
+          url "file:///#{TEST_FOLDER}/patches/noop-a.diff"
+          sha1 "fa8af2e803892e523fdedc6b758117c45e5749a2"
+        end
+      end.brew { assert_patched 'libexec/NOOP' }
+    end
+  end
+
+  def test_single_patch_dsl_with_strip
+    shutup do
+      formula do
+        patch :p1 do
+          url "file:///#{TEST_FOLDER}/patches/noop-a.diff"
+          sha1 "fa8af2e803892e523fdedc6b758117c45e5749a2"
+        end
+      end.brew { assert_patched 'libexec/NOOP' }
+    end
+  end
+
+  def test_single_patch_dsl_with_incorrect_strip
+    assert_raises(ErrorDuringExecution) do
+      shutup do
+        formula do
+          patch :p0 do
+            url "file:///#{TEST_FOLDER}/patches/noop-a.diff"
+            sha1 "fa8af2e803892e523fdedc6b758117c45e5749a2"
+          end
+        end.brew { }
       end
     end
   end
 
-  def test_patch_array
+  def test_patch_p0_dsl
     shutup do
-      Class.new(TestBall) do
-        def patches
-          ["file:///#{TEST_FOLDER}/patches/noop-a.diff"]
-        end
-      end.new("test_patch_array").brew do
-        s = read_file 'libexec/NOOP'
-        assert !s.include?("NOOP"), "File was unpatched."
-        assert s.include?("ABCD"), "File was not patched as expected."
+      formula do
+        patch :p0 do
+          url "file:///#{TEST_FOLDER}/patches/noop-b.diff"
+          sha1 "3b54bd576f998ef6d6623705ee023b55062b9504"
         end
+      end.brew { assert_patched 'libexec/NOOP' }
     end
   end
 
   def test_patch_p0
     shutup do
-      Class.new(TestBall) do
+      formula do
         def patches
-          "file:///#{TEST_FOLDER}/patches/noop-a.diff"
-        end
-      end.new("test_patch_p0").brew do
-        s = read_file 'libexec/NOOP'
-        assert !s.include?("NOOP"), "File was unpatched."
-        assert s.include?("ABCD"), "File was not patched as expected."
+          { :p0 => "file:///#{TEST_FOLDER}/patches/noop-b.diff" }
         end
+      end.brew { assert_patched 'libexec/NOOP' }
     end
   end
 
-  def test_patch_p1
+  def test_patch_array
     shutup do
-      Class.new(TestBall) do
+      formula do
+        def patches
+          ["file:///#{TEST_FOLDER}/patches/noop-a.diff"]
+        end
+      end.brew { assert_patched 'libexec/noop' }
+    end
+  end
+
+  def test_patch_hash
+    shutup do
+      formula do
+        def patches
+          { :p1 => "file:///#{TEST_FOLDER}/patches/noop-a.diff" }
+        end
+      end.brew { assert_patched 'libexec/noop' }
+    end
+  end
+
+  def test_patch_hash_array
+    shutup do
+      formula do
         def patches
           { :p1 => ["file:///#{TEST_FOLDER}/patches/noop-a.diff"] }
         end
-      end.new("test_patch_p1").brew do
-        s = read_file 'libexec/NOOP'
-        assert !s.include?("NOOP"), "File was unpatched."
-        assert s.include?("ABCD"), "File was not patched as expected."
-      end
+      end.brew { assert_patched 'libexec/noop' }
+    end
+  end
+
+  def test_patch_string
+    shutup do
+      formula do
+        patch %q{
+diff --git a/libexec/NOOP b/libexec/NOOP
+index bfdda4c..e08d8f4 100755
+--- a/libexec/NOOP
++++ b/libexec/NOOP
+@@ -1,2 +1,2 @@
+ #!/bin/bash
+-echo NOOP
+\ No newline at end of file
++echo ABCD
+\ No newline at end of file
+}
+      end.brew { assert_patched 'libexec/noop' }
+    end
+  end
+
+  def test_patch_string_with_strip
+    shutup do
+      formula do
+        patch :p0, %q{
+diff --git libexec/NOOP libexec/NOOP
+index bfdda4c..e08d8f4 100755
+--- libexec/NOOP
++++ libexec/NOOP
+@@ -1,2 +1,2 @@
+ #!/bin/bash
+-echo NOOP
+\ No newline at end of file
++echo ABCD
+\ No newline at end of file
+}
+      end.brew { assert_patched 'libexec/noop' }
     end
   end
 end