linkage_cache: cache input data and not results.

Cache all the non-weak dynamic library links for a keg rather than the
result of running `brew linkage`. This means that we correctly handle
changes to e.g. what non-keg files are present on disk.

Library/Homebrew/linkage_cache_store.rb

@@ -18,106 +18,55 @@ class LinkageCacheStore < CacheStore
   #
   # @return [Boolean]
   def keg_exists?
-    !database.get(keg_name).nil?
+    !database.get(@keg_name).nil?
   end
 
   # Inserts dylib-related information into the cache if it does not exist or
   # updates data into the linkage cache if it does exist
   #
-  # @param  [Hash] array_values: hash containing KVPs of { :type => Array | Set }
   # @param  [Hash] hash_values:  hash containing KVPs of { :type => Hash }
-  # @param  [Array[Hash]] values
-  # @raise  [TypeError] error if the values are not `Arary`, `Set`, or `Hash`
   # @return [nil]
-  def update!(array_values: {}, hash_values: {}, **values)
+  def update!(hash_values)
-    values.each do |key, value|
+    hash_values.each_key do |type|
-      if value.is_a?(Hash)
+      next if HASH_LINKAGE_TYPES.include?(type)
-        hash_values[key] = value
+
-      elsif value.is_a?(Array) || value.is_a?(Set)
+      raise TypeError, <<~EOS
-        array_values[key] = value
+        Can't update types that are not defined for the linkage store
-      else
+      EOS
-        raise TypeError, <<~EOS
-          Can't store types that are not `Array`, `Set` or `Hash` in the
-          linkage store.
-        EOS
-      end
     end
 
-    database.set keg_name, ruby_hash_to_json_string(
+    database.set @keg_name, ruby_hash_to_json_string(hash_values)
-      array_values: format_array_values(array_values),
-      hash_values: format_hash_values(hash_values),
-    )
   end
 
   # @param  [Symbol] the type to fetch from the `LinkageCacheStore`
-  # @raise  [TypeError] error if the type is not in `HASH_LINKAGE_TYPES` or
+  # @raise  [TypeError] error if the type is not in `HASH_LINKAGE_TYPES`
-  #   `ARRAY_LINKAGE_TYPES`
+  # @return [Hash]
-  # @return [Hash | Array]
   def fetch_type(type)
-    if HASH_LINKAGE_TYPES.include?(type)
+    unless HASH_LINKAGE_TYPES.include?(type)
-      fetch_hash_values(type)
-    elsif ARRAY_LINKAGE_TYPES.include?(type)
-      fetch_array_values(type)
-    else
       raise TypeError, <<~EOS
         Can't fetch types that are not defined for the linkage store
       EOS
     end
+
+    return {} unless keg_exists?
+
+    fetch_hash_values(type)
   end
 
   # @return [nil]
   def flush_cache!
-    database.delete(keg_name)
+    database.delete(@keg_name)
   end
 
   private
 
-  ARRAY_LINKAGE_TYPES = [
+  HASH_LINKAGE_TYPES = [:keg_files_dylibs].freeze
-    :system_dylibs, :variable_dylibs, :broken_dylibs, :indirect_deps,
-    :undeclared_deps, :unnecessary_deps
-  ].freeze
-  HASH_LINKAGE_TYPES = [:brewed_dylibs, :reverse_links, :broken_deps].freeze
-
-  # @return [String] the key to lookup items in the `CacheStore`
-  attr_reader :keg_name
-
-  # @param  [Symbol] the type to fetch from the `LinkageCacheStore`
-  # @return [Array]
-  def fetch_array_values(type)
-    keg_cache = database.get(keg_name)
-    return [] unless keg_cache
-    json_string_to_ruby_hash(keg_cache)["array_values"][type.to_s]
-  end
 
   # @param  [Symbol] type
   # @return [Hash]
   def fetch_hash_values(type)
-    keg_cache = database.get(keg_name)
+    keg_cache = database.get(@keg_name)
     return {} unless keg_cache
