Merge pull request #23019 from Homebrew/test-bot-missing-all-bottle

MikeMcQuaid · web-flow · commit 53ee764fe9bc · 2026-07-09T07:14:36.000Z
Fix spurious test-bot missing `:all` bottle warning
diff --git a/Library/Homebrew/test/test_bot/formulae_spec.rb b/Library/Homebrew/test/test_bot/formulae_spec.rb
@@ -101,54 +101,84 @@
   end
 
   describe "#annotate_missing_all_bottle" do
+    sig { params(formula_path: Pathname, tag: Utils::Bottles::Tag, sha256: String).void }
+    def write_platform_bottle_formula(formula_path, tag, sha256)
+      formula_path.dirname.mkpath
+      formula_path.write <<~RUBY
+        class Foo < Formula
+          desc "Foo"
+          homepage "https://example.com"
+          url "foo-1.0"
+          sha256 "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+
+          bottle do
+            sha256 cellar: :any_skip_relocation, #{tag.to_sym}: "#{sha256}"
+          end
+        end
+      RUBY
+    end
+
+    sig {
+      params(
+        tap_path: Pathname,
+        tag:      T.any(String, Utils::Bottles::Tag),
+        sha256:   String,
+        cellar:   String,
+      ).void
+    }
+    def write_bottle_json(tap_path, tag, sha256, cellar: "any_skip_relocation")
+      (tap_path/"foo--1.0.#{tag}.bottle.json").write JSON.generate(
+        "foo" => {
+          "bottle" => {
+            "cellar" => cellar,
+            "tags"   => {
+              tag.to_s => {
+                "sha256" => sha256,
+              },
+            },
+          },
+        },
+      )
+    end
+
+    sig { params(formula_path: Pathname).returns(Formula) }
+    def all_bottle_formula(formula_path)
+      formula("foo", path: formula_path) do
+        T.bind(self, T.class_of(Formula))
+        url "foo-1.0"
+        bottle do
+          sha256 cellar: :any_skip_relocation,
+                 all:    "cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"
+        end
+      end
+    end
+
+    sig { params(tap_path: Pathname, tmpdir: String).returns(Homebrew::TestBot::Formulae) }
+    def formulae_test_bot(tap_path, tmpdir)
+      described_class.new(
+        tap: instance_double(Tap, path: tap_path), git: "git", dry_run: true, fail_fast: false, verbose: false,
+        output_paths: {
+          bottle:                     Pathname.new("#{tmpdir}/bottle.txt"),
+          linkage:                    Pathname.new("#{tmpdir}/linkage.txt"),
+          skipped_or_failed_formulae: Pathname.new("#{tmpdir}/skipped.txt"),
+        }
+      )
+    end
+
     it "writes a warning annotation for a platform-specific bottle replacing an all bottle" do
       Dir.mktmpdir do |tmpdir|
         tag = Utils::Bottles.tag
         sha256 = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"
+        other_tag = (tag.to_s == "arm64_tahoe") ? "tahoe" : "arm64_tahoe"
+        other_sha256 = "dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd"
         tap_path = Pathname(tmpdir)
         formula_path = tap_path/"Formula/foo.rb"
-        formula_path.dirname.mkpath
-        formula_path.write <<~RUBY
-          class Foo < Formula
-            desc "Foo"
-            homepage "https://example.com"
-            url "foo-1.0"
-            sha256 "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
-
-            bottle do
-              sha256 cellar: :any_skip_relocation, #{tag.to_sym}: "#{sha256}"
-            end
-          end
-        RUBY
-        (tap_path/"foo--1.0.#{tag}.bottle.json").write JSON.generate(
-          "foo" => {
-            "bottle" => {
-              "tags" => {
-                tag.to_s => {
-                  "cellar" => "any_skip_relocation",
-                  "sha256" => sha256,
-                },
-              },
-            },
-          },
-        )
+        write_platform_bottle_formula(formula_path, tag, sha256)
+        write_bottle_json(tap_path, tag, sha256)
+        write_bottle_json(tap_path, other_tag, other_sha256)
 
-        old_formula = formula("foo", path: formula_path) do
-          T.bind(self, T.class_of(Formula))
-          url "foo-1.0"
-          bottle do
-            sha256 cellar: :any_skip_relocation,
-                   all:    "cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"
-          end
-        end
-        formulae = described_class.new(
-          tap: instance_double(Tap, path: tap_path), git: "git", dry_run: true, fail_fast: false, verbose: false,
-          output_paths: {
-            bottle:                     Pathname.new("#{tmpdir}/bottle.txt"),
-            linkage:                    Pathname.new("#{tmpdir}/linkage.txt"),
-            skipped_or_failed_formulae: Pathname.new("#{tmpdir}/skipped.txt"),
-          }
-        )
+        old_formula = all_bottle_formula(formula_path)
+        formulae = formulae_test_bot(tap_path, tmpdir)
 
         with_env(GITHUB_ACTIONS: "true", GITHUB_WORKSPACE: tap_path.to_s) do
           expect { formulae.send(:annotate_missing_all_bottle, old_formula, bottle_dir: tap_path) }
@@ -162,6 +192,87 @@ class Foo < Formula
         end
       end
     end
+
+    it "does not write a warning annotation when local JSON already has an all bottle" do
+      Dir.mktmpdir do |tmpdir|
+        tag = Utils::Bottles.tag
+        sha256 = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"
+        all_sha256 = "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee"
+        tap_path = Pathname(tmpdir)
+        formula_path = tap_path/"Formula/foo.rb"
+        write_platform_bottle_formula(formula_path, tag, sha256)
+        write_bottle_json(tap_path, tag, sha256)
+        write_bottle_json(tap_path, "all", all_sha256)
+
+        old_formula = all_bottle_formula(formula_path)
+        formulae = formulae_test_bot(tap_path, tmpdir)
+
+        with_env(GITHUB_ACTIONS: "true", GITHUB_WORKSPACE: tap_path.to_s) do
+          expect { formulae.send(:annotate_missing_all_bottle, old_formula, bottle_dir: tap_path) }
+            .not_to output.to_stdout
+        end
+      end
+    end
+
+    it "does not write a warning annotation for a single platform-specific bottle" do
+      Dir.mktmpdir do |tmpdir|
+        tag = Utils::Bottles.tag
+        sha256 = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"
+        tap_path = Pathname(tmpdir)
+        formula_path = tap_path/"Formula/foo.rb"
+        write_platform_bottle_formula(formula_path, tag, sha256)
+        write_bottle_json(tap_path, tag, sha256)
+
+        old_formula = all_bottle_formula(formula_path)
+        formulae = formulae_test_bot(tap_path, tmpdir)
+
+        with_env(GITHUB_ACTIONS: "true", GITHUB_WORKSPACE: tap_path.to_s) do
+          expect { formulae.send(:annotate_missing_all_bottle, old_formula, bottle_dir: tap_path) }
+            .not_to output.to_stdout
+        end
+      end
+    end
+
+    it "writes a warning annotation when matching checksums have different cellars" do
+      Dir.mktmpdir do |tmpdir|
+        tag = Utils::Bottles.tag
+        other_tag = (tag.to_s == "arm64_tahoe") ? "tahoe" : "arm64_tahoe"
+        sha256 = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"
+        tap_path = Pathname(tmpdir)
+        formula_path = tap_path/"Formula/foo.rb"
+        write_platform_bottle_formula(formula_path, tag, sha256)
+        write_bottle_json(tap_path, tag, sha256)
+        write_bottle_json(tap_path, other_tag, sha256, cellar: "any")
+
+        old_formula = all_bottle_formula(formula_path)
+        formulae = formulae_test_bot(tap_path, tmpdir)
+
+        with_env(GITHUB_ACTIONS: "true", GITHUB_WORKSPACE: tap_path.to_s) do
+          expect { formulae.send(:annotate_missing_all_bottle, old_formula, bottle_dir: tap_path) }
+            .to output(/title=foo: missing :all bottle::.*sha256 `#{sha256}`/).to_stdout
+        end
+      end
+    end
+
+    it "does not write a warning annotation when platform bottles can become an all bottle" do
+      Dir.mktmpdir do |tmpdir|
+        tag = Utils::Bottles.tag
+        other_tag = (tag.to_s == "arm64_tahoe") ? "tahoe" : "arm64_tahoe"
+        sha256 = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"
+        tap_path = Pathname(tmpdir)
+        formula_path = tap_path/"Formula/foo.rb"
+        write_platform_bottle_formula(formula_path, tag, sha256)
+        [tag.to_s, other_tag].each { |bottle_tag| write_bottle_json(tap_path, bottle_tag, sha256) }
+
+        old_formula = all_bottle_formula(formula_path)
+        formulae = formulae_test_bot(tap_path, tmpdir)
+
+        with_env(GITHUB_ACTIONS: "true", GITHUB_WORKSPACE: tap_path.to_s) do
+          expect { formulae.send(:annotate_missing_all_bottle, old_formula, bottle_dir: tap_path) }
+            .not_to output.to_stdout
+        end
+      end
+    end
   end
 
   describe "#testing_portable_ruby?" do
