Add `depends_on :skip_linux` and `fails_with :linux` DSLs

jonchang commented 6 years ago

[x] Have you followed the guidelines in our Contributing document?
[x] Have you checked to ensure there aren't other open Pull Requests for the same change?
[x] 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.
[x] Have you successfully run brew style with your changes locally?
[x] Have you successfully run brew tests with your changes locally?

jonchang commented 6 years ago

To test, brew edit hello and add either depends_on :skip_linux or fails_with :linux

then,

$ brew info hello
hello: stable 2.10 (bottled)
Program providing model for GNU coding standards and practices
https://www.gnu.org/software/hello/
Conflicts with:
  camlistore (because both install `hello` binaries)
Not installed
From: https://github.com/Linuxbrew/homebrew-core/blob/master/Formula/hello.rb
==> Requirements
Required: works on Linux ✘

install message

$ brew install hello
hello: This formula is known not to work on Linux. A pull request to fix
it is most welcome! Run `brew edit FORMULA` and remove the line
`depends_on :skip_linux`. Run `brew install FORMULA`. Edit the
formula to fix any errors, and try installing again. When you're
successful, please open a pull request! See here for details:

https://github.com/Linuxbrew/homebrew-core/blob/master/CONTRIBUTING.md
Error: An unsatisfied requirement failed this build.

setting HOMEBREW_FORCE_LINUX

$ HOMEBREW_FORCE_LINUX=1 brew info hello
hello: stable 2.10 (bottled)
Program providing model for GNU coding standards and practices
https://www.gnu.org/software/hello/
Conflicts with:
  camlistore (because both install `hello` binaries)
Not installed
From: https://github.com/Linuxbrew/homebrew-core/blob/master/Formula/hello.rb
==> Requirements
Required: works on Linux ✔

Requirements don't have access to formula options, so I don't think there's a way to pass e.g. --force-linux without hacking at formula.rb or similar.

sjackman commented 6 years ago

I believe you can access args.include? "--force-linux" from within the requirement without access to the formula.

sjackman commented 6 years ago

A few small comments, otherwise looks great! Thanks, Jon!

jonchang commented 6 years ago

I believe you can access args.include? "--force-linux" from within the requirement without access to the formula.

