From 2e8232de39721f542b4c7f8ffbb22c7a922cd8d1 Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Thu, 11 May 2023 10:50:12 -0500 Subject: [PATCH 1/9] Replace wildcard copy with a loop over children. Fixes one of the errors in https://github.com/orgs/Homebrew/discussions/4498 (specifically "cp: [...].app/*: No such file or directory"). --- Library/Homebrew/cask/artifact/moved.rb | 6 ++++-- .../Homebrew/test/cask/artifact/app_spec.rb | 21 +++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index 48861a0f61..ee878de0a5 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -94,8 +94,10 @@ module Cask if target.writable? source.children.each { |child| FileUtils.move(child, target + child.basename) } else - command.run!("/bin/cp", args: ["-pR", "#{source}/*", "#{source}/.*", "#{target}/"], - sudo: true) + source.children.each do |child| + command.run!("/bin/cp", args: ["-pR", child, target + child.basename], + sudo: true) + end end Quarantine.copy_xattrs(source, target) source.rmtree diff --git a/Library/Homebrew/test/cask/artifact/app_spec.rb b/Library/Homebrew/test/cask/artifact/app_spec.rb index 796f03f19f..da92098599 100644 --- a/Library/Homebrew/test/cask/artifact/app_spec.rb +++ b/Library/Homebrew/test/cask/artifact/app_spec.rb @@ -334,5 +334,26 @@ describe Cask::Artifact::App, :cask do expect(contents_path).to exist end + + it "properly handles non-writable directories" do + install_phase + + contents_path = target_path.join("Contents") + + allow(target_path).to receive(:writable?).and_return false + allow(command).to receive(:run!).and_call_original + allow(command).to receive(:run!) + .with("/bin/cp", args: ["-pR", source_path.join("Contents"), contents_path], + sudo: true) + .and_wrap_original do |original_method, *args, **kwargs| + original_method.call(*args, sudo_as_root: false, **kwargs) + end + + app.uninstall_phase(command: command, force: force, successor: cask) + expect(contents_path).not_to exist + + app.install_phase(command: command, adopt: adopt, force: force, predecessor: cask) + expect(contents_path).to exist + end end end From a1780c842c45d31eac3f22171767422455575f57 Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Thu, 11 May 2023 12:44:35 -0500 Subject: [PATCH 2/9] Improve unit test to make sure the sudo version is used. --- Library/Homebrew/test/cask/artifact/app_spec.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/test/cask/artifact/app_spec.rb b/Library/Homebrew/test/cask/artifact/app_spec.rb index da92098599..40c90f5059 100644 --- a/Library/Homebrew/test/cask/artifact/app_spec.rb +++ b/Library/Homebrew/test/cask/artifact/app_spec.rb @@ -340,17 +340,20 @@ describe Cask::Artifact::App, :cask do contents_path = target_path.join("Contents") - allow(target_path).to receive(:writable?).and_return false + expect(app.target).to receive(:writable?).at_least(:once).and_return false allow(command).to receive(:run!).and_call_original allow(command).to receive(:run!) .with("/bin/cp", args: ["-pR", source_path.join("Contents"), contents_path], sudo: true) .and_wrap_original do |original_method, *args, **kwargs| - original_method.call(*args, sudo_as_root: false, **kwargs) + original_method.call(*args, **kwargs, sudo: false) end + expect(FileUtils).not_to receive(:move).with(source_path.join("Contents"), contents_path) app.uninstall_phase(command: command, force: force, successor: cask) expect(contents_path).not_to exist + expect(target_path).to exist + expect(source_path.join("Contents")).to exist app.install_phase(command: command, adopt: adopt, force: force, predecessor: cask) expect(contents_path).to exist From d0e64e2c57232e8866d6d3eebe91819ce552b8b6 Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Thu, 11 May 2023 12:46:05 -0500 Subject: [PATCH 3/9] Use / for path concatenation. --- Library/Homebrew/cask/artifact/moved.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index ee878de0a5..429a0945a8 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -95,7 +95,7 @@ module Cask source.children.each { |child| FileUtils.move(child, target + child.basename) } else source.children.each do |child| - command.run!("/bin/cp", args: ["-pR", child, target + child.basename], + command.run!("/bin/cp", args: ["-pR", child, target/child.basename], sudo: true) end end From 29c992172666e4bad98d835b91c3d6292f0fa6f1 Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Thu, 11 May 2023 12:47:37 -0500 Subject: [PATCH 4/9] Also use / above. --- Library/Homebrew/cask/artifact/moved.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index 429a0945a8..ab5212d0f8 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -92,7 +92,7 @@ module Cask if target.directory? if target.writable? - source.children.each { |child| FileUtils.move(child, target + child.basename) } + source.children.each { |child| FileUtils.move(child, target/child.basename) } else source.children.each do |child| command.run!("/bin/cp", args: ["-pR", child, target/child.basename], From 84ad387661b2de5ed23b9319c6a9808e69ea4dd3 Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Thu, 11 May 2023 13:29:56 -0500 Subject: [PATCH 5/9] Clean up code for feedback. --- Library/Homebrew/cask/artifact/moved.rb | 6 ++---- Library/Homebrew/test/cask/artifact/app_spec.rb | 10 ++++------ 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index ab5212d0f8..e12d0160f7 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -94,10 +94,8 @@ module Cask if target.writable? source.children.each { |child| FileUtils.move(child, target/child.basename) } else - source.children.each do |child| - command.run!("/bin/cp", args: ["-pR", child, target/child.basename], - sudo: true) - end + command.run!("/bin/cp", args: ["-pR", *source.children, target], + sudo: true) end Quarantine.copy_xattrs(source, target) source.rmtree diff --git a/Library/Homebrew/test/cask/artifact/app_spec.rb b/Library/Homebrew/test/cask/artifact/app_spec.rb index 40c90f5059..3cf3b2eaa9 100644 --- a/Library/Homebrew/test/cask/artifact/app_spec.rb +++ b/Library/Homebrew/test/cask/artifact/app_spec.rb @@ -2,7 +2,7 @@ describe Cask::Artifact::App, :cask do let(:cask) { Cask::CaskLoader.load(cask_path("local-caffeine")) } - let(:command) { SystemCommand } + let(:command) { NeverSudoSystemCommand } let(:adopt) { false } let(:force) { false } let(:app) { cask.artifacts.find { |a| a.is_a?(described_class) } } @@ -342,12 +342,10 @@ describe Cask::Artifact::App, :cask do expect(app.target).to receive(:writable?).at_least(:once).and_return false allow(command).to receive(:run!).and_call_original - allow(command).to receive(:run!) - .with("/bin/cp", args: ["-pR", source_path.join("Contents"), contents_path], + expect(command).to receive(:run!) + .with("/bin/cp", args: ["-pR", source_path.join("Contents"), target_path], sudo: true) - .and_wrap_original do |original_method, *args, **kwargs| - original_method.call(*args, **kwargs, sudo: false) - end + expect(command).to receive(:run!).with(any_args) expect(FileUtils).not_to receive(:move).with(source_path.join("Contents"), contents_path) app.uninstall_phase(command: command, force: force, successor: cask) From 7a108d3db05c5cb01b69606126c2ef5b785eec16 Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Thu, 11 May 2023 17:09:27 -0500 Subject: [PATCH 6/9] Merge and_call_original instruction into expect() calls. --- Library/Homebrew/test/cask/artifact/app_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/test/cask/artifact/app_spec.rb b/Library/Homebrew/test/cask/artifact/app_spec.rb index 3cf3b2eaa9..93007ffb11 100644 --- a/Library/Homebrew/test/cask/artifact/app_spec.rb +++ b/Library/Homebrew/test/cask/artifact/app_spec.rb @@ -341,11 +341,11 @@ describe Cask::Artifact::App, :cask do contents_path = target_path.join("Contents") expect(app.target).to receive(:writable?).at_least(:once).and_return false - allow(command).to receive(:run!).and_call_original expect(command).to receive(:run!) .with("/bin/cp", args: ["-pR", source_path.join("Contents"), target_path], sudo: true) - expect(command).to receive(:run!).with(any_args) + .and_call_original + expect(command).to receive(:run!).with(any_args).and_call_original expect(FileUtils).not_to receive(:move).with(source_path.join("Contents"), contents_path) app.uninstall_phase(command: command, force: force, successor: cask) From 5e23a0563a9f8f0689fcc67b6825f27e2019c312 Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Thu, 11 May 2023 17:10:41 -0500 Subject: [PATCH 7/9] Switch base case expect() to allow(). --- Library/Homebrew/test/cask/artifact/app_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/test/cask/artifact/app_spec.rb b/Library/Homebrew/test/cask/artifact/app_spec.rb index 93007ffb11..6efde45942 100644 --- a/Library/Homebrew/test/cask/artifact/app_spec.rb +++ b/Library/Homebrew/test/cask/artifact/app_spec.rb @@ -345,7 +345,7 @@ describe Cask::Artifact::App, :cask do .with("/bin/cp", args: ["-pR", source_path.join("Contents"), target_path], sudo: true) .and_call_original - expect(command).to receive(:run!).with(any_args).and_call_original + allow(command).to receive(:run!).with(any_args).and_call_original expect(FileUtils).not_to receive(:move).with(source_path.join("Contents"), contents_path) app.uninstall_phase(command: command, force: force, successor: cask) From 4a3b8923f21c64fe6adaefb663e3a73a6f29f81f Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Fri, 12 May 2023 09:52:57 -0500 Subject: [PATCH 8/9] More test cleanup. --- Library/Homebrew/test/cask/artifact/app_spec.rb | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/Library/Homebrew/test/cask/artifact/app_spec.rb b/Library/Homebrew/test/cask/artifact/app_spec.rb index 6efde45942..eefdc2521e 100644 --- a/Library/Homebrew/test/cask/artifact/app_spec.rb +++ b/Library/Homebrew/test/cask/artifact/app_spec.rb @@ -338,23 +338,24 @@ describe Cask::Artifact::App, :cask do it "properly handles non-writable directories" do install_phase - contents_path = target_path.join("Contents") + source_contents_path = source_path.join("Contents") + target_contents_path = target_path.join("Contents") expect(app.target).to receive(:writable?).at_least(:once).and_return false + allow(command).to receive(:run!).with(any_args).and_call_original expect(command).to receive(:run!) - .with("/bin/cp", args: ["-pR", source_path.join("Contents"), target_path], + .with("/bin/cp", args: ["-pR", source_contents_path, target_path], sudo: true) .and_call_original - allow(command).to receive(:run!).with(any_args).and_call_original - expect(FileUtils).not_to receive(:move).with(source_path.join("Contents"), contents_path) + expect(FileUtils).not_to receive(:move).with(source_contents_path, target_contents_path) app.uninstall_phase(command: command, force: force, successor: cask) - expect(contents_path).not_to exist + expect(target_contents_path).not_to exist expect(target_path).to exist - expect(source_path.join("Contents")).to exist + expect(source_contents_path).to exist app.install_phase(command: command, adopt: adopt, force: force, predecessor: cask) - expect(contents_path).to exist + expect(target_contents_path).to exist end end end From b1c7f12fbbb9566a4eaac87996d74e78601e481c Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Fri, 12 May 2023 09:57:12 -0500 Subject: [PATCH 9/9] Clean up allow and expect. --- Library/Homebrew/test/cask/artifact/app_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/test/cask/artifact/app_spec.rb b/Library/Homebrew/test/cask/artifact/app_spec.rb index eefdc2521e..f3ba605472 100644 --- a/Library/Homebrew/test/cask/artifact/app_spec.rb +++ b/Library/Homebrew/test/cask/artifact/app_spec.rb @@ -341,13 +341,14 @@ describe Cask::Artifact::App, :cask do source_contents_path = source_path.join("Contents") target_contents_path = target_path.join("Contents") - expect(app.target).to receive(:writable?).at_least(:once).and_return false + allow(app.target).to receive(:writable?).and_return false allow(command).to receive(:run!).with(any_args).and_call_original + expect(command).to receive(:run!) .with("/bin/cp", args: ["-pR", source_contents_path, target_path], sudo: true) .and_call_original - expect(FileUtils).not_to receive(:move).with(source_contents_path, target_contents_path) + expect(FileUtils).not_to receive(:move).with(source_contents_path, an_instance_of(Pathname)) app.uninstall_phase(command: command, force: force, successor: cask) expect(target_contents_path).not_to exist