Merge pull request #14783 from MikeMcQuaid/official_tap_formula_cask_subdirs

Support loading formulae/casks from subdirectories
2023-02-24 12:47:46 +00:00 · 2023-02-24 12:47:46 +00:00 · bb4d518c0b
commit bb4d518c0b
parent 2d31f80f5c f280ce069b
9 changed files with 184 additions and 66 deletions
--- 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
--- 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)}"
--- 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
--- 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
--- a/Library/Homebrew/test/cask/cask_loader/from__path_loader_spec.rb
+++ b/Library/Homebrew/test/cask/cask_loader/from__path_loader_spec.rb
--- a/Library/Homebrew/test/cask/cask_loader/from_tap_loader_spec.rb
+++ 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
--- 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)
--- 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
--- 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