diff --git a/Library/Homebrew/test_bot/formulae.rb b/Library/Homebrew/test_bot/formulae.rb
@@ -481,7 +481,14 @@ def annotate_missing_all_bottle(formula, bottle_dir: Pathname.pwd)
         end
         return if sha256_nodes.any? { |sha256_node| bottle_sha256_node_tag.call(sha256_node, :all) }
 
-        tag_hash = local_bottle_hash(formula.name, bottle_dir:)&.dig(formula.name, "bottle", "tags", bottle_tag.to_s)
+        # This predictor only has local JSONs, so mirror the merge-time tag count
+        # and cellar/checksum dedupe gates; final merge handles version/rebuild.
+        local_tag_hashes = local_bottle_tag_hashes(formula.name, bottle_dir:)
+        return if local_tag_hashes.key?("all")
+        return if local_tag_hashes.count < 2
+        return if local_tag_hashes.values.uniq { |tag_hash| [tag_hash["cellar"], tag_hash["sha256"]] }.one?
+
+        tag_hash = local_tag_hashes[bottle_tag.to_s]
         line = sha256_nodes.find do |sha256_node|
           bottle_sha256_node_tag.call(sha256_node, bottle_tag.to_sym)
         end&.source_range&.line
@@ -510,6 +517,29 @@ def annotate_missing_all_bottle(formula, bottle_dir: Pathname.pwd)
         opoo "Failed to determine missing `:all` bottle impact for #{formula.full_name}: #{e}"
       end
 
+      sig {
+        params(formula_name: String, bottle_dir: Pathname)
+          .returns(T::Hash[String, T::Hash[String, T.anything]])
+      }
+      def local_bottle_tag_hashes(formula_name, bottle_dir:)
+        tag_hashes = T.let({}, T::Hash[String, T::Hash[String, T.anything]])
+        bottle_glob(formula_name, bottle_dir, ".json", bottle_tag: "*").each do |local_bottle_json|
+          bottle_hash = JSON.parse(local_bottle_json.read).dig(formula_name, "bottle")
+          next unless bottle_hash.is_a?(Hash)
+
+          cellar = bottle_hash["cellar"]
+          tags = bottle_hash["tags"]
+          next unless tags.is_a?(Hash)
+
+          tags.each do |tag, tag_hash|
+            next if !tag.is_a?(String) || !tag_hash.is_a?(Hash)
+
+            tag_hashes[tag] = tag_hash.merge("cellar" => tag_hash["cellar"] || cellar)
+          end
+        end
+        tag_hashes
+      end
+
       sig { params(formula: Formula).void }
       def livecheck(formula)
         return unless formula.livecheck_defined?
