From 119e02ceb08293dbc5dd1546dbc16cfc6155d9b0 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Tue, 9 Jul 2024 13:16:07 -0400 Subject: [PATCH] Cleanup `tap_git_head` and `uninstall_flight_blocks?` Co-authored-by: Kevin --- .../cask/artifact/abstract_flight_block.rb | 6 --- Library/Homebrew/cask/cask.rb | 13 ++++-- Library/Homebrew/cask/installer.rb | 2 +- Library/Homebrew/cask/tab.rb | 2 +- Library/Homebrew/tab.rb | 14 ++---- Library/Homebrew/test/cask/cask_spec.rb | 45 ++++++++++++++++--- Library/Homebrew/test/tab_spec.rb | 19 -------- 7 files changed, 55 insertions(+), 46 deletions(-) diff --git a/Library/Homebrew/cask/artifact/abstract_flight_block.rb b/Library/Homebrew/cask/artifact/abstract_flight_block.rb index 1a4cedc8df..5c8c4daec7 100644 --- a/Library/Homebrew/cask/artifact/abstract_flight_block.rb +++ b/Library/Homebrew/cask/artifact/abstract_flight_block.rb @@ -34,12 +34,6 @@ module Cask directives.keys.map(&:to_s).join(", ") end - def uninstall? - directives.keys.any? do |key| - key.to_s.start_with?("uninstall_") - end - end - private def class_for_dsl_key(dsl_key) diff --git a/Library/Homebrew/cask/cask.rb b/Library/Homebrew/cask/cask.rb index 00ff239de7..c2a53ade24 100644 --- a/Library/Homebrew/cask/cask.rb +++ b/Library/Homebrew/cask/cask.rb @@ -161,7 +161,12 @@ module Cask def uninstall_flight_blocks? artifacts.any? do |artifact| - artifact.is_a?(Artifact::AbstractFlightBlock) && artifact.uninstall? + case artifact + when Artifact::PreflightBlock + artifact.directives.key?(:uninstall_preflight) + when Artifact::PostflightBlock + artifact.directives.key?(:uninstall_postflight) + end end end @@ -480,11 +485,13 @@ module Cask artifacts.filter_map do |artifact| case artifact when Artifact::AbstractFlightBlock - next if uninstall_only && !artifact.uninstall? + uninstall_flight_block = artifact.directives.key?(:uninstall_preflight) || + artifact.directives.key?(:uninstall_postflight) + next if uninstall_only && !uninstall_flight_block # Only indicate whether this block is used as we don't load it from the API # We can skip this entirely once we move to internal JSON v3. - { artifact.summarize => nil } unless compact + { artifact.summarize.to_sym => nil } unless compact else zap_artifact = artifact.is_a?(Artifact::Zap) uninstall_artifact = artifact.respond_to?(:uninstall_phase) || artifact.respond_to?(:post_uninstall_phase) diff --git a/Library/Homebrew/cask/installer.rb b/Library/Homebrew/cask/installer.rb index b16ac8709c..f2b1b23b19 100644 --- a/Library/Homebrew/cask/installer.rb +++ b/Library/Homebrew/cask/installer.rb @@ -426,7 +426,7 @@ on_request: true) def remove_tabfile tabfile = @cask.tab.tabfile - FileUtils.rm_f tabfile if tabfile.present? && tabfile.exist? + FileUtils.rm_f tabfile if tabfile @cask.config_path.parent.rmdir_if_possible end diff --git a/Library/Homebrew/cask/tab.rb b/Library/Homebrew/cask/tab.rb index 68846ab3fa..23c48d65c7 100644 --- a/Library/Homebrew/cask/tab.rb +++ b/Library/Homebrew/cask/tab.rb @@ -32,7 +32,7 @@ module Cask tab.source = { "path" => cask.sourcefile_path.to_s, "tap" => cask.tap&.name, - "tap_git_head" => tap_git_head(cask), + "tap_git_head" => cask.tap_git_head, "version" => cask.version.to_s, } tab.uninstall_artifacts = cask.artifacts_list(uninstall_only: true) diff --git a/Library/Homebrew/tab.rb b/Library/Homebrew/tab.rb index 1d600f857e..267cd6bfc8 100644 --- a/Library/Homebrew/tab.rb +++ b/Library/Homebrew/tab.rb @@ -41,7 +41,7 @@ class AbstractTab "arch" => Hardware::CPU.arch, "source" => { "tap" => formula_or_cask.tap&.name, - "tap_git_head" => tap_git_head(formula_or_cask), + "tap_git_head" => formula_or_cask.tap_git_head, }, "built_on" => DevelopmentTools.build_system_info, } @@ -93,12 +93,6 @@ class AbstractTab new(attributes) end - def self.tap_git_head(formula_or_cask) - return unless formula_or_cask.tap&.installed? - - formula_or_cask.tap.git_head - end - def initialize(attributes = {}) attributes.each { |key, value| instance_variable_set(:"@#{key}", value) } end @@ -134,6 +128,7 @@ class Tab < AbstractTab attr_accessor :built_as_bottle, :changed_files, :stdlib, :aliases attr_writer :used_options, :unused_options, :compiler, :source_modified_time + attr_reader :tapped_from # Instantiates a {Tab} for a new installation of a formula. def self.create(formula, compiler, stdlib) @@ -169,8 +164,7 @@ class Tab < AbstractTab tab.source_modified_time ||= 0 tab.source ||= {} - tapped_from = tab.instance_variable_get(:@tapped_from) - tab.tap = tapped_from if !tapped_from.nil? && tapped_from != "path or URL" + tab.tap = tab.tapped_from if !tab.tapped_from.nil? && tab.tapped_from != "path or URL" tab.tap = "homebrew/core" if tab.tap == "mxcl/master" || tab.tap == "Homebrew/homebrew" if tab.source["spec"].nil? @@ -255,7 +249,7 @@ class Tab < AbstractTab tab.source = { "path" => formula.specified_path.to_s, "tap" => formula.tap&.name, - "tap_git_head" => tap_git_head(formula), + "tap_git_head" => formula.tap_git_head, "spec" => formula.active_spec_sym.to_s, "versions" => { "stable" => formula.stable&.version&.to_s, diff --git a/Library/Homebrew/test/cask/cask_spec.rb b/Library/Homebrew/test/cask/cask_spec.rb index 17e6b5c9be..038316a682 100644 --- a/Library/Homebrew/test/cask/cask_spec.rb +++ b/Library/Homebrew/test/cask/cask_spec.rb @@ -217,16 +217,16 @@ RSpec.describe Cask::Cask, :cask do it "returns all artifacts when no options are given" do expected_artifacts = [ - { "uninstall_preflight" => nil }, - { "preflight" => nil }, + { uninstall_preflight: nil }, + { preflight: nil }, { uninstall: [{ rmdir: "#{TEST_TMPDIR}/empty_directory_path", trash: ["#{TEST_TMPDIR}/foo", "#{TEST_TMPDIR}/bar"], }] }, { pkg: ["ManyArtifacts/ManyArtifacts.pkg"] }, { app: ["ManyArtifacts/ManyArtifacts.app"] }, - { "uninstall_postflight" => nil }, - { "postflight" => nil }, + { uninstall_postflight: nil }, + { postflight: nil }, { zap: [{ rmdir: ["~/Library/Caches/ManyArtifacts", "~/Library/Application Support/ManyArtifacts"], trash: "~/Library/Logs/ManyArtifacts.log", @@ -255,13 +255,13 @@ RSpec.describe Cask::Cask, :cask do it "returns only uninstall artifacts when uninstall_only is true" do expected_artifacts = [ - { "uninstall_preflight" => nil }, + { uninstall_preflight: nil }, { uninstall: [{ rmdir: "#{TEST_TMPDIR}/empty_directory_path", trash: ["#{TEST_TMPDIR}/foo", "#{TEST_TMPDIR}/bar"], }] }, { app: ["ManyArtifacts/ManyArtifacts.app"] }, - { "uninstall_postflight" => nil }, + { uninstall_postflight: nil }, { zap: [{ rmdir: ["~/Library/Caches/ManyArtifacts", "~/Library/Application Support/ManyArtifacts"], trash: "~/Library/Logs/ManyArtifacts.log", @@ -288,6 +288,39 @@ RSpec.describe Cask::Cask, :cask do end end + describe "#uninstall_flight_blocks?" do + matcher :have_uninstall_flight_blocks do + match do |actual| + actual.uninstall_flight_blocks? == true + end + end + + it "returns true when there are uninstall_preflight blocks" do + cask = Cask::CaskLoader.load("with-uninstall-preflight") + expect(cask).to have_uninstall_flight_blocks + end + + it "returns true when there are uninstall_postflight blocks" do + cask = Cask::CaskLoader.load("with-uninstall-postflight") + expect(cask).to have_uninstall_flight_blocks + end + + it "returns false when there are only preflight blocks" do + cask = Cask::CaskLoader.load("with-preflight") + expect(cask).not_to have_uninstall_flight_blocks + end + + it "returns false when there are only postflight blocks" do + cask = Cask::CaskLoader.load("with-postflight") + expect(cask).not_to have_uninstall_flight_blocks + end + + it "returns false when there are no flight blocks" do + cask = Cask::CaskLoader.load("local-caffeine") + expect(cask).not_to have_uninstall_flight_blocks + end + end + describe "#to_h" do let(:expected_json) { (TEST_FIXTURE_DIR/"cask/everything.json").read.strip } diff --git a/Library/Homebrew/test/tab_spec.rb b/Library/Homebrew/test/tab_spec.rb index 6eed8cf7d6..6c8414ae44 100644 --- a/Library/Homebrew/test/tab_spec.rb +++ b/Library/Homebrew/test/tab_spec.rb @@ -397,25 +397,6 @@ RSpec.describe Tab do end end - describe "::tap_git_head" do - it "returns nil if the tap is nil" do - formula = instance_double(Formula, tap: nil) - expect(described_class.tap_git_head(formula)).to be_nil - end - - it "returns nil if the tap is not installed" do - tap = instance_double(Tap, installed?: false) - formula = instance_double(Formula, tap:) - expect(described_class.tap_git_head(formula)).to be_nil - end - - it "returns the tap git head if the tap is installed" do - tap = instance_double(Tap, installed?: true, git_head: "0453e16c8e3fac73104da50927a86221ca0740c2") - formula = instance_double(Formula, tap:) - expect(described_class.tap_git_head(formula)).to eq("0453e16c8e3fac73104da50927a86221ca0740c2") - end - end - specify "#to_json" do json_tab = described_class.new(JSON.parse(tab.to_json)) expect(json_tab.homebrew_version).to eq(tab.homebrew_version)