From b20e3b5e6c12324ec88937f1b74983b540ffd9ec Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Tue, 9 May 2023 23:20:54 -0400 Subject: [PATCH 1/6] Strategy: Omit unnecessary curl options `#page_headers` and `#page_content` are both now using `#curl_output` either directly or indirectly, so the default values for the `print_stdout` and `print_stderr` options should align with the existing values in `DEFAULT_CURL_OPTIONS` and can now be omitted. I ran some tests with `verbose` removed and it doesn't *appear* to have any visible effect. I can't remember if I originally set it in response to a situation that we experience or if it was a precaution (after experiencing issues with `debug`) but I think it was only the latter. If this removal causes issues, we can always add it back. --- Library/Homebrew/livecheck/strategy.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/Library/Homebrew/livecheck/strategy.rb b/Library/Homebrew/livecheck/strategy.rb index abe14e200f868..94f4f67c3ddaa 100644 --- a/Library/Homebrew/livecheck/strategy.rb +++ b/Library/Homebrew/livecheck/strategy.rb @@ -65,10 +65,7 @@ module Strategy # Baseline `curl` options used in {Strategy} methods. DEFAULT_CURL_OPTIONS = { - print_stdout: false, - print_stderr: false, debug: false, - verbose: false, timeout: CURL_PROCESS_TIMEOUT, connect_timeout: CURL_CONNECT_TIMEOUT, max_time: CURL_MAX_TIME, From 46be93537234c2e945fc656530a36dac013bdc39 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Tue, 9 May 2023 23:23:49 -0400 Subject: [PATCH 2/6] GitHub: Allow passing curl options to some methods This commit makes it possible to pass additional curl options to `GitHub#get_latest_release` and `GitHub::API#open_rest`. These options are passed on to `#curl_output` and `#curl_args` in turn. This is only the subset of `#curl_with_workarounds` and `#curl_args` options that `livecheck` currently uses. This is intended to ensure that curl behaves in a way that's appropriate for livecheck. For example, `brew livecheck --debug` output is carefully crafted but `Utils::Curl` methods print additional debug output in the middle unless we override it with `debug: false`. In this scenario, we need to be able to pass `debug: false` to `#curl_options` to prevent its debug output from printing and that's one use case that this supports. --- Library/Homebrew/utils/github.rb | 20 ++++++++++++++++++-- Library/Homebrew/utils/github/api.rb | 22 ++++++++++++++++++++-- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index 830c6c5a811c9..dc3a939d2e619 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -248,9 +248,25 @@ def self.get_release(user, repo, tag) API.open_rest(url, request_method: :GET) end - def self.get_latest_release(user, repo) + def self.get_latest_release( + user, + repo, + debug: nil, + timeout: nil, + connect_timeout: nil, + max_time: nil, + retries: nil + ) url = "#{API_URL}/repos/#{user}/#{repo}/releases/latest" - API.open_rest(url, request_method: :GET) + + curl_options = { + debug: debug, + timeout: timeout, + connect_timeout: connect_timeout, + max_time: max_time, + retries: retries, + }.compact + API.open_rest(url, request_method: :GET, **curl_options) end def self.generate_release_notes(user, repo, tag, previous_tag: nil) diff --git a/Library/Homebrew/utils/github/api.rb b/Library/Homebrew/utils/github/api.rb index d5e9bdb4ddfdd..54ea556f380ae 100644 --- a/Library/Homebrew/utils/github/api.rb +++ b/Library/Homebrew/utils/github/api.rb @@ -181,7 +181,17 @@ def self.credentials_error_message(response_headers, needed_scopes) end def self.open_rest( - url, data: nil, data_binary_path: nil, request_method: nil, scopes: [].freeze, parse_json: true + url, + data: nil, + data_binary_path: nil, + request_method: nil, + scopes: [].freeze, + parse_json: true, + debug: nil, + timeout: nil, + connect_timeout: nil, + max_time: nil, + retries: nil ) # This is a no-op if the user is opting out of using the GitHub API. return block_given? ? yield({}) : {} if Homebrew::EnvConfig.no_github_api? @@ -222,7 +232,15 @@ def self.open_rest( args += ["--dump-header", T.must(headers_tmpfile.path)] - output, errors, status = curl_output("--location", url.to_s, *args, secrets: [token]) + curl_options = { + debug: debug, + timeout: timeout, + connect_timeout: connect_timeout, + max_time: max_time, + retries: retries, + }.compact + + output, errors, status = curl_output("--location", url.to_s, *args, secrets: [token], **curl_options) output, _, http_code = output.rpartition("\n") output, _, http_code = output.rpartition("\n") if http_code == "000" headers = headers_tmpfile.read From 44c99229c2f0adc33a213e7db976f71cfe178949 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Wed, 10 May 2023 18:39:39 -0400 Subject: [PATCH 3/6] GithubLatest: Use curl options in request Passing `DEFAULT_CURL_OPTIONS` to `#get_latest_release` ensures that curl operates in a fashion that's appropriate for livecheck. The most visible effect is that this will prevent `#curl_output` from printing the command in the middle of carefully-crafted `brew livecheck --debug` output. --- Library/Homebrew/livecheck/strategy/github_latest.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/livecheck/strategy/github_latest.rb b/Library/Homebrew/livecheck/strategy/github_latest.rb index 92792059b3895..3361ed7d20caa 100644 --- a/Library/Homebrew/livecheck/strategy/github_latest.rb +++ b/Library/Homebrew/livecheck/strategy/github_latest.rb @@ -141,7 +141,11 @@ def self.find_versions(url:, regex: DEFAULT_REGEX, **_unused, &block) match_data[:url] = generated[:url] - release = GitHub.get_latest_release(generated[:username], generated[:repository]) + release = GitHub.get_latest_release( + generated[:username], + generated[:repository], + **Strategy::DEFAULT_CURL_OPTIONS, + ) versions_from_content(release, regex, &block).each do |match_text| match_data[:matches][match_text] = Version.new(match_text) end From dd51e4f1fa2ddc146f6cc3ff19e13bde53923767 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Wed, 10 May 2023 18:49:10 -0400 Subject: [PATCH 4/6] GithubLatest: Use GitHub::API_URL in generated URL This will ensure that the strategy automatically inherits any change to `API_URL` in the future. This currently doesn't make a functional difference (i.e., the URL we actually use is created in `GitHub#get_latest_release`) but it's still an improvement. --- Library/Homebrew/livecheck/strategy/github_latest.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/livecheck/strategy/github_latest.rb b/Library/Homebrew/livecheck/strategy/github_latest.rb index 3361ed7d20caa..8cb4bf67f800f 100644 --- a/Library/Homebrew/livecheck/strategy/github_latest.rb +++ b/Library/Homebrew/livecheck/strategy/github_latest.rb @@ -72,7 +72,7 @@ def self.generate_input_values(url) match = url.sub(/\.git$/i, "").match(URL_MATCH_REGEX) return values if match.blank? - values[:url] = "https://api.github.com/repos/#{match[:username]}/#{match[:repository]}/releases/latest" + values[:url] = "#{GitHub::API_URL}/repos/#{match[:username]}/#{match[:repository]}/releases/latest" values[:username] = match[:username] values[:repository] = match[:repository] From 0f4f394153803ebe6fe4cbd8aeadce0906436906 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Wed, 10 May 2023 23:06:40 -0400 Subject: [PATCH 5/6] Add type signatures to modified GitHub methods This adds type signatures to `#get_latest_release` and `#open_rest`. I'm not deeply familiar with these methods but this is what I found when looking at current usage throughout brew and confirmed by running `brew typecheck` until all the issues were resolved. This also includes changes in `GitHub::API` methods to address areas where arguments differed from the expected type. --- Library/Homebrew/utils/github.rb | 11 +++++++++++ Library/Homebrew/utils/github/api.rb | 17 ++++++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index dc3a939d2e619..0efbbd9daed2b 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -248,6 +248,17 @@ def self.get_release(user, repo, tag) API.open_rest(url, request_method: :GET) end + sig { + params( + user: String, + repo: String, + debug: T.nilable(T::Boolean), + timeout: T.nilable(T.any(Integer, Float)), + connect_timeout: T.nilable(T.any(Integer, Float)), + max_time: T.nilable(T.any(Integer, Float)), + retries: T.nilable(Integer), + ).returns(T::Hash[String, T.untyped]) + } def self.get_latest_release( user, repo, diff --git a/Library/Homebrew/utils/github/api.rb b/Library/Homebrew/utils/github/api.rb index 54ea556f380ae..787e923d76708 100644 --- a/Library/Homebrew/utils/github/api.rb +++ b/Library/Homebrew/utils/github/api.rb @@ -180,6 +180,21 @@ def self.credentials_error_message(response_headers, needed_scopes) EOS end + sig { + params( + url: T.any(String, URI::HTTPS), + data: T.nilable(T::Hash[String, T.untyped]), + data_binary_path: T.nilable(String), + request_method: T.nilable(Symbol), + scopes: Array, + parse_json: T::Boolean, + debug: T.nilable(T::Boolean), + timeout: T.nilable(T.any(Integer, Float)), + connect_timeout: T.nilable(T.any(Integer, Float)), + max_time: T.nilable(T.any(Integer, Float)), + retries: T.nilable(Integer), + ).returns(T.untyped) + } def self.open_rest( url, data: nil, @@ -280,7 +295,7 @@ def self.paginate_rest(url, additional_query_params: nil, per_page: 100) def self.open_graphql(query, variables: nil, scopes: [].freeze, raise_errors: true) data = { query: query, variables: variables } - result = open_rest("#{API_URL}/graphql", scopes: scopes, data: data, request_method: "POST") + result = open_rest("#{API_URL}/graphql", scopes: scopes, data: data, request_method: :POST) if raise_errors if result["errors"].present? From 2a51ff070f3944aa7c6015fd0d62906694998ad9 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Thu, 11 May 2023 00:59:00 -0400 Subject: [PATCH 6/6] Strategy: Add more options documentation --- Library/Homebrew/livecheck/strategy.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Library/Homebrew/livecheck/strategy.rb b/Library/Homebrew/livecheck/strategy.rb index 94f4f67c3ddaa..3e9e1bef7c032 100644 --- a/Library/Homebrew/livecheck/strategy.rb +++ b/Library/Homebrew/livecheck/strategy.rb @@ -65,10 +65,16 @@ module Strategy # Baseline `curl` options used in {Strategy} methods. DEFAULT_CURL_OPTIONS = { + # By default, `Utils::Curl` methods will print text (e.g. the command) + # when `--debug` is set. `debug` is set to `false` to ensure that this + # unwanted text doesn't appear in livecheck's debug output. debug: false, timeout: CURL_PROCESS_TIMEOUT, connect_timeout: CURL_CONNECT_TIMEOUT, max_time: CURL_MAX_TIME, + # `Utils::Curl` uses curl's `--retry` option by default but this isn't + # valuable enough in the context of livecheck to justify the additional + # effort. A zero value ensures that the `--retry` option is omitted. retries: 0, }.freeze