Error: undefined local variable or method `args' for #<SkipLinuxRequirement: "skiplinux" []>

Poking around in pry I can access Homebrew.args but it's empty:

[1] pry(#<SkipLinuxRequirement>)> Homebrew.args
=> #<OpenStruct>

sjackman commented 6 years ago

You can mirror the logic for --verbose and HOMEBREW_VERBOSE. See https://github.com/Linuxbrew/brew/blob/master/Library/Homebrew/extend/ARGV.rb

sjackman commented 6 years ago

Have you had a chance to look into adding --force-linux? No rush at all.

sjackman commented 6 years ago

@MikeMcQuaid Hi, Mike. This discussion may sound familiar to you from a year ago or more. We'd like to improve the experience of Linuxbrew/core by tagging those formulae that are known not to work currently on Linux. I'd appreciate your input on this PR.

depends_on :macos indicates the formula truly depends on macOS.
A bottle for x86_64_linux indicates that that formulae is known to work on Linux.
We'd like to add fails_with :linux to formulae that have no successful installs and at least one build failure on analytics.

MikeMcQuaid commented 6 years ago

@sjackman I think it's worth not adding additional DSLs for this for now until we have Homebrew/brew the primary upstream for Linuxbrew.

It feels weird to overload the fails_with DSL for this; I'd rather see a depends_on requirement instead. If this is added to Homebrew/brew, though, note I'd want it to not be included in Homebrew/homebrew-core.

sjackman commented 6 years ago

I think it's worth not adding additional DSLs for this for now until we have Homebrew/brew the primary upstream for Linuxbrew.

Shall we merge this PR here in Linuxbrew/brew, so that we can start using it in Linuxbrew/core, before this PR is upstreamed to Homebrew/brew?

It feels weird to overload the fails_with DSL for this; I'd rather see a depends_on requirement instead.

We debated on Slack either depends_on :skip_linux or fails_with :linux. This PR implements both. Currently fails_with applies only to compilers, so it's a bit weird if you know the internals of Homebrew. If I didn't know the internals of Homebrew, I'd say that fails_with :linux would make more sense in English to a new contributor than depends_on :skip_linux. I'm really fine with either though. It's a mild preference.

If this is added to Homebrew/brew, though, note I'd want it to not be included in Homebrew/homebrew-core.

👍 It's intended for Linuxbrew/core.

MikeMcQuaid commented 6 years ago

Shall we merge this PR here in Linuxbrew/brew, so that we can start using it in Linuxbrew/core, before this PR is upstreamed to Homebrew/brew?

@sjackman It's up to you but I feel we're close enough now that it'd be good to try and avoid merging any new "features" in Linuxbrew/brew before we've got Homebrew/brew as the default remote for Linuxbrew users. If that means that PRs like this go to Homebrew/brew first: 🆒.

MikeMcQuaid commented 6 years ago

We debated on Slack either depends_on :skip_linux or fails_with :linux. This PR implements both. Currently fails_with applies only to compilers, so it's a bit weird if you know the internals of Homebrew. If I didn't know the internals of Homebrew, I'd say that fails_with :linux would make more sense in English to a new contributor than depends_on :skip_linux. I'm really fine with either though. It's a mild preference.

I'm not sure I understand what the behaviour changes are; can you elaborate?

sjackman commented 6 years ago

I'm not sure I understand what the behaviour changes are; can you elaborate?

depends_on :skip_linux and fails_with :linux are two names for the exact same feature. Both add a dependency on the requirement SkipLinuxRequirement, which displays a message indicating that this formula does not currently work on Linux. We're only debating which name we prefer.

sjackman commented 6 years ago

If that means that PRs like this go to Homebrew/brew first: 🆒.

Excellent. @jonchang Would you like to close this PR and submit it to Homebrew/brew?

jonchang commented 6 years ago

Yeah, I can do that. To be clear, this means we're not overloading fails_with and instead just doing depends_on :skip_linux?

sjackman commented 6 years ago

To be clear, this means we're not overloading fails_with and instead just doing depends_on :skip_linux?

I'm not yet sure. Which name do you prefer, Mike? fails_with :linux or depends_on :skip_linux ?

MikeMcQuaid commented 6 years ago

Sorry, I'll be more explicit: how does the behaviour differ for an end-user between depends_on :macos and depends_on :skip_linux/fails_with :linux? I appreciate that for the Linuxbrew maintainers there's a different meaning but I'm interested in the behaviour differences.

To save time: my guess is the end result is the same but the messaging may be different. In that case: what about something like depends_on :macos => :for_now or :temporarily or :until_fixed or something else? That could allow for custom messaging and differentiation for Linuxbrew maintainers while not needing to add too much extra logic for what's mostly syntactical sugar.

sjackman commented 6 years ago

Ah, I understand you now. The only behavioural difference is that fails_with :linux (other than displaying a different message) is that it may be overridden with HOMEBREW_FORCE_LINUX (current implemented) or brew install --force-linux (not yet implemented).

I'm somewhat less fond of the name depends_on :macos => :until_fixed because the problem is not that it currently depends on macOS, but that it currently fails on Linux, which I don't feel is conveyed by this DSL.

MikeMcQuaid commented 6 years ago

I'm somewhat less fond of the name depends_on :macos => :until_fixed because the problem is not that it currently depends on macOS, but that it currently fails on Linux, which I don't feel is conveyed by this DSL.

Given Homebrew only plans on supporting two platforms (macOS and Linux) I'd love to see if we can't figure out of accomplishing this with some use of depends_on :macos or depends_on :linux rather than needing to add and support another DSL. The behaviour you've described should be possible regardless of what the DSL reads like.

sjackman commented 6 years ago

To me, we're only debating the name, since depends_on and fails_with both already exist. I'd suggest one of the following:

depends_on :macos => :linux_fails
depends_on :skip_linux
fails_with :linux

My personal preference is fails_with :linux, but I'm fine with any of these three.

MikeMcQuaid commented 6 years ago

depends_on :macos => :linux_fails

I would prefer this (whatever the => :* ends up being) as it avoids adding an extra requirement.

My personal preference is fails_with :linux

That's the one I'm least in favour of because it overloads a DSL that's currently only used for compilers.

sjackman commented 6 years ago

@jonchang Are you up for submitting this PR to Homebrew/brew, with depends_on :macos => :linux_fails, --force-linux, HOMEBREW_FORCE_LINUX=1?

Any suggestions on the name => :linux_fails?

jonchang commented 6 years ago

@sjackman yeah I can get to it this week. I dunno if there's a better name than :linux_fails but I'll think about it.

sjackman commented 6 years ago

depends_on :macos => :skip_linux depends_on :macos => :help_wanted depends_on :macos => :until_fixed ?

sjackman commented 6 years ago

Thanks, Jon!

MikeMcQuaid commented 6 years ago

depends_on :macos => :help_wanted depends_on :macos => :until_fixed

I like these

sjackman commented 6 years ago

Closing this PR, because @jonchang plans to submit it upstream to Homebrew/brew.

sjackman commented 5 years ago

@jonchang Would you like to submit this depends_on :macos => :help_wanted PR to Homebrew/brew?

jonchang commented 5 years ago

Yeah I’ll do it tomorrow, still kinda jet lagged

sjackman commented 5 years ago

Awesome. Thanks, Jon!

Linuxbrew / brew

Add `depends_on :skip_linux` and `fails_with :linux` DSLs #831