Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

formula: add on_macos and on_linux #7257

Merged
merged 1 commit into from
Apr 13, 2020

Conversation

iMichka
Copy link
Member

@iMichka iMichka commented Apr 1, 2020

This will allow to bring homebrew-core and linuxbrew-core closer together.

See #7028

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

@iMichka iMichka self-assigned this Apr 1, 2020
Copy link
Member

@issyl0 issyl0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this! Thank you! Something for my tired brain to use to standardize both the cores again with not too much thought!

@sjackman
Copy link
Member

sjackman commented Apr 1, 2020

❯❯❯ grep -hr 'depends_on ".*if OS.mac?' $(brew --repo homebrew/core)/Formula | sort | wc -l
      18

Do we want to add macos_depends_on in this same PR?

I don't want to overcomplicate things… but… we'll likely also need conditional patches. Do we want a unified predicate syntax that works for both OS conditional depends_on and OS conditional patch do?

@issyl0
Copy link
Member

issyl0 commented Apr 1, 2020

If we have macos_depends_on and linux_depends_on, do we even need depends_on on its own? Wait, I suppose for the things that are dependencies for both platforms not just one. Hmm.

I think patches should be handled in a separate PR.

@Bo98
Copy link
Member

Bo98 commented Apr 1, 2020

Some ideas - feel free to disagree:

depends_on "linux-headers" => :linux
patch do
  url "..."
  sha256 "..."
  only_on :linux
end

@sjackman
Copy link
Member

sjackman commented Apr 1, 2020

I like Bo's suggestions. I'd like something as parallel as possible. So perhaps…

depends_on "linux-headers", :on => :linux

patch do
  
  on :linux
end

Or perhaps…

on_macos do
  depends_on 
  patch do
    
  end
end

though not sure that I like it any better. Just spitballing.

@issyl0
Copy link
Member

issyl0 commented Apr 1, 2020

I thought it might help to contextualise each of these suggestions in the context of a formula, so I mocked up some (non-functional, obviously) examples with one of our most common Linux dependencies: pkg-config, in the (randomly chosen) formula awscurl.


I like Bo's suggestions, but we'd have to do some designing around what would happen in the depends_on "foo" => :linux case for build- or test-only dependencies. depends_on "foo" => [:linux, :build] matches what we have already in some cases for build- and test-only dependencies, but it's not the cleanest when there are two categories of thing specified.


on_macos do
  depends_on 
  patch do
    
  end
end

My concern with this is that yes, it's self-contained, but it makes the general flow of a formula not feel so good and would be alien to most contributors, especially if we nest patches within this as well. And in which block do we put uses_from_macos and other depends_ons that exist for both macOS and Linux? Do they exist in this no-mans-land between the two? This'd also require a load of re-working of brew audit cops (DependencyOrder for one), though that's considerably less of a priority.

@iMichka
Copy link
Member Author

iMichka commented Apr 2, 2020

I see where you want to go with :linux, but why did we introduce uses_from_macos first? Why did we not introduce depends_on "zlib" => :from_macos instead?

Also, as @issyl0 said (if I understood well): we would be mixing two concepts: build/test stuff, and platform stuff. What would you do if you need a build dependency for Linux, but the same package is needed as a full dependency on mac? This might be too complex to write, or cumbersome at least.

I would like to keep this first PR small, so I am not focusing on patches (though the design choice for patches might influence this PR). Regarding mac only dependencies; I would like to introduce this in a second PR too, to keep this one small.

depends_on: for mac and linux
macos_depends_on: only for mac
linux_depends_on: only for linux
uses_from_macos: provided by macos, normal dependency on linux

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a far of the naming as-is. I'd like to explore the problem a lot more before we jump into a DSL (that we'll need to support indefinitely)

What are some examples of things that are depended on on Linux and not macOS? Why is that? Is it actually "Linux" or is it "not macOS" (these things are not interchangeable). Is it for Homebrew on Linux or is it the same in e.g. Debian?

uses_from_macos is communicating a specific thing about a dependency that macOS provides that Linux does not (which we still need an audit for so it's not used for packages not provided by macOS; I'd also like to see that work "finished" before we add more DSL).

@iMichka
Copy link
Member Author

iMichka commented Apr 2, 2020

Actually there are not so many, here are the main ones:

depends_on "[email protected]" unless OS.mac?
depends_on "linux-pam" unless OS.mac?
depends_on "texinfo" unless OS.mac?
depends_on "pkg-config" unless OS.mac?
depends_on "util-linux" unless OS.mac?

