macOS: rebranding by JLHwung · Pull Request #964 · Homebrew/brew

JLHwung · 2016-09-15T02:13:50Z

This PR is a revived version of #359. It replace all OS X reference to #{MacOS.os_name} except that

The files in the vendor folder
The files in the cask folder
The files without a .rb extension
Reference of a specific version e.g. OS X 10.8.2, OS X Yosemite

It also replace all OS X reference to macOS on all comments except for reference of a specific version.

This PR fixes #961.

There are 2 OS X references in brew.sh. The tricky part is that even /usr/bin/sw_ver -productName returns Mac OS X on macOS 10.12, so I don't know a safe way (except if-else on productVersion) to replace OS X string in shell scripts.

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?
There is similar pull request macOS: rebranding #359, this PR covers the works on macOS: rebranding #359.
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 tests with your changes locally?

I don't use os_name for :provided_pre_{mountain_lion|mavericks|el_capitan} as before these versions, the os_name was OS X.

I leave behind line 1985 as Mac OS Lion is the system name of 10.7

Note that we cannot use MacOS.os_name here as OS.mac might not be true. Use the recent OS name here should be fine.

DomT4 · 2016-09-15T03:29:52Z

      version < "10.9"
    end

+    def os_name


You'll need to pick up the changes I suggested here. Plain version comparisons like this don't work especially well, you get situations where 10.10 is correctly reported as OS X but 10.7 is flagged as macOS.

The proposed change in the linked issue also allows you to fix the situation where Requirements have their own @version we shouldn't be ignoring. My proposed fix in the other issue isn't perfect, and will need some additional testing & tweaking, but it's closer.

I'd better write a test for that.

reitermarkus · 2016-09-15T03:58:31Z


  def to_s
    return @explanation unless @explanation.empty?
+    os_name = OS.mac? ? MacOS.os_name : 'macOS'


Is this doing what it's supposed to be doing? If OS.mac? is false, how can it be 'macOS'?

I use MacOS.os_name before but CI complains that MacOS is uninitialized constant. Test always pass locally. So I thought may be it's the case when OS.mac is false?

Seems arbitrary that this is the only place where this is happening.

That's because you need to not use MacOS (or OS.mac?) outside of Library/Homebrew/os or Library/Homebrew/extend/os/. Instead, have an OS.name method which uses subclasses in Library/Homebrew/extend/os/mac to set the right macOS name.

Alternatively, edit brew.sh to set a HOMEBREW_OS_NAME (like HOMEBREW_OS_VERSION currently) which uses the correct version and then OS.name can just use ENV["HOMEBREW_OS_NAME"]

Shout if you need more help with this!

@MikeMcQuaid I think why CI complains is that when ENV["HOMEBREW_TEST_GENERIC_OS"] is set, MacOS is not initialized but ~~we can use OS::Mac.os_name(ENV["HOMEBREW_OSX_VERSION"]) here so that both CI and macOS should be happy.~~

Instead, have an OS.name method which uses subclasses in Library/Homebrew/extend/os/mac to set the right macOS name.

It would be confused if there is a os.os_name method that returns macOS even in Linux because we implicitly assume that it is called under macOS, if not TEST_GENERIC_OS, only.
Test failed because under TEST_GENERIC_OS, os/mac is not loaded so we cannot call the method in formula_support. Although formula_support is agnostic of os/mac, but obviously all the error messages defined there are only meaningful when running on os/mac. So I suggest:

requiring os/mac on test_formula_support

supply version to mimic the real behaviour on different macOS versions.

Alternatively, edit brew.sh to set a HOMEBREW_OS_NAME (like HOMEBREW_OS_VERSION currently) which uses the correct version and then OS.name can just use ENV["HOMEBREW_OS_NAME"]

It think this is over complicated. Basically I aim to adding the pedantry in a single place so that when macOS 10.14 is released we don't have to search everywhere to remove multiple methods and their usage.

reitermarkus · 2016-09-15T04:55:44Z


+    def os_name(version = @version)
+      # TODO: replace os_name by constant string "macOS" when macOS 10.14 is released
+      if Version.create(version) >= Version.create("10.12")


I think you could just use version >= "10.12" here.

Like @DomT4 said before, plain version comparison doesn't work well as "10.7" > "10.12" in the plain string comparison.

version = Version(version) unless version.is_a? Version version >= "10.12"

will work

@MikeMcQuaid version >= "10.12" will fail because ArgumentError: comparison of Version with String failed

Check the methods above; if you create a version the same as them (probably with .new) then you should be able to compare the same as them.

