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

Replace sorbet-runtime-stub with sorbet-runtime #13566

Merged
merged 2 commits into from Jul 21, 2022

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Jul 16, 2022

Resolves #11521.

This does have more overhead than the stub but I'm not convinced it's noticeable. I've done a monkey patch disabling some more common parts of runtime checking though to be on the safer side.

If the overhead is not much, we could expxand this in the future by not doing these monkey patches and doing things such as a one-pass basic evaluation of sig blocks to allow for example abstract classes to work if we want to make use of them. We don't need to commit to full level runtime checking to do this - there's different levels on the spectrum.

Should be able to replace #13563 if the user does brew install-bundler-gems --groups= first to clear previous settings. I would still however like information to fix the install error some people have since it'll still be an issue for brew typecheck. I cannot reproduce the issue myself.

@Bo98 Bo98 added the critical Critical change which should be shipped as soon as possible. label Jul 16, 2022
@BrewTestBot
Copy link
Member

BrewTestBot commented Jul 16, 2022

Review period ended.

@Bo98 Bo98 force-pushed the sorbet-runtime branch 2 times, most recently from bb0d0f9 to c2ed256 Compare July 16, 2022 02:48
@Bo98
Copy link
Member Author

Bo98 commented Jul 16, 2022

brew install-bundler-gems doesn't uninstall gems properly unless paired with updating/installing any other gem (because bundle check only fails if a declared gem is not installed - not if an installed gem is not declared), which is causing brew doctor to fail in CI. I feel like we've had that issue before.

Cop out fix is to roll it in with the next Sorbet version bump but we should probably fix it properly.

@Bo98 Bo98 force-pushed the sorbet-runtime branch 2 times, most recently from 66b340b to e2109d5 Compare July 16, 2022 04:23
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

One comment about more comments but otherwise seems fine thanks 👍🏻

Library/Homebrew/standalone/sorbet.rb Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member

Cop out fix is to roll it in with the next Sorbet version bump but we should probably fix it properly.

Yeh, that'd be good.

because bundle check only fails if a declared gem is not installed - not if an installed gem is not declared

Maybe needs an explicit bundle cleanup or bundle install --cleanup (neither are correct syntax, I think) to handle these cases?

@Bo98
Copy link
Member Author

Bo98 commented Jul 19, 2022

Maybe needs an explicit bundle cleanup or bundle install --cleanup (neither are correct syntax, I think) to handle these cases?

bundle cleanup seems to just make #5798 resurface.

@Bo98
Copy link
Member Author

Bo98 commented Jul 19, 2022

Ah looks like Bundler might have finally fixed the extensions directory mess in the standalone setup.rb we have: rubygems/rubygems@a9dae93. Though that will require using a newer version of Bundler.

Not really relevant to this PR but good to know nonetheless.

Edit: ok it maybe doesn't cover the platform part yet but it's a start.

@Bo98
Copy link
Member Author

Bo98 commented Jul 19, 2022

Pushed a Sorbet update here since I don't really know how to handle bundle cleanup without breaking everything. This should be sufficient to pass CI.

@Bo98 Bo98 merged commit 676ab51 into Homebrew:master Jul 21, 2022
@Bo98 Bo98 deleted the sorbet-runtime branch July 21, 2022 19:22
@github-actions github-actions bot added the outdated PR was locked due to age label Aug 21, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sorbet-runtime-stub is deprecated
3 participants