Add uses from macos #6162

gkpacker · 2019-05-22T17:36:11Z

Fixes #6150

Add uses_from_macos method:

On Linux:

Acts like depends_on, ignoring OS version specifications

On MacOS:

Delegate to depends_on only if system meets dependency os requirements

Usage

Add specified dependency based on MacOS version

class MyFormula < ::Formula
  url "foo.bar"

  depends_on "foo"
  uses_from_macos "bar", after: :el_capitan
  uses_from_macos "baz", before: :mojave
end

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?

sjackman

Nicely done, Gabriel! And thanks for the comprehensive tests!

Library/Homebrew/extend/os/mac/formula.rb

MikeMcQuaid · 2019-05-23T07:27:42Z

I'm away on a work trip right now but will review this when I get the chance. Thanks!

xu-cheng · 2019-05-23T07:49:03Z

FYI, you may want to implement this feature in SoftwareSpec instead of Formula in order to support different specs. For example:

head do
  uses_from_mac "python"
end

sjackman · 2019-05-23T15:43:26Z

@xu-cheng wrote… #6150 (comment)

Just my two cents, you may want to support :build, :test arguments for this new DSL. For example,
uses_from_macos "m4", tag: :build
uses_from_macos "gpatch", tag: [:build, :test]

sjackman · 2019-05-23T15:45:45Z

These are both good suggestions.

Gabriel, would you be up for moving the definition of uses_from_macos from Formula to SoftwareSpec? Formula would keep a definition of uses_from_macos that forwards to each of the formula's SoftwareSpec, similar to depends_on.

sjackman · 2019-05-23T15:47:43Z

uses_from_macos "m4", tag: :build
uses_from_macos "gpatch", tag: [:build, :test]

I suggest keeping the same syntax as depends_on:

uses_from_macos "m4" => :build
uses_from_macos "gpatch" => [:build, :test], :before => :mojave

MikeMcQuaid · 2019-05-23T16:09:18Z

I suggest keeping the same syntax as depends_on:

Agreed 👍

gkpacker · 2019-05-24T01:04:49Z

Gabriel, would you be up for moving the definition of uses_from_macos from Formula to SoftwareSpec?

Yeah, sure.

gkpacker · 2019-05-24T01:07:08Z

@sjackman Actually, i was wondering if it is really necessary, because uses_from_macos is only a depends_on 'mask', so whatever dependency it adds, it will pass to depends_on, right?

gkpacker · 2019-05-24T01:48:53Z

Ah, head isn't a formula

sjackman · 2019-05-24T05:47:07Z

Thank you, Gabriel! Sorry that I hadn't given you the right info in the first place.

MikeMcQuaid

Great start on this @gkpacker, thanks!

MikeMcQuaid · 2019-05-24T17:09:31Z

Library/Homebrew/extend/os/mac/formula.rb

+class Formula
+  class << self
+    def uses_from_macos(dep, **args)
+      depends_on(dep) if add_mac_dependency?(args)


Agreed with @xu-cheng this should should tags like :test and :build and pass them through accordingly and it should be on SoftwareSpec.

If dep is a hash, I think that does happen with the current code (untested).

I think it already works with tags, i'll add some specs for it

Library/Homebrew/extend/os/mac/formula.rb

MikeMcQuaid · 2019-05-24T17:10:37Z

Library/Homebrew/test/os/mac/formula_spec.rb

+require "formula"
+
+describe Formula do
+  describe "#uses_from_macos" do


Nice work on the tests 👏

sjackman

Excellent first contribution, Gabriel! I hope there are more to come. 🌟

xu-cheng · 2019-05-25T08:58:51Z

Library/Homebrew/extend/os/mac/software_spec.rb

+
+class SoftwareSpec
+  def uses_from_macos(deps, **args)
+    depends_on(deps) if add_mac_dependency?(args)


