From 3834ef1b738b6c0de29dfa485def8e8a84514713 Mon Sep 17 00:00:00 2001 From: apainintheneck Date: Tue, 5 Mar 2024 23:53:52 -0800 Subject: [PATCH 1/5] tap: cache more things at the Tap level I added two new methods to cache both installed and all taps. All taps includes core taps no matter if they're installed locally since they're always provided by the API anyway. This makes it easier to cache `Tap.each` while making the code easier to reason about. It also will be useful because we'll be able to avoid the `Tap.select(&:installed?` pattern that has recently invaded the codebase. Note: I also stopped clearing all tap instance caches before tests. Running `Tap.each` would cache existing taps which would lead to unexpected behavior since the only existing tap before each test is the core tap. This is the only tap whose directory is not cleaned up between tests so we just clear it's cache directly. We also now clear all tap instances after tests as well regardless of whether the API was used that time. --- Library/Homebrew/tap.rb | 45 +++++++++++++++++++--------- Library/Homebrew/test/spec_helper.rb | 6 ++-- 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/Library/Homebrew/tap.rb b/Library/Homebrew/tap.rb index 8e779c6ecf..2cf5efc378 100644 --- a/Library/Homebrew/tap.rb +++ b/Library/Homebrew/tap.rb @@ -402,6 +402,7 @@ class Tap end clear_cache + Tap.clear_cache $stderr.ohai "Tapping #{name}" unless quiet args = %W[clone #{requested_remote} #{path}] @@ -546,6 +547,7 @@ class Tap Commands.rebuild_commands_completion_list clear_cache + Tap.clear_cache return if !manual || !official? @@ -931,27 +933,42 @@ class Tap other = Tap.fetch(other) if other.is_a?(String) other.is_a?(self.class) && name == other.name end + alias eql? == - def self.each(&block) - return to_enum unless block + sig { returns(Integer) } + def hash + [self.class, name].hash + end - installed_taps = if TAP_DIRECTORY.directory? - TAP_DIRECTORY.subdirs - .flat_map(&:subdirs) - .map(&method(:from_path)) + # All locally installed taps. + sig { returns(T::Array[Tap]) } + def self.installed + cache[:installed] ||= if TAP_DIRECTORY.directory? + TAP_DIRECTORY.subdirs.flat_map(&:subdirs).map(&method(:from_path)) else [] end + end - available_taps = if Homebrew::EnvConfig.no_install_from_api? - installed_taps + # All locally installed and core taps. Core taps might not be installed locally when using the API. + sig { returns(T::Array[Tap]) } + def self.all + cache[:all] ||= begin + core_taps = [ + CoreTap.instance, + (CoreCaskTap.instance if OS.mac?), # rubocop:disable Homebrew/MoveToExtendOS + ].compact + + installed | core_taps + end + end + + def self.each(&block) + if Homebrew::EnvConfig.no_install_from_api? + installed.each(&block) else - default_taps = T.let([CoreTap.instance], T::Array[Tap]) - default_taps << CoreCaskTap.instance if OS.mac? # rubocop:disable Homebrew/MoveToExtendOS - installed_taps + default_taps - end.sort_by(&:name).uniq - - available_taps.each(&block) + all.each(&block) + end end # An array of all installed {Tap} names. diff --git a/Library/Homebrew/test/spec_helper.rb b/Library/Homebrew/test/spec_helper.rb index 48068f8f37..b8af2d24e9 100644 --- a/Library/Homebrew/test/spec_helper.rb +++ b/Library/Homebrew/test/spec_helper.rb @@ -204,7 +204,7 @@ RSpec.configure do |config| config.around do |example| Homebrew.raise_deprecation_exceptions = true - Tap.each(&:clear_cache) + Tap.installed.each(&:clear_cache) Cachable::Registry.clear_all_caches FormulaInstaller.clear_attempted FormulaInstaller.clear_installed @@ -244,9 +244,6 @@ RSpec.configure do |config| rescue SystemExit => e example.example.set_exception(e) ensure - # This depends on `HOMEBREW_NO_INSTALL_FROM_API`. - Tap.each(&:clear_cache) - ENV.replace(@__env) Context.current = Context::ContextStruct.new @@ -257,6 +254,7 @@ RSpec.configure do |config| @__stderr.close @__stdin.close + Tap.all.each(&:clear_cache) Cachable::Registry.clear_all_caches FileUtils.rm_rf [ From fb8c0d2b3086f8fe583723cf187a16d8e00a31c6 Mon Sep 17 00:00:00 2001 From: apainintheneck Date: Wed, 6 Mar 2024 20:58:34 -0800 Subject: [PATCH 2/5] s/Tap.select(&:installed?)/Tap.installed/ --- Library/Homebrew/cmd/readall.rb | 2 +- Library/Homebrew/cmd/tap.rb | 4 ++-- Library/Homebrew/cmd/update-report.rb | 4 ++-- Library/Homebrew/completions.rb | 6 +++--- Library/Homebrew/dev-cmd/audit.rb | 2 +- Library/Homebrew/diagnostic.rb | 4 ++-- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/Library/Homebrew/cmd/readall.rb b/Library/Homebrew/cmd/readall.rb index e20db93e36..530c41e3ee 100644 --- a/Library/Homebrew/cmd/readall.rb +++ b/Library/Homebrew/cmd/readall.rb @@ -57,7 +57,7 @@ module Homebrew raise UsageError, "`brew readall` needs a tap or `--eval-all` passed or `HOMEBREW_EVAL_ALL` set!" end - Tap.select(&:installed?) + Tap.installed else args.named.to_installed_taps end diff --git a/Library/Homebrew/cmd/tap.rb b/Library/Homebrew/cmd/tap.rb index f3f751ead3..1275b1a188 100644 --- a/Library/Homebrew/cmd/tap.rb +++ b/Library/Homebrew/cmd/tap.rb @@ -55,12 +55,12 @@ module Homebrew args = tap_args.parse if args.repair? - Tap.select(&:installed?).each do |tap| + Tap.installed.each do |tap| tap.link_completions_and_manpages tap.fix_remote_configuration end elsif args.no_named? - puts Tap.select(&:installed?) + puts Tap.installed.sort_by(&:name) else tap = Tap.fetch(args.named.first) begin diff --git a/Library/Homebrew/cmd/update-report.rb b/Library/Homebrew/cmd/update-report.rb index ad2bf864ab..0c455f3e4b 100644 --- a/Library/Homebrew/cmd/update-report.rb +++ b/Library/Homebrew/cmd/update-report.rb @@ -146,7 +146,7 @@ module Homebrew hub = ReporterHub.new updated_taps = [] - Tap.select(&:installed?).each do |tap| + Tap.installed.each do |tap| next if !tap.git? || tap.git_repo.origin_url.nil? next if (tap.core_tap? || tap.core_cask_tap?) && !Homebrew::EnvConfig.no_install_from_api? @@ -254,7 +254,7 @@ module Homebrew Commands.rebuild_commands_completion_list link_completions_manpages_and_docs - Tap.select(&:installed?).each(&:link_completions_and_manpages) + Tap.installed.each(&:link_completions_and_manpages) failed_fetch_dirs = ENV["HOMEBREW_MISSING_REMOTE_REF_DIRS"]&.split("\n") if failed_fetch_dirs.present? diff --git a/Library/Homebrew/completions.rb b/Library/Homebrew/completions.rb index 4e60b8209a..60d50f1555 100644 --- a/Library/Homebrew/completions.rb +++ b/Library/Homebrew/completions.rb @@ -72,7 +72,7 @@ module Homebrew sig { void } def self.link! Settings.write :linkcompletions, true - Tap.select(&:installed?).each do |tap| + Tap.installed.each do |tap| Utils::Link.link_completions tap.path, "brew completions link" end end @@ -80,7 +80,7 @@ module Homebrew sig { void } def self.unlink! Settings.write :linkcompletions, false - Tap.select(&:installed?).each do |tap| + Tap.installed.each do |tap| next if tap.official? Utils::Link.unlink_completions tap.path @@ -94,7 +94,7 @@ module Homebrew sig { returns(T::Boolean) } def self.completions_to_link? - Tap.select(&:installed?).each do |tap| + Tap.installed.each do |tap| next if tap.official? SHELLS.each do |shell| diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 321b26f1d7..9c065dd43d 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -189,7 +189,7 @@ module Homebrew # Run tap audits first named_arg_taps = [*audit_formulae, *audit_casks].map(&:tap).uniq if !args.tap && !no_named_args - tap_problems = Tap.select(&:installed?).each_with_object({}) do |tap, problems| + tap_problems = Tap.installed.each_with_object({}) do |tap, problems| next if args.tap && tap != args.tap next if named_arg_taps&.exclude?(tap) diff --git a/Library/Homebrew/diagnostic.rb b/Library/Homebrew/diagnostic.rb index aea24c3b95..652f517bdf 100644 --- a/Library/Homebrew/diagnostic.rb +++ b/Library/Homebrew/diagnostic.rb @@ -545,7 +545,7 @@ module Homebrew return if ENV["CI"] return unless Utils::Git.available? - commands = Tap.select(&:installed?).filter_map do |tap| + commands = Tap.installed.filter_map do |tap| next if tap.git_repo.default_origin_branch? "git -C $(brew --repo #{tap.name}) checkout #{tap.git_repo.origin_branch_name}" @@ -794,7 +794,7 @@ module Homebrew def check_for_tap_ruby_files_locations bad_tap_files = {} - Tap.select(&:installed?).each do |tap| + Tap.installed.each do |tap| unused_formula_dirs = tap.potential_formula_dirs - [tap.formula_dir] unused_formula_dirs.each do |dir| next unless dir.exist? From d4a273443c0d5bb7d84d4c96a5118aefefac6599 Mon Sep 17 00:00:00 2001 From: apainintheneck Date: Wed, 6 Mar 2024 21:00:43 -0800 Subject: [PATCH 3/5] tap: add #reverse_tap_migrations_renames to speed things up This gets used by `Tap.reverse_tap_migrations_renames` and reduces the amount of information that needs to be calculated on the fly every time. --- Library/Homebrew/cask/cask.rb | 2 +- Library/Homebrew/formula.rb | 2 +- Library/Homebrew/tap.rb | 37 +++++++++++++++++++++-------------- 3 files changed, 24 insertions(+), 17 deletions(-) diff --git a/Library/Homebrew/cask/cask.rb b/Library/Homebrew/cask/cask.rb index 0aac5ce4d9..e3d6d51041 100644 --- a/Library/Homebrew/cask/cask.rb +++ b/Library/Homebrew/cask/cask.rb @@ -87,7 +87,7 @@ module Cask sig { returns(T::Array[String]) } def old_tokens @old_tokens ||= if (tap = self.tap) - Tap.reverse_tap_migrations_renames.fetch("#{tap}/#{token}", []) + + Tap.reverse_tap_migrations_renames(tap, token) + tap.cask_reverse_renames.fetch(token, []) else [] diff --git a/Library/Homebrew/formula.rb b/Library/Homebrew/formula.rb index 46ffccd72f..d9394808b1 100644 --- a/Library/Homebrew/formula.rb +++ b/Library/Homebrew/formula.rb @@ -552,7 +552,7 @@ class Formula sig { returns(T::Array[String]) } def oldnames @oldnames ||= if (tap = self.tap) - Tap.reverse_tap_migrations_renames.fetch("#{tap}/#{name}", []) + + Tap.reverse_tap_migrations_renames(tap, name) + tap.formula_reverse_renames.fetch(name, []) else [] diff --git a/Library/Homebrew/tap.rb b/Library/Homebrew/tap.rb index 2cf5efc378..27a93283aa 100644 --- a/Library/Homebrew/tap.rb +++ b/Library/Homebrew/tap.rb @@ -189,6 +189,7 @@ class Tap @command_files = nil @tap_migrations = nil + @reverse_tap_migrations_renames = nil @audit_exceptions = nil @style_exceptions = nil @@ -853,21 +854,6 @@ class Tap end end - sig { returns(T::Hash[String, T::Array[String]]) } - def self.reverse_tap_migrations_renames - Tap.each_with_object({}) do |tap, hash| - tap.tap_migrations.each do |old_name, new_name| - new_tap_user, new_tap_repo, new_name = new_name.split("/", 3) - next unless new_name - - new_tap = Tap.fetch(T.must(new_tap_user), T.must(new_tap_repo)) - - hash["#{new_tap}/#{new_name}"] ||= [] - hash["#{new_tap}/#{new_name}"] << old_name - end - end - end - # Hash with tap migrations. sig { returns(T::Hash[String, String]) } def tap_migrations @@ -878,6 +864,27 @@ class Tap end end + sig { returns(T::Hash[String, T::Array[String]]) } + def reverse_tap_migrations_renames + @reverse_tap_migrations_renames ||= tap_migrations.each_with_object({}) do |(old_name, new_name), hash| + next if new_name.count("/") != 2 + + hash[new_name] ||= [] + hash[new_name] << old_name + end + end + + sig { params(new_tap: Tap, name_or_token: String).returns(T::Array[String]) } + def self.reverse_tap_migrations_renames(new_tap, name_or_token) + key = "#{new_tap}/#{name_or_token}" + + Tap.each_with_object([]) do |tap, array| + next unless (renames = tap.reverse_tap_migrations_renames[key]) + + array.concat(renames) + end + end + # Array with autobump names sig { returns(T::Array[String]) } def autobump From e6a453a20db67c3fa67787d1386d3a6be5f069e0 Mon Sep 17 00:00:00 2001 From: apainintheneck Date: Fri, 8 Mar 2024 20:13:20 -0800 Subject: [PATCH 4/5] tap: add some tests - Add tests for: - `Tap.each` - `Tap.installed` - `Tap.all` - `Tap#reverse_tap_migrations_renames` - `Tap.reverse_tap_migrations_renames` --- Library/Homebrew/test/tap_spec.rb | 85 ++++++++++++++++++++++++++++++- 1 file changed, 84 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/test/tap_spec.rb b/Library/Homebrew/test/tap_spec.rb index caf77e4a80..bd43008b9c 100644 --- a/Library/Homebrew/test/tap_spec.rb +++ b/Library/Homebrew/test/tap_spec.rb @@ -495,10 +495,52 @@ RSpec.describe Tap do expect(homebrew_foo_tap.config[:foo]).to be_nil end - describe "#each" do + describe ".each" do it "returns an enumerator if no block is passed" do expect(described_class.each).to be_an_instance_of(Enumerator) end + + context "when the core tap is not installed" do + around do |example| + FileUtils.rm_rf CoreTap.instance.path + example.run + ensure + (CoreTap.instance.path/"Formula").mkpath + end + + it "includes the core tap with the api" do + ENV.delete("HOMEBREW_NO_INSTALL_FROM_API") + expect(described_class.to_a).to include(CoreTap.instance) + end + + it "omits the core tap without the api" do + ENV["HOMEBREW_NO_INSTALL_FROM_API"] = "1" + expect(described_class.to_a).not_to include(CoreTap.instance) + end + end + end + + describe ".installed" do + it "includes only installed taps" do + expect(described_class.installed) + .to contain_exactly(CoreTap.instance, described_class.fetch("homebrew/foo")) + end + end + + describe ".all" do + it "includes the core and cask taps by default", :needs_macos do + expect(described_class.all).to contain_exactly( + CoreTap.instance, + CoreCaskTap.instance, + described_class.fetch("homebrew/foo"), + described_class.fetch("third-party/tap"), + ) + end + + it "includes the core tap and excludes the cask tap by default", :needs_linux do + expect(described_class.all) + .to contain_exactly(CoreTap.instance, described_class.fetch("homebrew/foo")) + end end describe "Formula Lists" do @@ -520,6 +562,47 @@ RSpec.describe Tap do end end + describe "tap migration renames" do + before do + (path/"tap_migrations.json").write <<~JSON + { + "adobe-air-sdk": "homebrew/cask", + "app-engine-go-32": "homebrew/cask/google-cloud-sdk", + "app-engine-go-64": "homebrew/cask/google-cloud-sdk", + "gimp": "homebrew/cask", + "horndis": "homebrew/cask", + "inkscape": "homebrew/cask", + "schismtracker": "homebrew/cask/schism-tracker" + } + JSON + end + + describe "#reverse_tap_migration_renames" do + it "returns the expected hash" do + expect(homebrew_foo_tap.reverse_tap_migrations_renames).to eq({ + "homebrew/cask/google-cloud-sdk" => %w[app-engine-go-32 app-engine-go-64], + "homebrew/cask/schism-tracker" => %w[schismtracker], + }) + end + end + + describe ".reverse_tap_migration_renames" do + let(:cask_tap) { CoreCaskTap.instance } + let(:core_tap) { CoreTap.instance } + + it "returns expected renames" do + [ + [cask_tap, "gimp", []], + [core_tap, "schism-tracker", []], + [cask_tap, "schism-tracker", %w[schismtracker]], + [cask_tap, "google-cloud-sdk", %w[app-engine-go-32 app-engine-go-64]], + ].each do |tap, name, result| + expect(described_class.reverse_tap_migrations_renames(tap, name)).to eq(result) + end + end + end + end + describe "#audit_exceptions" do it "returns the audit_exceptions hash" do setup_tap_files From 08442734ab9a62a333e92f08747f655ffe5dca0f Mon Sep 17 00:00:00 2001 From: apainintheneck Date: Fri, 8 Mar 2024 23:36:50 -0800 Subject: [PATCH 5/5] s/Tap.reverse_tap_migrations_renames/Tap.tap_migration_oldnames/ --- Library/Homebrew/cask/cask.rb | 2 +- Library/Homebrew/formula.rb | 2 +- Library/Homebrew/tap.rb | 10 +++++++--- .../test/api/internal_tap_json/formula_spec.rb | 2 +- Library/Homebrew/test/tap_spec.rb | 4 ++-- 5 files changed, 12 insertions(+), 8 deletions(-) diff --git a/Library/Homebrew/cask/cask.rb b/Library/Homebrew/cask/cask.rb index e3d6d51041..8c0870c4ec 100644 --- a/Library/Homebrew/cask/cask.rb +++ b/Library/Homebrew/cask/cask.rb @@ -87,7 +87,7 @@ module Cask sig { returns(T::Array[String]) } def old_tokens @old_tokens ||= if (tap = self.tap) - Tap.reverse_tap_migrations_renames(tap, token) + + Tap.tap_migration_oldnames(tap, token) + tap.cask_reverse_renames.fetch(token, []) else [] diff --git a/Library/Homebrew/formula.rb b/Library/Homebrew/formula.rb index d9394808b1..285f05a7c6 100644 --- a/Library/Homebrew/formula.rb +++ b/Library/Homebrew/formula.rb @@ -552,7 +552,7 @@ class Formula sig { returns(T::Array[String]) } def oldnames @oldnames ||= if (tap = self.tap) - Tap.reverse_tap_migrations_renames(tap, name) + + Tap.tap_migration_oldnames(tap, name) + tap.formula_reverse_renames.fetch(name, []) else [] diff --git a/Library/Homebrew/tap.rb b/Library/Homebrew/tap.rb index 27a93283aa..e38060f3b8 100644 --- a/Library/Homebrew/tap.rb +++ b/Library/Homebrew/tap.rb @@ -867,6 +867,9 @@ class Tap sig { returns(T::Hash[String, T::Array[String]]) } def reverse_tap_migrations_renames @reverse_tap_migrations_renames ||= tap_migrations.each_with_object({}) do |(old_name, new_name), hash| + # Only include renames: + # + `homebrew/cask/water-buffalo` + # - `homebrew/cask` next if new_name.count("/") != 2 hash[new_name] ||= [] @@ -874,9 +877,10 @@ class Tap end end - sig { params(new_tap: Tap, name_or_token: String).returns(T::Array[String]) } - def self.reverse_tap_migrations_renames(new_tap, name_or_token) - key = "#{new_tap}/#{name_or_token}" + # The old names a formula or cask had before getting migrated to the current tap. + sig { params(current_tap: Tap, name_or_token: String).returns(T::Array[String]) } + def self.tap_migration_oldnames(current_tap, name_or_token) + key = "#{current_tap}/#{name_or_token}" Tap.each_with_object([]) do |tap, array| next unless (renames = tap.reverse_tap_migrations_renames[key]) diff --git a/Library/Homebrew/test/api/internal_tap_json/formula_spec.rb b/Library/Homebrew/test/api/internal_tap_json/formula_spec.rb index fe5ebf47a9..1426677483 100644 --- a/Library/Homebrew/test/api/internal_tap_json/formula_spec.rb +++ b/Library/Homebrew/test/api/internal_tap_json/formula_spec.rb @@ -33,7 +33,7 @@ RSpec.describe "Internal Tap JSON -- Formula" do .with("internal/v3/homebrew-core.jws.json") .and_return([JSON.parse(internal_tap_json), false]) - # `Tap.reverse_tap_migrations_renames` looks for renames in every + # `Tap.tap_migration_oldnames` looks for renames in every # tap so `CoreCaskTap.tap_migrations` gets called and tries to # fetch stuff from the API. This just avoids errors. allow(Homebrew::API).to receive(:fetch_json_api_file) diff --git a/Library/Homebrew/test/tap_spec.rb b/Library/Homebrew/test/tap_spec.rb index bd43008b9c..0c2d110b3d 100644 --- a/Library/Homebrew/test/tap_spec.rb +++ b/Library/Homebrew/test/tap_spec.rb @@ -586,7 +586,7 @@ RSpec.describe Tap do end end - describe ".reverse_tap_migration_renames" do + describe ".tap_migration_oldnames" do let(:cask_tap) { CoreCaskTap.instance } let(:core_tap) { CoreTap.instance } @@ -597,7 +597,7 @@ RSpec.describe Tap do [cask_tap, "schism-tracker", %w[schismtracker]], [cask_tap, "google-cloud-sdk", %w[app-engine-go-32 app-engine-go-64]], ].each do |tap, name, result| - expect(described_class.reverse_tap_migrations_renames(tap, name)).to eq(result) + expect(described_class.tap_migration_oldnames(tap, name)).to eq(result) end end end