From e00d066d8797568854c199304f162c7cbc4a25f6 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 29 Jan 2024 18:14:31 -0800 Subject: [PATCH] Replace Time refinement with Timer Util --- Library/Homebrew/download_strategy.rb | 50 ++++++++++++----------- Library/Homebrew/extend/time.rb | 19 +-------- Library/Homebrew/extend/time.rbi | 9 ---- Library/Homebrew/system_command.rb | 6 +-- Library/Homebrew/test/utils/timer_spec.rb | 33 +++++++++++++++ Library/Homebrew/utils/curl.rb | 8 ++-- Library/Homebrew/utils/timer.rb | 21 ++++++++++ 7 files changed, 87 insertions(+), 59 deletions(-) delete mode 100644 Library/Homebrew/extend/time.rbi create mode 100644 Library/Homebrew/test/utils/timer_spec.rb create mode 100644 Library/Homebrew/utils/timer.rb diff --git a/Library/Homebrew/download_strategy.rb b/Library/Homebrew/download_strategy.rb index af8931b6fe..f96eb0274e 100644 --- a/Library/Homebrew/download_strategy.rb +++ b/Library/Homebrew/download_strategy.rb @@ -17,12 +17,10 @@ require "vendor/gems/mechanize/lib/mechanize/http/content_disposition_parser" require "utils/curl" require "utils/github" +require "utils/timer" require "github_packages" -require "extend/time" -using TimeRemaining - # @abstract Abstract superclass for all download strategies. # # @api private @@ -427,7 +425,7 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy use_cached_location = false if version.respond_to?(:latest?) && version.latest? resolved_url, _, last_modified, _, is_redirection = begin - resolve_url_basename_time_file_size(url, timeout: end_time&.remaining!) + resolve_url_basename_time_file_size(url, timeout: Utils::Timer.remaining!(end_time)) rescue ErrorDuringExecution raise unless use_cached_location end @@ -442,7 +440,7 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy puts "Already downloaded: #{cached_location}" else begin - _fetch(url: url, resolved_url: resolved_url, timeout: end_time&.remaining!) + _fetch(url: url, resolved_url: resolved_url, timeout: Utils::Timer.remaining!(end_time)) rescue ErrorDuringExecution raise CurlDownloadStrategyError, url end @@ -802,9 +800,9 @@ class SubversionDownloadStrategy < VCSDownloadStrategy args.concat Utils::Svn.invalid_cert_flags if meta[:trust_cert] == true if target.directory? - command! "svn", args: ["update", *args], chdir: target.to_s, timeout: timeout&.remaining + command! "svn", args: ["update", *args], chdir: target.to_s, timeout: Utils::Timer.remaining(timeout) else - command! "svn", args: ["checkout", url, target, *args], timeout: timeout&.remaining + command! "svn", args: ["checkout", url, target, *args], timeout: Utils::Timer.remaining(timeout) end end @@ -998,23 +996,23 @@ class GitDownloadStrategy < VCSDownloadStrategy command! "git", args: ["fetch", "origin", "--unshallow"], chdir: cached_location, - timeout: timeout&.remaining + timeout: Utils::Timer.remaining(timeout) else command! "git", args: ["fetch", "origin"], chdir: cached_location, - timeout: timeout&.remaining + timeout: Utils::Timer.remaining(timeout) end end sig { params(timeout: T.nilable(Time)).void } def clone_repo(timeout: nil) - command! "git", args: clone_args, timeout: timeout&.remaining + command! "git", args: clone_args, timeout: Utils::Timer.remaining(timeout) command! "git", args: ["config", "homebrew.cacheversion", cache_version], chdir: cached_location, - timeout: timeout&.remaining + timeout: Utils::Timer.remaining(timeout) configure_sparse_checkout if partial_clone_sparse_checkout? @@ -1025,7 +1023,8 @@ class GitDownloadStrategy < VCSDownloadStrategy 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, timeout: timeout&.remaining + command! "git", args: ["checkout", "-f", @ref, "--"], chdir: cached_location, + timeout: Utils::Timer.remaining(timeout) end sig { void } @@ -1047,11 +1046,11 @@ class GitDownloadStrategy < VCSDownloadStrategy command! "git", args: ["submodule", "foreach", "--recursive", "git submodule sync"], chdir: cached_location, - timeout: timeout&.remaining + timeout: Utils::Timer.remaining(timeout) command! "git", args: ["submodule", "update", "--init", "--recursive"], chdir: cached_location, - timeout: timeout&.remaining + timeout: Utils::Timer.remaining(timeout) fix_absolute_submodule_gitdir_references! end @@ -1214,12 +1213,15 @@ class CVSDownloadStrategy < VCSDownloadStrategy 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"], timeout: timeout&.remaining if @url.include? "pserver" + if @url.include? "pserver" + command! "cvs", args: [*quiet_flag, "-d", @url, "login"], + timeout: Utils::Timer.remaining(timeout) + end command! "cvs", args: [*quiet_flag, "-d", @url, "checkout", "-d", cached_location.basename, @module], chdir: cached_location.dirname, - timeout: timeout&.remaining + timeout: Utils::Timer.remaining(timeout) end sig { params(timeout: T.nilable(Time)).void } @@ -1227,7 +1229,7 @@ class CVSDownloadStrategy < VCSDownloadStrategy command! "cvs", args: [*quiet_flag, "update"], chdir: cached_location, - timeout: timeout&.remaining + timeout: Utils::Timer.remaining(timeout) end def split_url(in_url) @@ -1292,7 +1294,7 @@ class MercurialDownloadStrategy < VCSDownloadStrategy end clone_args << @url << cached_location.to_s - command! "hg", args: clone_args, timeout: timeout&.remaining + command! "hg", args: clone_args, timeout: Utils::Timer.remaining(timeout) end sig { params(timeout: T.nilable(Time)).void } @@ -1306,7 +1308,7 @@ class MercurialDownloadStrategy < VCSDownloadStrategy pull_args << "--rev" << @ref end - command! "hg", args: ["--cwd", cached_location, *pull_args], timeout: timeout&.remaining + command! "hg", args: ["--cwd", cached_location, *pull_args], timeout: Utils::Timer.remaining(timeout) update_args = %w[update --clean] update_args << if @ref_type && @ref @@ -1316,7 +1318,7 @@ class MercurialDownloadStrategy < VCSDownloadStrategy "default" end - command! "hg", args: ["--cwd", cached_location, *update_args], timeout: timeout&.remaining + command! "hg", args: ["--cwd", cached_location, *update_args], timeout: Utils::Timer.remaining(timeout) end def current_revision @@ -1376,7 +1378,7 @@ class BazaarDownloadStrategy < VCSDownloadStrategy # "lightweight" means history-less command! "bzr", args: ["checkout", "--lightweight", @url, cached_location], - timeout: timeout&.remaining + timeout: Utils::Timer.remaining(timeout) end sig { params(timeout: T.nilable(Time)).void } @@ -1384,7 +1386,7 @@ class BazaarDownloadStrategy < VCSDownloadStrategy command! "bzr", args: ["update"], chdir: cached_location, - timeout: timeout&.remaining + timeout: Utils::Timer.remaining(timeout) end end @@ -1430,12 +1432,12 @@ class FossilDownloadStrategy < VCSDownloadStrategy sig { params(timeout: T.nilable(Time)).void } def clone_repo(timeout: nil) - command! "fossil", args: ["clone", @url, cached_location], timeout: timeout&.remaining + command! "fossil", args: ["clone", @url, cached_location], timeout: Utils::Timer.remaining(timeout) end sig { params(timeout: T.nilable(Time)).void } def update(timeout: nil) - command! "fossil", args: ["pull", "-R", cached_location], timeout: timeout&.remaining + command! "fossil", args: ["pull", "-R", cached_location], timeout: Utils::Timer.remaining(timeout) end end diff --git a/Library/Homebrew/extend/time.rb b/Library/Homebrew/extend/time.rb index d62de2c8ea..75a79b864b 100644 --- a/Library/Homebrew/extend/time.rb +++ b/Library/Homebrew/extend/time.rb @@ -1,22 +1,7 @@ -# typed: true +# typed: strong # frozen_string_literal: true -module TimeRemaining - refine Time do - def remaining - T.bind(self, Time) - [0, self - Time.now].max - end - - def remaining! - r = remaining - - Kernel.raise Timeout::Error if r <= 0 - - r - end - end -end +require "time" class Time # Backwards compatibility for formulae that used this ActiveSupport extension diff --git a/Library/Homebrew/extend/time.rbi b/Library/Homebrew/extend/time.rbi deleted file mode 100644 index ab1c7254f9..0000000000 --- a/Library/Homebrew/extend/time.rbi +++ /dev/null @@ -1,9 +0,0 @@ -# typed: strict - -class Time - sig { returns(T.any(Integer, Float)) } - def remaining; end - - sig { returns(T.any(Integer, Float)) } - def remaining!; end -end diff --git a/Library/Homebrew/system_command.rb b/Library/Homebrew/system_command.rb index aac75e29b4..4d91367dfd 100644 --- a/Library/Homebrew/system_command.rb +++ b/Library/Homebrew/system_command.rb @@ -7,14 +7,12 @@ require "plist" require "shellwords" require "extend/io" -require "extend/time" +require "utils/timer" # Class for running sub-processes and capturing their output and exit status. # # @api private class SystemCommand - using TimeRemaining - # Helper functions for calling {SystemCommand.run}. module Mixin def system_command(executable, **options) @@ -259,7 +257,7 @@ class SystemCommand end end_time = Time.now + @timeout if @timeout - raise Timeout::Error if raw_wait_thr.join(end_time&.remaining).nil? + raise Timeout::Error if raw_wait_thr.join(Utils::Timer.remaining(end_time)).nil? @status = raw_wait_thr.value diff --git a/Library/Homebrew/test/utils/timer_spec.rb b/Library/Homebrew/test/utils/timer_spec.rb new file mode 100644 index 0000000000..e2a8fb1982 --- /dev/null +++ b/Library/Homebrew/test/utils/timer_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require "utils/timer" + +describe Utils::Timer do + describe "#remaining" do + it "returns nil when nil" do + expect(described_class.remaining(nil)).to be_nil + end + + it "returns time remaining when there is time remaining" do + expect(described_class.remaining(Time.now + 10)).to be > 1 + end + + it "returns 0 when there is no time remaining" do + expect(described_class.remaining(Time.now - 10)).to be 0 + end + end + + describe "#remaining!" do + it "returns nil when nil" do + expect(described_class.remaining!(nil)).to be_nil + end + + it "returns time remaining when there is time remaining" do + expect(described_class.remaining!(Time.now + 10)).to be > 1 + end + + it "returns 0 when there is no time remaining" do + expect { described_class.remaining!(Time.now - 10) }.to raise_error(Timeout::Error) + end + end +end diff --git a/Library/Homebrew/utils/curl.rb b/Library/Homebrew/utils/curl.rb index 98f6f6ed25..47110f6237 100644 --- a/Library/Homebrew/utils/curl.rb +++ b/Library/Homebrew/utils/curl.rb @@ -3,15 +3,13 @@ require "open3" -require "extend/time" +require "utils/timer" module Utils # Helper function for interacting with `curl`. # # @api private module Curl - using TimeRemaining - # This regex is used to extract the part of an ETag within quotation marks, # ignoring any leading weak validator indicator (`W/`). This simplifies # ETag comparison in `#curl_check_http_content`. @@ -140,7 +138,7 @@ module Utils result = system_command curl_executable(use_homebrew_curl: use_homebrew_curl), args: curl_args(*args, **options), env: env, - timeout: end_time&.remaining, + timeout: Utils::Timer.remaining(end_time), **command_options return result if result.success? || args.include?("--http1.1") @@ -151,7 +149,7 @@ module Utils if result.exit_status == 16 return curl_with_workarounds( *args, "--http1.1", - timeout: end_time&.remaining, **command_options, **options + timeout: Utils::Timer.remaining(end_time), **command_options, **options ) end diff --git a/Library/Homebrew/utils/timer.rb b/Library/Homebrew/utils/timer.rb new file mode 100644 index 0000000000..b01e715de4 --- /dev/null +++ b/Library/Homebrew/utils/timer.rb @@ -0,0 +1,21 @@ +# typed: strong +# frozen_string_literal: true + +module Utils + module Timer + sig { params(time: T.nilable(Time)).returns(T.any(Float, Integer, NilClass)) } + def self.remaining(time) + return unless time + + [0, time - Time.now].max + end + + sig { params(time: T.nilable(Time)).returns(T.any(Float, Integer, NilClass)) } + def self.remaining!(time) + r = remaining(time) + raise Timeout::Error if r && r <= 0 + + r + end + end +end