Current code will fail to handle uses_from_macos "foo" => :build, :before => :el_capitan. As in this case,
deps is { "foo" => :build, :before => : el_capitan }, while args is {}

I suggest something like the following. Though you may consider to choose better variable names.

Suggested change

depends_on(deps) if add_mac_dependency?(args)

if deps.is_a?(Hash)

dep, dep_tags = deps.first

deps.delete(dep)

depends_on(dep => dep_tags) if add_mac_dependency?(deps)

else

depends_on(deps) if add_mac_dependency?(args)

end

Further, I suggest you should test all four cases in the test cases.

uses_from_macos "foo"

uses_from_macos "foo" , :before => :el_capitan

uses_from_macos "foo" => :build

uses_from_macos "foo" => :build, :before => :el_capitan

Good catch, @xu-cheng! I suggest…

def uses_from_macos(dep, **args) if dep.is_a?(Hash) args = dep dep = Hash[*args.shift] end depends_on(dep) if add_mac_dependency?(args) end

Further, I suggest you should test all four cases in the test cases.

Good suggestion!

xu-cheng · 2019-05-25T09:00:23Z

Library/Homebrew/extend/os/mac/software_spec.rb

+    return false if args[:before] && OS::Mac.version >= args[:before]
+
+    true
+  end


From my understanding, uses_from_macos "foo" (i.e. without version args) should not add foo as dependency on macOS. However, the current logic does the opposite.

sjackman · 2019-05-25T17:12:48Z

Library/Homebrew/extend/os/mac/software_spec.rb

+
+class SoftwareSpec
+  def uses_from_macos(deps, **args)
+    depends_on(deps) if add_mac_dependency?(args)


Good catch, @xu-cheng! I suggest…

def uses_from_macos(dep, **args) if dep.is_a?(Hash) args = dep dep = Hash[*args.shift] end depends_on(dep) if add_mac_dependency?(args) end

Further, I suggest you should test all four cases in the test cases.

Good suggestion!

sjackman · 2019-05-25T17:13:58Z

Library/Homebrew/extend/os/mac/software_spec.rb

+
+    return false if args[:before] && OS::Mac.version >= args[:before]
+
+    true


From my understanding, uses_from_macos "foo" (i.e. without version args) should not add foo as dependency on macOS. However, the current logic does the opposite.

Suggested change

true

!args.empty?

Well, actually i think it makes sense to have this behaviour only on Linux, since it uses from macos. Without any version specification it seems that the method uses from macos unconditionally, so:

On MacOS

uses_from_macos "foo" # always add foo as dep in macos uses_from_macos "foo", after: :sierra # add foo as dep only after macos sierra

On Linux

uses_from_macos "foo" # Does nothing, since we're adding a macos dependency

What do you think? @sjackman @xu-cheng

In fact it's the exact opposite of that. uses_from_macos "foo", after: :sierra means that on macOS Sierra and later we use foo provided by macOS operating system, and so we do not need to to use the brewed dependency. Before macOS Sierra we need to install the brewed dependency. On Linux we always install the dependency.

On macOS

uses_from_macos "foo" # Never add foo as dep in macos uses_from_macos "foo", after: :sierra # add foo as dep only before macos sierra

On Linux

uses_from_macos "foo" # Always add foo as dep on Linux

gkpacker · 2019-05-27T01:02:24Z

Library/Homebrew/extend/os/mac/software_spec.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true

 class SoftwareSpec
+  undef uses_from_macos


Not sure about this, i was troubleshooting a misterious spec that was failling because it requires linux and it wasn't mocking with OS.linux?. Saw this on other files and added here...

gkpacker · 2019-05-27T01:03:35Z

Library/Homebrew/test/os/linux/software_spec_spec.rb

+      expect(spec.deps.last.tags).to include(:head)
+    end
+  end
+end


Maybe it shouldn't be on os/linux/ anyway, since it's the default behaviour

If these tests fail on macOS, then they do need to be in os/linux/. If they pass on macOS, then yeah, they could be moved.

You can use the :needs_linux tag instead for (the only) Linux specific tests. The test file folder locations should match the source file definitions.

Mocking OS.linux shouldn't be enough for this case? It was running the extended uses_from_macos version even returning true on OS.linux?

It would be sufficient but it's not how we've been doing things with other tests. Would appreciate consistency for now and could change elsewhere in a follow-up PR?

sjackman · 2019-05-28T05:44:31Z

Library/Homebrew/software_spec.rb

@@ -170,6 +170,12 @@ def depends_on(spec)
    add_dep_option(dep) if dep
  end

+  def uses_from_macos(deps, **_args)
+    deps.is_a?(Hash) && deps = Hash[*deps.shift]


Suggested change

deps.is_a?(Hash) && deps = Hash[*deps.shift]

deps = Hash[*deps.shift] if deps.is_a?(Hash)

Minor style nit, otherwise this PR looks good to me!

sjackman · 2019-05-28T05:45:48Z

Library/Homebrew/test/os/linux/software_spec_spec.rb

+      expect(spec.deps.last.tags).to include(:head)
+    end
+  end
+end


If these tests fail on macOS, then they do need to be in os/linux/. If they pass on macOS, then yeah, they could be moved.

MikeMcQuaid

A few comments from @xu-cheng and @sjackman still need addressed but looking good so far!

sjackman · 2019-05-29T19:28:55Z

Three CI failures on Linux to check out:

Failed examples:

rspec ./test/software_spec_spec.rb:136 # SoftwareSpec#uses_from_macos allows specifying dependencies
rspec ./test/software_spec_spec.rb:149 # SoftwareSpec#uses_from_macos ignores OS version specifications
rspec ./test/software_spec_spec.rb:142 # SoftwareSpec#uses_from_macos works with tags

https://dev.azure.com/Homebrew/Homebrew/_build/results?buildId=10713&view=logs

gkpacker · 2019-05-30T13:40:15Z

@sjackman How can i fix if can't run them locally?

gkpacker · 2019-05-30T16:16:05Z

Oh, sorry, dind't checked the logs

sjackman · 2019-05-30T20:30:52Z

I use Docker for macOS to run brew tests on Linux. It works great!

gkpacker · 2019-06-03T14:29:57Z

@sjackman Another few questions:

How can i run the CI on the last commit?
Do i have to commit another file?
What are the next steps?

sjackman · 2019-06-03T15:12:28Z

The CI seems stalled. To rerun it, rebase the PR onto origin/master and git push --force gkpacker add_uses_from_macos.

gkpacker · 2019-06-04T15:43:14Z

@sjackman Done!

sjackman

Thank you for this work, Gabriel!

sjackman · 2019-06-05T15:23:53Z

Merged! Thank you for your first contribution to Homebrew, Gabriel! 🥇

MikeMcQuaid · 2019-06-05T15:29:34Z

Great work here @gkpacker 🎉

A suggested next PR would be to expose this information in brew info --json! The relevant code is here:

brew/Library/Homebrew/formula.rb

