[Linux] Don't prompt users to `brew cask install` #6156

issyl0 · 2019-05-21T11:38:19Z

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?

I tried to install keybase, thinking I'd get the CLI, but no. On Linux,
casks don't work, yet I was still prompted to brew cask install keybase. When I tried that (just to make sure), I got the "casks are
only supported on MacOS" error.
This change makes it so we don't prompt people to install casks if
they're on platforms other than MacOS.

Before:

$ brew install keybase
Error: No available formula with the name "keybase"
Found a cask named "keybase" instead.

After:

Error: No available formula with the name "keybase"
==> Searching for a previously deleted formula (in the last month)...
Warning: homebrew/core is shallow clone. To get complete history run:
  git -C "$(brew --repo homebrew/core)" fetch --unshallow
Error: No previously deleted formula found.
==> Searching for similarly named formulae...
Error: No similarly named formulae found.
==> Searching taps...
==> Searching taps on GitHub...
Error: No formulae found in taps.

which is long, but still better messaging than "found a cask".

This is a draft PR. Things still to do include:

Tests. I can't find many places in the tests that differentiate between OSes. I might need some help here. 😬
This doesn't account for the cases where people try to install blacklisted formulae on Linux, for which the help text hardcodes "brew cask install". I'd welcome ideas on how to do this, because I couldn't see how to break the massive case statement up without excluding some formulae from either, using the common unless OS.mac? condition.

(I couldn't see anything in the contributing guide about whether draft PRs are good or not, so feel free to close this for that reason, or if this is not a feature you'd support!)

Library/Homebrew/missing_formula.rb

MikeMcQuaid · 2019-05-21T12:16:00Z

Library/Homebrew/missing_formula.rb

@@ -198,6 +200,7 @@ def deleted_reason(name, silent: false)

      def cask_reason(name, silent: false, show_info: false)
        return if silent
+        return unless OS.mac?


Ideally this would be a no-op method and have this method implementation moved to Library/Homebrew/extend/os/mac/missing_formula.rb instead.

Sorry, I'm not sure I understand what you mean?

Check out the other files in Library/Homebrew/extend/os/mac. We generally prefer taking that approach of extending the initial class to adding OS.mac? checks around the codebase.

Thanks. I've done this in 5bf5ea2 which I'll squash at the end if everything's OK.

MikeMcQuaid · 2019-05-23T07:28:44Z

Library/Homebrew/extend/os/linux/missing_formula.rb

+  module MissingFormula
+    class << self
+      def cask_reason(*)
+        # Casks aren't supported on non-MacOS.


Sorry, I should have explained this better but I was hoping for the reverse e.g. in os/mac/missing_formula.rb. Thanks!

Ah! 😳To be sure I understand the intended outcome for "reversing" this, I have a couple of questions as there are several approaches going around in my head:

The current missing_formula.rb cask_reason code is moved into the extend/os/mac/missing_formula.rb so that it's only called on MacOS, and for the original method call to be scoped to OS.mac??

The extend/os/mac/missing_formula.rb is changed to return if OS.linux?? But the Mac extend file is only required if already on MacOS, so Linux would never get there.

Or, something else?

The current missing_formula.rb cask_reason code is moved into the extend/os/mac/missing_formula.rb so that it's only called on MacOS

Yup, this, thanks!

Thanks for confirming. I've tested manually that it still works as intended on both MacOS and Linux, and pushed the new changes.

issyl0 · 2019-05-24T20:24:37Z

I tried to write tests for this new behaviour, then I realised that cask_reason is a cask method so won't run on non-MacOS anyway. So I'm not changing any behaviour test-wise, which is why I wasn't seeing any changes.

In terms of what's left here, half of this still depends on the linuxbrew-core PR, otherwise I think that's all the review comments dealt with?

Thanks for your patience!

MikeMcQuaid

Almost there, thanks again!

Library/Homebrew/missing_formula.rb

- I tried to install `keybase`, thinking I'd get the CLI. On Linux, casks don't work, yet I was still prompted to `brew cask install keybase`. When I tried that (just to make sure), I got the "casks are only supported on MacOS" error. - This change makes it so we don't prompt people to install casks if they're on platforms other than MacOS. Before: ``` ╭─issyl0@grus /home/linuxbrew/.linuxbrew/Homebrew ‹master› ╰─ $ brew install keybase Error: No available formula with the name "keybase" Found a cask named "keybase" instead. ``` After: ``` Error: No available formula with the name "keybase" ==> Searching for a previously deleted formula (in the last month)... Warning: homebrew/core is shallow clone. To get complete history run: git -C "$(brew --repo homebrew/core)" fetch --unshallow Error: No previously deleted formula found. ==> Searching for similarly named formulae... Error: No similarly named formulae found. ==> Searching taps... ==> Searching taps on GitHub... Error: No formulae found in taps. ```

issyl0 · 2019-05-25T14:47:33Z

Thanks, @MikeMcQuaid. I've pushed changes for your last two comments. :-)

MikeMcQuaid · 2019-05-26T08:47:58Z

Perfect, thanks again for the great work @issyl0, you rock!

MikeMcQuaid reviewed May 21, 2019

View reviewed changes

Library/Homebrew/missing_formula.rb Outdated Show resolved Hide resolved

MikeMcQuaid reviewed May 21, 2019

View reviewed changes

issyl0 force-pushed the dont_show_missing_formula_cask_info_on_linux branch 2 times, most recently from 5bf5ea2 to ce54f07 Compare May 22, 2019 22:09

MikeMcQuaid reviewed May 23, 2019

View reviewed changes

issyl0 force-pushed the dont_show_missing_formula_cask_info_on_linux branch 4 times, most recently from b1363fb to 9ecdc7f Compare May 24, 2019 20:22

MikeMcQuaid reviewed May 25, 2019

View reviewed changes

Library/Homebrew/missing_formula.rb Show resolved Hide resolved

Library/Homebrew/missing_formula.rb Outdated Show resolved Hide resolved

issyl0 force-pushed the dont_show_missing_formula_cask_info_on_linux branch from 9ecdc7f to 4a5b026 Compare May 25, 2019 14:46

MikeMcQuaid approved these changes May 26, 2019

View reviewed changes

MikeMcQuaid merged commit 515aef9 into Homebrew:master May 26, 2019

issyl0 deleted the dont_show_missing_formula_cask_info_on_linux branch May 26, 2019 08:48

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Linux] Don't prompt users to `brew cask install` #6156

[Linux] Don't prompt users to `brew cask install` #6156

issyl0 commented May 21, 2019

MikeMcQuaid May 21, 2019

issyl0 May 21, 2019

MikeMcQuaid May 21, 2019

issyl0 May 21, 2019 •

edited

Loading

MikeMcQuaid May 23, 2019

issyl0 May 23, 2019

MikeMcQuaid May 24, 2019

issyl0 May 24, 2019

issyl0 commented May 24, 2019

MikeMcQuaid left a comment

issyl0 commented May 25, 2019

MikeMcQuaid commented May 26, 2019

[Linux] Don't prompt users to brew cask install #6156

[Linux] Don't prompt users to brew cask install #6156

Conversation

issyl0 commented May 21, 2019

MikeMcQuaid May 21, 2019

Choose a reason for hiding this comment

issyl0 May 21, 2019

Choose a reason for hiding this comment

MikeMcQuaid May 21, 2019

Choose a reason for hiding this comment

issyl0 May 21, 2019 • edited Loading

Choose a reason for hiding this comment

MikeMcQuaid May 23, 2019

Choose a reason for hiding this comment

issyl0 May 23, 2019

Choose a reason for hiding this comment

MikeMcQuaid May 24, 2019

Choose a reason for hiding this comment

issyl0 May 24, 2019

Choose a reason for hiding this comment

issyl0 commented May 24, 2019

MikeMcQuaid left a comment

Choose a reason for hiding this comment

issyl0 commented May 25, 2019

MikeMcQuaid commented May 26, 2019

[Linux] Don't prompt users to `brew cask install` #6156

[Linux] Don't prompt users to `brew cask install` #6156

issyl0 May 21, 2019 •

edited

Loading