Use Sparkle sorting/filtering in #livecheck_min_os

[samford]

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

---

The #livecheck_min_os cask audit is intended to use a Sparkle feed's minimumSystemVersion information to ensure that the cask has a correct depends_on macos: ... value. It reimplements some of the logic in livecheck's Sparkle strategy but naively treats the first item in the appcast as newest and doesn't adhere to the Sparkle strategy's sorting/filtering.

Some appcasts are sorted from oldest to newest and some have items with out-of-order pubDate values, so naively using the first item in the audit has led to incorrect audit failures at times. As one example, this issue affected the hyperkey cask until upstream updated the appcast to sort items from newest to oldest. This issue is also mitigated by #16013, as that PR introduced logic to check the minimum system version from cask artifact plist files in addition to the existing livecheck block audit.

To be able to easily use the Sparkle strategy's Item sorting/filtering logic in the audit, I've refactored that code into #filter_items and #sort_items methods. As a side effect, this also means that the #items_from_content method doesn't filter (or sort) items anymore, which may be useful in the future (e.g., if we ever need to add the ability to pass unfiltered/unsorted items to a strategy block).

That said, this PR reimplements #livecheck_min_os to use Livecheck::Strategy#page_content to fetch the appcast content, #items_from_content to parse the appcast, and the new #filter_items and #sort_items methods. This aligns the audit with the Sparkle strategy's behavior and resolves the aforementioned issue.

---

If you wish to manually replicate this issue (for testing), you can edit the hyperkey cask to use an old snapshot of the appcast and the version/depends_on values from the previously-mentioned cask PR:

diff --git a/Casks/h/hyperkey.rb b/Casks/h/hyperkey.rb
index 59a73ad96678d1b8a70b6aea5cee151d4f5ff21a..c8766630af3cc3d5f04cbbbc70c703d70da6c1ec 100644
--- a/Casks/h/hyperkey.rb
+++ b/Casks/h/hyperkey.rb
@@ -1,6 +1,6 @@
 cask "hyperkey" do
-  version "0.23"
-  sha256 "8e8435b734737e0f08dc1278a2221e9f1a20ee75422a253b25503669a47fe9a1"
+  version "0.22"
+  sha256 "b4b78d923bb38424ceb302d595713b4541e0dd119e40aadb1e42d04d53d731b6"
 
   url "https://hyperkey.app/downloads/Hyperkey#{version}.dmg"
   name "Hyperkey"
@@ -8,12 +8,12 @@ cask "hyperkey" do
   homepage "https://hyperkey.app/"
 
   livecheck do
-    url "https://hyperkey.app/downloads/updates.xml"
+    url "https://web.archive.org/web/20230618095239id_/https://hyperkey.app/downloads/updates.xml"
     strategy :sparkle, &:short_version
   end
 
   auto_updates true
-  depends_on macos: ">= :catalina"
+  depends_on macos: ">= :high_sierra"
 
   app "Hyperkey.app"
 

On the [brew] master branch, you will also need to set plist_min_os to nil in #audit_min_os to ensure that only the #livecheck_min_os value is used. This is the simplest way of replicating the behavior before #16013 introduced a mitigation.

diff --git a/Library/Homebrew/cask/audit.rb b/Library/Homebrew/cask/audit.rb
index a01a8b136b8796ec1a5041dc8f751fcf6b0899ee..4cfceaf89715b23d2a6f6bb63789965bac303635 100644
--- a/Library/Homebrew/cask/audit.rb
+++ b/Library/Homebrew/cask/audit.rb
@@ -567,7 +567,8 @@ module Cask
 
       odebug "Auditing minimum OS version"
 
-      plist_min_os = cask_plist_min_os
+      # plist_min_os = cask_plist_min_os
+      plist_min_os = nil
       sparkle_min_os = livecheck_min_os
 
       debug_messages = []

With those changes in place, brew audit --cask --online --strict hyperkey should fail with Upstream defined :sierra as the minimum OS version and the cask defined :high_sierra (basically the same as the previous hyperkey PR). [Fair warning, archive.org is being weird and you may need to use 20230618095239 or 20230618095239im_ in the URL to get the unmodified XML (despite both being incorrect). id_ is the correct way of fetching the unmodified file but the server alternates between returning it from only one of these URLs at a time (probably due to a caching bug 🤷).]

After that, you can check out this PR branch (keeping the plist_min_os = nil change in place) and the same brew audit command should correctly pass.

---

This PR also modifies #items_from_content to use a MacOSVersion object as the Item.minimum_system_version value instead of a string. This reduces boilerplate when making comparisons (e.g., item.minimum_system_version >= :ventura) in Sparkle#filter_items, strategy blocks, and the #livecheck_min_os audit method, by removing the need to replicate the MacOSVersion error handling everywhere. I wanted to include this as part of #16158 but it was easier to do as part of the aforementioned refactoring.

Somewhat related, I've also added #presence calls to Item value assignments, to ensure that empty strings become nil. While debugging, I was surprised to find that some Item values were an empty string instead of nil, so this ensures that the values are consistent/predictable.

---

Lastly, this modifies Cask::Audit#livecheck_min_os to skip livecheck blocks with a Sparkle strategy block using the items argument (instead of item), as these ignore/override the Sparkle strategy's default sorting such that we can't predictably replicate the logic in the audit method.

To be clear, we could execute the strategy block but it will only return version strings. The source of the version information varies between strategy blocks, so it's not really feasible to correlate versions to Item objects to identify the minimumSystemVersion of the newest version. However, #cask_plist_min_os may take care of those instances, so it's not a total loss.

[MikeMcQuaid]

Thanks @samford, almost there, a few nits.

[MikeMcQuaid]

This seems to know way too much about the internals of this strategy block and would be better placed with something like a cannot_do_this? method.

[samford]

I'm not sure that I follow. Could you explain a bit more?

For context, Sparkle strategy blocks that use the items argument will categorically do something instead of or in addition to the default sorting. The usual argument is item (i.e., the first item after sorting/filtering) and we only use items when there's some issue where the first-sorted item isn't the newest version (e.g., an older version may have a more recent pubDate).

[MikeMcQuaid]

Thanks @samford!