From f280ce069ba12f185ffd64f75e1d8ad48543da3f Mon Sep 17 00:00:00 2001 From: Mike McQuaid Date: Fri, 24 Feb 2023 10:57:41 +0000 Subject: [PATCH] Support loading formulae/casks from subdirectories Previously, we required all formulae and casks to be in a specific formula or cask directory but did not check any subdirectories. This commit allows using subdirectories for official taps, the only ones likely to be big enough to warrant sharding in this way and to avoid potentially breaking backwards compatibility for existing taps. This was inspired by the most recent issues with homebrew-cask. --- Library/Homebrew/cask/cask_loader.rb | 15 +- Library/Homebrew/cmd/tap-info.rb | 3 - Library/Homebrew/formulary.rb | 35 +++-- Library/Homebrew/tap.rb | 132 ++++++++++++------ ...oader_spec.rb => from_path_loader_spec.rb} | 0 .../cask/cask_loader/from_tap_loader_spec.rb | 35 +++++ Library/Homebrew/test/formulary_spec.rb | 21 ++- Library/Homebrew/test/missing_formula_spec.rb | 6 +- Library/Homebrew/test/tap_spec.rb | 3 - 9 files changed, 184 insertions(+), 66 deletions(-) rename Library/Homebrew/test/cask/cask_loader/{from__path_loader_spec.rb => from_path_loader_spec.rb} (100%) create mode 100644 Library/Homebrew/test/cask/cask_loader/from_tap_loader_spec.rb diff --git a/Library/Homebrew/cask/cask_loader.rb b/Library/Homebrew/cask/cask_loader.rb index 5223ad2ee8..c62af1c1e3 100644 --- a/Library/Homebrew/cask/cask_loader.rb +++ b/Library/Homebrew/cask/cask_loader.rb @@ -167,7 +167,9 @@ module Cask def initialize(tapped_name) user, repo, token = tapped_name.split("/", 3) - super Tap.fetch(user, repo).cask_dir/"#{token}.rb" + tap = Tap.fetch(user, repo) + cask = CaskLoader.find_cask_in_tap(token, tap) + super cask end def load(config:) @@ -421,8 +423,15 @@ module Cask end def self.tap_paths(token) - Tap.map { |t| t.cask_dir/"#{token.to_s.downcase}.rb" } - .select(&:exist?) + Tap.map do |tap| + find_cask_in_tap(token.to_s.downcase, tap) + end.select(&:exist?) + end + + def self.find_cask_in_tap(token, tap) + filename = "#{token}.rb" + + Tap.cask_files_by_name(tap).fetch(filename, tap.cask_dir/filename) end end end diff --git a/Library/Homebrew/cmd/tap-info.rb b/Library/Homebrew/cmd/tap-info.rb index d97b6168d0..f72c611abd 100644 --- a/Library/Homebrew/cmd/tap-info.rb +++ b/Library/Homebrew/cmd/tap-info.rb @@ -50,17 +50,14 @@ module Homebrew tap_count = 0 formula_count = 0 command_count = 0 - pinned_count = 0 private_count = 0 Tap.each do |tap| tap_count += 1 formula_count += tap.formula_files.size command_count += tap.command_files.size - pinned_count += 1 if tap.pinned? private_count += 1 if tap.private? end info = "#{tap_count} #{"tap".pluralize(tap_count)}" - info += ", #{pinned_count} pinned" info += ", #{private_count} private" info += ", #{formula_count} #{"formula".pluralize(formula_count)}" info += ", #{command_count} #{"command".pluralize(command_count)}" diff --git a/Library/Homebrew/formulary.rb b/Library/Homebrew/formulary.rb index 0aee5e3d6a..ee45ee0faa 100644 --- a/Library/Homebrew/formulary.rb +++ b/Library/Homebrew/formulary.rb @@ -516,15 +516,14 @@ module Formulary def formula_name_path(tapped_name, warn: true) user, repo, name = tapped_name.split("/", 3).map(&:downcase) @tap = Tap.fetch user, repo - formula_dir = @tap.formula_dir || @tap.path - path = formula_dir/"#{name}.rb" + path = find_formula_from_name(name) unless path.file? if (possible_alias = @tap.alias_dir/name).file? path = possible_alias.resolved_path name = path.basename(".rb").to_s elsif (new_name = @tap.formula_renames[name]) && - (new_path = formula_dir/"#{new_name}.rb").file? + (new_path = find_formula_from_name(new_name)).file? old_name = name path = new_path name = new_name @@ -562,6 +561,12 @@ module Formulary e.issues_url = tap.issues_url || tap.to_s raise end + + private + + def find_formula_from_name(name) + Formulary.find_formula_in_tap(name, @tap) + end end # Pseudo-loader which will raise a {FormulaUnavailableError} when trying to load the corresponding formula. @@ -800,22 +805,28 @@ module Formulary end def self.core_path(name) - CoreTap.instance.formula_dir/"#{name.to_s.downcase}.rb" + find_formula_in_tap(name.to_s.downcase, CoreTap.instance) end def self.core_alias_path(name) CoreTap.instance.alias_dir/name.to_s.downcase end - def self.tap_paths(name, taps = Dir[HOMEBREW_LIBRARY/"Taps/*/*/"]) + def self.tap_paths(name, taps = Tap) name = name.to_s.downcase taps.map do |tap| - Pathname.glob([ - "#{tap}Formula/#{name}.rb", - "#{tap}HomebrewFormula/#{name}.rb", - "#{tap}#{name}.rb", - "#{tap}Aliases/#{name}", - ]).find(&:file?) - end.compact + formula_path = find_formula_in_tap(name, tap) + + alias_path = tap.alias_dir/name + next alias_path if !formula_path.exist? && alias_path.exist? + + formula_path + end.select(&:file?) + end + + def self.find_formula_in_tap(name, tap) + filename = "#{name}.rb" + + Tap.formula_files_by_name(tap).fetch(filename, tap.formula_dir/filename) end end diff --git a/Library/Homebrew/tap.rb b/Library/Homebrew/tap.rb index cc70e67acf..8024e2e112 100644 --- a/Library/Homebrew/tap.rb +++ b/Library/Homebrew/tap.rb @@ -1,4 +1,4 @@ -# typed: false +# typed: true # frozen_string_literal: true require "commands" @@ -416,7 +416,6 @@ class Tap abv = path.abv formatted_contents = contents.presence&.to_sentence&.dup&.prepend(" ") - unpin if pinned? CacheStoreDatabase.use(:descriptions) do |db| DescriptionCacheStore.new(db) .delete_from_formula_names!(formula_names) @@ -451,15 +450,23 @@ class Tap end # Path to the directory of all {Formula} files for this {Tap}. + sig { returns(Pathname) } def formula_dir - @formula_dir ||= potential_formula_dirs.find(&:directory?) || (path/"Formula") + # Official formulae taps always use this directory, saves time to hardcode. + @formula_dir ||= if official? + path/"Formula" + else + potential_formula_dirs.find(&:directory?) || (path/"Formula") + end end + sig { returns(T::Array[Pathname]) } def potential_formula_dirs @potential_formula_dirs ||= [path/"Formula", path/"HomebrewFormula", path].freeze end # Path to the directory of all {Cask} files for this {Tap}. + sig { returns(Pathname) } def cask_dir @cask_dir ||= path/"Casks" end @@ -483,15 +490,35 @@ class Tap end # An array of all {Formula} files of this {Tap}. + sig { returns(T::Array[Pathname]) } def formula_files @formula_files ||= if formula_dir.directory? - formula_dir.children.select(&method(:ruby_file?)) + # TODO: odeprecate the non-official/old logic with a new minor release somehow? + if official? + formula_dir.find + else + formula_dir.children + end.select(&method(:ruby_file?)) else [] end end + # A cached hash of {Formula} basenames to {Formula} file pathnames for a {Tap} + sig { params(tap: Tap).returns(T::Hash[String, Pathname]) } + def self.formula_files_by_name(tap) + cache_key = "formula_files_by_name_#{tap}" + cache.fetch(cache_key) do |key| + cache[key] = tap.formula_files.each_with_object({}) do |file, hash| + # If there's more than one file with the same basename: intentionally + # ignore the later ones here. + hash[file.basename.to_s] ||= file + end + end + end + # An array of all versioned {Formula} files of this {Tap}. + sig { returns(T::Array[Pathname]) } def versioned_formula_files @versioned_formula_files ||= formula_files.select do |file| file.basename(".rb").to_s =~ /@[\d.]+$/ @@ -499,16 +526,37 @@ class Tap end # An array of all {Cask} files of this {Tap}. + sig { returns(T::Array[Pathname]) } def cask_files @cask_files ||= if cask_dir.directory? - cask_dir.children.select(&method(:ruby_file?)) + # TODO: odeprecate the non-official/old logic with a new minor release somehow? + if official? + cask_dir.find + else + cask_dir.children + end.select(&method(:ruby_file?)) else [] end end + # A cached hash of {Cask} basenames to {Cask} file pathnames for a {Tap} + sig { params(tap: Tap).returns(T::Hash[String, Pathname]) } + def self.cask_files_by_name(tap) + cache_key = "cask_files_by_name_#{tap}" + + cache.fetch(cache_key) do |key| + cache[key] = tap.cask_files.each_with_object({}) do |file, hash| + # If there's more than one file with the same basename: intentionally + # ignore the later ones here. + hash[file.basename.to_s] ||= file + end + end + end + # returns true if the file has a Ruby extension # @private + sig { params(file: Pathname).returns(T::Boolean) } def ruby_file?(file) file.extname == ".rb" end @@ -516,45 +564,56 @@ class Tap # return true if given path would present a {Formula} file in this {Tap}. # accepts both absolute path and relative path (relative to this {Tap}'s path) # @private + sig { params(file: T.any(String, Pathname)).returns(T::Boolean) } def formula_file?(file) file = Pathname.new(file) unless file.is_a? Pathname file = file.expand_path(path) - ruby_file?(file) && file.parent == formula_dir + return false unless ruby_file?(file) + + file.to_s.start_with?("#{formula_dir}/") end # return true if given path would present a {Cask} file in this {Tap}. # accepts both absolute path and relative path (relative to this {Tap}'s path) # @private + sig { params(file: T.any(String, Pathname)).returns(T::Boolean) } def cask_file?(file) file = Pathname.new(file) unless file.is_a? Pathname file = file.expand_path(path) - ruby_file?(file) && file.parent == cask_dir + return false unless ruby_file?(file) + + file.to_s.start_with?("#{cask_dir}/") end # An array of all {Formula} names of this {Tap}. + sig { returns(T::Array[String]) } def formula_names @formula_names ||= formula_files.map(&method(:formula_file_to_name)) end # An array of all {Cask} tokens of this {Tap}. + sig { returns(T::Array[String]) } def cask_tokens @cask_tokens ||= cask_files.map(&method(:formula_file_to_name)) end # path to the directory of all alias files for this {Tap}. # @private + sig { returns(Pathname) } def alias_dir @alias_dir ||= path/"Aliases" end # an array of all alias files of this {Tap}. # @private + sig { returns(T::Array[Pathname]) } def alias_files @alias_files ||= Pathname.glob("#{alias_dir}/*").select(&:file?) end # an array of all aliases of this {Tap}. # @private + sig { returns(T::Array[String]) } def aliases @aliases ||= alias_files.map { |f| alias_file_to_name(f) } end @@ -584,11 +643,13 @@ class Tap @alias_reverse_table end + sig { returns(Pathname) } def command_dir @command_dir ||= path/"cmd" end # An array of all commands files of this {Tap}. + sig { returns(T::Array[Pathname]) } def command_files @command_files ||= if command_dir.directory? Commands.find_commands(command_dir) @@ -597,19 +658,7 @@ class Tap end end - # path to the pin record for this {Tap}. - # @private - def pinned_symlink_path - HOMEBREW_LIBRARY/"PinnedTaps/#{name}" - end - - # True if this {Tap} has been pinned. - def pinned? - return @pinned if instance_variable_defined?(:@pinned) - - @pinned = pinned_symlink_path.directory? - end - + sig { returns(Hash) } def to_hash hash = { "name" => name, @@ -635,6 +684,7 @@ class Tap end # Hash with tap formula renames. + sig { returns(Hash) } def formula_renames @formula_renames ||= if (rename_file = path/HOMEBREW_TAP_FORMULA_RENAMES_FILE).file? JSON.parse(rename_file.read) @@ -644,6 +694,7 @@ class Tap end # Hash with tap migrations. + sig { returns(Hash) } def tap_migrations @tap_migrations ||= if (migration_file = path/HOMEBREW_TAP_MIGRATIONS_FILE).file? JSON.parse(migration_file.read) @@ -665,18 +716,19 @@ class Tap end # Hash with pypi formula mappings - sig { returns(Hash) } def pypi_formula_mappings @pypi_formula_mappings = read_formula_list path/HOMEBREW_TAP_PYPI_FORMULA_MAPPINGS end # @private + sig { returns(T::Boolean) } def should_report_analytics? return Homebrew::EnvConfig.install_from_api? && official? unless installed? !private? end + sig { params(other: T.nilable(T.any(String, Tap))).returns(T::Boolean) } def ==(other) other = Tap.fetch(other) if other.is_a?(String) self.class == other.class && name == other.name @@ -695,6 +747,7 @@ class Tap end # An array of all installed {Tap} names. + sig { returns(T::Array[String]) } def self.names map(&:name).sort end @@ -712,11 +765,13 @@ class Tap end # @private + sig { params(file: Pathname).returns(String) } def formula_file_to_name(file) "#{name}/#{file.basename(".rb")}" end # @private + sig { params(file: Pathname).returns(String) } def alias_file_to_name(file) "#{name}/#{file.basename}" end @@ -796,10 +851,12 @@ class CoreTap < Tap super "Homebrew", "core" end + sig { returns(CoreTap) } def self.instance @instance ||= new end + sig { void } def self.ensure_installed! return if instance.installed? return if Homebrew::EnvConfig.install_from_api? @@ -811,6 +868,7 @@ class CoreTap < Tap instance.install end + sig { returns(String) } def remote super if installed? || !Homebrew::EnvConfig.install_from_api? @@ -840,24 +898,6 @@ class CoreTap < Tap super end - # @private - sig { void } - def pin - raise "Tap#pin is not available for CoreTap" - end - - # @private - sig { void } - def unpin - raise "Tap#unpin is not available for CoreTap" - end - - # @private - sig { returns(T::Boolean) } - def pinned? - false - end - # @private sig { returns(T::Boolean) } def core_tap? @@ -871,6 +911,7 @@ class CoreTap < Tap end # @private + sig { returns(Pathname) } def formula_dir @formula_dir ||= begin self.class.ensure_installed! @@ -879,6 +920,7 @@ class CoreTap < Tap end # @private + sig { returns(Pathname) } def alias_dir @alias_dir ||= begin self.class.ensure_installed! @@ -887,6 +929,7 @@ class CoreTap < Tap end # @private + sig { returns(Hash) } def formula_renames @formula_renames ||= begin self.class.ensure_installed! @@ -895,6 +938,7 @@ class CoreTap < Tap end # @private + sig { returns(Hash) } def tap_migrations @tap_migrations ||= begin self.class.ensure_installed! @@ -903,6 +947,7 @@ class CoreTap < Tap end # @private + sig { returns(Hash) } def audit_exceptions @audit_exceptions ||= begin self.class.ensure_installed! @@ -911,6 +956,7 @@ class CoreTap < Tap end # @private + sig { returns(Hash) } def style_exceptions @style_exceptions ||= begin self.class.ensure_installed! @@ -919,6 +965,7 @@ class CoreTap < Tap end # @private + sig { returns(Hash) } def pypi_formula_mappings @pypi_formula_mappings ||= begin self.class.ensure_installed! @@ -927,16 +974,19 @@ class CoreTap < Tap end # @private + sig { params(file: Pathname).returns(String) } def formula_file_to_name(file) file.basename(".rb").to_s end # @private + sig { params(file: Pathname).returns(String) } def alias_file_to_name(file) file.basename.to_s end # @private + sig { returns(T::Array[String]) } def aliases return super if installed? || !Homebrew::EnvConfig.install_from_api? @@ -944,6 +994,7 @@ class CoreTap < Tap end # @private + sig { returns(T::Array[String]) } def formula_names return super if installed? || !Homebrew::EnvConfig.install_from_api? @@ -953,8 +1004,11 @@ end # Permanent configuration per {Tap} using `git-config(1)`. class TapConfig + extend T::Sig + attr_reader :tap + sig { params(tap: Tap).void } def initialize(tap) @tap = tap end diff --git a/Library/Homebrew/test/cask/cask_loader/from__path_loader_spec.rb b/Library/Homebrew/test/cask/cask_loader/from_path_loader_spec.rb similarity index 100% rename from Library/Homebrew/test/cask/cask_loader/from__path_loader_spec.rb rename to Library/Homebrew/test/cask/cask_loader/from_path_loader_spec.rb diff --git a/Library/Homebrew/test/cask/cask_loader/from_tap_loader_spec.rb b/Library/Homebrew/test/cask/cask_loader/from_tap_loader_spec.rb new file mode 100644 index 0000000000..4d077cdc8d --- /dev/null +++ b/Library/Homebrew/test/cask/cask_loader/from_tap_loader_spec.rb @@ -0,0 +1,35 @@ +# typed: false +# frozen_string_literal: true + +describe Cask::CaskLoader::FromTapLoader do + let(:cask_name) { "testball" } + let(:cask_full_name) { "homebrew/cask/#{cask_name}" } + let(:cask_path) { Tap.default_cask_tap.cask_dir/"#{cask_name}.rb" } + + describe "#load" do + before do + cask_path.parent.mkpath + cask_path.write <<~RUBY + cask '#{cask_name}' do + url 'https://brew.sh/' + end + RUBY + end + + it "returns a Cask" do + expect(described_class.new(cask_full_name).load(config: nil)).to be_a(Cask::Cask) + end + + it "raises an error if the Cask cannot be found" do + expect { described_class.new("foo/bar/baz").load(config: nil) }.to raise_error(Cask::CaskUnavailableError) + end + + context "with sharded Cask directory" do + let(:cask_path) { Tap.default_cask_tap.cask_dir/cask_name[0]/"#{cask_name}.rb" } + + it "returns a Cask" do + expect(described_class.new(cask_full_name).load(config: nil)).to be_a(Cask::Cask) + end + end + end +end diff --git a/Library/Homebrew/test/formulary_spec.rb b/Library/Homebrew/test/formulary_spec.rb index 910931c86f..1aa1956539 100644 --- a/Library/Homebrew/test/formulary_spec.rb +++ b/Library/Homebrew/test/formulary_spec.rb @@ -81,6 +81,21 @@ describe Formulary do }.to raise_error(ArgumentError) end + context "with sharded Formula directory" do + before { CoreTap.instance.clear_cache } + + let(:formula_name) { "testball_sharded" } + let(:formula_path) { CoreTap.new.formula_dir/formula_name[0]/"#{formula_name}.rb" } + + it "returns a Formula" do + expect(described_class.factory(formula_name)).to be_a(Formula) + end + + it "returns a Formula when given a fully qualified name" do + expect(described_class.factory("homebrew/core/#{formula_name}")).to be_a(Formula) + end + end + context "when the Formula has the wrong class" do let(:formula_name) { "giraffe" } let(:formula_content) do @@ -171,7 +186,7 @@ describe Formulary do context "when loading from Tap" do let(:tap) { Tap.new("homebrew", "foo") } let(:another_tap) { Tap.new("homebrew", "bar") } - let(:formula_path) { tap.path/"#{formula_name}.rb" } + let(:formula_path) { tap.path/"Formula/#{formula_name}.rb" } it "returns a Formula when given a name" do expect(described_class.factory(formula_name)).to be_a(Formula) @@ -195,8 +210,8 @@ describe Formulary do end it "raises an error if a Formula is in multiple Taps" do - another_tap.path.mkpath - (another_tap.path/"#{formula_name}.rb").write formula_content + (another_tap.path/"Formula").mkpath + (another_tap.path/"Formula/#{formula_name}.rb").write formula_content expect { described_class.factory(formula_name) diff --git a/Library/Homebrew/test/missing_formula_spec.rb b/Library/Homebrew/test/missing_formula_spec.rb index d93851f460..84e96336bd 100644 --- a/Library/Homebrew/test/missing_formula_spec.rb +++ b/Library/Homebrew/test/missing_formula_spec.rb @@ -61,8 +61,8 @@ describe Homebrew::MissingFormula do before do tap_path = Tap::TAP_DIRECTORY/"homebrew/homebrew-foo" - tap_path.mkpath - (tap_path/"deleted-formula.rb").write "placeholder" + (tap_path/"Formula").mkpath + (tap_path/"Formula/deleted-formula.rb").write "placeholder" ENV.delete "GIT_AUTHOR_DATE" ENV.delete "GIT_COMMITTER_DATE" @@ -70,7 +70,7 @@ describe Homebrew::MissingFormula do system "git", "init" system "git", "add", "--all" system "git", "commit", "-m", "initial state" - system "git", "rm", "deleted-formula.rb" + system "git", "rm", "Formula/deleted-formula.rb" system "git", "commit", "-m", "delete formula 'deleted-formula'" end end diff --git a/Library/Homebrew/test/tap_spec.rb b/Library/Homebrew/test/tap_spec.rb index b13ee778b3..3850652ec2 100644 --- a/Library/Homebrew/test/tap_spec.rb +++ b/Library/Homebrew/test/tap_spec.rb @@ -528,15 +528,12 @@ describe Tap do expect(core_tap.name).to eq("homebrew/core") expect(core_tap.command_files).to eq([]) expect(core_tap).to be_installed - expect(core_tap).not_to be_pinned expect(core_tap).to be_official expect(core_tap).to be_a_core_tap end specify "forbidden operations" do expect { core_tap.uninstall }.to raise_error(RuntimeError) - expect { core_tap.pin }.to raise_error(RuntimeError) - expect { core_tap.unpin }.to raise_error(RuntimeError) end specify "files" do