formula_installer: prevent MaximumMacOSRequirement leakage

[DomT4]

• 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 successfully run brew tests with your changes locally?

---

Read the discussion in Homebrew/homebrew-core#3703. If you have a better idea, please file a competing PR. I'm sick to death of discussion.

[ilovezfs]

[MikeMcQuaid]

Pushed and made it more general in cc752e9. I checked and this approach is safe: if a requirement is for a dependent that's already installed and that dependency is not using a default_formula (which would have already been converted from a Requirement to Dependency at this stage) and there's no reason for the Requirement to take effect.

[UniqMartin]

If you have a better idea, please file a competing PR. I'm sick to death of discussion.

I'm not sure what exactly triggered this, but this certainly doesn't read like an invitation for constructive criticism (or any criticism at all). 😢

[MikeMcQuaid]

I'm not sure what exactly triggered this, but this certainly doesn't read like an invitation for constructive criticism (or any criticism at all). 😢

I think @DomT4's point (which I broadly agree with) is that an under-discussed bad solution that fixes a widespread user problem is much better than a perfect but unimplemented solution. There's a lot of stuff where honestly it's as quick to create a PR as write a comment.

[xu-cheng]

In my opinion this PR along with the additional commit basically starts to treat all requirements as build only. This is definitely incorrect as can be shown in following example.

$ brew cat foo
class Foo < Formula
  url "file:///usr/local/bin/brew"
  version "1.0"

  depends_on "baz"

  def install
    system Formula["baz"].opt_bin/"baz"
  end
end
$ brew cat baz
class Baz < Formula
  url "file:///usr/local/bin/brew"
  version "1.0"

  depends_on :python3

  def install
    (bin/"baz").write <<-EOS.undent
      #!/usr/bin/env python3
      print("hello world")
    EOS
  end
end
$ brew install baz
==> Using the sandbox
==> Downloading file:///usr/local/bin/brew
######################################################################## 100.0%
Warning: Cannot verify integrity of baz-1.0
A checksum was not provided for this resource
For your reference the SHA256 is: 0b16b7f7e219329a82a3d5d1f485d3ee0944995c9686f686505ca1dce304dd10
🍺  /usr/local/Cellar/baz/1.0: 2 files, 680B, built in 2 seconds
$ brew uninstall python3
Uninstalling /usr/local/Cellar/python3/3.5.2_1... (3,574 files, 54.3M)
$ brew install foo
==> Using the sandbox
==> Downloading file:///usr/local/bin/brew
Already downloaded: /Users/xucheng/Library/Caches/Homebrew/foo-1.0
Warning: Cannot verify integrity of foo-1.0
A checksum was not provided for this resource
For your reference the SHA256 is: 0b16b7f7e219329a82a3d5d1f485d3ee0944995c9686f686505ca1dce304dd10
==> /usr/local/opt/baz/bin/baz
Last 15 lines from /Users/xucheng/Library/Logs/Homebrew/foo/01.baz:
2016-08-09 19:24:27 +0800

/usr/local/opt/baz/bin/baz

env: python3: No such file or directory
READ THIS: https://git.io/brew-troubleshooting
If reporting this issue please do so at (not Homebrew/brew):
  https://github.com/Homebrew/homebrew-core/issues

For the problem in Homebrew/homebrew-core#3703, shouldn't it be solved by depends_on MaximumMacOSRequirement => [:el_capitan, :build]

[ilovezfs]

depends_on MaximumMacOSRequirement => [:el_capitan, :build]

Yes, this worked.

[MikeMcQuaid]

@xu-cheng: I quote:

If you have a better idea, please file a competing PR. I'm sick to death of discussion.

For the problem in Homebrew/homebrew-core#3703, shouldn't it be solved by depends_on MaximumMacOSRequirement => [:el_capitan, :build]

No, it should not. The "correct" solution would be to ensure that if a devel formula is installed it does not check for requirements or dependencies inside the stable spec. Another possible solution just for this case would be to make MaximumMacOSRequirement so it always has a :build tag.

[ilovezfs]

The "correct" solution would be to ensure that if a devel formula is installed it does not check for requirements or dependencies inside the stable spec.

See Homebrew/homebrew-core#3665 (comment).

[MikeMcQuaid]

@ilovezfs Your PR looks weird, it's like it's embedded in a comment

[ilovezfs]

@MikeMcQuaid that's because that code is actually wrong as mentioned lower down in the same issue, and no I do not intend to fix it or open a PR.

[MikeMcQuaid]

I do not intend to fix it or open a PR.

It seems this is the default position of Homebrew maintainers now ¯\_(ツ)_/¯

[ilovezfs]

It seems this is the default position of Homebrew maintainers now ¯_(ツ)_/¯

There's probably more than a few overlapping reasons for that.

[MikeMcQuaid]

There's probably more than a few overlapping reasons for that.

Not here but I'd genuinely like to listen what they are from your perspective. Can you send me an email or Slack message?

[DomT4]

I'm not sure what exactly triggered this, but this certainly doesn't read like an invitation for constructive criticism (or any criticism at all).

My wording was blunt because I was tired, amongst other less shiny things Homebrew has made me feel all too often over the last 6 months, of which I share a chunk of the blame & that blame being the biggest reason I told Mike last night I'll happily walk away from the project if it brings other maintainers back into the fold.

More gently worded, which is how I should have written it, I was just saying if people felt like this was an awful idea & they had a better one, could they please implement it rather than spending their time telling me why I was doing something awful they could never accept.

I always welcome constructive criticism on anything I submit (as hopefully I've showed by having accepted quite a bit of that advice over time), it helps me be less awful at all of this, but I think over the last 6 months we've become prone, as a team, to ramping up criticism to a level where the OP is made to feel exhausted when a PR finally ships because we chased some dream of perfection.

[ilovezfs]

@xu-cheng actually my test was wrong, depends_on MaximumMacOSRequirement => [:el_capitan, :build] doesn't seem to work.

[UniqMartin]

@DomT4 Thanks for the reply! ❤️ I now have a much better understanding of the circumstances than when I wrote my comment. I agree that there's room for improvement in the team and how we approach things and wrangle PRs, though I'm not sure how to best approach this …