From d2ff8794966e7f55b2f60b94168845631d371396 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sun, 13 Dec 2020 12:19:15 +0100 Subject: [PATCH 1/7] Add `BundleVersion` class. --- Library/Homebrew/bundle_version.rb | 89 +++++++++++++++++++ Library/Homebrew/test/bundle_version_spec.rb | 29 ++++++ .../test/unversioned_cask_checker_spec.rb | 29 ------ Library/Homebrew/unversioned_cask_checker.rb | 55 +----------- 4 files changed, 121 insertions(+), 81 deletions(-) create mode 100644 Library/Homebrew/bundle_version.rb create mode 100644 Library/Homebrew/test/bundle_version_spec.rb delete mode 100644 Library/Homebrew/test/unversioned_cask_checker_spec.rb diff --git a/Library/Homebrew/bundle_version.rb b/Library/Homebrew/bundle_version.rb new file mode 100644 index 0000000000..d6d357008d --- /dev/null +++ b/Library/Homebrew/bundle_version.rb @@ -0,0 +1,89 @@ +# typed: true +# frozen_string_literal: true + +require "system_command" + +module Homebrew + # Representation of a macOS bundle version, commonly found in `Info.plist` files. + # + # @api private + class BundleVersion + extend T::Sig + + extend SystemCommand::Mixin + + sig { params(info_plist_path: Pathname).returns(T.nilable(String)) } + def self.from_info_plist(info_plist_path) + plist = system_command!("plutil", args: ["-convert", "xml1", "-o", "-", info_plist_path]).plist + + short_version = plist["CFBundleShortVersionString"].presence + version = plist["CFBundleVersion"].presence + + new(short_version, version) if short_version || version + end + + sig { params(package_info_path: Pathname).returns(T.nilable(String)) } + def self.from_package_info(package_info_path) + Homebrew.install_bundler_gems! + require "nokogiri" + + xml = Nokogiri::XML(package_info_path.read) + + bundle_id = xml.xpath("//pkg-info//bundle-version//bundle").first&.attr("id") + return unless bundle_id + + bundle = xml.xpath("//pkg-info//bundle").find { |b| b["id"] == bundle_id } + return unless bundle + + short_version = bundle["CFBundleShortVersionString"] + version = bundle["CFBundleVersion"] + + new(short_version, version) if short_version || version + end + + sig { returns(T.nilable(String)) } + attr_reader :short_version, :version + + sig { params(short_version: T.nilable(String), version: T.nilable(String)).void } + def initialize(short_version, version) + @short_version = short_version.presence + @version = version.presence + + return if @short_version || @version + + raise ArgumentError, "`short_version` and `version` cannot both be `nil` or empty" + end + + def <=>(other) + [short_version, version].map { |v| Version.new(v) } <=> + [other.short_version, other.version].map { |v| Version.new(v) } + end + + # Create a nicely formatted version (on a best effor basis). + sig { returns(String) } + def nice_version + nice_parts.join(",") + end + + sig { returns(T::Array[String]) } + def nice_parts + return [short_version] if short_version == version + + if short_version && version + return [version] if version.match?(/\A\d+(\.\d+)+\Z/) && version.start_with?("#{short_version}.") + return [short_version] if short_version.match?(/\A\d+(\.\d+)+\Z/) && short_version.start_with?("#{version}.") + + if short_version.match?(/\A\d+(\.\d+)*\Z/) && version.match?(/\A\d+\Z/) + return [short_version] if short_version.start_with?("#{version}.") || short_version.end_with?(".#{version}") + + return [short_version, version] + end + end + + fallback = (short_version || version).sub(/\A[^\d]+/, "") + + [fallback] + end + private :nice_parts + end +end diff --git a/Library/Homebrew/test/bundle_version_spec.rb b/Library/Homebrew/test/bundle_version_spec.rb new file mode 100644 index 0000000000..4d653b98de --- /dev/null +++ b/Library/Homebrew/test/bundle_version_spec.rb @@ -0,0 +1,29 @@ +# typed: false +# frozen_string_literal: true + +require "bundle_version" + +describe Homebrew::BundleVersion do + describe "#nice_version" do + expected_mappings = { + ["1.2", nil] => "1.2", + [nil, "1.2.3"] => "1.2.3", + ["1.2", "1.2.3"] => "1.2.3", + ["1.2.3", "1.2"] => "1.2.3", + ["1.2.3", "8312"] => "1.2.3,8312", + ["2021", "2006"] => "2021,2006", + ["1.0", "1"] => "1.0", + ["1.0", "0"] => "1.0", + ["1.2.3.4000", "4000"] => "1.2.3.4000", + ["5", "5.0.45"] => "5.0.45", + ["XQuartz-2.7.11", "2.7.112"] => "2.7.11", + } + + expected_mappings.each do |(short_version, version), expected_version| + it "maps (#{short_version.inspect}, #{version.inspect}) to #{expected_version.inspect}" do + expect(described_class.new(short_version, version).nice_version) + .to eq expected_version + end + end + end +end diff --git a/Library/Homebrew/test/unversioned_cask_checker_spec.rb b/Library/Homebrew/test/unversioned_cask_checker_spec.rb deleted file mode 100644 index dc4ddbfef9..0000000000 --- a/Library/Homebrew/test/unversioned_cask_checker_spec.rb +++ /dev/null @@ -1,29 +0,0 @@ -# typed: false -# frozen_string_literal: true - -require "unversioned_cask_checker" - -describe Homebrew::UnversionedCaskChecker do - describe "::decide_between_versions" do - expected_mappings = { - [nil, nil] => nil, - ["1.2", nil] => "1.2", - [nil, "1.2.3"] => "1.2.3", - ["1.2", "1.2.3"] => "1.2.3", - ["1.2.3", "1.2"] => "1.2.3", - ["1.2.3", "8312"] => "1.2.3,8312", - ["2021", "2006"] => "2021,2006", - ["1.0", "1"] => "1.0", - ["1.0", "0"] => "1.0", - ["1.2.3.4000", "4000"] => "1.2.3.4000", - ["5", "5.0.45"] => "5.0.45", - } - - expected_mappings.each do |(short_version, version), expected_version| - it "maps (#{short_version}, #{version}) to #{expected_version}" do - expect(described_class.decide_between_versions(short_version, version)) - .to eq expected_version - end - end - end -end diff --git a/Library/Homebrew/unversioned_cask_checker.rb b/Library/Homebrew/unversioned_cask_checker.rb index d6e18c6e37..679782d6a7 100644 --- a/Library/Homebrew/unversioned_cask_checker.rb +++ b/Library/Homebrew/unversioned_cask_checker.rb @@ -1,6 +1,7 @@ # typed: true # frozen_string_literal: true +require "bundle_version" require "cask/cask" require "cask/installer" @@ -45,56 +46,6 @@ module Homebrew pkgs.count == 1 end - sig { params(info_plist_path: Pathname).returns(T.nilable(String)) } - def self.version_from_info_plist(info_plist_path) - plist = system_command!("plutil", args: ["-convert", "xml1", "-o", "-", info_plist_path]).plist - - short_version = plist["CFBundleShortVersionString"].presence - version = plist["CFBundleVersion"].presence - - return decide_between_versions(short_version, version) if short_version && version - end - - sig { params(package_info_path: Pathname).returns(T.nilable(String)) } - def self.version_from_package_info(package_info_path) - Homebrew.install_bundler_gems! - require "nokogiri" - - xml = Nokogiri::XML(package_info_path.read) - - bundle_id = xml.xpath("//pkg-info//bundle-version//bundle").first&.attr("id") - return unless bundle_id - - bundle = xml.xpath("//pkg-info//bundle").find { |b| b["id"] == bundle_id } - return unless bundle - - short_version = bundle["CFBundleShortVersionString"] - version = bundle["CFBundleVersion"] - - return decide_between_versions(short_version, version) if short_version && version - end - - sig do - params(short_version: T.nilable(String), version: T.nilable(String)) - .returns(T.nilable(String)) - end - def self.decide_between_versions(short_version, version) - return short_version if short_version == version - - if short_version && version - return version if version.match?(/\A\d+(\.\d+)+\Z/) && version.start_with?("#{short_version}.") - return short_version if short_version.match?(/\A\d+(\.\d+)+\Z/) && short_version.start_with?("#{version}.") - - if short_version.match?(/\A\d+(\.\d+)*\Z/) && version.match?(/\A\d+\Z/) - return short_version if short_version.start_with?("#{version}.") || short_version.end_with?(".#{version}") - - return "#{short_version},#{version}" - end - end - - short_version || version - end - sig { returns(T.nilable(String)) } def guess_cask_version if apps.empty? && pkgs.empty? @@ -120,7 +71,7 @@ module Homebrew end info_plist_paths.each do |info_plist_path| - if (version = self.class.version_from_info_plist(info_plist_path)) + if (version = BundleVersion.from_info_plist(info_plist_path)&.nice_version) return version end end @@ -149,7 +100,7 @@ module Homebrew package_info_path = extract_dir/"PackageInfo" if package_info_path.exist? - if (version = self.class.version_from_package_info(package_info_path)) + if (version = BundleVersion.from_package_info(package_info_path)&.nice_version) return version end elsif packages.count == 1 From 849321cbffb81c62935ca4da8d4888ae9120fad1 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Mon, 14 Dec 2020 02:09:58 +0100 Subject: [PATCH 2/7] Prefer `version` for comparision of `BundleVersion`s. --- Library/Homebrew/bundle_version.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/bundle_version.rb b/Library/Homebrew/bundle_version.rb index d6d357008d..0ccbb93239 100644 --- a/Library/Homebrew/bundle_version.rb +++ b/Library/Homebrew/bundle_version.rb @@ -55,8 +55,8 @@ module Homebrew end def <=>(other) - [short_version, version].map { |v| Version.new(v) } <=> - [other.short_version, other.version].map { |v| Version.new(v) } + [version, short_version].map { |v| v&.yield_self(&Version.public_method(:new)) } <=> + [other.version, other.short_version].map { |v| v&.yield_self(&Version.public_method(:new)) } end # Create a nicely formatted version (on a best effor basis). From 7c6116af992b5fda8bc175277d862ef03624c984 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Mon, 14 Dec 2020 09:04:54 +0100 Subject: [PATCH 3/7] Remove fallback for `BundleVersion`. --- Library/Homebrew/bundle_version.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Library/Homebrew/bundle_version.rb b/Library/Homebrew/bundle_version.rb index 0ccbb93239..8c9b07b48e 100644 --- a/Library/Homebrew/bundle_version.rb +++ b/Library/Homebrew/bundle_version.rb @@ -80,9 +80,7 @@ module Homebrew end end - fallback = (short_version || version).sub(/\A[^\d]+/, "") - - [fallback] + [short_version, version].compact end private :nice_parts end From 7b9556db0618e3b3752d74572a3c4108a36878fe Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Mon, 14 Dec 2020 09:26:30 +0100 Subject: [PATCH 4/7] Remove trailing `version` from `short_version`. --- Library/Homebrew/bundle_version.rb | 2 ++ Library/Homebrew/test/bundle_version_spec.rb | 1 + 2 files changed, 3 insertions(+) diff --git a/Library/Homebrew/bundle_version.rb b/Library/Homebrew/bundle_version.rb index 8c9b07b48e..b13849ba1d 100644 --- a/Library/Homebrew/bundle_version.rb +++ b/Library/Homebrew/bundle_version.rb @@ -67,6 +67,8 @@ module Homebrew sig { returns(T::Array[String]) } def nice_parts + short_version = self.short_version&.delete_suffix("(#{version})") if version + return [short_version] if short_version == version if short_version && version diff --git a/Library/Homebrew/test/bundle_version_spec.rb b/Library/Homebrew/test/bundle_version_spec.rb index 4d653b98de..e750fd1974 100644 --- a/Library/Homebrew/test/bundle_version_spec.rb +++ b/Library/Homebrew/test/bundle_version_spec.rb @@ -17,6 +17,7 @@ describe Homebrew::BundleVersion do ["1.2.3.4000", "4000"] => "1.2.3.4000", ["5", "5.0.45"] => "5.0.45", ["XQuartz-2.7.11", "2.7.112"] => "2.7.11", + ["2.5.2(3329)", "3329"] => "2.5.2,3329", } expected_mappings.each do |(short_version, version), expected_version| From 91d6009891e84ec1a19bc3b9edabf4db4f96b184 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Mon, 14 Dec 2020 11:39:55 +0100 Subject: [PATCH 5/7] Remove special case for XQuartz. --- Library/Homebrew/test/bundle_version_spec.rb | 23 ++++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/Library/Homebrew/test/bundle_version_spec.rb b/Library/Homebrew/test/bundle_version_spec.rb index e750fd1974..987ec54162 100644 --- a/Library/Homebrew/test/bundle_version_spec.rb +++ b/Library/Homebrew/test/bundle_version_spec.rb @@ -6,18 +6,17 @@ require "bundle_version" describe Homebrew::BundleVersion do describe "#nice_version" do expected_mappings = { - ["1.2", nil] => "1.2", - [nil, "1.2.3"] => "1.2.3", - ["1.2", "1.2.3"] => "1.2.3", - ["1.2.3", "1.2"] => "1.2.3", - ["1.2.3", "8312"] => "1.2.3,8312", - ["2021", "2006"] => "2021,2006", - ["1.0", "1"] => "1.0", - ["1.0", "0"] => "1.0", - ["1.2.3.4000", "4000"] => "1.2.3.4000", - ["5", "5.0.45"] => "5.0.45", - ["XQuartz-2.7.11", "2.7.112"] => "2.7.11", - ["2.5.2(3329)", "3329"] => "2.5.2,3329", + ["1.2", nil] => "1.2", + [nil, "1.2.3"] => "1.2.3", + ["1.2", "1.2.3"] => "1.2.3", + ["1.2.3", "1.2"] => "1.2.3", + ["1.2.3", "8312"] => "1.2.3,8312", + ["2021", "2006"] => "2021,2006", + ["1.0", "1"] => "1.0", + ["1.0", "0"] => "1.0", + ["1.2.3.4000", "4000"] => "1.2.3.4000", + ["5", "5.0.45"] => "5.0.45", + ["2.5.2(3329)", "3329"] => "2.5.2,3329", } expected_mappings.each do |(short_version, version), expected_version| From 30c35659b625ac070b02dc0a3811671c8a85ebf2 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Mon, 14 Dec 2020 11:47:57 +0100 Subject: [PATCH 6/7] Fix `#nice_version`. --- Library/Homebrew/bundle_version.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/bundle_version.rb b/Library/Homebrew/bundle_version.rb index b13849ba1d..9c3e59e261 100644 --- a/Library/Homebrew/bundle_version.rb +++ b/Library/Homebrew/bundle_version.rb @@ -67,7 +67,8 @@ module Homebrew sig { returns(T::Array[String]) } def nice_parts - short_version = self.short_version&.delete_suffix("(#{version})") if version + short_version = self.short_version + short_version = short_version&.delete_suffix("(#{version})") if version return [short_version] if short_version == version From 7dbb9b406b7236784c415ece63e6344bf6c10953 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Tue, 15 Dec 2020 15:21:14 +0100 Subject: [PATCH 7/7] Fix type signatures in `BundleVersion`. --- Library/Homebrew/bundle_version.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/bundle_version.rb b/Library/Homebrew/bundle_version.rb index 9c3e59e261..ca63aa36b8 100644 --- a/Library/Homebrew/bundle_version.rb +++ b/Library/Homebrew/bundle_version.rb @@ -12,7 +12,7 @@ module Homebrew extend SystemCommand::Mixin - sig { params(info_plist_path: Pathname).returns(T.nilable(String)) } + sig { params(info_plist_path: Pathname).returns(T.nilable(T.attached_class)) } def self.from_info_plist(info_plist_path) plist = system_command!("plutil", args: ["-convert", "xml1", "-o", "-", info_plist_path]).plist @@ -22,7 +22,7 @@ module Homebrew new(short_version, version) if short_version || version end - sig { params(package_info_path: Pathname).returns(T.nilable(String)) } + sig { params(package_info_path: Pathname).returns(T.nilable(T.attached_class)) } def self.from_package_info(package_info_path) Homebrew.install_bundler_gems! require "nokogiri" @@ -68,9 +68,11 @@ module Homebrew sig { returns(T::Array[String]) } def nice_parts short_version = self.short_version + version = self.version + short_version = short_version&.delete_suffix("(#{version})") if version - return [short_version] if short_version == version + return [T.must(short_version)] if short_version == version if short_version && version return [version] if version.match?(/\A\d+(\.\d+)+\Z/) && version.start_with?("#{short_version}.")