From ffc503f1d05f04470ce1bada59b9243fde855522 Mon Sep 17 00:00:00 2001 From: apainintheneck Date: Mon, 6 May 2024 23:34:23 -0700 Subject: [PATCH 1/3] Fix cask source file path loading issues There are two big changes here. Both have to do with how we want to load casks in different scenarios. One also is related to formulae. 1. Prevent loading casks & formulae outside of taps for specific commands. There are certain commands like `bump`, `bump-*-pr`, `livecheck` and `audit` where it really makes no sense to try and run things if the specified formulae or cask is not in a tap. A new `#to_formulae_and_casks_with_taps` method was added to the `CLI::NamedArgs` class to allow us to easily grab and validate formulae and casks from named arguments. 2. Always load the source file path when loading casks with the path loader. There was an edge case where all JSON cask files were being loaded without setting the source file path because most of the work was handed off to the API loader where that normally would make more sense. Now we set that when calling the API loader which solves the problem. This improves the user experience of people using the `--cache` and `fetch` commands in certain edge cases. Hopefully it makes the user experience a bit more consistent. A regression test was added for this point. --- Library/Homebrew/cask/cask_loader.rb | 12 ++++++++---- Library/Homebrew/cli/named_args.rb | 14 ++++++++++++++ Library/Homebrew/dev-cmd/audit.rb | 2 +- Library/Homebrew/dev-cmd/bump.rb | 8 +------- Library/Homebrew/dev-cmd/livecheck.rb | 8 +------- .../test/cask/cask_loader/from_path_loader_spec.rb | 9 +++++++++ 6 files changed, 34 insertions(+), 19 deletions(-) diff --git a/Library/Homebrew/cask/cask_loader.rb b/Library/Homebrew/cask/cask_loader.rb index b92b06e38f..288350d4d3 100644 --- a/Library/Homebrew/cask/cask_loader.rb +++ b/Library/Homebrew/cask/cask_loader.rb @@ -125,7 +125,9 @@ module Cask @content = path.read(encoding: "UTF-8") @config = config - return FromAPILoader.new(token, from_json: JSON.parse(@content)).load(config:) if path.extname == ".json" + if path.extname == ".json" + return FromAPILoader.new(token, from_json: JSON.parse(@content), path:).load(config:) + end begin instance_eval(content, path).tap do |cask| @@ -278,10 +280,11 @@ module Cask new("#{tap}/#{token}") end - sig { params(token: String, from_json: Hash).void } - def initialize(token, from_json: T.unsafe(nil)) + sig { params(token: String, from_json: Hash, path: T.nilable(Pathname)).void } + def initialize(token, from_json: T.unsafe(nil), path: nil) @token = token.sub(%r{^homebrew/(?:homebrew-)?cask/}i, "") - @path = CaskLoader.default_path(@token) + @sourcefile_path = path + @path = path || CaskLoader.default_path(@token) @from_json = from_json end @@ -290,6 +293,7 @@ module Cask cask_options = { loaded_from_api: true, + sourcefile_path: @sourcefile_path, source: JSON.pretty_generate(json_cask), config:, loader: self, diff --git a/Library/Homebrew/cli/named_args.rb b/Library/Homebrew/cli/named_args.rb index 834e073aa3..cdbfc29276 100644 --- a/Library/Homebrew/cli/named_args.rb +++ b/Library/Homebrew/cli/named_args.rb @@ -103,6 +103,20 @@ module Homebrew .map(&:freeze).freeze end + # Returns formulae and casks after validating that a tap is present for each of them. + def to_formulae_and_casks_with_taps + to_formulae_and_casks.each do |formula_or_cask| + case formula_or_cask + when Formula + odie "Formula #{formula_or_cask.name} is not in a tap!" unless formula_or_cask.tap + when Cask::Cask + odie "Cask #{formula_or_cask.token} is not in a tap!" unless formula_or_cask.tap + else + raise ArgumentError, "Expected a formula or a cask: #{formula_or_cask}" + end + end + end + def to_formulae_and_casks_and_unavailable(only: parent&.only_formula_or_cask, method: nil) @to_formulae_casks_unknowns ||= {} @to_formulae_casks_unknowns[method] = downcased_unique_named.map do |name| diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 3dde5d54da..98ebb3738f 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -158,7 +158,7 @@ module Homebrew "brew audit [name ...]" end - args.named.to_formulae_and_casks + args.named.to_formulae_and_casks_with_taps .partition { |formula_or_cask| formula_or_cask.is_a?(Formula) } end end diff --git a/Library/Homebrew/dev-cmd/bump.rb b/Library/Homebrew/dev-cmd/bump.rb index acd841eaf2..2c38e2addf 100644 --- a/Library/Homebrew/dev-cmd/bump.rb +++ b/Library/Homebrew/dev-cmd/bump.rb @@ -78,13 +78,7 @@ module Homebrew casks = args.formula? ? [] : Cask::Caskroom.casks formulae + casks elsif args.named.present? - if args.formula? - args.named.to_formulae - elsif args.cask? - args.named.to_casks - else - args.named.to_formulae_and_casks - end + args.named.to_formulae_and_casks_with_taps end formulae_and_casks = formulae_and_casks&.sort_by do |formula_or_cask| diff --git a/Library/Homebrew/dev-cmd/livecheck.rb b/Library/Homebrew/dev-cmd/livecheck.rb index 4868097656..b4566fa4d9 100644 --- a/Library/Homebrew/dev-cmd/livecheck.rb +++ b/Library/Homebrew/dev-cmd/livecheck.rb @@ -73,13 +73,7 @@ module Homebrew casks = args.formula? ? [] : Cask::Cask.all(eval_all: args.eval_all?) formulae + casks elsif args.named.present? - if args.formula? - args.named.to_formulae - elsif args.cask? - args.named.to_casks - else - args.named.to_formulae_and_casks - end + args.named.to_formulae_and_casks_with_taps elsif File.exist?(watchlist_path) begin names = Pathname.new(watchlist_path).read.lines diff --git a/Library/Homebrew/test/cask/cask_loader/from_path_loader_spec.rb b/Library/Homebrew/test/cask/cask_loader/from_path_loader_spec.rb index fccd5b580b..0322a42ba3 100644 --- a/Library/Homebrew/test/cask/cask_loader/from_path_loader_spec.rb +++ b/Library/Homebrew/test/cask/cask_loader/from_path_loader_spec.rb @@ -42,5 +42,14 @@ RSpec.describe Cask::CaskLoader::FromPathLoader do /invalid 'depends_on macos' value: unknown or unsupported macOS version:/) end end + + context "with a JSON cask file" do + let(:sourcefile_path) { TEST_FIXTURE_DIR/"cask/everything.json" } + + it "loads a cask with a source file path" do + cask = described_class.new(sourcefile_path).load(config: nil) + expect(cask.sourcefile_path).to eq sourcefile_path + end + end end end From 60149fb5de8912ffdb8b586bd64f0efbd8c599ff Mon Sep 17 00:00:00 2001 From: apainintheneck Date: Tue, 7 May 2024 21:17:57 -0700 Subject: [PATCH 2/3] cli/named_args: improve message for casks/formulae without taps This just improves the messaging in the `#to_formulae_and_casks_with_taps` method so that users have a better idea of what to try next. Ex. ``` Error: These formulae and casks are not in any locally installed taps! - discord - iterm2 - visual-studio-code - zstd You may need to run `brew tap` to install additional taps. ``` --- Library/Homebrew/cli/named_args.rb | 32 +++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/Library/Homebrew/cli/named_args.rb b/Library/Homebrew/cli/named_args.rb index cdbfc29276..7ab9fe1a0b 100644 --- a/Library/Homebrew/cli/named_args.rb +++ b/Library/Homebrew/cli/named_args.rb @@ -105,16 +105,30 @@ module Homebrew # Returns formulae and casks after validating that a tap is present for each of them. def to_formulae_and_casks_with_taps - to_formulae_and_casks.each do |formula_or_cask| - case formula_or_cask - when Formula - odie "Formula #{formula_or_cask.name} is not in a tap!" unless formula_or_cask.tap - when Cask::Cask - odie "Cask #{formula_or_cask.token} is not in a tap!" unless formula_or_cask.tap - else - raise ArgumentError, "Expected a formula or a cask: #{formula_or_cask}" + formulae_and_casks_with_taps, formulae_and_casks_without_taps = + to_formulae_and_casks.partition do |formula_or_cask| + case formula_or_cask + when Formula, Cask::Cask + formula_or_cask.tap&.installed? + else + # TODO: Refactor methods so that Sorbet can tell that this is unreachable. + raise ArgumentError, "Not a formula or cask: #{formula_or_cask.class}" + end end - end + + return formulae_and_casks_with_taps if formulae_and_casks_without_taps.blank? + + types = [] + types << "formulae" if formulae_and_casks_without_taps.any?(Formula) + types << "casks" if formulae_and_casks_without_taps.any?(Cask::Cask) + + odie <<~ERROR + These #{types.join(" and ")} are not in any locally installed taps! + + - #{formulae_and_casks_without_taps.sort_by(&:to_s).join("\n- ")} + + You may need to run `brew tap` to install additional taps. + ERROR end def to_formulae_and_casks_and_unavailable(only: parent&.only_formula_or_cask, method: nil) From 3dcb26072b1b96cbd3ae94f8361e7d0f983e630c Mon Sep 17 00:00:00 2001 From: apainintheneck Date: Wed, 8 May 2024 18:13:35 -0700 Subject: [PATCH 3/3] cli/named_args: address review feedback - use `T.cast` to avoid unnecessary error handling with Sorbet - avoid using dashes in terminal output to make piping easier --- Library/Homebrew/cli/named_args.rb | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/Library/Homebrew/cli/named_args.rb b/Library/Homebrew/cli/named_args.rb index 7ab9fe1a0b..c92693a87e 100644 --- a/Library/Homebrew/cli/named_args.rb +++ b/Library/Homebrew/cli/named_args.rb @@ -107,13 +107,7 @@ module Homebrew def to_formulae_and_casks_with_taps formulae_and_casks_with_taps, formulae_and_casks_without_taps = to_formulae_and_casks.partition do |formula_or_cask| - case formula_or_cask - when Formula, Cask::Cask - formula_or_cask.tap&.installed? - else - # TODO: Refactor methods so that Sorbet can tell that this is unreachable. - raise ArgumentError, "Not a formula or cask: #{formula_or_cask.class}" - end + T.cast(formula_or_cask, T.any(Formula, Cask::Cask)).tap&.installed? end return formulae_and_casks_with_taps if formulae_and_casks_without_taps.blank? @@ -125,7 +119,7 @@ module Homebrew odie <<~ERROR These #{types.join(" and ")} are not in any locally installed taps! - - #{formulae_and_casks_without_taps.sort_by(&:to_s).join("\n- ")} + #{formulae_and_casks_without_taps.sort_by(&:to_s).join("\n ")} You may need to run `brew tap` to install additional taps. ERROR