There are a few more exotic ones. Sometimes build scripts or libraries require different libraries on linux and on mac (I found a few: jemalloc, gmp, gperf ...). On purpose, by mistake, or due to bugs.

Sometimes it was not noticed on mac, because the build is done on a clean CI, but we had some users on Linux complaining about opportunistic linking with libraries installed outside of brew while building from source: this made us add more dependencies (or try to disable these features when possible) over time to avoid build failures.

Maybe some of these are not necessary anymore and can be cleaned up. But stuff like util-linux or linux-pam will remain.

I had a look at linuxbrew-core, there are still many cases were we could use uses_from_macos, so if you use a regex to count the remaining cases: this is not really representative.

One thing which is also linux-only right now are the xorg libraries, but this should be fixed once xorg is merged into homebrew-core.

@MikeMcQuaid
Copy link
Member

depends_on "[email protected]" unless OS.mac?

This looks like uses_from_macos (even although it technically doesn't ship with [email protected] it does ship with openssl).

depends_on "texinfo" unless OS.mac?

This is a uses_from_macos.

depends_on "pkg-config" unless OS.mac?

This may be a bit of a uses_from_macos (as macOS provides some pkg-config files) or if not I'd argue it's not specific to Linux (e.g. it would likely also apply to BSD or even Windows).

depends_on "linux-pam" unless OS.mac?
depends_on "util-linux" unless OS.mac?

This seem legitimately Linux-only dependencies (clue is in the name, I guess 😂). I think I'd prefer a depends_on "linux-pam" => :linux as mentioned above.

Sometimes it was not noticed on mac, because the build is done on a clean CI, but we had some users on Linux complaining about opportunistic linking with libraries installed outside of brew while building from source: this made us add more dependencies (or try to disable these features when possible) over time to avoid build failures.
Maybe some of these are not necessary anymore and can be cleaned up.

Yeh, these seem like ones that don't warrant being different on macOS and Linux.

One thing which is also linux-only right now are the xorg libraries, but this should be fixed once xorg is merged into homebrew-core.

Yeh, this is another case where I suspect that macOS and Linux will want to unify the formulae.

I think (given what's happened with uses_from_macos) that I'd want to have a whitelist for depends_on => :linux ship in the same PR as the PR adding the functionality and we can save it for packages that literally only exist on Linux (i.e. not on BSD).

Thoughts?

@issyl0
Copy link
Member

issyl0 commented Apr 2, 2020

depends_on "[email protected]" unless OS.mac?

This looks like uses_from_macos (even although it technically doesn't ship with [email protected] it does ship with openssl).

depends_on "texinfo" unless OS.mac?

This is a uses_from_macos.

I'll get on and sync both of these then!

depends_on "util-linux" unless OS.mac?

This seem legitimately Linux-only dependencies (clue is in the name, I guess ).

I also thought this, but there are some uses_from_macos "util-linux" occurrences in Homebrew/homebrew-core. Should I remove them?

@MikeMcQuaid
Copy link
Member

I also thought this, but there are some uses_from_macos "util-linux" occurrences in Homebrew/homebrew-core. Should I remove them?

@issyl0 Yes but I think higher priority: would you (or anyone else here) be able to work on a uses_from_macos whitelist rubocop or audit? If not, I can take a look at some point.

@iMichka
Copy link
Member Author

iMichka commented Apr 3, 2020

@issyl0 Yes but I think higher priority: would you (or anyone else here) be able to work on a uses_from_macos whitelist rubocop or audit? If not, I can take a look at some point.

That would be great. It's not the topic of this PR, this can be done in parallel. But definitively needed. Of course if we want to whitelist the linux only dependencies, it would be great to have the mac whitelist implemented upfront, so we can extend that whitelist mechanism for this Linux PR too).

depends_on "[email protected]" unless OS.mac?
This looks like uses_from_macos (even although it technically doesn't ship with [email protected] it does ship with openssl).

I would not misuse uses_from_macos for this then. I think there was already some confusion about the naming of [email protected] vs openssl (there should have been an [email protected]).
On the other side maybe these formulae probably just need [email protected] on mac too, we need to check this, and this discussion is not needed.

depends_on "pkg-config" unless OS.mac?
This may be a bit of a uses_from_macos (as macOS provides some pkg-config files) or if not I'd argue it's not specific to Linux (e.g. it would likely also apply to BSD or even Windows).

I always wondered how this worked on mac. The .pc files are there, but no pkg-config is delivered by the system? I would be ok to use uses_from_macos for that one.

I think (given what's happened with uses_from_macos) that I'd want to have a whitelist for depends_on => :linux

I am fine to have a whitelist, to keep this under control.

I still disagree about the syntax: depends_on => :linux is a different mechanism in the brew code compared to linux_depends_on. linux_depends_on is a method, and uses_from_macos is also a method. depends_on => :linux uses a hash rocket, which is something different than a method. I think using the latter would make both mechanisms incosistent from a DLS/syntax/code point of view.

@MikeMcQuaid
Copy link
Member

I would not misuse uses_from_macos for this then. I think there was already some confusion about the naming of [email protected] vs openssl (there should have been an [email protected]).

I believe uses_from_macos "openssl" should work as you would expect it to.

On the other side maybe these formulae probably just need [email protected] on mac too, we need to check this, and this discussion is not needed.

Well, they don't "need" it as they are building fine 😉. I'm not as allergic to the system LibreSSL as some maintainers (past at least) have been so I think it's worth leaving as-is.

depends_on => :linux uses a hash rocket, which is something different than a method. I think using the latter would make both mechanisms incosistent from a DLS/syntax/code point of view.

Yup, that's intentional. I don't think the underlying method of implementation needs to be consistent between macOS and Linux. We may end up with a need for depends_on => :macos at some point too.

uses_from_macos is special because it applies to a whitelistable list of packages and because it describes what's actually happening: there is a dependency there but it's provided by the macOS system. It provides additional information about the formula on macOS even although it's a no-op. Furthermore, given that Homebrew on Linux is a Linux port of a macOS package manager it's expected that there will still be a degree of macOS primacy.

@issyl0
Copy link
Member

issyl0 commented Apr 4, 2020

I've started to work on the uses_from_macos audit, building on @jonchang's work in #6265, over here.

@sjackman
Copy link
Member

sjackman commented Apr 6, 2020

depends_on "pkg-config" unless OS.mac?
This may be a bit of a uses_from_macos (as macOS provides some pkg-config files) or if not I'd argue it's not specific to Linux (e.g. it would likely also apply to BSD or even Windows).

I always wondered how this worked on mac. The .pc files are there, but no pkg-config is delivered by the system? I would be ok to use uses_from_macos for that one.

I would recommend not using uses_from_macos "pkg-config" since macOS does not provide a pkg-config executable. What I suspect is happening here is that when a dependency is provided by macOS, then on macOS the build script knows exactly where to find the dependency (in /usr/lib) and pkg-config is not needed, whereas on Linux the build script uses pkg-config to find the dependency.

See for example https://github.com/Homebrew/linuxbrew-core/blob/master/Formula/sratoolkit.rb

  uses_from_macos "libxml2"
  uses_from_macos "perl"

  depends_on "pkg-config" => :build unless OS.mac?

https://github.com/Homebrew/linuxbrew-core/blob/c4c2211bea800c415a1454c47c529438aca87209/Formula/sratoolkit.rb#L18-L21

On macOS libxml2 is a known location. On Linux it uses pkg-config to find libxml2 (unverified).

@sjackman
Copy link
Member

sjackman commented Apr 6, 2020

We may end up with a need for depends_on => :macos at some point too.

Yes, there are 18 occurrences of dependencies needed on macOS but not on Linux.
See #7257 (comment)

@MikeMcQuaid
Copy link
Member

Yes, there are 18 occurrences of dependencies needed on macOS but not on Linux.
See #7257 (comment)

Another option (considering, as you've pointed out, the need for macOS or Linux-specific patches) would be a

macos do
  depends_on ...
  patch ...
end

linux do
  depends_on ...
  patch ...
end

We've already had that in the past due to stable do, head do, etc. so it might be a good fit to avoid having to add a bunch more custom DSL.

@dawidd6
Copy link
Member

dawidd6 commented Apr 7, 2020

macos do
  depends_on ...
  patch ...
end

linux do
  depends_on ...
  patch ...
end

It won't be any different from what we are doing right now, just a change from if OS.mac? to macos do, but it surely looks nicer.

@iMichka
Copy link
Member Author

iMichka commented Apr 7, 2020

I think I like it, because it solves the patch and the depends_on issue quite nicely. If everyone agrees on that syntax I'll work on that.

@MikeMcQuaid
Copy link
Member

It won't be any different from what we are doing right now, just a change from if OS.mac? to macos do, but it surely looks nicer.

It is a bit different: if OS.linux? means if you're running on Mac that line is effectively invisible to you (unless you override OS.linux? globally). This makes it hard to parse a formula on macOS with Linux-specific components (and vice-versa).

I still think uses_from_macos, keg_only :provided_by_macos (and possibly some other future DSLs) will have their place because they are more descriptive but for something that allows global parsing this works.

This pattern can also work in e.g. def install where it can effectively just mean if OS.linux? (as you never need to parse the contents of def install and it means we can more easily state/audit "don't do if OS.linux? in formulae ever").

@sjackman
Copy link
Member

sjackman commented Apr 7, 2020

Another option (considering, as you've pointed out, the need for macOS or Linux-specific patches) would be a

I suggested on_macos do and on_linux do a week ago, just saying. 😉
#7257 (comment)

Now on to the bike shedding, I have a mild preference for on_macos do over macos do. Any other opinions from the 🥜 gallery?

@iMichka iMichka requested a review from issyl0 April 10, 2020 06:57
@iMichka iMichka changed the title formula: add linux_depends_on formula: add on_macos and on_linux Apr 10, 2020
@iMichka
Copy link
Member Author

iMichka commented Apr 10, 2020

I tested the new blocks with dependencies and patches.

I can also add a test for resources? Are there other things we might want to test? I'll split the tests I wrote in more smaller subtests if there is is more content to be added.

What do you think?

@issyl0
Copy link
Member

issyl0 commented Apr 10, 2020

I have a process question. What's the plan for using this in the wild? Because switching over depends_on "foo" unless OS.mac? to the new on_linux construct is good, but which repo does it go in? I assume Homebrew/linuxbrew-core in the first instance, but when do we sync those up to Homebrew/homebrew-core? 'Cause it might be confusing to users to have on_linux in Homebrew/homebrew-core formulae, but we'll still have to tell them that changing those things doesn't get tested by CI and that it's still in a separate repo. 🤔

@MikeMcQuaid
Copy link
Member

I can also add a test for resources?

If that's something you'll need in linuxbrew-core: do it 😁

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good (and implementation is nicely simple).

Library/Homebrew/formula.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula.rb Show resolved Hide resolved
@iMichka
Copy link
Member Author

iMichka commented Apr 11, 2020

I added the doc, and extended the tests.

@issyl0 lets discuss the process somewhere else than this PR :)

@issyl0
Copy link
Member

issyl0 commented Apr 12, 2020

Sudden thought: caveats are a thing we should probably handle here too.

╭─issyl0@chi ~/repos/homebrew/linuxbrew-core ‹master ✓›
╰─$ ack -A3 "caveats" . | grep "unless OS.mac?"
Formula/gnu-time.rb-36-    return unless OS.mac?
Formula/gnu-indent.rb-42-    return unless OS.mac?
Formula/ed.rb-34-    return unless OS.mac?
Formula/findutils.rb-66-    return unless OS.mac?
Formula/gnu-units.rb-37-    return unless OS.mac?
Formula/make.rb-34-    return unless OS.mac?
Formula/fontforge.rb-57-    return unless OS.mac?
Formula/grep.rb-43-    return unless OS.mac?
Formula/gnu-sed.rb-40-    return unless OS.mac?
Formula/gnu-tar.rb-59-    return unless OS.mac?
Formula/gnu-which.rb-38-    return unless OS.mac?

@MikeMcQuaid
Copy link
Member

Sudden thought: caveats are a thing we should probably handle here too.

They'll be handled as-is with the current functionality.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

Library/Homebrew/formula.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula.rb Outdated Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member

Looks good, nice work @iMichka!

@MikeMcQuaid MikeMcQuaid merged commit cbb5fd7 into Homebrew:master Apr 13, 2020
@iMichka iMichka deleted the linuxdependson branch April 13, 2020 08:19
@iMichka
Copy link
Member Author

iMichka commented Apr 13, 2020

Thanks!

@iMichka
Copy link
Member Author

iMichka commented Apr 13, 2020

I am working on adding an audit for this. I need more opinions.

The tests in this PR were written with the idea in my mind that there would be multiple on_macos/on_linux blocks in a formula, with their content close to where the stuff is used: urls near urls, dependencies near dependencies, patches near patches, and so on ...

@MikeMcQuaid proposed an alternative, with only one single block, like a stable do block.
I wrote a fake formula example (it is not so far away from the reality, sadly).

class Crystal < Formula
  desc "Fast and statically typed, compiled language with Ruby-like syntax"
  homepage "https://crystal-lang.org/"

  on_macos do
    stable do
      url "https://github.com/crystal-lang/crystal/archive/mac-0.34.0.tar.gz"
      sha256 "973293ffbcfa4fb073f6a2f833b0ce5b82b72f7899427f39d7e5610ffc9029c8"

      resource "shards" do
        url "https://github.com/crystal-lang/shards/archive/v0.10.0.tar.gz"
        sha256 "3aea420df959552d1866d473c878ab1ed0b58489c4c9881ef40a170cfb775459"
      end
    end

    resource "boot" do
      url "https://github.com/crystal-lang/crystal/releases/download/0.33.0/crystal-0.33.0-1-darwin-x86_64.tar.gz"
      version "0.33.0-1"
      sha256 "edb48d4d8276fc5e689fb95c5dc669d115ad3d1bf22485dad77d44a370e585fd"
    end
  end

  on_linux do
    stable do
      url "https://github.com/crystal-lang/crystal/archive/linux-0.34.0.tar.gz"
      sha256 "973293ffbcfa4fb073f6a2f833b0ce5b82b72f7899427f39d7e5610ffc9029c8"

      resource "shards" do
        url "https://github.com/crystal-lang/shards/archive/v0.10.0.tar.gz"
        sha256 "3aea420df959552d1866d473c878ab1ed0b58489c4c9881ef40a170cfb775459"
      end
    end

    depends_on "perl"

    resource "boot" do
      url "https://github.com/crystal-lang/crystal/releases/download/0.33.0/crystal-0.33.0-1-linux-x86_64.tar.gz"
      version "0.33.0-1"
      sha256 "9b9e078e9ba24fb97ee591d5f0a57c88cd018bd85ed6bdde9a30e5834b158128"
    end

    # special linux only resource
    resource "res" do
      url "https://github.com/ivmai/bdwgc/releases/download/v8.0.4/linux-resource-8.0.4.tar.gz"
      sha256 "436a0ddc67b1ac0b0405b61a9675bca9e075c8156f4debd1d06f3a56c7cd289d"
    end

    # special linux patch
    patch :p1 do
      url "https://github.com/test/test/test/5668de71107022a316ee967162bc16c10754b9ce.patch?full_index=1"
      sha256 "5c42d4b37cf4997bb6af3f9b00f5513644e1287c322607dc980a1955a09246e3"
    end
  end

  bottle do
    sha256 "7f6f09fefecbeab7ff11bcd35b501339f99aa2b4409c8612e1eae2c4ad0a206c" => :catalina
    sha256 "a2a5055a9abe2db444e2a165133703a1c203d692f66c0e1fb326c97a28c8ef80" => :mojave
    sha256 "c37811acb4753d689d8d7455b0ddfd64bf9530c430240563608e05b6bed8cedd" => :high_sierra
  end

  head do
    url "https://github.com/crystal-lang/crystal.git"

    resource "shards" do
      url "https://github.com/crystal-lang/shards.git"
    end
  end

  depends_on "pkg-config"

  uses_from_macos "zlib"

  # Crystal uses an extended version of bdw-gc to handle multi-threading
  resource "bdw-gc" do
    url "https://github.com/ivmai/bdwgc/releases/download/v8.0.4/gc-8.0.4.tar.gz"
    sha256 "436a0ddc67b1ac0b0405b61a9675bca9e075c8156f4debd1d06f3a56c7cd289d"

    # extension to handle multi-threading
    patch :p1 do
      url "https://github.com/ivmai/bdwgc/commit/5668de71107022a316ee967162bc16c10754b9ce.patch?full_index=1"
      sha256 "5c42d4b37cf4997bb6af3f9b00f5513644e1287c322607dc980a1955a09246e3"
    end
  end

  def install
    ...
  end
end

What do you think?

@issyl0
Copy link
Member

issyl0 commented Apr 13, 2020

I did a bit of playing around with this in a tap and got something like this which does install cleanly. It's a bit weird at first glance, but on reflection, I do like that everything is in its place and near to the "normal" locations of depends_on and caveats. The way I originally thought this would go - much like Michka - was to have multiple of these on_linux and on_macos blocks for each section.

@MikeMcQuaid
Copy link
Member

What do you think?

I like that example (except I'd have the OS-specific stuff lower down but we can debate that later).

I did a bit of playing around with this in a tap and got something like this which does install cleanly. It's a bit weird at first glance, but on reflection, I do like that everything is in its place and near to the "normal" locations of depends_on and caveats. The way I originally thought this would go - much like Michka - was to have multiple of these on_linux and on_macos blocks for each section.

I think we want to allow something like that but what I'd do would be:

def caveats
  on_macos do
    "gds-cli depends on aws-vault being installed.  You can install it with `brew cask install aws-vault`."
  end
end

@iMichka iMichka mentioned this pull request May 3, 2020
6 tasks
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 2, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants