From 31c560e578004814744078abacded3621bdee3bf Mon Sep 17 00:00:00 2001 From: Case Taintor Date: Thu, 26 Sep 2024 11:14:30 +0200 Subject: [PATCH 1/2] Improve cask --adopt to only care about the installed version if auto_update is false --- Library/Homebrew/cask/artifact/moved.rb | 4 +- Library/Homebrew/cask/installer.rb | 3 +- .../Homebrew/test/cask/artifact/app_spec.rb | 64 ++++++++++++++----- 3 files changed, 53 insertions(+), 18 deletions(-) diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index b8be1ecd6907a..1a4e1629f8356 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -31,7 +31,7 @@ def summarize_installed private - def move(adopt: false, force: false, verbose: false, predecessor: nil, reinstall: false, + def move(adopt: false, auto_updates: 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." @@ -79,6 +79,8 @@ def move(adopt: false, force: false, verbose: false, predecessor: nil, reinstall ).success? end + same = true if auto_updates + unless same raise CaskError, "It seems the existing #{self.class.english_name} is different from " \ diff --git a/Library/Homebrew/cask/installer.rb b/Library/Homebrew/cask/installer.rb index 6877f13ee651c..729f01471132e 100644 --- a/Library/Homebrew/cask/installer.rb +++ b/Library/Homebrew/cask/installer.rb @@ -253,7 +253,8 @@ def install_artifacts(predecessor: nil) next if artifact.is_a?(Artifact::Binary) && !binaries? artifact.install_phase( - command: @command, verbose: verbose?, adopt: adopt?, force: force?, predecessor:, + command: @command, verbose: verbose?, adopt: adopt?, auto_updates: @cask.auto_updates, + force: force?, predecessor: ) already_installed_artifacts.unshift(artifact) end diff --git a/Library/Homebrew/test/cask/artifact/app_spec.rb b/Library/Homebrew/test/cask/artifact/app_spec.rb index 72f644cd45be1..b4f0e800a35d7 100644 --- a/Library/Homebrew/test/cask/artifact/app_spec.rb +++ b/Library/Homebrew/test/cask/artifact/app_spec.rb @@ -5,12 +5,13 @@ let(:command) { NeverSudoSystemCommand } let(:adopt) { false } let(:force) { false } + let(:auto_updates) { false } let(:app) { cask.artifacts.find { |a| a.is_a?(described_class) } } let(:source_path) { cask.staged_path.join("Caffeine.app") } let(:target_path) { cask.config.appdir.join("Caffeine.app") } - let(:install_phase) { app.install_phase(command:, adopt:, force:) } + let(:install_phase) { app.install_phase(command:, adopt:, force:, auto_updates:) } let(:uninstall_phase) { app.uninstall_phase(command:, force:) } before do @@ -83,24 +84,55 @@ let(:adopt) { true } describe "when the target compares different from the source" do - it "avoids clobbering the existing app" do - stdout = <<~EOS - ==> Adopting existing App at '#{target_path}' - EOS + describe "when the cask does not auto_updates" do + it "avoids clobbering the existing app if brew manages updates" do + stdout = <<~EOS + ==> Adopting existing App at '#{target_path}' + EOS + + expect { install_phase } + .to output(stdout).to_stdout + .and raise_error( + Cask::CaskError, + "It seems the existing App is different from the one being installed.", + ) + + expect(source_path).to be_a_directory + expect(target_path).to be_a_directory + expect(File.identical?(source_path, target_path)).to be false + + contents_path = target_path.join("Contents/Info.plist") + expect(contents_path).not_to exist + end + end - expect { install_phase } - .to output(stdout).to_stdout - .and raise_error( - Cask::CaskError, - "It seems the existing App is different from the one being installed.", - ) + describe "when the cask auto_updates" do + before do + target_path.delete + FileUtils.cp_r source_path, target_path + File.write(target_path.join("Contents/Info.plist"), "different") + end - expect(source_path).to be_a_directory - expect(target_path).to be_a_directory - expect(File.identical?(source_path, target_path)).to be false + let(:auto_updates) { true } - contents_path = target_path.join("Contents/Info.plist") - expect(contents_path).not_to exist + it "adopts the existing app" do + stdout = <<~EOS + ==> Adopting existing App at '#{target_path}' + EOS + + stderr = "" + + expect { install_phase } + .to output(stdout).to_stdout + .and output(stderr).to_stderr + + expect(source_path).to be_a_symlink + expect(target_path).to be_a_directory + + contents_path = target_path.join("Contents/Info.plist") + expect(contents_path).to exist + expect(File.read(contents_path)).to eq("different") + end end end From 20444638f33c329f3b7d139b3d0cd026c9062ab1 Mon Sep 17 00:00:00 2001 From: Case Taintor Date: Thu, 26 Sep 2024 13:41:22 +0200 Subject: [PATCH 2/2] modifies logic to only compare versions if auto_updates is false --- Library/Homebrew/cask/artifact/moved.rb | 56 ++++++++++++------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index 1a4e1629f8356..ee4a040051249 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -51,40 +51,40 @@ def move(adopt: false, auto_updates: false, force: false, verbose: false, predec if adopt ohai "Adopting existing #{self.class.english_name} at '#{target}'" - source_plist = Pathname("#{source}/Contents/Info.plist") - target_plist = Pathname("#{target}/Contents/Info.plist") - same = if source_plist.size? && - (source_bundle_version = Homebrew::BundleVersion.from_info_plist(source_plist)) && - target_plist.size? && - (target_bundle_version = Homebrew::BundleVersion.from_info_plist(target_plist)) - if source_bundle_version.short_version == target_bundle_version.short_version - if source_bundle_version.version == target_bundle_version.version - true + unless auto_updates + source_plist = Pathname("#{source}/Contents/Info.plist") + target_plist = Pathname("#{target}/Contents/Info.plist") + same = if source_plist.size? && + (source_bundle_version = Homebrew::BundleVersion.from_info_plist(source_plist)) && + target_plist.size? && + (target_bundle_version = Homebrew::BundleVersion.from_info_plist(target_plist)) + if source_bundle_version.short_version == target_bundle_version.short_version + if source_bundle_version.version == target_bundle_version.version + true + else + onoe "The bundle version of #{source} is #{source_bundle_version.version} but " \ + "is #{target_bundle_version.version} for #{target}!" + false + end else - onoe "The bundle version of #{source} is #{source_bundle_version.version} but " \ - "is #{target_bundle_version.version} for #{target}!" + onoe "The bundle short version of #{source} is #{source_bundle_version.short_version} but " \ + "is #{target_bundle_version.short_version} for #{target}!" false end else - onoe "The bundle short version of #{source} is #{source_bundle_version.short_version} but " \ - "is #{target_bundle_version.short_version} for #{target}!" - false + command.run( + "/usr/bin/diff", + args: ["--recursive", "--brief", source, target], + verbose:, + print_stdout: verbose, + ).success? end - else - command.run( - "/usr/bin/diff", - args: ["--recursive", "--brief", source, target], - verbose:, - print_stdout: verbose, - ).success? - end - same = true if auto_updates - - 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 end # Remove the source as we don't need to move it to the target location