Skip to content
New issue

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

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

Already on GitHub? # to your account

Add utils/backtrace requires #17762

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

samford
Copy link
Member

@samford samford commented Jul 15, 2024

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This is primarily intended to resolve the uninitialized constant Utils::Backtrace error in formula_versions.rb:60 (e.g., https://github.com/Homebrew/homebrew-core/actions/runs/9942660636/job/27464770982?pr=177398, from Homebrew/homebrew-core#177398) but I expanded it to try to cover all existing usage of Utils::Backtrace.

Per Rylan's comment below, I've followed the existing pattern, where utils/backtrace is required in the context of where it's used. Many of these cases use Backtrace in a conditional manner, so I've tried to ensure that the require follows suit.

@Moisan Moisan mentioned this pull request Jul 15, 2024
7 tasks
Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

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

I'm not opposed to this, but it does feel like this shouldn't go in formula.rb if there is no direct call within this file.

It looks like it's used in 12 files (not including tests and other sorbet stuff). Maybe it's better to add it to those directly instead? I'm not sure how much of a performance difference it makes.

For context, I did this last time (not claiming this is the best way though): https://github.com/Homebrew/brew/pull/17738/files

@samford samford force-pushed the formula-require-utils-backtrace branch from 31694f1 to cb61340 Compare July 15, 2024 18:23
@samford samford marked this pull request as draft July 15, 2024 18:58
@samford samford force-pushed the formula-require-utils-backtrace branch from cb61340 to 679f84f Compare July 15, 2024 19:52
@samford samford marked this pull request as ready for review July 15, 2024 19:52
@samford
Copy link
Member Author

samford commented Jul 15, 2024

I updated this to cover all the instances that I found where Utils::Backtrace is used but there's no require "utils/backtrace". If it's already available in any of these contexts, just let me know and I can pare this back accordingly.

I generally used the approach that @Rylan12 linked to, where utils/backtrace is required in the context of where it's used (instead of at the top of the file). When there were conditions around the use of Utils::Backtrace, I tried to have the require respect the same conditions (with the exception of odebug calls requiring a debug context).

One thing to note is that cask/audit.rb requires utils/backtrace at the top of the file. If you remove that and only require it where Utils::Backtrace is used, it leads to errors when running brew tests. There may be a way to resolve that issue but I figured it would be best to leave it alone for now.

I would appreciate more eyes on this, as I'm not sure if this is the best way to go about it all.

@samford samford force-pushed the formula-require-utils-backtrace branch from 679f84f to 79c7334 Compare July 15, 2024 20:13
@samford samford changed the title formula: require utils/backtrace Add utils/backtrace requires Jul 15, 2024
This is primarily intended to resolve the `uninitialized constant
Utils::Backtrace` error in `formula_versions.rb:60` but I expanded it
to try to cover all existing usage of `Utils::Backtrace`.

I've followed the existing pattern, where `utils/backtrace` is
required in the context of where it's used. Many of these cases use
`Backtrace` in a conditional manner, so I've tried to ensure that the
`require` follows suit.
@samford samford force-pushed the formula-require-utils-backtrace branch from 79c7334 to 11d6785 Compare July 15, 2024 21:48
Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @samford!

@Rylan12 Rylan12 merged commit 9538f42 into Homebrew:master Jul 16, 2024
24 checks passed
@samford samford deleted the formula-require-utils-backtrace branch July 16, 2024 16:50
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants