-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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.
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
Verify more constants are not loaded at startup #18012
Conversation
Library/Homebrew/test/support/helper/cmd/brew-verify-undefined.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea, thanks for this @apainintheneck!
Loading different constants can be tricky with all the requires in Homebrew so we want to strengthen the existing checks to make sure that things are not getting required when there is a performance penalty. This expands the existing check to include more constants beyond `Formula` that we don't expect to be defined and that pull in a lot of other dependencies.
dd52b78
to
f6a6979
Compare
Formula | ||
Formulary | ||
Homebrew::API | ||
Tap |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👏🏻 😍
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?Loading different constants can be tricky with all the requires in Homebrew so we want to strengthen the existing checks to make sure that things are not getting required when there is a performance penalty. This expands the existing check to include more constants beyond
Formula
that we don't expect to be defined and that pull in a lot of other dependencies.Note: This currently fails the
Tap
andHomebrew::API
checks but those should resolve themselves after #18010 gets merged in.Note 2: This PR conflicts with #18008 so we should wait until that one gets merged in.