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

Verify more constants are not loaded at startup #18012

Merged
merged 1 commit into from
Aug 15, 2024
Merged

Verify more constants are not loaded at startup #18012

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Library/Homebrew/test/global_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
# frozen_string_literal: true

RSpec.describe Homebrew, :integration_test do
it "does not invoke `require \"formula\"` at startup" do
expect { brew "verify-formula-undefined" }
it "does not require slow dependencies at startup" do
expect { brew "verify-undefined" }
.to not_to_output.to_stdout
.and not_to_output.to_stderr
.and be_a_success
Expand Down
23 changes: 0 additions & 23 deletions Library/Homebrew/test/support/helper/cmd/brew-verify-formula-undefined.rb
View file
Open in desktop

This file was deleted.

40 changes: 40 additions & 0 deletions Library/Homebrew/test/support/helper/cmd/brew-verify-undefined.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# typed: strict
# frozen_string_literal: true

require "cli/parser"

UNDEFINED_CONSTANTS = %w[
Cask::Cask
Formula
Formulary
Homebrew::API
Tap
Copy link
Member

Choose a reason for hiding this comment

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

Nice work @apainintheneck! Any other big constants you think we could add in here? I'm happy to take a look and suggest some if that's be helpful. My thinking would just be to add various require to global.rb and use brew prof to see which ones have the biggest impact on a brew ruby -e 'puts :foo' (or similar quick command that goes through Ruby-land) output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a few more that we don't want to be loaded at start up but they are required by some of the existing files in this list so it's not strictly necessary to add them here. I'm thinking of FormulaInstaller and Keg in this case. That being said the check here is really simple so there is essentially no cost to us expanding this list. Feel free to take a look and add any that seem mildly suspicious to the list. Homebrew::API is an example of a borderline one that I decided to add on a whim.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @apainintheneck. Inspired by your great work here: I've opened #18065 as a follow-up.

I feel pretty confident after this that I can stop asking people to not add them to global.rb as CI will now catch this instead. Couldn't have done it without your idea here, wonderful stuff 👏🏻 😍

].freeze

module Homebrew
module Cmd
class VerifyUndefined < AbstractCommand
end
end
end

parser = Homebrew::CLI::Parser.new(Homebrew::Cmd::VerifyUndefined) do
usage_banner <<~EOS
`verify-undefined`

Verifies that the following constants have not been defined
at startup to make sure that startup times stay consistent.

Contants:
#{UNDEFINED_CONSTANTS.join("\n")}
EOS
end

parser.parse

UNDEFINED_CONSTANTS.each do |constant_name|
Object.const_get(constant_name)
ofail "#{constant_name} should not be defined at startup"
rescue NameError
# We expect this to error as it should not be defined.
end