diff --git a/Library/Homebrew/cask/cask.rb b/Library/Homebrew/cask/cask.rb index edffe9b8e1..3252cab748 100644 --- a/Library/Homebrew/cask/cask.rb +++ b/Library/Homebrew/cask/cask.rb @@ -121,11 +121,14 @@ module Cask @default_config = T.let(config || Config.new, Config) - @config = T.let(if config_path.exist? - Config.from_json(File.read(config_path), ignore_invalid_keys: true) - else - @default_config - end, Config) + @config = T.let( + if config_path.exist? + Config.from_json(File.read(config_path), ignore_invalid_keys: true) + else + @default_config + end, + Config, + ) refresh end diff --git a/Library/Homebrew/cmd/readall.rb b/Library/Homebrew/cmd/readall.rb index c34974aa75..a42f6b2fc8 100644 --- a/Library/Homebrew/cmd/readall.rb +++ b/Library/Homebrew/cmd/readall.rb @@ -38,7 +38,7 @@ module Homebrew Homebrew.with_no_api_env do if args.syntax? && args.no_named? scan_files = "#{HOMEBREW_LIBRARY_PATH}/**/*.rb" - ruby_files = Dir.glob(scan_files).grep_v(%r{/(vendor)/}) + ruby_files = Dir.glob(scan_files).grep_v(%r{/(vendor)/}).map { Pathname(it) } Homebrew.failed = true unless Readall.valid_ruby_syntax?(ruby_files) end diff --git a/Library/Homebrew/extend/os/mac/readall.rb b/Library/Homebrew/extend/os/mac/readall.rb index 3c632f70f9..c28138396d 100644 --- a/Library/Homebrew/extend/os/mac/readall.rb +++ b/Library/Homebrew/extend/os/mac/readall.rb @@ -12,8 +12,15 @@ module OS requires_ancestor { Kernel } - sig { params(tap: ::Tap, os_name: T.nilable(Symbol), arch: T.nilable(Symbol)).returns(T::Boolean) } - def valid_casks?(tap, os_name: nil, arch: ::Hardware::CPU.type) + sig { + params( + tap: ::Tap, + os_name: T.nilable(Symbol), + arch: T.nilable(Symbol), + files: T.nilable(T::Array[::Pathname]), + ).returns(T::Boolean) + } + def valid_casks?(tap, os_name: nil, arch: ::Hardware::CPU.type, files: nil) return super if os_name == :linux current_macos_version = if os_name.is_a?(Symbol) @@ -23,7 +30,7 @@ module OS end success = T.let(true, T::Boolean) - tap.cask_files.each do |file| + (files || tap.cask_files).each do |file| cask = ::Cask::CaskLoader.load(file) # Fine to have missing URLs for unsupported macOS diff --git a/Library/Homebrew/readall.rb b/Library/Homebrew/readall.rb index bbd3d06d54..b9785bad71 100644 --- a/Library/Homebrew/readall.rb +++ b/Library/Homebrew/readall.rb @@ -3,34 +3,59 @@ require "formula" require "cask/cask_loader" -require "system_command" +require "tempfile" require "utils/output" # Helper module for validating syntax in taps. module Readall extend T::Generic extend Cachable - extend SystemCommand::Mixin extend Utils::Output::Mixin Cache = type_template { { fixed: T::Hash[Symbol, T.untyped] } } private_class_method :cache + MIN_FILES_PER_WORKER = 4 + private_constant :MIN_FILES_PER_WORKER + + # Buffers Ruby compile warnings from {.syntax_errors_or_warnings?} so they + # can be filtered before being printed to `$stderr`. + module WarningBuffer + sig { params(message: String, category: T.nilable(Symbol)).void } + def warn(message, category: nil) + buffer = Readall.warning_buffer + buffer ? buffer << message : super + end + end + private_constant :WarningBuffer + Warning.singleton_class.prepend(WarningBuffer) + + @warning_buffer = T.let(nil, T.nilable(T::Array[String])) + + class << self + sig { returns(T.nilable(T::Array[String])) } + attr_accessor :warning_buffer + end + sig { params(ruby_files: T::Array[Pathname]).returns(T::Boolean) } def self.valid_ruby_syntax?(ruby_files) - failed = T.let(false, T::Boolean) - ruby_files.each do |ruby_file| - # As a side effect, print syntax errors/warnings to `$stderr`. - failed = true if syntax_errors_or_warnings?(ruby_file) + parallel_slices_valid?(ruby_files) do |files| + failed = T.let(false, T::Boolean) + files.each do |ruby_file| + # As a side effect, print syntax errors/warnings to `$stderr`. + failed = true if syntax_errors_or_warnings?(ruby_file) + end + !failed end - !failed end sig { params(alias_dir: Pathname, formula_dir: Pathname).returns(T::Boolean) } def self.valid_aliases?(alias_dir, formula_dir) return true unless alias_dir.directory? + formula_basenames = Set.new(formula_dir.glob("**/*.rb").map { |formula_file| formula_file.basename.to_s }) + failed = T.let(false, T::Boolean) alias_dir.each_child do |f| if !f.symlink? @@ -41,7 +66,7 @@ module Readall failed = true end - if formula_dir.glob("**/#{f.basename}.rb").any?(&:exist?) + if formula_basenames.include?("#{f.basename}.rb") onoe "Formula duplicating alias: #{f}" failed = true end @@ -49,12 +74,16 @@ module Readall !failed end - sig { params(tap: Tap, bottle_tag: T.nilable(Utils::Bottles::Tag)).returns(T::Boolean) } - def self.valid_formulae?(tap, bottle_tag: nil) + sig { + params( + tap: Tap, bottle_tag: T.nilable(Utils::Bottles::Tag), files: T.nilable(T::Array[Pathname]), + ).returns(T::Boolean) + } + def self.valid_formulae?(tap, bottle_tag: nil, files: nil) cache[:valid_formulae] ||= {} success = T.let(true, T::Boolean) - tap.formula_files.each do |file| + (files || tap.formula_files).each do |file| valid = cache[:valid_formulae][file] next if valid == true || valid&.include?(bottle_tag) @@ -82,8 +111,12 @@ module Readall success end - sig { params(tap: Tap, os_name: T.nilable(Symbol), arch: T.nilable(Symbol)).returns(T::Boolean) } - def self.valid_casks?(tap, os_name: nil, arch: nil) + sig { + params( + tap: Tap, os_name: T.nilable(Symbol), arch: T.nilable(Symbol), files: T.nilable(T::Array[Pathname]), + ).returns(T::Boolean) + } + def self.valid_casks?(tap, os_name: nil, arch: nil, files: nil) validating_linux = if os_name.nil? Homebrew::SimulateSystem.current_os == :linux else @@ -95,7 +128,7 @@ module Readall os_and_arch += " on #{(arch == :intel) ? "Intel x86_64" : "ARM64"}" if arch success = T.let(true, T::Boolean) - tap.cask_files.each do |file| + (files || tap.cask_files).each do |file| next if file.read.match?(/^\s*depends_on(?:\s*\(\s*|\s+)(?::macos\b|macos:)/) cask = if arch @@ -155,40 +188,129 @@ module Readall success = false unless valid_aliases end - if no_simulate - success = false unless valid_formulae?(tap) - success = false unless valid_casks?(tap) - else - os_arch_combinations.each do |os, arch| - bottle_tag = Utils::Bottles::Tag.new(system: os, arch:) - next unless bottle_tag.valid_combination? + items = tap.formula_files.map { |file| [:formula, file] } + + tap.cask_files.map { |file| [:cask, file] } - Homebrew::SimulateSystem.with(os:, arch:) do - success = false unless valid_formulae?(tap, bottle_tag:) - success = false unless valid_casks?(tap, os_name: os, arch:) + all_files_valid = parallel_slices_valid?(items) do |slice| + formula_files = slice.filter_map { |type, file| file if type == :formula } + cask_files = slice.filter_map { |type, file| file if type == :cask } + + slice_success = T.let(true, T::Boolean) + if no_simulate + slice_success = false unless valid_formulae?(tap, files: formula_files) + slice_success = false unless valid_casks?(tap, files: cask_files) + else + os_arch_combinations.each do |os, arch| + bottle_tag = Utils::Bottles::Tag.new(system: os, arch:) + next unless bottle_tag.valid_combination? + + Homebrew::SimulateSystem.with(os:, arch:) do + slice_success = false unless valid_formulae?(tap, bottle_tag:, files: formula_files) + slice_success = false unless valid_casks?(tap, os_name: os, arch:, files: cask_files) + end end end + slice_success end + success = false unless all_files_valid success end sig { params(filename: Pathname).returns(T::Boolean) } private_class_method def self.syntax_errors_or_warnings?(filename) - # Retrieve messages about syntax errors/warnings printed to `$stderr`. - _, err, status = system_command(RUBY_PATH, args: ["-c", "-w", filename], print_stderr: false).to_a + # Compile in-process (much faster than spawning `ruby -c -w` per file), + # buffering compile warnings so they can be filtered. + error = T.let(nil, T.nilable(String)) + warnings = self.warning_buffer = [] + old_verbose = $VERBOSE + $VERBOSE = true + begin + RubyVM::InstructionSequence.compile_file(filename.to_s) + rescue ScriptError, ArgumentError => e + error = "#{e.message.chomp}\n" + ensure + $VERBOSE = old_verbose + self.warning_buffer = nil + end # Ignore unnecessary warning about named capture conflicts. # See https://bugs.ruby-lang.org/issues/12359. - messages = err.lines - .grep_v(/named capture conflicts a local variable/) - .join + messages = warnings.grep_v(/named capture conflicts a local variable/).join + messages += error if error $stderr.print messages - # Only syntax errors result in a non-zero status code. To detect syntax - # warnings we also need to inspect the output to `$stderr`. - !status.success? || !messages.chomp.empty? + # Both syntax errors and syntax warnings count as failures. + !messages.chomp.empty? + end + + sig { + type_parameters(:U).params( + items: T::Array[T.type_parameter(:U)], + _block: T.proc.params(arg0: T::Array[T.type_parameter(:U)]).returns(T::Boolean), + ).returns(T::Boolean) + } + private_class_method def self.parallel_slices_valid?(items, &_block) + require "hardware" + + worker_count = [Hardware::CPU.cores, items.length / MIN_FILES_PER_WORKER].min + return yield(items) if worker_count <= 1 + + workers = items.each_slice((items.length.to_f / worker_count).ceil).map do |slice| + reader, writer = IO.pipe + stdout_file = Tempfile.new("readall-stdout") + stderr_file = Tempfile.new("readall-stderr") + pid = Process.fork do + reader.close + success = begin + # Capture output so parallel workers cannot interleave lines. + $stdout = stdout_file.to_io + $stderr = stderr_file.to_io + yield(slice) + rescue Interrupt + false + # Report any worker exception as a validation failure. + rescue Exception => e # rubocop:disable Lint/RescueException + $stderr.puts e.full_message + false + ensure + $stdout.flush + $stderr.flush + end + writer.write(Marshal.dump(success)) + writer.close + exit!(true) + end + writer.close + [pid, reader, stdout_file, stderr_file] + end + + success = T.let(true, T::Boolean) + workers.each do |pid, reader, stdout_file, stderr_file| + worker_success = begin + # The data being loaded was written by our own forked child process. + Marshal.load(reader) # rubocop:disable Security/MarshalLoad + rescue EOFError + nil + end + reader.close + Process.wait(pid) + + [stdout_file, stderr_file].each(&:rewind) + $stdout.print stdout_file.read + $stderr.print stderr_file.read + [stdout_file, stderr_file].each(&:close!) + + case worker_success + when nil + onoe "readall worker exited unexpectedly!" + success = false + when false + success = false + end + end + success end end diff --git a/Library/Homebrew/style.rb b/Library/Homebrew/style.rb index b3fb4b68fa..f4d85e1620 100644 --- a/Library/Homebrew/style.rb +++ b/Library/Homebrew/style.rb @@ -3,6 +3,7 @@ require "shellwords" require "source_location" +require "stringio" require "system_command" require "tap" require "utils/output" @@ -88,9 +89,53 @@ module Homebrew end end - rubocop_result = if files.present? && ruby_files.empty? - (output_type == :json) ? [] : true - else + rubocop_needed = files.blank? || ruby_files.any? + shell_needed = files.blank? || shell_files.any? + + actionlint_files = github_workflow_files if files.blank? && actionlint_files.blank? + has_actionlint_workflow = actionlint_files.any? do |path| + path.to_s.end_with?("/.github/workflows/actionlint.yml") + end + odebug "actionlint workflow detected. Skipping actionlint checks." if has_actionlint_workflow + actionlint_needed = files.blank? || (!has_actionlint_workflow && actionlint_files.any?) + + # Resolve the linter executables (installing them if necessary) before + # spawning threads so those threads cannot race to install formulae. + shellcheck_path = (shellcheck if shell_needed || actionlint_needed) + shfmt_path = (shfmt_executable if shell_needed) + actionlint_path = (actionlint if actionlint_needed) + + shellcheck_out = StringIO.new + shellcheck_err = StringIO.new + shfmt_out = StringIO.new + shfmt_err = StringIO.new + actionlint_out = StringIO.new + actionlint_err = StringIO.new + + # Run the shell and GitHub Actions checks on background threads with + # buffered output while RuboCop runs on the main thread. + shell_thread = Thread.new do + shellcheck_result = if shell_needed + run_shellcheck(shell_files, output_type, fix:, shellcheck_path:, + out: shellcheck_out, err: shellcheck_err) + elsif output_type == :json + [] + else + true + end + # `shellcheck --fix` and `shfmt --write` may touch the same files so + # they must not run concurrently with each other. + shfmt_result = !shell_needed || run_shfmt!(shell_files, fix:, shfmt_path:, + out: shfmt_out, err: shfmt_err) + [shellcheck_result, shfmt_result] + end + actionlint_thread = Thread.new do + !actionlint_needed || + run_actionlint!(actionlint_files, actionlint_path:, shellcheck_path:, + out: actionlint_out, err: actionlint_err) + end + + rubocop_result = if rubocop_needed run_rubocop(ruby_files, output_type, fix:, todo:, @@ -98,24 +143,23 @@ module Homebrew display_cop_names:, reset_cache:, debug:, verbose:) - end - - shellcheck_result = if files.present? && shell_files.empty? - (output_type == :json) ? [] : true + elsif output_type == :json + [] else - run_shellcheck(shell_files, output_type, fix:) + true end - shfmt_result = files.present? && shell_files.empty? - shfmt_result ||= run_shfmt!(shell_files, fix:) + shellcheck_result, shfmt_result = shell_thread.value + actionlint_result = actionlint_thread.value - actionlint_files = github_workflow_files if files.blank? && actionlint_files.blank? - has_actionlint_workflow = actionlint_files.any? do |path| - path.to_s.end_with?("/.github/workflows/actionlint.yml") + [ + [shellcheck_out, shellcheck_err], + [shfmt_out, shfmt_err], + [actionlint_out, actionlint_err], + ].each do |out, err| + $stdout.print out.string + $stderr.print err.string end - odebug "actionlint workflow detected. Skipping actionlint checks." if has_actionlint_workflow - actionlint_result = files.present? && (has_actionlint_workflow || actionlint_files.empty?) - actionlint_result ||= run_actionlint!(actionlint_files) if output_type == :json Offenses.new( @@ -200,7 +244,7 @@ module Homebrew HOMEBREW_CACHE.mkpath cache_dir = HOMEBREW_CACHE.realpath/"style" cache_env = if (!cache_dir.exist? && cache_dir.parent.writable?) || cache_dir.writable? - args << "--parallel" unless fix + args << "--parallel" FileUtils.rm_rf cache_dir if reset_cache @@ -239,10 +283,17 @@ module Homebrew end sig { - params(files: T::Array[Pathname], output_type: Symbol, fix: T::Boolean) - .returns(T.nilable(T.any(T::Boolean, T::Array[T::Hash[String, T.untyped]]))) + params( + files: T::Array[Pathname], + output_type: Symbol, + fix: T::Boolean, + shellcheck_path: T.nilable(Pathname), + out: T.any(IO, StringIO), + err: T.any(IO, StringIO), + ).returns(T.nilable(T.any(T::Boolean, T::Array[T::Hash[String, T.untyped]]))) } - def self.run_shellcheck(files, output_type, fix: false) + def self.run_shellcheck(files, output_type, fix: false, shellcheck_path: nil, out: $stdout, err: $stderr) + shellcheck_path ||= shellcheck files = shell_scripts if files.blank? files = files.map(&:realpath) # use absolute file paths @@ -252,8 +303,6 @@ module Homebrew "--enable=all", "--external-sources", "--source-path=#{HOMEBREW_LIBRARY}", - "--", - *files, ] if fix @@ -264,17 +313,23 @@ module Homebrew # -p0 (--strip=0) : do not strip path prefixes, since we are at root directory # NOTE: We use short flags for compatibility. patch_command = %w[patch -g 0 -f -d / -p0] - patches = system_command(shellcheck, args: ["--format=diff", *args]).stdout + patches = shellcheck_chunks(shellcheck_path, files, ["--format=diff", *args]).map(&:stdout).join Utils.safe_popen_write(*patch_command) { |p| p.write(patches) } if patches.present? end case output_type when :print - system shellcheck, "--format=tty", *args - $CHILD_STATUS.success? + print_args = ["--format=tty", *args] + print_args << "--color=always" if Tty.color? + results = shellcheck_chunks(shellcheck_path, files, print_args) + results.each do |result| + out.print result.stdout + err.print result.stderr + end + results.all?(&:success?) when :json - result = system_command shellcheck, args: ["--format=json", *args] - json = json_result!(result) + results = shellcheck_chunks(shellcheck_path, files, ["--format=json", *args]) + json = results.flat_map { |result| json_result!(result) } # Convert to same format as RuboCop offenses. severity_hash = { "style" => "refactor", "info" => "convention" } @@ -312,8 +367,37 @@ module Homebrew end end - sig { params(files: T::Array[Pathname], fix: T::Boolean).returns(T::Boolean) } - def self.run_shfmt!(files, fix: false) + sig { + params( + shellcheck_path: Pathname, + files: T::Array[Pathname], + args: T::Array[String], + ).returns(T::Array[SystemCommand::Result]) + } + private_class_method def self.shellcheck_chunks(shellcheck_path, files, args) + require "hardware" + + chunk_count = [Hardware::CPU.cores, files.length].min + return [] if chunk_count.zero? + + files.each_slice((files.length.to_f / chunk_count).ceil).map do |chunk| + Thread.new do + system_command shellcheck_path, args: [*args, "--", *chunk], print_stderr: false + end + end.map(&:value) + end + + sig { + params( + files: T::Array[Pathname], + fix: T::Boolean, + shfmt_path: T.nilable(Pathname), + out: T.any(IO, StringIO), + err: T.any(IO, StringIO), + ).returns(T::Boolean) + } + def self.run_shfmt!(files, fix: false, shfmt_path: nil, out: $stdout, err: $stderr) + shfmt_path ||= shfmt_executable files = shell_scripts if files.blank? # Do not format completions and Dockerfile files.delete(HOMEBREW_REPOSITORY/"completions/bash/brew") @@ -322,23 +406,27 @@ module Homebrew args = ["--language-dialect", "bash", "--indent", "2", "--case-indent", "--", *files] args.unshift("--write") if fix # need to add before "--" - require "formula" - shfmt_executable = T.cast( - Formula["shfmt"].ensure_installed!(latest: true, - reason: "formatting shell scripts", - executable: "shfmt"), - Pathname, - ) - system( - { "HOMEBREW_SHFMT" => shfmt_executable.to_s }, - shfmt, - *args, - ) - $CHILD_STATUS.success? + result = system_command shfmt, + args:, + env: { "HOMEBREW_SHFMT" => shfmt_path.to_s }, + print_stderr: false + out.print result.stdout + err.print result.stderr + result.success? end - sig { params(files: T::Array[Pathname]).returns(T::Boolean) } - def self.run_actionlint!(files) + sig { + params( + files: T::Array[Pathname], + actionlint_path: T.nilable(Pathname), + shellcheck_path: T.nilable(Pathname), + out: T.any(IO, StringIO), + err: T.any(IO, StringIO), + ).returns(T::Boolean) + } + def self.run_actionlint!(files, actionlint_path: nil, shellcheck_path: nil, out: $stdout, err: $stderr) + actionlint_path ||= actionlint + shellcheck_path ||= shellcheck files = github_workflow_files if files.blank? tap_configs = files.filter_map do |f| @@ -350,18 +438,21 @@ module Homebrew end.uniq config_file = if tap_configs.one? - tap_configs.first + tap_configs.fetch(0) else HOMEBREW_REPOSITORY/".github/actionlint.yaml" end # the ignore is to avoid false positives in e.g. actions, homebrew-test-bot - system actionlint, "-shellcheck", shellcheck, - "-config-file", config_file, - "-ignore", "image: string; options: string", - "-ignore", "label .* is unknown", - *files - $CHILD_STATUS.success? + args = ["-shellcheck", shellcheck_path, + "-config-file", config_file, + "-ignore", "image: string; options: string", + "-ignore", "label .* is unknown"] + args << "-color" if Tty.color? + result = system_command actionlint_path, args: [*args, *files], print_stderr: false + out.print result.stdout + err.print result.stderr + result.success? end sig { params(result: SystemCommand::Result).returns(T.untyped) } @@ -412,6 +503,14 @@ module Homebrew HOMEBREW_LIBRARY/"Homebrew/utils/shfmt.sh" end + sig { returns(Pathname) } + private_class_method def self.shfmt_executable + require "formula" + T.cast(Formula["shfmt"].ensure_installed!(latest: true, + reason: "formatting shell scripts", + executable: "shfmt"), Pathname) + end + sig { returns(Pathname) } def self.actionlint require "formula" diff --git a/Library/Homebrew/test/cmd/readall_spec.rb b/Library/Homebrew/test/cmd/readall_spec.rb index 4dd796a6a1..b0ab304e12 100644 --- a/Library/Homebrew/test/cmd/readall_spec.rb +++ b/Library/Homebrew/test/cmd/readall_spec.rb @@ -65,6 +65,83 @@ RSpec.describe Homebrew::Cmd::ReadallCmd do expect(success).to be false end + describe "Readall.valid_ruby_syntax?" do + it "returns true for valid Ruby files" do + file = mktmpdir/"valid.rb" + file.write "puts 1\n" + + success = T.let(false, T::Boolean) + expect { success = Readall.valid_ruby_syntax?([file]) }.not_to output.to_stderr + expect(success).to be true + end + + it "prints errors for files with invalid syntax" do + file = mktmpdir/"invalid.rb" + file.write "def foo(\n" + + success = T.let(true, T::Boolean) + expect { success = Readall.valid_ruby_syntax?([file]) }.to output(/syntax error/).to_stderr + expect(success).to be false + end + + it "prints warnings for files with questionable syntax" do + file = mktmpdir/"warning.rb" + file.write "def foo\n bar = 1\n nil\nend\n" + + success = T.let(true, T::Boolean) + expect { success = Readall.valid_ruby_syntax?([file]) }.to output(/unused variable/).to_stderr + expect(success).to be false + end + + it "aggregates failures across parallel worker processes" do + dir = mktmpdir + files = (1..9).map do |i| + file = dir/"valid#{i}.rb" + file.write "puts #{i}\n" + file + end + bad_file = dir/"invalid.rb" + bad_file.write "def foo(\n" + files << bad_file + + success = T.let(true, T::Boolean) + expect { success = Readall.valid_ruby_syntax?(files) }.to output(/syntax error/).to_stderr + expect(success).to be false + end + end + + it "validates tap files in parallel worker processes" do + tap_path = mktmpdir + cask_files = (1..8).map do |i| + file = tap_path/"Casks/linux-example#{i}.rb" + file.dirname.mkpath + file.write <<~RUBY + cask "linux-example#{i}" do + version "1.0" + sha256 arm: "0000000000000000000000000000000000000000000000000000000000000000" + url "https://example.invalid/x.tar.gz" + name "Example" + desc "Cask missing Linux stanzas" + homepage "https://example.invalid/" + binary "x" + end + RUBY + file + end + + success = T.let(true, T::Boolean) + expect do + success = Homebrew::SimulateSystem.with(os: :linux) do + Readall.valid_tap?( + instance_double(Tap, formula_files: [], cask_files:), + os_arch_combinations: [[:linux, :arm]], + ) + end + end.to output(a_string_matching(/(?=.*linux-example1\.rb)(?=.*linux-example8\.rb)/m)).to_stderr + + expect(success).to be false + end + it "explains nil sha256 values when loading tap casks on Linux" do tap_path = mktmpdir linux_cask_file = tap_path/"Casks/linux-example.rb" diff --git a/Library/Homebrew/test/style_spec.rb b/Library/Homebrew/test/style_spec.rb index 5c929c3f17..34de3183fc 100644 --- a/Library/Homebrew/test/style_spec.rb +++ b/Library/Homebrew/test/style_spec.rb @@ -52,11 +52,14 @@ RSpec.describe Homebrew::Style do end describe ".run_actionlint!" do + let(:actionlint_result) do + instance_double(SystemCommand::Result, success?: true, stdout: "", stderr: "") + end + before do allow(described_class).to receive_messages(actionlint: "actionlint", shellcheck: "shellcheck") - # Run a trivial command so $CHILD_STATUS is non-nil after the stubbed `system` call. - system("true") - allow(described_class).to receive(:system).and_return(true) + allow(Tty).to receive(:color?).and_return(false) + allow(described_class).to receive(:system_command).and_return(actionlint_result) end it "uses a tap's actionlint config when present" do @@ -69,13 +72,15 @@ RSpec.describe Homebrew::Style do tap_config = tap_path/".github/actionlint.yaml" tap_config.write "self-hosted-runner:\n labels: []\n" - expect(described_class).to receive(:system).with( - "actionlint", "-shellcheck", "shellcheck", - "-config-file", tap_config, - "-ignore", "image: string; options: string", - "-ignore", "label .* is unknown", - workflow - ) + expect(described_class).to receive(:system_command).with( + "actionlint", + args: ["-shellcheck", "shellcheck", + "-config-file", tap_config, + "-ignore", "image: string; options: string", + "-ignore", "label .* is unknown", + workflow], + print_stderr: false, + ).and_return(actionlint_result) described_class.run_actionlint!([workflow]) end @@ -87,13 +92,15 @@ RSpec.describe Homebrew::Style do workflow = workflows_dir/"ci.yml" workflow.write "name: CI" - expect(described_class).to receive(:system).with( - "actionlint", "-shellcheck", "shellcheck", - "-config-file", HOMEBREW_REPOSITORY/".github/actionlint.yaml", - "-ignore", "image: string; options: string", - "-ignore", "label .* is unknown", - workflow - ) + expect(described_class).to receive(:system_command).with( + "actionlint", + args: ["-shellcheck", "shellcheck", + "-config-file", HOMEBREW_REPOSITORY/".github/actionlint.yaml", + "-ignore", "image: string; options: string", + "-ignore", "label .* is unknown", + workflow], + print_stderr: false, + ).and_return(actionlint_result) described_class.run_actionlint!([workflow]) end @@ -111,13 +118,15 @@ RSpec.describe Homebrew::Style do workflow2 = tap2_path/".github/workflows/ci.yml" workflow2.write "name: CI" - expect(described_class).to receive(:system).with( - "actionlint", "-shellcheck", "shellcheck", - "-config-file", HOMEBREW_REPOSITORY/".github/actionlint.yaml", - "-ignore", "image: string; options: string", - "-ignore", "label .* is unknown", - workflow1, workflow2 - ) + expect(described_class).to receive(:system_command).with( + "actionlint", + args: ["-shellcheck", "shellcheck", + "-config-file", HOMEBREW_REPOSITORY/".github/actionlint.yaml", + "-ignore", "image: string; options: string", + "-ignore", "label .* is unknown", + workflow1, workflow2], + print_stderr: false, + ).and_return(actionlint_result) described_class.run_actionlint!([workflow1, workflow2]) end @@ -162,15 +171,47 @@ RSpec.describe Homebrew::Style do reason: "formatting shell scripts", executable: "shfmt") .and_return(Pathname.new("/usr/bin/shfmt")) - system("true") - expect(described_class).to receive(:system).with( - { "HOMEBREW_SHFMT" => "/usr/bin/shfmt" }, + shfmt_result = instance_double(SystemCommand::Result, success?: true, stdout: "", stderr: "") + expect(described_class).to receive(:system_command).with( HOMEBREW_LIBRARY/"Homebrew/utils/shfmt.sh", - "--language-dialect", "bash", "--indent", "2", "--case-indent", "--", shell_file - ).and_return(true) + args: ["--language-dialect", "bash", "--indent", "2", "--case-indent", "--", shell_file], + env: { "HOMEBREW_SHFMT" => "/usr/bin/shfmt" }, + print_stderr: false, + ).and_return(shfmt_result) - described_class.run_shfmt!([shell_file]) + expect(described_class.run_shfmt!([shell_file])).to be true + end + end + + describe ".run_shellcheck" do + it "runs shellcheck in parallel chunks and merges their JSON results" do + dir = mktmpdir + log = dir/"shellcheck-args.log" + fake_shellcheck = dir/"shellcheck" + fake_shellcheck.write <<~SCRIPT + #!/bin/bash + echo "$*" >> "#{log}" + echo "[]" + SCRIPT + fake_shellcheck.chmod 0755 + + files = (1..3).map do |i| + file = dir/"script#{i}.sh" + file.write "#!/bin/bash\n" + file + end + + allow(Hardware::CPU).to receive(:cores).and_return(2) + + offenses = described_class.run_shellcheck(files, :json, shellcheck_path: fake_shellcheck) + + expect(offenses).to eq [] + chunks = log.read.lines + expect(chunks.length).to eq 2 + first_chunk = chunks.find { |chunk| chunk.include?("script1.sh") } + expect(first_chunk).to include("script2.sh") + expect(first_chunk).not_to include("script3.sh") end end