Merge pull request #4808 from reitermarkus/remove-popen-read

Remove some `#popen_read`s.

Library/Homebrew/extend/os/mac/system_config.rb

@@ -6,12 +6,11 @@ class SystemConfig
       # java_home doesn't exist on all macOSs; it might be missing on older versions.
       return "N/A" unless File.executable? "/usr/libexec/java_home"
 
-      java_xml = Utils.popen_read("/usr/libexec/java_home", "--xml", "--failfast", err: :close)
-      return "N/A" unless $CHILD_STATUS.success?
+      out, _, status = system_command("/usr/libexec/java_home", args: ["--xml", "--failfast"], print_stderr: false)
+      return "N/A" unless status.success?
       javas = []
-      REXML::XPath.each(
-        REXML::Document.new(java_xml), "//key[text()='JVMVersion']/following-sibling::string"
-      ) do |item|
+      xml = REXML::Document.new(out)
+      REXML::XPath.each(xml, "//key[text()='JVMVersion']/following-sibling::string") do |item|
         javas << item.text
       end
       javas.uniq.join(", ")

Library/Homebrew/readall.rb

@@ -73,13 +73,12 @@ module Readall
     private
 
     def syntax_errors_or_warnings?(rb)
-      # Retrieve messages about syntax errors/warnings printed to `$stderr`, but
-      # discard a `Syntax OK` printed to `$stdout` (in absence of syntax errors).
-      messages = Utils.popen_read("#{RUBY_PATH} -c -w #{rb} 2>&1 >/dev/null")
+      # Retrieve messages about syntax errors/warnings printed to `$stderr`.
+      _, err, status = system_command(RUBY_PATH, args: ["-c", "-w", rb], print_stderr: false)
 
       # Ignore unnecessary warning about named capture conflicts.
       # See https://bugs.ruby-lang.org/issues/12359.
-      messages = messages.lines
+      messages = err.lines
                     .grep_v(/named capture conflicts a local variable/)
                     .join
 
@@ -87,7 +86,7 @@ module Readall
 
       # Only syntax errors result in a non-zero status code. To detect syntax
       # warnings we also need to inspect the output to `$stderr`.
-      !$CHILD_STATUS.success? || !messages.chomp.empty?
+      !status.success? || !messages.chomp.empty?
     end
   end
 end

Library/Homebrew/requirements/java_requirement.rb

@@ -122,7 +122,7 @@ class JavaRequirement < Requirement
   end
 
   def satisfies_version(java)
-    java_version_s = Utils.popen_read(java, "-version", err: :out)[/\d+.\d/]
+    java_version_s = system_command(java, args: ["-version"], print_stderr: false).stderr[/\d+.\d/]
     return false unless java_version_s
     java_version = Version.create(java_version_s)
     needed_version = Version.create(version_without_plus)

Library/Homebrew/system_command.rb

@@ -30,16 +30,16 @@ class SystemCommand
   def run!
     puts command.shelljoin.gsub(/\\=/, "=") if verbose? || ARGV.debug?
 
-    @merged_output = []
+    @output = []
 
     each_output_line do |type, line|
       case type
       when :stdout
         $stdout << line if print_stdout?
-        @merged_output << [:stdout, line]
+        @output << [:stdout, line]
       when :stderr
         $stderr << line if print_stderr?
-        @merged_output << [:stderr, line]
+        @output << [:stderr, line]
       end
     end
 
@@ -100,7 +100,7 @@ class SystemCommand
     return if @status.success?
     raise ErrorDuringExecution.new(command,
                                    status: @status,
-                                   output: @merged_output)
+                                   output: @output)
   end
 
   def expanded_args
@@ -128,7 +128,7 @@ class SystemCommand
     @status = raw_wait_thr.value
   rescue SystemCallError => e
     @status = $CHILD_STATUS
-    @merged_output << [:stderr, e.message]
+    @output << [:stderr, e.message]
   end
 
   def write_input_to(raw_stdin)
@@ -158,24 +158,31 @@ class SystemCommand
   end
 
   def result
