diff --git a/lib/hbc/artifact/moved.rb b/lib/hbc/artifact/moved.rb index d78a5642f1bd..8d229fe7d2b4 100644 --- a/lib/hbc/artifact/moved.rb +++ b/lib/hbc/artifact/moved.rb @@ -22,7 +22,7 @@ def install_phase each_artifact do |artifact| load_specification(artifact) next unless preflight_checks - delete if File.exist?(target) && force + delete if Hbc::Utils.path_occupied?(target) && force move end end @@ -64,7 +64,7 @@ def load_specification(artifact_spec) end def preflight_checks - if target.exist? + if Hbc::Utils.path_occupied?(target) if force ohai(warning_target_exists { |s| s << 'overwriting.' }) else @@ -96,7 +96,7 @@ def delete raise Hbc::CaskError.new( "Cannot remove undeletable #{english_name}") when force - Hbc::Utils.permissions_rmtree(target, command: @command) + Hbc::Utils.gain_permissions_remove(target, command: @command) else target.rmtree end diff --git a/lib/hbc/installer.rb b/lib/hbc/installer.rb index 36d58adf51b7..883787c3b210 100644 --- a/lib/hbc/installer.rb +++ b/lib/hbc/installer.rb @@ -312,21 +312,23 @@ def zap purge_caskroom_path end - def permissions_rmtree(path) - Hbc::Utils.permissions_rmtree(path, command: @command) + def gain_permissions_remove(path) + Hbc::Utils.gain_permissions_remove(path, command: @command) end def purge_versioned_files odebug "Purging files for version #{@cask.version} of Cask #{@cask}" # versioned staged distribution - permissions_rmtree(@cask.staged_path) + gain_permissions_remove(@cask.staged_path) # Homebrew-cask metadata if @cask.metadata_versioned_container_path.respond_to?(:children) and @cask.metadata_versioned_container_path.exist? @cask.metadata_versioned_container_path.children.each do |subdir| - permissions_rmtree subdir unless PERSISTENT_METADATA_SUBDIRS.include?(subdir.basename) + unless PERSISTENT_METADATA_SUBDIRS.include?(subdir.basename) + gain_permissions_remove(subdir) + end end end Hbc::Utils.rmdir_if_possible(@cask.metadata_versioned_container_path) @@ -338,6 +340,6 @@ def purge_versioned_files def purge_caskroom_path odebug "Purging all staged versions of Cask #{@cask}" - permissions_rmtree(@cask.caskroom_path) + gain_permissions_remove(@cask.caskroom_path) end end diff --git a/lib/hbc/utils.rb b/lib/hbc/utils.rb index d50567b68971..2a2b93e58841 100644 --- a/lib/hbc/utils.rb +++ b/lib/hbc/utils.rb @@ -159,41 +159,52 @@ def self.rmdir_if_possible(dir) end end - def self.permissions_rmtree(path, options = {}) - command = options.fetch(:command, Hbc::SystemCommand) + def self.gain_permissions_remove(path, options = {}) if path.respond_to?(:rmtree) and path.exist? - tried_permissions = false - tried_ownership = false - begin + gain_permissions(path, ['-R'], options) do |path| path.rmtree - rescue StandardError => e - # in case of permissions problems - unless tried_permissions - # todo Better handling for the case where path is a symlink. - # The -h and -R flags cannot be combined, and behavior is - # 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: ['-R', '--', '000', path]) - command.run('/bin/chmod', must_succeed: false, - args: ['-R', '--', 'u+rwx', path]) - command.run('/bin/chmod', must_succeed: false, - args: ['-R', '-N', path]) - tried_permissions = true - retry # rmtree - end - unless tried_ownership - # in case of ownership problems - # todo Further examine files to see if ownership is the problem - # before using sudo+chown - ohai "Using sudo to gain ownership of path '#{path}'" - command.run('/usr/sbin/chown', :args => ['-R', '--', current_user, path], - :sudo => true) - tried_ownership = true - # retry chflags/chmod after chown - tried_permissions = false - retry # rmtree - end + end + elsif File.symlink?(path) + gain_permissions(path, ['-h'], options) do |path| + FileUtils.rm_f(path) + end + end + end + + def self.gain_permissions(path, command_args, options = {}) + command = options.fetch(:command, Hbc::SystemCommand) + tried_permissions = false + tried_ownership = false + begin + yield path + rescue StandardError => e + # in case of permissions problems + unless tried_permissions + # todo Better handling for the case where path is a symlink. + # The -h and -R flags cannot be combined, and behavior is + # 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 + end + unless tried_ownership + # in case of ownership problems + # todo Further examine files to see if ownership is the problem + # before using sudo+chown + ohai "Using sudo to gain ownership of path '#{path}'" + command.run('/usr/sbin/chown', + :args => command_args + ['--', current_user, path], + :sudo => true) + tried_ownership = true + # retry chflags/chmod after chown + tried_permissions = false + retry # rmtree end end end @@ -236,6 +247,10 @@ def self.file_is_descendant(file, dir) return false end + def self.path_occupied?(path) + File.exist?(path) || File.symlink?(path) + end + def self.error_message_with_suggestions <<-EOS.undent #{ Tty.reset.bold } diff --git a/test/cask/artifact/app_test.rb b/test/cask/artifact/app_test.rb index 3276d6c333c3..0f6d96b0b1ed 100644 --- a/test/cask/artifact/app_test.rb +++ b/test/cask/artifact/app_test.rb @@ -177,6 +177,61 @@ end end + describe "when the target is a broken symlink" do + let(:target_path) { + Hbc.appdir.join('Caffeine.app') + } + + let(:deleted_path) { + local_caffeine.staged_path.join( + 'Deleted.app') + } + + before do + deleted_path.mkdir + File.symlink(deleted_path, target_path) + deleted_path.rmdir + end + + it "leaves the target alone" do + cask = local_caffeine + TestHelper.must_output(self, lambda { + Hbc::Artifact::App.new(cask).install_phase + }, "==> It seems there is already an App at '#{target_path}'; not moving.") + + File.symlink?(target_path).must_equal true + end + + describe "given the force option" do + let(:install_phase) { + lambda { + Hbc::Artifact::App.new( + local_caffeine, force: true).install_phase + } + } + + it "overwrites the existing app" do + cask = local_caffeine + + expected = [ + "==> It seems there is already an App at '#{target_path}'; overwriting.", + "==> Removing App: '#{target_path}'", + "==> Moving App 'Caffeine.app' to '#{target_path}'" + ] + TestHelper.must_output(self, install_phase, + expected.join("\n")) + + source_path = cask.staged_path.join('Caffeine.app') + + File.exist?(source_path).must_equal false + File.ftype(target_path).must_equal 'directory' + + contents_path = target_path.join('Contents/Info.plist') + File.exist?(contents_path).must_equal true + end + end + end + it "gives a warning if the source doesn't exist" do cask = local_caffeine staged_app_path = cask.staged_path.join('Caffeine.app')