new patch DSL, with support for checksums

[camillol]

Man has long dreamed of having checksumming supported for patches (#6795, #10199).
Unfortunately, all who have tried the challenge ended up giving up (#11956).

Until now.

New DSL examples (from the socat formula):

patch :p0, "https://trac.macports.org/export/90442/trunk/dports/sysutils/socat/files/patch-xioexit.c.diff", "4a72f278d960310144556f7e1c2645f492b289bdc28de9f786a1ecd6dcdf21b7"
patch :p1, DATA if build.devel?

Yes, the lines are long.

[SNIP]

[MikeMcQuaid]

I'm in favour of this in theory as long as Homebrew can provide new checksums (which isn't clear from the code) and we can do something about the line length (perhaps use a block instead)?

[camillol]

What do you mean by homebrew providing new checksums?

For the long lines, you can just break them:

patch :p0,
  "https://trac.macports.org/export/90442/trunk/dports/sysutils/socat/files/patch-xioexit.c.diff",
  "4a72f278d960310144556f7e1c2645f492b289bdc28de9f786a1ecd6dcdf21b7"
patch :p1, DATA if build.devel?

[jacknagel]

Test suite needs to be updated, it doesn't even run after applying this

[adamv]

This should be a separate pull request (because it can be pulled while the patches DSL is still under review.)

[camillol]

@adamv #21654

[adamv]

Does this support sha1 patches or just sha256 patches?

[adamv]

NOTE: I'm going to edit out the large animated gif above

[camillol]

@jacknagel test suit updated.
@adamv only sha256.

[camillol]

For now, we don't need to use anything but sha256, so a raw string is enough.
Alternatively, we could pass in a checksum object, returned by a suitable function. For example, it might be nice to have patch :p1, url, sha256(...), but only if we can have checksum sha256(...) in the main formula DSL, which we can't because we're already committed to sha256 being a class method.
In any case, if we ever need other hashes in the future, we can add support for them either by passing them as checksum objects, or as strings of different length (assuming they won't be the same length as 256), or as strings with a prefix ('sha999:...').
The third option is to have a separate argument specifying the hash type, e.g. patch :p1, url, :sha256, '...', but that's ugly and unnecessary.

[MikeMcQuaid]

@camillol I mean some way of (even if an option or ARGV.homebrew_developer?) of downloading a patch and emitting the checksum to the console if it's missing so it becomes easier to populate them without having to curl it manually. It would be good if checksums could be SHA-1 and SHA-256 and we default to the latter for consistency elsewhere.

I still would rather we split this into a block rather than adding another argument onto a method invocation.

[adamv]

Check the checksum length to see if it should be treated as sha1 or sha256.

[camillol]

@MikeMcQuaid now you can give an empty string for the patch checksum (patch :p1, url, ""), and homebrew will print a warning showing the patch's checksum. It will also warn about checksumless patches in the old patches method (but not for DATA, of course).

@adamv I don't see the need to support sha1.

[MikeMcQuaid]

@camillol That sounds good. Probably want to make that only print for developers so we don't spam too hard existing formulae that haven't been updated. Might want to make it an audit rule (@jacknagel)? I dunno.

I do, however, think it's really important we both have SHA-1 support and default to that rather than SHA-256. Unless I'm overruled by a few other maintainers I kinda insist on it.

[camillol]

Why is SHA-1 support important?

[MikeMcQuaid]

Because we use it most widely for archive checksums and I think SHA-256 is overkill at this point (as I explained in the other PR).

[camillol]

Ok, but what's the problem? Are you concerned that it might be too secure? Is there a performance issue?

[camillol]

Removed the formulary change, btw.

[MikeMcQuaid]

@camillol Why not use SHA-512 or SHA-3? If we want to use SHA-256 as default everywhere in Homebrew that's a different discussion. As SHA-1 is the default for formulae tarballs it should be the default for patches too.

[LinusU]

Just a suggestion, as I'm considering taking a stab at this and also include cache support for patches.

patch do
  url 'https://trac.macports.org/export/90442/trunk/dports/sysutils/socat/files/patch-xioexit.c.diff'
  sha256 '4a72f278d960310144556f7e1c2645f492b289bdc28de9f786a1ecd6dcdf21b7'
end

patch do
  data DATA
  sha1 '40840f60975de4010e490a8c0f97c3082d32e60c'
end

Maybe even allow this, thoughts?

patch do
  applyUnless OS.Linux?
  url 'https://trac.macports.org/export/90442/trunk/dports/sysutils/socat/files/patch-xioexit.c.diff'
  sha256 '4a72f278d960310144556f7e1c2645f492b289bdc28de9f786a1ecd6dcdf21b7'
end
patch do
  applyIf MacOS.version > 14
  url 'https://trac.macports.org/export/90442/trunk/dports/sysutils/socat/files/patch-xioexit.c.diff'
  sha256 '4a72f278d960310144556f7e1c2645f492b289bdc28de9f786a1ecd6dcdf21b7'
end

[jacknagel]

@LinusU I have a complete implementation, it's not quite merge ready though: #22775

[jacknagel]

Closing since we're going a different direction here.

[LinusU]

@jacknagel Cool!