From a28e1aa422c02c6afd674545dbbbd5efcbb02a3b Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Fri, 28 Apr 2023 17:23:30 -0400 Subject: [PATCH 1/4] livecheck: Selectively pass args to #find_versions The existing way of passing values to `#find_versions` methods in strategies leads to type issues when the Sorbet runtime is enabled. We've also recently talked about moving away from nilable args when we can specify a default value but this doesn't work if we pass in a `nil` value (like we're currently doing). This commit aims to address both of those areas by better controlling which arguments we're passing to `#find_versions`. This approach naively handles `cask`/`url` arguments by special-casing `ExtractPlist`. However, we should be checking the strategy's `#find_versions` method for a `cask` or `url` keyword parameter. The issue is that `strategy.method(:find_versions).parameters` is returning `[[:rest, :args], [:block, :blk]]` instead of the actual parameters like `[[:keyreq, :url], [:key, :regex], [:keyrest, :unused], [:block, :block]]`. --- Library/Homebrew/livecheck/livecheck.rb | 26 +++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/Library/Homebrew/livecheck/livecheck.rb b/Library/Homebrew/livecheck/livecheck.rb index e41891e39f..06eaf7bf71 100644 --- a/Library/Homebrew/livecheck/livecheck.rb +++ b/Library/Homebrew/livecheck/livecheck.rb @@ -733,13 +733,22 @@ module Homebrew end puts "Homebrew curl?: Yes" if debug && homebrew_curl.present? - strategy_data = strategy.find_versions( - url: url, + strategy_args = { regex: livecheck_regex, homebrew_curl: homebrew_curl, - cask: cask, - &livecheck_strategy_block - ) + } + # TODO: Set `cask`/`url` args based on the presence of the keyword arg + # in the strategy's `#find_versions` method once we figure out why + # `strategy.method(:find_versions).parameters` isn't working as + # expected. + if strategy_name == "ExtractPlist" + strategy_args[:cask] = cask if cask.present? + else + strategy_args[:url] = url + end + strategy_args.compact! + + strategy_data = strategy.find_versions(**strategy_args, &livecheck_strategy_block) match_version_map = strategy_data[:matches] regex = strategy_data[:regex] messages = strategy_data[:messages] @@ -912,12 +921,13 @@ module Homebrew puts if debug && strategy.blank? next if strategy.blank? - strategy_data = strategy.find_versions( + strategy_args = { url: url, regex: livecheck_regex, homebrew_curl: false, - &livecheck_strategy_block - ) + }.compact + + strategy_data = strategy.find_versions(**strategy_args, &livecheck_strategy_block) match_version_map = strategy_data[:matches] regex = strategy_data[:regex] messages = strategy_data[:messages] From 1c03018b6a0c830b615663d138a43da39fd56562 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Fri, 28 Apr 2023 23:24:34 -0400 Subject: [PATCH 2/4] Launchpad: Use DEFAULT_REGEX as default regex arg Using `DEFAULT_REGEX` as the default value of the `#find_versions` `regex` parameter allows us to tighten the type. --- Library/Homebrew/livecheck/strategy/launchpad.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/livecheck/strategy/launchpad.rb b/Library/Homebrew/livecheck/strategy/launchpad.rb index 773870f5e0..cf6518ac07 100644 --- a/Library/Homebrew/livecheck/strategy/launchpad.rb +++ b/Library/Homebrew/livecheck/strategy/launchpad.rb @@ -71,15 +71,15 @@ module Homebrew sig { params( url: String, - regex: T.nilable(Regexp), + regex: Regexp, unused: T.nilable(T::Hash[Symbol, T.untyped]), block: T.nilable(Proc), ).returns(T::Hash[Symbol, T.untyped]) } - def self.find_versions(url:, regex: nil, **unused, &block) + def self.find_versions(url:, regex: DEFAULT_REGEX, **unused, &block) generated = generate_input_values(url) - PageMatch.find_versions(url: generated[:url], regex: regex || DEFAULT_REGEX, **unused, &block) + PageMatch.find_versions(url: generated[:url], regex: regex, **unused, &block) end end end From 6597ee6fd3a21a20f6f96d5e0d3a63745dc989c5 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Sat, 29 Apr 2023 00:23:41 -0400 Subject: [PATCH 3/4] Sparkle: Account for nil content value `content` can be `nil` when a request doesn't succeed but `#versions_from_content` expects a `String` value, so we need to guard against a `nil` value like we do in other strategies. --- Library/Homebrew/livecheck/strategy/sparkle.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/Library/Homebrew/livecheck/strategy/sparkle.rb b/Library/Homebrew/livecheck/strategy/sparkle.rb index 03a34676e8..4fc45aa2c7 100644 --- a/Library/Homebrew/livecheck/strategy/sparkle.rb +++ b/Library/Homebrew/livecheck/strategy/sparkle.rb @@ -190,6 +190,7 @@ module Homebrew match_data.merge!(Strategy.page_content(url)) content = match_data.delete(:content) + return match_data if content.blank? versions_from_content(content, regex, &block).each do |version_text| match_data[:matches][version_text] = Version.new(version_text) From a2e966c186e0dcdbae0ff17b05e0f752bf7c7f8a Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Sat, 29 Apr 2023 01:07:27 -0400 Subject: [PATCH 4/4] Xml: Move require outside of #parse_xml Since we use `REXML::Document` in the type signature for `#parse_xml`, we can encounter an `uninitialized constant Homebrew::Livecheck::Strategy::Xml::REXML` error in strategies like `Sparkle` that use `Xml#parse_xml` internally when the Sorbet runtime is used. Moving the related require outside of the `#parse_xml` method and into the `Xml` strategy proper resolves this issue. --- Library/Homebrew/livecheck/strategy/xml.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/livecheck/strategy/xml.rb b/Library/Homebrew/livecheck/strategy/xml.rb index 06c9b09457..5c0816b96c 100644 --- a/Library/Homebrew/livecheck/strategy/xml.rb +++ b/Library/Homebrew/livecheck/strategy/xml.rb @@ -1,6 +1,8 @@ # typed: true # frozen_string_literal: true +require "rexml/document" + module Homebrew module Livecheck module Strategy @@ -53,8 +55,6 @@ module Homebrew # @return [REXML::Document, nil] sig { params(content: String).returns(T.nilable(REXML::Document)) } def self.parse_xml(content) - require "rexml/document" - parsing_tries = 0 begin REXML::Document.new(content)