From 8c4f29d9832785fcc7e7aa1544643dd03762b2a9 Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Mon, 3 Apr 2023 17:13:57 -0500 Subject: [PATCH 01/11] Don't remove cask directories when upgrading. Fixes Homebrew/homebrew-cask#102721. --- Library/Homebrew/cask/artifact/moved.rb | 82 +++++++++++++++++-------- Library/Homebrew/cask/installer.rb | 2 +- 2 files changed, 57 insertions(+), 27 deletions(-) diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index 477cd0470a..2ec672683e 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -34,39 +34,50 @@ module Cask private - def move(adopt: false, force: false, verbose: false, command: nil, **options) + def move(adopt: false, force: false, verbose: false, upgrade: false, command: nil, **options) unless source.exist? raise CaskError, "It seems the #{self.class.english_name} source '#{source}' is not there." end if Utils.path_occupied?(target) - if adopt - ohai "Adopting existing #{self.class.english_name} at '#{target}'" - same = command.run( - "/usr/bin/diff", - args: ["--recursive", "--brief", source, target], - verbose: verbose, - print_stdout: verbose, - ).success? + if upgrade && target.directory? && target.children.empty? + # An upgrade removed the directory contents but left the directory itself (see below). + unless source.directory? + if target.parent.writable? && !force + target.rmdir + else + Utils.gain_permissions_remove(target, command: command) + end + end + else + if adopt + ohai "Adopting existing #{self.class.english_name} at '#{target}'" + same = command.run( + "/usr/bin/diff", + args: ["--recursive", "--brief", source, target], + verbose: verbose, + print_stdout: verbose, + ).success? - unless same - raise CaskError, - "It seems the existing #{self.class.english_name} is different from " \ - "the one being installed." + unless same + raise CaskError, + "It seems the existing #{self.class.english_name} is different from " \ + "the one being installed." + end + + # Remove the source as we don't need to move it to the target location + source.rmtree + + return post_move(command) end - # Remove the source as we don't need to move it to the target location - source.rmtree + message = "It seems there is already #{self.class.english_article} " \ + "#{self.class.english_name} at '#{target}'" + raise CaskError, "#{message}." unless force - return post_move(command) + opoo "#{message}; overwriting." + delete(target, force: force, command: command, **options) end - - message = "It seems there is already #{self.class.english_article} " \ - "#{self.class.english_name} at '#{target}'" - raise CaskError, "#{message}." unless force - - opoo "#{message}; overwriting." - delete(target, force: force, command: command, **options) end ohai "Moving #{self.class.english_name} '#{source.basename}' to '#{target}'" @@ -79,7 +90,16 @@ module Cask end end - if target.dirname.writable? + if target.directory? + 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) + end + # TODO: copy extended attributes + source.rmtree + elsif target.dirname.writable? FileUtils.move(source, target) else # default sudo user isn't necessarily able to write to Homebrew's locations @@ -125,13 +145,23 @@ module Cask delete(target, force: force, command: command, **options) end - def delete(target, force: false, command: nil, **_) + def delete(target, force: false, upgrade: false, command: nil, **_) ohai "Removing #{self.class.english_name} '#{target}'" raise CaskError, "Cannot remove undeletable #{self.class.english_name}." if MacOS.undeletable?(target) return unless Utils.path_occupied?(target) - if target.parent.writable? && !force + if upgrade && target.directory? + # If an app folder is deleted, macOS considers the app uninstalled and removes some data. + # Remove only the contents to handle this case. + target.children.each do |child| + if target.writable? && !force + child.rmtree + else + Utils.gain_permissions_remove(child, command: command) + end + end + elsif target.parent.writable? && !force target.rmtree else Utils.gain_permissions_remove(target, command: command) diff --git a/Library/Homebrew/cask/installer.rb b/Library/Homebrew/cask/installer.rb index e5d7ea2559..f92469e8e1 100644 --- a/Library/Homebrew/cask/installer.rb +++ b/Library/Homebrew/cask/installer.rb @@ -234,7 +234,7 @@ on_request: true) next if artifact.is_a?(Artifact::Binary) && !binaries? - artifact.install_phase(command: @command, verbose: verbose?, adopt: adopt?, force: force?) + artifact.install_phase(command: @command, verbose: verbose?, adopt: adopt?, force: force?, upgrade: upgrade?) already_installed_artifacts.unshift(artifact) end From 3f8998a4fc2153f9a82374cf6ec22979e27a0585 Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Mon, 3 Apr 2023 20:21:37 -0500 Subject: [PATCH 02/11] Reset extended attributes of the base directories. --- Library/Homebrew/cask/artifact/moved.rb | 5 +- Library/Homebrew/cask/quarantine.rb | 14 ++++ Library/Homebrew/cask/utils/copy-xattrs.swift | 80 +++++++++++++++++++ 3 files changed, 98 insertions(+), 1 deletion(-) create mode 100755 Library/Homebrew/cask/utils/copy-xattrs.swift diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index 2ec672683e..fa1915364a 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -2,6 +2,7 @@ # frozen_string_literal: true require "cask/artifact/relocated" +require "cask/quarantine" module Cask module Artifact @@ -97,7 +98,9 @@ module Cask command.run!("/bin/cp", args: ["-pR", "#{source}/*", "#{source}/.*", "#{target}/"], sudo: true) end - # TODO: copy extended attributes + unless Quarantine.copy_xattrs(source, target) + opoo "Unable to transfer extended attributes on the root directory" + end source.rmtree elsif target.dirname.writable? FileUtils.move(source, target) diff --git a/Library/Homebrew/cask/quarantine.rb b/Library/Homebrew/cask/quarantine.rb index 7333b4e231..ed36a41d82 100644 --- a/Library/Homebrew/cask/quarantine.rb +++ b/Library/Homebrew/cask/quarantine.rb @@ -14,6 +14,7 @@ module Cask QUARANTINE_ATTRIBUTE = "com.apple.quarantine" QUARANTINE_SCRIPT = (HOMEBREW_LIBRARY_PATH/"cask/utils/quarantine.swift").freeze + COPY_XATTRS_SCRIPT = (HOMEBREW_LIBRARY_PATH/"cask/utils/copy-xattrs.swift").freeze def self.swift @swift ||= DevelopmentTools.locate("swift") @@ -174,5 +175,18 @@ module Cask raise CaskQuarantinePropagationError.new(to, quarantiner.stderr) end + + def self.copy_xattrs(from, to) + odebug "Copying xattrs from #{from} to #{to}" + + copier = system_command!(swift, + args: [ + *swift_target_args, + COPY_XATTRS_SCRIPT, + from, + to, + ]) + copier.success? + end end end diff --git a/Library/Homebrew/cask/utils/copy-xattrs.swift b/Library/Homebrew/cask/utils/copy-xattrs.swift new file mode 100755 index 0000000000..f118290c81 --- /dev/null +++ b/Library/Homebrew/cask/utils/copy-xattrs.swift @@ -0,0 +1,80 @@ +#!/usr/bin/swift + +import Foundation + +struct SwiftErr: TextOutputStream { + public static var stream = SwiftErr() + + mutating func write(_ string: String) { + fputs(string, stderr) + } +} + +guard CommandLine.arguments.count >= 3 else { + print("Usage: swift copy-xattrs.swift ") + exit(2) +} + +CommandLine.arguments[2].withCString { destPath in + let destNamesLen = listxattr(destPath, nil, 0, 0) + if destNamesLen == -1 { + print("listxattr for destination failed: \(errno)", to: &SwiftErr.stream) + exit(1) + } + let destNamesBuf = UnsafeMutablePointer.allocate(capacity: destNamesLen) + if listxattr(destPath, destNamesBuf, destNamesLen, 0) != destNamesLen { + print("Attributes changed during system call", to: &SwiftErr.stream) + exit(1) + } + + var destNamesIdx = 0 + while destNamesIdx < destNamesLen { + let attribute = destNamesBuf + destNamesIdx + + if removexattr(destPath, attribute, 0) != 0 { + print("removexattr for \(String(cString: attribute)) failed: \(errno)", to: &SwiftErr.stream) + exit(1) + } + + destNamesIdx += strlen(attribute) + 1 + } + destNamesBuf.deallocate() + + CommandLine.arguments[1].withCString { sourcePath in + let sourceNamesLen = listxattr(sourcePath, nil, 0, 0) + if sourceNamesLen == -1 { + print("listxattr for source failed: \(errno)", to: &SwiftErr.stream) + exit(1) + } + let sourceNamesBuf = UnsafeMutablePointer.allocate(capacity: sourceNamesLen) + if listxattr(sourcePath, sourceNamesBuf, sourceNamesLen, 0) != sourceNamesLen { + print("Attributes changed during system call", to: &SwiftErr.stream) + exit(1) + } + + var sourceNamesIdx = 0 + while sourceNamesIdx < sourceNamesLen { + let attribute = sourceNamesBuf + sourceNamesIdx + + let valueLen = getxattr(sourcePath, attribute, nil, 0, 0, 0) + if valueLen == -1 { + print("getxattr for \(String(cString: attribute)) failed: \(errno)", to: &SwiftErr.stream) + exit(1) + } + let valueBuf = UnsafeMutablePointer.allocate(capacity: valueLen) + if getxattr(sourcePath, attribute, valueBuf, valueLen, 0, 0) != valueLen { + print("Attributes changed during system call", to: &SwiftErr.stream) + exit(1) + } + + if setxattr(destPath, attribute, valueBuf, valueLen, 0, 0) != 0 { + print("setxattr for \(String(cString: attribute)) failed: \(errno)", to: &SwiftErr.stream) + exit(1) + } + + valueBuf.deallocate() + sourceNamesIdx += strlen(attribute) + 1 + } + sourceNamesBuf.deallocate() + } +} From 5c9fa845a8eddb7f70e66668dfb26fe194a9eedd Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Tue, 4 Apr 2023 10:21:44 -0500 Subject: [PATCH 03/11] Add unit test for upgrading. --- .../Homebrew/test/cask/artifact/app_spec.rb | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/Library/Homebrew/test/cask/artifact/app_spec.rb b/Library/Homebrew/test/cask/artifact/app_spec.rb index 1c84d2efcc..47bcd14a59 100644 --- a/Library/Homebrew/test/cask/artifact/app_spec.rb +++ b/Library/Homebrew/test/cask/artifact/app_spec.rb @@ -311,4 +311,29 @@ describe Cask::Artifact::App, :cask do end end end + + describe "upgrade" do + # Fix for https://github.com/Homebrew/homebrew-cask/issues/102721 + it "reuses the same directory" do + install_phase + + contents_path = target_path.join("Contents/Info.plist") + + expect(target_path).to exist + inode = target_path.stat.ino + expect(contents_path).to exist + + app.uninstall_phase(command: command, force: force, upgrade: true) + + expect(target_path).to exist + expect(target_path.children).to be_empty + expect(contents_path).not_to exist + + app.install_phase(command: command, adopt: adopt, force: force, upgrade: true) + expect(target_path).to exist + expect(target_path.stat.ino).to eq(inode) + + expect(contents_path).to exist + end + end end From dcc4ae76e8f1eadeff7d6f8fd36c2724d67f4db0 Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Tue, 4 Apr 2023 11:36:27 -0500 Subject: [PATCH 04/11] Also keep app directories with "brew reinstall". --- .../cask/artifact/abstract_uninstall.rb | 4 +-- Library/Homebrew/cask/artifact/moved.rb | 9 ++++--- Library/Homebrew/cask/cmd/reinstall.rb | 3 ++- Library/Homebrew/cask/installer.rb | 27 ++++++++++--------- .../Homebrew/test/cask/artifact/app_spec.rb | 4 +-- 5 files changed, 26 insertions(+), 21 deletions(-) diff --git a/Library/Homebrew/cask/artifact/abstract_uninstall.rb b/Library/Homebrew/cask/artifact/abstract_uninstall.rb index 7b4d522a7a..2f7f17595f 100644 --- a/Library/Homebrew/cask/artifact/abstract_uninstall.rb +++ b/Library/Homebrew/cask/artifact/abstract_uninstall.rb @@ -270,8 +270,8 @@ module Cask end end - def uninstall_login_item(*login_items, command: nil, upgrade: false, **_) - return if upgrade + def uninstall_login_item(*login_items, command: nil, upgrade_or_reinstall: false, **_) + return if upgrade_or_reinstall apps = cask.artifacts.select { |a| a.class.dsl_key == :app } derived_login_items = apps.map { |a| { path: a.target } } diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index fa1915364a..f3fa07be6e 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -35,13 +35,14 @@ module Cask private - def move(adopt: false, force: false, verbose: false, upgrade: false, command: nil, **options) + def move(adopt: false, force: false, verbose: false, upgrade_or_reinstall: false, reinstall: false, + command: nil, **options) unless source.exist? raise CaskError, "It seems the #{self.class.english_name} source '#{source}' is not there." end if Utils.path_occupied?(target) - if upgrade && target.directory? && target.children.empty? + if upgrade_or_reinstall && target.directory? && target.children.empty? # An upgrade removed the directory contents but left the directory itself (see below). unless source.directory? if target.parent.writable? && !force @@ -148,13 +149,13 @@ module Cask delete(target, force: force, command: command, **options) end - def delete(target, force: false, upgrade: false, command: nil, **_) + def delete(target, force: false, upgrade_or_reinstall: false, command: nil, **_) ohai "Removing #{self.class.english_name} '#{target}'" raise CaskError, "Cannot remove undeletable #{self.class.english_name}." if MacOS.undeletable?(target) return unless Utils.path_occupied?(target) - if upgrade && target.directory? + if upgrade_or_reinstall && target.directory? # If an app folder is deleted, macOS considers the app uninstalled and removes some data. # Remove only the contents to handle this case. target.children.each do |child| diff --git a/Library/Homebrew/cask/cmd/reinstall.rb b/Library/Homebrew/cask/cmd/reinstall.rb index 9637805450..2812c055b2 100644 --- a/Library/Homebrew/cask/cmd/reinstall.rb +++ b/Library/Homebrew/cask/cmd/reinstall.rb @@ -41,6 +41,7 @@ module Cask force: force, skip_cask_deps: skip_cask_deps, require_sha: require_sha, + reinstall: true, quarantine: quarantine, zap: zap, }.compact @@ -48,7 +49,7 @@ module Cask options[:quarantine] = true if options[:quarantine].nil? casks.each do |cask| - Installer.new(cask, **options).reinstall + Installer.new(cask, **options).install end end end diff --git a/Library/Homebrew/cask/installer.rb b/Library/Homebrew/cask/installer.rb index f92469e8e1..96ddcce544 100644 --- a/Library/Homebrew/cask/installer.rb +++ b/Library/Homebrew/cask/installer.rb @@ -22,7 +22,7 @@ module Cask def initialize(cask, command: SystemCommand, force: false, adopt: false, skip_cask_deps: false, binaries: true, verbose: false, - zap: false, require_sha: false, upgrade: false, + zap: false, require_sha: false, upgrade: false, reinstall: false, installed_as_dependency: false, quarantine: true, verify_download_integrity: true, quiet: false) @cask = cask @@ -34,7 +34,7 @@ module Cask @verbose = verbose @zap = zap @require_sha = require_sha - @reinstall = false + @reinstall = reinstall @upgrade = upgrade @installed_as_dependency = installed_as_dependency @quarantine = quarantine @@ -143,17 +143,11 @@ on_request: true) end end - def reinstall - odebug "Cask::Installer#reinstall" - @reinstall = true - install - end - def uninstall_existing_cask return unless @cask.installed? # Always force uninstallation, ignore method parameter - cask_installer = Installer.new(@cask, verbose: verbose?, force: true, upgrade: upgrade?) + cask_installer = Installer.new(@cask, verbose: verbose?, force: true, upgrade: upgrade?, reinstall: true) zap? ? cask_installer.zap : cask_installer.uninstall end @@ -234,7 +228,8 @@ on_request: true) next if artifact.is_a?(Artifact::Binary) && !binaries? - artifact.install_phase(command: @command, verbose: verbose?, adopt: adopt?, force: force?, upgrade: upgrade?) + artifact.install_phase(command: @command, verbose: verbose?, adopt: adopt?, force: force?, + upgrade_or_reinstall: upgrade? || reinstall?) already_installed_artifacts.unshift(artifact) end @@ -461,7 +456,11 @@ on_request: true) if artifact.respond_to?(:uninstall_phase) odebug "Uninstalling artifact of class #{artifact.class}" artifact.uninstall_phase( - command: @command, verbose: verbose?, skip: clear, force: force?, upgrade: upgrade?, + command: @command, + verbose: verbose?, + skip: clear, + force: force?, + upgrade_or_reinstall: upgrade? || reinstall?, ) end @@ -469,7 +468,11 @@ on_request: true) odebug "Post-uninstalling artifact of class #{artifact.class}" artifact.post_uninstall_phase( - command: @command, verbose: verbose?, skip: clear, force: force?, upgrade: upgrade?, + command: @command, + verbose: verbose?, + skip: clear, + force: force?, + upgrade_or_reinstall: upgrade? || reinstall?, ) end end diff --git a/Library/Homebrew/test/cask/artifact/app_spec.rb b/Library/Homebrew/test/cask/artifact/app_spec.rb index 47bcd14a59..a172bd7bc6 100644 --- a/Library/Homebrew/test/cask/artifact/app_spec.rb +++ b/Library/Homebrew/test/cask/artifact/app_spec.rb @@ -323,13 +323,13 @@ describe Cask::Artifact::App, :cask do inode = target_path.stat.ino expect(contents_path).to exist - app.uninstall_phase(command: command, force: force, upgrade: true) + app.uninstall_phase(command: command, force: force, upgrade_or_reinstall: true) expect(target_path).to exist expect(target_path.children).to be_empty expect(contents_path).not_to exist - app.install_phase(command: command, adopt: adopt, force: force, upgrade: true) + app.install_phase(command: command, adopt: adopt, force: force, upgrade_or_reinstall: true) expect(target_path).to exist expect(target_path.stat.ino).to eq(inode) From 9fedaee4622f42431874d39f53df6763352bb5b1 Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Sat, 8 Apr 2023 16:13:12 -0500 Subject: [PATCH 05/11] Only keep empty directories when the new version also has them. --- .../cask/artifact/abstract_uninstall.rb | 4 +- Library/Homebrew/cask/artifact/moved.rb | 12 ++-- Library/Homebrew/cask/installer.rb | 45 ++++++------ Library/Homebrew/cask/upgrade.rb | 8 +-- .../Homebrew/test/cask/artifact/app_spec.rb | 4 +- Library/Homebrew/test/cask/upgrade_spec.rb | 68 ++++++++++++++++++ .../cask/Casks/outdated/renamed-app.rb | 9 +++ .../fixtures/cask/Casks/renamed-app.rb | 9 +++ .../cask/NewApp.app/Contents/Info.plist | 0 .../cask/NewApp.app/Contents/MacOS/NewApp | 0 .../fixtures/cask/NewApp.app/Contents/PkgInfo | 0 .../Contents/Resources/Caffeine.icns | 0 .../test/support/fixtures/cask/new-app.tar.gz | Bin 0 -> 295 bytes .../test/support/fixtures/cask/old-app.tar.gz | Bin 0 -> 294 bytes 14 files changed, 125 insertions(+), 34 deletions(-) create mode 100644 Library/Homebrew/test/support/fixtures/cask/Casks/outdated/renamed-app.rb create mode 100644 Library/Homebrew/test/support/fixtures/cask/Casks/renamed-app.rb create mode 100644 Library/Homebrew/test/support/fixtures/cask/NewApp.app/Contents/Info.plist create mode 100644 Library/Homebrew/test/support/fixtures/cask/NewApp.app/Contents/MacOS/NewApp create mode 100644 Library/Homebrew/test/support/fixtures/cask/NewApp.app/Contents/PkgInfo create mode 100644 Library/Homebrew/test/support/fixtures/cask/NewApp.app/Contents/Resources/Caffeine.icns create mode 100644 Library/Homebrew/test/support/fixtures/cask/new-app.tar.gz create mode 100644 Library/Homebrew/test/support/fixtures/cask/old-app.tar.gz diff --git a/Library/Homebrew/cask/artifact/abstract_uninstall.rb b/Library/Homebrew/cask/artifact/abstract_uninstall.rb index 2f7f17595f..7fff5cfe31 100644 --- a/Library/Homebrew/cask/artifact/abstract_uninstall.rb +++ b/Library/Homebrew/cask/artifact/abstract_uninstall.rb @@ -270,8 +270,8 @@ module Cask end end - def uninstall_login_item(*login_items, command: nil, upgrade_or_reinstall: false, **_) - return if upgrade_or_reinstall + def uninstall_login_item(*login_items, command: nil, successor: nil, **_) + return unless successor.nil? apps = cask.artifacts.select { |a| a.class.dsl_key == :app } derived_login_items = apps.map { |a| { path: a.target } } diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index f3fa07be6e..e97a65b6f4 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -35,14 +35,16 @@ module Cask private - def move(adopt: false, force: false, verbose: false, upgrade_or_reinstall: false, reinstall: false, + def move(adopt: false, force: false, verbose: false, predecessor: nil, reinstall: false, command: nil, **options) unless source.exist? raise CaskError, "It seems the #{self.class.english_name} source '#{source}' is not there." end if Utils.path_occupied?(target) - if upgrade_or_reinstall && target.directory? && target.children.empty? + if !predecessor.nil? && target.directory? && target.children.empty? && predecessor.artifacts.any? do |a| + a.class.dsl_key == self.class.dsl_key && a.target == target + end # An upgrade removed the directory contents but left the directory itself (see below). unless source.directory? if target.parent.writable? && !force @@ -149,13 +151,15 @@ module Cask delete(target, force: force, command: command, **options) end - def delete(target, force: false, upgrade_or_reinstall: false, command: nil, **_) + def delete(target, force: false, successor: nil, command: nil, **_) ohai "Removing #{self.class.english_name} '#{target}'" raise CaskError, "Cannot remove undeletable #{self.class.english_name}." if MacOS.undeletable?(target) return unless Utils.path_occupied?(target) - if upgrade_or_reinstall && target.directory? + if !successor.nil? && target.directory? && successor.artifacts.any? do |a| + a.class.dsl_key == self.class.dsl_key && a.target == self.target + end # If an app folder is deleted, macOS considers the app uninstalled and removes some data. # Remove only the contents to handle this case. target.children.each do |child| diff --git a/Library/Homebrew/cask/installer.rb b/Library/Homebrew/cask/installer.rb index 96ddcce544..d289e30795 100644 --- a/Library/Homebrew/cask/installer.rb +++ b/Library/Homebrew/cask/installer.rb @@ -95,6 +95,7 @@ module Cask raise CaskAlreadyInstalledError, @cask end + predecessor = (reinstall? && @cask.installed?) ? @cask : nil check_conflicts @@ -110,7 +111,7 @@ module Cask @cask.config = @cask.default_config.merge(old_config) - install_artifacts + install_artifacts(predecessor: predecessor) if (tap = @cask.tap) && tap.should_report_analytics? ::Utils::Analytics.report_event(:cask_install, package_name: @cask.token, tap_name: tap.name, @@ -148,7 +149,7 @@ on_request: true) # Always force uninstallation, ignore method parameter cask_installer = Installer.new(@cask, verbose: verbose?, force: true, upgrade: upgrade?, reinstall: true) - zap? ? cask_installer.zap : cask_installer.uninstall + zap? ? cask_installer.zap(successor: @cask) : cask_installer.uninstall(successor: @cask) end sig { returns(String) } @@ -215,7 +216,7 @@ on_request: true) Quarantine.propagate(from: primary_container.path, to: to) end - def install_artifacts + def install_artifacts(predecessor: nil) artifacts = @cask.artifacts already_installed_artifacts = [] @@ -229,7 +230,7 @@ on_request: true) next if artifact.is_a?(Artifact::Binary) && !binaries? artifact.install_phase(command: @command, verbose: verbose?, adopt: adopt?, force: force?, - upgrade_or_reinstall: upgrade? || reinstall?) + predecessor: predecessor) already_installed_artifacts.unshift(artifact) end @@ -391,10 +392,10 @@ on_request: true) @cask.download_sha_path.atomic_write(@cask.new_download_sha) if @cask.checksumable? end - def uninstall + def uninstall(successor: nil) load_installed_caskfile! oh1 "Uninstalling Cask #{Formatter.identifier(@cask)}" - uninstall_artifacts(clear: true) + uninstall_artifacts(clear: true, successor: successor) if !reinstall? && !upgrade? remove_download_sha remove_config_file @@ -412,8 +413,8 @@ on_request: true) FileUtils.rm_f @cask.download_sha_path if @cask.download_sha_path.exist? end - def start_upgrade - uninstall_artifacts + def start_upgrade(successor:) + uninstall_artifacts(successor: successor) backup end @@ -432,10 +433,10 @@ on_request: true) backup_metadata_path.rename @cask.metadata_versioned_path end - def revert_upgrade + def revert_upgrade(predecessor) opoo "Reverting upgrade for Cask #{@cask}" restore_backup - install_artifacts + install_artifacts(predecessor: predecessor) end def finalize_upgrade @@ -446,7 +447,7 @@ on_request: true) puts summary end - def uninstall_artifacts(clear: false) + def uninstall_artifacts(clear: false, successor: nil) artifacts = @cask.artifacts odebug "Uninstalling artifacts" @@ -456,11 +457,11 @@ on_request: true) if artifact.respond_to?(:uninstall_phase) odebug "Uninstalling artifact of class #{artifact.class}" artifact.uninstall_phase( - command: @command, - verbose: verbose?, - skip: clear, - force: force?, - upgrade_or_reinstall: upgrade? || reinstall?, + command: @command, + verbose: verbose?, + skip: clear, + force: force?, + successor: successor, ) end @@ -468,16 +469,16 @@ on_request: true) odebug "Post-uninstalling artifact of class #{artifact.class}" artifact.post_uninstall_phase( - command: @command, - verbose: verbose?, - skip: clear, - force: force?, - upgrade_or_reinstall: upgrade? || reinstall?, + command: @command, + verbose: verbose?, + skip: clear, + force: force?, + successor: successor, ) end end - def zap + def zap(successor: nil) load_installed_caskfile! ohai "Implied `brew uninstall --cask #{@cask}`" uninstall_artifacts diff --git a/Library/Homebrew/cask/upgrade.rb b/Library/Homebrew/cask/upgrade.rb index 17c38f43be..eab3a2342f 100644 --- a/Library/Homebrew/cask/upgrade.rb +++ b/Library/Homebrew/cask/upgrade.rb @@ -180,22 +180,22 @@ module Cask new_cask_installer.fetch # Move the old cask's artifacts back to staging - old_cask_installer.start_upgrade + old_cask_installer.start_upgrade(successor: new_cask) # And flag it so in case of error started_upgrade = true # Install the new cask new_cask_installer.stage - new_cask_installer.install_artifacts + new_cask_installer.install_artifacts(predecessor: old_cask) new_artifacts_installed = true # If successful, wipe the old cask from staging old_cask_installer.finalize_upgrade rescue => e - new_cask_installer.uninstall_artifacts if new_artifacts_installed + new_cask_installer.uninstall_artifacts(successor: old_cask) if new_artifacts_installed new_cask_installer.purge_versioned_files - old_cask_installer.revert_upgrade if started_upgrade + old_cask_installer.revert_upgrade(predecessor: new_cask) if started_upgrade raise e end diff --git a/Library/Homebrew/test/cask/artifact/app_spec.rb b/Library/Homebrew/test/cask/artifact/app_spec.rb index a172bd7bc6..aa401e345e 100644 --- a/Library/Homebrew/test/cask/artifact/app_spec.rb +++ b/Library/Homebrew/test/cask/artifact/app_spec.rb @@ -323,13 +323,13 @@ describe Cask::Artifact::App, :cask do inode = target_path.stat.ino expect(contents_path).to exist - app.uninstall_phase(command: command, force: force, upgrade_or_reinstall: true) + app.uninstall_phase(command: command, force: force, successor: cask) expect(target_path).to exist expect(target_path.children).to be_empty expect(contents_path).not_to exist - app.install_phase(command: command, adopt: adopt, force: force, upgrade_or_reinstall: true) + app.install_phase(command: command, adopt: adopt, force: force, predecessor: cask) expect(target_path).to exist expect(target_path.stat.ino).to eq(inode) diff --git a/Library/Homebrew/test/cask/upgrade_spec.rb b/Library/Homebrew/test/cask/upgrade_spec.rb index 5647f3eceb..d10b53be54 100644 --- a/Library/Homebrew/test/cask/upgrade_spec.rb +++ b/Library/Homebrew/test/cask/upgrade_spec.rb @@ -13,6 +13,9 @@ describe Cask::Upgrade, :cask do let(:local_transmission) { Cask::CaskLoader.load("local-transmission") } let(:local_caffeine_path) { local_caffeine.config.appdir.join("Caffeine.app") } let(:local_caffeine) { Cask::CaskLoader.load("local-caffeine") } + let(:renamed_app) { Cask::CaskLoader.load("renamed-app") } + let(:renamed_app_old_path) { renamed_app.config.appdir.join("OldApp.app") } + let(:renamed_app_new_path) { renamed_app.config.appdir.join("NewApp.app") } let(:args) { Homebrew::CLI::Args.new } context "when the upgrade is successful" do @@ -22,6 +25,7 @@ describe Cask::Upgrade, :cask do "outdated/local-transmission", "outdated/auto-updates", "outdated/version-latest", + "outdated/renamed-app", ] end @@ -41,6 +45,11 @@ describe Cask::Upgrade, :cask do expect(local_transmission_path).to be_a_directory expect(local_transmission.versions).to include("2.60") + expect(renamed_app).to be_installed + expect(renamed_app_old_path).to be_a_directory + expect(renamed_app_new_path).not_to be_a_directory + expect(renamed_app.versions).to include("1.0.0") + described_class.upgrade_casks(args: args) expect(local_caffeine).to be_installed @@ -50,6 +59,11 @@ describe Cask::Upgrade, :cask do expect(local_transmission).to be_installed expect(local_transmission_path).to be_a_directory expect(local_transmission.versions).to include("2.61") + + expect(renamed_app).to be_installed + expect(renamed_app_old_path).not_to be_a_directory + expect(renamed_app_new_path).to be_a_directory + expect(renamed_app.versions).to include("2.0.0") end it "updates only the Casks specified in the command line" do @@ -61,6 +75,11 @@ describe Cask::Upgrade, :cask do expect(local_transmission_path).to be_a_directory expect(local_transmission.versions).to include("2.60") + expect(renamed_app).to be_installed + expect(renamed_app_old_path).to be_a_directory + expect(renamed_app_new_path).not_to be_a_directory + expect(renamed_app.versions).to include("1.0.0") + described_class.upgrade_casks(local_caffeine, args: args) expect(local_caffeine).to be_installed @@ -70,6 +89,11 @@ describe Cask::Upgrade, :cask do expect(local_transmission).to be_installed expect(local_transmission_path).to be_a_directory expect(local_transmission.versions).to include("2.60") + + expect(renamed_app).to be_installed + expect(renamed_app_old_path).to be_a_directory + expect(renamed_app_new_path).not_to be_a_directory + expect(renamed_app.versions).to include("1.0.0") end it 'updates "auto_updates" and "latest" Casks when their tokens are provided in the command line' do @@ -107,6 +131,11 @@ describe Cask::Upgrade, :cask do expect(local_transmission_path).to be_a_directory expect(local_transmission.versions).to include("2.60") + expect(renamed_app).to be_installed + expect(renamed_app_old_path).to be_a_directory + expect(renamed_app_new_path).not_to be_a_directory + expect(renamed_app.versions).to include("1.0.0") + expect(version_latest).to be_installed expect(version_latest_path_1).to be_a_directory expect(version_latest.versions).to include("latest") @@ -128,6 +157,11 @@ describe Cask::Upgrade, :cask do expect(local_transmission_path).to be_a_directory expect(local_transmission.versions).to include("2.61") + expect(renamed_app).to be_installed + expect(renamed_app_old_path).not_to be_a_directory + expect(renamed_app_new_path).to be_a_directory + expect(renamed_app.versions).to include("2.0.0") + expect(version_latest).to be_installed expect(version_latest_path_2).to be_a_directory expect(version_latest.versions).to include("latest") @@ -187,6 +221,7 @@ describe Cask::Upgrade, :cask do "outdated/local-transmission", "outdated/auto-updates", "outdated/version-latest", + "outdated/renamed-app", ] end @@ -208,6 +243,11 @@ describe Cask::Upgrade, :cask do expect(local_transmission_path).to be_a_directory expect(local_transmission.versions).to include("2.60") + expect(renamed_app).to be_installed + expect(renamed_app_old_path).to be_a_directory + expect(renamed_app_new_path).not_to be_a_directory + expect(renamed_app.versions).to include("1.0.0") + described_class.upgrade_casks(dry_run: true, args: args) expect(local_caffeine).to be_installed @@ -219,6 +259,12 @@ describe Cask::Upgrade, :cask do expect(local_transmission_path).to be_a_directory expect(local_transmission.versions).to include("2.60") expect(local_transmission.versions).not_to include("2.61") + + expect(renamed_app).to be_installed + expect(renamed_app_old_path).to be_a_directory + expect(renamed_app_new_path).not_to be_a_directory + expect(renamed_app.versions).to include("1.0.0") + expect(renamed_app.versions).not_to include("2.0.0") end it "would update only the Casks specified in the command line" do @@ -256,6 +302,11 @@ describe Cask::Upgrade, :cask do expect(auto_updates_path).to be_a_directory expect(auto_updates.versions).to include("2.57") + expect(renamed_app).to be_installed + expect(renamed_app_old_path).to be_a_directory + expect(renamed_app_new_path).not_to be_a_directory + expect(renamed_app.versions).to include("1.0.0") + described_class.upgrade_casks(local_caffeine, auto_updates, dry_run: true, args: args) expect(local_caffeine).to be_installed @@ -267,6 +318,12 @@ describe Cask::Upgrade, :cask do expect(auto_updates_path).to be_a_directory expect(auto_updates.versions).to include("2.57") expect(auto_updates.versions).not_to include("2.61") + + expect(renamed_app).to be_installed + expect(renamed_app_old_path).to be_a_directory + expect(renamed_app_new_path).not_to be_a_directory + expect(renamed_app.versions).to include("1.0.0") + expect(renamed_app.versions).not_to include("2.0.0") end end @@ -286,6 +343,11 @@ describe Cask::Upgrade, :cask do expect(local_transmission_path).to be_a_directory expect(local_transmission.versions).to include("2.60") + expect(renamed_app).to be_installed + expect(renamed_app_old_path).to be_a_directory + expect(renamed_app_new_path).not_to be_a_directory + expect(renamed_app.versions).to include("1.0.0") + expect(version_latest).to be_installed # Change download sha so that :latest cask decides to update itself version_latest.download_sha_path.write("fake download sha") @@ -308,6 +370,12 @@ describe Cask::Upgrade, :cask do expect(local_transmission.versions).to include("2.60") expect(local_transmission.versions).not_to include("2.61") + expect(renamed_app).to be_installed + expect(renamed_app_old_path).to be_a_directory + expect(renamed_app_new_path).not_to be_a_directory + expect(renamed_app.versions).to include("1.0.0") + expect(renamed_app.versions).not_to include("2.0.0") + expect(version_latest).to be_installed expect(version_latest.outdated_download_sha?).to be(true) end diff --git a/Library/Homebrew/test/support/fixtures/cask/Casks/outdated/renamed-app.rb b/Library/Homebrew/test/support/fixtures/cask/Casks/outdated/renamed-app.rb new file mode 100644 index 0000000000..e93b90cb8e --- /dev/null +++ b/Library/Homebrew/test/support/fixtures/cask/Casks/outdated/renamed-app.rb @@ -0,0 +1,9 @@ +cask "renamed-app" do + version "1.0.0" + sha256 "cf001ed6c81820e049dc7a353957dab8936b91f1956ee74ff0b3eb59791f1ad9" + + url "file://#{TEST_FIXTURE_DIR}/cask/old-app.tar.gz" + homepage "https://brew.sh/" + + app "OldApp.app" +end diff --git a/Library/Homebrew/test/support/fixtures/cask/Casks/renamed-app.rb b/Library/Homebrew/test/support/fixtures/cask/Casks/renamed-app.rb new file mode 100644 index 0000000000..3e1f367fa5 --- /dev/null +++ b/Library/Homebrew/test/support/fixtures/cask/Casks/renamed-app.rb @@ -0,0 +1,9 @@ +cask "renamed-app" do + version "2.0.0" + sha256 "9f88a6f3d8a7977cd3c116c56ee7a20a3c69e838a1d4946f815a926a57883299" + + url "file://#{TEST_FIXTURE_DIR}/cask/new-app.tar.gz" + homepage "https://brew.sh/" + + app "NewApp.app" +end diff --git a/Library/Homebrew/test/support/fixtures/cask/NewApp.app/Contents/Info.plist b/Library/Homebrew/test/support/fixtures/cask/NewApp.app/Contents/Info.plist new file mode 100644 index 0000000000..e69de29bb2 diff --git a/Library/Homebrew/test/support/fixtures/cask/NewApp.app/Contents/MacOS/NewApp b/Library/Homebrew/test/support/fixtures/cask/NewApp.app/Contents/MacOS/NewApp new file mode 100644 index 0000000000..e69de29bb2 diff --git a/Library/Homebrew/test/support/fixtures/cask/NewApp.app/Contents/PkgInfo b/Library/Homebrew/test/support/fixtures/cask/NewApp.app/Contents/PkgInfo new file mode 100644 index 0000000000..e69de29bb2 diff --git a/Library/Homebrew/test/support/fixtures/cask/NewApp.app/Contents/Resources/Caffeine.icns b/Library/Homebrew/test/support/fixtures/cask/NewApp.app/Contents/Resources/Caffeine.icns new file mode 100644 index 0000000000..e69de29bb2 diff --git a/Library/Homebrew/test/support/fixtures/cask/new-app.tar.gz b/Library/Homebrew/test/support/fixtures/cask/new-app.tar.gz new file mode 100644 index 0000000000000000000000000000000000000000..a07fc7c8fe5b76c35d8db589b42a8474c86b2455 GIT binary patch literal 295 zcmV+?0oeW@iwFRx!!cw41MQZMrrIF9j50X(E=R9Zm6Kq$pm zg!q zuBz`J6L!H>{t*EMf)XtFSI8Lo&%u=c2jjcrp*E=|LHzyKD1iLuV9NikZMxF!Ti>Ut z5&nllyvjeK#*=>t^Pil+uKmRx3%%-)bY-{WP tTqDZA4E{N2Ey#ZkF2DcAl@v9i{PW@crzJ@8PY?vLB%fSKmN)0onc^iwFRZ!!cw41MQZ7)0r-Nn3+*0u}loX_zU{|VK%|7~vvV+WtS5#s$zL?r(?nDf7@ zo9Z!P7hL5Zk)c4QKn4FwXdwSNnDhT&e1AMNCe Date: Mon, 24 Apr 2023 10:50:01 -0500 Subject: [PATCH 06/11] Update for feedback. --- .../Homebrew/cask/artifact/abstract_uninstall.rb | 2 +- Library/Homebrew/cask/artifact/moved.rb | 4 ++-- Library/Homebrew/cask/quarantine.rb | 15 +++++++-------- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/Library/Homebrew/cask/artifact/abstract_uninstall.rb b/Library/Homebrew/cask/artifact/abstract_uninstall.rb index 7fff5cfe31..f95a828771 100644 --- a/Library/Homebrew/cask/artifact/abstract_uninstall.rb +++ b/Library/Homebrew/cask/artifact/abstract_uninstall.rb @@ -271,7 +271,7 @@ module Cask end def uninstall_login_item(*login_items, command: nil, successor: nil, **_) - return unless successor.nil? + return if successor apps = cask.artifacts.select { |a| a.class.dsl_key == :app } derived_login_items = apps.map { |a| { path: a.target } } diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index e97a65b6f4..5976146726 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -43,7 +43,7 @@ module Cask if Utils.path_occupied?(target) if !predecessor.nil? && target.directory? && target.children.empty? && predecessor.artifacts.any? do |a| - a.class.dsl_key == self.class.dsl_key && a.target == target + a.instance_of?(self.class) && instance_of?(a.class) && a.target == target end # An upgrade removed the directory contents but left the directory itself (see below). unless source.directory? @@ -158,7 +158,7 @@ module Cask return unless Utils.path_occupied?(target) if !successor.nil? && target.directory? && successor.artifacts.any? do |a| - a.class.dsl_key == self.class.dsl_key && a.target == self.target + a.instance_of?(self.class) && instance_of?(a.class) && a.target == self.target end # If an app folder is deleted, macOS considers the app uninstalled and removes some data. # Remove only the contents to handle this case. diff --git a/Library/Homebrew/cask/quarantine.rb b/Library/Homebrew/cask/quarantine.rb index ed36a41d82..0e13a65dd1 100644 --- a/Library/Homebrew/cask/quarantine.rb +++ b/Library/Homebrew/cask/quarantine.rb @@ -179,14 +179,13 @@ module Cask def self.copy_xattrs(from, to) odebug "Copying xattrs from #{from} to #{to}" - copier = system_command!(swift, - args: [ - *swift_target_args, - COPY_XATTRS_SCRIPT, - from, - to, - ]) - copier.success? + system_command!(swift, + args: [ + *swift_target_args, + COPY_XATTRS_SCRIPT, + from, + to, + ]) end end end From 432e0c6b5bda70ad54610236d7e85f55f5f83bff Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Mon, 24 Apr 2023 10:59:01 -0500 Subject: [PATCH 07/11] Extract artifact checks to matching_artifact? function. --- Library/Homebrew/cask/artifact/moved.rb | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index 5976146726..cefa8a029f 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -42,9 +42,7 @@ module Cask end if Utils.path_occupied?(target) - if !predecessor.nil? && target.directory? && target.children.empty? && predecessor.artifacts.any? do |a| - a.instance_of?(self.class) && instance_of?(a.class) && a.target == target - end + if target.directory? && target.children.empty? && matching_artifact?(predecessor) # An upgrade removed the directory contents but left the directory itself (see below). unless source.directory? if target.parent.writable? && !force @@ -124,6 +122,14 @@ module Cask add_altname_metadata(target, source.basename, command: command) end + def matching_artifact?(cask) + return false unless cask + + cask.artifacts.any? do |a| + a.instance_of?(self.class) && instance_of?(a.class) && a.target == target + end + end + def move_back(skip: false, force: false, command: nil, **options) FileUtils.rm source if source.symlink? && source.dirname.join(source.readlink) == target @@ -157,9 +163,7 @@ module Cask return unless Utils.path_occupied?(target) - if !successor.nil? && target.directory? && successor.artifacts.any? do |a| - a.instance_of?(self.class) && instance_of?(a.class) && a.target == self.target - end + if target.directory? && matching_artifact?(successor) # If an app folder is deleted, macOS considers the app uninstalled and removes some data. # Remove only the contents to handle this case. target.children.each do |child| From 07fdbd3fd94bd82b4052a0468294ad9c4d73d575 Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Mon, 24 Apr 2023 17:08:13 -0500 Subject: [PATCH 08/11] Handle zap successor. --- Library/Homebrew/cask/installer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/cask/installer.rb b/Library/Homebrew/cask/installer.rb index 3a0723da4b..a5153c4258 100644 --- a/Library/Homebrew/cask/installer.rb +++ b/Library/Homebrew/cask/installer.rb @@ -481,7 +481,7 @@ on_request: true) def zap(successor: nil) load_installed_caskfile! ohai "Implied `brew uninstall --cask #{@cask}`" - uninstall_artifacts + uninstall_artifacts(successor: successor) if (zap_stanzas = @cask.artifacts.select { |a| a.is_a?(Artifact::Zap) }).empty? opoo "No zap stanza present for Cask '#{@cask}'" else From 84fe93e5d71770db1225023781afdd2da24217a5 Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Thu, 27 Apr 2023 10:40:45 -0500 Subject: [PATCH 09/11] Remove successor behavior for zapping. --- Library/Homebrew/cask/installer.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/cask/installer.rb b/Library/Homebrew/cask/installer.rb index ef2cb1ef10..e53849cf99 100644 --- a/Library/Homebrew/cask/installer.rb +++ b/Library/Homebrew/cask/installer.rb @@ -147,7 +147,7 @@ on_request: true) # Always force uninstallation, ignore method parameter cask_installer = Installer.new(@cask, verbose: verbose?, force: true, upgrade: upgrade?, reinstall: true) - zap? ? cask_installer.zap(successor: @cask) : cask_installer.uninstall(successor: @cask) + zap? ? cask_installer.zap : cask_installer.uninstall(successor: @cask) end sig { returns(String) } @@ -476,10 +476,10 @@ on_request: true) end end - def zap(successor: nil) + def zap load_installed_caskfile! ohai "Implied `brew uninstall --cask #{@cask}`" - uninstall_artifacts(successor: successor) + uninstall_artifacts if (zap_stanzas = @cask.artifacts.select { |a| a.is_a?(Artifact::Zap) }).empty? opoo "No zap stanza present for Cask '#{@cask}'" else From 4935a8fbb29a23799bc8b6d54591c561172df0c2 Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Sun, 30 Apr 2023 12:29:40 -0500 Subject: [PATCH 10/11] Remove unreachable warning. --- Library/Homebrew/cask/artifact/moved.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index 41bf8a5944..48861a0f61 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -97,9 +97,7 @@ module Cask command.run!("/bin/cp", args: ["-pR", "#{source}/*", "#{source}/.*", "#{target}/"], sudo: true) end - unless Quarantine.copy_xattrs(source, target) - opoo "Unable to transfer extended attributes on the root directory" - end + Quarantine.copy_xattrs(source, target) source.rmtree elsif target.dirname.writable? FileUtils.move(source, target) From 3e249a94282d8d3d30e8e752f87887ca04574491 Mon Sep 17 00:00:00 2001 From: JBYoshi <12983479+JBYoshi@users.noreply.github.com> Date: Wed, 3 May 2023 11:29:01 -0500 Subject: [PATCH 11/11] Improve styling. --- Library/Homebrew/cask/installer.rb | 2 +- Library/Homebrew/cask/quarantine.rb | 16 +++--- Library/Homebrew/cask/utils/copy-xattrs.swift | 52 +++++++++---------- 3 files changed, 36 insertions(+), 34 deletions(-) diff --git a/Library/Homebrew/cask/installer.rb b/Library/Homebrew/cask/installer.rb index e53849cf99..266125a394 100644 --- a/Library/Homebrew/cask/installer.rb +++ b/Library/Homebrew/cask/installer.rb @@ -93,7 +93,7 @@ module Cask raise CaskAlreadyInstalledError, @cask end - predecessor = (reinstall? && @cask.installed?) ? @cask : nil + predecessor = @cask if reinstall? && @cask.installed? check_conflicts diff --git a/Library/Homebrew/cask/quarantine.rb b/Library/Homebrew/cask/quarantine.rb index 3328bf7da4..66e5f6bfe8 100644 --- a/Library/Homebrew/cask/quarantine.rb +++ b/Library/Homebrew/cask/quarantine.rb @@ -177,13 +177,15 @@ module Cask def self.copy_xattrs(from, to) odebug "Copying xattrs from #{from} to #{to}" - system_command!(swift, - args: [ - *swift_target_args, - COPY_XATTRS_SCRIPT, - from, - to, - ]) + system_command!( + swift, + args: [ + *swift_target_args, + COPY_XATTRS_SCRIPT, + from, + to, + ], + ) end end end diff --git a/Library/Homebrew/cask/utils/copy-xattrs.swift b/Library/Homebrew/cask/utils/copy-xattrs.swift index f118290c81..794242ed13 100755 --- a/Library/Homebrew/cask/utils/copy-xattrs.swift +++ b/Library/Homebrew/cask/utils/copy-xattrs.swift @@ -15,66 +15,66 @@ guard CommandLine.arguments.count >= 3 else { exit(2) } -CommandLine.arguments[2].withCString { destPath in - let destNamesLen = listxattr(destPath, nil, 0, 0) - if destNamesLen == -1 { +CommandLine.arguments[2].withCString { destinationPath in + let destinationNamesLength = listxattr(destinationPath, nil, 0, 0) + if destinationNamesLength == -1 { print("listxattr for destination failed: \(errno)", to: &SwiftErr.stream) exit(1) } - let destNamesBuf = UnsafeMutablePointer.allocate(capacity: destNamesLen) - if listxattr(destPath, destNamesBuf, destNamesLen, 0) != destNamesLen { + let destinationNamesBuffer = UnsafeMutablePointer.allocate(capacity: destinationNamesLength) + if listxattr(destinationPath, destinationNamesBuffer, destinationNamesLength, 0) != destinationNamesLength { print("Attributes changed during system call", to: &SwiftErr.stream) exit(1) } - var destNamesIdx = 0 - while destNamesIdx < destNamesLen { - let attribute = destNamesBuf + destNamesIdx + var destinationNamesIndex = 0 + while destinationNamesIndex < destinationNamesLength { + let attribute = destinationNamesBuffer + destinationNamesIndex - if removexattr(destPath, attribute, 0) != 0 { + if removexattr(destinationPath, attribute, 0) != 0 { print("removexattr for \(String(cString: attribute)) failed: \(errno)", to: &SwiftErr.stream) exit(1) } - destNamesIdx += strlen(attribute) + 1 + destinationNamesIndex += strlen(attribute) + 1 } - destNamesBuf.deallocate() + destinationNamesBuffer.deallocate() CommandLine.arguments[1].withCString { sourcePath in - let sourceNamesLen = listxattr(sourcePath, nil, 0, 0) - if sourceNamesLen == -1 { + let sourceNamesLength = listxattr(sourcePath, nil, 0, 0) + if sourceNamesLength == -1 { print("listxattr for source failed: \(errno)", to: &SwiftErr.stream) exit(1) } - let sourceNamesBuf = UnsafeMutablePointer.allocate(capacity: sourceNamesLen) - if listxattr(sourcePath, sourceNamesBuf, sourceNamesLen, 0) != sourceNamesLen { + let sourceNamesBuffer = UnsafeMutablePointer.allocate(capacity: sourceNamesLength) + if listxattr(sourcePath, sourceNamesBuffer, sourceNamesLength, 0) != sourceNamesLength { print("Attributes changed during system call", to: &SwiftErr.stream) exit(1) } - var sourceNamesIdx = 0 - while sourceNamesIdx < sourceNamesLen { - let attribute = sourceNamesBuf + sourceNamesIdx + var sourceNamesIndex = 0 + while sourceNamesIndex < sourceNamesLength { + let attribute = sourceNamesBuffer + sourceNamesIndex - let valueLen = getxattr(sourcePath, attribute, nil, 0, 0, 0) - if valueLen == -1 { + let valueLength = getxattr(sourcePath, attribute, nil, 0, 0, 0) + if valueLength == -1 { print("getxattr for \(String(cString: attribute)) failed: \(errno)", to: &SwiftErr.stream) exit(1) } - let valueBuf = UnsafeMutablePointer.allocate(capacity: valueLen) - if getxattr(sourcePath, attribute, valueBuf, valueLen, 0, 0) != valueLen { + let valueBuffer = UnsafeMutablePointer.allocate(capacity: valueLength) + if getxattr(sourcePath, attribute, valueBuffer, valueLength, 0, 0) != valueLength { print("Attributes changed during system call", to: &SwiftErr.stream) exit(1) } - if setxattr(destPath, attribute, valueBuf, valueLen, 0, 0) != 0 { + if setxattr(destinationPath, attribute, valueBuffer, valueLength, 0, 0) != 0 { print("setxattr for \(String(cString: attribute)) failed: \(errno)", to: &SwiftErr.stream) exit(1) } - valueBuf.deallocate() - sourceNamesIdx += strlen(attribute) + 1 + valueBuffer.deallocate() + sourceNamesIndex += strlen(attribute) + 1 } - sourceNamesBuf.deallocate() + sourceNamesBuffer.deallocate() } }