Cask: require macOS #3325

maxim-belkin · 2017-10-16T17:44:47Z

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 tests with your changes locally?

the hierarchy of cask commands requires macOS. Moving it under extend/os makes Homebrew/brew compatible with other Operating Systems.

maxim-belkin · 2017-10-16T17:45:50Z

@sjackman, please review

sjackman · 2017-10-16T18:23:34Z

I'd be inclined to put this file under os/mac/cmd/cask.rb rather than extend/, and modify the logic that searches for commands to search the directory os/mac/cmd/ before cmd/ on macOS.

sjackman · 2017-10-16T18:24:07Z

Library/Homebrew/extend/os/cmd/cask.rb

+if OS.mac?
+  require "extend/os/mac/cmd/cask"
+else
+  odie "Homebrew Cask is supported on macOS only."


I'd suggest…

The cask command requires macOS.

maybe I'm not correct, but it seems weird for a command to require an operating system, no?
A command/program can require another program/command but it is either supported or not supported by the operating system.

Library/Homebrew/requirements/macos_requirement.rb says macOS is required.

I think that requiring an operating system is fine. e.g. "iTunes requires either macOS or Windows."

Library/Homebrew/requirements/macos_requirement.rb says macOS is required.

$ git blame Library/Homebrew/requirements/macos_requirement.rb 8f728d3a (Shaun Jackman 2017-02-25 10:17:25 -0800 1) class MacOSRequirement < Requirement 8f728d3a (Shaun Jackman 2017-02-25 10:17:25 -0800 2) fatal true 8f728d3a (Shaun Jackman 2017-02-25 10:17:25 -0800 3) 8f728d3a (Shaun Jackman 2017-02-25 10:17:25 -0800 4) satisfy(build_env: false) { OS.mac? } 8f728d3a (Shaun Jackman 2017-02-25 10:17:25 -0800 5) 8f728d3a (Shaun Jackman 2017-02-25 10:17:25 -0800 6) def message 8f728d3a (Shaun Jackman 2017-02-25 10:17:25 -0800 7) "macOS is required." 8f728d3a (Shaun Jackman 2017-02-25 10:17:25 -0800 8) end 8f728d3a (Shaun Jackman 2017-02-25 10:17:25 -0800 9) end

:)

There is no Library/Homebrew/requirements/macos_requirement.rb in Homebrew

Ah, hasn't been merged yet. See #3270

I suggest moving this file to os/cmd/cask.rb

sjackman

On second thought, modifying the command search logic to search os/#{os}/cmd seems like overkill. What you have is good. However, I'd suggest putting everything under os/cmd rather than extend/os/cmd. The directory extend is intended for extending (monkey-patching) classes.

sjackman · 2017-10-17T06:23:45Z

Library/Homebrew/cmd/cask.rb

-    Hbc::CLI.run(*ARGV)
-  end
-end
+require "extend/os/cmd/cask"


require "os/cmd/cask"

sjackman · 2017-10-17T06:24:57Z

Library/Homebrew/extend/os/cmd/cask.rb

@@ -0,0 +1,5 @@
+if OS.mac?
+  require "extend/os/mac/cmd/cask"


require "os/mac/cmd/cask"

sjackman · 2017-10-17T06:25:19Z

Library/Homebrew/extend/os/mac/cmd/cask.rb

+  def cask
+    Hbc::CLI.run(*ARGV)
+  end
+end


I suggest moving this file to os/mac/cmd/cask.rb

sjackman · 2017-10-17T06:25:42Z

Library/Homebrew/extend/os/cmd/cask.rb

+if OS.mac?
+  require "extend/os/mac/cmd/cask"
+else
+  odie "Homebrew Cask is supported on macOS only."


I suggest moving this file to os/cmd/cask.rb

maxim-belkin · 2017-10-17T08:39:17Z

done!

sjackman

Looks good to me. Thanks, Maxim.

MikeMcQuaid · 2017-10-18T09:49:48Z

the hierarchy of cask commands requires macOS.

What happens if you run brew cask as-is?

MikeMcQuaid · 2017-10-18T09:50:26Z

Library/Homebrew/cmd/cask.rb

-  module_function
-
-  def cask
-    Hbc::CLI.run(*ARGV)


I think odie "The cask command requires macOS." if OS.linux? above this line could be sufficient for this whole PR.

This is literally what Linuxbrew does right now: https://github.com/Linuxbrew/brew/blob/master/Library/Homebrew/cmd/cask.rb#L8

I will submit a PR that ports Linuxbrew's changes. Also, I believe it should be unless OS.mac? because one can't expect cask to work anywhere else, right?

MikeMcQuaid · 2017-10-18T09:50:40Z

@reitermarkus I'd love your thoughts on this.

reitermarkus · 2017-10-18T11:36:45Z

I think this is a bit overkill, and I agree with #3325 (comment).

maxim-belkin · 2017-10-18T13:23:41Z

What happens if you run brew cask as-is?

# Removed current Linuxbrew's `odie` guard
$ brew cask
==> Tapping caskroom/cask
Cloning into '/home/linuxbrew/.linuxbrew/Library/Taps/caskroom/homebrew-cask'...
remote: Counting objects: 3854, done.
remote: Compressing objects: 100% (3838/3838), done.
remote: Total 3854 (delta 30), reused 547 (delta 12), pack-reused 0
Receiving objects: 100% (3854/3854), 1.32 MiB | 0 bytes/s, done.
Resolving deltas: 100% (30/30), done.
Checking connectivity... done.
Tapped 0 formulae (3,862 files, 4.2MB)
brew-cask provides a friendly homebrew-style CLI workflow for the
administration of macOS applications distributed as binaries.

Commands:

    --version              displays the Homebrew-Cask version
    audit                  verifies installability of Casks
    cat                    dump raw source of the given Cask to the standard output
    cleanup                cleans up cached downloads and tracker symlinks
    create                 creates the given Cask and opens it in an editor
    doctor                 checks for configuration issues
    edit                   edits the given Cask
    fetch                  downloads remote application files to local cache
    home                   opens the homepage of the given Cask
    info                   displays information about the given Cask
    install                installs the given Cask
    list                   with no args, lists installed Casks; given installed Casks, lists staged files
    outdated               list the outdated installed Casks
    reinstall              reinstalls the given Cask
    search                 searches all known Casks
    style                  checks Cask style using RuboCop
    uninstall              uninstalls the given Cask
    zap                    zaps all files associated with the given Cask

See also "man brew-cask"
Error: Unknown command: 

[linuxbrew@6c72c34c91f9] [Wed Oct 18 13:22:03] [Last Exit Code: 1]
~/.linuxbrew/Library/Homebrew
(master) $ brew cask cat
Error: This command requires a Cask token.

[linuxbrew@6c72c34c91f9] [Wed Oct 18 13:22:09] [Last Exit Code: 1]
~/.linuxbrew/Library/Homebrew
(master) $ brew cask info
Error: This command requires a Cask token.

MikeMcQuaid · 2017-10-20T09:09:16Z

(master) $ brew cask cat
Error: This command requires a Cask token.

What happens if you specify an actual cask, though. brew cask info 1password, for instance?

maxim-belkin · 2017-10-20T13:22:31Z

(master) $ brew cask info 1password
1password: 3.8.22
https://1password.com/
Not installed
From: https://github.com/caskroom/homebrew-cask/blob/master/Casks/1password.rb
==> Name
1Password
==> Artifacts
1Password.app (App)


(master) $ brew cask install 1password
==> Creating Caskroom at /home/linuxbrew/.linuxbrew/Caskroom
Error: No such file or directory - /usr/sbin/chown
Follow the instructions here:
  https://github.com/caskroom/homebrew-cask#reporting-bugs
