Merge pull request #4453 from reitermarkus/fix-system-command

Fix `SystemCommand` `:path`.

Library/Homebrew/cask/lib/hbc/artifact/installer.rb

@@ -14,16 +14,32 @@ module Hbc
             To complete the installation of Cask #{cask}, you must also
             run the installer at
 
-              '#{path}'
+              '#{cask.staged_path.join(path)}'
           EOS
         end
       end
 
       module ScriptInstaller
         def install_phase(command: nil, **_)
-          ohai "Running #{self.class.dsl_key} script '#{path.relative_path_from(cask.staged_path)}'"
+          ohai "Running #{self.class.dsl_key} script '#{path}'"
-          FileUtils.chmod "+x", path unless path.executable?
+
-          command.run(path, **args, path: PATH.new(HOMEBREW_PREFIX/"bin", HOMEBREW_PREFIX/"sbin", ENV["PATH"]))
+          absolute_path = if path.absolute?
+            path
+          else
+            cask.staged_path.join(path)
+          end
+
+          if absolute_path.exist? && !absolute_path.executable?
+            FileUtils.chmod "+x", absolute_path
+          end
+
+          executable = if absolute_path.exist?
+            absolute_path
+          else
+            path
+          end
+
+          command.run!(executable, **args, env: { "PATH" => PATH.new(HOMEBREW_PREFIX/"bin", HOMEBREW_PREFIX/"sbin", ENV["PATH"]) })
         end
       end
 
@@ -54,7 +70,7 @@ module Hbc
         super(cask)
 
         if args.key?(:manual)
-          @path = cask.staged_path.join(args[:manual])
+          @path = Pathname(args[:manual])
           @args = []
           extend(ManualInstaller)
           return
@@ -65,17 +81,16 @@ module Hbc
         )
         raise CaskInvalidError.new(cask, "#{self.class.dsl_key} missing executable") if path.nil?
 
-        path = Pathname(path)
+        @path = Pathname(path)
-        @path = path.absolute? ? path : cask.staged_path.join(path)
         extend(ScriptInstaller)
       end
 
       def summarize
-        path.relative_path_from(cask.staged_path).to_s
+        path.to_s
       end
 
       def to_h
-        { path: path.relative_path_from(cask.staged_path).to_s }.tap do |h|
+        { path: path }.tap do |h|
           h[:args] = args unless is_a?(ManualInstaller)
         end
       end

Library/Homebrew/cask/lib/hbc/system_command.rb

@@ -47,7 +47,7 @@ module Hbc
       @must_succeed = must_succeed
       options.extend(HashValidator).assert_valid_keys(:chdir)
       @options = options
-      @env = { "PATH" => ENV["PATH"] }.merge(env)
+      @env = env
 
       @env.keys.grep_v(/^[\w&&\D]\w*$/) do |name|
         raise ArgumentError, "Invalid variable name: '#{name}'"
@@ -55,7 +55,7 @@ module Hbc
     end
 
     def command
-      [*sudo_prefix, executable, *args]
+      [*sudo_prefix, *env_args, executable, *args]
     end
 
     private
@@ -64,18 +64,22 @@ module Hbc
 
     attr_predicate :sudo?, :print_stdout?, :print_stderr?, :must_succeed?
 
+    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
+
+      ["env", *variables]
+    end
+
     def sudo_prefix
       return [] unless sudo?
       askpass_flags = ENV.key?("SUDO_ASKPASS") ? ["-A"] : []
-      prefix = ["/usr/bin/sudo", *askpass_flags, "-E"]
+      ["/usr/bin/sudo", *askpass_flags, "-E", "--"]
-
-      env.each do |name, value|
-        sanitized_name = Shellwords.escape(name)
-        sanitized_value = Shellwords.escape(value)
-        prefix << "#{sanitized_name}=#{sanitized_value}"
-      end
-
-      prefix << "--"
     end
 
     def assert_success
@@ -97,7 +101,7 @@ module Hbc
       executable, *args = expanded_command
 
       raw_stdin, raw_stdout, raw_stderr, raw_wait_thr =
-        Open3.popen3(env, [executable, executable], *args, **options)
+        Open3.popen3([executable, executable], *args, **options)
 
       write_input_to(raw_stdin)
       raw_stdin.close_write

Library/Homebrew/cask/lib/hbc/utils/hash_validator.rb

@@ -2,6 +2,6 @@ module HashValidator
   def assert_valid_keys(*valid_keys)
     unknown_keys = keys - valid_keys
     return if unknown_keys.empty?
