new patch DSL, with support for checksums

Library/Homebrew/formula.rb

@@ -15,6 +15,7 @@
 class Formula
   include FileUtils
   include Utils::Inreplace
+  include Patches
   extend BuildEnvironmentDSL
 
   attr_reader :name, :path, :homepage, :downloader
@@ -187,17 +188,6 @@ def caveats; nil end
   # 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
-
   # rarely, you don't want your library symlinked into the main prefix
   # see gettext.rb for an example
   def keg_only?
@@ -233,7 +223,7 @@ def brew
 
     stage do
       begin
-        patch
+        apply_patches
         # we allow formulae to do anything they want to the Ruby process
         # so load any deps before this point! And exit asap afterwards
         yield self
@@ -598,26 +588,6 @@ def stage
     end
   end
 
-  def patch
-    patch_list = Patches.new(patches)
-    return if patch_list.empty?
-
-    if patch_list.external_patches?
-      ohai "Downloading patches"
-      patch_list.download!
-    end
-
-    ohai "Patching"
-    patch_list.each do |p|
-      case p.compression
-        when :gzip  then safe_system "/usr/bin/gunzip",  p.compressed_filename
-        when :bzip2 then safe_system "/usr/bin/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', '-f', *(p.patch_args)
-    end
-  end
-
   def self.method_added method
     case method
     when :brew

Library/Homebrew/patches.rb

@@ -1,82 +1,131 @@
 require 'stringio'
+require 'checksum'
+
+module Patches
+  # This mixin contains all the patch-related code.
+
+  # There is a new DSL for specifying patches:
+  #   patch :p0, 'http://example.com/patch.diff', <sha256>
+  #   patch :p1, DATA
+  # DATA is for patches built into the formula: put __END__ at the end
+  # of the formula script and append the diff.
+
+  # The old "def patches" method is still supported:
+  # 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', DATA],
+  #     :p1 =>  'http://bar.com/patch2'
+  #   }
+  def patches; end
+
+  # We want to be able to have "patch :p1, DATA" in our formula; however,
+  # when the formulary loads the formula using require, DATA is not defined
+  # (since the formula is not the main script), so we define it here to avoid
+  # a load error.
+  DATA = nil unless defined?(DATA)
+
+  def self.included(base)
+    base.class_eval do
+      def self.patchlist
+        @patchlist ||= []
+      end
 
-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
+      # This is the new, preferred DSL that allows a checksum to be specified.
+      def self.patch patch_p, url_or_io, sha1_or_256=nil
+        patchlist << Patch.new(patch_p, '%03d-homebrew.diff' % patchlist.length, url_or_io, sha1_or_256)
       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!}
+  # Note that the DSL consists of class methods, and thus the patchlist is
+  # created as a variable on the class object. On the other hand, the old
+  # patches method is an instance method, so the patchlist can only be
+  # completed when an instance of the formula is created. Here we make it so
+  # that the patchlist in both contexts refers to the same list. This is ok
+  # because we won't have multiple instances of the same formula subclass.
+  # The same assumption is made in several places in formula.rb (e.g. in the
+  # handling of options), but we're documenting it here for the benefit of
+  # future maintainers.
+  def patchlist
+    unless @patchlist
+      @patchlist = self.class.patchlist
+      process_legacy_patches
+    end
+    @patchlist
   end
 
-  private
-
-  def external_patches
-    @patches.select{|p| p.external?}
+  def process_legacy_patches
+    # Handle legacy "patches" method
+    legacy_p = patches
+    return if legacy_p.nil? or legacy_p.empty?
+    # This can be an array of patches, or a hash with patch_p as keys
+    legacy_p = { :p1 => legacy_p } unless legacy_p.kind_of? Hash
+    legacy_p.each do |patch_p, urls|
+      # This can be an array of urls, but also an individual url, or DATA
+      urls = [urls] unless urls.kind_of? Array
+      urls.each {|url| self.class.patch patch_p, url, ""}
+    end
   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 apply_patches
+    return if patchlist.empty?
+
+    external_patches = patchlist.select{|p| p.external?}
+    unless external_patches.empty?
+      ohai "Downloading patches"
+      # downloading all at once is much more efficient, especially for FTP
+      curl *(external_patches.collect{|p| p.curl_args}.flatten)
+      external_patches.each do |p|
+        p.stage!
+        pf = Pathname.new(p.compressed_filename)
+        begin
+          pf.verify_checksum p.checksum
+        rescue ChecksumMissingError
+          opoo "Cannot verify patch integrity"
+          puts "The formula did not provide a checksum for the patch: #{p.url}"
+          puts "For your reference the SHA256 is: #{pf.sha256}"
+        rescue ChecksumMismatchError => e
+          e.advice = "Bad patch: " + p.url
+          raise e
+        end
+      end
+    end
+    patchlist.each{|p| p.write_data! if p.data?}
 
-  def normalize_patches patches
-    if patches.kind_of? Hash
-      patches
-    else
-      { :p1 => patches } # We assume -p1
+    ohai "Patching"
+    patchlist.each do |p|
+      case p.compression
+        when :gzip  then safe_system "/usr/bin/gunzip",  p.compressed_filename
+        when :bzip2 then safe_system "/usr/bin/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', '-f', *(p.patch_args)
     end
   end
-
 end
 
 class Patch
   # Used by formula to unpack after downloading
-  attr_reader :compression
-  attr_reader :compressed_filename
+  attr_reader :compression, :compressed_filename, :checksum
   # Used by audit
   attr_reader :url
 
-  def initialize patch_p, filename, url
+  def initialize patch_p, filename, url, sha1_or_256
     @patch_p = patch_p
     @patch_filename = filename
-    @compressed_filename = nil
+    @compressed_filename = @patch_filename
     @compression = nil
     @url = nil
+    @data = nil
+    checksumtype = (sha1_or_256 && sha1_or_256.length == 40) ? :sha1 : :sha256
+    @checksum = Checksum.new(checksumtype, sha1_or_256)
 
     if url.kind_of? IO or url.kind_of? StringIO
+      @data = url.read.to_s
       # File-like objects. Most common when DATA is passed.
-      write_data url
+      # We can't write this during initialization because we're still in the formula
+      # loading phase.
     elsif looks_like_url(url)
       @url = url # Save URL to mark this as an external patch
     else
@@ -88,6 +137,7 @@ def initialize patch_p, filename, url
 
   # rename the downloaded file to take compression into account
   def stage!
+    write_data unless @data.nil?
     return unless external?
     detect_compression!
     case @compression
@@ -100,6 +150,16 @@ def stage!
     end
   end
 
+  # Write the given file object (DATA) out to a local file for patch
+  def write_data!
+    pn = Pathname.new @patch_filename
+    pn.write(brew_var_substitution(@data))
+  end
+
+  def data?
+    not @data.nil?
+  end
+
   def external?
     not @url.nil?
   end
@@ -126,12 +186,6 @@ def detect_compression!
     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)

Library/Homebrew/test/test_patches.rb

@@ -3,52 +3,55 @@
 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 formula_with_patches &block
+    formula do
+      url "http://example.com/foo"
+      version "0.0"
+      define_method :patches, &block
+    end
+  end
+
   def test_patchSingleString
-    patches = Patches.new("http://example.com/patch.diff")
-    assert_equal 1, patches.patches.length
-    p = patches.patches[0]
+    f = formula_with_patches { "http://example.com/patch.diff" }
+    assert_equal 1, f.patchlist.length
+    p = f.patchlist[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
+    f = formula_with_patches { ["http://example.com/patch1.diff", "http://example.com/patch2.diff"] }
+    assert_equal 2, f.patchlist.length
 
-    p1 = patches.patches[0]
+    p1 = f.patchlist[0]
     assert_equal :p1, p1.patch_p
 
-    p2 = patches.patches[0]
+    p2 = f.patchlist[0]
     assert_equal :p1, p2.patch_p
   end
 
   def test_p0_hash_to_string
-    patches = Patches.new({
+    f = formula_with_patches do {
       :p0 => "http://example.com/patch.diff"
-    })
-    assert_equal 1, patches.patches.length
+    } end
+    assert_equal 1, f.patchlist.length
 
-    p = patches.patches[0]
+    p = f.patchlist[0]
     assert_equal :p0, p.patch_p
   end
 
   def test_p1_hash_to_string
-    patches = Patches.new({
+    f = formula_with_patches do {
       :p1 => "http://example.com/patch.diff"
-    })
-    assert_equal 1, patches.patches.length
+    } end
+    assert_equal 1, f.patchlist.length
 
-    p = patches.patches[0]
+    p = f.patchlist[0]
     assert_equal :p1, p.patch_p
   end
 
@@ -57,12 +60,12 @@ def test_mixed_hash_to_strings
       :p1 => "http://example.com/patch1.diff",
       :p0 => "http://example.com/patch0.diff"
     }
-    patches = Patches.new(expected)
-    assert_equal 2, patches.patches.length
+    f = formula_with_patches { expected }
+    assert_equal 2, f.patchlist.length
 
     # Make sure unique filenames were assigned
     filenames = Set.new
-    patches.each do |p|
+    f.patchlist.each do |p|
       filenames << p.patch_filename
     end
 
@@ -74,12 +77,12 @@ def test_mixed_hash_to_arrays
       :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
+    f = formula_with_patches { expected }
+    assert_equal 4, f.patchlist.length
 
     # Make sure unique filenames were assigned
     filenames = Set.new
-    patches.each do |p|
+    f.patchlist.each do |p|
       filenames << p.patch_filename
     end