rubocops/patches: GitLab patches should use .diff

[jonchang]

Only .diff URLs return output comparable to the diffs from git diff --full-index. While the extra metadata from .patch is nice, the instability of the patch contents is undesirable. This has caused issues with patch contents changing over time, for example in Homebrew/homebrew-core#43156 and Homebrew/homebrew-core#72297.

I'll coordinate getting this into homebrew/core and linuxbrew/core but it will take a little while.

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew style with your changes locally?
• Have you successfully run brew typecheck with your changes locally?
• Have you successfully run brew tests with your changes locally?

---

[BrewTestBot]

Review period will end on 2021-03-03 at 08:16:56 UTC.

[carlocab]

Would be good to wait for Homebrew/homebrew-core#70826, as I really don't want to re-run CI again on it.

[BrewTestBot]

Review period ended.

[BrewTestBot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[carlocab]

Resolved the merge conflicts for you.

[carlocab]

Homebrew/homebrew-core#74023
https://github.com/Homebrew/linuxbrew-core/pull/22752

The tricky part here is that it looks like the tap_syntax test will prevent the {home,linux}brew/core PRs from being tested until this PR is merged.

[nandahkrishna]

Thanks for resolving the conflicts, @carlocab!

[carlocab]

Being able to auto-correct patch URLs was a little useful, but it would've been nice if we could get an updated checksum with that too. I realised that while opening the {homebrew,linuxbrew}/core PRs.

[Rylan12]

You might be able to do something like this:

correct = patch_url_node.source.gsub(/\.patch/, ".diff")
patch = Resource.new "patch"
patch.url correct
patch.downloader.fetch
patch.clear_cache

# The checksum can be accessed using:
patch.cached_download.sha256

This would probably need to be done in an audit rather than a rubocop, though, as robocops don't have access to the rest of the Homebrew infrastructure (and probably shouldn't). It's probably not a great idea to have rubocop -a start downloading resources, too.

[carlocab]

This would probably need to be done in an audit rather than a rubocop

Agree. Though there's probably a more generic audit possible here: one that checks a URL's declared sha256 and whether it still matches. Thinking about it, though, I'm not sure we always want that fixed automatically with audit --online --fix.

[Rylan12]

Thinking about this a little more: in what situations would a checksum not match but the install would still succeed?

As far as I know the only time brew audit --online is really run is in the homebrew/core CI, after the install is completed. So, given that the formula installed successfully, I think it's reasonable to assume that the checksums all match. Just not sure it really helps much unless there's a use case I'm missing.

How often would you use something like this? It could be a --strict audit so a user could confirm that the checksums match without needing to install the formula. It's not something I'd find super useful, I think, but I wouldn't be opposed to a PR.

[carlocab]

Thinking about this a little more: in what situations would a checksum not match but the install would still succeed?

This never happens. The install fails as soon as there is a checksum mismatch.

How often would you use something like this?

You can use it when you've used a .patch extension for a GitLab patch but need to fix it so it's a .diff extension. Or, maybe you put in the tarball checksum wrong (maybe you were updating a formula manually, and used curl to pipe to sha256sum instead of curl -L) and want to easily update the checksum without a bunch of copying and pasting. (I've seen this a handful of times in homebrew/core.)

[Rylan12]

How about a brew command for that? It seems like less of an audit kind of thing and more like an update-python-resources kind of thing, if that makes sense.

Maybe brew update-checksums? If you really wanted, you could have a --fail-on-mismatch flag to fail when a mismatch is detected instead of writing the changes.

[carlocab]

Maybe, but I'm thinking there are still circumstances you'd want to verify the sources even without needing to install a formula. For instance, I think regularly checking whether our sources still exist and have matching checksums might be useful, instead of discovering them in PRs that test lots of formulae (e.g. Go, Python, Rust).

It would need to be a non-failing test, because these tarballs disappear / get retagged all the time. But knowing ahead of time could be helpful.

Though I guess it's also not a good idea to download 6000+ tarballs and check them in a single CI run, so maybe just waiting till we have to install something is fine.

[Rylan12]

Just thinking out loud, maybe this command could have a --deps flag to also check dependencies of a formula to help with the PR testing many formulae problem.

Could also have a --tap flag to check all formulae in a tap (probably not for much use on homebrew/core, though, as there's just a ton of resources).

It doesn't feel like the kind of thing that needs to or should happen on every PR run which is why I'm kind of pushing for a command like this rather than an audit. I wouldn't be opposed to an audit though if it seems like that makes more sense.

Also, we could have this command run once a day or once a week and post updates to an issue of all checksum mismatches to help us stay on top of things.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[Rylan12]

Any new thoughts on such a command?

Either way, I think this can be merged if we remove the autocorrection, right?

[carlocab]

Any new thoughts on such a command?

Hadn't really had the chance to think about it 😅

Either way, I think this can be merged if we remove the autocorrection, right?

And deactivate the audit temporarily so that the PRs in {home,linux}brew/core can be merged.

[carlocab]

Thanks @Rylan12. Will update my core PRs.

[Rylan12]

(Oops, more merge conflicts from #11170)

[carlocab]

https://github.com/Homebrew/linuxbrew-core/pull/22752 and Homebrew/homebrew-core#74023 updated.

[Rylan12]

Great! I've updated this branch to fix the conflicts. I've also commented this audit out with a TODO so it can be re-added once the core PRs are merged. I'll open a follow-up PR uncommenting it as soon as this is merged.

[Rylan12]

Follow-up in #11191