-    raise CaskError, %Q(Unknown keys: #{unknown_keys.inspect}. Running "#{UPDATE_CMD}" will likely fix it.)
+    raise %Q(Unknown keys: #{unknown_keys.inspect}. Running "brew update" will likely fix it.)
   end
 end

Library/Homebrew/test/cask/artifact/installer_spec.rb

@@ -0,0 +1,40 @@
+describe Hbc::Artifact::Installer, :cask do
+  let(:staged_path) { mktmpdir }
+  let(:cask) { instance_double("Cask", staged_path: staged_path, config: nil) }
+  subject(:installer) { described_class.new(cask, **args) }
+  let(:command) { Hbc::SystemCommand }
+
+  let(:args) { {} }
+
+  describe "#install_phase" do
+    context "when given a manual installer" do
+      let(:args) { { manual: "installer" } }
+
+      it "shows a message prompting to run the installer manually" do
+        expect {
+          installer.install_phase(command: command)
+        }.to output(%r{run the installer at\s+'#{staged_path}/installer'}).to_stdout
+      end
+    end
+
+    context "when given a script installer" do
+      let(:executable) { staged_path/"executable" }
+      let(:args) { { script: { executable: "executable" } } }
+
+      before(:each) do
+        FileUtils.touch executable
+      end
+
+      it "looks for the executable in HOMEBREW_PREFIX" do
+        expect(command).to receive(:run!).with(
+          executable,
+          a_hash_including(
+            env: { "PATH" => PATH.new("#{HOMEBREW_PREFIX}/bin", "#{HOMEBREW_PREFIX}/sbin", ENV["PATH"]) },
+          ),
+        )
+
+        installer.install_phase(command: command)
+      end
+    end
+  end
+end

Library/Homebrew/test/cask/dsl_spec.rb

@@ -520,7 +520,7 @@ describe Hbc::DSL, :cask do
       it "allows installer manual to be specified" do
         installer = cask.artifacts.first
         expect(installer).to be_a(Hbc::Artifact::Installer::ManualInstaller)
-        expect(installer.path).to eq(cask.staged_path.join("Caffeine.app"))
+        expect(installer.path).to eq(Pathname("Caffeine.app"))
       end
     end
   end

Library/Homebrew/test/cask/system_command_spec.rb

@@ -2,7 +2,7 @@ describe Hbc::SystemCommand, :cask do
   describe "#initialize" do
     let(:env_args) { ["bash", "-c", 'printf "%s" "${A?}" "${B?}" "${C?}"'] }
 
-    describe "given some environment variables" do
+    context "when given some environment variables" do
       subject {
         described_class.new(
           "env",
@@ -15,10 +15,10 @@ describe Hbc::SystemCommand, :cask do
       its("run!.stdout") { is_expected.to eq("123") }
 
       describe "the resulting command line" do
-        it "does not include the given variables" do
+        it "includes the given variables explicitly" do
           expect(Open3)
             .to receive(:popen3)
-            .with(a_hash_including("PATH"), ["env"] * 2, *env_args, {})
+            .with(["env", "env"], "A=1", "B=2", "C=3", "env", *env_args, {})
             .and_call_original
 
           subject.run!
@@ -26,7 +26,7 @@ describe Hbc::SystemCommand, :cask do
       end
     end
 
-    describe "given some environment variables and sudo: true" do
+    context "when given some environment variables and sudo: true" do
       subject {
         described_class.new(
           "env",
@@ -41,9 +41,8 @@ describe Hbc::SystemCommand, :cask do
         it "includes the given variables explicitly" do
           expect(Open3)
             .to receive(:popen3)
-            .with(an_instance_of(Hash), ["/usr/bin/sudo"] * 2,
+            .with(["/usr/bin/sudo", "/usr/bin/sudo"], "-E", "--",
-                "-E", a_string_starting_with("PATH="),
+                  "env", "A=1", "B=2", "C=3", "env", *env_args, {})
-                "A=1", "B=2", "C=3", "--", "env", *env_args, {})
             .and_wrap_original do |original_popen3, *_, &block|
               original_popen3.call("/usr/bin/true", &block)
             end
@@ -54,7 +53,7 @@ describe Hbc::SystemCommand, :cask do
     end
   end
 
-  describe "when the exit code is 0" do
+  context "when the exit code is 0" do
     describe "its result" do
       subject { described_class.run("/usr/bin/true") }
 
@@ -63,10 +62,10 @@ describe Hbc::SystemCommand, :cask do
     end
   end
 
-  describe "when the exit code is 1" do
+  context "when the exit code is 1" do
     let(:command) { "/usr/bin/false" }
 
-    describe "and the command must succeed" do
+    context "and the command must succeed" do
       it "throws an error" do
         expect {
           described_class.run!(command)
@@ -74,7 +73,7 @@ describe Hbc::SystemCommand, :cask do
       end
     end
 
-    describe "and the command does not have to succeed" do
+    context "and the command does not have to succeed" do
       describe "its result" do
         subject { described_class.run(command) }
 
@@ -84,7 +83,7 @@ describe Hbc::SystemCommand, :cask do
     end
   end
 
-  describe "given a pathname" do
+  context "when given a pathname" do
     let(:command) { "/bin/ls" }
     let(:path)    { Pathname(Dir.mktmpdir) }
 
@@ -100,7 +99,7 @@ describe Hbc::SystemCommand, :cask do
     end
   end
 
-  describe "with both STDOUT and STDERR output from upstream" do
+  context "with both STDOUT and STDERR output from upstream" do
     let(:command) { "/bin/bash" }
     let(:options) {
       { args: [
@@ -119,7 +118,7 @@ describe Hbc::SystemCommand, :cask do
       end
     end
 
-    describe "with default options" do
+    context "with default options" do
       it "echoes only STDERR" do
         expected = [2, 4, 6].map { |i| "#{i}\n" }.join
         expect {
@@ -130,7 +129,7 @@ describe Hbc::SystemCommand, :cask do
       include_examples("it returns '1 2 3 4 5 6'")
     end
 
-    describe "with print_stdout" do
+    context "with print_stdout" do
       before do
         options.merge!(print_stdout: true)
       end
@@ -144,7 +143,7 @@ describe Hbc::SystemCommand, :cask do
       include_examples("it returns '1 2 3 4 5 6'")
     end
 
-    describe "without print_stderr" do
+    context "without print_stderr" do
       before do
         options.merge!(print_stderr: false)
       end
@@ -158,7 +157,7 @@ describe Hbc::SystemCommand, :cask do
       include_examples("it returns '1 2 3 4 5 6'")
     end
 
-    describe "with print_stdout but without print_stderr" do
+    context "with print_stdout but without print_stderr" do
       before do
         options.merge!(print_stdout: true, print_stderr: false)
       end
@@ -174,7 +173,7 @@ describe Hbc::SystemCommand, :cask do
     end
   end
 
-  describe "with a very long STDERR output" do
+  context "with a very long STDERR output" do
     let(:command) { "/bin/bash" }
     let(:options) {
       { args: [
@@ -190,10 +189,23 @@ describe Hbc::SystemCommand, :cask do
     end
   end
 
-  describe "given an invalid variable name" do
+  context "when given an invalid variable name" do
     it "raises an ArgumentError" do
       expect { described_class.run("true", env: { "1ABC" => true }) }
         .to raise_error(ArgumentError, /variable name/)
     end
   end
+
+  it "looks for executables in a custom PATH" do
+    mktmpdir do |path|
+      (path/"tool").write <<~SH
+        #!/bin/sh
+        echo Hello, world!
+      SH
+
+      FileUtils.chmod "+x", path/"tool"
+
+      expect(described_class.run("tool", env: { "PATH" => path }).stdout).to include "Hello, world!"
+    end
+  end
 end

Library/Homebrew/test/support/helper/cask/fake_system_command.rb

@@ -1,5 +1,5 @@
 def sudo(*args)
-  ["/usr/bin/sudo", "-E", "PATH=#{ENV["PATH"]}", "--"] + args.flatten
+  ["/usr/bin/sudo", "-E", "--"] + args.flatten
 end
 
 module Hbc

Library/Homebrew/test/support/helper/spec/shared_examples/hbc_staged.rb

@@ -80,7 +80,7 @@ shared_examples Hbc::Staged do
     allow(staged).to receive(:Pathname).and_return(fake_pathname)
 
     Hbc::FakeSystemCommand.expects_command(
-      ["/usr/bin/sudo", "-E", "PATH=#{ENV["PATH"]}", "--", "/usr/sbin/chown", "-R", "--", "fake_user:staff", fake_pathname],
+      sudo("/usr/sbin/chown", "-R", "--", "fake_user:staff", fake_pathname),
     )
 
     staged.set_ownership(fake_pathname.to_s)
@@ -93,7 +93,7 @@ shared_examples Hbc::Staged do
     allow(staged).to receive(:Pathname).and_return(fake_pathname)
 
     Hbc::FakeSystemCommand.expects_command(
-      ["/usr/bin/sudo", "-E", "PATH=#{ENV["PATH"]}", "--", "/usr/sbin/chown", "-R", "--", "fake_user:staff", fake_pathname, fake_pathname],
+      sudo("/usr/sbin/chown", "-R", "--", "fake_user:staff", fake_pathname, fake_pathname),
     )
 
     staged.set_ownership([fake_pathname.to_s, fake_pathname.to_s])
@@ -105,7 +105,7 @@ shared_examples Hbc::Staged do
     allow(staged).to receive(:Pathname).and_return(fake_pathname)
 
     Hbc::FakeSystemCommand.expects_command(
-      ["/usr/bin/sudo", "-E", "PATH=#{ENV["PATH"]}", "--", "/usr/sbin/chown", "-R", "--", "other_user:other_group", fake_pathname],
+      sudo("/usr/sbin/chown", "-R", "--", "other_user:other_group", fake_pathname),
     )
 
     staged.set_ownership(fake_pathname.to_s, user: "other_user", group: "other_group")