Patch improvements: DSL, checksums, caching #22775

jacknagel · 2013-09-24T04:05:06Z

This implements a new patch DSL along with support for checksums and caching.

Here is what the DSL looks like so far. I'm not married to it, so please make suggestions. In particular, I don't find patch :p0 do aesthetically pleasing. However I don't want to push everything into one method call (i.e., patch :p0, 'url', 'checksum') because the lines get too long.

# p1 is assumed, otherwise pass a symbol argument
patch do
  url '...'
  sha1 '...'
end

patch :p0 do
  url '...'
  sha1 '...'
end

# patches can go in the stable, devel, and head blocks
# as well, rather than using conditionals
stable do
  # ...

  patch do
    url '...'
    sha1 '...'
  end
end

# embedded patches (__END__)
# note: we use :DATA because we can only use the
# actual DATA constant properly in the build script 
patch :DATA
patch :p0, :DATA

# embed a patch without using __END__
# this means it is now possible to have multiple embedded
# patches, and make only some of them conditional
patch :p0, %q{
--- src/main.c  2012-06-30 12:22:29.000000000 +0200
+++ src/main.c  2012-06-30 12:19:11.000000000 +0200
@@ -25,6 +25,7 @@

 #include <time.h>
 #include <sys/stat.h>
+#include <netinet/udp.h>
 #include <pwd.h>
 #include <grp.h>
 #include "queue.h"
}

Defining a patches method still works, but these patches are not cached (as they lack checksums).
External patches are implemented using Resources, so instead of everything being downloaded up front, patches are downloaded and applied one at a time. The way resources are coupled to their download strategies makes it impossible to preserve the old behavior and use the existing caching/checksum machinery. I don't think this is a problem, because they are cached, but it is different.

adamv · 2013-09-24T04:10:44Z

Library/Homebrew/formula.rb

@@ -210,7 +213,7 @@ def options; [] end
  #   }
  # 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


I see this is changed from returning nil by default to {}, but there may be formulae that override and end up returning nil anyway. Is that still handled?

Probably not, I put it on my list.

MikeMcQuaid · 2013-09-24T11:42:02Z

This looks very good to me. Just to be super-explicit: fetch will now download external patches with checksums now, yeh?

jacknagel · 2013-09-24T15:46:15Z

Yep.

MikeMcQuaid · 2013-09-24T15:53:18Z

Very cool.

jacknagel · 2013-09-25T04:43:43Z

Going through existing patches, some thoughts:

Most embedded patches are applied at -p1, because this is our default, and we control the embedded patches.
Most external patches are from macports, and most of them are applied with -p0.

Perhaps the default for embedded patches should remain -p1, but for external patches it should be -p0, to reduce the number of places we need to specify the strip level?

adamv · 2013-09-25T04:46:41Z

Another question: git diffs are -p1 by default, right?

jacknagel · 2013-09-25T04:49:10Z

Yeah, that's another (and maybe the primary) reason most embedded patches are -p1.

jacknagel · 2013-09-25T04:56:27Z

We could automatically use -p0 for MacPorts URLs, while still allowing an explicit argument to override it.

MikeMcQuaid · 2013-09-25T08:25:49Z

I'd make it explicit for external except maybe MacPorts (if you think that wouldn't be confusing).

jacknagel · 2013-09-25T22:21:08Z

I'm leaning towards punting on adding any magic at this point. We can always re-evaluate and make some stuff implicit later, but we can't undo it (breaking compat) if it turns out to be a bad idea.

MikeMcQuaid · 2013-09-26T09:03:07Z

Sounds good.

jacknagel · 2013-09-27T19:23:13Z

Updated a few formulae to serve as examples. I'm not planning to do a bunch of them at once, though, since it requires a bit more work than the dependency updates. We can do them a handful at a time when people get a chance.

jacknagel · 2013-10-03T01:59:56Z

Anyone have any further comments on the DSL?

MikeMcQuaid · 2013-10-03T06:52:58Z

Looks good to me. Might be nice if there was a way to do multiple patches in a block that remained readable.

jacknagel · 2013-11-01T06:34:47Z

I cleaned up the flow a bit. Now all patches are fetched and checksummed before any are applied, which matches the current behavior.

MikeMcQuaid · 2013-11-01T18:22:25Z

Looks good to me.

manphiz · 2013-11-01T18:25:54Z

+1.

Also something not related to this PR but will be helpful is to introduce some other routine like post_resource_download that is evaluated before patches for unpacking resources, so that patches can be applied to other resources as well. See homebrew/versions/llvm33 for an ugly example.

MikeMcQuaid · 2013-11-01T18:27:35Z

Another thing while we're talking about patches is that it would be useful to support Git's binary patches.

This commit introduces a new patch implementation that supports checksums and caching. Patches are declared in blocks: patch do url ... sha1 ... end A strip level of -p1 is assumed. It can be overridden using a symbol argument: patch :p0 do url ... sha1 ... end Patches can be declared in stable, devel, and head blocks. This form is preferred over using conditionals. stable do # ... patch do url ... sha1 ... end end Embedded (__END__) patches are declared like so: patch :DATA patch :p0, :DATA Patches can also be embedded by passing a string. This makes it possible to provide multiple embedded patches while making only some of them conditional. patch :p0, "..."

jacknagel · 2014-03-12T21:32:58Z

@Homebrew/owners anybody want to review this again? Nothing has really changed, I just rebased it, so I think it's ready.

I have a separate branch with some updated formulae, I don't want to kill the build bot though.

adamv · 2014-03-12T21:33:44Z

I'll try to take a look tonight.

MikeMcQuaid · 2014-03-12T22:36:16Z

Library/Homebrew/patch.rb

+    else
+      {}
+    end.each_pair do |strip, urls|
+      urls = [urls] unless Array === urls


Is this preferential to urls = urls.to_a?

Yeah, in this case urls can be a string (think { :p1 => "https://..." })

MikeMcQuaid · 2014-03-12T22:37:13Z

Looks good to me.

adamv · 2014-03-13T03:56:58Z

Looks fine here.

jacknagel · 2014-03-14T02:42:48Z

Merged. I updated the example formula, and I'll update the wiki soon.

adamv reviewed Sep 24, 2013
View reviewed changes

jacknagel mentioned this pull request Oct 24, 2013

new patch DSL, with support for checksums #21648

Closed

lanterndev mentioned this pull request Nov 21, 2013

user experience when installing formulas should provide security information #24566

Closed

MikeMcQuaid mentioned this pull request Feb 24, 2014

Mutt recipe pulls in patches via HTTP, performs no signature checking #26965

Closed

jacknagel self-assigned this Mar 5, 2014

geoff-nixon mentioned this pull request Mar 6, 2014

Secure patches, revisited. #27252

Closed

jacknagel added 4 commits March 12, 2014 16:11

Enable new patch implementation with compatibility layer

9eac189

Teach fetch to download patches

8cf7502

Teach audit about new patches implementation

f263b9f

MikeMcQuaid reviewed Mar 12, 2014
View reviewed changes

jacknagel closed this Mar 14, 2014

jacknagel deleted the patches branch March 14, 2014 02:42

Homebrew locked and limited conversation to collaborators Feb 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Patch improvements: DSL, checksums, caching #22775

Patch improvements: DSL, checksums, caching #22775

jacknagel commented Sep 24, 2013

adamv Sep 24, 2013

jacknagel Sep 24, 2013

MikeMcQuaid commented Sep 24, 2013

jacknagel commented Sep 24, 2013

MikeMcQuaid commented Sep 24, 2013

jacknagel commented Sep 25, 2013

adamv commented Sep 25, 2013

jacknagel commented Sep 25, 2013

jacknagel commented Sep 25, 2013

MikeMcQuaid commented Sep 25, 2013

jacknagel commented Sep 25, 2013

MikeMcQuaid commented Sep 26, 2013

jacknagel commented Sep 27, 2013

jacknagel commented Oct 3, 2013

MikeMcQuaid commented Oct 3, 2013

jacknagel commented Nov 1, 2013

MikeMcQuaid commented Nov 1, 2013

manphiz commented Nov 1, 2013

MikeMcQuaid commented Nov 1, 2013

jacknagel commented Mar 12, 2014

adamv commented Mar 12, 2014

MikeMcQuaid Mar 12, 2014

jacknagel Mar 13, 2014

MikeMcQuaid commented Mar 12, 2014

adamv commented Mar 13, 2014

jacknagel commented Mar 14, 2014

Patch improvements: DSL, checksums, caching #22775

Patch improvements: DSL, checksums, caching #22775

Conversation

jacknagel commented Sep 24, 2013

adamv Sep 24, 2013

Choose a reason for hiding this comment

jacknagel Sep 24, 2013

Choose a reason for hiding this comment

MikeMcQuaid commented Sep 24, 2013

jacknagel commented Sep 24, 2013

MikeMcQuaid commented Sep 24, 2013

jacknagel commented Sep 25, 2013

adamv commented Sep 25, 2013

jacknagel commented Sep 25, 2013

jacknagel commented Sep 25, 2013

MikeMcQuaid commented Sep 25, 2013

jacknagel commented Sep 25, 2013

MikeMcQuaid commented Sep 26, 2013

jacknagel commented Sep 27, 2013

jacknagel commented Oct 3, 2013

MikeMcQuaid commented Oct 3, 2013

jacknagel commented Nov 1, 2013

MikeMcQuaid commented Nov 1, 2013

manphiz commented Nov 1, 2013

MikeMcQuaid commented Nov 1, 2013

jacknagel commented Mar 12, 2014

adamv commented Mar 12, 2014

MikeMcQuaid Mar 12, 2014

Choose a reason for hiding this comment

jacknagel Mar 13, 2014

Choose a reason for hiding this comment

MikeMcQuaid commented Mar 12, 2014

adamv commented Mar 13, 2014

jacknagel commented Mar 14, 2014