Lines 1590 to 1657 in 7c5f71e

    
           def to_hash 
        
             dependencies = deps 
        
             hsh = { 
        
               "name"                     => name, 
        
               "full_name"                => full_name, 
        
               "oldname"                  => oldname, 
        
               "aliases"                  => aliases.sort, 
        
               "versioned_formulae"       => versioned_formulae.map(&:name), 
        
               "desc"                     => desc, 
        
               "homepage"                 => homepage, 
        
               "versions"                 => { 
        
                 "stable" => stable&.version&.to_s, 
        
                 "devel"  => devel&.version&.to_s, 
        
                 "head"   => head&.version&.to_s, 
        
                 "bottle" => !bottle_specification.checksums.empty?, 
        
               }, 
        
               "revision"                 => revision, 
        
               "version_scheme"           => version_scheme, 
        
               "bottle"                   => {}, 
        
               "keg_only"                 => keg_only?, 
        
               "bottle_disabled"          => bottle_disabled?, 
        
               "options"                  => [], 
        
               "build_dependencies"       => dependencies.select(&:build?) 
        
                                                         .map(&:name) 
        
                                                         .uniq, 
        
               "dependencies"             => dependencies.reject(&:optional?) 
        
                                                         .reject(&:recommended?) 
        
                                                         .reject(&:build?) 
        
                                                         .map(&:name) 
        
                                                         .uniq, 
        
               "recommended_dependencies" => dependencies.select(&:recommended?) 
        
                                                         .map(&:name) 
        
                                                         .uniq, 
        
               "optional_dependencies"    => dependencies.select(&:optional?) 
        
                                                         .map(&:name) 
        
                                                         .uniq, 
        
               "requirements"             => [], 
        
               "conflicts_with"           => conflicts.map(&:name), 
        
               "caveats"                  => caveats&.gsub(HOMEBREW_PREFIX, "$(brew --prefix)"), 
        
               "installed"                => [], 
        
               "linked_keg"               => linked_version&.to_s, 
        
               "pinned"                   => pinned?, 
        
               "outdated"                 => outdated?, 
        
             } 
        
             %w[stable devel].each do |spec_sym| 
        
               next unless spec = send(spec_sym) 
        
               next unless spec.bottle_defined? 
        
               bottle_spec = spec.bottle_specification 
        
               bottle_info = { 
        
                 "rebuild"  => bottle_spec.rebuild, 
        
                 "cellar"   => (cellar = bottle_spec.cellar).is_a?(Symbol) ? cellar.inspect : cellar, 
        
                 "prefix"   => bottle_spec.prefix, 
        
                 "root_url" => bottle_spec.root_url, 
        
               } 
        
               bottle_info["files"] = {} 
        
               bottle_spec.collector.keys.each do |os| 
        
                 bottle_url = "#{bottle_spec.root_url}/#{Bottle::Filename.create(self, os, bottle_spec.rebuild).bintray}" 
        
                 checksum = bottle_spec.collector[os] 
        
                 bottle_info["files"][os] = { 
        
                   "url"                   => bottle_url, 
        
                   checksum.hash_type.to_s => checksum.hexdigest, 
        
                 } 
        
               end 
        
               hsh["bottle"][spec_sym] = bottle_info 
        
             end

sjackman · 2019-06-05T19:12:20Z

Another suggested next PR would be to revive this PR to add depends_on :macos => :help_wanted from @jonchang to the archived Linuxbrew/brew and submit it to Homebrew/brew.
Linuxbrew/brew#831 (comment)

sjackman · 2019-06-11T16:50:29Z

Homebrew/brew 2.1.5 is released, which includes this PR and the DSL command uses_from_macos. We can start replacing depends_on "foo" unless OS.mac? in Homebrew/linuxbrew-core with uses_from_macos "foo" in Homebrew/homebrew-core.

gkpacker mentioned this pull request May 22, 2019

Add uses_from_macos "python" DSL #6150

Closed

sjackman requested a review from MikeMcQuaid May 22, 2019 20:59

sjackman suggested changes May 22, 2019

View reviewed changes

Library/Homebrew/extend/os/mac/formula.rb Outdated Show resolved Hide resolved

sjackman reviewed May 23, 2019

View reviewed changes

Library/Homebrew/extend/os/mac/formula.rb Outdated Show resolved Hide resolved

MikeMcQuaid reviewed May 24, 2019

View reviewed changes

sjackman approved these changes May 25, 2019

View reviewed changes

xu-cheng suggested changes May 25, 2019

View reviewed changes

sjackman suggested changes May 25, 2019

View reviewed changes

gkpacker commented May 27, 2019

View reviewed changes

sjackman suggested changes May 28, 2019

View reviewed changes

MikeMcQuaid requested changes May 28, 2019

View reviewed changes

gkpacker added 2 commits June 3, 2019 15:52

Add #uses_from_macos specs

f2abaf6

Add default #uses_from_macos behaviour to Formula

7980863

gkpacker added 15 commits June 3, 2019 15:52

Extends Formula class with mac os #uses_from_macos

a7e3304

Create extended os formula specs

01c5bc4

Add _ to unused args

0c1da29

Increase readability with early returns

848b338

Lint os/mac/formula

22f7653

Removes formula class extension

412f7d9

Extends SoftwareSpec with #uses_from_macos

aafe875

Add default #uses_from_macos behaviour spec

38653f0

Fix spec

7a9bfb4

Add more spec cases

332acbb

Add hash support

77f2d01

Extract linux spec cases & add hash support to default #uses_from_macos

41f3993

Add needs_linux tag to specs that fails on MacOS

b2969e6

Fix typo

678f89f

Remove old os/mac/formula spec

9fa8dc5

gkpacker force-pushed the add_uses_from_macos branch from 57d20a1 to 9fa8dc5 Compare June 3, 2019 18:53

sjackman approved these changes Jun 5, 2019

View reviewed changes

MikeMcQuaid approved these changes Jun 5, 2019

View reviewed changes

sjackman merged commit 7c5f71e into Homebrew:master Jun 5, 2019

jonchang mentioned this pull request Jun 29, 2019

audit: uses_from_macos only uses macOS software #6265

Closed

6 tasks

EricFromCanada mentioned this pull request Sep 17, 2019

Expose uses_from_macos list in formula API #6467

Merged

6 tasks

lock bot added the outdated PR was locked due to age label Jan 1, 2020

lock bot locked as resolved and limited conversation to collaborators Jan 1, 2020

gkpacker deleted the add_uses_from_macos branch July 15, 2020 21:31

-    depends_on(deps) if add_mac_dependency?(args)
+   if deps.is_a?(Hash)
+     dep, dep_tags = deps.first
+     deps.delete(dep)
+     depends_on(dep => dep_tags) if add_mac_dependency?(deps)
+   else
+     depends_on(deps) if add_mac_dependency?(args)
+   end


		return false if args[:before] && OS::Mac.version >= args[:before]

		true

	deps.is_a?(Hash) && deps = Hash[*deps.shift]
	deps = Hash[*deps.shift] if deps.is_a?(Hash)

Add uses from macos #6162

Add uses from macos #6162

Conversation

gkpacker commented May 22, 2019 • edited Loading

Fixes #6150

Usage

sjackman left a comment

Choose a reason for hiding this comment

MikeMcQuaid commented May 23, 2019

xu-cheng commented May 23, 2019

sjackman commented May 23, 2019

sjackman commented May 23, 2019

sjackman commented May 23, 2019 • edited Loading

MikeMcQuaid commented May 23, 2019

gkpacker commented May 24, 2019

gkpacker commented May 24, 2019

gkpacker commented May 24, 2019

sjackman commented May 24, 2019

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjackman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjackman May 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjackman May 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

sjackman commented May 29, 2019

gkpacker commented May 30, 2019

gkpacker commented May 30, 2019

sjackman commented May 30, 2019

gkpacker commented Jun 3, 2019

sjackman commented Jun 3, 2019 • edited Loading

gkpacker commented Jun 4, 2019

sjackman left a comment

Choose a reason for hiding this comment

sjackman commented Jun 5, 2019

MikeMcQuaid commented Jun 5, 2019

sjackman commented Jun 5, 2019

sjackman commented Jun 11, 2019

gkpacker commented May 22, 2019 •

edited

Loading

sjackman commented May 23, 2019 •

edited

Loading

sjackman May 25, 2019 •

edited

Loading

sjackman May 25, 2019 •

edited

Loading

sjackman commented Jun 3, 2019 •

edited

Loading