From 24c9b599a6e7269504bfa92d76552792321f4b20 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Wed, 24 Mar 2021 10:55:33 +0100 Subject: [PATCH 1/7] Revert "Revert "Merge pull request #10864 from reitermarkus/command-timeout"" This reverts commit 57b2660cae8cf43efd2b88d84a7174706328c001. --- Library/Homebrew/cask/download.rb | 9 +- Library/Homebrew/cask/installer.rb | 17 +- Library/Homebrew/download_strategy.rb | 189 ++++++++++++++--------- Library/Homebrew/extend/time.rb | 18 +++ Library/Homebrew/sorbet/plugins/using.rb | 12 ++ Library/Homebrew/system_command.rb | 31 +++- Library/Homebrew/utils/curl.rb | 26 +++- 7 files changed, 208 insertions(+), 94 deletions(-) create mode 100644 Library/Homebrew/extend/time.rb diff --git a/Library/Homebrew/cask/download.rb b/Library/Homebrew/cask/download.rb index a5b3d31f74..4c80eb2628 100644 --- a/Library/Homebrew/cask/download.rb +++ b/Library/Homebrew/cask/download.rb @@ -19,9 +19,10 @@ module Cask @quarantine = quarantine end - def fetch(verify_download_integrity: true) + def fetch(quiet: nil, verify_download_integrity: true, timeout: nil) downloaded_path = begin - downloader.fetch + downloader.shutup! if quiet + downloader.fetch(timeout: timeout) downloader.cached_location rescue => e error = CaskError.new("Download failed on Cask '#{cask}' with message: #{e}") @@ -40,8 +41,8 @@ module Cask end end - def time_file_size - downloader.resolved_time_file_size + def time_file_size(timeout: nil) + downloader.resolved_time_file_size(timeout: timeout) end def clear_cache diff --git a/Library/Homebrew/cask/installer.rb b/Library/Homebrew/cask/installer.rb index 4b50d16279..4b1395786b 100644 --- a/Library/Homebrew/cask/installer.rb +++ b/Library/Homebrew/cask/installer.rb @@ -62,13 +62,15 @@ module Cask EOS end - def fetch + sig { params(quiet: T.nilable(T::Boolean), timeout: T.nilable(T.any(Integer, Float))).void } + def fetch(quiet: nil, timeout: nil) odebug "Cask::Installer#fetch" verify_has_sha if require_sha? && !force? - satisfy_dependencies - download + download(quiet: quiet, timeout: timeout) + + satisfy_dependencies end def stage @@ -162,9 +164,10 @@ module Cask @downloader ||= Download.new(@cask, quarantine: quarantine?) end - sig { returns(Pathname) } - def download - @download ||= downloader.fetch(verify_download_integrity: @verify_download_integrity) + sig { params(quiet: T.nilable(T::Boolean), timeout: T.nilable(T.any(Integer, Float))).returns(Pathname) } + def download(quiet: nil, timeout: nil) + @download ||= downloader.fetch(quiet: quiet, verify_download_integrity: @verify_download_integrity, +timeout: timeout) end def verify_has_sha @@ -179,7 +182,7 @@ module Cask def primary_container @primary_container ||= begin - downloaded_path = download + downloaded_path = download(quiet: true) UnpackStrategy.detect(downloaded_path, type: @cask.container&.type, merge_xattrs: true) end end diff --git a/Library/Homebrew/download_strategy.rb b/Library/Homebrew/download_strategy.rb index 088dbb6b71..6eb002d824 100644 --- a/Library/Homebrew/download_strategy.rb +++ b/Library/Homebrew/download_strategy.rb @@ -15,6 +15,9 @@ require "utils/curl" require "github_packages" +require "extend/time" +using TimeRemaining + # @abstract Abstract superclass for all download strategies. # # @api private @@ -52,7 +55,7 @@ class AbstractDownloadStrategy # Download and cache the resource at {#cached_location}. # # @api public - def fetch; end + def fetch(timeout: nil); end # Disable any output during downloading. # @@ -176,18 +179,20 @@ class VCSDownloadStrategy < AbstractDownloadStrategy # Download and cache the repository at {#cached_location}. # # @api public - def fetch + def fetch(timeout: nil) + end_time = Time.now + timeout if timeout + ohai "Cloning #{url}" if cached_location.exist? && repo_valid? puts "Updating #{cached_location}" - update + update(timeout: timeout) elsif cached_location.exist? puts "Removing invalid repository from cache" clear_cache - clone_repo + clone_repo(timeout: end_time) else - clone_repo + clone_repo(timeout: end_time) end version.update_commit(last_commit) if head? @@ -232,9 +237,11 @@ class VCSDownloadStrategy < AbstractDownloadStrategy raise NotImplementedError end - def clone_repo; end + sig { params(timeout: T.nilable(Time)).void } + def clone_repo(timeout: nil); end - def update; end + sig { params(timeout: T.nilable(Time)).void } + def update(timeout: nil); end def current_revision; end @@ -353,7 +360,9 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy # Download and cache the file at {#cached_location}. # # @api public - def fetch + def fetch(timeout: nil) + end_time = Time.now + timeout if timeout + download_lock = LockFile.new(temporary_path.basename) download_lock.lock @@ -364,7 +373,7 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy ohai "Downloading #{url}" - resolved_url, _, url_time, = resolve_url_basename_time_file_size(url) + resolved_url, _, url_time, = resolve_url_basename_time_file_size(url, timeout: end_time&.remaining!) fresh = if cached_location.exist? && url_time url_time <= cached_location.mtime @@ -378,7 +387,7 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy puts "Already downloaded: #{cached_location}" else begin - _fetch(url: url, resolved_url: resolved_url) + _fetch(url: url, resolved_url: resolved_url, timeout: end_time&.remaining!) rescue ErrorDuringExecution raise CurlDownloadStrategyError, url end @@ -395,6 +404,8 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy puts "Trying a mirror..." retry + rescue Timeout::Error => e + raise Timeout::Error, "Timed out downloading #{self.url}: #{e}" end ensure download_lock&.unlock @@ -406,19 +417,19 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy rm_rf(temporary_path) end - def resolved_time_file_size - _, _, time, file_size = resolve_url_basename_time_file_size(url) + def resolved_time_file_size(timeout: nil) + _, _, time, file_size = resolve_url_basename_time_file_size(url, timeout: timeout) [time, file_size] end private - def resolved_url_and_basename - resolved_url, basename, = resolve_url_basename_time_file_size(url) + def resolved_url_and_basename(timeout: nil) + resolved_url, basename, = resolve_url_basename_time_file_size(url, timeout: nil) [resolved_url, basename] end - def resolve_url_basename_time_file_size(url) + def resolve_url_basename_time_file_size(url, timeout: nil) @resolved_info_cache ||= {} return @resolved_info_cache[url] if @resolved_info_cache.include?(url) @@ -426,7 +437,7 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy url = url.sub(%r{^((ht|f)tps?://)?}, "#{domain.chomp("/")}/") end - out, _, status = curl_output("--location", "--silent", "--head", "--request", "GET", url.to_s) + out, _, status= curl_output("--location", "--silent", "--head", "--request", "GET", url.to_s, timeout: timeout) lines = status.success? ? out.lines.map(&:chomp) : [] @@ -485,7 +496,7 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy @resolved_info_cache[url] = [redirect_url, basename, time, file_size] end - def _fetch(url:, resolved_url:) + def _fetch(url:, resolved_url:, timeout:) ohai "Downloading from #{resolved_url}" if url != resolved_url if Homebrew::EnvConfig.no_insecure_redirect? && @@ -494,7 +505,7 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy raise CurlDownloadStrategyError, url end - curl_download resolved_url, to: temporary_path + curl_download resolved_url, to: temporary_path, timeout: timeout end # Curl options to be always passed to curl, @@ -567,9 +578,9 @@ class CurlApacheMirrorDownloadStrategy < CurlDownloadStrategy @combined_mirrors = [*@mirrors, *backup_mirrors] end - def resolve_url_basename_time_file_size(url) + def resolve_url_basename_time_file_size(url, timeout: nil) if url == self.url - super("#{apache_mirrors["preferred"]}#{apache_mirrors["path_info"]}") + super("#{apache_mirrors["preferred"]}#{apache_mirrors["path_info"]}", timeout: timeout) else super end @@ -592,7 +603,7 @@ end class CurlPostDownloadStrategy < CurlDownloadStrategy private - def _fetch(url:, resolved_url:) + def _fetch(url:, resolved_url:, timeout:) args = if meta.key?(:data) escape_data = ->(d) { ["-d", URI.encode_www_form([d])] } [url, *meta[:data].flat_map(&escape_data)] @@ -601,7 +612,7 @@ class CurlPostDownloadStrategy < CurlDownloadStrategy query.nil? ? [url, "-X", "POST"] : [url, "-d", query] end - curl_download(*args, to: temporary_path) + curl_download(*args, to: temporary_path, timeout: timeout) end end @@ -641,7 +652,7 @@ class SubversionDownloadStrategy < VCSDownloadStrategy # Download and cache the repository at {#cached_location}. # # @api public - def fetch + def fetch(timeout: nil) if @url.chomp("/") != repo_url || !silent_command("svn", args: ["switch", @url, cached_location]).success? clear_cache end @@ -682,7 +693,11 @@ class SubversionDownloadStrategy < VCSDownloadStrategy end end - def fetch_repo(target, url, revision = nil, ignore_externals: false) + sig { + params(target: Pathname, url: String, revision: T.nilable(String), ignore_externals: T::Boolean, + timeout: T.nilable(Time)).void + } + def fetch_repo(target, url, revision = nil, ignore_externals: false, timeout: nil) # Use "svn update" when the repository already exists locally. # This saves on bandwidth and will have a similar effect to verifying the # cache as it will make any changes to get the right revision. @@ -702,9 +717,9 @@ class SubversionDownloadStrategy < VCSDownloadStrategy end if target.directory? - command!("svn", args: ["update", *args], chdir: target.to_s) + command! "svn", args: ["update", *args], chdir: target.to_s, timeout: timeout&.remaining else - command!("svn", args: ["checkout", url, target, *args]) + command! "svn", args: ["checkout", url, target, *args], timeout: timeout&.remaining end end @@ -717,20 +732,22 @@ class SubversionDownloadStrategy < VCSDownloadStrategy (cached_location/".svn").directory? end - def clone_repo + sig { params(timeout: T.nilable(Time)).void } + def clone_repo(timeout: nil) case @ref_type when :revision - fetch_repo cached_location, @url, @ref + fetch_repo cached_location, @url, @ref, timeout: timeout when :revisions # nil is OK for main_revision, as fetch_repo will then get latest main_revision = @ref[:trunk] - fetch_repo cached_location, @url, main_revision, ignore_externals: true + fetch_repo cached_location, @url, main_revision, ignore_externals: true, timeout: timeout externals do |external_name, external_url| - fetch_repo cached_location/external_name, external_url, @ref[external_name], ignore_externals: true + fetch_repo cached_location/external_name, external_url, @ref[external_name], ignore_externals: true, + timeout: timeout end else - fetch_repo cached_location, @url + fetch_repo cached_location, @url, timeout: timeout end end alias update clone_repo @@ -779,12 +796,13 @@ class GitDownloadStrategy < VCSDownloadStrategy 0 end - def update + sig { params(timeout: T.nilable(Time)).void } + def update(timeout: nil) config_repo - update_repo - checkout + update_repo(timeout: timeout) + checkout(timeout: timeout) reset - update_submodules if submodules? + update_submodules(timeout: timeout) if submodules? end def shallow_clone? @@ -845,6 +863,7 @@ class GitDownloadStrategy < VCSDownloadStrategy end end + sig { void } def config_repo command! "git", args: ["config", "remote.origin.url", @url], @@ -857,35 +876,42 @@ class GitDownloadStrategy < VCSDownloadStrategy chdir: cached_location end - def update_repo + sig { params(timeout: T.nilable(Time)).void } + def update_repo(timeout: nil) return if @ref_type != :branch && ref? if !shallow_clone? && shallow_dir? command! "git", - args: ["fetch", "origin", "--unshallow"], - chdir: cached_location + args: ["fetch", "origin", "--unshallow"], + chdir: cached_location, + timeout: timeout&.remaining else command! "git", - args: ["fetch", "origin"], - chdir: cached_location + args: ["fetch", "origin"], + chdir: cached_location, + timeout: timeout&.remaining end end - def clone_repo - command! "git", args: clone_args + sig { params(timeout: T.nilable(Time)).void } + def clone_repo(timeout: nil) + command! "git", args: clone_args, timeout: timeout&.remaining command! "git", - args: ["config", "homebrew.cacheversion", cache_version], - chdir: cached_location - checkout - update_submodules if submodules? + args: ["config", "homebrew.cacheversion", cache_version], + chdir: cached_location, + timeout: timeout&.remaining + checkout(timeout: timeout) + update_submodules(timeout: timeout) if submodules? end - def checkout + sig { params(timeout: T.nilable(Time)).void } + def checkout(timeout: nil) ohai "Checking out #{@ref_type} #{@ref}" if @ref_type && @ref - command! "git", args: ["checkout", "-f", @ref, "--"], chdir: cached_location + command! "git", args: ["checkout", "-f", @ref, "--"], chdir: cached_location, timeout: timeout&.remaining end + sig { void } def reset ref = case @ref_type when :branch @@ -899,13 +925,16 @@ class GitDownloadStrategy < VCSDownloadStrategy chdir: cached_location end - def update_submodules + sig { params(timeout: T.nilable(Time)).void } + def update_submodules(timeout: nil) command! "git", - args: ["submodule", "foreach", "--recursive", "git submodule sync"], - chdir: cached_location + args: ["submodule", "foreach", "--recursive", "git submodule sync"], + chdir: cached_location, + timeout: timeout&.remaining command! "git", - args: ["submodule", "update", "--init", "--recursive"], - chdir: cached_location + args: ["submodule", "update", "--init", "--recursive"], + chdir: cached_location, + timeout: timeout&.remaining fix_absolute_submodule_gitdir_references! end @@ -1057,19 +1086,23 @@ class CVSDownloadStrategy < VCSDownloadStrategy "-Q" unless verbose? end - def clone_repo + sig { params(timeout: T.nilable(Time)).void } + def clone_repo(timeout: nil) # Login is only needed (and allowed) with pserver; skip for anoncvs. - command! "cvs", args: [*quiet_flag, "-d", @url, "login"] if @url.include? "pserver" + command! "cvs", args: [*quiet_flag, "-d", @url, "login"], timeout: timeout&.remaining if @url.include? "pserver" command! "cvs", - args: [*quiet_flag, "-d", @url, "checkout", "-d", cached_location.basename, @module], - chdir: cached_location.dirname + args: [*quiet_flag, "-d", @url, "checkout", "-d", cached_location.basename, @module], + chdir: cached_location.dirname, + timeout: timeout&.remaining end - def update + sig { params(timeout: T.nilable(Time)).void } + def update(timeout: nil) command! "cvs", - args: [*quiet_flag, "update"], - chdir: cached_location + args: [*quiet_flag, "update"], + chdir: cached_location, + timeout: timeout&.remaining end def split_url(in_url) @@ -1121,12 +1154,14 @@ class MercurialDownloadStrategy < VCSDownloadStrategy (cached_location/".hg").directory? end - def clone_repo - command! "hg", args: ["clone", @url, cached_location] + sig { params(timeout: T.nilable(Time)).void } + def clone_repo(timeout: nil) + command! "hg", args: ["clone", @url, cached_location], timeout: timeout&.remaining end - def update - command! "hg", args: ["--cwd", cached_location, "pull", "--update"] + sig { params(timeout: T.nilable(Time)).void } + def update(timeout: nil) + command! "hg", args: ["--cwd", cached_location, "pull", "--update"], timeout: timeout&.remaining update_args = if @ref_type && @ref ohai "Checking out #{@ref_type} #{@ref}" @@ -1135,7 +1170,7 @@ class MercurialDownloadStrategy < VCSDownloadStrategy ["--clean"] end - command! "hg", args: ["--cwd", cached_location, "update", *update_args] + command! "hg", args: ["--cwd", cached_location, "update", *update_args], timeout: timeout&.remaining end end @@ -1184,16 +1219,20 @@ class BazaarDownloadStrategy < VCSDownloadStrategy (cached_location/".bzr").directory? end - def clone_repo + sig { params(timeout: T.nilable(Time)).void } + def clone_repo(timeout: nil) # "lightweight" means history-less command! "bzr", - args: ["checkout", "--lightweight", @url, cached_location] + args: ["checkout", "--lightweight", @url, cached_location], + timeout: timeout&.remaining end - def update + sig { params(timeout: T.nilable(Time)).void } + def update(timeout: nil) command! "bzr", - args: ["update"], - chdir: cached_location + args: ["update"], + chdir: cached_location, + timeout: timeout&.remaining end end @@ -1236,12 +1275,14 @@ class FossilDownloadStrategy < VCSDownloadStrategy "fossil" end - def clone_repo - silent_command!("fossil", args: ["clone", @url, cached_location]) + sig { params(timeout: T.nilable(Time)).void } + def clone_repo(timeout: nil) + silent_command! "fossil", args: ["clone", @url, cached_location], timeout: timeout&.remaining end - def update - silent_command!("fossil", args: ["pull", "-R", cached_location]) + sig { params(timeout: T.nilable(Time)).void } + def update(timeout: nil) + silent_command! "fossil", args: ["pull", "-R", cached_location], timeout: timeout&.remaining end end diff --git a/Library/Homebrew/extend/time.rb b/Library/Homebrew/extend/time.rb new file mode 100644 index 0000000000..81779cd8dc --- /dev/null +++ b/Library/Homebrew/extend/time.rb @@ -0,0 +1,18 @@ +# typed: false +# frozen_string_literal: true + +module TimeRemaining + refine Time do + def remaining + [0, self - Time.now].max + end + + def remaining! + r = remaining + + raise Timeout::Error if r <= 0 + + r + end + end +end diff --git a/Library/Homebrew/sorbet/plugins/using.rb b/Library/Homebrew/sorbet/plugins/using.rb index e1bf761b29..288c4c6c44 100644 --- a/Library/Homebrew/sorbet/plugins/using.rb +++ b/Library/Homebrew/sorbet/plugins/using.rb @@ -28,4 +28,16 @@ when "HashValidator" def assert_valid_keys!(*valid_keys); end end RUBY +when "TimeRemaining" + puts <<-RUBY + # typed: strict + + class ::Time + sig { returns(T.any(Integer, Float)) } + def remaining; end + + sig { returns(T.any(Integer, Float)) } + def remaining!; end + end + RUBY end diff --git a/Library/Homebrew/system_command.rb b/Library/Homebrew/system_command.rb index 96e8b542df..ea44cf25db 100644 --- a/Library/Homebrew/system_command.rb +++ b/Library/Homebrew/system_command.rb @@ -10,13 +10,15 @@ require "extend/io" require "extend/predicable" require "extend/hash_validator" +require "extend/time" + # Class for running sub-processes and capturing their output and exit status. # # @api private class SystemCommand extend T::Sig - using HashValidator + using TimeRemaining # Helper functions for calling {SystemCommand.run}. module Mixin @@ -78,10 +80,24 @@ class SystemCommand verbose: T.nilable(T::Boolean), secrets: T.any(String, T::Array[String]), chdir: T.any(String, Pathname), + timeout: T.nilable(T.any(Integer, Float)), ).void } - def initialize(executable, args: [], sudo: false, env: {}, input: [], must_succeed: false, - print_stdout: false, print_stderr: true, debug: nil, verbose: nil, secrets: [], chdir: T.unsafe(nil)) + def initialize( + executable, + args: [], + sudo: false, + env: {}, + input: [], + must_succeed: false, + print_stdout: false, + print_stderr: true, + debug: nil, + verbose: false, + secrets: [], + chdir: T.unsafe(nil), + timeout: nil + ) require "extend/ENV" @executable = executable @args = args @@ -100,6 +116,7 @@ class SystemCommand @verbose = verbose @secrets = (Array(secrets) + ENV.sensitive_environment.values).uniq @chdir = chdir + @timeout = timeout end sig { returns(T::Array[String]) } @@ -197,9 +214,12 @@ class SystemCommand sig { params(sources: T::Array[IO], _block: T.proc.params(type: Symbol, line: String).void).void } def each_line_from(sources, &_block) + end_time = Time.now + @timeout if @timeout sources_remaining = sources.dup - while sources_remaining.present? - readable_sources, = IO.select(sources_remaining) + + loop do + readable_sources, = IO.select(sources_remaining, [], [], end_time&.remaining!) + raise Timeout::Error if readable_sources.nil? readable_sources = T.must(readable_sources) break if readable_sources.empty? @@ -211,6 +231,7 @@ class SystemCommand rescue EOFError source.close_read sources_remaining.delete(source) + return if sources_remaining.empty? rescue IO::WaitReadable next end diff --git a/Library/Homebrew/utils/curl.rb b/Library/Homebrew/utils/curl.rb index 8e2a798ed3..f6d7cbea75 100644 --- a/Library/Homebrew/utils/curl.rb +++ b/Library/Homebrew/utils/curl.rb @@ -3,11 +3,15 @@ require "open3" +require "extend/time" + module Utils # Helper function for interacting with `curl`. # # @api private module Curl + using TimeRemaining + module_function def curl_executable @@ -49,14 +53,20 @@ module Utils args << "--silent" unless $stdout.tty? end + args << "--connect-timeout" << connect_timeout.round(3) if options[:connect_timeout] + args << "--max-time" << max_time.round(3) if options[:max_time] args << "--retry" << Homebrew::EnvConfig.curl_retries unless options[:retry] == false + args << "--retry-max-time" << retry_max_time.round if options[:retry_max_time] args + extra_args end def curl_with_workarounds( - *args, secrets: nil, print_stdout: nil, print_stderr: nil, debug: nil, verbose: nil, env: {}, **options + *args, + secrets: nil, print_stdout: nil, print_stderr: nil, debug: nil, verbose: nil, env: {}, timeout: nil, **options ) + end_time = Time.now + timeout if timeout + command_options = { secrets: secrets, print_stdout: print_stdout, @@ -68,14 +78,22 @@ module Utils # SSL_CERT_FILE can be incorrectly set by users or portable-ruby and screw # with SSL downloads so unset it here. result = system_command curl_executable, - args: curl_args(*args, **options), - env: { "SSL_CERT_FILE" => nil }.merge(env), + args: curl_args(*args, **options), + env: { "SSL_CERT_FILE" => nil }.merge(env), + timeout: end_time&.remaining, **command_options return result if result.success? || !args.exclude?("--http1.1") + raise Timeout::Error, result.stderr.chomp if result.status.exitstatus == 28 + # Error in the HTTP2 framing layer - return curl_with_workarounds(*args, "--http1.1", **command_options, **options) if result.status.exitstatus == 16 + if result.status.exitstatus == 16 + return curl_with_workarounds( + *args, "--http1.1", + timeout: end_time&.remaining, **command_options, **options + ) + end # This is a workaround for https://github.com/curl/curl/issues/1618. if result.status.exitstatus == 56 # Unexpected EOF From cdcd216237276fac07e6934e31eaeca5c3944f23 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Wed, 24 Mar 2021 10:55:38 +0100 Subject: [PATCH 2/7] Revert "Revert "Merge pull request #10898 from reitermarkus/audit-timeout"" This reverts commit 0b8a9bc1a14d8513ab57423fe028d72a52b38b3d. --- Library/Homebrew/utils/curl.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/utils/curl.rb b/Library/Homebrew/utils/curl.rb index f6d7cbea75..0a25609653 100644 --- a/Library/Homebrew/utils/curl.rb +++ b/Library/Homebrew/utils/curl.rb @@ -85,7 +85,7 @@ module Utils return result if result.success? || !args.exclude?("--http1.1") - raise Timeout::Error, result.stderr.chomp if result.status.exitstatus == 28 + raise Timeout::Error, result.stderr.lines.last.chomp if timeout && result.status.exitstatus == 28 # Error in the HTTP2 framing layer if result.status.exitstatus == 16 @@ -176,9 +176,12 @@ module Utils hash_needed = false if url != secure_url user_agents.each do |user_agent| - secure_details = + secure_details = begin curl_http_content_headers_and_checksum(secure_url, specs: specs, hash_needed: true, user_agent: user_agent) + rescue Timeout::Error + next + end next unless http_status_ok?(secure_details[:status]) From b6ed8915e5688de8cd782eb2084d483458d8ced1 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Wed, 24 Mar 2021 09:04:49 +0100 Subject: [PATCH 3/7] Add compatibility layer for `AbstractDownloadStrategy#fetch`. --- Library/Homebrew/compat.rb | 2 ++ Library/Homebrew/compat/download_strategy.rb | 26 ++++++++++++++++++++ Library/Homebrew/dev-cmd/pr-pull.rb | 5 ++-- Library/Homebrew/extend/pathname.rb | 1 + Library/Homebrew/global.rb | 10 +++----- Library/Homebrew/utils.rb | 5 ++-- docs/Formula-Cookbook.md | 4 ++- 7 files changed, 42 insertions(+), 11 deletions(-) create mode 100644 Library/Homebrew/compat/download_strategy.rb diff --git a/Library/Homebrew/compat.rb b/Library/Homebrew/compat.rb index b4806eee61..a4e5ff6ae5 100644 --- a/Library/Homebrew/compat.rb +++ b/Library/Homebrew/compat.rb @@ -1,2 +1,4 @@ # typed: strict # frozen_string_literal: true + +require "compat/download_strategy" diff --git a/Library/Homebrew/compat/download_strategy.rb b/Library/Homebrew/compat/download_strategy.rb new file mode 100644 index 0000000000..2b098f01b2 --- /dev/null +++ b/Library/Homebrew/compat/download_strategy.rb @@ -0,0 +1,26 @@ +# typed: false +# frozen_string_literal: true + +class AbstractDownloadStrategy + module Compat + def fetch(timeout: nil) + super() + end + end + + class << self + def method_added(method) + if method == :fetch && instance_method(method).arity.zero? + odeprecated "`def fetch` in a subclass of #{self}", + "`def fetch(timeout: nil, **options)` and output a warning " \ + "when `options` contains new unhandled options" + + class_eval do + prepend Compat + end + end + + super + end + end +end diff --git a/Library/Homebrew/dev-cmd/pr-pull.rb b/Library/Homebrew/dev-cmd/pr-pull.rb index 29d120955b..bfa1962d39 100644 --- a/Library/Homebrew/dev-cmd/pr-pull.rb +++ b/Library/Homebrew/dev-cmd/pr-pull.rb @@ -455,7 +455,7 @@ end class GitHubArtifactDownloadStrategy < AbstractFileDownloadStrategy extend T::Sig - def fetch + def fetch(timeout: nil) ohai "Downloading #{url}" if cached_location.exist? puts "Already downloaded: #{cached_location}" @@ -463,7 +463,8 @@ class GitHubArtifactDownloadStrategy < AbstractFileDownloadStrategy begin curl "--location", "--create-dirs", "--output", temporary_path, url, *meta.fetch(:curl_args, []), - secrets: meta.fetch(:secrets, []) + secrets: meta.fetch(:secrets, []), + timeout: timeout rescue ErrorDuringExecution raise CurlDownloadStrategyError, url end diff --git a/Library/Homebrew/extend/pathname.rb b/Library/Homebrew/extend/pathname.rb index 7a9c013df6..6533d4dc2e 100644 --- a/Library/Homebrew/extend/pathname.rb +++ b/Library/Homebrew/extend/pathname.rb @@ -1,6 +1,7 @@ # typed: true # frozen_string_literal: true +require "context" require "resource" require "metafiles" diff --git a/Library/Homebrew/global.rb b/Library/Homebrew/global.rb index 1a9c16c045..ffb5024b1b 100644 --- a/Library/Homebrew/global.rb +++ b/Library/Homebrew/global.rb @@ -4,6 +4,7 @@ require_relative "load_path" require "English" +require "fileutils" require "json" require "json/add/exception" require "pathname" @@ -39,8 +40,6 @@ ActiveSupport::Inflector.inflections(:en) do |inflect| inflect.irregular "it", "they" end -require "utils/sorbet" - HOMEBREW_BOTTLE_DEFAULT_DOMAIN = ENV["HOMEBREW_BOTTLE_DEFAULT_DOMAIN"] HOMEBREW_BREW_DEFAULT_GIT_REMOTE = ENV["HOMEBREW_BREW_DEFAULT_GIT_REMOTE"] HOMEBREW_CORE_DEFAULT_GIT_REMOTE = ENV["HOMEBREW_CORE_DEFAULT_GIT_REMOTE"] @@ -73,10 +72,11 @@ HOMEBREW_PULL_OR_COMMIT_URL_REGEX = %r[https://github\.com/([\w-]+)/([\w-]+)?/(?:pull/(\d+)|commit/[0-9a-fA-F]{4,40})].freeze HOMEBREW_BOTTLES_EXTNAME_REGEX = /\.([a-z0-9_]+)\.bottle\.(?:(\d+)\.)?tar\.gz$/.freeze -require "fileutils" +require "utils/sorbet" -require "os" require "env_config" +require "compat" unless Homebrew::EnvConfig.no_compat? +require "os" require "messages" module Homebrew @@ -152,7 +152,5 @@ require "official_taps" require "tap" require "tap_constants" -require "compat" unless Homebrew::EnvConfig.no_compat? - # Enables `patchelf.rb` write support. HOMEBREW_PATCHELF_RB_WRITE = ENV["HOMEBREW_NO_PATCHELF_RB_WRITE"].blank?.freeze diff --git a/Library/Homebrew/utils.rb b/Library/Homebrew/utils.rb index cc1bf9c445..83bae0d40d 100644 --- a/Library/Homebrew/utils.rb +++ b/Library/Homebrew/utils.rb @@ -1,6 +1,8 @@ # typed: false # frozen_string_literal: true +require "time" + require "utils/analytics" require "utils/curl" require "utils/fork" @@ -16,7 +18,6 @@ require "utils/repology" require "utils/svn" require "utils/tty" require "tap_constants" -require "time" module Homebrew extend Context @@ -206,7 +207,7 @@ module Kernel # Don't throw deprecations at all for cached, .brew or .metadata files. return if backtrace.any? do |line| - next true if line.include?(HOMEBREW_CACHE) + next true if line.include?(HOMEBREW_CACHE.to_s) next true if line.include?("/.brew/") next true if line.include?("/.metadata/") diff --git a/docs/Formula-Cookbook.md b/docs/Formula-Cookbook.md index 8e49bbaf9a..0796ec2ff2 100644 --- a/docs/Formula-Cookbook.md +++ b/docs/Formula-Cookbook.md @@ -630,7 +630,9 @@ If you need more control over the way files are downloaded and staged, you can c ```ruby class MyDownloadStrategy < SomeHomebrewDownloadStrategy - def fetch + def fetch(timeout: nil, **options) + opoo "Unhandled options in #{self.class}#fetch: #{options.keys.join(", ")}" unless options.empty? + # downloads output to `temporary_path` end end From 5cca3c5fd753103e93f25ca901462d65c1775ffb Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Thu, 25 Mar 2021 12:18:22 +0100 Subject: [PATCH 4/7] Mark `cached_location` and `url` public. --- Library/Homebrew/download_strategy.rb | 55 +++++++++++++++++++++------ 1 file changed, 43 insertions(+), 12 deletions(-) diff --git a/Library/Homebrew/download_strategy.rb b/Library/Homebrew/download_strategy.rb index 6eb002d824..6dc021ed5e 100644 --- a/Library/Homebrew/download_strategy.rb +++ b/Library/Homebrew/download_strategy.rb @@ -38,7 +38,19 @@ class AbstractDownloadStrategy end end - attr_reader :cache, :cached_location, :url, :meta, :name, :version + # The download URL. + # + # @api public + sig { returns(String) } + attr_reader :url + + # Location of the cached download. + # + # @api public + sig { returns(Pathname) } + attr_reader :cached_location + + attr_reader :cache, :meta, :name, :version private :meta, :name, :version @@ -111,6 +123,7 @@ class AbstractDownloadStrategy # Returns the most recent modified time for all files in the current working directory after stage. # # @api public + sig { returns(Time) } def source_modified_time Pathname.pwd.to_enum(:find).select(&:file?).map(&:mtime).max end @@ -219,10 +232,12 @@ class VCSDownloadStrategy < AbstractDownloadStrategy version.respond_to?(:head?) && version.head? end + # @!attribute [r] last_commit # Return last commit's unique identifier for the repository. # Return most recent modified timestamp unless overridden. # # @api public + sig { returns(String) } def last_commit source_modified_time.to_i.to_s end @@ -659,7 +674,8 @@ class SubversionDownloadStrategy < VCSDownloadStrategy super end - # (see AbstractDownloadStrategy#source_modified_time) + # @see AbstractDownloadStrategy#source_modified_time + # @api public sig { returns(Time) } def source_modified_time time = if Version.create(Utils::Svn.version) >= Version.create("1.9") @@ -672,7 +688,9 @@ class SubversionDownloadStrategy < VCSDownloadStrategy Time.parse time end - # (see VCSDownloadStrategy#source_modified_time) + # @see VCSDownloadStrategy#last_commit + # @api public + sig { returns(String) } def last_commit out, = silent_command("svn", args: ["info", "--show-item", "revision"], chdir: cached_location) out.strip @@ -771,14 +789,17 @@ class GitDownloadStrategy < VCSDownloadStrategy @shallow = meta.fetch(:shallow, true) end - # (see AbstractDownloadStrategy#source_modified_time) + # @see AbstractDownloadStrategy#source_modified_time + # @api public sig { returns(Time) } def source_modified_time out, = silent_command("git", args: ["--git-dir", git_dir, "show", "-s", "--format=%cD"]) Time.parse(out) end - # (see VCSDownloadStrategy#source_modified_time) + # @see VCSDownloadStrategy#last_commit + sig { returns(String) } + # @api public def last_commit out, = silent_command("git", args: ["--git-dir", git_dir, "rev-parse", "--short=7", "HEAD"]) out.chomp @@ -1051,7 +1072,8 @@ class CVSDownloadStrategy < VCSDownloadStrategy end end - # (see AbstractDownloadStrategy#source_modified_time) + # @see AbstractDownloadStrategy#source_modified_time + # @api public sig { returns(Time) } def source_modified_time # Filter CVS's files because the timestamp for each of them is the moment @@ -1124,7 +1146,8 @@ class MercurialDownloadStrategy < VCSDownloadStrategy @url = @url.sub(%r{^hg://}, "") end - # (see AbstractDownloadStrategy#source_modified_time) + # @see AbstractDownloadStrategy#source_modified_time + # @api public sig { returns(Time) } def source_modified_time out, = silent_command("hg", @@ -1133,7 +1156,9 @@ class MercurialDownloadStrategy < VCSDownloadStrategy Time.parse(out) end - # (see VCSDownloadStrategy#source_modified_time) + # @see VCSDownloadStrategy#last_commit + # @api public + sig { returns(String) } def last_commit out, = silent_command("hg", args: ["parent", "--template", "{node|short}", "-R", cached_location]) out.chomp @@ -1185,7 +1210,8 @@ class BazaarDownloadStrategy < VCSDownloadStrategy @url.sub!(%r{^bzr://}, "") end - # (see AbstractDownloadStrategy#source_modified_time) + # @see AbstractDownloadStrategy#source_modified_time + # @api public sig { returns(Time) } def source_modified_time out, = silent_command("bzr", args: ["log", "-l", "1", "--timezone=utc", cached_location]) @@ -1195,7 +1221,9 @@ class BazaarDownloadStrategy < VCSDownloadStrategy Time.parse(timestamp) end - # (see VCSDownloadStrategy#source_modified_time) + # @see VCSDownloadStrategy#last_commit + # @api public + sig { returns(String) } def last_commit out, = silent_command("bzr", args: ["revno", cached_location]) out.chomp @@ -1247,14 +1275,17 @@ class FossilDownloadStrategy < VCSDownloadStrategy @url = @url.sub(%r{^fossil://}, "") end - # (see AbstractDownloadStrategy#source_modified_time) + # @see AbstractDownloadStrategy#source_modified_time + # @api public sig { returns(Time) } def source_modified_time out, = silent_command("fossil", args: ["info", "tip", "-R", cached_location]) Time.parse(out[/^uuid: +\h+ (.+)$/, 1]) end - # (see VCSDownloadStrategy#source_modified_time) + # @see VCSDownloadStrategy#last_commit + # @api public + sig { returns(String) } def last_commit out, = silent_command("fossil", args: ["info", "tip", "-R", cached_location]) out[/^uuid: +(\h+) .+$/, 1] From 59f4a711cd0bc04e005a7b4d7eef317ea8169310 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sat, 3 Apr 2021 05:17:08 +0200 Subject: [PATCH 5/7] Deprecate `_fetch` without `timeout` option. --- Library/Homebrew/compat/early.rb | 4 ++++ .../compat/{ => early}/download_strategy.rb | 0 .../Homebrew/{compat.rb => compat/late.rb} | 2 +- .../Homebrew/compat/late/download_strategy.rb | 20 +++++++++++++++++++ Library/Homebrew/global.rb | 4 +++- Library/Homebrew/system_command.rb | 1 + 6 files changed, 29 insertions(+), 2 deletions(-) create mode 100644 Library/Homebrew/compat/early.rb rename Library/Homebrew/compat/{ => early}/download_strategy.rb (100%) rename Library/Homebrew/{compat.rb => compat/late.rb} (52%) create mode 100644 Library/Homebrew/compat/late/download_strategy.rb diff --git a/Library/Homebrew/compat/early.rb b/Library/Homebrew/compat/early.rb new file mode 100644 index 0000000000..4f0d4338c8 --- /dev/null +++ b/Library/Homebrew/compat/early.rb @@ -0,0 +1,4 @@ +# typed: strict +# frozen_string_literal: true + +require_relative "early/download_strategy" diff --git a/Library/Homebrew/compat/download_strategy.rb b/Library/Homebrew/compat/early/download_strategy.rb similarity index 100% rename from Library/Homebrew/compat/download_strategy.rb rename to Library/Homebrew/compat/early/download_strategy.rb diff --git a/Library/Homebrew/compat.rb b/Library/Homebrew/compat/late.rb similarity index 52% rename from Library/Homebrew/compat.rb rename to Library/Homebrew/compat/late.rb index a4e5ff6ae5..948b451f06 100644 --- a/Library/Homebrew/compat.rb +++ b/Library/Homebrew/compat/late.rb @@ -1,4 +1,4 @@ # typed: strict # frozen_string_literal: true -require "compat/download_strategy" +require_relative "late/download_strategy" diff --git a/Library/Homebrew/compat/late/download_strategy.rb b/Library/Homebrew/compat/late/download_strategy.rb new file mode 100644 index 0000000000..9f66c02d16 --- /dev/null +++ b/Library/Homebrew/compat/late/download_strategy.rb @@ -0,0 +1,20 @@ +# typed: false +# frozen_string_literal: true + +class CurlDownloadStrategy + module Compat + def _fetch(*args, **options) + unless options.key?(:timeout) + odeprecated "#{self.class}#_fetch" + options[:timeout] = nil + end + super(*args, **options) + end + end + + prepend Compat +end + +class CurlPostDownloadStrategy + prepend Compat +end diff --git a/Library/Homebrew/global.rb b/Library/Homebrew/global.rb index ffb5024b1b..d668dac471 100644 --- a/Library/Homebrew/global.rb +++ b/Library/Homebrew/global.rb @@ -75,7 +75,7 @@ HOMEBREW_BOTTLES_EXTNAME_REGEX = /\.([a-z0-9_]+)\.bottle\.(?:(\d+)\.)?tar\.gz$/. require "utils/sorbet" require "env_config" -require "compat" unless Homebrew::EnvConfig.no_compat? +require "compat/early" unless Homebrew::EnvConfig.no_compat? require "os" require "messages" @@ -154,3 +154,5 @@ require "tap_constants" # Enables `patchelf.rb` write support. HOMEBREW_PATCHELF_RB_WRITE = ENV["HOMEBREW_NO_PATCHELF_RB_WRITE"].blank?.freeze + +require "compat/late" unless Homebrew::EnvConfig.no_compat? diff --git a/Library/Homebrew/system_command.rb b/Library/Homebrew/system_command.rb index ea44cf25db..05371490c1 100644 --- a/Library/Homebrew/system_command.rb +++ b/Library/Homebrew/system_command.rb @@ -220,6 +220,7 @@ class SystemCommand loop do readable_sources, = IO.select(sources_remaining, [], [], end_time&.remaining!) raise Timeout::Error if readable_sources.nil? + readable_sources = T.must(readable_sources) break if readable_sources.empty? From 06a5811b4bd80046aae03a0be36e1c929015ad70 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sat, 3 Apr 2021 06:03:59 +0200 Subject: [PATCH 6/7] Simplify and fix `each_line_from`. --- Library/Homebrew/system_command.rb | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/Library/Homebrew/system_command.rb b/Library/Homebrew/system_command.rb index 05371490c1..8a6a8aa2b6 100644 --- a/Library/Homebrew/system_command.rb +++ b/Library/Homebrew/system_command.rb @@ -215,30 +215,30 @@ class SystemCommand sig { params(sources: T::Array[IO], _block: T.proc.params(type: Symbol, line: String).void).void } def each_line_from(sources, &_block) end_time = Time.now + @timeout if @timeout - sources_remaining = sources.dup + + sources = { + sources[0] => :stdout, + sources[1] => :stderr, + } loop do - readable_sources, = IO.select(sources_remaining, [], [], end_time&.remaining!) + readable_sources, = IO.select(sources.keys, [], [], end_time&.remaining!) raise Timeout::Error if readable_sources.nil? - readable_sources = T.must(readable_sources) - - break if readable_sources.empty? - - readable_sources.each do |source| + break if readable_sources.none? do |source| line = source.readline_nonblock || "" - type = (source == sources[0]) ? :stdout : :stderr - yield(type, line) + yield(sources.fetch(source), line) + true rescue EOFError source.close_read - sources_remaining.delete(source) - return if sources_remaining.empty? + sources.delete(source) + sources.any? rescue IO::WaitReadable - next + true end end - sources_remaining.each(&:close_read) + sources.each_key(&:close_read) end # Result containing the output and exit status of a finished sub-process. From ab04cfed831cf82232f4c1d815ada5f112133a71 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sat, 3 Apr 2021 20:10:25 +0200 Subject: [PATCH 7/7] Improve `_fetch` compatibility layer. --- .../compat/early/download_strategy.rb | 26 ++++++++++++++++--- Library/Homebrew/compat/late.rb | 2 -- .../Homebrew/compat/late/download_strategy.rb | 20 -------------- 3 files changed, 23 insertions(+), 25 deletions(-) delete mode 100644 Library/Homebrew/compat/late/download_strategy.rb diff --git a/Library/Homebrew/compat/early/download_strategy.rb b/Library/Homebrew/compat/early/download_strategy.rb index 2b098f01b2..bef8ae17a3 100644 --- a/Library/Homebrew/compat/early/download_strategy.rb +++ b/Library/Homebrew/compat/early/download_strategy.rb @@ -2,21 +2,41 @@ # frozen_string_literal: true class AbstractDownloadStrategy - module Compat + module CompatFetch def fetch(timeout: nil) super() end end + module Compat_Fetch # rubocop:disable Naming/ClassAndModuleCamelCase + def _fetch(*args, **options) + options[:timeout] = nil unless options.key?(:timeout) + + begin + super + rescue ArgumentError => e + raise unless e.message.include?("timeout") + + odeprecated "`def _fetch` in a subclass of `CurlDownloadStrategy`" + options.delete(:timeout) + super(*args, **options) + end + end + end + class << self def method_added(method) if method == :fetch && instance_method(method).arity.zero? - odeprecated "`def fetch` in a subclass of #{self}", + odeprecated "`def fetch` in a subclass of `#{self}`", "`def fetch(timeout: nil, **options)` and output a warning " \ "when `options` contains new unhandled options" class_eval do - prepend Compat + prepend CompatFetch + end + elsif method == :_fetch + class_eval do + prepend Compat_Fetch end end diff --git a/Library/Homebrew/compat/late.rb b/Library/Homebrew/compat/late.rb index 948b451f06..b4806eee61 100644 --- a/Library/Homebrew/compat/late.rb +++ b/Library/Homebrew/compat/late.rb @@ -1,4 +1,2 @@ # typed: strict # frozen_string_literal: true - -require_relative "late/download_strategy" diff --git a/Library/Homebrew/compat/late/download_strategy.rb b/Library/Homebrew/compat/late/download_strategy.rb deleted file mode 100644 index 9f66c02d16..0000000000 --- a/Library/Homebrew/compat/late/download_strategy.rb +++ /dev/null @@ -1,20 +0,0 @@ -# typed: false -# frozen_string_literal: true - -class CurlDownloadStrategy - module Compat - def _fetch(*args, **options) - unless options.key?(:timeout) - odeprecated "#{self.class}#_fetch" - options[:timeout] = nil - end - super(*args, **options) - end - end - - prepend Compat -end - -class CurlPostDownloadStrategy - prepend Compat -end