Merge pull request #16196 from samford/cask/rework-livecheck_min_os-audit

Use Sparkle sorting/filtering in #livecheck_min_os

Library/Homebrew/cask/audit.rb

@@ -574,7 +574,7 @@ module Cask
       debug_messages << "Plist #{plist_min_os}" if plist_min_os
       debug_messages << "Sparkle #{sparkle_min_os}" if sparkle_min_os
       odebug "Minimum OS version: #{debug_messages.join(" | ")}" unless debug_messages.empty?
-      min_os = [sparkle_min_os, plist_min_os].compact.max
+      min_os = [plist_min_os, sparkle_min_os].compact.max
 
       return if min_os.nil? || min_os <= HOMEBREW_MACOS_OLDEST_ALLOWED
 
@@ -597,31 +597,31 @@ module Cask
       return unless cask.livecheckable?
       return if cask.livecheck.strategy != :sparkle
 
-      out, _, status = curl_output("--fail", "--silent", "--location", cask.livecheck.url)
+      # `Sparkle` strategy blocks that use the `items` argument (instead of
-      return unless status.success?
+      # `item`) contain arbitrary logic that ignores/overrides the strategy's
+      # sorting, so we can't identify which item would be first/newest here.
+      return if cask.livecheck.strategy_block.present? &&
+                cask.livecheck.strategy_block.parameters[0] == [:opt, :items]
 
-      require "rexml/document"
+      content = Homebrew::Livecheck::Strategy.page_content(cask.livecheck.url)[:content]
-
+      return if content.blank?
-      xml = begin
-        REXML::Document.new(out)
-      rescue REXML::ParseException
-        nil
-      end
-
-      return if xml.blank?
-
-      item = xml.elements["//rss//channel//item"]
-      return if item.blank?
-
-      min_os = item.elements["sparkle:minimumSystemVersion"]&.text
-      min_os = "11" if min_os == "10.16"
-      return if min_os.blank?
 
       begin
-        MacOSVersion.new(min_os).strip_patch
+        items = Homebrew::Livecheck::Strategy::Sparkle.sort_items(
-      rescue MacOSVersion::Error
+          Homebrew::Livecheck::Strategy::Sparkle.filter_items(
-        nil
+            Homebrew::Livecheck::Strategy::Sparkle.items_from_content(content),
+          ),
+        )
+      rescue
+        return
       end
+      return if items.blank?
+
+      min_os = items[0]&.minimum_system_version&.strip_patch
+      # Big Sur is sometimes identified as 10.16, so we override it to the
+      # expected macOS version (11).
+      min_os = MacOSVersion.new("11") if min_os == "10.16"
+      min_os
     end
 
     sig { returns(T.nilable(MacOSVersion)) }

Library/Homebrew/livecheck/strategy/sparkle.rb

@@ -21,6 +21,9 @@ module Homebrew
         # The `Regexp` used to determine if the strategy applies to the URL.
         URL_MATCH_REGEX = %r{^https?://}i.freeze
 
+        # Common `os` values used in appcasts to refer to macOS.
+        APPCAST_MACOS_STRINGS = ["macos", "osx"].freeze
+
         # Whether the strategy can be applied to the provided URL.
         #
         # @param url [String] the URL to match against
@@ -86,19 +89,29 @@ module Homebrew
             enclosure = item.elements["enclosure"]
 
             if enclosure
-              url = enclosure["url"]
+              url = enclosure["url"].presence
-              short_version = enclosure["shortVersionString"]
+              short_version = enclosure["shortVersionString"].presence
-              version = enclosure["version"]
+              version = enclosure["version"].presence
-              os = enclosure["os"]
+              os = enclosure["os"].presence
             end
 
-            title = item.elements["title"]&.text&.strip
+            title = item.elements["title"]&.text&.strip&.presence
-            link = item.elements["link"]&.text&.strip
+            link = item.elements["link"]&.text&.strip&.presence
             url ||= link
-            channel = item.elements["channel"]&.text&.strip
+            channel = item.elements["channel"]&.text&.strip&.presence
-            release_notes_link = item.elements["releaseNotesLink"]&.text&.strip
+            release_notes_link = item.elements["releaseNotesLink"]&.text&.strip&.presence
-            short_version ||= item.elements["shortVersionString"]&.text&.strip
+            short_version ||= item.elements["shortVersionString"]&.text&.strip&.presence
-            version ||= item.elements["version"]&.text&.strip
+            version ||= item.elements["version"]&.text&.strip&.presence
+
+            minimum_system_version_text =
+              item.elements["minimumSystemVersion"]&.text&.strip&.gsub(/\A\D+|\D+\z/, "")&.presence
+            if minimum_system_version_text.present?
+              minimum_system_version = begin
+                MacOSVersion.new(minimum_system_version_text)
+              rescue MacOSVersion::Error
+                nil
+              end
+            end
 
             pub_date = item.elements["pubDate"]&.text&.strip&.presence&.then do |date_string|
               Time.parse(date_string)
@@ -114,18 +127,6 @@ module Homebrew
 
             bundle_version = BundleVersion.new(short_version, version) if short_version || version
 
-            next if os && !((os == "osx") || (os == "macos"))
-
-            if (minimum_system_version = item.elements["minimumSystemVersion"]&.text&.gsub(/\A\D+|\D+\z/, ""))
-              macos_minimum_system_version = begin
-                MacOSVersion.new(minimum_system_version).strip_patch
-              rescue MacOSVersion::Error
-                nil
-              end
-
-              next if macos_minimum_system_version&.prerelease?
-            end
-
             data = {
               title:                  title,
               link:                   link,
@@ -146,6 +147,33 @@ module Homebrew
           end.compact
         end
 
+        # Filters out items that aren't suitable for Homebrew.
+        #
+        # @param items [Array] appcast items
+        # @return [Array]
+        sig { params(items: T::Array[Item]).returns(T::Array[Item]) }
+        def self.filter_items(items)
+          items.select do |item|
+            # Omit items with an explicit `os` value that isn't macOS
+            next false if item.os && APPCAST_MACOS_STRINGS.none?(item.os)
+
+            # Omit items for prerelease macOS versions
+            next false if item.minimum_system_version&.strip_patch&.prerelease?
+
+            true
+          end.compact
+        end
+
+        # Sorts items from newest to oldest.
+        #
+        # @param items [Array] appcast items
+        # @return [Array]
+        sig { params(items: T::Array[Item]).returns(T::Array[Item]) }
+        def self.sort_items(items)
+          items.sort_by { |item| [item.pub_date, item.bundle_version] }
+               .reverse
+        end
+
         # Uses `#items_from_content` to identify versions from the Sparkle
         # appcast content or, if a block is provided, passes the content to
         # the block to handle matching.
@@ -161,7 +189,7 @@ module Homebrew
           ).returns(T::Array[String])
         }
         def self.versions_from_content(content, regex = nil, &block)
-          items = items_from_content(content).sort_by { |item| [item.pub_date, item.bundle_version] }.reverse
+          items = sort_items(filter_items(items_from_content(content)))
           return [] if items.blank?
 
           item = items.first

Library/Homebrew/test/livecheck/strategy/sparkle_spec.rb

@@ -28,6 +28,17 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
   # `Sparkle::Item` objects.
   let(:item_hashes) do
     {
+      # The 1.2.4 version is only used in tests as the basis for an item that
+      # should be excluded (after modifications).
+      v124: {
+        title:                  "Version 1.2.4",
+        release_notes_link:     "https://www.example.com/example/1.2.4.html",
+        pub_date:               "Fri, 02 Jan 2021 01:23:45 +0000",
+        url:                    "https://www.example.com/example/example-1.2.4.tar.gz",
+        short_version:          "1.2.4",
+        version:                "124",
+        minimum_system_version: "10.10",
+      },
       v123: {
         title:                  "Version 1.2.3",
         release_notes_link:     "https://www.example.com/example/1.2.3.html",
@@ -128,6 +139,16 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
       </item>
     EOS
 
+    # Set the first item in a copy of `appcast` to a bad `minimumSystemVersion`
+    # value, to test `MacOSVersion::Error` handling.
+    bad_macos_version = appcast.sub(
+      v123_item,
+      v123_item.sub(
+        /(<sparkle:minimumSystemVersion>)[^<]+?</m,
+        '\1Not a macOS version<',
+      ),
+    )
+
     # Set the first item in a copy of `appcast` to the "beta" channel, to test
     # filtering items by channel using a `strategy` block.
     beta_channel_item = appcast.sub(
@@ -155,6 +176,7 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
     {
       appcast:             appcast,
       omitted_items:       omitted_items,
+      bad_macos_version:   bad_macos_version,
       beta_channel_item:   beta_channel_item,
       no_versions_item:    no_versions_item,
       no_items:            no_items,
@@ -166,6 +188,17 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
 
   let(:items) do
     {
+      v124: Homebrew::Livecheck::Strategy::Sparkle::Item.new(
+        title:                  item_hashes[:v124][:title],
+        release_notes_link:     item_hashes[:v124][:release_notes_link],
+        pub_date:               Time.parse(item_hashes[:v124][:pub_date]),
+        url:                    item_hashes[:v124][:url],
+        bundle_version:         Homebrew::BundleVersion.new(
+          item_hashes[:v124][:short_version],
+          item_hashes[:v124][:version],
+        ),
+        minimum_system_version: MacOSVersion.new(item_hashes[:v124][:minimum_system_version]),
+      ),
       v123: Homebrew::Livecheck::Strategy::Sparkle::Item.new(
         title:                  item_hashes[:v123][:title],
         release_notes_link:     item_hashes[:v123][:release_notes_link],
@@ -175,7 +208,7 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
           item_hashes[:v123][:short_version],
           item_hashes[:v123][:version],
         ),
-        minimum_system_version: item_hashes[:v123][:minimum_system_version],
+        minimum_system_version: MacOSVersion.new(item_hashes[:v123][:minimum_system_version]),
       ),
       v122: Homebrew::Livecheck::Strategy::Sparkle::Item.new(
         title:                  item_hashes[:v122][:title],
@@ -189,7 +222,7 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
           item_hashes[:v122][:short_version],
           item_hashes[:v122][:version],
         ),
-        minimum_system_version: item_hashes[:v122][:minimum_system_version],
+        minimum_system_version: MacOSVersion.new(item_hashes[:v122][:minimum_system_version]),
       ),
       v121: Homebrew::Livecheck::Strategy::Sparkle::Item.new(
         title:                  item_hashes[:v121][:title],
@@ -201,7 +234,7 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
           item_hashes[:v121][:short_version],
           item_hashes[:v121][:version],
         ),
-        minimum_system_version: item_hashes[:v121][:minimum_system_version],
+        minimum_system_version: MacOSVersion.new(item_hashes[:v121][:minimum_system_version]),
       ),
       v120: Homebrew::Livecheck::Strategy::Sparkle::Item.new(
         title:                  item_hashes[:v120][:title],
@@ -213,7 +246,7 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
           item_hashes[:v120][:short_version],
           item_hashes[:v120][:version],
         ),
-        minimum_system_version: item_hashes[:v120][:minimum_system_version],
+        minimum_system_version: MacOSVersion.new(item_hashes[:v120][:minimum_system_version]),
       ),
     }
   end
@@ -234,6 +267,15 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
       ],
     }
 
+    bad_macos_version_item = items[:v123].clone
+    bad_macos_version_item.minimum_system_version = nil
+    item_arrays[:bad_macos_version] = [
+      bad_macos_version_item,
+      items[:v122],
+      items[:v121],
+      items[:v120],
+    ]
+
     beta_channel_item = items[:v123].clone
     beta_channel_item.channel = "beta"
     item_arrays[:beta_channel_item] = [
@@ -278,11 +320,40 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
       expect(items_from_appcast[0].short_version).to eq(item_hashes[:v123][:short_version])
       expect(items_from_appcast[0].version).to eq(item_hashes[:v123][:version])
 
+      expect(sparkle.items_from_content(xml[:bad_macos_version])).to eq(item_arrays[:bad_macos_version])
       expect(sparkle.items_from_content(xml[:beta_channel_item])).to eq(item_arrays[:beta_channel_item])
       expect(sparkle.items_from_content(xml[:no_versions_item])).to eq(item_arrays[:no_versions_item])
     end
   end
 
+  describe "::filter_items" do
+    let(:items_non_mac_os) do
+      item = items[:v124].clone
+      item.os = "not-osx-or-macos"
+      item_arrays[:appcast] + [item]
+    end
+
+    let(:items_prerelease_minimum_system_version) do
+      item = items[:v124].clone
+      item.minimum_system_version = MacOSVersion.new("100")
+      item_arrays[:appcast] + [item]
+    end
+
+    it "removes items with a non-mac OS" do
+      expect(sparkle.filter_items(items_non_mac_os)).to eq(item_arrays[:appcast])
+    end
+
+    it "removes items with a prerelease minimumSystemVersion" do
+      expect(sparkle.filter_items(items_prerelease_minimum_system_version)).to eq(item_arrays[:appcast])
+    end
+  end
+
+  describe "::sort_items" do
+    it "returns a sorted array of items" do
+      expect(sparkle.sort_items(item_arrays[:appcast])).to eq(item_arrays[:appcast_sorted])
+    end
+  end
+
   # `#versions_from_content` sorts items by `pub_date` and `bundle_version`, so
   # these tests have to account for this behavior in the expected output.
   # For example, the version 122 item doesn't have a parseable `pub_date` and