Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use Sparkle sorting/filtering in #livecheck_min_os #16196

Merged
merged 6 commits into from
Nov 17, 2023
Merged

Use Sparkle sorting/filtering in #livecheck_min_os #16196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4f46923
Sparkle: Ensure empty strings become nil
samford Nov 8, 2023
bc2ce97
Sparkle: Move sorting/filtering into methods
samford Nov 8, 2023
d376b46
Skip items strategy blocks in #livecheck_min_os
samford Nov 8, 2023
28451bd
Use Sparkle sorting/filtering in #livecheck_min_os
samford Nov 8, 2023
491e2c4
Reorder #audit_min_os array for consistency
samford Nov 8, 2023
75ce724
Sparkle: Refactor macOS os strings into constant
samford Nov 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 22 additions & 22 deletions Library/Homebrew/cask/audit.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,7 @@
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

Check warning on line 577 in Library/Homebrew/cask/audit.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/cask/audit.rb#L577 

Added line #L577 was not covered by tests

return if min_os.nil? || min_os <= HOMEBREW_MACOS_OLDEST_ALLOWED

Expand All @@ -597,31 +597,31 @@
return unless cask.livecheckable?
return if cask.livecheck.strategy != :sparkle

out, _, status = curl_output("--fail", "--silent", "--location", cask.livecheck.url)
return unless status.success?
# `Sparkle` strategy blocks that use the `items` argument (instead of
# `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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


require "rexml/document"

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?
content = Homebrew::Livecheck::Strategy.page_content(cask.livecheck.url)[:content]

Check warning on line 606 in Library/Homebrew/cask/audit.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/cask/audit.rb#L606 

Added line #L606 was not covered by tests
return if content.blank?

begin
MacOSVersion.new(min_os).strip_patch
rescue MacOSVersion::Error
nil
items = Homebrew::Livecheck::Strategy::Sparkle.sort_items(

Check warning on line 610 in Library/Homebrew/cask/audit.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/cask/audit.rb#L610 

Added line #L610 was not covered by tests
Homebrew::Livecheck::Strategy::Sparkle.filter_items(
Homebrew::Livecheck::Strategy::Sparkle.items_from_content(content),
),
)
rescue
return

Check warning on line 616 in Library/Homebrew/cask/audit.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/cask/audit.rb#L616 

Added line #L616 was not covered by tests
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

Check warning on line 624 in Library/Homebrew/cask/audit.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/cask/audit.rb#L624 

Added line #L624 was not covered by tests
end

sig { returns(T.nilable(MacOSVersion)) }
Expand Down
74 changes: 51 additions & 23 deletions Library/Homebrew/livecheck/strategy/sparkle.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
# 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
Expand Down Expand Up @@ -86,19 +89,29 @@
enclosure = item.elements["enclosure"]

if enclosure
url = enclosure["url"]
short_version = enclosure["shortVersionString"]
version = enclosure["version"]
os = enclosure["os"]
url = enclosure["url"].presence
short_version = enclosure["shortVersionString"].presence
version = enclosure["version"].presence
os = enclosure["os"].presence
end

title = item.elements["title"]&.text&.strip
link = item.elements["link"]&.text&.strip
title = item.elements["title"]&.text&.strip&.presence
samford marked this conversation as resolved.
Show resolved Hide resolved
link = item.elements["link"]&.text&.strip&.presence
url ||= link
channel = item.elements["channel"]&.text&.strip
release_notes_link = item.elements["releaseNotesLink"]&.text&.strip
short_version ||= item.elements["shortVersionString"]&.text&.strip
version ||= item.elements["version"]&.text&.strip
channel = item.elements["channel"]&.text&.strip&.presence
release_notes_link = item.elements["releaseNotesLink"]&.text&.strip&.presence
short_version ||= item.elements["shortVersionString"]&.text&.strip&.presence
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

Check warning on line 112 in Library/Homebrew/livecheck/strategy/sparkle.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/livecheck/strategy/sparkle.rb#L112 

Added line #L112 was not covered by tests
end
end

pub_date = item.elements["pubDate"]&.text&.strip&.presence&.then do |date_string|
Time.parse(date_string)
Expand All @@ -114,18 +127,6 @@

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,
Expand All @@ -146,6 +147,33 @@
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.
Expand All @@ -161,7 +189,7 @@
).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
Expand Down
79 changes: 75 additions & 4 deletions Library/Homebrew/test/livecheck/strategy/sparkle_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,17 @@ def create_appcast_xml(items_str = "")
# `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",
Expand Down Expand Up @@ -128,6 +139,16 @@ def create_appcast_xml(items_str = "")
</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(
Expand Down Expand Up @@ -155,6 +176,7 @@ def create_appcast_xml(items_str = "")
{
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,
Expand All @@ -166,6 +188,17 @@ def create_appcast_xml(items_str = "")

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],
Expand All @@ -175,7 +208,7 @@ def create_appcast_xml(items_str = "")
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],
Expand All @@ -189,7 +222,7 @@ def create_appcast_xml(items_str = "")
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],
Expand All @@ -201,7 +234,7 @@ def create_appcast_xml(items_str = "")
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],
Expand All @@ -213,7 +246,7 @@ def create_appcast_xml(items_str = "")
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
Expand All @@ -234,6 +267,15 @@ def create_appcast_xml(items_str = "")
],
}

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] = [
Expand Down Expand Up @@ -278,11 +320,40 @@ def create_appcast_xml(items_str = "")
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
Expand Down