Only checksum file downloads, not VCS downloads

[hmarr]

• 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?

---

#13358 fixes Homebrew/homebrew-cask-fonts#5855 for SVN casks (and #13354 was the initial approach), but I think the problem still exists for other VCS downloads such as Git. The special case of partial clones with sparse checkouts is handled in #13232 via ccdd45d, but for regular Git downloads I'm able to reproduce the problem.

Example cask

cask "hmarr-gitignore" do
  version :latest
  sha256 :no_check

  url "https://github.com/hmarr/dotfiles.git",
      verified: "github.com/hmarr/dotfiles",
      branch: "main"
  name "hmarr-gitignore"
  homepage "https://github.com/hmarr/dotfiles"

  artifact "gitignore_global", target: "/tmp/gitignore"
end

Output

$ brew install hmarr/test/hmarr-gitignore
==> Cloning https://github.com/hmarr/dotfiles.git
Cloning into '/Users/hmarr/Library/Caches/Homebrew/Cask/hmarr-gitignore--git'...
==> Checking out branch main
Already on 'main'
Your branch is up to date with 'origin/main'.
Warning: No checksum defined for cask 'hmarr-gitignore', skipping verification.
==> Installing Cask hmarr-gitignore
==> Moving Generic Artifact 'gitignore_global' to '/tmp/gitignore'
==> Backing Generic Artifact 'gitignore' up to '/opt/homebrew/Caskroom/hmarr-gitignore/latest/gitignore_global'
==> Removing Generic Artifact '/tmp/gitignore'
==> Purging files for version latest of Cask hmarr-gitignore
Error: Is a directory @ io_fread - /Users/hmarr/Library/Caches/Homebrew/Cask/hmarr-gitignore--git

This change uses the logic in the DownloadStrategyDetector to determine whether to calculate a checksum or not. It looks like all of the concrete DownloadStrategy implementation inherit either from AbstractFileDownloadStrategy or VCSDownloadStrategy, so I think checking for AbstractFileDownloadStrategy in the list of ancestors should be a robust approach. If there's a better way of doing this, I'd be happy to change tack, though!

$ rg '^class' Library/Homebrew/download_strategy.rb
24:class AbstractDownloadStrategy
182:class VCSDownloadStrategy < AbstractDownloadStrategy
272:class AbstractFileDownloadStrategy < AbstractDownloadStrategy
371:class CurlDownloadStrategy < AbstractFileDownloadStrategy
575:class HomebrewCurlDownloadStrategy < CurlDownloadStrategy
588:class CurlGitHubPackagesDownloadStrategy < CurlDownloadStrategy
610:class CurlApacheMirrorDownloadStrategy < CurlDownloadStrategy
648:class CurlPostDownloadStrategy < CurlDownloadStrategy
668:class NoUnzipCurlDownloadStrategy < CurlDownloadStrategy
680:class LocalBottleDownloadStrategy < AbstractFileDownloadStrategy
690:class SubversionDownloadStrategy < VCSDownloadStrategy
805:class GitDownloadStrategy < VCSDownloadStrategy
1021:class GitHubGitDownloadStrategy < GitDownloadStrategy
1106:class CVSDownloadStrategy < VCSDownloadStrategy
1188:class MercurialDownloadStrategy < VCSDownloadStrategy
1252:class BazaarDownloadStrategy < VCSDownloadStrategy
1317:class FossilDownloadStrategy < VCSDownloadStrategy
1370:class DownloadStrategyDetector

Output with this change

$ bin/brew install hmarr/test/hmarr-gitignore
==> Cloning https://github.com/hmarr/dotfiles.git
Cloning into '/Users/hmarr/Library/Caches/Homebrew/Cask/hmarr-gitignore--git'...
==> Checking out branch main
Already on 'main'
Your branch is up to date with 'origin/main'.
Warning: No checksum defined for cask 'hmarr-gitignore', skipping verification.
==> Installing Cask hmarr-gitignore
==> Moving Generic Artifact 'gitignore_global' to '/tmp/gitignore'
🍺  hmarr-gitignore was successfully installed!

Shout out to @apainintheneck and @bevanjkay for the initial work on fixing this error 🙌

[apainintheneck]

I don't think VCS downloads are currently supported with casks other than SVN and now the Git sparse checkout strategy that you're adding. Does it make sense to check for an unsupported edge case?

[hmarr]

Oh interesting, TIL! Currently, I don't think there's anything stopping you using another VCS download strategy — the sparse checkout approach doesn't do anything special to pick git, it just uses the fact that it's inferred from the .git url suffix by the DownloadStrategyDetector. (I'm new to the homebrew codebase, though, so take this with a pinch of salt).

Given it currently is possible, I think it'd be marginally better to handle this case, or to explicitly prevent use of the VCS downloaders for casks if that's the intended behaviour.

[apainintheneck]

That is definitely interesting because it's not the behavior I would have expected from reading the docs. I wonder if casks are audited for acceptable download strategy before they are submitted or if we just hope they use a supported strategy. It's also possible that the docs are outdated as well.

Given it currently is possible, I think it'd be marginally better to handle this case, or to explicitly prevent use of the VCS downloaders for casks if that's the intended behaviour.

I agree but I'd like to get other people's opinions on this too.

[hmarr]

Agreed, the svn section of the docs about Cask URLs does give the impression that only HTTP(S) and svn based downloads are supported even though in practice various download strategies seem to work.

[apainintheneck]

Just following up. There are indeed no checks made when loading casks limiting them to the download strategies mentioned in the docs (which right now are :curl, :post and :svn). That being said I was unable to find examples of any casks with other using clauses in the homebrew-casks and homebrew-cask-fonts taps. We could add a check for that but I think it would make sense to wait until after #13232 gets merged in so that we can cover that case too.

[MikeMcQuaid]

Makes sense to me, thanks @hmarr!

[MikeMcQuaid]

@apainintheneck Not sure where you stand on this one; can you elaborate further?

[apainintheneck]

I think this PR makes sense to me. It means we won't have to update this method every time a new download strategy is added which is good.

That being said the example that wasn't working is an undocumented use case. Shouldn't we either document that or add a check somewhere to enforce that? But I can open another issue about that if you want.

[MikeMcQuaid]

That being said the example that wasn't working is an undocumented use case. Shouldn't we either document that or add a check somewhere to enforce that? But I can open another issue about that if you want.

I think fine to leave undocumented and just let people fix undocumented things like they have in this PR 😁