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

Define OS::Mac on Linux #3820

Merged
merged 2 commits into from
Feb 21, 2018
Merged

Define OS::Mac on Linux #3820

merged 2 commits into from
Feb 21, 2018

Conversation

sjackman
Copy link
Member

@sjackman sjackman commented Feb 20, 2018

Define OS::Mac on Linux for formula API compatibility.

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

Define MacOS.version, MacOS.full_version, and MacOS::Xcode.version to Version::NULL on Linux so that brew readall succeeds and Homebrew/brew can tap Homebrew/core on Linux.

Define MacOS.version, MacOS.full_version, and MacOS::Xcode.version to
Version::NULL on Linux so that brew readall succeeds and Homebrew/brew
can tap Homebrew/core on Linux.
@maxim-belkin
Copy link
Member

maxim-belkin commented Feb 21, 2018

Hmmm... wouldn't it be better to define two new methods instead, i.e. olderthan? and newerthan? so that, as an example

MacOS.version <= :snow_leopard

becomes

MacOS.version.older_than? :snow_leopard

And make both of the methods return false on Linux... Otherwise, the above comparison will be returning true on Linux

@maxim-belkin
Copy link
Member

$ uname -s && brew irb <<< "Version::NULL < :snow_leopard"
Linux
==> Interactive Homebrew Shell
Example commands available with: brew irb --examples
Switch to inspect mode.
Version::NULL < :snow_leopard
true

@sjackman
Copy link
Member Author

sjackman commented Feb 21, 2018

The primary intention of this patch is to make it possible to tap Homebrew/core using Homebrew/brew running on Linux. We can discuss in a separate issue possible changes to the Homebrew formula API.

@maxim-belkin
Copy link
Member

Four methods would be required, actually 🤔 :

<    :  older_than?
<=   :  not_newer_than?
>=   :  not_older_than?
>    :  newer_than?

@maxim-belkin
Copy link
Member

Sure, but this is a hack. MacOS.version is meaningless on Linux and should not be returning NULL. If anything, it should give something like "not defined". We can not assume that all Linux distributions correspond to old macOS.

@sjackman
Copy link
Member Author

sjackman commented Feb 21, 2018

Version::NULL does mean not defined. You can use Version.null? to test whether a version is null.

@maxim-belkin
Copy link
Member

Version::NULL < something evaluates to true for any number and any string, whereas
Version::NULL > something evaluates to false even for negative numbers.
So, whenever there is MacOS.version < something comparison in a formula, it will be considered as true on Linux. So, unless it's augmented/prefixed by OS.mac? && this might lead to incorrect/undesired behavior or unnecessary patches being applied

$ uname -s
Linux

$ cd $(brew --repo homebrew/core)/Formula

$ grep -w "MacOS.version <" * | wc -l
312

$ grep -w "MacOS.version <" * | grep 'OS.mac?' | wc -l
14

@MikeMcQuaid
Copy link
Member

Version::NULL does mean not defined. You can use Version.null? to test whether a version is null.

I agree, let's keep with this approach.

@sjackman Does this patch enable an otherwise unpatched Homebrew/brew to brew readall on Homebrew/homebrew-core?

@ilovezfs
Copy link
Contributor

ilovezfs commented Feb 21, 2018

So, whenever there is MacOS.version < something comparison in a formula, it will be considered as true on Linux. So, unless it's augmented/prefixed by OS.mac? && this might lead to incorrect/undesired behavior or unnecessary patches being applied

This is caused by spaghetti formulae that attempt to interweave macOS and Linux support and/or reuse macOS formulae from Homebrew/homebrew-core.

@MikeMcQuaid
Copy link
Member

Thanks again @sjackman!

@sjackman sjackman deleted the macos-version branch February 21, 2018 16:34
@sjackman
Copy link
Member Author

sjackman commented Feb 21, 2018

Does this patch enable an otherwise unpatched Homebrew/brew to brew readall on Homebrew/homebrew-core?

Yes.

Thanks for merging, Mike!

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants