From 355df64d93b48ce3d791d2556e3cf84ef005e1e3 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Fri, 27 Jul 2018 03:21:14 +0200 Subject: [PATCH] Use `SystemCommand` for `curl`. --- .../cask/lib/hbc/download_strategy.rb | 4 +- Library/Homebrew/exceptions.rb | 4 ++ Library/Homebrew/system_command.rb | 40 +++++++++----- .../test/cask/download_strategy_spec.rb | 4 +- Library/Homebrew/test/cask/pkg_spec.rb | 2 +- .../helper/cask/fake_system_command.rb | 2 +- .../test/system_command_result_spec.rb | 16 +++++- Library/Homebrew/test/system_command_spec.rb | 52 ++++++++++++------- Library/Homebrew/test/utils/curl_spec.rb | 4 +- Library/Homebrew/utils/curl.rb | 16 +++--- Library/Homebrew/utils/github.rb | 7 ++- 11 files changed, 99 insertions(+), 52 deletions(-) diff --git a/Library/Homebrew/cask/lib/hbc/download_strategy.rb b/Library/Homebrew/cask/lib/hbc/download_strategy.rb index 44ef287b70..ca80120838 100644 --- a/Library/Homebrew/cask/lib/hbc/download_strategy.rb +++ b/Library/Homebrew/cask/lib/hbc/download_strategy.rb @@ -106,10 +106,10 @@ module Hbc LockFile.new(temporary_path.basename).with_lock do _fetch end - rescue ErrorDuringExecution + rescue ErrorDuringExecution => e # 33 == range not supported # try wiping the incomplete download and retrying once - if $CHILD_STATUS.exitstatus == 33 && had_incomplete_download + if e.status.exitstatus == 33 && had_incomplete_download ohai "Trying a full download" temporary_path.unlink had_incomplete_download = false diff --git a/Library/Homebrew/exceptions.rb b/Library/Homebrew/exceptions.rb index 0b37cbaa71..dc3f5f9e36 100644 --- a/Library/Homebrew/exceptions.rb +++ b/Library/Homebrew/exceptions.rb @@ -526,7 +526,11 @@ end # raised by safe_system in utils.rb class ErrorDuringExecution < RuntimeError + attr_reader :status + def initialize(cmd, status:, output: nil) + @status = status + s = "Failure while executing; `#{cmd.shelljoin.gsub(/\\=/, "=")}` exited with #{status.exitstatus}." unless [*output].empty? diff --git a/Library/Homebrew/system_command.rb b/Library/Homebrew/system_command.rb index c49d773a90..37b5983bd8 100644 --- a/Library/Homebrew/system_command.rb +++ b/Library/Homebrew/system_command.rb @@ -37,7 +37,13 @@ class SystemCommand puts line.chomp if print_stdout? @merged_output << [:stdout, line] when :stderr - $stderr.puts Formatter.error(line.chomp) if print_stderr? + if print_stderr? + if line.start_with?("\r") + $stderr.print line + else + $stderr.puts Formatter.error(line.chomp) + end + end @merged_output << [:stderr, line] end end @@ -76,13 +82,18 @@ class SystemCommand def env_args return [] if env.empty? - variables = env.map do |name, value| - sanitized_name = Shellwords.escape(name) - sanitized_value = Shellwords.escape(value) - "#{sanitized_name}=#{sanitized_value}" - end + unset_variables = env.select { |_, value| value.nil? } + .flat_map { |name,| ["-u", name] } - ["env", *variables] + set_variables = + env.reject { |_, value| value.nil? } + .flat_map do |name, value| + sanitized_name = Shellwords.escape(name) + sanitized_value = Shellwords.escape(value) + "#{sanitized_name}=#{sanitized_value}" + end + + ["env", *unset_variables, *set_variables] end def sudo_prefix @@ -102,7 +113,7 @@ class SystemCommand @expanded_args ||= args.map do |arg| if arg.respond_to?(:to_path) File.absolute_path(arg) - elsif arg.is_a?(Integer) || arg.is_a?(Float) + elsif arg.is_a?(Integer) || arg.is_a?(Float) || arg.is_a?(URI) arg.to_s else arg.to_str @@ -157,23 +168,28 @@ class SystemCommand hash[type] << line end - Result.new(command, output[:stdout], output[:stderr], @status.exitstatus) + Result.new(command, output[:stdout], output[:stderr], @status) end class Result - attr_accessor :command, :stdout, :stderr, :exit_status + attr_accessor :command, :stdout, :stderr, :status, :exit_status - def initialize(command, stdout, stderr, exit_status) + def initialize(command, stdout, stderr, status) @command = command @stdout = stdout @stderr = stderr - @exit_status = exit_status + @status = status + @exit_status = status.exitstatus end def success? @exit_status.zero? end + def to_ary + [stdout, stderr, status] + end + def plist @plist ||= begin output = stdout diff --git a/Library/Homebrew/test/cask/download_strategy_spec.rb b/Library/Homebrew/test/cask/download_strategy_spec.rb index 0769bb0679..0a5ed4e80c 100644 --- a/Library/Homebrew/test/cask/download_strategy_spec.rb +++ b/Library/Homebrew/test/cask/download_strategy_spec.rb @@ -39,7 +39,7 @@ describe "download strategies", :cask do let(:url_options) { { user_agent: "Mozilla/25.0.1" } } it "adds the appropriate curl args" do - expect(downloader).to receive(:safe_system) { |*args| + expect(downloader).to receive(:system_command!) { |*, args:, **| expect(args.each_cons(2)).to include(["--user-agent", "Mozilla/25.0.1"]) } @@ -53,7 +53,7 @@ describe "download strategies", :cask do let(:url_options) { { user_agent: :fake } } it "adds the appropriate curl args" do - expect(downloader).to receive(:safe_system) { |*args| + expect(downloader).to receive(:system_command!) { |*, args:, **| expect(args.each_cons(2).to_a).to include(["--user-agent", a_string_matching(/Mozilla.*Mac OS X 10.*AppleWebKit/)]) } diff --git a/Library/Homebrew/test/cask/pkg_spec.rb b/Library/Homebrew/test/cask/pkg_spec.rb index 2c1f38f0a7..6871514dda 100644 --- a/Library/Homebrew/test/cask/pkg_spec.rb +++ b/Library/Homebrew/test/cask/pkg_spec.rb @@ -148,7 +148,7 @@ describe Hbc::Pkg, :cask do "/usr/sbin/pkgutil", args: ["--pkg-info-plist", pkg_id], ).and_return( - SystemCommand::Result.new(nil, pkg_info_plist, nil, 0), + SystemCommand::Result.new(nil, pkg_info_plist, nil, instance_double(Process::Status, exitstatus: 0)), ) info = pkg.info diff --git a/Library/Homebrew/test/support/helper/cask/fake_system_command.rb b/Library/Homebrew/test/support/helper/cask/fake_system_command.rb index b8f8aa4347..e740e73539 100644 --- a/Library/Homebrew/test/support/helper/cask/fake_system_command.rb +++ b/Library/Homebrew/test/support/helper/cask/fake_system_command.rb @@ -52,7 +52,7 @@ class FakeSystemCommand if response.respond_to?(:call) response.call(command_string, options) else - SystemCommand::Result.new(command, response, "", 0) + SystemCommand::Result.new(command, response, "", OpenStruct.new(exitstatus: 0)) end end diff --git a/Library/Homebrew/test/system_command_result_spec.rb b/Library/Homebrew/test/system_command_result_spec.rb index b454e644f2..d65339a133 100644 --- a/Library/Homebrew/test/system_command_result_spec.rb +++ b/Library/Homebrew/test/system_command_result_spec.rb @@ -1,8 +1,22 @@ require "system_command" describe SystemCommand::Result do + describe "#to_ary" do + subject(:result) { + described_class.new([], "output", "error", instance_double(Process::Status, exitstatus: 0, success?: true)) + } + + it "can be destructed like `Open3.capture3`" do + out, err, status = result + + expect(out).to eq "output" + expect(err).to eq "error" + expect(status).to be_a_success + end + end + describe "#plist" do - subject { described_class.new(command, stdout, "", 0).plist } + subject { described_class.new(command, stdout, "", instance_double(Process::Status, exitstatus: 0)).plist } let(:command) { ["true"] } let(:garbage) { diff --git a/Library/Homebrew/test/system_command_spec.rb b/Library/Homebrew/test/system_command_spec.rb index 0b74d1fbfe..a69091816b 100644 --- a/Library/Homebrew/test/system_command_spec.rb +++ b/Library/Homebrew/test/system_command_spec.rb @@ -1,17 +1,20 @@ describe SystemCommand do describe "#initialize" do let(:env_args) { ["bash", "-c", 'printf "%s" "${A?}" "${B?}" "${C?}"'] } + let(:env) { { "A" => "1", "B" => "2", "C" => "3" } } + let(:sudo) { false } + + subject(:command) { + described_class.new( + "env", + args: env_args, + env: env, + must_succeed: true, + sudo: sudo, + ) + } context "when given some environment variables" do - subject { - described_class.new( - "env", - args: env_args, - env: { "A" => "1", "B" => "2", "C" => "3" }, - must_succeed: true, - ) - } - its("run!.stdout") { is_expected.to eq("123") } describe "the resulting command line" do @@ -21,21 +24,23 @@ describe SystemCommand do .with(["env", "env"], "A=1", "B=2", "C=3", "env", *env_args, {}) .and_call_original - subject.run! + command.run! end end end + context "when given an environment variable which is set to nil" do + let(:env) { { "A" => "1", "B" => "2", "C" => nil } } + + it "unsets them" do + expect { + command.run! + }.to raise_error(/C: parameter null or not set/) + end + end + context "when given some environment variables and sudo: true" do - subject { - described_class.new( - "env", - args: env_args, - env: { "A" => "1", "B" => "2", "C" => "3" }, - must_succeed: true, - sudo: true, - ) - } + let(:sudo) { true } describe "the resulting command line" do it "includes the given variables explicitly" do @@ -47,7 +52,7 @@ describe SystemCommand do original_popen3.call("true", &block) end - subject.run! + command.run! end end end @@ -215,5 +220,12 @@ describe SystemCommand do described_class.run("non_existent_executable") }.not_to raise_error end + + it 'does not format `stderr` when it starts with \r' do + expect { + system_command "bash", + args: ["-c", 'printf "\r%s" "################### 27.6%" 1>&2'] + }.to output("\r################### 27.6%").to_stderr + end end end diff --git a/Library/Homebrew/test/utils/curl_spec.rb b/Library/Homebrew/test/utils/curl_spec.rb index a843a2f2de..33d01a7f62 100644 --- a/Library/Homebrew/test/utils/curl_spec.rb +++ b/Library/Homebrew/test/utils/curl_spec.rb @@ -4,12 +4,12 @@ describe "curl" do describe "curl_args" do it "returns -q as the first argument when HOMEBREW_CURLRC is not set" do # -q must be the first argument according to "man curl" - expect(curl_args("foo")[1]).to eq("-q") + expect(curl_args("foo").first).to eq("-q") end it "doesn't return -q as the first argument when HOMEBREW_CURLRC is set" do ENV["HOMEBREW_CURLRC"] = "1" - expect(curl_args("foo")[1]).to_not eq("-q") + expect(curl_args("foo").first).to_not eq("-q") end end end diff --git a/Library/Homebrew/utils/curl.rb b/Library/Homebrew/utils/curl.rb index 216c2bf81e..7f0d08b364 100644 --- a/Library/Homebrew/utils/curl.rb +++ b/Library/Homebrew/utils/curl.rb @@ -11,7 +11,7 @@ def curl_executable end def curl_args(*extra_args, show_output: false, user_agent: :default) - args = [curl_executable.to_s] + args = [] # do not load .curlrc unless requested (must be the first argument) args << "-q" unless ENV["HOMEBREW_CURLRC"] @@ -40,18 +40,18 @@ end def curl(*args) # SSL_CERT_FILE can be incorrectly set by users or portable-ruby and screw # with SSL downloads so unset it here. - with_env SSL_CERT_FILE: nil do - safe_system(*curl_args(*args)) - end + system_command! curl_executable, + args: curl_args(*args), + env: { "SSL_CERT_FILE" => nil } end def curl_download(*args, to: nil, continue_at: "-", **options) had_incomplete_download ||= File.exist?(to) curl("--location", "--remote-time", "--continue-at", continue_at.to_s, "--output", to, *args, **options) -rescue ErrorDuringExecution +rescue ErrorDuringExecution => e # `curl` error 33: HTTP server doesn't seem to support byte ranges. Cannot resume. # HTTP status 416: Requested range not satisfiable - if ($CHILD_STATUS.exitstatus == 33 || had_incomplete_download) && continue_at == "-" + if (e.status.exitstatus == 33 || had_incomplete_download) && continue_at == "-" continue_at = 0 had_incomplete_download = false retry @@ -61,7 +61,9 @@ rescue ErrorDuringExecution end def curl_output(*args, **options) - Open3.capture3(*curl_args(*args, show_output: true, **options)) + system_command(curl_executable, + args: curl_args(*args, show_output: true, **options), + print_stderr: false) end def curl_check_http_content(url, user_agents: [:default], check_content: false, strict: false, require_http: false) diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index 40eb4e4388..08148769f5 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -131,13 +131,12 @@ module GitHub # This is a no-op if the user is opting out of using the GitHub API. return block_given? ? yield({}) : {} if ENV["HOMEBREW_NO_GITHUB_API"] - args = %W[--header application/vnd.github.v3+json --write-out \n%{http_code}] # rubocop:disable Lint/NestedPercentLiteral - args += curl_args + args = ["--header", "application/vnd.github.v3+json", "--write-out", "\n%{http_code}"] token, username = api_credentials case api_credentials_type when :keychain - args += %W[--user #{username}:#{token}] + args += ["--user", "#{username}:#{token}"] when :environment args += ["--header", "Authorization: token #{token}"] end @@ -162,7 +161,7 @@ module GitHub args += ["--dump-header", headers_tmpfile.path] - output, errors, status = curl_output(url.to_s, "--location", *args) + output, errors, status = curl_output("--location", url.to_s, *args) output, _, http_code = output.rpartition("\n") output, _, http_code = output.rpartition("\n") if http_code == "000" headers = headers_tmpfile.read