From b6062acdbe1f9997301975775f9d4c000c38edba Mon Sep 17 00:00:00 2001 From: Issy Long Date: Tue, 14 Mar 2023 23:08:22 +0000 Subject: [PATCH 1/5] rubocops/cask: Enforce the order of `on_#{arch}` blocks - These were previously being manually fixed which is time maintainers could have spent fixing more important problems. - I don't work with Casks much at all, so I was unsure as to what the existing "arch" and "on_arch_conditional" parts were, if they're deprecated or if things were eventually going to migrate to `on_#{arch}` blocks? --- .../rubocops/cask/constants/stanza.rb | 2 +- .../test/rubocops/cask/stanza_order_spec.rb | 61 +++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/rubocops/cask/constants/stanza.rb b/Library/Homebrew/rubocops/cask/constants/stanza.rb index d8129cc51b..b8bf81c818 100644 --- a/Library/Homebrew/rubocops/cask/constants/stanza.rb +++ b/Library/Homebrew/rubocops/cask/constants/stanza.rb @@ -6,7 +6,7 @@ module RuboCop # Constants available globally for use in all cask cops. module Constants STANZA_GROUPS = [ - [:arch, :on_arch_conditional], + [:on_arm, :on_intel, :arch, :on_arch_conditional], [:version, :sha256], [:language], [:url, :appcast, :name, :desc, :homepage], diff --git a/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb b/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb index 9d515c275d..5ed7543770 100644 --- a/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb +++ b/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb @@ -474,4 +474,65 @@ describe RuboCop::Cop::Cask::StanzaOrder do include_examples "does not report any offenses" end + + context "when `on_arch` blocks are out of order" do + let(:source) do + <<~CASK + cask 'foo' do + on_intel do + url "https://foo.brew.sh/foo-intel.zip" + sha256 :no_check + version :latest + end + + on_arm do + url "https://foo.brew.sh/foo-arm.zip" + sha256 :no_check + version :latest + end + + name "Foo" + end + CASK + end + + let(:expected_offenses) do + [{ + message: "`on_intel` stanza out of order", + severity: :convention, + line: 2, + column: 2, + source: "on_intel do\n url \"https://foo.brew.sh/foo-intel.zip\"\n sha256 :no_check\n version :latest\n end", # rubocop:disable Layout/LineLength + }, { + message: "`on_arm` stanza out of order", + severity: :convention, + line: 8, + column: 2, + source: "on_arm do\n url \"https://foo.brew.sh/foo-arm.zip\"\n sha256 :no_check\n version :latest\n end", # rubocop:disable Layout/LineLength + }] + end + + let(:correct_source) do + <<~CASK + cask 'foo' do + on_arm do + url "https://foo.brew.sh/foo-arm.zip" + sha256 :no_check + version :latest + end + + on_intel do + url "https://foo.brew.sh/foo-intel.zip" + sha256 :no_check + version :latest + end + + name "Foo" + end + CASK + end + + include_examples "reports offenses" + include_examples "autocorrects source" + end end From b484a29006a3104e533ab0f603a598fedd6b5e54 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Tue, 14 Mar 2023 23:30:47 +0000 Subject: [PATCH 2/5] rubocops/cask: `arch` stanzas come before & are separate to `on_#{arch}` - https://github.com/Homebrew/brew/pull/14976#issuecomment-1469002998. - This drops the number of offenses currently in the Cask repo down from 111 to 46. --- Library/Homebrew/rubocops/cask/constants/stanza.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/rubocops/cask/constants/stanza.rb b/Library/Homebrew/rubocops/cask/constants/stanza.rb index b8bf81c818..aeeee29880 100644 --- a/Library/Homebrew/rubocops/cask/constants/stanza.rb +++ b/Library/Homebrew/rubocops/cask/constants/stanza.rb @@ -6,7 +6,8 @@ module RuboCop # Constants available globally for use in all cask cops. module Constants STANZA_GROUPS = [ - [:on_arm, :on_intel, :arch, :on_arch_conditional], + [:arch, :on_arch_conditional], + [:on_arm, :on_intel], [:version, :sha256], [:language], [:url, :appcast, :name, :desc, :homepage], From d97ed0a7c27684fb6621a4a27ba3afa7c8650e2b Mon Sep 17 00:00:00 2001 From: Issy Long Date: Tue, 14 Mar 2023 23:47:39 +0000 Subject: [PATCH 3/5] rubocops/cask: Ensure ordering of all the `on_#{arch,system}` blocks - Complaining about only `on_arm` and `on_intel` was too restrictive since casks can have many `on_system` blocks (`on_#{arch}` and `on_#{os}`). - We're a bit of the way there, anyway. Still doesn't support stanza ordering within blocks, but that's for another time (there's a separate issue that's been open for a while - 14017). --- .../rubocops/cask/constants/stanza.rb | 6 +- .../test/rubocops/cask/stanza_order_spec.rb | 117 ++++++++++++++++++ 2 files changed, 120 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/constants/stanza.rb b/Library/Homebrew/rubocops/cask/constants/stanza.rb index aeeee29880..00c909b63d 100644 --- a/Library/Homebrew/rubocops/cask/constants/stanza.rb +++ b/Library/Homebrew/rubocops/cask/constants/stanza.rb @@ -5,9 +5,11 @@ module RuboCop module Cask # Constants available globally for use in all cask cops. module Constants + ON_SYSTEM_METHODS = [:arm, :intel, *MacOSVersions::SYMBOLS.keys].map { |option| :"on_#{option}" }.freeze + STANZA_GROUPS = [ [:arch, :on_arch_conditional], - [:on_arm, :on_intel], + ON_SYSTEM_METHODS, [:version, :sha256], [:language], [:url, :appcast, :name, :desc, :homepage], @@ -56,8 +58,6 @@ module RuboCop end.freeze STANZA_ORDER = STANZA_GROUPS.flatten.freeze - - ON_SYSTEM_METHODS = [:arm, :intel, *MacOSVersions::SYMBOLS.keys].map { |option| :"on_#{option}" }.freeze end end end diff --git a/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb b/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb index 5ed7543770..b33db17f4d 100644 --- a/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb +++ b/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb @@ -535,4 +535,121 @@ describe RuboCop::Cop::Cask::StanzaOrder do include_examples "reports offenses" include_examples "autocorrects source" end + + # TODO: detect out-of-order stanzas in nested expressions + context "when the on_arch and on_os stanzas are nested" do + let(:source) do + <<~CASK + cask 'foo' do + on_arm do + url "https://foo.brew.sh/foo-arm-all.zip" + sha256 :no_check + version :latest + end + + on_intel do + on_ventura do + url "https://foo.brew.sh/foo-intel-ventura.zip" + sha256 :no_check + end + on_mojave do + url "https://foo.brew.sh/foo-intel-mojave.zip" + sha256 :no_check + end + on_catalina do + url "https://foo.brew.sh/foo-intel-catalina.zip" + sha256 :no_check + end + on_big_sur do + url "https://foo.brew.sh/foo-intel-big-sur.zip" + sha256 :no_check + end + + version :latest + end + + name "Foo" + end + CASK + end + + include_examples "does not report any offenses" + end + + context "when the on_os stanzas are out of order" do + let(:source) do + <<~CASK + cask "foo" do + on_ventura do + url "https://foo.brew.sh/foo-ventura.zip" + sha256 :no_check + end + on_catalina do + url "https://foo.brew.sh/foo-catalina.zip" + sha256 :no_check + end + on_mojave do + url "https://foo.brew.sh/foo-mojave.zip" + sha256 :no_check + end + on_big_sur do + url "https://foo.brew.sh/foo-big-sur.zip" + sha256 :no_check + end + + name "Foo" + end + CASK + end + + let(:expected_offenses) do + [{ + message: "`on_catalina` stanza out of order", + severity: :convention, + line: 6, + column: 2, + source: "on_catalina do\n url \"https://foo.brew.sh/foo-catalina.zip\"\n sha256 :no_check\n end", + }, { + message: "`on_mojave` stanza out of order", + severity: :convention, + line: 10, + column: 2, + source: "on_mojave do\n url \"https://foo.brew.sh/foo-mojave.zip\"\n sha256 :no_check\n end", + }, { + message: "`on_big_sur` stanza out of order", + severity: :convention, + line: 14, + column: 2, + source: "on_big_sur do\n url \"https://foo.brew.sh/foo-big-sur.zip\"\n sha256 :no_check\n end", + }] + end + + let(:correct_source) do + <<~CASK + cask "foo" do + on_ventura do + url "https://foo.brew.sh/foo-ventura.zip" + sha256 :no_check + end + on_big_sur do + url "https://foo.brew.sh/foo-big-sur.zip" + sha256 :no_check + end + on_catalina do + url "https://foo.brew.sh/foo-catalina.zip" + sha256 :no_check + end + on_mojave do + url "https://foo.brew.sh/foo-mojave.zip" + sha256 :no_check + end + + name "Foo" + end + CASK + end + + include_examples "reports offenses" + include_examples "autocorrects source" + end end From 48b1279b00634414236010a72f9f97531840ae09 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Thu, 16 Mar 2023 23:49:32 +0000 Subject: [PATCH 4/5] cask/audits: `on_#{os_version}` stanza order is oldest => newest - This, ie Mojave first, is more common in real Casks than the alternative of newest to oldest ie Ventura first. - Doing it this way reduces the number of offenses from ~500 to ~200. --- .../rubocops/cask/constants/stanza.rb | 7 +++++- .../test/rubocops/cask/stanza_order_spec.rb | 22 +++++++++---------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/constants/stanza.rb b/Library/Homebrew/rubocops/cask/constants/stanza.rb index 00c909b63d..0fff837f3f 100644 --- a/Library/Homebrew/rubocops/cask/constants/stanza.rb +++ b/Library/Homebrew/rubocops/cask/constants/stanza.rb @@ -6,10 +6,15 @@ module RuboCop # Constants available globally for use in all cask cops. module Constants ON_SYSTEM_METHODS = [:arm, :intel, *MacOSVersions::SYMBOLS.keys].map { |option| :"on_#{option}" }.freeze + ON_SYSTEM_METHODS_STANZA_ORDER = [ + :arm, + :intel, + *MacOSVersions::SYMBOLS.reverse_each.to_h.keys, # Oldest OS blocks first since that's more common in Casks. + ].map { |option, _| :"on_#{option}" }.freeze STANZA_GROUPS = [ [:arch, :on_arch_conditional], - ON_SYSTEM_METHODS, + ON_SYSTEM_METHODS_STANZA_ORDER, [:version, :sha256], [:language], [:url, :appcast, :name, :desc, :homepage], diff --git a/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb b/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb index b33db17f4d..b53f937e6e 100644 --- a/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb +++ b/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb @@ -604,11 +604,11 @@ describe RuboCop::Cop::Cask::StanzaOrder do let(:expected_offenses) do [{ - message: "`on_catalina` stanza out of order", + message: "`on_ventura` stanza out of order", severity: :convention, - line: 6, + line: 2, column: 2, - source: "on_catalina do\n url \"https://foo.brew.sh/foo-catalina.zip\"\n sha256 :no_check\n end", + source: "on_ventura do\n url \"https://foo.brew.sh/foo-ventura.zip\"\n sha256 :no_check\n end", }, { message: "`on_mojave` stanza out of order", severity: :convention, @@ -627,20 +627,20 @@ describe RuboCop::Cop::Cask::StanzaOrder do let(:correct_source) do <<~CASK cask "foo" do - on_ventura do - url "https://foo.brew.sh/foo-ventura.zip" - sha256 :no_check - end - on_big_sur do - url "https://foo.brew.sh/foo-big-sur.zip" + on_mojave do + url "https://foo.brew.sh/foo-mojave.zip" sha256 :no_check end on_catalina do url "https://foo.brew.sh/foo-catalina.zip" sha256 :no_check end - on_mojave do - url "https://foo.brew.sh/foo-mojave.zip" + on_big_sur do + url "https://foo.brew.sh/foo-big-sur.zip" + sha256 :no_check + end + on_ventura do + url "https://foo.brew.sh/foo-ventura.zip" sha256 :no_check end From 0af5825dfbccd7b664f787826b396b897326554b Mon Sep 17 00:00:00 2001 From: Issy Long Date: Fri, 17 Mar 2023 22:56:50 +0000 Subject: [PATCH 5/5] rubocops/cask: `on_#{os_version}` is after [`version` & `sha256`] group - This still doesn't pass `brew readall` for Casks, but it gets us a little closer since if `url` has a `version` interpolated in it, the `version` stanza has to come first. - See https://github.com/Homebrew/homebrew-cask/pull/143201 for the current failures. --- Library/Homebrew/rubocops/cask/constants/stanza.rb | 2 +- .../fixtures/cask/Casks/with-depends-on-macos-failure.rb | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/constants/stanza.rb b/Library/Homebrew/rubocops/cask/constants/stanza.rb index 0fff837f3f..5e589338b9 100644 --- a/Library/Homebrew/rubocops/cask/constants/stanza.rb +++ b/Library/Homebrew/rubocops/cask/constants/stanza.rb @@ -14,8 +14,8 @@ module RuboCop STANZA_GROUPS = [ [:arch, :on_arch_conditional], - ON_SYSTEM_METHODS_STANZA_ORDER, [:version, :sha256], + ON_SYSTEM_METHODS_STANZA_ORDER, [:language], [:url, :appcast, :name, :desc, :homepage], [:livecheck], diff --git a/Library/Homebrew/test/support/fixtures/cask/Casks/with-depends-on-macos-failure.rb b/Library/Homebrew/test/support/fixtures/cask/Casks/with-depends-on-macos-failure.rb index f7d2061796..bbde7db9c3 100644 --- a/Library/Homebrew/test/support/fixtures/cask/Casks/with-depends-on-macos-failure.rb +++ b/Library/Homebrew/test/support/fixtures/cask/Casks/with-depends-on-macos-failure.rb @@ -2,9 +2,6 @@ cask "with-depends-on-macos-failure" do version "1.2.3" sha256 "67cdb8a02803ef37fdbf7e0be205863172e41a561ca446cd84f0d7ab35a99d94" - url "file://#{TEST_FIXTURE_DIR}/cask/caffeine.zip" - homepage "https://brew.sh/with-depends-on-macos-failure" - # guarantee a mismatched release on_mojave :or_older do depends_on macos: :catalina @@ -16,5 +13,8 @@ cask "with-depends-on-macos-failure" do depends_on macos: :catalina end + url "file://#{TEST_FIXTURE_DIR}/cask/caffeine.zip" + homepage "https://brew.sh/with-depends-on-macos-failure" + app "Caffeine.app" end