-    json_string_to_ruby_hash(keg_cache)["hash_values"][type.to_s]
+    json_string_to_ruby_hash(keg_cache)[type.to_s]
-  end
-
-  # Formats the linkage data for `array_values` into a kind which can be parsed
-  # by the `json_string_to_ruby_hash` method. Internally converts ruby `Set`s to
-  # `Array`s
-  #
-  # @param  [Hash]
-  # @return [String]
-  def format_array_values(hash)
-    hash.each_with_object({}) { |(k, v), h| h[k] = v.to_a }
-  end
-
-  # Formats the linkage data for `hash_values` into a kind which can be parsed
-  # by the `json_string_to_ruby_hash` method. Converts ruby `Set`s to `Array`s,
-  # and converts ruby `Pathname`s to `String`s
-  #
-  # @param  [Hash]
-  # @return [String]
-  def format_hash_values(hash)
-    hash.each_with_object({}) do |(outer_key, outer_values), outer_hash|
-      outer_hash[outer_key] = outer_values.each_with_object({}) do |(k, v), h|
-        h[k] = v.to_a.map(&:to_s)
-      end
-    end
   end
 end

Library/Homebrew/linkage_checker.rb

@@ -3,140 +3,119 @@ require "formula"
 require "linkage_cache_store"
 
 class LinkageChecker
-  def initialize(keg, formula = nil, use_cache: false, cache_db:)
+  attr_reader :undeclared_deps
+
+  def initialize(keg, formula = nil, cache_db:,
+                 use_cache: !ENV["HOMEBREW_LINKAGE_CACHE"].nil?)
     @keg = keg
     @formula = formula || resolve_formula(keg)
+    @store = LinkageCacheStore.new(keg.name, cache_db) if use_cache
 
-    if use_cache
+    @system_dylibs    = Set.new
-      @store = LinkageCacheStore.new(keg.name, cache_db)
+    @broken_dylibs    = Set.new
-      flush_cache_and_check_dylibs unless @store.keg_exists?
+    @variable_dylibs  = Set.new
-    else
+    @brewed_dylibs    = Hash.new { |h, k| h[k] = Set.new }
-      flush_cache_and_check_dylibs
+    @reverse_links    = Hash.new { |h, k| h[k] = Set.new }
-    end
+    @broken_deps      = Hash.new { |h, k| h[k] = [] }
+    @indirect_deps    = []
+    @undeclared_deps  = []
+    @unnecessary_deps = []
+
+    check_dylibs
   end
 
   def display_normal_output
-    display_items "System libraries", system_dylibs
+    display_items "System libraries", @system_dylibs
-    display_items "Homebrew libraries", brewed_dylibs
+    display_items "Homebrew libraries", @brewed_dylibs
-    display_items "Indirect dependencies with linkage", indirect_deps
+    display_items "Indirect dependencies with linkage", @indirect_deps
-    display_items "Variable-referenced libraries", variable_dylibs
+    display_items "Variable-referenced libraries", @variable_dylibs
-    display_items "Missing libraries", broken_dylibs
+    display_items "Missing libraries", @broken_dylibs
-    display_items "Broken dependencies", broken_deps
+    display_items "Broken dependencies", @broken_deps
-    display_items "Undeclared dependencies with linkage", undeclared_deps
+    display_items "Undeclared dependencies with linkage", @undeclared_deps
-    display_items "Dependencies with no linkage", unnecessary_deps
+    display_items "Dependencies with no linkage", @unnecessary_deps
   end
 
   def display_reverse_output
-    return if reverse_links.empty?
+    return if @reverse_links.empty?
-    sorted = reverse_links.sort
+    @reverse_links.sort.each do |dylib, files|
-    sorted.each do |dylib, files|
       puts dylib
       files.each do |f|
         unprefixed = f.to_s.strip_prefix "#{keg}/"
         puts "  #{unprefixed}"
       end
-      puts unless dylib == sorted.last[0]
+      puts if dylib != sorted.last.first
     end
   end
 
   def display_test_output(puts_output: true)
-    display_items "Missing libraries", broken_dylibs, puts_output: puts_output
+    display_items "Missing libraries", @broken_dylibs, puts_output: puts_output
-    display_items "Broken dependencies", broken_deps, puts_output: puts_output
+    display_items "Broken dependencies", @broken_deps, puts_output: puts_output
     puts "No broken library linkage" unless broken_library_linkage?
   end
 
   def broken_library_linkage?