Or you may need to use/try > rather than >=

@MikeMcQuaid Thank you! Version.new LGTM, I'd better add more test to ensure it does not break.

MikeMcQuaid · 2016-09-15T07:20:45Z

      version < "10.9"
    end

+    def os_name(version = @version)


Could this maybe just be called name?

@MikeMcQuaid I think name is a bit too general to use as a method name 😄. Well to tell you the truth I just find it cumbersome to rewrite all the commit messages from "use os_name" to "use name".

In that case you can avoid making version a Version if it already is one and 10.12 doesn't need to be a Version.

@MikeMcQuaid use Version here because

we need to pass Version to os_name on macos_requirement

"10.7" > "10.12" so plain string comparison is not sufficient

version = Version(version) unless version.is_a? Version

@MikeMcQuaid Ahh, this time I get your point. Thank you.

MikeMcQuaid · 2016-09-15T07:25:48Z


  def to_s
    return @explanation unless @explanation.empty?
+    os_name = OS.mac? ? MacOS.os_name : 'macOS'


That's because you need to not use MacOS (or OS.mac?) outside of Library/Homebrew/os or Library/Homebrew/extend/os/. Instead, have an OS.name method which uses subclasses in Library/Homebrew/extend/os/mac to set the right macOS name.

Alternatively, edit brew.sh to set a HOMEBREW_OS_NAME (like HOMEBREW_OS_VERSION currently) which uses the correct version and then OS.name can just use ENV["HOMEBREW_OS_NAME"]

Shout if you need more help with this!

…REW_OSX_VERSION"]

MikeMcQuaid · 2016-09-15T15:41:21Z

      version < "10.9"
    end

+    def os_name(version = @version)


In that case you can avoid making version a Version if it already is one and 10.12 doesn't need to be a Version.

MikeMcQuaid · 2016-09-15T15:42:55Z


  def to_s
    return @explanation unless @explanation.empty?
+    os_name = OS::Mac.os_name(ENV["HOMEBREW_OSX_VERSION"])


Sorry, this needs to use ENV["HOMEBREW_OS_VERSION"] (not OSX_VERSION) and be in the OS class so it works on non-OS X platforms. Alternatively, all the OS X references in code need to be moved into extend/os.

I think I'd better revert this change to use MacOS.os_name unconditionally, and require os/mac on test_formula_support. Apparently the error message is meaningless on a Linux, isn't it?

Yep, ideally we'd not have any user-facing messages that say "OS X" outside of Library/Homebrew/os/mac/ or Library/Homebrew/extend/os/mac.

So I think when test_formula_support makes assertions on the user-facing messages, it should require os/mac otherwise it does not simulate the real cases.
Apparently we are facing same os/mac loading problem if we write a test for KegOnlyReason::valid method.

os/mac should not be used/needed outside of those two directories.

@MikeMcQuaid Yep, I mean requiring os/mac on test_formula_support, just like what we do in test_os_mac_language.

@MikeMcQuaid How about I move Class KegOnlyReason in formula_support to /extend/os/mac (as we do for formula_cellar_checks) ? As the whole class is macOS-only and ideally we should not have it outside Library/Homebrew/os/mac, right? I am wondering how to specify that this method is macOnly and instruct the test should go into the macOs-only branch.

@MikeMcQuaid If necessary I can also move other OS X referencing user-facing messages to /extend/os/mac.

@JLHwung That'd be good, thanks!

@MikeMcQuaid CI complains that KegOnlyReason is uninitialized on linuxbrew because I move to os/mac. How would this class be used on Linux?

…la_support

Ideally on non-macOS platform we should do nothing on keg_only except we expand its concept

UniqMartin · 2016-09-17T15:15:26Z

As mentioned in #961 (comment) by now I'm strongly in favor of maximum simplicity and using “macOS” everywhere as soon as 10.12 is released. However, if we really wanted to be serious about adapting our output, the distinction will need to be three-fold (“Mac OS X” for 10.0-10.6, “OS X” for 10.7-10.11, and “macOS” for 10.12+).

MikeMcQuaid · 2016-09-17T19:07:10Z

As mentioned in #961 (comment):
The API reference at https://developer.apple.com/reference/ only ever speaks of “macOS” when talking about when an API was introduced, even if it's “macOS 10.0+”.

This is by far the strongest argument for me so I'm 👍 with just using macOS all over.

@JLHwung I'm so sorry to send you on such a wild chase for this, my apologies. I hope you've learnt some stuff through doing so and will continue to contribute to Homebrew. Let me know if you'd rather doing the mass-macOS replacement or if you'd rather we did. Thanks so much for your contributions.

JLHwung · 2016-09-18T15:42:10Z

@MikeMcQuaid Sorry for late reply as I am in vacation now. I've learnt a lot more than it appears at the beginning, and thank you for your review. I cannot spare time on replacing before 20th so it'd better assigned to other people.

As I am working on this pr, I find many macOS-dependent code does not locate in os/mac or extend/os/mac, shall we split the platform-dependent codes or just let it be?

UniqMartin · 2016-09-18T19:55:12Z

Thanks you for your work here!

As I am working on this pr, I find many macOS-dependent code does not locate in os/mac or extend/os/mac, shall we split the platform-dependent codes or just let it be?

This is an ongoing process and we started it fairly recently in an attempt to make Homebrew truly platform-agnostic (to the extent it makes sense) and to help move back code from Linuxbrew into Homebrew with proper abstractions and cleaner code structure. If that's a topic that interests you, we're more than happy to have someone help us with this endeavor.

JLHwung added 17 commits September 14, 2016 19:36

mac: add os_name method

8c9b5f5

system_config: use os_name

3a02065

extend/os/mac/diagnostic: use os_name

558874b

diagnostic: use os_name

4292cd5

xcode: use os_name

a12ca84

minimum_macos_requirement: use os_name

ba147c1

maximum_macos_requirement: use os_name

0430ce4

formula_support: use os_name

4476669

I don't use os_name for :provided_pre_{mountain_lion|mavericks|el_capitan} as before these versions, the os_name was OS X.

github: use os_name

4af6644

mac: add TODO on os_name method

e45aac4

fetch: replace OS X in comment to macOS

885c079

install: replace OS X in comment to macOS

f5386eb

linkapps: replace OS X in comment to macOS

efecd8c

formula: replace OS X in comment to macOS

6256e77

I leave behind line 1985 as Mac OS Lion is the system name of 10.7

formula_support: replace OS X in comment to macOS

240ba5b

formula_support: MacOS.os_name is used in warning message

0b27131

integration_cmds: replace OS X by macOS on constant string

6efce80

Note that we cannot use MacOS.os_name here as OS.mac might not be true. Use the recent OS name here should be fine.

JLHwung mentioned this pull request Sep 15, 2016

Adapt to macOS/OS X naming convention in diagnostics #961

Closed

DomT4 reviewed Sep 15, 2016

View reviewed changes

JLHwung added 3 commits September 15, 2016 00:37

formula_support: compute os_name on message

0990a7d

mac: pass version to os_name

c3c2f52

os_mac: add os_name test

a080dd8

reitermarkus reviewed Sep 15, 2016

View reviewed changes

os_mac_os_name: fix 'Loading macOS on generic OS'

5fa2441

reitermarkus reviewed Sep 15, 2016

View reviewed changes

MikeMcQuaid requested changes Sep 15, 2016

View reviewed changes

formula_support: use static method to compute os_name from ENV["HOMEB…

ccdbc40

…REW_OSX_VERSION"]

MikeMcQuaid requested changes Sep 15, 2016

View reviewed changes

JLHwung added 2 commits September 15, 2016 14:38

mac: do not create version when version is Version

284ee90

os_mac_os_name: add test with version

0db155f

JLHwung force-pushed the macOS_rebranding branch from d36f386 to 0db155f Compare September 15, 2016 17:41

JLHwung added 3 commits September 15, 2016 15:33

formula_support: move KegOnlyReason to /extend/os/mac

cb45da1

formula_support: split test KegOnlyReasonTests into test os_mac_formu…

b31ac23

…la_support

add a dummy KegOnlyReason for non-macOS platform

fb255d8

Ideally on non-macOS platform we should do nothing on keg_only except we expand its concept

JLHwung force-pushed the macOS_rebranding branch from 8f0d1e7 to fb255d8 Compare September 16, 2016 15:13

JLHwung closed this Sep 18, 2016

Homebrew locked and limited conversation to collaborators May 3, 2018

Uh oh!

Conversation

JLHwung commented Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DomT4 Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reitermarkus Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLHwung Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UniqMartin commented Sep 17, 2016

Uh oh!

MikeMcQuaid commented Sep 17, 2016

Uh oh!

JLHwung commented Sep 18, 2016

JLHwung commented Sep 15, 2016 •

edited

Loading

DomT4 Sep 15, 2016 •

edited

Loading

reitermarkus Sep 15, 2016 •

edited

Loading

JLHwung Sep 15, 2016 •

edited

Loading