From 16e605e05674548633a7b636e2a869a2ad806ae1 Mon Sep 17 00:00:00 2001 From: apainintheneck Date: Mon, 1 Jul 2024 18:41:53 -0700 Subject: [PATCH] Load tap migration renames from API with short names This is a follow-up to 484498e. I added loading for tap migration renames from the API but it apparently only worked for full names. There was a bug in both the code and the tests which prevented loading by short names. This fixes those bugs so everything should be good now. --- Library/Homebrew/cask/cask_loader.rb | 6 +- Library/Homebrew/formulary.rb | 6 +- .../cask/cask_loader/from_api_loader_spec.rb | 82 ++-- Library/Homebrew/test/formulary_spec.rb | 410 +++++++++--------- 4 files changed, 245 insertions(+), 259 deletions(-) diff --git a/Library/Homebrew/cask/cask_loader.rb b/Library/Homebrew/cask/cask_loader.rb index e8a8303163..812040637b 100644 --- a/Library/Homebrew/cask/cask_loader.rb +++ b/Library/Homebrew/cask/cask_loader.rb @@ -458,14 +458,14 @@ module Cask # If it exists in the default tap, never treat it as ambiguous with another tap. if (core_cask_tap = CoreCaskTap.instance).installed? && - (loader = super("#{core_cask_tap}/#{token}", warn:))&.path&.exist? - return loader + (core_cask_loader = super("#{core_cask_tap}/#{token}", warn:))&.path&.exist? + return core_cask_loader end loaders = Tap.select { |tap| tap.installed? && !tap.core_cask_tap? } .filter_map { |tap| super("#{tap}/#{token}", warn:) } .uniq(&:path) - .select { |tap| tap.path.exist? } + .select { |loader| loader.is_a?(FromAPILoader) || loader.path.exist? } case loaders.count when 1 diff --git a/Library/Homebrew/formulary.rb b/Library/Homebrew/formulary.rb index 8d86f13e72..1a7afbd1e8 100644 --- a/Library/Homebrew/formulary.rb +++ b/Library/Homebrew/formulary.rb @@ -825,14 +825,14 @@ module Formulary # If it exists in the default tap, never treat it as ambiguous with another tap. if (core_tap = CoreTap.instance).installed? && - (loader = super("#{core_tap}/#{name}", warn:))&.path&.exist? - return loader + (core_loader = super("#{core_tap}/#{name}", warn:))&.path&.exist? + return core_loader end loaders = Tap.select { |tap| tap.installed? && !tap.core_tap? } .filter_map { |tap| super("#{tap}/#{name}", warn:) } .uniq(&:path) - .select { |tap| tap.path.exist? } + .select { |loader| loader.is_a?(FromAPILoader) || loader.path.exist? } case loaders.count when 1 diff --git a/Library/Homebrew/test/cask/cask_loader/from_api_loader_spec.rb b/Library/Homebrew/test/cask/cask_loader/from_api_loader_spec.rb index 624c1f3cbf..43f2314c22 100644 --- a/Library/Homebrew/test/cask/cask_loader/from_api_loader_spec.rb +++ b/Library/Homebrew/test/cask/cask_loader/from_api_loader_spec.rb @@ -1,16 +1,16 @@ # frozen_string_literal: true RSpec.describe Cask::CaskLoader::FromAPILoader, :cask do - shared_context "with API setup" do |new_token| - let(:token) { new_token } - let(:cask_from_source) { Cask::CaskLoader.load(token) } + shared_context "with API setup" do |local_token| + let(:api_token) { "#{local_token}-api" } + let(:cask_from_source) { Cask::CaskLoader.load(local_token) } let(:cask_json) do hash = cask_from_source.to_hash_with_variations json = JSON.pretty_generate(hash) JSON.parse(json) end - let(:casks_from_api_hash) { { cask_json["token"] => cask_json.except("token") } } - let(:api_loader) { described_class.new(token, from_json: cask_json) } + let(:casks_from_api_hash) { { api_token => cask_json.except("token") } } + let(:api_loader) { described_class.new(api_token, from_json: cask_json) } before do allow(Homebrew::API::Cask) @@ -26,7 +26,7 @@ RSpec.describe Cask::CaskLoader::FromAPILoader, :cask do context "when not using the API", :no_api do it "returns false" do - expect(described_class.try_new(token)).to be_nil + expect(described_class.try_new(api_token)).to be_nil end end @@ -36,19 +36,19 @@ RSpec.describe Cask::CaskLoader::FromAPILoader, :cask do end it "returns a loader for valid token" do - expect(described_class.try_new(token)) + expect(described_class.try_new(api_token)) .to be_a(described_class) - .and have_attributes(token:) + .and have_attributes(token: api_token) end it "returns a loader for valid full name" do - expect(described_class.try_new("homebrew/cask/#{token}")) + expect(described_class.try_new("homebrew/cask/#{api_token}")) .to be_a(described_class) - .and have_attributes(token:) + .and have_attributes(token: api_token) end it "returns nil for full name with invalid tap" do - expect(described_class.try_new("homebrew/foo/#{token}")).to be_nil + expect(described_class.try_new("homebrew/foo/#{api_token}")).to be_nil end context "with core tap migration renames" do @@ -62,100 +62,92 @@ RSpec.describe Cask::CaskLoader::FromAPILoader, :cask do FileUtils.rm_rf foo_tap.path end - it "returns the core cask if the short name clashes with a tap migration rename" do - (foo_tap.path/"tap_migrations.json").write <<~JSON - { "#{token}": "homebrew/cask/#{token}-v2" } - JSON - - expect(Cask::CaskLoader::FromNameLoader.try_new(token)) - .to be_a(Cask::CaskLoader::FromNameLoader) - .and have_attributes(token:) - end - it "returns the tap migration rename by old token" do - old_token = "#{token}-old" + old_token = "#{api_token}-old" (foo_tap.path/"tap_migrations.json").write <<~JSON - { "#{old_token}": "homebrew/cask/#{token}" } + { "#{old_token}": "homebrew/cask/#{api_token}" } JSON - expect(Cask::CaskLoader::FromNameLoader.try_new(old_token)) - .to be_a(described_class) - .and have_attributes(token:) + loader = Cask::CaskLoader::FromNameLoader.try_new(old_token) + expect(loader).to be_a(described_class) + expect(loader.token).to eq api_token + expect(loader.path).not_to exist end it "returns the tap migration rename by old full name" do - old_token = "#{token}-old" + old_token = "#{api_token}-old" (foo_tap.path/"tap_migrations.json").write <<~JSON - { "#{old_token}": "homebrew/cask/#{token}" } + { "#{old_token}": "homebrew/cask/#{api_token}" } JSON - expect(Cask::CaskLoader::FromTapLoader.try_new("#{foo_tap}/#{old_token}")) - .to be_a(described_class) - .and have_attributes(token:) + loader = Cask::CaskLoader::FromTapLoader.try_new("#{foo_tap}/#{old_token}") + expect(loader).to be_a(described_class) + expect(loader.token).to eq api_token + expect(loader.path).not_to exist end end end end describe "#load" do - shared_examples "loads from API" do |cask_token, caskfile_only| + shared_examples "loads from API" do |cask_token, caskfile_only:| include_context "with API setup", cask_token let(:cask_from_api) { api_loader.load(config: nil) } it "loads from JSON API" do expect(cask_from_api).to be_a(Cask::Cask) - expect(cask_from_api.token).to eq(cask_token) + expect(cask_from_api.token).to eq(api_token) expect(cask_from_api.loaded_from_api?).to be(true) expect(cask_from_api.caskfile_only?).to be(caskfile_only) end end context "with a binary stanza" do - include_examples "loads from API", "with-binary", false + include_examples "loads from API", "with-binary", caskfile_only: false end context "with cask dependencies" do - include_examples "loads from API", "with-depends-on-cask-multiple", false + include_examples "loads from API", "with-depends-on-cask-multiple", caskfile_only: false end context "with formula dependencies" do - include_examples "loads from API", "with-depends-on-formula-multiple", false + include_examples "loads from API", "with-depends-on-formula-multiple", caskfile_only: false end context "with macos dependencies" do - include_examples "loads from API", "with-depends-on-macos-array", false + include_examples "loads from API", "with-depends-on-macos-array", caskfile_only: false end context "with an installer stanza" do - include_examples "loads from API", "with-installer-script", false + include_examples "loads from API", "with-installer-script", caskfile_only: false end context "with uninstall stanzas" do - include_examples "loads from API", "with-uninstall-multi", false + include_examples "loads from API", "with-uninstall-multi", caskfile_only: false end context "with a zap stanza" do - include_examples "loads from API", "with-zap", false + include_examples "loads from API", "with-zap", caskfile_only: false end context "with a preflight stanza" do - include_examples "loads from API", "with-preflight", true + include_examples "loads from API", "with-preflight", caskfile_only: true end context "with an uninstall-preflight stanza" do - include_examples "loads from API", "with-uninstall-preflight", true + include_examples "loads from API", "with-uninstall-preflight", caskfile_only: true end context "with a postflight stanza" do - include_examples "loads from API", "with-postflight", true + include_examples "loads from API", "with-postflight", caskfile_only: true end context "with an uninstall-postflight stanza" do - include_examples "loads from API", "with-uninstall-postflight", true + include_examples "loads from API", "with-uninstall-postflight", caskfile_only: true end context "with a language stanza" do - include_examples "loads from API", "with-languages", true + include_examples "loads from API", "with-languages", caskfile_only: true end end end diff --git a/Library/Homebrew/test/formulary_spec.rb b/Library/Homebrew/test/formulary_spec.rb index 43fb416bd1..86566a72b7 100644 --- a/Library/Homebrew/test/formulary_spec.rb +++ b/Library/Homebrew/test/formulary_spec.rb @@ -55,37 +55,10 @@ RSpec.describe Formulary do end describe "::factory" do - before do - formula_path.dirname.mkpath - formula_path.write formula_content - end - - it "returns a Formula" do - expect(described_class.factory(formula_name)).to be_a(Formula) - end - - it "returns a Formula when given a fully qualified name" do - expect(described_class.factory("homebrew/core/#{formula_name}")).to be_a(Formula) - end - - it "raises an error if the Formula cannot be found" do - expect do - described_class.factory("not_existed_formula") - end.to raise_error(FormulaUnavailableError) - end - - it "raises an error if ref is nil" do - expect do - described_class.factory(nil) - end.to raise_error(TypeError) - end - - context "with sharded Formula directory" do - let(:formula_name) { "testball_sharded" } - let(:formula_path) do - core_tap = CoreTap.instance - (core_tap.formula_dir/formula_name[0]).mkpath - core_tap.new_formula_path(formula_name) + context "without the API" do + before do + formula_path.dirname.mkpath + formula_path.write formula_content end it "returns a Formula" do @@ -95,180 +68,209 @@ RSpec.describe Formulary do it "returns a Formula when given a fully qualified name" do expect(described_class.factory("homebrew/core/#{formula_name}")).to be_a(Formula) end - end - context "when the Formula has the wrong class" do - let(:formula_name) { "giraffe" } - let(:formula_content) do - <<~RUBY - class Wrong#{described_class.class_s(formula_name)} < Formula - end - RUBY - end - - it "raises an error" do + it "raises an error if the Formula cannot be found" do expect do - described_class.factory(formula_name) - end.to raise_error(TapFormulaClassUnavailableError) - end - end - - it "returns a Formula when given a path" do - expect(described_class.factory(formula_path)).to be_a(Formula) - end - - it "returns a Formula when given a URL", :needs_utils_curl, :no_api do - formula = described_class.factory("file://#{formula_path}") - expect(formula).to be_a(Formula) - end - - context "when given a bottle" do - subject(:formula) { described_class.factory(bottle) } - - it "returns a Formula" do - expect(formula).to be_a(Formula) + described_class.factory("not_existed_formula") + end.to raise_error(FormulaUnavailableError) end - it "calling #local_bottle_path on the returned Formula returns the bottle path" do - expect(formula.local_bottle_path).to eq(bottle.realpath) - end - end - - context "when given an alias" do - subject(:formula) { described_class.factory("foo") } - - let(:alias_dir) { CoreTap.instance.alias_dir } - let(:alias_path) { alias_dir/"foo" } - - before do - alias_dir.mkpath - FileUtils.ln_s formula_path, alias_path - end - - it "returns a Formula" do - expect(formula).to be_a(Formula) - end - - it "calling #alias_path on the returned Formula returns the alias path" do - expect(formula.alias_path).to eq(alias_path) - end - end - - context "with installed Formula" do - before do - # don't try to load/fetch gcc/glibc - allow(DevelopmentTools).to receive_messages(needs_libc_formula?: false, needs_compiler_formula?: false) - end - - let(:installed_formula) { described_class.factory(formula_path) } - let(:installer) { FormulaInstaller.new(installed_formula) } - - it "returns a Formula when given a rack" do - installer.fetch - installer.install - - f = described_class.from_rack(installed_formula.rack) - expect(f).to be_a(Formula) - end - - it "returns a Formula when given a Keg" do - installer.fetch - installer.install - - keg = Keg.new(installed_formula.prefix) - f = described_class.from_keg(keg) - expect(f).to be_a(Formula) - end - end - - context "when migrating from a Tap" do - let(:tap) { Tap.fetch("homebrew", "foo") } - let(:another_tap) { Tap.fetch("homebrew", "bar") } - let(:tap_migrations_path) { tap.path/"tap_migrations.json" } - let(:another_tap_formula_path) { another_tap.path/"Formula/#{formula_name}.rb" } - - before do - tap.path.mkpath - another_tap_formula_path.dirname.mkpath - another_tap_formula_path.write formula_content - end - - after do - FileUtils.rm_rf tap.path - FileUtils.rm_rf another_tap.path - end - - it "returns a Formula that has gone through a tap migration into homebrew/core" do - tap_migrations_path.write <<~EOS - { - "#{formula_name}": "homebrew/core" - } - EOS - formula = described_class.factory("#{tap}/#{formula_name}") - expect(formula).to be_a(Formula) - expect(formula.tap).to eq(CoreTap.instance) - expect(formula.path).to eq(formula_path) - end - - it "returns a Formula that has gone through a tap migration into another tap" do - tap_migrations_path.write <<~EOS - { - "#{formula_name}": "#{another_tap}" - } - EOS - formula = described_class.factory("#{tap}/#{formula_name}") - expect(formula).to be_a(Formula) - expect(formula.tap).to eq(another_tap) - expect(formula.path).to eq(another_tap_formula_path) - end - end - - context "when loading from Tap" do - let(:tap) { Tap.fetch("homebrew", "foo") } - let(:another_tap) { Tap.fetch("homebrew", "bar") } - let(:formula_path) { tap.path/"Formula/#{formula_name}.rb" } - let(:alias_name) { "bar" } - let(:alias_dir) { tap.alias_dir } - let(:alias_path) { alias_dir/alias_name } - - before do - alias_dir.mkpath - FileUtils.ln_s formula_path, alias_path - end - - it "returns a Formula when given a name" do - expect(described_class.factory(formula_name)).to be_a(Formula) - end - - it "returns a Formula from an Alias path" do - expect(described_class.factory(alias_name)).to be_a(Formula) - end - - it "returns a Formula from a fully qualified Alias path" do - expect(described_class.factory("#{tap.name}/#{alias_name}")).to be_a(Formula) - end - - it "raises an error when the Formula cannot be found" do + it "raises an error if ref is nil" do expect do - described_class.factory("#{tap}/not_existed_formula") - end.to raise_error(TapFormulaUnavailableError) + described_class.factory(nil) + end.to raise_error(TypeError) end - it "returns a Formula when given a fully qualified name" do - expect(described_class.factory("#{tap}/#{formula_name}")).to be_a(Formula) + context "with sharded Formula directory" do + let(:formula_name) { "testball_sharded" } + let(:formula_path) do + core_tap = CoreTap.instance + (core_tap.formula_dir/formula_name[0]).mkpath + core_tap.new_formula_path(formula_name) + end + + it "returns a Formula" do + expect(described_class.factory(formula_name)).to be_a(Formula) + end + + it "returns a Formula when given a fully qualified name" do + expect(described_class.factory("homebrew/core/#{formula_name}")).to be_a(Formula) + end end - it "raises an error if a Formula is in multiple Taps" do - (another_tap.path/"Formula").mkpath - (another_tap.path/"Formula/#{formula_name}.rb").write formula_content + context "when the Formula has the wrong class" do + let(:formula_name) { "giraffe" } + let(:formula_content) do + <<~RUBY + class Wrong#{described_class.class_s(formula_name)} < Formula + end + RUBY + end - expect do - described_class.factory(formula_name) - end.to raise_error(TapFormulaAmbiguityError) + it "raises an error" do + expect do + described_class.factory(formula_name) + end.to raise_error(TapFormulaClassUnavailableError) + end + end + + it "returns a Formula when given a path" do + expect(described_class.factory(formula_path)).to be_a(Formula) + end + + it "returns a Formula when given a URL", :needs_utils_curl, :no_api do + formula = described_class.factory("file://#{formula_path}") + expect(formula).to be_a(Formula) + end + + context "when given a bottle" do + subject(:formula) { described_class.factory(bottle) } + + it "returns a Formula" do + expect(formula).to be_a(Formula) + end + + it "calling #local_bottle_path on the returned Formula returns the bottle path" do + expect(formula.local_bottle_path).to eq(bottle.realpath) + end + end + + context "when given an alias" do + subject(:formula) { described_class.factory("foo") } + + let(:alias_dir) { CoreTap.instance.alias_dir } + let(:alias_path) { alias_dir/"foo" } + + before do + alias_dir.mkpath + FileUtils.ln_s formula_path, alias_path + end + + it "returns a Formula" do + expect(formula).to be_a(Formula) + end + + it "calling #alias_path on the returned Formula returns the alias path" do + expect(formula.alias_path).to eq(alias_path) + end + end + + context "with installed Formula" do + before do + # don't try to load/fetch gcc/glibc + allow(DevelopmentTools).to receive_messages(needs_libc_formula?: false, needs_compiler_formula?: false) + end + + let(:installed_formula) { described_class.factory(formula_path) } + let(:installer) { FormulaInstaller.new(installed_formula) } + + it "returns a Formula when given a rack" do + installer.fetch + installer.install + + f = described_class.from_rack(installed_formula.rack) + expect(f).to be_a(Formula) + end + + it "returns a Formula when given a Keg" do + installer.fetch + installer.install + + keg = Keg.new(installed_formula.prefix) + f = described_class.from_keg(keg) + expect(f).to be_a(Formula) + end + end + + context "when migrating from a Tap" do + let(:tap) { Tap.fetch("homebrew", "foo") } + let(:another_tap) { Tap.fetch("homebrew", "bar") } + let(:tap_migrations_path) { tap.path/"tap_migrations.json" } + let(:another_tap_formula_path) { another_tap.path/"Formula/#{formula_name}.rb" } + + before do + tap.path.mkpath + another_tap_formula_path.dirname.mkpath + another_tap_formula_path.write formula_content + end + + after do + FileUtils.rm_rf tap.path + FileUtils.rm_rf another_tap.path + end + + it "returns a Formula that has gone through a tap migration into homebrew/core" do + tap_migrations_path.write <<~EOS + { + "#{formula_name}": "homebrew/core" + } + EOS + formula = described_class.factory("#{tap}/#{formula_name}") + expect(formula).to be_a(Formula) + expect(formula.tap).to eq(CoreTap.instance) + expect(formula.path).to eq(formula_path) + end + + it "returns a Formula that has gone through a tap migration into another tap" do + tap_migrations_path.write <<~EOS + { + "#{formula_name}": "#{another_tap}" + } + EOS + formula = described_class.factory("#{tap}/#{formula_name}") + expect(formula).to be_a(Formula) + expect(formula.tap).to eq(another_tap) + expect(formula.path).to eq(another_tap_formula_path) + end + end + + context "when loading from Tap" do + let(:tap) { Tap.fetch("homebrew", "foo") } + let(:another_tap) { Tap.fetch("homebrew", "bar") } + let(:formula_path) { tap.path/"Formula/#{formula_name}.rb" } + let(:alias_name) { "bar" } + let(:alias_dir) { tap.alias_dir } + let(:alias_path) { alias_dir/alias_name } + + before do + alias_dir.mkpath + FileUtils.ln_s formula_path, alias_path + end + + it "returns a Formula when given a name" do + expect(described_class.factory(formula_name)).to be_a(Formula) + end + + it "returns a Formula from an Alias path" do + expect(described_class.factory(alias_name)).to be_a(Formula) + end + + it "returns a Formula from a fully qualified Alias path" do + expect(described_class.factory("#{tap.name}/#{alias_name}")).to be_a(Formula) + end + + it "raises an error when the Formula cannot be found" do + expect do + described_class.factory("#{tap}/not_existed_formula") + end.to raise_error(TapFormulaUnavailableError) + end + + it "returns a Formula when given a fully qualified name" do + expect(described_class.factory("#{tap}/#{formula_name}")).to be_a(Formula) + end + + it "raises an error if a Formula is in multiple Taps" do + (another_tap.path/"Formula").mkpath + (another_tap.path/"Formula/#{formula_name}.rb").write formula_content + + expect do + described_class.factory(formula_name) + end.to raise_error(TapFormulaAmbiguityError) + end end end - context "when loading from the API" do + context "with the API" do def formula_json_contents(extra_items = {}) { formula_name => { @@ -496,25 +498,16 @@ RSpec.describe Formulary do FileUtils.rm_rf foo_tap.path end - it "returns the core formula if the short name clashes with a tap migration rename" do - (foo_tap.path/"tap_migrations.json").write <<~JSON - { "#{formula_name}": "homebrew/core/#{formula_name}-v2" } - JSON - - expect(described_class::FromNameLoader.try_new(formula_name)) - .to be_a(described_class::FromNameLoader) - .and have_attributes(name: formula_name) - end - it "returns the tap migration rename by old formula_name" do old_formula_name = "#{formula_name}-old" (foo_tap.path/"tap_migrations.json").write <<~JSON { "#{old_formula_name}": "homebrew/core/#{formula_name}" } JSON - expect(described_class::FromNameLoader.try_new(old_formula_name)) - .to be_a(described_class::FromAPILoader) - .and have_attributes(name: formula_name) + loader = described_class::FromNameLoader.try_new(old_formula_name) + expect(loader).to be_a(described_class::FromAPILoader) + expect(loader.name).to eq formula_name + expect(loader.path).not_to exist end it "returns the tap migration rename by old full name" do @@ -523,9 +516,10 @@ RSpec.describe Formulary do { "#{old_formula_name}": "homebrew/core/#{formula_name}" } JSON - expect(described_class::FromTapLoader.try_new("#{foo_tap}/#{old_formula_name}")) - .to be_a(described_class::FromAPILoader) - .and have_attributes(name: formula_name) + loader = described_class::FromTapLoader.try_new("#{foo_tap}/#{old_formula_name}") + expect(loader).to be_a(described_class::FromAPILoader) + expect(loader.name).to eq formula_name + expect(loader.path).not_to exist end end end