From 4c97d46329604196c10d396c3ee402081621e311 Mon Sep 17 00:00:00 2001 From: Camillo Lugaresi Date: Sun, 4 Aug 2013 03:43:19 -0500 Subject: [PATCH 1/4] new patch DSL, with support for checksums --- Library/Homebrew/formula.rb | 34 +------- Library/Homebrew/patches.rb | 157 +++++++++++++++++++++++------------- 2 files changed, 101 insertions(+), 90 deletions(-) diff --git a/Library/Homebrew/formula.rb b/Library/Homebrew/formula.rb index 09191fed1565..591e5d907ee8 100644 --- a/Library/Homebrew/formula.rb +++ b/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 diff --git a/Library/Homebrew/patches.rb b/Library/Homebrew/patches.rb index ccd677e7db9b..eb96261556a1 100644 --- a/Library/Homebrew/patches.rb +++ b/Library/Homebrew/patches.rb @@ -1,82 +1,118 @@ 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', + # 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, sha256=nil + patchlist << Patch.new(patch_p, '%03d-homebrew.diff' % patchlist.length, url_or_io, sha256) 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!} + 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! + begin + Pathname.new(p.compressed_filename).verify_checksum p.checksum + rescue ChecksumMissingError + # accept this silently for now + 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, sha256 @patch_p = patch_p @patch_filename = filename - @compressed_filename = nil + @compressed_filename = @patch_filename @compression = nil @url = nil + @data = nil + @checksum = Checksum.new(:sha256, sha256) 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 +124,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 +137,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 +173,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) From 95d360ca065f843e018514b271a496be4b9fbc3e Mon Sep 17 00:00:00 2001 From: Camillo Lugaresi Date: Sun, 4 Aug 2013 15:16:35 -0500 Subject: [PATCH 2/4] updated testsuite --- Library/Homebrew/test/test_patches.rb | 55 ++++++++++++++------------ Library/Homebrew/test/test_patching.rb | 28 ++++++++++++- 2 files changed, 56 insertions(+), 27 deletions(-) diff --git a/Library/Homebrew/test/test_patches.rb b/Library/Homebrew/test/test_patches.rb index 668d4b395ec1..6a44d98642ed 100644 --- a/Library/Homebrew/test/test_patches.rb +++ b/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 diff --git a/Library/Homebrew/test/test_patching.rb b/Library/Homebrew/test/test_patching.rb index 6642704c0833..fcb90282e49d 100644 --- a/Library/Homebrew/test/test_patching.rb +++ b/Library/Homebrew/test/test_patching.rb @@ -6,6 +6,32 @@ def read_file path File.open(path, 'r') { |f| f.read } end + def test_patch_dsl_p0 + shutup do + Class.new(TestBall) do + patch :p0, "file:///#{TEST_FOLDER}/patches/noop-b.diff", + "57958271bb802a59452d0816e0670d16c8b70bdf6530bcf6f78726489ad89b90" + end.new("test_patch_dsl_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." + end + end + end + + def test_patch_dsl_p1 + shutup do + Class.new(TestBall) do + patch :p1, "file:///#{TEST_FOLDER}/patches/noop-a.diff", + "83404f4936d3257e65f176c4ffb5a5b8d6edd644a21c8d8dcc73e22a6d28fcfa" + end.new("test_patch_dsl_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 + end + def test_single_patch shutup do Class.new(TestBall) do @@ -38,7 +64,7 @@ def test_patch_p0 shutup do Class.new(TestBall) do def patches - "file:///#{TEST_FOLDER}/patches/noop-a.diff" + { :p0 => ["file:///#{TEST_FOLDER}/patches/noop-b.diff"] } end end.new("test_patch_p0").brew do s = read_file 'libexec/NOOP' From c57edd1e7fce5126f37e5190dd391922ed02c7bd Mon Sep 17 00:00:00 2001 From: Camillo Lugaresi Date: Sun, 4 Aug 2013 20:32:05 -0500 Subject: [PATCH 3/4] warn when a patch doesn't have a checksum --- Library/Homebrew/patches.rb | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/patches.rb b/Library/Homebrew/patches.rb index eb96261556a1..2582a24cdcfb 100644 --- a/Library/Homebrew/patches.rb +++ b/Library/Homebrew/patches.rb @@ -38,6 +38,15 @@ def self.patch patch_p, url_or_io, sha256=nil end end + # 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 @@ -69,10 +78,13 @@ def apply_patches curl *(external_patches.collect{|p| p.curl_args}.flatten) external_patches.each do |p| p.stage! + pf = Pathname.new(p.compressed_filename) begin - Pathname.new(p.compressed_filename).verify_checksum p.checksum + pf.verify_checksum p.checksum rescue ChecksumMissingError - # accept this silently for now + 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 From 7c950db7ea6623e74338766805dd31b612bae0e5 Mon Sep 17 00:00:00 2001 From: Camillo Lugaresi Date: Sun, 11 Aug 2013 12:58:05 -0500 Subject: [PATCH 4/4] support SHA1 in addition to SHA256 for patches --- Library/Homebrew/patches.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Library/Homebrew/patches.rb b/Library/Homebrew/patches.rb index 2582a24cdcfb..34cc8eb3c9bc 100644 --- a/Library/Homebrew/patches.rb +++ b/Library/Homebrew/patches.rb @@ -32,8 +32,8 @@ def self.patchlist end # This is the new, preferred DSL that allows a checksum to be specified. - def self.patch patch_p, url_or_io, sha256=nil - patchlist << Patch.new(patch_p, '%03d-homebrew.diff' % patchlist.length, url_or_io, sha256) + 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 @@ -111,14 +111,15 @@ class Patch # Used by audit attr_reader :url - def initialize patch_p, filename, url, sha256 + def initialize patch_p, filename, url, sha1_or_256 @patch_p = patch_p @patch_filename = filename @compressed_filename = @patch_filename @compression = nil @url = nil @data = nil - @checksum = Checksum.new(:sha256, sha256) + 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