upgrade: check for already broken dependents

[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 tests with your changes locally?

---

After upgrading a formula, it currently checks for broken dependents after upgrading any outdated dependents. If there are no outdated dependents, it exits early and doesn't check for broken dependents. This adds an earlier check for already broken dependents so they can be fixed even if there are no outdated dependents. (sorry that's kind of a mouthful)

I duplicated the block that checks for broken dependents; let me know if it should have its own method instead.

I haven't profiled this, but I expect it will perform linkage checks for dependents more often.

[MikeMcQuaid]

I duplicated the block that checks for broken dependents; let me know if it should have its own method instead.

Yes, I think this would be desirable.

This adds an earlier check for already broken dependents so they can be fixed even if there are no outdated dependents.

So this fix, if I understand correctly, is to fix the case where they haven't been broken by this brew upgrade but may have done by e.g. a previous one or user action?

This behaviour (and possibly your previous PR) may also warrant being added to cmd/install.rb and cmd/reinstall.rb which also both perform some of these checks.

Just to call it out: it does seem potentially surprising that this would start reinstalling things that are unrelated to anything that's being upgraded. I think it's still probably the right move but might want to improve the messaging.

[scpeters]

I duplicated the block that checks for broken dependents; let me know if it should have its own method instead.

Yes, I think this would be desirable.

I moved the logic to a check_broken_dependents method in the same file in 24e7f55

This adds an earlier check for already broken dependents so they can be fixed even if there are no outdated dependents.

So this fix, if I understand correctly, is to fix the case where they haven't been broken by this brew upgrade but may have done by e.g. a previous one or user action?

That's correct

This behaviour (and possibly your previous PR) may also warrant being added to cmd/install.rb and cmd/reinstall.rb which also both perform some of these checks.

Just to call it out: it does seem potentially surprising that this would start reinstalling things that are unrelated to anything that's being upgraded. I think it's still probably the right move but might want to improve the messaging.

Yes, if we make this change we should improve the messaging. Alternatively, we could put this behavior in a separate command brew rebuild_broken_bottles or brew linkage --rebuild-broken-from-source

[MikeMcQuaid]

Let's try this out for now and see how people react. Thanks again @scpeters!

[scpeters]

thanks!