Split check command into separate file (#9707)

bin/crew

@@ -15,13 +15,14 @@ require 'mkmf'
 require 'tmpdir'
 require 'uri'
 begin
-  require_relative '../commands/const'
+  require_relative '../commands/check'
 rescue LoadError
   # Older crew installs won't have the commands dir in the sparse checkout,
   # so disable sparse checkout if those files are missing.
   system 'git sparse-checkout disable'
-  require_relative '../commands/const'
+  require_relative '../commands/check'
 end
+require_relative '../commands/const'
 require_relative '../commands/files'
 require_relative '../commands/help'
 require_relative '../commands/list'
@@ -1602,53 +1603,6 @@ def upload(pkg_name = nil, pkg_version = nil, gitlab_token = nil, gitlab_token_u
   end
 end
 
-def copy_package(pkg_name, prompt_msg = '')
-  pkg_file = File.join(CREW_LOCAL_REPO_ROOT, 'packages', "#{pkg_name}.rb")
-  # Use rubocop to sanitize package file, and let errors get flagged.
-  if PackageUtils.installed?('ruby_rubocop')
-    puts "Using rubocop to sanitize #{pkg_file} .".orange
-    system 'gem install rubocop' unless Kernel.system('rubocop --help 2>/dev/null', %i[out err] => File::NULL)
-    system "rubocop -c #{File.join(CREW_LOCAL_REPO_ROOT, '.rubocop.yml')} -A #{pkg_file}", exception: true
-  else
-    puts 'Package '.lightred + 'ruby_rubocop'.orange + " is not installed. Rubocop will not be used to sanitize  #{pkg_file} . 😔 You may try this: ".lightred + 'crew install ruby_rubocop'.lightblue
-  end
-  next_pkg = nil
-  if @opt_force
-    FileUtils.cp pkg_file, "#{CREW_PACKAGES_PATH}/"
-    puts "\nCopied #{pkg_file} to #{CREW_PACKAGES_PATH}.\n".lightgreen
-  else
-    # This pulls the operation from the calling function
-    operation = caller_locations(1, 2)[1].to_s.split[3].split('_')[0]
-    puts prompt_msg.yellow
-    if Package.agree_default_yes("\nWould you like to copy #{pkg_name}.rb to crew and start the #{operation}")
-      FileUtils.cp pkg_file, "#{CREW_PACKAGES_PATH}/"
-      puts "\nCopied #{pkg_file} to #{CREW_PACKAGES_PATH}.\n".lightgreen
-    else
-      puts "#{operation.capitalize} skipped."
-      next_pkg = true
-    end
-  end
-  return next_pkg
-end
-
-def check_package(pkg_name)
-  return unless Dir.exist? CREW_LOCAL_REPO_ROOT
-  return unless File.file? "#{CREW_LOCAL_REPO_ROOT}/packages/#{pkg_name}.rb"
-  return copy_package(pkg_name) if @opt_force
-
-  # Prompt to copy the local repo package to crew if the package is not found.
-  unless File.file? "#{CREW_PACKAGES_PATH}/#{pkg_name}.rb"
-    prompt_msg = "\nThe crew package #{pkg_name} does not exist."
-    return copy_package(pkg_name, prompt_msg)
-  end
-
-  # Compare local repo package to the crew repo package and prompt to copy if necessary to prepare for the operation.
-  unless FileUtils.identical? "#{CREW_LOCAL_REPO_ROOT}/packages/#{pkg_name}.rb", "#{CREW_PACKAGES_PATH}/#{pkg_name}.rb"
-    prompt_msg = "\n#{CREW_LOCAL_REPO_ROOT}/packages/#{pkg_name}.rb does not match the crew package."
-    return copy_package(pkg_name, prompt_msg)
-  end
-end
-
 def build_command(args)
   abort 'Unable to locate local repo root directory. Change to a local chromebrew git repo directory and try again.'.lightred unless Dir.exist? CREW_LOCAL_REPO_ROOT
   abort 'Change to a local chromebrew git repo directory and try again.'.lightred if CREW_PACKAGES_PATH.include?(CREW_LOCAL_REPO_ROOT)
@@ -1669,7 +1623,7 @@ def build_command(args)
     else
       @pkg_name = name
     end
-    next if check_package(@pkg_name)
+    next unless Command.check(name, @opt_force)
 
     search @pkg_name
     print_current_package CREW_VERBOSE
@@ -1695,18 +1649,7 @@ end
 
 def check_command(args)
   args['<name>'].each do |name|
-    check_package(name)
+    Command.check(name, @opt_force)
-    search name
-    if @opt_version
-      Dir.chdir CREW_PACKAGES_PATH do
-        system "../tools/version.rb #{name} #{@short_verbose}"
-      end
-    else
-      Dir.chdir CREW_PACKAGES_PATH do
-        system "../tests/prop_test #{name}"
-        system "../tests/buildsystem_test #{name}"
-      end
-    end
   end
 end
 
@@ -1787,7 +1730,7 @@ def install_command(args)
       puts "Package #{@pkg_name} already installed, skipping...".lightgreen
       next
     end
-    next if check_package(@pkg_name)
+    next unless Command.check(name, @opt_force)
     search @pkg_name
     print_current_package true
     @pkg.build_from_source = true if @opt_source || @opt_recursive || CREW_BUILD_FROM_SOURCE
@@ -1825,7 +1768,7 @@ end
 def reinstall_command(args)
   args['<name>'].each do |name|
     @pkg_name = name
-    next if check_package(@pkg_name)
+    next unless Command.check(name, @opt_force)
     search @pkg_name
     print_current_package
     @pkg.build_from_source = true if @opt_source || @opt_recursive || CREW_BUILD_FROM_SOURCE

commands/check.rb

@@ -0,0 +1,63 @@
+require 'fileutils'
+require_relative '../lib/const'
+require_relative '../lib/package'
+require_relative '../lib/package_utils'
+
+class Command
+  def self.check(name, force)
+    local_package = File.join(CREW_LOCAL_REPO_ROOT, 'packages', "#{name}.rb")
+    crew_package = File.join(CREW_PACKAGES_PATH, "#{name}.rb")
+
+    # We return true here in order to exit early but behave as if the check passed, so that other operations can continue.
+    unless File.file?(local_package)
+      # If the operation is a bare 'crew check', then we don't want to silently skip it.
+      puts 'No local package file found, skipping check.'.lightred if caller_locations(1, 2)[0].to_s.split[3].split('_')[0].split('#')[1].to_s == 'check'
+      return true
+    end
+
+    # Use rubocop to sanitize package file, and let errors get flagged.
+    if PackageUtils.installed?('ruby_rubocop')
+      puts "Using rubocop to sanitize #{local_package}".orange
+      system "rubocop -c #{File.join(CREW_LOCAL_REPO_ROOT, '.rubocop.yml')} -A #{local_package}", exception: true
+    else
+      puts "Rubocop is not installed, and thus will not be used to sanitize #{local_package}".lightred
+      puts 'To install Rubocop, run the following command: '.lightred + 'crew install ruby_rubocop'.lightblue
+    end
+
+    to_copy = force
+
+    # Prompt to copy the local repo package to crew if the package is not found.
+    unless force || File.file?(crew_package)
+      puts "The crew package '#{name}' does not exist."
+      to_copy = true
+    end
+
+    # Compare local repo package to the crew repo package and prompt to copy if necessary to prepare for the operation.
+    unless force || (File.file?(crew_package) && FileUtils.identical?(local_package, crew_package))
+      puts "#{CREW_LOCAL_REPO_ROOT}/packages/#{name}.rb does not match the crew package."
+      to_copy = true
+    end
+
+    if to_copy && !force
+      # This pulls the operation from the calling function.
+      operation = caller_locations(1, 2)[0].to_s.split[3].split('_')[0].split('#')[1]
+      if Package.agree_default_yes("\nWould you like to copy #{name}.rb to crew and start the #{operation}")
+        to_copy = true
+      else
+        return false
+      end
+    end
+
+    if to_copy
+      FileUtils.copy_file(local_package, crew_package)
+      puts "Copied #{local_package} to #{CREW_PACKAGES_PATH}".lightgreen
+    end
+
+    # Run property and buildsystem tests on the package, and fail if they fail.
+    return false unless system "#{CREW_LIB_PATH}/tests/prop_test #{name}"
+    return false unless system "#{CREW_LIB_PATH}/tests/buildsystem_test #{name}"
+
+    # If we're still here every test has passed, so return true.
+    return true
+  end
+end

commands/help.rb

@@ -15,10 +15,10 @@ class Command
       EOT
     when 'check'
       puts <<~EOT
-        Check package(s) for syntax errors and upstream updates.
+        Check package(s) for syntax errors, and copy local packages to the chromebrew directory.
-        Usage: crew check [-V|--version] [-v|--verbose] <package1> [<package2> ...]
+        Usage: crew check [-f|--force] <package1> [<package2> ...]
-        If `-V` or `--version` is present, it will search for an upstream update.
+        Local packages will be copied to the chromebrew directory if they do not exist there, or if they are different to the chromebrew package files.
-        If `-v` or `--verbose` is present, up to date packages will be displayed.
+        If `-f` or `--force` is present, packages will be copied without question.
       EOT
     when 'const'
       puts <<~EOT

lib/const.rb

@@ -3,7 +3,7 @@
 require 'etc'
 
 OLD_CREW_VERSION ||= defined?(CREW_VERSION) ? CREW_VERSION : '1.0'
-CREW_VERSION ||= '1.57.2' unless defined?(CREW_VERSION) && CREW_VERSION == OLD_CREW_VERSION
+CREW_VERSION ||= '1.57.3' unless defined?(CREW_VERSION) && CREW_VERSION == OLD_CREW_VERSION
 
 # Kernel architecture.
 KERN_ARCH ||= Etc.uname[:machine]
@@ -330,18 +330,18 @@ CREW_DOCOPT ||= <<~DOCOPT
   Chromebrew - Package manager for Chrome OS https://chromebrew.github.io
 
   Usage:
-    crew build [options] [-k|--keep] [-v|--verbose] <name> ...
+    crew build [options] [-f|--force] [-k|--keep] [-v|--verbose] <name> ...
-    crew check [options] [-V|--version] [-v|--verbose] <name> ...
+    crew check [-f|--force] <name> ...
     crew const [options] [-v|--verbose] [<name> ...]
     crew deps [options] [--deep] [-t|--tree] [-b|--include-build-deps] [--exclude-buildessential] [-v|--verbose] <name> ...
     crew download [options] [-s|--source] [-v|--verbose] <name> ...
     crew files [options] <name> ...
     crew help [options] [<command>] [-v|--verbose] [<subcommand>]
-    crew install [options] [-k|--keep] [-s|--source] [-S|--recursive-build] [-v|--verbose] <name> ...
+    crew install [options] [-f|--force] [-k|--keep] [-s|--source] [-S|--recursive-build] [-v|--verbose] <name> ...
     crew list [options] [-v|--verbose] (available|compatible|incompatible|essential|installed)
     crew postinstall [options] [-v|--verbose] <name> ...
     crew prop [options] [<property>]
-    crew reinstall [options] [-k|--keep] [-s|--source] [-S|--recursive-build] [-v|--verbose] <name> ...
+    crew reinstall [options] [-f|--force] [-k|--keep] [-s|--source] [-S|--recursive-build] [-v|--verbose] <name> ...
     crew remove [options] [-v|--verbose] <name> ...
     crew search [options] [-v|--verbose] <name> ...
     crew sysinfo [options] [-v|--verbose]

tests/buildsystem_test

@@ -31,7 +31,8 @@ end
 if ARGV[0]
   ARGV.each do |name|
     if File.file?(File.join(CREW_PACKAGES_PATH, "#{name}.rb"))
-      check_buildsystem(File.join(CREW_PACKAGES_PATH, "#{name}.rb"), verbose: true)
+      test_result = check_buildsystem(File.join(CREW_PACKAGES_PATH, "#{name}.rb"), verbose: true)
+      exit(1) if test_result.positive?
     else
       puts "Package #{name} not found.".lightred
     end

tests/commands/remove.rb

@@ -19,15 +19,17 @@ class RemoveCommandTest < Minitest::Test
     end
   end
 
+  # We've chosen zstd here because its an essential package we can remove and quickly reinstall without breaking crew.
+  # If this changes, and there isn't an essential package we can remove without catastrophic failure, feel free to remove this test entirely.
   def test_force_remove_essential_package
-    puts 'Testing the forced removal of essential package zlib. This should succeed.'
+    puts 'Testing the forced removal of essential package zstd. This should succeed.'
 
-    expected_output = "zlib removed!\n"
+    expected_output = "zstd removed!\n"
     assert_output expected_output, nil do
-      Command.remove(Package.load_package('zlib.rb'), force: true)
+      Command.remove(Package.load_package('zstd.rb'), force: true)
     end
     # We did just remove an essential package, so let's reinstall that now before it causes any issues.
-    system 'crew install zlib', %i[out err] => File::NULL
+    system 'crew install zstd', %i[out err] => File::NULL
   end
 
   def test_remove_package_with_essential_file

tests/dep_test

@@ -20,7 +20,8 @@ end
 if ARGV[0]
   ARGV.each do |name|
     if File.file?(File.join(CREW_PACKAGES_PATH, "#{name}.rb"))
-      check_for_removed_dependencies(Package.load_package(File.join(CREW_PACKAGES_PATH, "#{name}.rb")), verbose: true)
+      test_result = check_for_removed_dependencies(Package.load_package(File.join(CREW_PACKAGES_PATH, "#{name}.rb")), verbose: true)
+      exit(1) if test_result.positive?
     else
       puts "Package #{name} not found.".lightred
     end

tests/prop_test

@@ -43,7 +43,8 @@ end
 if ARGV[0]
   ARGV.each do |name|
     if File.file?(File.join(CREW_PACKAGES_PATH, "#{name}.rb"))
-      check_properties(Package.load_package(File.join(CREW_PACKAGES_PATH, "#{name}.rb")), verbose: true)
+      test_result = check_properties(Package.load_package(File.join(CREW_PACKAGES_PATH, "#{name}.rb")), verbose: true)
+      exit(1) if test_result.positive?
     else
       puts "Package #{name} not found.".lightred
     end