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.

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

Remove problematic :final from Requirement sigs #18814

Merged
merged 1 commit into from
Nov 25, 2024
Merged

Conversation

dduugg
Copy link
Member

@dduugg dduugg commented Nov 24, 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?

Partial revert of https://github.com/Homebrew/brew/pull/16640/files#diff-b75efe9741aed6830d257b35e55eb243a38c7b187774b42cf26be6deef8990db

We only enabled :final enforcement two days ago, which has caused GH Action failures, e.g. https://github.com/Homebrew/homebrew-core/actions/runs/11991937306/job/33431223236?pr=198784#step:3:322

Copy link
Contributor

@apainintheneck apainintheneck left a comment

Choose a reason for hiding this comment

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

I can reproduce the failure locally with brew audit --formula hyperestraier --git --skip-style so reverting makes sense. It'd also be interesting to find out why this only affects some formulae. I tried a few other formulae locally and they succeeded.

@dduugg
Copy link
Member Author

dduugg commented Nov 24, 2024

Thanks! Used that to discover that it also fails with :final sig on fatal?. (The remaining :final instance method here, modify_build_environment, doesn't seem to present a problem, at least with this specific formula.)

I can't immediately identify the problem, I suspect it may be a sorbet issue, it will have to wait until I have the bandwidth to dive in though.

@dduugg dduugg changed the title Remove :final from Requirement#satisfied? sig Remove problematic :final from Requirement sigs Nov 24, 2024
@apainintheneck
Copy link
Contributor

Here's the full backtrace for the curious: https://gist.github.com/apainintheneck/0b8154b50940b80f3114e8969cccc0e0

I can't make heads or tails of it.

@Bo98
Copy link
Member

Bo98 commented Nov 25, 2024

It'll be from old formulae: Homebrew/homebrew-core@60003ee#diff-7af2303fb6e63cb2941f96909dcf3eef5cd35a6eab39c1691e4ec06f1d6e3f7a.

Errors from basic loading/parsing of old formula must result in a FormulaUnreadableError, i.e. be in this list:

rescue NameError, ArgumentError, ScriptError, MethodDeprecatedError, MacOSVersion::Error => e

Unfortunately, Sorbet does not map this to an error like they do for TypeError: https://github.com/sorbet/sorbet/blob/370510182162529f01e9f2323e37c7b27131276e/gems/sorbet-runtime/lib/types/private/methods/_methods.rb#L184.

@Bo98
Copy link
Member

Bo98 commented Nov 25, 2024

In any case: while the old syntax is not really supported, some third-party taps do use still use it (because we never really deprecated it) and we probably shouldn't make those formulae completely unreadable: https://github.com/brewsci/homebrew-science/blob/5d14d2fda3934c0eb8243c1cbdb9fb4621ec110b/Formula/lp_solve.rb#L6. Or at least not in a patch release.

@Bo98 Bo98 merged commit a542ef9 into master Nov 25, 2024
28 checks passed
@Bo98 Bo98 deleted the revert-final-sig branch November 25, 2024 03:36
def fatal? = false
end
end
.to raise_error(RuntimeError, /\AThe method `fatal\?` on #{described_class.name} was declared as final/)
Copy link
Member Author

Choose a reason for hiding this comment

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

It's interesting that I added this test before I discovered that :final checks had to be explicitly enabled

So it seems Sorbet is aware of being run in tests, and enables :final in that context, which was counterproductive in our case…

@MikeMcQuaid
Copy link
Member

Thanks for fix @dduugg!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants