From bd8805b14fb8a2275fcc3b21d78c555d6f5c72e3 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Fri, 3 Jul 2020 16:27:35 -0400 Subject: [PATCH 01/12] style: separate make commands --- Library/Homebrew/dev-cmd/audit.rb | 3 --- Library/Homebrew/rubocops/text.rb | 7 +++++++ Library/Homebrew/test/rubocops/text_spec.rb | 11 +++++++++++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index b1f3462b07..1a3749ca63 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -858,9 +858,6 @@ module Homebrew ) end - # TODO: check could be in RuboCop - problem "Use separate make calls" if line.include?("make && make") - if line =~ /JAVA_HOME/i && [formula.name, *formula.deps.map(&:name)].none? { |name| name.match?(/^openjdk(@|$)/) } && formula.requirements.none? { |req| req.is_a?(JavaRequirement) } diff --git a/Library/Homebrew/rubocops/text.rb b/Library/Homebrew/rubocops/text.rb index 6938d5f5fc..c41fd4c913 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -62,6 +62,13 @@ module RuboCop find_method_with_args(body_node, :system, "cargo", "build") do problem "use \"cargo\", \"install\", *std_cargo_args" end + + find_every_method_call_by_name(body_node, :system).each do |m| + next unless parameters_passed?(m, /make && make/) + + offending_node(m) + problem "Use separate `make` calls" + end end end end diff --git a/Library/Homebrew/test/rubocops/text_spec.rb b/Library/Homebrew/test/rubocops/text_spec.rb index 38204d4ef6..561ff18f9e 100644 --- a/Library/Homebrew/test/rubocops/text_spec.rb +++ b/Library/Homebrew/test/rubocops/text_spec.rb @@ -215,5 +215,16 @@ describe RuboCop::Cop::FormulaAudit::Text do end RUBY end + + it "When make calls are not separated" do + expect_offense(<<~RUBY) + class Foo < Formula + def install + system "make && make install" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use separate `make` calls + end + end + RUBY + end end end From 1e943d7b6f21c61104c932cecd4176cfac02bb1e Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sun, 5 Jul 2020 11:42:16 -0400 Subject: [PATCH 02/12] style: env :std deprecated in homebrew-core --- Library/Homebrew/dev-cmd/audit.rb | 5 ----- Library/Homebrew/rubocops/text.rb | 6 ++++++ Library/Homebrew/test/.rubocop_todo.yml | 3 ++- Library/Homebrew/test/rubocops/text_spec.rb | 17 +++++++++++++++++ 4 files changed, 25 insertions(+), 6 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 1a3749ca63..b27a74dcc2 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -887,11 +887,6 @@ module Homebrew # TODO: check could be in RuboCop problem "Use pkgshare instead of (share#{Regexp.last_match(1)}\"#{formula.name}\")" end - - return unless @core_tap - - # TODO: check could be in RuboCop - problem "`env :std` in homebrew/core formulae is deprecated" if line.include?("env :std") end def audit_reverse_migration diff --git a/Library/Homebrew/rubocops/text.rb b/Library/Homebrew/rubocops/text.rb index c41fd4c913..eec390e5c9 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -79,6 +79,12 @@ module RuboCop find_method_with_args(body_node, :go_resource) do problem "`go_resource`s are deprecated. Please ask upstream to implement Go vendoring" end + + return unless formula_tap == "homebrew-core" + + find_method_with_args(body_node, :env, :std) do + problem "`env :std` in homebrew/core formulae is deprecated" + end end end end diff --git a/Library/Homebrew/test/.rubocop_todo.yml b/Library/Homebrew/test/.rubocop_todo.yml index 18986235db..59e8c081af 100644 --- a/Library/Homebrew/test/.rubocop_todo.yml +++ b/Library/Homebrew/test/.rubocop_todo.yml @@ -21,7 +21,7 @@ RSpec/InstanceVariable: - 'utils/git_spec.rb' - 'version_spec.rb' -# Offense count: 74 +# Offense count: 75 RSpec/MultipleDescribes: Exclude: - 'ENV_spec.rb' @@ -94,6 +94,7 @@ RSpec/MultipleDescribes: - 'rubocops/class_spec.rb' - 'rubocops/formula_desc_spec.rb' - 'rubocops/lines_spec.rb' + - 'rubocops/text_spec.rb' - 'rubocops/urls_spec.rb' - 'software_spec_spec.rb' - 'tap_spec.rb' diff --git a/Library/Homebrew/test/rubocops/text_spec.rb b/Library/Homebrew/test/rubocops/text_spec.rb index 561ff18f9e..b464f7ef51 100644 --- a/Library/Homebrew/test/rubocops/text_spec.rb +++ b/Library/Homebrew/test/rubocops/text_spec.rb @@ -228,3 +228,20 @@ describe RuboCop::Cop::FormulaAudit::Text do end end end + +describe RuboCop::Cop::FormulaAuditStrict::Text do + subject(:cop) { described_class.new } + + context "When auditing formula text" do + it "when deprecated `env :std` is present in homebrew-core" do + expect_offense(<<~RUBY, "/homebrew-core/") + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + + env :std + ^^^^^^^^ `env :std` in homebrew/core formulae is deprecated + end + RUBY + end + end +end From 63b81d847a8ffa40a979034997c4e1ee15c24c24 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sun, 5 Jul 2020 11:46:53 -0400 Subject: [PATCH 03/12] style: env :userpaths is deprecated --- Library/Homebrew/dev-cmd/audit.rb | 3 --- Library/Homebrew/rubocops/text.rb | 4 ++++ Library/Homebrew/test/rubocops/text_spec.rb | 11 +++++++++++ 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index b27a74dcc2..5f3bfe1876 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -867,9 +867,6 @@ module Homebrew return unless @strict - # TODO: check could be in RuboCop - problem "`env :userpaths` in formulae is deprecated" if line.include?("env :userpaths") - # TODO: check could be in RuboCop problem "`#{Regexp.last_match(1)}` is now unnecessary" if line =~ /(require ["']formula["'])/ diff --git a/Library/Homebrew/rubocops/text.rb b/Library/Homebrew/rubocops/text.rb index eec390e5c9..77d25c77a7 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -80,6 +80,10 @@ module RuboCop problem "`go_resource`s are deprecated. Please ask upstream to implement Go vendoring" end + find_method_with_args(body_node, :env, :userpaths) do + problem "`env :userpaths` in homebrew/core formulae is deprecated" + end + return unless formula_tap == "homebrew-core" find_method_with_args(body_node, :env, :std) do diff --git a/Library/Homebrew/test/rubocops/text_spec.rb b/Library/Homebrew/test/rubocops/text_spec.rb index b464f7ef51..8a6668ae46 100644 --- a/Library/Homebrew/test/rubocops/text_spec.rb +++ b/Library/Homebrew/test/rubocops/text_spec.rb @@ -233,6 +233,17 @@ describe RuboCop::Cop::FormulaAuditStrict::Text do subject(:cop) { described_class.new } context "When auditing formula text" do + it "when deprecated `env :userpaths` is present" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + + env :userpaths + ^^^^^^^^^^^^^^ `env :userpaths` in homebrew/core formulae is deprecated + end + RUBY + end + it "when deprecated `env :std` is present in homebrew-core" do expect_offense(<<~RUBY, "/homebrew-core/") class Foo < Formula From 9ad342eba04cfa994746b43e87b7151b59d5a362 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sun, 5 Jul 2020 13:50:48 -0400 Subject: [PATCH 04/12] style: don't concatenate in string interpolation --- Library/Homebrew/dev-cmd/audit.rb | 6 ------ Library/Homebrew/rubocops/text.rb | 9 +++++++++ Library/Homebrew/test/rubocops/text_spec.rb | 11 +++++++++++ 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 5f3bfe1876..b6507d56f6 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -843,12 +843,6 @@ module Homebrew problem "Don't need to interpolate \"#{Regexp.last_match(2)}\" with #{Regexp.last_match(1)}" end - # Check for string concatenation; prefer interpolation - if line =~ /(#\{\w+\s*\+\s*['"][^}]+\})/ - # TODO: check could be in RuboCop - problem "Try not to concatenate paths in string interpolation:\n #{Regexp.last_match(1)}" - end - # Prefer formula path shortcuts in Pathname+ if line =~ %r{\(\s*(prefix\s*\+\s*(['"])(bin|include|libexec|lib|sbin|share|Frameworks)[/'"])} # TODO: check could be in RuboCop diff --git a/Library/Homebrew/rubocops/text.rb b/Library/Homebrew/rubocops/text.rb index 77d25c77a7..8aa2b9ab2a 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -69,6 +69,15 @@ module RuboCop offending_node(m) problem "Use separate `make` calls" end + + body_node.each_descendant(:dstr) do |dstr_node| + dstr_node.each_descendant(:begin) do |interpolation_node| + next unless interpolation_node.source.match?(/#\{\w+\s*\+\s*['"][^}]+\}/) + + offending_node(interpolation_node) + problem "Do not concatenate paths in string interpolation" + end + end end end end diff --git a/Library/Homebrew/test/rubocops/text_spec.rb b/Library/Homebrew/test/rubocops/text_spec.rb index 8a6668ae46..f915daafcd 100644 --- a/Library/Homebrew/test/rubocops/text_spec.rb +++ b/Library/Homebrew/test/rubocops/text_spec.rb @@ -226,6 +226,17 @@ describe RuboCop::Cop::FormulaAudit::Text do end RUBY end + + it "When concatenating in string interpolation" do + expect_offense(<<~RUBY) + class Foo < Formula + def install + ohai "foo \#{bar + "baz"}" + ^^^^^^^^^^^^^^ Do not concatenate paths in string interpolation + end + end + RUBY + end end end From b4a9565b8bf370d90dc6b7900f6542998e5665d8 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sun, 5 Jul 2020 14:32:27 -0400 Subject: [PATCH 05/12] style: require java dependency for JAVA_HOME --- Library/Homebrew/dev-cmd/audit.rb | 7 --- Library/Homebrew/rubocops/text.rb | 15 ++++++ Library/Homebrew/test/rubocops/text_spec.rb | 55 +++++++++++++++++++++ 3 files changed, 70 insertions(+), 7 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index b6507d56f6..4e3b29dfbd 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -852,13 +852,6 @@ module Homebrew ) end - if line =~ /JAVA_HOME/i && - [formula.name, *formula.deps.map(&:name)].none? { |name| name.match?(/^openjdk(@|$)/) } && - formula.requirements.none? { |req| req.is_a?(JavaRequirement) } - # TODO: check could be in RuboCop - problem "Use `depends_on :java` to set JAVA_HOME" - end - return unless @strict # TODO: check could be in RuboCop diff --git a/Library/Homebrew/rubocops/text.rb b/Library/Homebrew/rubocops/text.rb index 8aa2b9ab2a..3559d12ce1 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -78,6 +78,21 @@ module RuboCop problem "Do not concatenate paths in string interpolation" end end + + find_strings(body_node).each do |n| + next unless regex_match_group(n, /JAVA_HOME/i) + + next if @formula_name.match?(/^openjdk(@|$)/) + + next if find_every_method_call_by_name(body_node, :depends_on).any? do |dependency| + dependency.each_descendant(:str).count.zero? || + regex_match_group(dependency.each_descendant(:str).first, /^openjdk(@|$)/) || + depends_on?(:java) + end + + offending_node(n) + problem "Use `depends_on :java` to set JAVA_HOME" + end end end end diff --git a/Library/Homebrew/test/rubocops/text_spec.rb b/Library/Homebrew/test/rubocops/text_spec.rb index f915daafcd..7ab156c74d 100644 --- a/Library/Homebrew/test/rubocops/text_spec.rb +++ b/Library/Homebrew/test/rubocops/text_spec.rb @@ -237,6 +237,61 @@ describe RuboCop::Cop::FormulaAudit::Text do end RUBY end + + it "When using JAVA_HOME without a java dependency" do + expect_offense(<<~RUBY) + class Foo < Formula + def install + ohai "JAVA_HOME" + ^^^^^^^^^^^ Use `depends_on :java` to set JAVA_HOME + end + end + RUBY + end + + it "When using JAVA_HOME with an openjdk dependency" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + depends_on "openjdk" + def install + ohai "JAVA_HOME" + end + end + RUBY + end + + it "When using JAVA_HOME with an openjdk build dependency" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + depends_on "openjdk" => :build + def install + ohai "JAVA_HOME" + end + end + RUBY + end + + it "When using JAVA_HOME with a java dependency" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + depends_on :java + def install + ohai "JAVA_HOME" + end + end + RUBY + end + + it "When using JAVA_HOME with a java build dependency" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + depends_on :java => :build + def install + ohai "JAVA_HOME" + end + end + RUBY + end end end From 792533462a39362771f606b56cc968e31f9259ab Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sun, 5 Jul 2020 15:02:47 -0400 Subject: [PATCH 06/12] style: don't use prefix + directory --- Library/Homebrew/dev-cmd/audit.rb | 9 --------- Library/Homebrew/rubocops/text.rb | 13 ++++++++++++ Library/Homebrew/test/rubocops/text_spec.rb | 22 +++++++++++++++++++++ 3 files changed, 35 insertions(+), 9 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 4e3b29dfbd..32927b085f 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -843,15 +843,6 @@ module Homebrew problem "Don't need to interpolate \"#{Regexp.last_match(2)}\" with #{Regexp.last_match(1)}" end - # Prefer formula path shortcuts in Pathname+ - if line =~ %r{\(\s*(prefix\s*\+\s*(['"])(bin|include|libexec|lib|sbin|share|Frameworks)[/'"])} - # TODO: check could be in RuboCop - problem( - "\"(#{Regexp.last_match(1)}...#{Regexp.last_match(2)})\" should" \ - " be \"(#{Regexp.last_match(3).downcase}+...)\"", - ) - end - return unless @strict # TODO: check could be in RuboCop diff --git a/Library/Homebrew/rubocops/text.rb b/Library/Homebrew/rubocops/text.rb index 3559d12ce1..aa9ea3a726 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -93,6 +93,19 @@ module RuboCop offending_node(n) problem "Use `depends_on :java` to set JAVA_HOME" end + + find_strings(body_node).each do |n| + # Skip strings that don't start with one of the keywords + next unless regex_match_group(n, %r{^(bin|include|libexec|lib|sbin|share|Frameworks)/?}) + + parent = n.parent + # Only look at keywords that have `prefix` before them + prefix_keyword_regex = %r{(prefix\s*\+\s*["'](bin|include|libexec|lib|sbin|share|Frameworks))["'/]} + if match = parent.source.match(prefix_keyword_regex) + offending_node(parent) + problem "Use `#{match[2].downcase}` instead of `#{match[1]}\"`" + end + end end end end diff --git a/Library/Homebrew/test/rubocops/text_spec.rb b/Library/Homebrew/test/rubocops/text_spec.rb index 7ab156c74d..82545b3fc1 100644 --- a/Library/Homebrew/test/rubocops/text_spec.rb +++ b/Library/Homebrew/test/rubocops/text_spec.rb @@ -292,6 +292,28 @@ describe RuboCop::Cop::FormulaAudit::Text do end RUBY end + + it "When using `prefix + \"bin\"` instead of `bin`" do + expect_offense(<<~RUBY) + class Foo < Formula + def install + ohai prefix + "bin" + ^^^^^^^^^^^^^^ Use `bin` instead of `prefix + "bin"` + end + end + RUBY + end + + it "When using `prefix + \"bin/foo\"` instead of `bin`" do + expect_offense(<<~RUBY) + class Foo < Formula + def install + ohai prefix + "bin/foo" + ^^^^^^^^^^^^^^^^^^ Use `bin` instead of `prefix + "bin"` + end + end + RUBY + end end end From 1859162735cee39438253e51ef66601efc9c666a Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sun, 5 Jul 2020 19:58:26 -0400 Subject: [PATCH 07/12] style: use pkgshare instead of share/foo --- Library/Homebrew/dev-cmd/audit.rb | 16 +--- Library/Homebrew/rubocops/text.rb | 14 +++ Library/Homebrew/test/dev-cmd/audit_spec.rb | 54 ------------ Library/Homebrew/test/rubocops/text_spec.rb | 96 +++++++++++++++++++++ 4 files changed, 113 insertions(+), 67 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 32927b085f..e25c30e786 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -848,20 +848,10 @@ module Homebrew # TODO: check could be in RuboCop problem "`#{Regexp.last_match(1)}` is now unnecessary" if line =~ /(require ["']formula["'])/ - if line.match?(%r{#\{share\}/#{Regexp.escape(formula.name)}[/'"]}) - # TODO: check could be in RuboCop - problem "Use \#{pkgshare} instead of \#{share}/#{formula.name}" - end + return unless !@core_tap && line =~ /depends_on .+ if build\.with(out)?\?\(?["']\w+["']\)?/ - if !@core_tap && line =~ /depends_on .+ if build\.with(out)?\?\(?["']\w+["']\)?/ - # TODO: check could be in RuboCop - problem "`Use :optional` or `:recommended` instead of `#{Regexp.last_match(0)}`" - end - - if line =~ %r{share(\s*[/+]\s*)(['"])#{Regexp.escape(formula.name)}(?:\2|/)} - # TODO: check could be in RuboCop - problem "Use pkgshare instead of (share#{Regexp.last_match(1)}\"#{formula.name}\")" - end + # TODO: check could be in RuboCop + problem "`Use :optional` or `:recommended` instead of `#{Regexp.last_match(0)}`" end def audit_reverse_migration diff --git a/Library/Homebrew/rubocops/text.rb b/Library/Homebrew/rubocops/text.rb index aa9ea3a726..28456e8e93 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -121,6 +121,20 @@ module RuboCop problem "`env :userpaths` in homebrew/core formulae is deprecated" end + body_node.each_descendant(:dstr) do |dstr_node| + next unless match = dstr_node.source.match(%r{(\#{share}/#{Regexp.escape(@formula_name)})[ /"]}) + + offending_node(dstr_node) + problem "Use `\#{pkgshare}` instead of `#{match[1]}`" + end + + find_every_method_call_by_name(body_node, :share).each do |share_node| + if match = share_node.parent.source.match(%r{(share\s*[/+]\s*"#{Regexp.escape(@formula_name)})[/"]}) + offending_node(share_node.parent) + problem "Use `pkgshare` instead of `#{match[1]}\"`" + end + end + return unless formula_tap == "homebrew-core" find_method_with_args(body_node, :env, :std) do diff --git a/Library/Homebrew/test/dev-cmd/audit_spec.rb b/Library/Homebrew/test/dev-cmd/audit_spec.rb index a7b770592f..75a5fa8af8 100644 --- a/Library/Homebrew/test/dev-cmd/audit_spec.rb +++ b/Library/Homebrew/test/dev-cmd/audit_spec.rb @@ -179,60 +179,6 @@ module Homebrew end end - # Intentionally outputted non-interpolated strings - # rubocop:disable Lint/InterpolationCheck - describe "#line_problems" do - specify "pkgshare" do - fa = formula_auditor "foo", <<~RUBY, strict: true - class Foo < Formula - url "https://brew.sh/foo-1.0.tgz" - end - RUBY - - fa.line_problems 'ohai "#{share}/foo"', 3 - expect(fa.problems.shift).to eq("Use \#{pkgshare} instead of \#{share}/foo") - - fa.line_problems 'ohai "#{share}/foo/bar"', 3 - expect(fa.problems.shift).to eq("Use \#{pkgshare} instead of \#{share}/foo") - - fa.line_problems 'ohai share/"foo"', 3 - expect(fa.problems.shift).to eq('Use pkgshare instead of (share/"foo")') - - fa.line_problems 'ohai share/"foo/bar"', 3 - expect(fa.problems.shift).to eq('Use pkgshare instead of (share/"foo")') - - fa.line_problems 'ohai "#{share}/foo-bar"', 3 - expect(fa.problems).to eq([]) - - fa.line_problems 'ohai share/"foo-bar"', 3 - expect(fa.problems).to eq([]) - - fa.line_problems 'ohai share/"bar"', 3 - expect(fa.problems).to eq([]) - end - - # Regression test for https://github.com/Homebrew/legacy-homebrew/pull/48744 - # Formulae with "++" in their name would break various audit regexps: - # Error: nested *?+ in regexp: /^libxml++3\s/ - specify "++ in name" do - fa = formula_auditor "foolibc++", <<~RUBY, strict: true - class Foolibcxx < Formula - desc "foolibc++ is a test" - url "https://brew.sh/foo-1.0.tgz" - end - RUBY - - fa.line_problems 'ohai "#{share}/foolibc++"', 3 - expect(fa.problems.shift) - .to eq("Use \#{pkgshare} instead of \#{share}/foolibc++") - - fa.line_problems 'ohai share/"foolibc++"', 3 - expect(fa.problems.shift) - .to eq('Use pkgshare instead of (share/"foolibc++")') - end - end - # rubocop:enable Lint/InterpolationCheck - describe "#audit_github_repository" do specify "#audit_github_repository when HOMEBREW_NO_GITHUB_API is set" do ENV["HOMEBREW_NO_GITHUB_API"] = "1" diff --git a/Library/Homebrew/test/rubocops/text_spec.rb b/Library/Homebrew/test/rubocops/text_spec.rb index 82545b3fc1..c357375ae4 100644 --- a/Library/Homebrew/test/rubocops/text_spec.rb +++ b/Library/Homebrew/test/rubocops/text_spec.rb @@ -342,5 +342,101 @@ describe RuboCop::Cop::FormulaAuditStrict::Text do end RUBY end + + it "when `\#{share}/foo` is used instead of `\#{pkgshare}`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai "\#{share}/foo" + ^^^^^^^^^^^^^^ Use `\#{pkgshare}` instead of `\#{share}/foo` + end + end + RUBY + end + + it "when `\#{share}/foo/bar` is used instead of `\#{pkgshare}/bar`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai "\#{share}/foo/bar" + ^^^^^^^^^^^^^^^^^^ Use `\#{pkgshare}` instead of `\#{share}/foo` + end + end + RUBY + end + + it "when `\#{share}/foolibc++` is used instead of `\#{pkgshare}/foolibc++`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foolibc++.rb") + class Foo < Formula + def install + ohai "\#{share}/foolibc++" + ^^^^^^^^^^^^^^^^^^^^ Use `\#{pkgshare}` instead of `\#{share}/foolibc++` + end + end + RUBY + end + + it "when `share/\"foo\"` is used instead of `pkgshare`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai share/"foo" + ^^^^^^^^^^^ Use `pkgshare` instead of `share/"foo"` + end + end + RUBY + end + + it "when `share/\"foo/bar\"` is used instead of `pkgshare`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai share/"foo/bar" + ^^^^^^^^^^^^^^^ Use `pkgshare` instead of `share/"foo"` + end + end + RUBY + end + + it "when `share/\"foolibc++\"` is used instead of `pkgshare`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foolibc++.rb") + class Foo < Formula + def install + ohai share/"foolibc++" + ^^^^^^^^^^^^^^^^^ Use `pkgshare` instead of `share/"foolibc++"` + end + end + RUBY + end + + it "when `\#{share}/foo-bar` doesn't match formula name" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai "\#{share}/foo-bar" + end + end + RUBY + end + + it "when `share/foo-bar` doesn't match formula name" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai share/"foo-bar" + end + end + RUBY + end + + it "when `share/bar` doesn't match formula name" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai share/"bar" + end + end + RUBY + end end end From e0b5c2093abd5fe860b136385bf29b3d76d2c695 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sun, 5 Jul 2020 23:43:09 -0400 Subject: [PATCH 08/12] style: refactor OptionDeclarations cop --- Library/Homebrew/dev-cmd/audit.rb | 5 -- Library/Homebrew/rubocops/lines.rb | 52 +++++++++++--------- Library/Homebrew/test/rubocops/lines_spec.rb | 37 ++++++++------ 3 files changed, 50 insertions(+), 44 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index e25c30e786..48aea66162 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -847,11 +847,6 @@ module Homebrew # TODO: check could be in RuboCop problem "`#{Regexp.last_match(1)}` is now unnecessary" if line =~ /(require ["']formula["'])/ - - return unless !@core_tap && line =~ /depends_on .+ if build\.with(out)?\?\(?["']\w+["']\)?/ - - # TODO: check could be in RuboCop - problem "`Use :optional` or `:recommended` instead of `#{Regexp.last_match(0)}`" end def audit_reverse_migration diff --git a/Library/Homebrew/rubocops/lines.rb b/Library/Homebrew/rubocops/lines.rb index 93c9cd3ba9..a2566fb8fb 100644 --- a/Library/Homebrew/rubocops/lines.rb +++ b/Library/Homebrew/rubocops/lines.rb @@ -102,6 +102,33 @@ module RuboCop def audit_formula(_node, _class_node, _parent_class_node, body_node) problem "Use new-style option definitions" if find_method_def(body_node, :options) + if formula_tap == "homebrew-core" + # Use of build.with? implies options, which are forbidden in homebrew/core + find_instance_method_call(body_node, :build, :without?) do + problem "Formulae in homebrew/core should not use `build.without?`." + end + find_instance_method_call(body_node, :build, :with?) do + problem "Formulae in homebrew/core should not use `build.with?`." + end + + return + end + + # Matches `depends_on "foo" if build.with?("bar")` or depends_on "foo" if build.without?("bar")` + depends_on_build_regex = /depends_on .+ (if build\.with(out)?\?\(["']\w+["']\))/ + + find_instance_method_call(body_node, :build, :with?) do |n| + next unless match = n.parent.source.match(depends_on_build_regex) + + problem "Use `:optional` or `:recommended` instead of `#{match[1]}`" + end + + find_instance_method_call(body_node, :build, :without?) do |n| + next unless match = n.parent.source.match(depends_on_build_regex) + + problem "Use `:optional` or `:recommended` instead of `#{match[1]}`" + end + find_instance_method_call(body_node, :build, :without?) do |method| next unless unless_modifier?(method.parent) @@ -143,29 +170,8 @@ module RuboCop problem "Don't duplicate 'with': Use `build.with? \"#{match[1]}\"` to check for \"--with-#{match[1]}\"" end - find_instance_method_call(body_node, :build, :include?) do |method| - arg = parameters(method).first - next unless match = regex_match_group(arg, /^with(out)?-(.*)/) - - problem "Use build.with#{match[1]}? \"#{match[2]}\" instead of " \ - "build.include? 'with#{match[1]}-#{match[2]}'" - end - - find_instance_method_call(body_node, :build, :include?) do |method| - arg = parameters(method).first - next unless match = regex_match_group(arg, /^--(.*)$/) - - problem "Reference '#{match[1]}' without dashes" - end - - return if formula_tap != "homebrew-core" - - # Use of build.with? implies options, which are forbidden in homebrew/core - find_instance_method_call(body_node, :build, :without?) do - problem "Formulae in homebrew/core should not use `build.without?`." - end - find_instance_method_call(body_node, :build, :with?) do - problem "Formulae in homebrew/core should not use `build.with?`." + find_instance_method_call(body_node, :build, :include?) do + problem "`build.include?` is deprecated" end end diff --git a/Library/Homebrew/test/rubocops/lines_spec.rb b/Library/Homebrew/test/rubocops/lines_spec.rb index 95ad56c311..46d24fab94 100644 --- a/Library/Homebrew/test/rubocops/lines_spec.rb +++ b/Library/Homebrew/test/rubocops/lines_spec.rb @@ -201,6 +201,24 @@ describe RuboCop::Cop::FormulaAudit::OptionDeclarations do RUBY end + it "build.without? in dependencies" do + expect_offense(<<~RUBY) + class Foo < Formula + depends_on "bar" if build.without?("baz") + ^^^^^^^^^^^^^^^^^^^^^ Use `:optional` or `:recommended` instead of `if build.without?("baz")` + end + RUBY + end + + it "build.with? in dependencies" do + expect_offense(<<~RUBY) + class Foo < Formula + depends_on "bar" if build.with?("baz") + ^^^^^^^^^^^^^^^^^^ Use `:optional` or `:recommended` instead of `if build.with?("baz")` + end + RUBY + end + it "unless build.without? conditional" do expect_offense(<<~RUBY) class Foo < Formula @@ -279,27 +297,14 @@ describe RuboCop::Cop::FormulaAudit::OptionDeclarations do RUBY end - it "build.include? conditional" do + it "build.include? deprecated" do expect_offense(<<~RUBY) class Foo < Formula desc "foo" url 'https://brew.sh/foo-1.0.tgz' def post_install - return if build.include? "without-bar" - ^^^^^^^^^^^ Use build.without? \"bar\" instead of build.include? 'without-bar' - end - end - RUBY - end - - it "build.include? with dashed args conditional" do - expect_offense(<<~RUBY) - class Foo < Formula - desc "foo" - url 'https://brew.sh/foo-1.0.tgz' - def post_install - return if build.include? "--bar" - ^^^^^ Reference 'bar' without dashes + return if build.include? "foo" + ^^^^^^^^^^^^^^^^^^^^ `build.include?` is deprecated end end RUBY From 1a703a1234b5ce8bd934899a17fa778e3a38c301 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Mon, 6 Jul 2020 01:26:34 -0400 Subject: [PATCH 09/12] regex_match_group: handle non UTF-8 encoded strings --- Library/Homebrew/rubocops/extend/formula.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/rubocops/extend/formula.rb b/Library/Homebrew/rubocops/extend/formula.rb index 34b8a4a7f1..b7e5bfccb8 100644 --- a/Library/Homebrew/rubocops/extend/formula.rb +++ b/Library/Homebrew/rubocops/extend/formula.rb @@ -35,7 +35,7 @@ module RuboCop # Checks for regex match of pattern in the node and # sets the appropriate instance variables to report the match def regex_match_group(node, pattern) - string_repr = string_content(node) + string_repr = string_content(node).encode("UTF-8", invalid: :replace) match_object = string_repr.match(pattern) return unless match_object From 9e52712b089952d5de0df4b46e5c6abaab5805ab Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Mon, 6 Jul 2020 10:26:21 -0400 Subject: [PATCH 10/12] style: don't need require "formula" --- Library/Homebrew/dev-cmd/audit.rb | 9 ++------- Library/Homebrew/rubocops/text.rb | 14 +++++++++++++- Library/Homebrew/test/rubocops/text_spec.rb | 11 +++++++++++ 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 48aea66162..f0ddbd2965 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -838,15 +838,10 @@ module Homebrew def line_problems(line, _lineno) # Check for string interpolation of single values. - if line =~ /(system|inreplace|gsub!|change_make_var!).*[ ,]"#\{([\w.]+)\}"/ - # TODO: check could be in RuboCop - problem "Don't need to interpolate \"#{Regexp.last_match(2)}\" with #{Regexp.last_match(1)}" - end - - return unless @strict + return unless line =~ /(system|inreplace|gsub!|change_make_var!).*[ ,]"#\{([\w.]+)\}"/ # TODO: check could be in RuboCop - problem "`#{Regexp.last_match(1)}` is now unnecessary" if line =~ /(require ["']formula["'])/ + problem "Don't need to interpolate \"#{Regexp.last_match(2)}\" with #{Regexp.last_match(1)}" end def audit_reverse_migration diff --git a/Library/Homebrew/rubocops/text.rb b/Library/Homebrew/rubocops/text.rb index 28456e8e93..ff86ddd1c4 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -6,7 +6,19 @@ module RuboCop module Cop module FormulaAudit class Text < FormulaCop - def audit_formula(_node, _class_node, _parent_class_node, body_node) + def audit_formula(node, _class_node, _parent_class_node, body_node) + @full_source_content = source_buffer(node).source + + if match = @full_source_content.match(/^require ['"]formula['"]$/) + @offensive_node = node + @source_buf = source_buffer(node) + @line_no = match.pre_match.count("\n") + 1 + @column = 0 + @length = match[0].length + @offense_source_range = source_range(@source_buf, @line_no, @column, @length) + problem "`#{match}` is now unnecessary" + end + if !find_node_method_by_name(body_node, :plist_options) && find_method_def(body_node, :plist) problem "Please set plist_options when using a formula-defined plist." diff --git a/Library/Homebrew/test/rubocops/text_spec.rb b/Library/Homebrew/test/rubocops/text_spec.rb index c357375ae4..4ccb551145 100644 --- a/Library/Homebrew/test/rubocops/text_spec.rb +++ b/Library/Homebrew/test/rubocops/text_spec.rb @@ -6,6 +6,17 @@ describe RuboCop::Cop::FormulaAudit::Text do subject(:cop) { described_class.new } context "When auditing formula text" do + it "with `require \"formula\"` is present" do + expect_offense(<<~RUBY) + require "formula" + ^^^^^^^^^^^^^^^^^ `require "formula"` is now unnecessary + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + homepage "https://brew.sh" + end + RUBY + end + it "with both openssl and libressl optional dependencies" do expect_offense(<<~RUBY) class Foo < Formula From a720d45bd06fd27b490aa856e751de83e74322b1 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Mon, 6 Jul 2020 12:17:21 -0400 Subject: [PATCH 11/12] cleanup audit file --- Library/Homebrew/dev-cmd/audit.rb | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index f0ddbd2965..81ae3b5026 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -231,11 +231,6 @@ module Homebrew end def audit_file - # TODO: check could be in RuboCop - if text.to_s.match?(/inreplace [^\n]* do [^\n]*\n[^\n]*\.gsub![^\n]*\n\ *end/m) - problem "'inreplace ... do' was used for a single substitution (use the non-block form instead)." - end - if formula.core_formula? && @versioned_formula unversioned_formula = begin # build this ourselves as we want e.g. homebrew/core to be present @@ -830,20 +825,6 @@ module Homebrew end end - def audit_lines - text.without_patch.split("\n").each_with_index do |line, lineno| - line_problems(line, lineno + 1) - end - end - - def line_problems(line, _lineno) - # Check for string interpolation of single values. - return unless line =~ /(system|inreplace|gsub!|change_make_var!).*[ ,]"#\{([\w.]+)\}"/ - - # TODO: check could be in RuboCop - problem "Don't need to interpolate \"#{Regexp.last_match(2)}\" with #{Regexp.last_match(1)}" - end - def audit_reverse_migration # Only enforce for new formula being re-added to core return unless @strict From 69e89d7a0159c81cad5b40af3856bd465fa28355 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Fri, 10 Jul 2020 15:07:28 -0400 Subject: [PATCH 12/12] add TODOs for future refactoring --- Library/Homebrew/rubocops/lines.rb | 1 + Library/Homebrew/rubocops/text.rb | 1 + 2 files changed, 2 insertions(+) diff --git a/Library/Homebrew/rubocops/lines.rb b/Library/Homebrew/rubocops/lines.rb index a2566fb8fb..0310b94348 100644 --- a/Library/Homebrew/rubocops/lines.rb +++ b/Library/Homebrew/rubocops/lines.rb @@ -118,6 +118,7 @@ module RuboCop depends_on_build_regex = /depends_on .+ (if build\.with(out)?\?\(["']\w+["']\))/ find_instance_method_call(body_node, :build, :with?) do |n| + # TODO: this should be refactored to a direct method match next unless match = n.parent.source.match(depends_on_build_regex) problem "Use `:optional` or `:recommended` instead of `#{match[1]}`" diff --git a/Library/Homebrew/rubocops/text.rb b/Library/Homebrew/rubocops/text.rb index ff86ddd1c4..85f0a89ba4 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -112,6 +112,7 @@ module RuboCop parent = n.parent # Only look at keywords that have `prefix` before them + # TODO: this should be refactored to a direct method match prefix_keyword_regex = %r{(prefix\s*\+\s*["'](bin|include|libexec|lib|sbin|share|Frameworks))["'/]} if match = parent.source.match(prefix_keyword_regex) offending_node(parent)