Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve performance of brew readall #16007

Merged
merged 1 commit into from
Sep 28, 2023
Merged

Improve performance of brew readall #16007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions Library/Homebrew/extend/os/mac/readall.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

module Readall
class << self
def valid_casks?(casks, os_name: nil, arch: Hardware::CPU.type)
def valid_casks?(tap, os_name: nil, arch: Hardware::CPU.type)
return true if os_name == :linux

current_macos_version = if os_name.is_a?(Symbol)
Expand All @@ -13,7 +13,7 @@
end

success = T.let(true, T::Boolean)
casks.each do |file|
tap.cask_files.each do |file|

Check warning on line 16 in Library/Homebrew/extend/os/mac/readall.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/extend/os/mac/readall.rb#L16 

Added line #L16 was not covered by tests
cask = Cask::CaskLoader.load(file)

# Fine to have missing URLs for unsupported macOS
Expand Down
7 changes: 6 additions & 1 deletion Library/Homebrew/formula.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2398,7 +2398,7 @@ def to_hash_with_variations

variations = {}

if path.exist? && (self.class.on_system_blocks_exist? || @on_system_blocks_exist)
if path.exist? && on_system_blocks_exist?
formula_contents = path.read
OnSystem::ALL_OS_ARCH_COMBINATIONS.each do |os, arch|
bottle_tag = Utils::Bottles::Tag.new(system: os, arch: arch)
Expand Down Expand Up @@ -2452,6 +2452,11 @@ def bottle_hash
hash
end

# @private
def on_system_blocks_exist?
self.class.on_system_blocks_exist? || @on_system_blocks_exist
end

# @private
def fetch(verify_download_integrity: true)
active_spec.fetch(verify_download_integrity: verify_download_integrity)
Expand Down
45 changes: 28 additions & 17 deletions Library/Homebrew/readall.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
# @api private
module Readall
class << self
include Cachable

private :cache

def valid_ruby_syntax?(ruby_files)
failed = T.let(false, T::Boolean)
ruby_files.each do |ruby_file|
Expand Down Expand Up @@ -39,19 +43,26 @@
!failed
end

def valid_formulae?(formulae, bottle_tag: nil)
success = T.let(true, T::Boolean)
formulae.each do |file|
base = Formulary.factory(file)
next if bottle_tag.blank? || !base.path.exist? || !base.class.on_system_blocks_exist?

formula_contents = base.path.read
def valid_formulae?(tap, bottle_tag: nil)
cache[:valid_formulae] ||= {}

Check warning on line 47 in Library/Homebrew/readall.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/readall.rb#L47 

Added line #L47 was not covered by tests

readall_namespace = Formulary.class_s("Readall#{bottle_tag.to_sym.capitalize}")
readall_formula_class = Formulary.load_formula(base.name, base.path, formula_contents, readall_namespace,
flags: base.class.build_flags, ignore_errors: true)
readall_formula_class.new(base.name, base.path, :stable,
alias_path: base.alias_path, force_bottle: base.force_bottle)
success = T.let(true, T::Boolean)
tap.formula_files.each do |file|
valid = cache[:valid_formulae][file]

Check warning on line 51 in Library/Homebrew/readall.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/readall.rb#L49-L51 

Added lines #L49 - L51 were not covered by tests
next if valid == true || valid&.include?(bottle_tag)

formula_name = file.basename(".rb").to_s
formula_contents = file.read(encoding: "UTF-8")

Check warning on line 55 in Library/Homebrew/readall.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/readall.rb#L54-L55 

Added lines #L54 - L55 were not covered by tests

readall_namespace = "ReadallNamespace"
readall_formula_class = Formulary.load_formula(formula_name, file, formula_contents, readall_namespace,

Check warning on line 58 in Library/Homebrew/readall.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/readall.rb#L57-L58 

Added lines #L57 - L58 were not covered by tests
flags: [], ignore_errors: false)
readall_formula = readall_formula_class.new(formula_name, file, :stable, tap: tap)
cache[:valid_formulae][file] = if readall_formula.on_system_blocks_exist?

Check warning on line 61 in Library/Homebrew/readall.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/readall.rb#L60-L61 

Added lines #L60 - L61 were not covered by tests
[bottle_tag, *cache[:valid_formulae][file]]
else
true
end
Comment on lines +61 to +65
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cache[:valid_formulae][file] = if readall_formula.on_system_blocks_exist?
[bottle_tag, *cache[:valid_formulae][file]]
else
true
end
unless readall_formula.on_system_blocks_exist?
cache[:valid_formulae][file] = true
end

I don't think we really need to track the case where a formula uses on system blocks. We only run this method once per os/arch combination anyway.

The main thing we're trying to do here is avoid reading formula without on system blocks multiple times (since they shouldn't change), right?

[2] brew(main)> CoreTap.instance.formula_files.count do |file|
[2] brew(main)*   Formulary.factory(file).on_system_blocks_exist?
[2] brew(main)* end
=> 880
[3] brew(main)> CoreTap.instance.formula_files.count
=> 6781

Isn't it possible to use the manual os and arch functions in formula definitions though like OS.mac?? Or maybe that's only limited to certain stanzas like install and test that aren't relevant for brew readall. To be fair this command was update recently to test against different os/arch combinations but it does change the behavior, right?

Copy link
Member Author

@Bo98 Bo98 Sep 16, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't actually allow on_system blocks inside install and test. We do however in caveats, which is read by to_hash.

Idea being it would catch something like:

def caveats
  on_linux do
    oopsie
  end

  "test"
end

Basically the hope is that brew generate-formula-api should never fail if brew readall passes, or at least 99.9% of the time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Do you think this sort of optimization would be useful for the cask repo as well at some point?

rescue Interrupt
raise
rescue Exception => e # rubocop:disable Lint/RescueException
Expand All @@ -62,7 +73,7 @@
success
end

def valid_casks?(_casks, os_name: nil, arch: nil)
def valid_casks?(_tap, os_name: nil, arch: nil)
true
end

Expand All @@ -75,16 +86,16 @@
end

if no_simulate
success = false unless valid_formulae?(tap.formula_files)
success = false unless valid_casks?(tap.cask_files)
success = false unless valid_formulae?(tap)
success = false unless valid_casks?(tap)
else
os_arch_combinations.each do |os, arch|
bottle_tag = Utils::Bottles::Tag.new(system: os, arch: arch)
next unless bottle_tag.valid_combination?

Homebrew::SimulateSystem.with os: os, arch: arch do
success = false unless valid_formulae?(tap.formula_files, bottle_tag: bottle_tag)
success = false unless valid_casks?(tap.cask_files, os_name: os, arch: arch)
success = false unless valid_formulae?(tap, bottle_tag: bottle_tag)
success = false unless valid_casks?(tap, os_name: os, arch: arch)
end
end
end
Expand Down
2 changes: 2 additions & 0 deletions Library/Homebrew/test/spec_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ def find_files
Tab.clear_cache
Dependency.clear_cache
Requirement.clear_cache
Readall.clear_cache if defined?(Readall)
FormulaInstaller.clear_attempted
FormulaInstaller.clear_installed
FormulaInstaller.clear_fetched
Expand Down Expand Up @@ -251,6 +252,7 @@ def find_files
Tab.clear_cache
Dependency.clear_cache
Requirement.clear_cache
Readall.clear_cache if defined?(Readall)

FileUtils.rm_rf [
*TEST_DIRECTORIES,
Expand Down