-    !broken_dylibs.empty? || !broken_deps.empty?
+    !@broken_dylibs.empty? || !@broken_deps.empty?
-  end
-
-  def undeclared_deps
-    @undeclared_deps ||= store.fetch_type(:undeclared_deps)
   end
 
   private
 
   attr_reader :keg, :formula, :store
 
-  # 'Hash-type' cache values
-
-  def brewed_dylibs
-    @brewed_dylibs ||= store.fetch_type(:brewed_dylibs)
-  end
-
-  def reverse_links
-    @reverse_links ||= store.fetch_type(:reverse_links)
-  end
-
-  def broken_deps
-    @broken_deps ||= store.fetch_type(:broken_deps)
-  end
-
-  # 'Path-type' cached values
-
-  def system_dylibs
-    @system_dylibs ||= store.fetch_type(:system_dylibs)
-  end
-
-  def broken_dylibs
-    @broken_dylibs ||= store.fetch_type(:broken_dylibs)
-  end
-
-  def variable_dylibs
-    @variable_dylibs ||= store.fetch_type(:variable_dylibs)
-  end
-
-  def indirect_deps
-    @indirect_deps ||= store.fetch_type(:indirect_deps)
-  end
-
-  def unnecessary_deps
-    @unnecessary_deps ||= store.fetch_type(:unnecessary_deps)
-  end
-
   def dylib_to_dep(dylib)
     dylib =~ %r{#{Regexp.escape(HOMEBREW_PREFIX)}/(opt|Cellar)/([\w+-.@]+)/}
     Regexp.last_match(2)
   end
 
-  def flush_cache_and_check_dylibs
+  def check_dylibs
-    reset_dylibs!
+    keg_files_dylibs_was_empty = false
+    keg_files_dylibs = store&.fetch_type(:keg_files_dylibs)
+    keg_files_dylibs ||= {}
+    if keg_files_dylibs.empty?
+      keg_files_dylibs_was_empty = true
+      @keg.find do |file|
+        next if file.symlink? || file.directory?
+        next if !file.dylib? && !file.binary_executable? && !file.mach_o_bundle?
+        # weakly loaded dylibs may not actually exist on disk, so skip them
+        # when checking for broken linkage
+        keg_files_dylibs[file] =
+          file.dynamically_linked_libraries(except: :LC_LOAD_WEAK_DYLIB)
+      end
+    end
 
     checked_dylibs = Set.new
-    @keg.find do |file|
-      next if file.symlink? || file.directory?
-      next if !file.dylib? && !file.binary_executable? && !file.mach_o_bundle?
 
-      # weakly loaded dylibs may not actually exist on disk, so skip them
+    keg_files_dylibs.each do |file, dylibs|
-      # when checking for broken linkage
+      dylibs.each do |dylib|
-      file.dynamically_linked_libraries(except: :LC_LOAD_WEAK_DYLIB)
-          .each do |dylib|
         @reverse_links[dylib] << file
+
         next if checked_dylibs.include? dylib
+        checked_dylibs << dylib
+
         if dylib.start_with? "@"
           @variable_dylibs << dylib
-        else
+          next
-          begin
+        end
-            owner = Keg.for Pathname.new(dylib)
+
-          rescue NotAKegError
+        begin
-            @system_dylibs << dylib
+          owner = Keg.for Pathname.new(dylib)
-          rescue Errno::ENOENT
+        rescue NotAKegError
-            next if harmless_broken_link?(dylib)
+          @system_dylibs << dylib
-            if (dep = dylib_to_dep(dylib))
+        rescue Errno::ENOENT
-              @broken_deps[dep] |= [dylib]
+          next if harmless_broken_link?(dylib)
-            else
+          if (dep = dylib_to_dep(dylib))
-              @broken_dylibs << dylib
+            @broken_deps[dep] |= [dylib]
-            end
+          else
-          else
+            @broken_dylibs << dylib
-            tap = Tab.for_keg(owner).tap
+          end
-            f = if tap.nil? || tap.core_tap?
+        else
-              owner.name
+          tap = Tab.for_keg(owner).tap
-            else
+          f = if tap.nil? || tap.core_tap?
-              "#{tap}/#{owner.name}"
+            owner.name
-            end
+          else
-            @brewed_dylibs[f] << dylib
+            "#{tap}/#{owner.name}"
-          end
+          end
+          @brewed_dylibs[f] << dylib
         end
-        checked_dylibs << dylib
       end
     end
 
@@ -144,7 +123,10 @@ class LinkageChecker
       @indirect_deps, @undeclared_deps, @unnecessary_deps =
         check_undeclared_deps
     end
-    store_dylibs!
+
+    return unless keg_files_dylibs_was_empty
+
+    store&.update!(keg_files_dylibs: keg_files_dylibs)
   end
 
   def check_undeclared_deps
@@ -243,33 +225,4 @@ class LinkageChecker
   rescue FormulaUnavailableError
     opoo "Formula unavailable: #{keg.name}"
   end
-
-  # Helper function to reset dylib values
-  def reset_dylibs!
-    store&.flush_cache!
-    @system_dylibs    = Set.new
-    @broken_dylibs    = Set.new
-    @variable_dylibs  = Set.new
-    @brewed_dylibs    = Hash.new { |h, k| h[k] = Set.new }
-    @reverse_links    = Hash.new { |h, k| h[k] = Set.new }
-    @broken_deps      = Hash.new { |h, k| h[k] = [] }
-    @indirect_deps    = []
-    @undeclared_deps  = []
-    @unnecessary_deps = []
-  end
-
-  # Updates data store with package path values
-  def store_dylibs!
-    store&.update!(
-      system_dylibs: system_dylibs,
-      variable_dylibs: variable_dylibs,
-      broken_dylibs: broken_dylibs,
-      indirect_deps: indirect_deps,
-      broken_deps: broken_deps,
-      undeclared_deps: undeclared_deps,
-      unnecessary_deps: unnecessary_deps,
-      brewed_dylibs: brewed_dylibs,
-      reverse_links: reverse_links,
-    )
-  end
 end

Library/Homebrew/test/linkage_cache_store_spec.rb

@@ -32,20 +32,13 @@ describe LinkageCacheStore do
     context "a `value` is a `Hash`" do
       it "sets the cache for the `keg_name`" do
         expect(database).to receive(:set).with(keg_name, anything)
-        subject.update!(a_value: { key: ["value"] })
+        subject.update!(keg_files_dylibs: { key: ["value"] })
       end
     end
 
-    context "a `value` is an `Array`" do
+    context "a `value` is not a `Hash`" do
-      it "sets the cache for the `keg_name`" do
+      it "raises a `TypeError` if a `value` is not a `Hash`" do
-        expect(database).to receive(:set).with(keg_name, anything)
+        expect { subject.update!(a_value: ["value"]) }.to raise_error(TypeError)
-        subject.update!(a_value: ["value"])
-      end
-    end
-
-    context "a `value` is not an `Array` or `Hash`" do
-      it "raises a `TypeError` if a `value` is not an `Array` or `Hash`" do
-        expect { subject.update!(key: 1) }.to raise_error(TypeError)
       end
     end
   end
@@ -64,21 +57,11 @@ describe LinkageCacheStore do
       end
 
       it "returns a `Hash` of values" do
-        expect(subject.fetch_type(:brewed_dylibs)).to be_an_instance_of(Hash)
+        expect(subject.fetch_type(:keg_files_dylibs)).to be_an_instance_of(Hash)
       end
     end
 
-    context "`ARRAY_LINKAGE_TYPES.include?(type)`" do
+    context "`type` not in `HASH_LINKAGE_TYPES`" do
-      before(:each) do
-        expect(database).to receive(:get).with(keg_name).and_return(nil)
-      end
-
-      it "returns an `Array` of values" do
-        expect(subject.fetch_type(:system_dylibs)).to be_an_instance_of(Array)
-      end
-    end
-
-    context "`type` not in `HASH_LINKAGE_TYPES` or `ARRAY_LINKAGE_TYPES`" do
       it "raises a `TypeError` if the `type` is not supported" do
         expect { subject.fetch_type(:bad_type) }.to raise_error(TypeError)
       end