From fd62cdf6360ee5e73b5d6cae3d113f7f4a8e44ee Mon Sep 17 00:00:00 2001 From: apainintheneck Date: Sat, 25 Feb 2023 02:08:39 -0800 Subject: [PATCH 1/3] cask: delay loading from source API For casks with certain stanzas, *flight and language stanzas in this case, we need to use the caskfile to install them correctly. The JSON API is not an option. This delays loading from the source API until just before we try to install one of these casks. This reduces the number of requests we make to the source API. --- Library/Homebrew/cask/cask.rb | 42 +++++++--- Library/Homebrew/cask/cask_loader.rb | 38 +++------- Library/Homebrew/cask/dsl.rb | 1 - Library/Homebrew/cask/installer.rb | 39 +++++++--- .../cask/cask_loader/from_api_loader_spec.rb | 76 ++++++++----------- 5 files changed, 103 insertions(+), 93 deletions(-) diff --git a/Library/Homebrew/cask/cask.rb b/Library/Homebrew/cask/cask.rb index 6810544d82..eb968f31d0 100644 --- a/Library/Homebrew/cask/cask.rb +++ b/Library/Homebrew/cask/cask.rb @@ -83,15 +83,14 @@ module Cask @tap end - def initialize(token, sourcefile_path: nil, source: nil, source_checksum: nil, tap: nil, - config: nil, allow_reassignment: false, loaded_from_api: false, loader: nil, &block) + def initialize(token, sourcefile_path: nil, source: nil, tap: nil, + config: nil, allow_reassignment: false, loader: nil, &block) @token = token @sourcefile_path = sourcefile_path @source = source - @ruby_source_checksum = source_checksum @tap = tap @allow_reassignment = allow_reassignment - @loaded_from_api = loaded_from_api + @loaded_from_api = false @loader = loader @block = block @@ -166,6 +165,12 @@ module Cask !versions.empty? end + # The caskfile is needed during installation when there are + # `*flight` blocks or the cask has multiple languages + def caskfile_only? + languages.any? || artifacts.any?(Artifact::AbstractFlightBlock) + end + sig { returns(T.nilable(Time)) } def install_time _, time = timestamped_versions.last @@ -258,6 +263,27 @@ module Cask end end + def ruby_source_checksum + @ruby_source_checksum ||= { + "sha256" => Digest::SHA256.file(sourcefile_path).hexdigest, + }.freeze + end + + def languages + @languages ||= @dsl.languages + end + + def tap_git_head + @tap_git_head ||= tap&.git_head + end + + def populate_from_api!(json_cask) + @loaded_from_api = true + @languages = json_cask[:languages] + @tap_git_head = json_cask[:tap_git_head] + @ruby_source_checksum = json_cask[:ruby_source_checksum].freeze + end + def to_s @token end @@ -306,7 +332,7 @@ module Cask "conflicts_with" => conflicts_with, "container" => container&.pairs, "auto_updates" => auto_updates, - "tap_git_head" => tap&.git_head, + "tap_git_head" => tap_git_head, "languages" => languages, "ruby_source_checksum" => ruby_source_checksum, } @@ -360,12 +386,6 @@ module Cask hash end - def ruby_source_checksum - @ruby_source_checksum ||= { - "sha256" => Digest::SHA256.file(sourcefile_path).hexdigest, - } - end - def artifacts_list artifacts.map do |artifact| case artifact diff --git a/Library/Homebrew/cask/cask_loader.rb b/Library/Homebrew/cask/cask_loader.rb index 0473882f5f..110f52dee5 100644 --- a/Library/Homebrew/cask/cask_loader.rb +++ b/Library/Homebrew/cask/cask_loader.rb @@ -45,10 +45,7 @@ module Cask private def cask(header_token, **options, &block) - checksum = { - "sha256" => Digest::SHA256.hexdigest(content), - } - Cask.new(header_token, source: content, source_checksum: checksum, **options, config: @config, &block) + Cask.new(header_token, source: content, **options, config: @config, &block) end end @@ -212,8 +209,6 @@ module Cask class FromAPILoader attr_reader :token, :path - FLIGHT_STANZAS = [:preflight, :postflight, :uninstall_preflight, :uninstall_postflight].freeze - def self.can_load?(ref) return false if Homebrew::EnvConfig.no_install_from_api? return false unless ref.is_a?(String) @@ -233,16 +228,7 @@ module Cask json_cask = @from_json || Homebrew::API::Cask.all_casks[token] cask_source = JSON.pretty_generate(json_cask) - json_cask = Homebrew::API.merge_variations(json_cask).deep_symbolize_keys - - # Use the cask-source API if there are any `*flight` blocks or the cask has multiple languages - if json_cask[:artifacts].any? { |artifact| FLIGHT_STANZAS.include?(artifact.keys.first) } || - json_cask[:languages].any? - cask_source = Homebrew::API::Cask.fetch_source(token, - git_head: json_cask[:tap_git_head], - sha256: json_cask.dig(:ruby_source_checksum, :sha256)) - return FromContentLoader.new(cask_source).load(config: config) - end + json_cask = Homebrew::API.merge_variations(json_cask).deep_symbolize_keys.freeze tap = Tap.fetch(json_cask[:tap]) if json_cask[:tap].to_s.include?("/") @@ -252,13 +238,7 @@ module Cask json_cask[:url_specs][:using] = using.to_sym end - Cask.new(token, - tap: tap, - source: cask_source, - source_checksum: json_cask[:ruby_source_checksum], - config: config, - loaded_from_api: true, - loader: self) do + api_cask = Cask.new(token, tap: tap, source: cask_source, config: config, loader: self) do version json_cask[:version] if json_cask[:sha256] == "no_check" @@ -318,15 +298,21 @@ module Cask # convert generic string replacements into actual ones artifact = cask.loader.from_h_hash_gsubs(artifact, appdir) key = artifact.keys.first - send(key, *artifact[key]) + if artifact[key].nil? + # for artifacts with blocks that can't be loaded from the API + send(key) {} # empty on purpose + else + send(key, *artifact[key]) + end end if json_cask[:caveats].present? # convert generic string replacements into actual ones - json_cask[:caveats] = cask.loader.from_h_string_gsubs(json_cask[:caveats], appdir) - caveats json_cask[:caveats] + caveats cask.loader.from_h_string_gsubs(json_cask[:caveats], appdir) end end + api_cask.populate_from_api!(json_cask) + api_cask end def from_h_string_gsubs(string, appdir) diff --git a/Library/Homebrew/cask/dsl.rb b/Library/Homebrew/cask/dsl.rb index 9617e849d0..f33472be57 100644 --- a/Library/Homebrew/cask/dsl.rb +++ b/Library/Homebrew/cask/dsl.rb @@ -77,7 +77,6 @@ module Cask :depends_on, :homepage, :language, - :languages, :name, :sha256, :staged_path, diff --git a/Library/Homebrew/cask/installer.rb b/Library/Homebrew/cask/installer.rb index ceb2c528e5..9d18ca01f0 100644 --- a/Library/Homebrew/cask/installer.rb +++ b/Library/Homebrew/cask/installer.rb @@ -64,6 +64,8 @@ module Cask def fetch(quiet: nil, timeout: nil) odebug "Cask::Installer#fetch" + load_cask_from_source_api! if @cask.loaded_from_api && @cask.caskfile_only? + verify_has_sha if require_sha? && !force? download(quiet: quiet, timeout: timeout) @@ -149,16 +151,8 @@ module Cask def uninstall_existing_cask return unless @cask.installed? - # use the same cask file that was used for installation, if possible - installed_caskfile = @cask.installed_caskfile - installed_cask = begin - installed_caskfile.exist? ? CaskLoader.load(installed_caskfile) : @cask - rescue CaskInvalidError # could be thrown by call to CaskLoader#load with outdated caskfile - @cask # default - end - # Always force uninstallation, ignore method parameter - cask_installer = Installer.new(installed_cask, verbose: verbose?, force: true, upgrade: upgrade?) + cask_installer = Installer.new(@cask, verbose: verbose?, force: true, upgrade: upgrade?) zap? ? cask_installer.zap : cask_installer.uninstall end @@ -403,6 +397,7 @@ module Cask end def uninstall + load_installed_caskfile! oh1 "Uninstalling Cask #{Formatter.identifier(@cask)}" uninstall_artifacts(clear: true) if !reinstall? && !upgrade? @@ -480,6 +475,7 @@ module Cask end def zap + load_installed_caskfile! ohai "Implied `brew uninstall --cask #{@cask}`" uninstall_artifacts if (zap_stanzas = @cask.artifacts.select { |a| a.is_a?(Artifact::Zap) }).empty? @@ -547,5 +543,30 @@ module Cask odebug "Purging all staged versions of Cask #{@cask}" gain_permissions_remove(@cask.caskroom_path) end + + private + + # load the same cask file that was used for installation, if possible + def load_installed_caskfile! + installed_caskfile = @cask.installed_caskfile + + if installed_caskfile.exist? + begin + @cask = CaskLoader.load(installed_caskfile) + return + rescue CaskInvalidError + # could be caused by trying to load outdated caskfile + end + end + + load_cask_from_source_api! if @cask.loaded_from_api && @cask.caskfile_only? + # otherwise we default to the current cask + end + + def load_cask_from_source_api! + options = { git_head: @cask.tap_git_head, sha256: @cask.ruby_source_checksum["sha256"] } + cask_source = Homebrew::API::Cask.fetch_source(@cask.token, **options) + @cask = CaskLoader::FromContentLoader.new(cask_source).load(config: @cask.config) + end end end diff --git a/Library/Homebrew/test/cask/cask_loader/from_api_loader_spec.rb b/Library/Homebrew/test/cask/cask_loader/from_api_loader_spec.rb index a10e101ae0..a6e9afddf0 100644 --- a/Library/Homebrew/test/cask/cask_loader/from_api_loader_spec.rb +++ b/Library/Homebrew/test/cask/cask_loader/from_api_loader_spec.rb @@ -59,80 +59,64 @@ describe Cask::CaskLoader::FromAPILoader, :cask do end describe "#load" do - shared_examples "loads from fetched source" do |cask_token| - include_context "with API setup", cask_token - let(:content_loader) { instance_double(Cask::CaskLoader::FromContentLoader) } - - it "fetches cask source from API" do - expect(Homebrew::API::Cask).to receive(:fetch_source).once - expect(Cask::CaskLoader::FromContentLoader) - .to receive(:new).once - .and_return(content_loader) - expect(content_loader).to receive(:load).once - - api_loader.load(config: nil) - end - end - - context "with a preflight stanza" do - include_examples "loads from fetched source", "with-preflight" - end - - context "with an uninstall-preflight stanza" do - include_examples "loads from fetched source", "with-uninstall-preflight" - end - - context "with a postflight stanza" do - include_examples "loads from fetched source", "with-postflight" - end - - context "with an uninstall-postflight stanza" do - include_examples "loads from fetched source", "with-uninstall-postflight" - end - - context "with a language stanza" do - include_examples "loads from fetched source", "with-languages" - end - - shared_examples "loads from API" do |cask_token| + shared_examples "loads from API" do |cask_token, caskfile_only| include_context "with API setup", cask_token let(:cask_from_api) { api_loader.load(config: nil) } it "loads from JSON API" do - expect(Homebrew::API::Cask).not_to receive(:fetch_source) - expect(Cask::CaskLoader::FromContentLoader).not_to receive(:new) - expect(cask_from_api).to be_a(Cask::Cask) expect(cask_from_api.token).to eq(cask_token) + expect(cask_from_api.loaded_from_api).to be(true) + expect(cask_from_api.caskfile_only?).to be(caskfile_only) end end context "with a binary stanza" do - include_examples "loads from API", "with-binary" + include_examples "loads from API", "with-binary", false end context "with cask dependencies" do - include_examples "loads from API", "with-depends-on-cask-multiple" + include_examples "loads from API", "with-depends-on-cask-multiple", false end context "with formula dependencies" do - include_examples "loads from API", "with-depends-on-formula-multiple" + include_examples "loads from API", "with-depends-on-formula-multiple", false end context "with macos dependencies" do - include_examples "loads from API", "with-depends-on-macos-array" + include_examples "loads from API", "with-depends-on-macos-array", false end context "with an installer stanza" do - include_examples "loads from API", "with-installer-script" + include_examples "loads from API", "with-installer-script", false end context "with uninstall stanzas" do - include_examples "loads from API", "with-uninstall-multi" + include_examples "loads from API", "with-uninstall-multi", false end context "with a zap stanza" do - include_examples "loads from API", "with-zap" + include_examples "loads from API", "with-zap", false + end + + context "with a preflight stanza" do + include_examples "loads from API", "with-preflight", true + end + + context "with an uninstall-preflight stanza" do + include_examples "loads from API", "with-uninstall-preflight", true + end + + context "with a postflight stanza" do + include_examples "loads from API", "with-postflight", true + end + + context "with an uninstall-postflight stanza" do + include_examples "loads from API", "with-uninstall-postflight", true + end + + context "with a language stanza" do + include_examples "loads from API", "with-languages", true end end end From 9af04fdf38f06a97089d9207541d02e9ff230d22 Mon Sep 17 00:00:00 2001 From: apainintheneck Date: Sun, 26 Feb 2023 11:26:44 -0800 Subject: [PATCH 2/3] Add installer tests --- Library/Homebrew/test/cask/installer_spec.rb | 41 ++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/Library/Homebrew/test/cask/installer_spec.rb b/Library/Homebrew/test/cask/installer_spec.rb index 27779e11ea..b120ef9ad4 100644 --- a/Library/Homebrew/test/cask/installer_spec.rb +++ b/Library/Homebrew/test/cask/installer_spec.rb @@ -235,6 +235,22 @@ describe Cask::Installer, :cask do expect(latest_cask.download_sha_path).to be_a_file end + + context "when loaded from the api and caskfile is required" do + let(:path) { cask_path("local-caffeine") } + let(:content) { File.read(path) } + + it "installs cask" do + expect(Homebrew::API::Cask).to receive(:fetch_source).once.and_return(content) + + caffeine = Cask::CaskLoader.load(path) + expect(caffeine).to receive(:loaded_from_api).once.and_return(true) + expect(caffeine).to receive(:caskfile_only?).once.and_return(true) + + described_class.new(caffeine).install + expect(Cask::CaskLoader.load(path)).to be_installed + end + end end describe "uninstall" do @@ -269,6 +285,31 @@ describe Cask::Installer, :cask do expect(Cask::Caskroom.path.join("local-caffeine", mutated_version)).not_to be_a_directory expect(Cask::Caskroom.path.join("local-caffeine")).not_to be_a_directory end + + context "when loaded from the api, caskfile is required and installed caskfile is invalid" do + let(:path) { cask_path("local-caffeine") } + let(:content) { File.read(path) } + let(:invalid_path) { instance_double(Pathname) } + + before do + allow(invalid_path).to receive(:exist?).and_return(false) + end + + it "uninstalls cask" do + expect(Homebrew::API::Cask).to receive(:fetch_source).twice.and_return(content) + + caffeine = Cask::CaskLoader.load(path) + expect(caffeine).to receive(:loaded_from_api).twice.and_return(true) + expect(caffeine).to receive(:caskfile_only?).twice.and_return(true) + expect(caffeine).to receive(:installed_caskfile).once.and_return(invalid_path) + + described_class.new(caffeine).install + expect(Cask::CaskLoader.load(path)).to be_installed + + described_class.new(caffeine).uninstall + expect(Cask::CaskLoader.load(path)).not_to be_installed + end + end end describe "uninstall_existing_cask" do From b91e93cda0e1900b55903a1573376458d7e8c22e Mon Sep 17 00:00:00 2001 From: apainintheneck Date: Mon, 27 Feb 2023 23:51:43 -0800 Subject: [PATCH 3/3] Load cask source with tap info This info is used in the installer to decide whether to report analytics. --- Library/Homebrew/cask/cask_loader.rb | 10 +--------- Library/Homebrew/cask/installer.rb | 2 +- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/Library/Homebrew/cask/cask_loader.rb b/Library/Homebrew/cask/cask_loader.rb index dd6b20a3d3..0828bd9c1f 100644 --- a/Library/Homebrew/cask/cask_loader.rb +++ b/Library/Homebrew/cask/cask_loader.rb @@ -46,7 +46,7 @@ module Cask private def cask(header_token, **options, &block) - Cask.new(header_token, source: content, **options, config: @config, &block) + Cask.new(header_token, source: content, tap: tap, **options, config: @config, &block) end end @@ -146,18 +146,10 @@ module Cask super && !Tap.from_path(ref).nil? end - attr_reader :tap - def initialize(path) @tap = Tap.from_path(path) super(path) end - - private - - def cask(*args, &block) - super(*args, tap: tap, &block) - end end # Loads a cask from a specific tap. diff --git a/Library/Homebrew/cask/installer.rb b/Library/Homebrew/cask/installer.rb index 9d18ca01f0..f9eedc625e 100644 --- a/Library/Homebrew/cask/installer.rb +++ b/Library/Homebrew/cask/installer.rb @@ -566,7 +566,7 @@ module Cask def load_cask_from_source_api! options = { git_head: @cask.tap_git_head, sha256: @cask.ruby_source_checksum["sha256"] } cask_source = Homebrew::API::Cask.fetch_source(@cask.token, **options) - @cask = CaskLoader::FromContentLoader.new(cask_source).load(config: @cask.config) + @cask = CaskLoader::FromContentLoader.new(cask_source, tap: @cask.tap).load(config: @cask.config) end end end