List cask full-names #2878

wendorf · 2017-07-09T02:35:47Z

brew cask list supports the --full-name flag which will include the tap name for casks not part of the core caskroom/cask tap. For example, if cask "foo-beta" is installed from the caskroom/versions cask, brew cask list --full-name will report the name as "caskroom/versions/foo-beta".

This will be especially helpful with brew bundle cleanup, which currently cannot correctly identify a cask installed from a non-core tap.

reitermarkus · 2017-07-09T03:10:59Z

Library/Homebrew/test/cask/cli/list_spec.rb

@@ -14,6 +14,22 @@
    EOS
  end

+  it "lists full names" do
+    casks = %w[local-caffeine external-caskroom/spec/external-cask local-transmission].map { |c| Hbc::CaskLoader.load(c) }


Make this multiple lines.

Addressed in the latest commit

reitermarkus · 2017-07-09T03:21:03Z

Library/Homebrew/formula.rb

@@ -1364,6 +1364,18 @@ def self.clear_installed_formulae_cache
    @installed = nil
  end

+  def self.comparison


Since this is used for both Formulae and Casks, this should live somewhere else, and have a self-explanatory name.

Also, maybe add a test for this. This could even be simplified to ->(a) { [a.include?("/"), a] } I think, but I'm not sure without a test.

There's no location that jumps out to me as more appropriate. My first thought is to add formula_comparer.rb, but this still suffers from the "this is used by formulae and casks" issue and I'm hesitant to add another file to ./Library/Homebrew. Do you have a location in mind? Is there a common name for "formulae and casks"? Maybe "token_comparer.rb" or "full_token_comparer.rb"?

Agreed this should live elsewhere and additionally a more descriptive name would be good (tap_and_name_comparison or similar)

Thanks for the thoughts! I've moved this to utils/comparer.rb as Comparer.tap_and_name and tested it.

I think Comparer is too generic a class name and it doesn't make sense having a single method there. tap_and_name_comparison in Utils makes more sense.

Sounds good. It's now in utils/comparison.rb, but extends Utils instead of creating a new module.

Sorry to not be clearer: please put it in utils.rb, thanks.

Whoops, sorry about this misunderstanding. Just pushed out another change to move it there as a global method.

reitermarkus · 2017-07-09T03:26:55Z

Library/Homebrew/test/support/helper/spec/shared_context/homebrew_cask.rb

@@ -18,6 +18,7 @@

 RSpec.shared_context "Homebrew-Cask" do
  around(:each) do |example|
+    external_caskroom_tap = Tap.fetch("external-caskroom", "spec")


I'd name this third-party instead of external-caskroom.

Addressed in the latest commit

reitermarkus · 2017-07-09T03:35:18Z

Library/Homebrew/cask/lib/hbc/cask.rb

+        Hbc.default_tap
+      elsif @sourcefile_path.to_s =~ HOMEBREW_TAP_PATH_REGEX
+        Tap.fetch(Regexp.last_match(1), Regexp.last_match(2))
+      end


I don't think this will ever work this way with brew cask list. For installed Casks, the sourcefile_path will always be the one in the Caskroom metadata directory, hence will never contain any information about the Tap.

I found this to be required and correct for third-party taps. For example, the sourcefile_path for caskroom/homebrew-versions/microsoft-remote-desktop-beta is "/usr/local/Homebrew/Library/Taps/caskroom/homebrew-versions/Casks/microsoft-remote-desktop-beta.rb" in my environment. Regexp.last_match(1) is "caskroom" and Regexp.last_match(2) is "homebrew-versions".

Maybe this should be set from CaskLoader instead.

Sure thing—just pushed that change. It's a little funky to me now that the cask_spec is also testing CaskLoader logic, but it was doing that before so I think it's fine.

MikeMcQuaid

LGTM once @reitermarkus has 👍d Cask bits.

reitermarkus · 2017-07-15T12:21:43Z

Library/Homebrew/cask/lib/hbc/cask_loader.rb

@@ -31,6 +31,12 @@ def initialize(path)

        @token = path.basename(".rb").to_s
        @path = path
+
+        @tap ||= if @path == CaskLoader.default_path(@token)


What I meant here was that it should only be set by the FromTapLoader, since this is the only case where we can be sure of the Tap. And Cask#full_name will still default to Hbc.default_tap if it is nil.

Unfortunately, this would make --full-name not work, since the locally-installed casks are not loaded with FromTapLoader.

The two conditionals use full paths (default_path resolves to something like /usr/local/Homebrew/Library/Taps/caskroom/homebrew-cask/Casks/foo.rb, and HOMEBREW_TAP_PATH_REGEX to something like (?-mix:\\/usr\\/local\\/Homebrew\\/Library\\/Taps\\/([\\w-]+)\\/([\\w-]+)\\/(.*))), so I believe we can be quite confident that this is the correct tap. This is the same logic that is used in Formula#initialize, which is why I originally felt comfortable putting it directly in Hbc::Cask#initialize instead of FromPathLoader.

