Move Sorbet gems into an optional group

[Bo98]

• 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?

---

Whenever a new macOS version is released, all operations which invoke Homebrew.install_bundler_gems! are non-functional until Sorbet releases a gem compatible with the new macOS version.

This PR limits this impact by only installing Sorbet gems when brew typecheck or brew tests is run. This means that other commands like brew audit, brew style, brew test etc can still be run - enabling formula development to happen.

This also covers unsupported archs. While not officially supported by us, this PR will allow most developer commands to work again on those systems.

The list of groups installed is remembered. So if you run brew typecheck once, then it'll keep Sorbet installed and updated when you run brew audit, for example.

[BrewTestBot]

Review period will end on 2021-05-13 at 15:25:19 UTC.

[MikeMcQuaid]

Makes sense to me! My only worry is that we end up uninstalling these in some circumstances due to BUNDLE_CLEAN.

[Bo98]

In which scenarios would that happen? If it's on bundle install then I store the gem groups installed via Homebrew::Settings.write(:gemgroups, ...) and retrieve it again when Homebrew.install_bundler_gems! is called.

The groups passed into Homebrew.install_bundler_gems! append to this list (though there's only one possible value at the moment). It's possible to clear this with a brew install-bundler-gems --groups= if desired.

[MikeMcQuaid]

In which scenarios would that happen? If it's on bundle install then I store the gem groups installed via Homebrew::Settings.write(:gemgroups, ...) and retrieve it again when Homebrew.install_bundler_gems! is called.

Sounds like: no scenarios 🤞🏻.

✅ 🚢

[Bo98]

Using Homebrew::Settings here is actually a bit of a pain here.

Homebrew.install_bundler_gems! is invoked very early in the loading process when HOMEBREW_BOOTSNAP or HOMEBREW_SORBET_RUNTIME is set. This means that the vast majority of Homebrew isn't loaded at this point.

Challenges are:

• Sorbet type signatures.
  • In the case of HOMEBREW_SORBET_RUNTIME, Sorbet might not be installed yet.
  • In the HOMEBREW_BOOTSNAP case, even the stubs haven't been loaded yet.
  • Solution would be to move the signatures to RBI files, which makes sense but has a cascading effect - the Settings module relies on system_command (though this could be changed to not use that) and also relies on config.rb which loads extend/git_repository, all of which also use type signatures.
• The use of HOMEBREW_REPOSITORY. This env isn't always set in all cases. Running brew style with HOMEBREW_SORBET_RUNTIME or HOMEBREW_BOOTSNAP will invoke Homebrew.install_bundler_gems!, but this env isn't set so we need to figure out the repository through other methods.

Basically Homebrew::Settings probably needs to be stripped down to basically pure Ruby with ideally minimal to no require lines or other external help.

[MikeMcQuaid]

Using Homebrew::Settings here is actually a bit of a pain here.

I'd pass on using it given the above. I'm wondering if a much simpler solution would be better e.g. making the Gemfile conditional on the OS version/arch?

[BrewTestBot]

Review period ended.

[Bo98]

Ok so I played around with this just to experiment with what approach might be best.

I learned that there were parts of utils/gems that already required env variables, so I did the following:

• Moved config earlier in the load cycle. This make sure the brew environment is set up and creates the necessary constants.
  • To do this I have a startup file which is load_path + extras like Bootstrap and config loading. This allows Rubocop to still work outside the Homebrew environment by just using load_path (Homebrew.install_bundler_gems! is already not invoked nor supported in this scenario).
  • This unveiled a couple bugs - like the use of HOMEBREW_LIBRARY env in Utils.setup_gem_environment! - which will not exist when running under Rubocop (but HOMEBREW_LIBRARY_PATH will since that's not reliant on an env).
• Restricted settings to use Kernel.system (with Ruby 2.6's own exception throwing feature - this didn't exist when we first created safe_system) and Utils.popen_read. I don't think I've regressed anything here but I realise this takes a different direction from recent changes so have a look and see if it matches what you expect.
• Moved a limited amount of type signatures to RBI files - this is restricted to things that need to load before Sorbet (we do so already for utils/gems for similar reasons). extend/git_repository is the main change here, which is a little annoying since it is only used for one constant and those methods don't actually matter at the gem loading stage.
• Fixed errors (already present before this PR but only revealed in tests by the above changes) when running rubocop outside the brew environment but with either HOMEBREW_BOOTSNAP or HOMEBREW_SORBET_RUNTIME set.

It seems like a bit of moving around just to get something not too important to work, but since doing this revealed a couple bugs then I'm leaning towards this being the better approach to make.

However, if you disagree I'm happy to just do this:

making the Gemfile conditional on the OS version/arch?

[MikeMcQuaid]

However, if you disagree I'm happy to just do this:

making the Gemfile conditional on the OS version/arch?

I'm not sure I feel super strongly in one direction or another but it might be worth playing around with that (time boxed, please don't let me waste too much more of your time) in another PR so we can compare the two. The amount of changes required here just to not install Sorbet are starting to feel a little heavy (but perhaps for the best, hard to tell without comparing to another approach)?

Feel free to push back on this or any other requests, too.

[MikeMcQuaid]

What happens if you add this back? Does it error-out in a sufficiently easy to understand version? I wonder if it's worth having a comment here noting the .rbi file?

[Bo98]

It errors because require "sorbet-runtime" or require "sorbet-runtime-stub" hasn't been run yet so T doesn't exist.

[MikeMcQuaid]

Yeh, I figured that's why it errored but I was wondering how it errored i.e. if the messaging is obvious enough that people will understand what they've done wrong here.

[Bo98]

Traceback (most recent call last):
	5: from /usr/local/Homebrew/Library/Homebrew/brew.rb:31:in `<main>'
	4: from /usr/local/Homebrew/Library/Homebrew/brew.rb:31:in `require_relative'
	3: from /usr/local/Homebrew/Library/Homebrew/global.rb:75:in `<top (required)>'
	2: from /usr/local/Homebrew/Library/Homebrew/global.rb:75:in `require'
	1: from /usr/local/Homebrew/Library/Homebrew/config.rb:14:in `<top (required)>'
/usr/local/Homebrew/Library/Homebrew/config.rb:15:in `<module:EnvVar>': uninitialized constant EnvVar::T (NameError)

[Bo98]

Everything works fine for me locally.

undefined method `error_message_for_obj' for nil:NilClass

Not sure why not in the CI.

[MikeMcQuaid]

Everything works fine for me locally.

undefined method `error_message_for_obj' for nil:NilClass

Not sure why not in the CI.

@Bo98 perhaps try to unset HOMEBREW_DEVELOPER locally and start from a clean state with git clean -xdf in your brew --repo?

[Bo98]

Still can't reproduce. I suspect it is some parallelism or spec ordering issue.

Manually trawling through the code, this line could be problematic:

brew/Library/Homebrew/style.rb

Line 88 in 33e71c9

require "rubocops"

since it will trigger the load path and Sorbet to be reinitialised (rubocops.rb is designed to be run standalone).

[Bo98]

Apart from macOS coverage being stuck as "queued for processing" and thus showing an overall -5.61% (which is probably a serverside issue and not to do with my changes), this seems to work now.

[Bo98]

Lastly, this doesn't do anything anymore:

https://github.com/Homebrew/brew/blob/f7d54023c34becec896ce7cb16230114b1644109/Library/Homebrew/test/support/lib/startup/config.rb#L52-L59

because this is now run before default prefix in global.rb is established and thus won't be overriding anything.

Do we need it? Only difference I noticed is a different execution path in dev-cmd/bottle, which doesn't break existing tests because we only cover relocatable stuff.
If it's desired to keep, I suppose we can have a separate default_prefix.rb or something.

[Bo98]

If it's desired to keep, I suppose we can have a separate default_prefix.rb or something.

Did this to be safe and to limit behavioural changes in this PR.

[MikeMcQuaid]

Looks good, thanks @Bo98!