Revamp APIs around bottle specifications

Library/Homebrew/dev-cmd/bottle.rb

@@ -350,9 +350,9 @@ module Homebrew
       tab_json = Utils::Bottles.file_from_bottle(bottle_path, tab_path)
       tab = Tab.from_file_content(tab_json, tab_path)
 
-      _, _, bottle_cellar = Formula[f.name].bottle_specification.checksum_for(bottle_tag, no_older_versions: true)
+      tag_spec = Formula[f.name].bottle_specification.tag_specification_for(bottle_tag, no_older_versions: true)
-      relocatable = [:any, :any_skip_relocation].include?(bottle_cellar)
+      relocatable = [:any, :any_skip_relocation].include?(tag_spec.cellar)
-      skip_relocation = bottle_cellar == :any_skip_relocation
+      skip_relocation = tag_spec.cellar == :any_skip_relocation
 
       prefix = bottle_tag.default_prefix
       cellar = bottle_tag.default_cellar
@@ -506,7 +506,7 @@ module Homebrew
 
     old_spec = f.bottle_specification
     if args.keep_old? && !old_spec.checksums.empty?
-      mismatches = [:root_url, :prefix, :rebuild].reject do |key|
+      mismatches = [:root_url, :rebuild].reject do |key|
         old_spec.send(key) == bottle.send(key)
       end
       unless mismatches.empty?
@@ -551,7 +551,7 @@ module Homebrew
         },
         "bottle"  => {
           "root_url" => bottle.root_url,
-          "prefix"   => bottle.prefix,
+          "prefix"   => prefix.to_s, # TODO: 3.3.0: deprecate this
           "cellar"   => bottle_cellar.to_s,
           "rebuild"  => bottle.rebuild,
           "date"     => Pathname(filename.to_s).mtime.strftime("%F"),
@@ -640,16 +640,16 @@ module Homebrew
                              bottle_hash["formula"]["pkg_version"] == formula.pkg_version.to_s &&
                              bottle.rebuild  != old_bottle_spec.rebuild &&
                              bottle.root_url == old_bottle_spec.root_url
-        bottle.collector.keys.all? do |tag|
+        bottle.collector.tags.all? do |tag|
-          bottle_collector_tag = bottle.collector[tag]
+          tag_spec = bottle.collector.specification_for(tag)
-          next false if bottle_collector_tag.blank?
+          next false if tag_spec.blank?
 
-          old_bottle_spec_collector_tag = old_bottle_spec.collector[tag]
+          old_tag_spec = old_bottle_spec.collector.specification_for(tag)
-          next false if old_bottle_spec_collector_tag.blank?
+          next false if old_tag_spec.blank?
 
-          next false if bottle_collector_tag[:cellar] != old_bottle_spec_collector_tag[:cellar]
+          next false if tag_spec.cellar != old_tag_spec.cellar
 
-          bottle_collector_tag[:checksum].hexdigest == old_bottle_spec_collector_tag[:checksum].hexdigest
+          tag_spec.checksum.hexdigest == old_tag_spec.checksum.hexdigest
         end
       end
 
@@ -744,7 +744,6 @@ module Homebrew
 
     new_values = {
       root_url: new_bottle_hash["root_url"],
-      prefix:   new_bottle_hash["prefix"],
       rebuild:  new_bottle_hash["rebuild"],
     }
 
@@ -762,17 +761,17 @@ module Homebrew
 
     return [mismatches, checksums] if old_keys.exclude? :sha256
 
-    old_bottle_spec.collector.each_key do |tag|
+    old_bottle_spec.collector.each_tag do |tag|
-      old_checksum_hash = old_bottle_spec.collector[tag]
+      old_tag_spec = old_bottle_spec.collector.specification_for(tag)
-      old_hexdigest = old_checksum_hash[:checksum].hexdigest
+      old_hexdigest = old_tag_spec.checksum.hexdigest
-      old_cellar = old_checksum_hash[:cellar]
+      old_cellar = old_tag_spec.cellar
       new_value = new_bottle_hash.dig("tags", tag.to_s)
       if new_value.present? && new_value["sha256"] != old_hexdigest
         mismatches << "sha256 #{tag}: old: #{old_hexdigest.inspect}, new: #{new_value["sha256"].inspect}"
       elsif new_value.present? && new_value["cellar"] != old_cellar.to_s
         mismatches << "cellar #{tag}: old: #{old_cellar.to_s.inspect}, new: #{new_value["cellar"].inspect}"
       else
-        checksums << { cellar: old_cellar, tag => old_hexdigest }
+        checksums << { cellar: old_cellar, tag.to_sym => old_hexdigest }
       end
     end
 

Library/Homebrew/extend/os/linux/software_spec.rb

@@ -3,8 +3,9 @@
 
 class BottleSpecification
   extend T::Sig
-  sig { returns(T::Boolean) }
+
-  def skip_relocation?
+  sig { params(tag: Utils::Bottles::Tag).returns(T::Boolean) }
+  def skip_relocation?(tag: Utils::Bottles.tag)
     false
   end
 end

Library/Homebrew/extend/os/mac/utils/bottles.rb

@@ -41,11 +41,10 @@ module Utils
 
         return if tag_version.blank?
 
-        keys.find do |key|
+        tags.find do |candidate|
-          key_tag = Tag.from_symbol(key)
+          next if candidate.arch != tag.arch
-          next if key_tag.arch != tag.arch
 
-          key_tag.to_macos_version <= tag_version
+          candidate.to_macos_version <= tag_version
         rescue MacOSVersionError
           false
         end

Library/Homebrew/formula.rb

@@ -2034,17 +2034,17 @@ class Formula
       "root_url" => bottle_spec.root_url,
       "files"    => {},
     }