I tried out changing Hbc::Scopes::installed (used by the brew cask list command) to fetch casks by tap path, thus using FromTapLoader (see the patch), but failed the rspec ./test/cask/scopes_spec.rb:21 # Hbc::Scopes installed optimizes performance by resolving to a fully qualified path before calling Hbc::CaskLoader.load test. It looks like this optimization was put there for good reason, so I'm hesitant to change that behavior.

@reitermarkus Is there another option you think I should consider? Or do you think my existing approach is okay since the path checking is checking explicitly for the Homebrew tap install locations? I think path checking is the only way to do it without making seemingly-undesirable changes to how brew cask list gets its casks.

@reitermarkus Thanks for your work on #2965 , which makes this part unnecessary. I've rebased and removed the unneeded code, so now this doesn't need to touch CaskLoader or the Cask constructor at all.

stale · 2017-08-26T18:41:10Z

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

reitermarkus · 2017-08-27T11:43:04Z

Library/Homebrew/test/support/helper/spec/shared_context/homebrew_cask.rb

@@ -18,6 +18,7 @@

 RSpec.shared_context "Homebrew-Cask" do
  around(:each) do |example|
+    third_party_tap = Tap.fetch("third-party", "spec")


Maybe call this "tap" instead of "spec".

reitermarkus · 2017-09-10T04:46:12Z

@wendorf, one small change, and this should be good to go.

`brew cask list` supports the `--full-name` flag which will include the tap name for casks not part of the core caskroom/cask tap. For example, if cask "foo-beta" is installed from the caskroom/versions cask, `brew cask list --full-name` will report the name as "caskroom/versions/foo-beta".

wendorf · 2017-09-16T17:31:41Z

@reitermarkus Thanks for the additional feedback. Just did another rebase and push, so hopefully this is ready to be merged!

reitermarkus · 2017-09-17T02:04:09Z

Thanks, @wendorf!

This was referenced Jul 9, 2017

Document list --full-name flag Homebrew/homebrew-cask#36298

Merged

CaskDumper uses cask full-name Homebrew/homebrew-bundle#278

Merged

reitermarkus requested changes Jul 9, 2017

View reviewed changes

wendorf force-pushed the brew_cask_list_full-name branch 5 times, most recently from 9b1a022 to 0a6d0a3 Compare July 10, 2017 14:45

MikeMcQuaid approved these changes Jul 12, 2017

View reviewed changes

wendorf force-pushed the brew_cask_list_full-name branch from 0a6d0a3 to fa7819f Compare July 13, 2017 01:32

reitermarkus requested changes Jul 15, 2017

View reviewed changes

wendorf force-pushed the brew_cask_list_full-name branch from fa7819f to d3cab95 Compare July 20, 2017 03:24

reitermarkus mentioned this pull request Jul 29, 2017

Detect Tap in CaskLoader. #2965

Merged

5 tasks

wendorf force-pushed the brew_cask_list_full-name branch from d3cab95 to 12a4d34 Compare August 5, 2017 18:09

stale bot added the stale No recent activity label Aug 26, 2017

reitermarkus reviewed Aug 27, 2017

View reviewed changes

stale bot removed the stale No recent activity label Aug 27, 2017

wendorf force-pushed the brew_cask_list_full-name branch from 12a4d34 to acf1b27 Compare September 16, 2017 17:00

reitermarkus approved these changes Sep 17, 2017

View reviewed changes

reitermarkus merged commit dbc4a38 into Homebrew:master Sep 17, 2017

Homebrew locked and limited conversation to collaborators May 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List cask full-names #2878

List cask full-names #2878

wendorf commented Jul 9, 2017

reitermarkus Jul 9, 2017

wendorf Jul 9, 2017

reitermarkus Jul 9, 2017

wendorf Jul 9, 2017

MikeMcQuaid Jul 9, 2017

wendorf Jul 9, 2017

MikeMcQuaid Jul 9, 2017

wendorf Jul 10, 2017

MikeMcQuaid Jul 10, 2017

wendorf Jul 10, 2017

reitermarkus Jul 9, 2017

wendorf Jul 9, 2017

reitermarkus Jul 9, 2017

wendorf Jul 9, 2017

reitermarkus Jul 12, 2017

wendorf Jul 13, 2017

MikeMcQuaid left a comment

reitermarkus Jul 15, 2017

wendorf Jul 15, 2017

wendorf Jul 20, 2017

wendorf Aug 5, 2017

stale bot commented Aug 26, 2017

reitermarkus Aug 27, 2017

reitermarkus commented Sep 10, 2017

wendorf commented Sep 16, 2017

reitermarkus commented Sep 17, 2017

List cask full-names #2878

List cask full-names #2878

Conversation

wendorf commented Jul 9, 2017

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

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

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

stale bot commented Aug 26, 2017

Choose a reason for hiding this comment

reitermarkus commented Sep 10, 2017

wendorf commented Sep 16, 2017

reitermarkus commented Sep 17, 2017