From c494528f15a9cfd956b07bdada6c372a16d36d56 Mon Sep 17 00:00:00 2001 From: Michka Popoff Date: Thu, 23 Apr 2020 11:32:23 +0200 Subject: [PATCH] on_os_blocks: add audit --- Library/Homebrew/rubocops/components_order.rb | 178 +++++++-- .../test/rubocops/components_order_spec.rb | 338 ++++++++++++++++++ 2 files changed, 486 insertions(+), 30 deletions(-) diff --git a/Library/Homebrew/rubocops/components_order.rb b/Library/Homebrew/rubocops/components_order.rb index 5049598b4b..155bd18e72 100644 --- a/Library/Homebrew/rubocops/components_order.rb +++ b/Library/Homebrew/rubocops/components_order.rb @@ -10,6 +10,11 @@ module RuboCop # - `component_precedence_list` has component hierarchy in a nested list # where each sub array contains components' details which are at same precedence level class ComponentsOrder < FormulaCop + # `aspell`: options and resources should be grouped by language + COMPONENT_WHITELIST = %w[ + aspell + ].freeze + def audit_formula(_node, _class_node, _parent_class_node, body_node) component_precedence_list = [ [{ name: :include, type: :method_call }], @@ -34,6 +39,8 @@ module RuboCop [{ name: :deprecated_option, type: :method_call }], [{ name: :depends_on, type: :method_call }], [{ name: :uses_from_macos, type: :method_call }], + [{ name: :on_macos, type: :block_call }], + [{ name: :on_linux, type: :block_call }], [{ name: :conflicts_with, type: :method_call }], [{ name: :skip_clean, type: :method_call }], [{ name: :cxxstdlib_check, type: :method_call }], @@ -49,50 +56,115 @@ module RuboCop [{ name: :test, type: :block_call }], ] - @present_components = component_precedence_list.map do |components| - relevant_components = [] - components.each do |component| - case component[:type] - when :method_call - relevant_components += find_method_calls_by_name(body_node, component[:name]).to_a - when :block_call - relevant_components += find_blocks(body_node, component[:name]).to_a - when :method_definition - relevant_components << find_method_def(body_node, component[:name]) - end - end - relevant_components.delete_if(&:nil?) + @present_components, @offensive_nodes = check_order(component_precedence_list, body_node) + + component_problem @offensive_nodes[0], @offensive_nodes[1] if @offensive_nodes + + component_precedence_list = [ + [{ name: :depends_on, type: :method_call }], + [{ name: :resource, type: :block_call }], + [{ name: :patch, type: :method_call }, { name: :patch, type: :block_call }], + ] + + on_macos_blocks = find_blocks(body_node, :on_macos) + + if on_macos_blocks.length > 1 + @offensive_node = on_macos_blocks.second + @offense_source_range = on_macos_blocks.second.source_range + problem "there can only be one `on_macos` block in a formula." end - # Check if each present_components is above rest of the present_components - @present_components.take(@present_components.size - 1).each_with_index do |preceding_component, p_idx| - next if preceding_component.empty? + check_on_os_block_content(component_precedence_list, on_macos_blocks.first) if on_macos_blocks.any? - @present_components.drop(p_idx + 1).each do |succeeding_component| - next if succeeding_component.empty? + on_linux_blocks = find_blocks(body_node, :on_linux) - @offensive_nodes = check_precedence(preceding_component, succeeding_component) - component_problem @offensive_nodes[0], @offensive_nodes[1] if @offensive_nodes + if on_linux_blocks.length > 1 + @offensive_node = on_linux_blocks.second + @offense_source_range = on_linux_blocks.second.source_range + problem "there can only be one `on_linux` block in a formula." + end + + check_on_os_block_content(component_precedence_list, on_linux_blocks.first) if on_linux_blocks.any? + + resource_blocks = find_blocks(body_node, :resource) + resource_blocks.each do |resource_block| + on_macos_blocks = find_blocks(resource_block.body, :on_macos) + on_linux_blocks = find_blocks(resource_block.body, :on_linux) + + if on_macos_blocks.length.zero? && on_linux_blocks.length.zero? + # Found nothing. Try without .body as depending on the code, + # on_macos or on_linux might be in .body or not ... + on_macos_blocks = find_blocks(resource_block, :on_macos) + on_linux_blocks = find_blocks(resource_block, :on_linux) + + next if on_macos_blocks.length.zero? && on_linux_blocks.length.zero? + end + + @offensive_node = resource_block + @offense_source_range = resource_block.source_range + + if on_macos_blocks.length > 1 + problem "there can only be one `on_macos` block in a resource block." + next + end + + if on_linux_blocks.length > 1 + problem "there can only be one `on_linux` block in a resource block." + next + end + + if on_macos_blocks.length == 1 && on_linux_blocks.length.zero? + problem "you need to define an `on_linux` block within your resource block." + next + end + + if on_macos_blocks.length.zero? && on_linux_blocks.length == 1 + problem "you need to define an `on_macos` block within your resource block." + next + end + + on_macos_block = on_macos_blocks.first + on_linux_block = on_linux_blocks.first + + child_nodes = on_macos_block.body.child_nodes + if child_nodes[0].method_name.to_s != "url" && child_nodes[1].method_name.to_s != "sha256" + problem "only an url and a sha256 (in the right order) are allowed in a `on_macos` " \ + "block within a resource block." + next + end + + child_nodes = on_linux_block.body.child_nodes + if child_nodes[0].method_name.to_s != "url" && child_nodes[1].method_name.to_s != "sha256" + problem "only an url and a sha256 (in the right order) are allowed in a `on_linux` " \ + "block within a resource block." end end end - # `aspell`: options and resources should be grouped by language - WHITELIST = %w[ - aspell - ].freeze + def check_on_os_block_content(component_precedence_list, on_os_block) + _, offensive_node = check_order(component_precedence_list, on_os_block.body) + component_problem(*offensive_node) if offensive_node + on_os_block.body.child_nodes.each do |child| + valid_node = depends_on_node?(child) + # Check for RuboCop::AST::SendNode instances only, as we are checking the + # method_name for patches and resources. + next unless child.instance_of? RuboCop::AST::SendNode - # Method to format message for reporting component precedence violations - def component_problem(c1, c2) - return if WHITELIST.include?(@formula_name) + valid_node ||= child.method_name.to_s == "patch" + valid_node ||= child.method_name.to_s == "resource" - problem "`#{format_component(c1)}` (line #{line_number(c1)}) " \ - "should be put before `#{format_component(c2)}` " \ - "(line #{line_number(c2)})" + @offensive_node = on_os_block + @offense_source_range = on_os_block.source_range + unless valid_node + problem "`#{on_os_block.method_name}` can only include `depends_on`, `patch` and `resource` nodes." + end + end end # autocorrect method gets called just after component_problem method call def autocorrect(_node) + return if @offensive_nodes.nil? + succeeding_node = @offensive_nodes[0] preceding_node = @offensive_nodes[1] lambda do |corrector| @@ -130,6 +202,52 @@ module RuboCop return [idx, comp.index(node1), comp] if comp.member?(node1) end end + + def check_order(component_precedence_list, body_node) + present_components = component_precedence_list.map do |components| + components.flat_map do |component| + case component[:type] + when :method_call + find_method_calls_by_name(body_node, component[:name]).to_a + when :block_call + find_blocks(body_node, component[:name]).to_a + when :method_definition + find_method_def(body_node, component[:name]) + end + end.compact + end + + # Check if each present_components is above rest of the present_components + offensive_nodes = nil + present_components.take(present_components.size - 1).each_with_index do |preceding_component, p_idx| + next if preceding_component.empty? + + present_components.drop(p_idx + 1).each do |succeeding_component| + next if succeeding_component.empty? + + offensive_nodes = check_precedence(preceding_component, succeeding_component) + break if offensive_nodes + end + end + + [present_components, offensive_nodes] + end + + # Method to format message for reporting component precedence violations + def component_problem(c1, c2) + return if COMPONENT_WHITELIST.include?(@formula_name) + + problem "`#{format_component(c1)}` (line #{line_number(c1)}) " \ + "should be put before `#{format_component(c2)}` " \ + "(line #{line_number(c2)})" + end + + # Node pattern method to match + # `depends_on` variants + def_node_matcher :depends_on_node?, <<~EOS + {(if _ (send nil? :depends_on ...) nil?) + (send nil? :depends_on ...)} + EOS end end end diff --git a/Library/Homebrew/test/rubocops/components_order_spec.rb b/Library/Homebrew/test/rubocops/components_order_spec.rb index 821cbd64a1..53e04e01bf 100644 --- a/Library/Homebrew/test/rubocops/components_order_spec.rb +++ b/Library/Homebrew/test/rubocops/components_order_spec.rb @@ -90,6 +90,47 @@ describe RuboCop::Cop::FormulaAudit::ComponentsOrder do end RUBY end + + it "the on_macos block is not after uses_from_macos" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + on_macos do + depends_on "readline" + end + uses_from_macos "bar" + ^^^^^^^^^^^^^^^^^^^^^ `uses_from_macos` (line 6) should be put before `on_macos` (line 3) + end + RUBY + end + + it "the on_linux block is not after uses_from_macos" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + on_linux do + depends_on "readline" + end + uses_from_macos "bar" + ^^^^^^^^^^^^^^^^^^^^^ `uses_from_macos` (line 6) should be put before `on_linux` (line 3) + end + RUBY + end + + it "the on_linux block is not after the on_macos block" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + on_linux do + depends_on "vim" + end + on_macos do + ^^^^^^^^^^^ `on_macos` (line 6) should be put before `on_linux` (line 3) + depends_on "readline" + end + end + RUBY + end end context "When auditing formula components order with autocorrect" do @@ -141,4 +182,301 @@ describe RuboCop::Cop::FormulaAudit::ComponentsOrder do expect(corrected_source).to eq(correct_source) end end + + context "no on_os_block" do + it "does not fail when there is no on_os block" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + homepage "https://brew.sh" + + depends_on "pkg-config" => :build + + def install + end + end + RUBY + end + end + + context "on_os_block" do + it "correctly uses on_macos and on_linux blocks" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + homepage "https://brew.sh" + + depends_on "pkg-config" => :build + + uses_from_macos "libxml2" + + on_macos do + depends_on "perl" + + resource "resource1" do + url "https://brew.sh/resource1.tar.gz" + sha256 "a2f5650770e1c87fb335af19a9b7eb73fc05ccf22144eb68db7d00cd2bcb0902" + + patch do + url "https://raw.githubusercontent.com/Homebrew/formula-patches/0ae366e6/patch3.diff" + sha256 "89fa3c95c329ec326e2e76493471a7a974c673792725059ef121e6f9efb05bf4" + end + end + + resource "resource2" do + url "https://brew.sh/resource2.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + + patch do + url "https://raw.githubusercontent.com/Homebrew/formula-patches/0ae366e6/patch1.diff" + sha256 "89fa3c95c329ec326e2e76493471a7a974c673792725059ef121e6f9efb05bf4" + end + + patch do + url "https://raw.githubusercontent.com/Homebrew/formula-patches/0ae366e6/patch2.diff" + sha256 "89fa3c95c329ec326e2e76493471a7a974c673792725059ef121e6f9efb05bf4" + end + end + + on_linux do + depends_on "readline" + end + + def install + end + end + RUBY + end + end + + context "on_macos_block" do + it "correctly uses as single on_macos block" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + homepage "https://brew.sh" + + on_macos do + depends_on "readline" + end + + def install + end + end + RUBY + end + end + + context "on_linux_block" do + it "correctly uses as single on_linux block" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + homepage "https://brew.sh" + + on_linux do + depends_on "readline" + end + + def install + end + end + RUBY + end + end + + it "there can only be one on_macos block" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + on_macos do + depends_on "readline" + end + + on_macos do + ^^^^^^^^^^^ there can only be one `on_macos` block in a formula. + depends_on "foo" + end + end + RUBY + end + + it "there can only be one on_linux block" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + on_linux do + depends_on "readline" + end + + on_linux do + ^^^^^^^^^^^ there can only be one `on_linux` block in a formula. + depends_on "foo" + end + end + RUBY + end + + it "the on_macos block can only contain depends_on, patch and resource nodes" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + on_macos do + ^^^^^^^^^^^ `on_macos` can only include `depends_on`, `patch` and `resource` nodes. + depends_on "readline" + uses_from_macos "ncurses" + end + end + RUBY + end + + it "the on_linux block can only contain depends_on, patch and resource nodes" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + on_linux do + ^^^^^^^^^^^ `on_linux` can only include `depends_on`, `patch` and `resource` nodes. + depends_on "readline" + uses_from_macos "ncurses" + end + end + RUBY + end + + context "resource" do + it "correctly uses an on_macos and on_linux block" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + homepage "https://brew.sh" + + resource do + on_macos do + url "https://brew.sh/resource1.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + + on_linux do + url "https://brew.sh/resource2.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + end + end + RUBY + end + + it "there are two on_macos blocks, which is not allowed" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + + resource do + ^^^^^^^^^^^ there can only be one `on_macos` block in a resource block. + on_macos do + url "https://brew.sh/resource1.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + + on_macos do + url "https://brew.sh/resource2.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + end + end + RUBY + end + + it "there are two on_linux blocks, which is not allowed" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + + resource do + ^^^^^^^^^^^ there can only be one `on_linux` block in a resource block. + on_linux do + url "https://brew.sh/resource1.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + + on_linux do + url "https://brew.sh/resource2.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + end + end + RUBY + end + + it "there is a on_macos block but no on_linux block" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + + resource do + ^^^^^^^^^^^ you need to define an `on_linux` block within your resource block. + on_macos do + url "https://brew.sh/resource1.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + end + end + RUBY + end + + it "there is a on_linux block but no on_macos block" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + + resource do + ^^^^^^^^^^^ you need to define an `on_macos` block within your resource block. + on_linux do + url "https://brew.sh/resource1.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + end + end + RUBY + end + + it "the content of the on_macos block is wrong in a resource block" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + + resource do + ^^^^^^^^^^^ only an url and a sha256 (in the right order) are allowed in a `on_macos` block within a resource block. + on_macos do + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + url "https://brew.sh/resource2.tar.gz" + end + + on_linux do + url "https://brew.sh/resource2.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + end + end + RUBY + end + + it "the content of the on_linux block is wrong in a resource block" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + + resource do + ^^^^^^^^^^^ only an url and a sha256 (in the right order) are allowed in a `on_linux` block within a resource block. + on_macos do + url "https://brew.sh/resource2.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + + on_linux do + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + url "https://brew.sh/resource2.tar.gz" + end + end + end + RUBY + end + + include_examples "formulae exist", described_class::COMPONENT_WHITELIST + end end