extend/ENV/super: allow O{1,0} to accept a block

[carlocab]

• 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 makes ENV.O{1,0} behave like ENV.deparallelize.

This should also allow us to build libgcrypt's jitter entropy collector,
which we currently disable because it interacts poorly with our compiler
shims. See #11201 for a previous attempt at this.

[BrewTestBot]

Review period will end on 2021-07-07 at 23:59:03 UTC.

[carlocab]

Not sure why yield doesn't work, but I don't really get Ruby metaprogramming.

[MikeMcQuaid]

Makes sense to me. I'd love to see more of these methods take block forms to be used in a more finely-grained manner (and perhaps even deprecate the non-block for of deparallelize). with_env may be a cleaner solution to this?

[carlocab]

with_env may be a cleaner solution to this?

with_env would work if we change these methods to only accept blocks. I don't see a way of making it work if you want the environment change to persist until the end of the install method, but maybe I just don't understand with_env.

[MikeMcQuaid]

with_env would work if we change these methods to only accept blocks. I don't see a way of making it work if you want the environment change to persist until the end of the install method, but maybe I just don't understand with_env.

I was thinking maybe just for the block case?

[carlocab]

So you mean something like

  %w[O1 O0].each do |opt|
    define_method opt do |&block|
      if block
        with_env(HOMEBREW_OPTIMIZATION_LEVEL: opt) { block.call }
      else
        send(:[]=, "HOMEBREW_OPTIMIZATION_LEVEL", opt)
      end
    end
  end

?

[BrewTestBot]

Review period ended.

[MikeMcQuaid]

@carlocab Yeh, that looks nice to me 👍🏻

[carlocab]

Oops. Sorbet really does not like the new version:

❯ brew typecheck
extend/ENV/super.rb:343: This code is unreachable https://srb.help/7006
     343 |        with_env(HOMEBREW_OPTIMIZATION_LEVEL: opt) { block.call }
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    extend/ENV/super.rb:342: This condition was always falsy (NilClass)
     342 |      if block
                   ^^^^^
  Got NilClass originating from:
    extend/ENV/super.rb:341:
     341 |    define_method opt do |&block|
                                     ^^^^^

[MikeMcQuaid]

@carlocab Given there's now only two of these methods: I'd suggest avoiding define_method and defining them normally.

[carlocab]

Ah, just saw your comment. Convincing Sorbet that block can occasionally not be nil isn't too much more code. Would you still rather these two methods be defined separately?

[MikeMcQuaid]

@carlocab Sorry, yeh, I think it'd be nicer.

[carlocab]

Sure, that's fine. I've pushed the changes.

[MikeMcQuaid]

Can you add a comment here explaining why? Thanks!

[MikeMcQuaid]

Thanks, sorry for all the back and forth 🙇🏻

[carlocab]

The apology is appreciated but very much unnecessary 🙂