/home/linuxbrew/.linuxbrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/open3.rb:199:in `spawn'
/home/linuxbrew/.linuxbrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/open3.rb:199:in `popen_run'
/home/linuxbrew/.linuxbrew/Library/Homebrew/vendor/portable-ruby/2.3.3/lib/ruby/2.3.0/open3.rb:95:in `popen3'
/home/linuxbrew/.linuxbrew/Library/Homebrew/cask/lib/hbc/system_command.rb:80:in `each_output_line'
/home/linuxbrew/.linuxbrew/Library/Homebrew/cask/lib/hbc/system_command.rb:25:in `run!'
/home/linuxbrew/.linuxbrew/Library/Homebrew/cask/lib/hbc/system_command.rb:14:in `run'
/home/linuxbrew/.linuxbrew/Library/Homebrew/cask/lib/hbc/caskroom.rb:15:in `ensure_caskroom_exists'
/home/linuxbrew/.linuxbrew/Library/Homebrew/cask/lib/hbc.rb:40:in `init'
/home/linuxbrew/.linuxbrew/Library/Homebrew/compat/hbc.rb:18:in `init'
/home/linuxbrew/.linuxbrew/Library/Homebrew/cask/lib/hbc/cli.rb:166:in `run'
/home/linuxbrew/.linuxbrew/Library/Homebrew/cask/lib/hbc/cli.rb:131:in `run'
/home/linuxbrew/.linuxbrew/Library/Homebrew/cmd/cask.rb:8:in `cask'
/home/linuxbrew/.linuxbrew/Library/Homebrew/brew.rb:99:in `<main>'

MikeMcQuaid · 2017-10-20T13:27:59Z

(master) $ brew cask install 1password

I think this is the command that could reasonably be interpreted to not work on Linux. I think it's a good thing that brew cask info 1password can work on Linux.

stale · 2017-11-10T13:54:17Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

maxim-belkin · 2017-11-10T14:06:10Z

Closing per #3325 (comment)

sjackman suggested changes Oct 16, 2017

View reviewed changes

sjackman suggested changes Oct 17, 2017

View reviewed changes

sjackman approved these changes Oct 17, 2017

View reviewed changes

MikeMcQuaid requested changes Oct 18, 2017

View reviewed changes

cask: macOS-only command

99bd6b6

maxim-belkin force-pushed the cask-linux branch from 11662dc to 99bd6b6 Compare October 18, 2017 13:27

maxim-belkin changed the title ~~Cask linux~~ Cask: require macOS Oct 18, 2017

iMichka mentioned this pull request Oct 23, 2017

brew cask not supported: should be reflected in search & command-outputs, or implement Linuxbrew/brew#512

Closed

stale bot added the stale No recent activity label Nov 10, 2017

maxim-belkin closed this Nov 10, 2017

sjackman mentioned this pull request Nov 22, 2017

Installing casks is supported only on macOS #3468

Merged

5 tasks

Homebrew locked and limited conversation to collaborators May 4, 2018

maxim-belkin deleted the cask-linux branch August 9, 2020 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cask: require macOS #3325

Cask: require macOS #3325

maxim-belkin commented Oct 16, 2017

maxim-belkin commented Oct 16, 2017

sjackman commented Oct 16, 2017 •

edited

Loading

sjackman Oct 16, 2017

maxim-belkin Oct 16, 2017

sjackman Oct 16, 2017

sjackman Oct 16, 2017

maxim-belkin Oct 16, 2017

sjackman Oct 16, 2017

sjackman Oct 17, 2017

sjackman left a comment

sjackman Oct 17, 2017

sjackman Oct 17, 2017

sjackman Oct 17, 2017

sjackman Oct 17, 2017

maxim-belkin commented Oct 17, 2017

sjackman left a comment

MikeMcQuaid commented Oct 18, 2017

MikeMcQuaid Oct 18, 2017

maxim-belkin Oct 18, 2017

MikeMcQuaid commented Oct 18, 2017

reitermarkus commented Oct 18, 2017

maxim-belkin commented Oct 18, 2017

MikeMcQuaid commented Oct 20, 2017

maxim-belkin commented Oct 20, 2017

MikeMcQuaid commented Oct 20, 2017

stale bot commented Nov 10, 2017

maxim-belkin commented Nov 10, 2017

Cask: require macOS #3325

Cask: require macOS #3325

Conversation

maxim-belkin commented Oct 16, 2017

maxim-belkin commented Oct 16, 2017

sjackman commented Oct 16, 2017 • 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

sjackman 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

maxim-belkin commented Oct 17, 2017

sjackman left a comment

Choose a reason for hiding this comment

MikeMcQuaid commented Oct 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Oct 18, 2017

reitermarkus commented Oct 18, 2017

maxim-belkin commented Oct 18, 2017

MikeMcQuaid commented Oct 20, 2017

maxim-belkin commented Oct 20, 2017

MikeMcQuaid commented Oct 20, 2017

stale bot commented Nov 10, 2017

maxim-belkin commented Nov 10, 2017

sjackman commented Oct 16, 2017 •

edited

Loading