Speed up keg installation with fewer code-signing calls

[scpeters]

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

---

I noticed some formulae in the osrf/simulation tap starting taking much longer to install on macOS Intel processors after #15903 was merged. That only adds a call to codesign --verify for files, which quickly returns for unsigned files, but this code-signing method is called within loops over every library that each installed dylib links against, so it is called many times. I noticed on my machine that the osrf/simulation/gz-sim8 formula bottle pouring time has increased from 1 minute to over 6 minutes.

I believe that the methods in os/mac/keg are only called from two methods in extend/os/mac/keg_relocate. This pull request removes the calls to codesign_patched_binary from os/mac/keg and replaces that with individual calls to that method at the end of each loop in the relocate_dynamic_linkage and fix_dynamic_linkage methods in extend/os/mac/keg_relocate, which reduces bottle pouring times back to the times I was seeing before #15903 was merged.

If we want to retain the current behavior of the methods in os/mac/keg, we could add to these methods an optional codesign parameter that defaults to true but explicitly pass false to those methods from relocate_dynamic_linkage and fix_dynamic_linkage (see master...scpeters:keg_codesign_fewer_times). Please let me know if that approach is preferred, and we can use that other branch instead, but I started with this one because it is simpler.

[MikeMcQuaid]

Thanks @scpeters! Makes sense to me but would love one more ✅ from one of the folks I requested before this is merged.

[Bo98]

LGTM. Though given we don't necessarily need to codesign every file (e.g. if no modifications was made) and this change will start doing that, I reckon it might be worth doing a @needs_codesigning = true in the methods that called codesign before and then do codesign_patched_binary(file) if @needs_codesigning. This should be even quicker in certain cases and also reduce the chance of any breakages.

[woodruffw]

LGTM as well -- agreed with @Bo98's comment about not needing codesigning on every file, but IMO that could be a further incremental improvement rather than necessary to this PR 🙂

[scpeters]

LGTM. Though given we don't necessarily need to codesign every file (e.g. if no modifications was made) and this change will start doing that, I reckon it might be worth doing a @needs_codesigning = true in the methods that called codesign before and then do codesign_patched_binary(file) if @needs_codesigning. This should be even quicker in certain cases and also reduce the chance of any breakages.

Good call, you're right that this will call codesign on files that it wasn't called on before.

Small implementation question: if I'm not mistaken, @needs_codesigning would be an instance variable, but I think the logic depends on the file argument passed to the methods in os/mac/keg, so I think it would be more appropriate to pass needs_codesigning by reference to the method. Does that make sense?

[Bo98]

Small implementation question: if I'm not mistaken, @needs_codesigning would be an instance variable, but I think the logic depends on the file argument passed to the methods in os/mac/keg, so I think it would be more appropriate to pass needs_codesigning by reference to the method. Does that make sense?

Ah yes sorry. Alternative could be to return true/false depending on whether the file was modified or not and do stuff like:

        needs_codesigning = false

        if file.dylib?
          # ...
          needs_codesigning ||= change_dylib_id(id, file)
        end

        each_linkage_for(file, # ...
          # ...
          needs_codesigning ||= change_install_name(old_name, new_name, file) if new_name
        end

        each_linkage_for(file, # ...
          # ...
          needs_codesigning ||= change_rpath(old_name, new_name, file) if new_name
        end

        codesign_patched_binary(file) if needs_codesigning

[scpeters]

Ah yes sorry. Alternative could be to return true/false depending on whether the file was modified or not and do stuff like:

Thanks, I like this so I implemented it and force-pushed to b3304aa, then realized I hadn't run brew typecheck and then fixed that in 9235aa2. I did have to add boolean return values in change_rpath in extend/os/linux/keg_relocate to satisfy the type checker

[apainintheneck]

I'm pretty sure that the pattern of needs_codesigning ||= method_call(args) won't work the same way as before. The ||= won't evaluate the method on the right side of the expression if needs_codesigning is truthy.

irb(main):015:0> val = nil
=> nil
irb(main):016:0> val ||= (puts :print; :first_value)
print
=> :first_value
irb(main):017:0> val ||= (puts :print; :second_value)
=> :first_value

Off the top of my head, you could do.

needs_codesigning = method_call(args) || needs_codesigning

needs_codesigning = []
needs_codesigning << method_call(args)
...
if needs_codesigning.any?
...

[apainintheneck]

Looking at this a bit more my previous comment needs to be addressed before this is ready to merge.

[Bo98]

Sorry, yeah I oversimplified it. Idea in my head was

modified = change_dylib_id(...)
need_codesigning ||= modified

Array also works too though.

[apainintheneck]

I doubt the loops go on that long but probably better to avoid arrays just in case. @Bo98's solution above should work well.

[scpeters]

modified = change_dylib_id(...)
need_codesigning ||= modified

applied in 27901bc

and wow, satisfying typecheck with boolean types was very confusing

[scpeters]

I just squashed and rebased on master

[MikeMcQuaid]

Looks good to me, thanks again @scpeters!