Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve cask --adopt to only care about the installed version if auto… #18420

Merged
merged 2 commits into from
Sep 26, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Library/Homebrew/cask/artifact/moved.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand Down Expand Up @@ -79,6 +79,8 @@ def move(adopt: false, force: false, verbose: false, predecessor: nil, reinstall
).success?
end

same = true if auto_updates
MikeMcQuaid marked this conversation as resolved.
Show resolved Hide resolved

unless same
raise CaskError,
"It seems the existing #{self.class.english_name} is different from " \
Expand Down
3 changes: 2 additions & 1 deletion Library/Homebrew/cask/installer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
64 changes: 48 additions & 16 deletions Library/Homebrew/test/cask/artifact/app_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
Loading