diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index e12d0160f77c7..392c6673bacec 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -82,13 +82,7 @@ def move(adopt: false, force: false, verbose: false, predecessor: nil, reinstall ohai "Moving #{self.class.english_name} '#{source.basename}' to '#{target}'" - unless target.dirname.exist? - if target.dirname.ascend.find(&:directory?).writable? - target.dirname.mkpath - else - command.run!("/bin/mkdir", args: ["-p", target.dirname], sudo: true) - end - end + Utils.gain_permissions_mkpath(target.dirname, command: command) unless target.dirname.exist? if target.directory? if target.writable? diff --git a/Library/Homebrew/cask/artifact/symlinked.rb b/Library/Homebrew/cask/artifact/symlinked.rb index 3d920c1ede19e..0d65a40e3a890 100644 --- a/Library/Homebrew/cask/artifact/symlinked.rb +++ b/Library/Homebrew/cask/artifact/symlinked.rb @@ -43,7 +43,7 @@ def summarize_installed private - def link(force: false, **options) + def link(force: false, command: nil, **_options) unless source.exist? raise CaskError, "It seems the #{self.class.link_type_english_name.downcase} " \ @@ -57,26 +57,29 @@ def link(force: false, **options) if force && target.symlink? && (target.realpath == source.realpath || target.realpath.to_s.start_with?("#{cask.caskroom_path}/")) opoo "#{message}; overwriting." - target.delete + Utils.gain_permissions_remove(target, command: command) else raise CaskError, "#{message}." end end ohai "Linking #{self.class.english_name} '#{source.basename}' to '#{target}'" - create_filesystem_link(**options) + create_filesystem_link(command: command) end - def unlink(**) + def unlink(command: nil, **) return unless target.symlink? ohai "Unlinking #{self.class.english_name} '#{target}'" - target.delete + Utils.gain_permissions_remove(target, command: command) end - def create_filesystem_link(command: nil, **_) - target.dirname.mkpath - command.run!("/bin/ln", args: ["-h", "-f", "-s", "--", source, target]) + def create_filesystem_link(command: nil) + Utils.gain_permissions_mkpath(target.dirname, command: command) + + command.run! "/bin/ln", args: ["-h", "-f", "-s", "--", source, target], + sudo: !target.dirname.writable? + add_altname_metadata(source, target.basename, command: command) end end diff --git a/Library/Homebrew/cask/utils.rb b/Library/Homebrew/cask/utils.rb index dd5ba92ca0acb..877e3bde0b7f7 100644 --- a/Library/Homebrew/cask/utils.rb +++ b/Library/Homebrew/cask/utils.rb @@ -11,19 +11,44 @@ module Cask # # @api private module Utils + def self.gain_permissions_mkpath(path, command: SystemCommand) + dir = path.ascend.find(&:directory?) + return if path == dir + + if dir.writable? + path.mkpath + else + command.run!("/bin/mkdir", args: ["-p", "--", path], sudo: true) + end + end + def self.gain_permissions_remove(path, command: SystemCommand) - if path.respond_to?(:rmtree) && path.exist? - gain_permissions(path, ["-R"], command) do |p| - if p.parent.writable? + directory = false + permission_flags = if path.symlink? + ["-h"] + elsif path.directory? + directory = true + ["-R"] + elsif path.exist? + [] + else + # Nothing to remove. + return + end + + gain_permissions(path, permission_flags, command) do |p| + if p.parent.writable? + if directory p.rmtree else - command.run("/bin/rm", - args: ["-r", "-f", "--", p], - sudo: true) + FileUtils.rm_f p end + else + recursive_flag = directory ? ["-R"] : [] + command.run!("/bin/rm", + args: recursive_flag + ["-f", "--", p], + sudo: true) end - elsif File.symlink?(path) - gain_permissions(path, ["-h"], command, &FileUtils.method(:rm_f)) end end @@ -40,13 +65,10 @@ def self.gain_permissions(path, command_args, command) # dependent on whether the file argument has a trailing # slash. This should do the right thing, but is fragile. command.run("/usr/bin/chflags", - must_succeed: false, args: command_args + ["--", "000", path]) command.run("/bin/chmod", - must_succeed: false, args: command_args + ["--", "u+rwx", path]) command.run("/bin/chmod", - must_succeed: false, args: command_args + ["-N", path]) tried_permissions = true retry # rmtree diff --git a/Library/Homebrew/test/cask/artifact/app_spec.rb b/Library/Homebrew/test/cask/artifact/app_spec.rb index f3ba60547241d..978045a720bc5 100644 --- a/Library/Homebrew/test/cask/artifact/app_spec.rb +++ b/Library/Homebrew/test/cask/artifact/app_spec.rb @@ -171,21 +171,12 @@ end it "overwrites the existing app" do - expect(command).to receive(:run).with( - "/bin/chmod", args: [ - "-R", "--", "u+rwx", target_path - ], must_succeed: false - ).and_call_original - expect(command).to receive(:run).with( - "/bin/chmod", args: [ - "-R", "-N", target_path - ], must_succeed: false - ).and_call_original - expect(command).to receive(:run).with( - "/usr/bin/chflags", args: [ - "-R", "--", "000", target_path - ], must_succeed: false - ).and_call_original + expect(command).to receive(:run).with("/usr/bin/chflags", + args: ["-R", "--", "000", target_path]).and_call_original + expect(command).to receive(:run).with("/bin/chmod", + args: ["-R", "--", "u+rwx", target_path]).and_call_original + expect(command).to receive(:run).with("/bin/chmod", + args: ["-R", "-N", target_path]).and_call_original stdout = <<~EOS ==> Removing App '#{target_path}' diff --git a/Library/Homebrew/test/cask/utils_spec.rb b/Library/Homebrew/test/cask/utils_spec.rb new file mode 100644 index 0000000000000..634afaeb95b5f --- /dev/null +++ b/Library/Homebrew/test/cask/utils_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +describe Cask::Utils do + let(:command) { NeverSudoSystemCommand } + let(:dir) { mktmpdir } + let(:path) { dir/"a/b/c" } + let(:link) { dir/"link" } + + describe "::gain_permissions_mkpath" do + it "creates a directory" do + expect(path).not_to exist + described_class.gain_permissions_mkpath path, command: command + expect(path).to be_a_directory + described_class.gain_permissions_mkpath path, command: command + expect(path).to be_a_directory + end + + context "when parent directory is not writable" do + it "creates a directory with `sudo`" do + FileUtils.chmod "-w", dir + expect(dir).not_to be_writable + + expect(command).to receive(:run!).exactly(:once).and_wrap_original do |original, *args, **options| + FileUtils.chmod "+w", dir + original.call(*args, **options) + FileUtils.chmod "-w", dir + end + + expect(path).not_to exist + described_class.gain_permissions_mkpath path, command: command + expect(path).to be_a_directory + described_class.gain_permissions_mkpath path, command: command + expect(path).to be_a_directory + + expect(dir).not_to be_writable + FileUtils.chmod "+w", dir + end + end + end + + describe "::gain_permissions_remove" do + it "removes the symlink, not the file it points to" do + path.dirname.mkpath + FileUtils.touch path + FileUtils.ln_s path, link + + expect(path).to be_a_file + expect(link).to be_a_symlink + expect(link.realpath).to eq path + + described_class.gain_permissions_remove link, command: command + + expect(path).to be_a_file + expect(link).not_to exist + + described_class.gain_permissions_remove path, command: command + + expect(path).not_to exist + end + + it "removes the symlink, not the directory it points to" do + path.mkpath + FileUtils.ln_s path, link + + expect(path).to be_a_directory + expect(link).to be_a_symlink + expect(link.realpath).to eq path + + described_class.gain_permissions_remove link, command: command + + expect(path).to be_a_directory + expect(link).not_to exist + + described_class.gain_permissions_remove path, command: command + + expect(path).not_to exist + end + end +end