-    output = @merged_output.each_with_object(stdout: "", stderr: "") do |(type, line), hash|
-      hash[type] << line
-    end
-
-    Result.new(command, output[:stdout], output[:stderr], @status)
+    Result.new(command, @output, @status)
   end
 
   class Result
-    attr_accessor :command, :stdout, :stderr, :status, :exit_status
+    attr_accessor :command, :status, :exit_status
 
-    def initialize(command, stdout, stderr, status)
+    def initialize(command, output, status)
       @command       = command
-      @stdout      = stdout
-      @stderr      = stderr
+      @output        = output
       @status        = status
       @exit_status   = status.exitstatus
     end
 
+    def stdout
+      @stdout ||= @output.select { |type,| type == :stdout }
+                         .map { |_, line| line }
+                         .join
+    end
+
+    def stderr
+      @stderr ||= @output.select { |type,| type == :stderr }
+                         .map { |_, line| line }
+                         .join
+    end
+
     def success?
       @exit_status.zero?
     end

Library/Homebrew/system_config.rb

@@ -79,9 +79,9 @@ class SystemConfig
 
     def describe_java
       return "N/A" unless which "java"
-      java_version = Utils.popen_read("java", "-version")
-      return "N/A" unless $CHILD_STATUS.success?
-      java_version[/java version "([\d\._]+)"/, 1] || "N/A"
+      _, err, status = system_command("java", args: ["-version"], print_stderr: false)
+      return "N/A" unless status.success?
+      err[/java version "([\d\._]+)"/, 1] || "N/A"
     end
 
     def describe_git
@@ -90,13 +90,14 @@ class SystemConfig
     end
 
     def describe_curl
-      curl_version_output = Utils.popen_read("#{curl_executable} --version", err: :close)
-      curl_version_output =~ /^curl ([\d\.]+)/
-      curl_version = Regexp.last_match(1)
+      out, = system_command(curl_executable, args: ["--version"])
+
+      if /^curl (?<curl_version>[\d\.]+)/ =~ out
         "#{curl_version} => #{curl_executable}"
-    rescue
+      else
         "N/A"
       end
+    end
 
     def dump_verbose_config(f = $stdout)
       f.puts "HOMEBREW_VERSION: #{HOMEBREW_VERSION}"

Library/Homebrew/test/cask/pkg_spec.rb

@@ -150,7 +150,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, instance_double(Process::Status, exitstatus: 0)),
+        SystemCommand::Result.new(nil, [[:stdout, pkg_info_plist]], instance_double(Process::Status, exitstatus: 0)),
       )
 
       info = pkg.info

Library/Homebrew/test/java_requirement_spec.rb

@@ -55,7 +55,7 @@ describe JavaRequirement do
       def setup_java_with_version(version)
         IO.write java, <<~SH
           #!/bin/sh
-          echo 'java version "#{version}"'
+          echo 'java version "#{version}"' 1>&2
         SH
         FileUtils.chmod "+x", java
       end

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, "", OpenStruct.new(exitstatus: 0))
+      SystemCommand::Result.new(command, [[:stdout, response]], OpenStruct.new(exitstatus: 0))
     end
   end
 

Library/Homebrew/test/system_command_result_spec.rb

@@ -2,8 +2,14 @@ require "system_command"
 
 describe SystemCommand::Result do
   describe "#to_ary" do
+    let(:output) {
+      [
+        [:stdout, "output"],
+        [:stderr, "error"],
+      ]
+    }
     subject(:result) {
-      described_class.new([], "output", "error", instance_double(Process::Status, exitstatus: 0, success?: true))
+      described_class.new([], output, instance_double(Process::Status, exitstatus: 0, success?: true))
     }
 
     it "can be destructed like `Open3.capture3`" do
@@ -16,7 +22,9 @@ describe SystemCommand::Result do
   end
 
   describe "#plist" do
-    subject { described_class.new(command, stdout, "", instance_double(Process::Status, exitstatus: 0)).plist }
+    subject {
+      described_class.new(command, [[:stdout, stdout]], instance_double(Process::Status, exitstatus: 0)).plist
+    }
 
     let(:command) { ["true"] }
     let(:garbage) {