-    bottle_spec.collector.each_key do |os|
+    bottle_spec.collector.each_tag do |tag|
-      collector_os = bottle_spec.collector[os]
+      tag_spec = bottle_spec.collector.specification_for(tag)
-      os_cellar = collector_os[:cellar]
+      os_cellar = tag_spec.cellar
       os_cellar = os_cellar.inspect if os_cellar.is_a?(Symbol)
 
-      checksum = collector_os[:checksum].hexdigest
+      checksum = tag_spec.checksum.hexdigest
-      filename = Bottle::Filename.create(self, os, bottle_spec.rebuild)
+      filename = Bottle::Filename.create(self, tag, bottle_spec.rebuild)
       path, = Utils::Bottles.path_resolved_basename(bottle_spec.root_url, name, checksum, filename)
       url = "#{bottle_spec.root_url}/#{path}"
 
-      hash["files"][os] = {
+      hash["files"][tag.to_sym] = {
         "cellar" => os_cellar,
         "url"    => url,
         "sha256" => checksum,

Library/Homebrew/formula_installer.rb

@@ -150,13 +150,14 @@ class FormulaInstaller
       return false
     end
 
-    bottle = formula.bottle_specification
+    bottle = formula.bottle
-    unless bottle.compatible_locations?
+    if bottle && !bottle.compatible_locations?
       if output_warning
+        prefix = Pathname(bottle.cellar).parent
         opoo <<~EOS
           Building #{formula.full_name} from source as the bottle needs:
           - HOMEBREW_CELLAR: #{bottle.cellar} (yours is #{HOMEBREW_CELLAR})
-          - HOMEBREW_PREFIX: #{bottle.prefix} (yours is #{HOMEBREW_PREFIX})
+          - HOMEBREW_PREFIX: #{prefix} (yours is #{HOMEBREW_PREFIX})
         EOS
       end
       return false

Library/Homebrew/software_spec.rb

@@ -91,7 +91,7 @@ class SoftwareSpec
   end
 
   def bottle_defined?
-    !bottle_specification.collector.keys.empty?
+    !bottle_specification.collector.tags.empty?
   end
 
   def bottle_tag?(tag = nil)
@@ -302,7 +302,7 @@ class Bottle
 
   extend Forwardable
 
-  attr_reader :name, :resource, :prefix, :cellar, :rebuild
+  attr_reader :name, :resource, :cellar, :rebuild
 
   def_delegators :resource, :url, :verify_download_integrity
   def_delegators :resource, :cached_download
@@ -314,15 +314,14 @@ class Bottle
     @resource.specs[:bottle] = true
     @spec = spec
 
-    checksum, tag, cellar = spec.checksum_for(Utils::Bottles.tag(tag))
+    tag_spec = spec.tag_specification_for(Utils::Bottles.tag(tag))
 
-    @prefix = spec.prefix
+    @tag = tag_spec.tag
-    @tag = tag
+    @cellar = tag_spec.cellar
-    @cellar = cellar
     @rebuild = spec.rebuild
 
     @resource.version = formula.pkg_version
-    @resource.checksum = checksum
+    @resource.checksum = tag_spec.checksum
 
     root_url(spec.root_url, spec.root_url_specs)
   end
@@ -342,12 +341,12 @@ class Bottle
   end
 
   def compatible_locations?
-    @spec.compatible_locations?
+    @spec.compatible_locations?(tag: @tag)
   end
 
   # Does the bottle need to be relocated?
   def skip_relocation?
-    @spec.skip_relocation?
+    @spec.skip_relocation?(tag: @tag)
   end
 
   def stage
@@ -469,12 +468,11 @@ class BottleSpecification
 
   attr_rw :rebuild
   attr_accessor :tap
-  attr_reader :all_tags_cellar, :collector, :root_url_specs, :repository, :prefix
+  attr_reader :collector, :root_url_specs, :repository
 
   sig { void }
   def initialize
     @rebuild = 0
-    @prefix = Homebrew::DEFAULT_PREFIX
     @repository = Homebrew::DEFAULT_REPOSITORY
     @collector = Utils::Bottles::Collector.new
     @root_url_specs = {}
@@ -497,27 +495,26 @@ class BottleSpecification
     end
   end
 
-  def cellar(val = nil)
+  def cellar(_val = nil)
-    if val.present?
     odisabled(
       "`cellar` in a bottle block",
       "`brew style --fix` on the formula to update the style or use `sha256` with a `cellar:` argument",
     )
   end
 
-    if val.nil?
+  sig { params(tag: Utils::Bottles::Tag).returns(T::Boolean) }
-      return collector.dig(Utils::Bottles.tag.to_sym, :cellar) || @all_tags_cellar || Homebrew::DEFAULT_CELLAR
+  def compatible_locations?(tag: Utils::Bottles.tag)
+    spec = collector.specification_for(tag)
+    cellar = if spec.present?
+      spec.cellar
+    else
+      tag.default_cellar
     end
 
-    @all_tags_cellar = val
-  end
-
-  def compatible_locations?
-    # this looks like it should check prefix and repository too but to be
-    # `cellar :any` actually requires no references to the cellar, prefix or
-    # repository.
     return true if [:any, :any_skip_relocation].include?(cellar)
 
+    prefix = Pathname(cellar).parent.to_s
+
     compatible_cellar = cellar == HOMEBREW_CELLAR.to_s
     compatible_prefix = prefix == HOMEBREW_PREFIX.to_s
 
@@ -525,14 +522,15 @@ class BottleSpecification
   end
 
   # Does the {Bottle} this {BottleSpecification} belongs to need to be relocated?
-  sig { returns(T::Boolean) }
+  sig { params(tag: Utils::Bottles::Tag).returns(T::Boolean) }
-  def skip_relocation?
+  def skip_relocation?(tag: Utils::Bottles.tag)
-    cellar == :any_skip_relocation
+    spec = collector.specification_for(tag)
+    spec&.cellar == :any_skip_relocation
   end
 
   sig { params(tag: T.any(Symbol, Utils::Bottles::Tag), no_older_versions: T::Boolean).returns(T::Boolean) }
   def tag?(tag, no_older_versions: false)
-    checksum_for(tag, no_older_versions: no_older_versions) ? true : false
+    collector.tag?(tag, no_older_versions: no_older_versions)
   end
 
   # Checksum methods in the DSL's bottle block take
@@ -569,40 +567,35 @@ class BottleSpecification
 
     tag = Utils::Bottles::Tag.from_symbol(tag)
 
-    cellar ||= all_tags_cellar
     cellar ||= tag.default_cellar
 
-    collector[tag.to_sym] = { checksum: Checksum.new(digest), cellar: cellar }
+    collector.add(tag, checksum: Checksum.new(digest), cellar: cellar)
   end
 
   sig {
-    params(
+    params(tag: Utils::Bottles::Tag, no_older_versions: T::Boolean)
-      tag:               T.any(Symbol, Utils::Bottles::Tag),
+      .returns(T.nilable(Utils::Bottles::TagSpecification))
-      no_older_versions: T::Boolean,
-    ).returns(
-      T.nilable([Checksum, Symbol, T.any(Symbol, String)]),
-    )
   }
-  def checksum_for(tag, no_older_versions: false)
+  def tag_specification_for(tag, no_older_versions: false)
-    collector.fetch_checksum_for(tag, no_older_versions: no_older_versions)
+    collector.specification_for(tag, no_older_versions: no_older_versions)
   end
 
   def checksums
-    tags = collector.keys.sort_by do |sym|
+    tags = collector.tags.sort_by do |tag|
-      tag = Utils::Bottles::Tag.from_symbol(sym)
       version = tag.to_macos_version
       # Give arm64 bottles a higher priority so they are first
       priority = tag.arch == :arm64 ? "2" : "1"
-      "#{priority}.#{version}_#{sym}"
+      "#{priority}.#{version}_#{tag}"
     rescue MacOSVersionError
       # Sort non-MacOS tags below MacOS tags.
-      "0.#{sym}"
+      "0.#{tag}"
     end
     tags.reverse.map do |tag|
+      spec = collector.specification_for(tag)
       {
-        "tag"    => tag,
+        "tag"    => spec.tag.to_sym,
-        "digest" => collector[tag][:checksum],
+        "digest" => spec.checksum,
-        "cellar" => collector[tag][:cellar],
+        "cellar" => spec.cellar,
       }
     end
   end

Library/Homebrew/test/dev-cmd/bottle_spec.rb

@@ -386,7 +386,7 @@ describe "brew bottle" do
 
     describe "#merge_bottle_spec" do
       it "allows new bottle hash to be empty" do
-        valid_keys = [:root_url, :prefix, :cellar, :rebuild, :sha256]
+        valid_keys = [:root_url, :cellar, :rebuild, :sha256]
         old_spec = BottleSpecification.new
         old_spec.sha256(big_sur: "f59bc65c91e4e698f6f050e1efea0040f57372d4dcf0996cbb8f97ced320403b")
         expect { homebrew.merge_bottle_spec(valid_keys, old_spec, {}) }.not_to raise_error

Library/Homebrew/test/software_spec/bottle_spec.rb

@@ -17,8 +17,8 @@ describe BottleSpecification do
 
       checksums.each_pair do |cat, digest|
         bottle_spec.sha256(cat => digest)
-        checksum, = bottle_spec.checksum_for(cat)
+        tag_spec = bottle_spec.tag_specification_for(Utils::Bottles::Tag.from_symbol(cat))
-        expect(Checksum.new(digest)).to eq(checksum)
+        expect(Checksum.new(digest)).to eq(tag_spec.checksum)
       end
     end
 
@@ -32,11 +32,11 @@ describe BottleSpecification do
 
       checksums.each do |checksum|
         bottle_spec.sha256(cellar: checksum[:cellar], checksum[:tag] => checksum[:digest])
-        digest, tag, cellar = bottle_spec.checksum_for(checksum[:tag])
+        tag_spec = bottle_spec.tag_specification_for(Utils::Bottles::Tag.from_symbol(checksum[:tag]))
-        expect(Checksum.new(checksum[:digest])).to eq(digest)
+        expect(Checksum.new(checksum[:digest])).to eq(tag_spec.checksum)
-        expect(checksum[:tag]).to eq(tag)
+        expect(checksum[:tag]).to eq(tag_spec.tag.to_sym)
         checksum[:cellar] ||= Homebrew::DEFAULT_CELLAR
-        expect(checksum[:cellar]).to eq(cellar)
+        expect(checksum[:cellar]).to eq(tag_spec.cellar)
       end
     end
   end

Library/Homebrew/test/utils/bottles/collector_spec.rb

@@ -6,43 +6,50 @@ require "utils/bottles"
 describe Utils::Bottles::Collector do
   subject(:collector) { described_class.new }
 
-  describe "#fetch_checksum_for" do
+  let(:catalina) { Utils::Bottles::Tag.from_symbol(:catalina) }
+  let(:mojave) { Utils::Bottles::Tag.from_symbol(:mojave) }
+
+  describe "#specification_for" do
     it "returns passed tags" do
-      collector[:mojave] = { checksum: Checksum.new("foo_checksum"), cellar: "foo_cellar" }
+      collector.add(mojave, checksum: Checksum.new("foo_checksum"), cellar: "foo_cellar")
-      collector[:catalina] = { checksum: Checksum.new("bar_checksum"), cellar: "bar_cellar" }
+      collector.add(catalina, checksum: Checksum.new("bar_checksum"), cellar: "bar_cellar")
-      expect(collector.fetch_checksum_for(:catalina)).to eq(["bar_checksum", :catalina, "bar_cellar"])
+      spec = collector.specification_for(catalina)
+      expect(spec).not_to be_nil
+      expect(spec.tag).to eq(catalina)
+      expect(spec.checksum).to eq("bar_checksum")
+      expect(spec.cellar).to eq("bar_cellar")
     end
 
     it "returns nil if empty" do
-      expect(collector.fetch_checksum_for(:foo)).to be nil
+      expect(collector.specification_for(Utils::Bottles::Tag.from_symbol(:foo))).to be nil
     end
 
     it "returns nil when there is no match" do
-      collector[:catalina] = "foo"
+      collector.add(catalina, checksum: Checksum.new("bar_checksum"), cellar: "bar_cellar")
-      expect(collector.fetch_checksum_for(:foo)).to be nil
+      expect(collector.specification_for(Utils::Bottles::Tag.from_symbol(:foo))).to be nil
     end
 
     it "uses older tags when needed", :needs_macos do
-      collector[:mojave] = "foo"
+      collector.add(mojave, checksum: Checksum.new("foo_checksum"), cellar: "foo_cellar")
-      expect(collector.send(:find_matching_tag, Utils::Bottles::Tag.from_symbol(:mojave))).to eq(:mojave)
+      expect(collector.send(:find_matching_tag, mojave)).to eq(mojave)
-      expect(collector.send(:find_matching_tag, Utils::Bottles::Tag.from_symbol(:catalina))).to eq(:mojave)
+      expect(collector.send(:find_matching_tag, catalina)).to eq(mojave)
     end
 
     it "does not use older tags when requested not to", :needs_macos do
       allow(Homebrew::EnvConfig).to receive(:developer?).and_return(true)
       allow(Homebrew::EnvConfig).to receive(:skip_or_later_bottles?).and_return(true)
       allow(OS::Mac.version).to receive(:prerelease?).and_return(true)
-      collector[:mojave] = "foo"
+      collector.add(mojave, checksum: Checksum.new("foo_checksum"), cellar: "foo_cellar")
-      expect(collector.send(:find_matching_tag, Utils::Bottles::Tag.from_symbol(:mojave))).to eq(:mojave)
+      expect(collector.send(:find_matching_tag, mojave)).to eq(mojave)
-      expect(collector.send(:find_matching_tag, Utils::Bottles::Tag.from_symbol(:catalina))).to be_nil
+      expect(collector.send(:find_matching_tag, catalina)).to be_nil
     end
 
     it "ignores HOMEBREW_SKIP_OR_LATER_BOTTLES on release versions", :needs_macos do
       allow(Homebrew::EnvConfig).to receive(:skip_or_later_bottles?).and_return(true)
       allow(OS::Mac.version).to receive(:prerelease?).and_return(false)
-      collector[:mojave] = "foo"
+      collector.add(mojave, checksum: Checksum.new("foo_checksum"), cellar: "foo_cellar")
-      expect(collector.send(:find_matching_tag, Utils::Bottles::Tag.from_symbol(:mojave))).to eq(:mojave)
+      expect(collector.send(:find_matching_tag, mojave)).to eq(mojave)
-      expect(collector.send(:find_matching_tag, Utils::Bottles::Tag.from_symbol(:catalina))).to eq(:mojave)
+      expect(collector.send(:find_matching_tag, catalina)).to eq(mojave)
     end
   end
 end

Library/Homebrew/utils/bottles.rb

@@ -11,6 +11,7 @@ module Utils
     class << self
       extend T::Sig
 
+      # Gets the tag for the running OS.
       def tag(symbol = nil)
         return Tag.from_symbol(symbol) if symbol.present?
 
@@ -160,6 +161,14 @@ module Utils
         end
       end
 
+      def eql?(other)
+        self.class == other.class && self == other
+      end
+
+      def hash
+        [system, arch].hash
+      end
+
       sig { returns(Symbol) }
       def to_sym
         if system == :all && arch == :all
@@ -217,40 +226,74 @@ module Utils
       end
     end
 
+    # The specification for a specific tag
+    class TagSpecification
+      extend T::Sig
+
+      sig { returns(Utils::Bottles::Tag) }
+      attr_reader :tag
+
+      sig { returns(Checksum) }
+      attr_reader :checksum
+
+      sig { returns(T.any(Symbol, String)) }
+      attr_reader :cellar
+
+      def initialize(tag:, checksum:, cellar:)
+        @tag = tag
+        @checksum = checksum
+        @cellar = cellar
+      end
+    end
+
     # Collector for bottle specifications.
     class Collector
       extend T::Sig
 
-      extend Forwardable
-
-      def_delegators :@checksums, :keys, :[], :[]=, :key?, :each_key, :dig
-
       sig { void }
       def initialize
-        @checksums = {}
+        @tag_specs = T.let({}, T::Hash[Utils::Bottles::Tag, Utils::Bottles::TagSpecification])
+      end
+
+      sig { returns(T::Array[Utils::Bottles::Tag]) }
+      def tags
+        @tag_specs.keys
+      end
+
+      sig { params(tag: Utils::Bottles::Tag, checksum: Checksum, cellar: T.any(Symbol, String)).void }
+      def add(tag, checksum:, cellar:)
+        spec = Utils::Bottles::TagSpecification.new(tag: tag, checksum: checksum, cellar: cellar)
+        @tag_specs[tag] = spec
+      end
+
+      sig { params(tag: Utils::Bottles::Tag, no_older_versions: T::Boolean).returns(T::Boolean) }
+      def tag?(tag, no_older_versions: false)
+        tag = find_matching_tag(tag, no_older_versions: no_older_versions)
+        tag.present?
+      end
+
+      sig { params(block: T.proc.params(tag: Utils::Bottles::Tag).void).void }
+      def each_tag(&block)
+        @tag_specs.each_key(&block)
       end
 
       sig {
-        params(
+        params(tag: Utils::Bottles::Tag, no_older_versions: T::Boolean)
-          tag:               T.any(Symbol, Utils::Bottles::Tag),
+          .returns(T.nilable(Utils::Bottles::TagSpecification))
-          no_older_versions: T::Boolean,
-        ).returns(
-          T.nilable([Checksum, Symbol, T.any(Symbol, String)]),
-        )
       }
-      def fetch_checksum_for(tag, no_older_versions: false)
+      def specification_for(tag, no_older_versions: false)
-        tag = Utils::Bottles::Tag.from_symbol(tag) if tag.is_a?(Symbol)
+        tag = find_matching_tag(tag, no_older_versions: no_older_versions)
-        tag = find_matching_tag(tag, no_older_versions: no_older_versions)&.to_sym
+        @tag_specs[tag] if tag
-        return self[tag][:checksum], tag, self[tag][:cellar] if tag
       end
 
       private
 
       def find_matching_tag(tag, no_older_versions: false)
-        if key?(tag.to_sym)
+        if @tag_specs.key?(tag)
           tag
-        elsif key?(:all)
+        else
-          Tag.from_symbol(:all)
+          all = Tag.from_symbol(:all)
+          all if @tag_specs.key?(all)
         end
       end
     end