Merge pull request #10190 from SeekingMeaning/bottle-keep-old

bottle: check actual bottle block contents when `--keep-old`

Library/Homebrew/dev-cmd/bottle.rb

@@ -496,11 +496,14 @@ module Homebrew
         update_or_add = T.let(nil, T.nilable(String))
 
         Utils::Inreplace.inreplace(path) do |s|
-          if s.inreplace_string.include? "bottle do"
+          formula_contents = s.inreplace_string
+          bottle_node = Utils::AST.bottle_block(formula_contents)
+          if bottle_node.present?
             update_or_add = "update"
             if args.keep_old?
+              old_keys = Utils::AST.body_children(bottle_node.body).map(&:method_name)
               old_bottle_spec = Formulary.factory(path).bottle_specification
-              mismatches, checksums = merge_bottle_spec(old_bottle_spec, bottle_hash["bottle"])
+              mismatches, checksums = merge_bottle_spec(old_keys, old_bottle_spec, bottle_hash["bottle"])
               if mismatches.present?
                 odie <<~EOS
                   --keep-old was passed but there are changes in:
@@ -511,12 +514,12 @@ module Homebrew
               output = bottle_output bottle
             end
             puts output
-            Utils::AST.replace_bottle_stanza!(s.inreplace_string, output)
+            Utils::AST.replace_bottle_stanza!(formula_contents, output)
           else
             odie "--keep-old was passed but there was no existing bottle block!" if args.keep_old?
             puts output
             update_or_add = "add"
-            Utils::AST.add_bottle_stanza!(s.inreplace_string, output)
+            Utils::AST.add_bottle_stanza!(formula_contents, output)
           end
         end
 
@@ -539,33 +542,33 @@ module Homebrew
     end
   end
 
-  def merge_bottle_spec(old_bottle_spec, new_bottle_hash)
+  def merge_bottle_spec(old_keys, old_bottle_spec, new_bottle_hash)
     mismatches = []
     checksums = []
 
-    {
+    new_values = {
       root_url: new_bottle_hash["root_url"],
       prefix:   new_bottle_hash["prefix"],
-      cellar:   new_bottle_hash["cellar"].to_sym,
+      cellar:   new_bottle_hash["cellar"],
       rebuild:  new_bottle_hash["rebuild"],
-    }.each do |key, new_value|
+    }
-      old_value = old_bottle_spec.send(key)
+
-      next if key == :rebuild && old_value.zero?
+    old_keys.each do |key|
-      next if key == :prefix && old_value == Homebrew::DEFAULT_PREFIX
+      next if key == :sha256
-      next if key == :cellar && [
+
-        Homebrew::DEFAULT_MACOS_CELLAR,
+      old_value = old_bottle_spec.send(key).to_s
-        Homebrew::DEFAULT_MACOS_ARM_CELLAR,
+      new_value = new_values[key].to_s
-        Homebrew::DEFAULT_LINUX_CELLAR,
+      next if key == :cellar && old_value == "any" && new_value == "any_skip_relocation"
-      ].include?(old_value)
-      next if key == :cellar && old_value == :any && new_value == :any_skip_relocation
       next if old_value.present? && new_value == old_value
 
       mismatches << "#{key}: old: #{old_value.inspect}, new: #{new_value.inspect}"
     end
 
+    return [mismatches, checksums] unless old_keys.include? :sha256
+
     old_bottle_spec.collector.each_key do |tag|
       old_value = old_bottle_spec.collector[tag].hexdigest
-      new_value = new_bottle_hash["tags"][tag.to_s]
+      new_value = new_bottle_hash.dig("tags", tag.to_s)
       if new_value.present?
         mismatches << "sha256 => #{tag}"
       else

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

@@ -173,6 +173,66 @@ describe Homebrew do
       "d9cc50eec8ac243148a121049c236cba06af4a0b1156ab397d0a2850aa79c137",
     )
   end
+
+  describe "#merge_bottle_spec" do
+    it "allows new bottle hash to be empty" do
+      valid_keys = [:root_url, :prefix, :cellar, :rebuild, :sha256]
+      old_spec = BottleSpecification.new
+      old_spec.sha256("f59bc65c91e4e698f6f050e1efea0040f57372d4dcf0996cbb8f97ced320403b" => :big_sur)
+      expect { homebrew.merge_bottle_spec(valid_keys, old_spec, {}) }.not_to raise_error
+    end
+
+    it "checks for conflicting root URL" do
+      old_spec = BottleSpecification.new
+      old_spec.root_url("https://failbrew.bintray.com/bottles")
+      new_hash = { "root_url" => "https://testbrew.bintray.com/bottles" }
+      expect(homebrew.merge_bottle_spec([:root_url], old_spec, new_hash)).to eq [
+        ['root_url: old: "https://failbrew.bintray.com/bottles", new: "https://testbrew.bintray.com/bottles"'],
+        [],
+      ]
+    end
+
+    it "checks for conflicting prefix" do
+      old_spec = BottleSpecification.new
+      old_spec.prefix("/opt/failbrew")
+      new_hash = { "prefix" => "/opt/testbrew" }
+      expect(homebrew.merge_bottle_spec([:prefix], old_spec, new_hash)).to eq [
+        ['prefix: old: "/opt/failbrew", new: "/opt/testbrew"'],
+        [],
+      ]
+    end
+
+    it "checks for conflicting cellar" do
+      old_spec = BottleSpecification.new
+      old_spec.cellar("/opt/failbrew/Cellar")
+      new_hash = { "cellar" => "/opt/testbrew/Cellar" }
+      expect(homebrew.merge_bottle_spec([:cellar], old_spec, new_hash)).to eq [
+        ['cellar: old: "/opt/failbrew/Cellar", new: "/opt/testbrew/Cellar"'],
+        [],
+      ]
+    end
+
+    it "checks for conflicting rebuild number" do
+      old_spec = BottleSpecification.new
+      old_spec.rebuild(1)
+      new_hash = { "rebuild" => 2 }
+      expect(homebrew.merge_bottle_spec([:rebuild], old_spec, new_hash)).to eq [
+        ['rebuild: old: "1", new: "2"'],
+        [],
+      ]
+    end
+
+    it "checks for conflicting checksums" do
+      old_spec = BottleSpecification.new
+      old_spec.sha256("109c0cb581a7b5d84da36d84b221fb9dd0f8a927b3044d82611791c9907e202e" => :catalina)
+      old_spec.sha256("7571772bf7a0c9fe193e70e521318b53993bee6f351976c9b6e01e00d13d6c3f" => :mojave)
+      new_hash = { "tags" => { "catalina" => "ec6d7f08412468f28dee2be17ad8cd8b883b16b34329efcecce019b8c9736428" } }
+      expect(homebrew.merge_bottle_spec([:sha256], old_spec, new_hash)).to eq [
+        ["sha256 => catalina"],
+        [{ "7571772bf7a0c9fe193e70e521318b53993bee6f351976c9b6e01e00d13d6c3f" => :mojave }],
+      ]
+    end
+  end
 end
 
 describe "brew bottle --merge", :integration_test, :needs_linux do

Library/Homebrew/utils/ast.rb

@@ -9,6 +9,25 @@ module Utils
     class << self
       extend T::Sig
 
+      def body_children(body_node)
+        if body_node.nil?
+          []
+        elsif body_node.begin_type?
+          body_node.children.compact
+        else
+          [body_node]
+        end
+      end
+
+      def bottle_block(formula_contents)
+        formula_stanza(formula_contents, :bottle, type: :block_call)
+      end
+
+      def formula_stanza(formula_contents, name, type: nil)
+        _, children = process_formula(formula_contents)
+        children.find { |child| call_node_match?(child, name: name, type: type) }
+      end
+
       def replace_bottle_stanza!(formula_contents, bottle_output)
         replace_formula_stanza!(formula_contents, :bottle, bottle_output.strip, type: :block_call)
       end
@@ -18,8 +37,7 @@ module Utils
       end
 
       def replace_formula_stanza!(formula_contents, name, replacement, type: nil)
-        processed_source, body_node = process_formula(formula_contents)
+        processed_source, children = process_formula(formula_contents)
-        children = body_node.begin_type? ? body_node.children.compact : [body_node]
         stanza_node = children.find { |child| call_node_match?(child, name: name, type: type) }
         raise "Could not find #{name} stanza!" if stanza_node.nil?
 
@@ -29,10 +47,10 @@ module Utils
       end
 
       def add_formula_stanza!(formula_contents, name, text, type: nil)
-        processed_source, body_node = process_formula(formula_contents)
+        processed_source, children = process_formula(formula_contents)
 
-        preceding_component = if body_node.begin_type?
+        preceding_component = if children.length > 1
-          body_node.children.compact.reduce do |previous_child, current_child|
+          children.reduce do |previous_child, current_child|
             if formula_component_before_target?(current_child,
                                                 target_name: name,
                                                 target_type: type)
@@ -42,7 +60,7 @@ module Utils
             end
           end
         else
-          body_node
+          children.first
         end
         preceding_component = preceding_component.last_argument if preceding_component.send_type?
 
@@ -84,10 +102,10 @@ module Utils
 
         raise "Could not find formula class!" if class_node.nil?
 
-        body_node = class_node.body
+        children = body_children(class_node.body)
-        raise "Formula class is empty!" if body_node.nil?
+        raise "Formula class is empty!" if children.empty?
 
-        [processed_source, body_node]
+        [processed_source, children]
       end
 
       def formula_component_before_target?(node, target_name:, target_type: nil)