From 0c86288d245f254d1f6f54787d0e7c0af60d666d Mon Sep 17 00:00:00 2001 From: Maximilian Downey Twiss Date: Tue, 18 Mar 2025 09:42:05 +1100 Subject: [PATCH] Clean up rubocop configuration pt.1 (#11566) --- .pre-commit-config.yaml | 4 +-- .rubocop.yml | 54 +++++++++++++++++++----------------- bin/crew | 16 +++++------ lib/const.rb | 14 +++++----- lib/convenience_functions.rb | 2 +- lib/deb_utils.rb | 2 +- lib/downloader.rb | 16 +++++------ lib/package.rb | 10 +++---- lib/selector.rb | 1 + packages/firefox.rb | 2 +- packages/foremost.rb | 4 +-- packages/nss.rb | 4 +-- packages/qemacs.rb | 12 ++++---- packages/vifm.rb | 8 +++--- tools/getrealdeps.rb | 4 +-- 15 files changed, 78 insertions(+), 75 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 1027c3b52..9c68ce141 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -3,7 +3,7 @@ # These pre-commit hooks mirror the Github CI, so running them locally will catch errors earlier. repos: - repo: https://github.com/rubocop/rubocop - rev: v1.64.1 + rev: v1.74.0 hooks: - id: rubocop - repo: https://github.com/syntaqx/git-hooks @@ -17,6 +17,6 @@ repos: - id: markdownlint args: [-s.mdl_style.rb] - repo: https://github.com/adrienverge/yamllint - rev: v1.35.1 + rev: v1.36.1 hooks: - id: yamllint diff --git a/.rubocop.yml b/.rubocop.yml index 59679c6d2..a058d7792 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -22,9 +22,7 @@ Lint/OrAssignmentToConstant: - 'lib/const.rb' - 'lib/fixup.rb' -Lint/SuppressedException: - Enabled: false - +# TODO Layout/HashAlignment: EnforcedHashRocketStyle: - separator @@ -35,83 +33,89 @@ Layout/HashAlignment: # End temporarily disabled cops +# These cops have either been disabled permanently or have a non-default configuration option. +# If you permanently disable another cop, please add a comment explaining why. +# If you set a non-default configuration for a cop, please add a comment explaining why. + +# An empty line after a guard clause can improve readability, but mandating it reduces compactness. Layout/EmptyLineAfterGuardClause: Enabled: false +# TODO Layout/FirstHashElementIndentation: Enabled: false # EnforcedStyle: consistent +# A maximum line length tends to impede development more than it helps readability. Layout/LineLength: Enabled: false +# Having an explicit return can often improve readability. Style/RedundantReturn: Enabled: false +# A comment at the top of every file is not worth the minor benefits of frozen strings. +# Besides, Ruby is in the process of freezing strings by default anyway. Style/FrozenStringLiteralComment: Enabled: false +# Explicitly freezing every constant is too verbose, and provides minor benefits regardless. Style/MutableConstant: Enabled: false +# Mandating top-level documentation comments and the like will more often than not lead to meaningless documentation. +# Let people choose what does and does not need to be documented. Style/Documentation: Enabled: false +# While using a guard clause reduces indentation and lines of code, it is not always the most logical flow. +# Let people choose which is best depending on the scenario. Style/GuardClause: Enabled: false -Style/DocumentDynamicEvalDefinition: - Enabled: false - -# https://github.com/rubocop/rubocop/issues/12132 -Style/OptionalBooleanParameter: - Enabled: false - +# TODO Style/ClassVars: Enabled: false +# TODO Style/GlobalVars: Enabled: false +# TODO Style/FormatStringToken: Enabled: false +# TODO Style/MultilineBlockChain: Enabled: false +# The only numeric literals in the codebase long enough to trip this are kilobyte sizes, for which a thousands separator does not improve readability. Style/NumericLiterals: Enabled: false -Style/RedundantCapitalW: - Enabled: false - -Style/RedundantLineContinuation: - Enabled: false - +# TODO Style/RedundantStringEscape: Enabled: false +# Enabling Metrics results in offenses that essentially mandate the arbitrary splitting of logic into additional files and methods. +# Even the rubocop codebase itself often inline disables these offfenses, so lets save the time and disable them all here. Metrics: Enabled: false -Naming/AccessorMethodName: - Enabled: false - -Naming/BlockParameterName: - Enabled: false - +# We disable this for packages because we don't want to incorrectly convert things like Xz_java into XzJava. +# Many package names include - or _ and we don't want to lose that information by mistakenly converting into CamelCase. Naming/ClassAndModuleCamelCase: Exclude: - 'packages/*' +# We allow all caps variables to mirror our constants, so things like @ARCH_LDFLAGS are allowed. Naming/VariableName: EnforcedStyle: snake_case # Allow variable names which are in all caps (with optional numbers and underscores) AllowedPatterns: - '^@?[A-Z0-9_]+$' -Naming/VariableNumber: - Enabled: false - +# If there is meaningful information to convey about the contents of a heredoc, a comment is the best way to do it. +# Forcing unique names for all heredoc delimiters does not improve documentation in the vast majority of cases. Naming/HeredocDelimiterNaming: Enabled: false diff --git a/bin/crew b/bin/crew index b00baa68f..3964761a5 100755 --- a/bin/crew +++ b/bin/crew @@ -139,7 +139,7 @@ at_exit do ExitMessage.print end -def print_current_package(extra = false) +def print_current_package(extra: false) status = if PackageUtils.installed?(@pkg.name) :installed elsif !PackageUtils.compatible?(@pkg) @@ -167,7 +167,7 @@ def print_current_package(extra = false) puts '' end -def set_package(pkg_path) +def check_load_package(pkg_path) begin @pkg = Package.load_package(pkg_path) rescue SyntaxError => e @@ -179,7 +179,7 @@ end def search(pkg_name, pkg_path: File.join(CREW_PACKAGES_PATH, "#{pkg_name}.rb"), silent: false) begin - return set_package(pkg_path) if File.file?(pkg_path) + return check_load_package(pkg_path) if File.file?(pkg_path) rescue StandardError => e puts "Error with #{pkg_name}.rb: #{e}".lightred unless e.to_s.include?('uninitialized constant') end @@ -486,7 +486,7 @@ def download end end # Download file if not cached. - downloader url, sha256sum, filename, CREW_VERBOSE + downloader url, sha256sum, filename, verbose: CREW_VERBOSE puts "#{@pkg.name.capitalize} archive downloaded.".lightgreen # Stow file in cache if requested, if file is not from cache, @@ -513,7 +513,7 @@ def download else unless git # We don't want to download a git repository as a file. FileUtils.mkdir_p @extract_dir - downloader url, sha256sum, filename, CREW_VERBOSE + downloader url, sha256sum, filename, verbose: CREW_VERBOSE puts "#{filename}: File downloaded.".lightgreen @@ -1625,7 +1625,7 @@ def build_command(args) next unless Command.check(name, @opt_force) search @pkg_name - print_current_package CREW_VERBOSE + print_current_package extra: CREW_VERBOSE next unless @pkg_name # Process preflight block to see if package should be built @@ -1680,7 +1680,7 @@ def download_command(args) @pkg_name = name search @pkg_name @pkg.build_from_source = true if @opt_source - print_current_package CREW_VERBOSE + print_current_package extra: CREW_VERBOSE if ARGV.intersect?(%w[download]) && @pkg.is_fake? fake_pkg_deplist = @pkg.get_deps_list(return_attr: true).flat_map(&:keys).uniq until fake_pkg_deplist.blank? @@ -1731,7 +1731,7 @@ def install_command(args) end next unless Command.check(name, @opt_force) search @pkg_name - print_current_package true + print_current_package extra: true @pkg.build_from_source = true if @opt_source || @opt_recursive || CREW_BUILD_FROM_SOURCE next unless @pkg_name diff --git a/lib/const.rb b/lib/const.rb index c947f1cf4..e96d51c1e 100644 --- a/lib/const.rb +++ b/lib/const.rb @@ -3,7 +3,7 @@ require 'etc' OLD_CREW_VERSION ||= defined?(CREW_VERSION) ? CREW_VERSION : '1.0' -CREW_VERSION ||= '1.57.8' unless defined?(CREW_VERSION) && CREW_VERSION == OLD_CREW_VERSION +CREW_VERSION ||= '1.57.9' unless defined?(CREW_VERSION) && CREW_VERSION == OLD_CREW_VERSION # Kernel architecture. KERN_ARCH ||= Etc.uname[:machine] @@ -61,7 +61,7 @@ else end # Use sane minimal defaults if in container and no override specified. -CREW_KERNEL_VERSION ||= \ +CREW_KERNEL_VERSION ||= if CREW_IN_CONTAINER && ENV.fetch('CREW_KERNEL_VERSION', nil).nil? ARCH.eql?('i686') ? '3.8' : '5.10' else @@ -106,7 +106,7 @@ CREW_FORCE ||= ARGV.intersect?(%w[-f --force]) unless defined?(CREW_FORCE) CREW_VERBOSE ||= ARGV.intersect?(%w[-v --verbose]) unless defined?(CREW_VERBOSE) # Set CREW_NPROC from environment variable, `distcc -j`, or `nproc`. -CREW_NPROC ||= \ +CREW_NPROC ||= if File.file?("#{CREW_PREFIX}/bin/distcc") ENV.fetch('CREW_NPROC', `distcc -j`.chomp) else @@ -139,7 +139,7 @@ CREW_BRANCH ||= ENV.fetch('CREW_BRANCH', 'master') unless defined?(CREW_BRANCH) USER ||= Etc.getlogin unless defined?(USER) unless defined?(CHROMEOS_RELEASE) - CHROMEOS_RELEASE = \ + CHROMEOS_RELEASE = if File.exist?('/etc/lsb-release') File.read('/etc/lsb-release')[/CHROMEOS_RELEASE_CHROME_MILESTONE||=(.+)/, 1] else @@ -168,7 +168,7 @@ CREW_DOWNLOADER_RETRY ||= ENV.fetch('CREW_DOWNLOADER_RETRY', 3).to_i unless defi CREW_HIDE_PROGBAR = ENV.fetch('CREW_HIDE_PROGBAR', false) unless defined?(CREW_HIDE_PROGBAR) # set certificate file location for lib/downloader.rb -SSL_CERT_FILE ||= \ +SSL_CERT_FILE ||= if ENV['SSL_CERT_FILE'] && File.exist?(ENV['SSL_CERT_FILE']) ENV['SSL_CERT_FILE'] elsif File.exist?("#{CREW_PREFIX}/etc/ssl/certs/ca-certificates.crt") @@ -177,7 +177,7 @@ SSL_CERT_FILE ||= \ '/etc/ssl/certs/ca-certificates.crt' end -SSL_CERT_DIR ||= \ +SSL_CERT_DIR ||= if ENV['SSL_CERT_DIR'] && Dir.exist?(ENV['SSL_CERT_DIR']) ENV['SSL_CERT_DIR'] elsif Dir.exist?("#{CREW_PREFIX}/etc/ssl/certs") @@ -211,7 +211,7 @@ CREW_COMMON_FNO_LTO_FLAGS ||= "#{CREW_CORE_FLAGS} -fno-lto" CREW_LDFLAGS ||= "-flto=auto #{CREW_LINKER_FLAGS}" CREW_FNO_LTO_LDFLAGS ||= '-fno-lto' -CREW_ENV_OPTIONS_HASH ||= \ +CREW_ENV_OPTIONS_HASH ||= if CREW_DISABLE_ENV_OPTIONS { 'CREW_DISABLE_ENV_OPTIONS' => '1' } else diff --git a/lib/convenience_functions.rb b/lib/convenience_functions.rb index 36c563f34..fb889a339 100644 --- a/lib/convenience_functions.rb +++ b/lib/convenience_functions.rb @@ -115,7 +115,7 @@ class ConvenienceFunctions def self.unset_default_browser(browser_name, browser_binary) Dir.chdir("#{CREW_PREFIX}/bin") do - if File.exist?('x-www-browser') && File.symlink?('x-www-browser') && \ + if File.exist?('x-www-browser') && File.symlink?('x-www-browser') && File.realpath('x-www-browser') == "#{CREW_PREFIX}/share/#{browser_name.downcase}/#{browser_binary}" FileUtils.rm "#{CREW_PREFIX}/bin/x-www-browser" end diff --git a/lib/deb_utils.rb b/lib/deb_utils.rb index cd3ef20fa..32fa321d7 100644 --- a/lib/deb_utils.rb +++ b/lib/deb_utils.rb @@ -29,7 +29,7 @@ module DebUtils end # read file meta - name, _modtime, _uid, _gid, mode, size, end_char = \ + name, _modtime, _uid, _gid, mode, size, end_char = line.scan(/(.{16})(.{12})(.{6})(.{6})(.{8})(.{10})(.{1})/).flatten.map(&:strip) # remove slash suffix from filename (if any) diff --git a/lib/downloader.rb b/lib/downloader.rb index 6cbfb22ab..80e601836 100644 --- a/lib/downloader.rb +++ b/lib/downloader.rb @@ -24,7 +24,7 @@ rescue RuntimeError => e end end -def downloader(url, sha256sum, filename = File.basename(url), verbose = false) +def downloader(url, sha256sum, filename = File.basename(url), verbose: false) # downloader: wrapper for all Chromebrew downloaders (`net/http`,`curl`...) # Usage: downloader , , , # @@ -39,13 +39,13 @@ def downloader(url, sha256sum, filename = File.basename(url), verbose = false) if CREW_USE_CURL || !ENV['CREW_DOWNLOADER'].to_s.empty? # force using external downloader if either CREW_USE_CURL or ENV['CREW_DOWNLOADER'] is set puts "external_downloader(#{uri}, #{filename}, #{verbose})" if verbose - external_downloader(uri, filename, verbose) + external_downloader(uri, filename, verbose: verbose) else case uri.scheme when 'http', 'https' # use net/http if the url protocol is http(s):// puts "http_downloader(#{uri}, #{filename}, #{verbose})" if verbose - http_downloader(uri, filename, verbose) + http_downloader(uri, filename, verbose: verbose) when 'file' # use FileUtils to copy if it is a local file (the url protocol is file://) if File.exist?(uri.path) @@ -56,7 +56,7 @@ def downloader(url, sha256sum, filename = File.basename(url), verbose = false) else # use external downloader (curl by default) if the url protocol is not http(s):// or file:// puts "external_downloader(#{uri}, #{filename}, #{verbose})" if verbose - external_downloader(uri, filename, verbose) + external_downloader(uri, filename, verbose: verbose) end end @@ -88,10 +88,10 @@ rescue StandardError => e # fallback to curl if error occurred puts "external_downloader(#{uri}, #{filename}, #{verbose})" if verbose - external_downloader(uri, filename, verbose) + external_downloader(uri, filename, verbose: verbose) end -def http_downloader(uri, filename = File.basename(url), verbose = false) +def http_downloader(uri, filename = File.basename(url), verbose: false) # http_downloader: Downloader based on net/http library ssl_error_retry = 0 @@ -119,7 +119,7 @@ def http_downloader(uri, filename = File.basename(url), verbose = false) redirect_uri.scheme ||= uri.scheme redirect_uri.host ||= uri.host - return send(__method__, redirect_uri, filename, verbose) + return send(__method__, redirect_uri, filename, verbose: verbose) else abort "Download of #{uri} failed with error #{response.code}: #{response.msg}".lightred end @@ -167,7 +167,7 @@ rescue OpenSSL::SSL::SSLError ssl_error_retry <= 3 ? retry : raise end -def external_downloader(uri, filename = File.basename(url), verbose = false) +def external_downloader(uri, filename = File.basename(url), verbose: false) # external_downloader: wrapper for external downloaders in CREW_DOWNLOADER (curl by default) # default curl cmdline, CREW_DOWNLOADER should be in this format also diff --git a/lib/package.rb b/lib/package.rb index 0b3a85312..52e7b15ee 100644 --- a/lib/package.rb +++ b/lib/package.rb @@ -143,9 +143,9 @@ class Package deps = pkg_obj.dependencies # Append buildessential to deps if building from source is needed/specified. - if ((include_build_deps == true) || ((include_build_deps == 'auto') && is_source)) && \ - !pkg_obj.no_compile_needed? && \ - !exclude_buildessential && \ + if ((include_build_deps == true) || ((include_build_deps == 'auto') && is_source)) && + !pkg_obj.no_compile_needed? && + !exclude_buildessential && !@checked_list.keys.include?('buildessential') deps = { 'buildessential' => [[:build]] }.merge(deps) @@ -155,8 +155,8 @@ class Package expanded_deps = deps.uniq.map do |dep, (dep_tags, ver_check)| # Check build dependencies only if building from source is needed/specified. # Do not recursively find :build based build dependencies. - next unless (include_build_deps == true && @crew_current_package == pkg_obj.name) || \ - ((include_build_deps == 'auto') && is_source && @crew_current_package == pkg_obj.name) || \ + next unless (include_build_deps == true && @crew_current_package == pkg_obj.name) || + ((include_build_deps == 'auto') && is_source && @crew_current_package == pkg_obj.name) || !dep_tags.include?(:build) # Overwrite tags if parent dependency is a build dependency. diff --git a/lib/selector.rb b/lib/selector.rb index f524af299..e409e13cd 100644 --- a/lib/selector.rb +++ b/lib/selector.rb @@ -94,6 +94,7 @@ class Selector # discard any input in the input buffer $stdin.read_nonblock(1024) rescue IO::WaitReadable + # We wait here for reading as per https://docs.ruby-lang.org/en/master/IO.html#method-c-select ensure Thread.current[:input] = $stdin.getc end diff --git a/packages/firefox.rb b/packages/firefox.rb index aa947debd..44229d99a 100644 --- a/packages/firefox.rb +++ b/packages/firefox.rb @@ -103,7 +103,7 @@ class Firefox < Package def self.preremove Dir.chdir("#{CREW_PREFIX}/bin") do - if File.exist?('x-www-browser') && File.symlink?('x-www-browser') && \ + if File.exist?('x-www-browser') && File.symlink?('x-www-browser') && File.realpath('x-www-browser') == "#{CREW_PREFIX}/bin/firefox" FileUtils.rm "#{CREW_PREFIX}/bin/x-www-browser" end diff --git a/packages/foremost.rb b/packages/foremost.rb index ae13dd645..3e58eae46 100644 --- a/packages/foremost.rb +++ b/packages/foremost.rb @@ -22,8 +22,8 @@ class Foremost < Package end def self.install - FileUtils.mkdir_p ["#{CREW_DEST_PREFIX}/bin", \ - "#{CREW_DEST_PREFIX}/man/man1", \ + FileUtils.mkdir_p ["#{CREW_DEST_PREFIX}/bin", + "#{CREW_DEST_PREFIX}/man/man1", "#{CREW_DEST_PREFIX}/etc"] FileUtils.cp_r 'foremost', "#{CREW_DEST_PREFIX}/bin/" FileUtils.cp_r 'foremost.8.gz', "#{CREW_DEST_PREFIX}/man/man1/foremost.1.gz" diff --git a/packages/nss.rb b/packages/nss.rb index 5221b3a83..d1074fb71 100644 --- a/packages/nss.rb +++ b/packages/nss.rb @@ -26,12 +26,12 @@ class Nss < Package depends_on 'zlib' # R def self.build - @build_64 = ARCH == 'x86_64' ? '1' : '0' + @build64 = ARCH == 'x86_64' ? '1' : '0' @arch_cflags = '' @arch_ldflags = @arch_cflags Dir.chdir 'nss' do - system "env opt_build=1 build_64=#{@build_64} \ + system "env opt_build=1 build_64=#{@build64} \ NSS_ENABLE_WERROR=0 NS_USE_GCC=1 USEABSPATH=NO \ NSS_GYP_PREFIX=#{CREW_PREFIX} CFLAGS='-pipe #{@arch_cflags}' \ CXXFLAGS='-pipe #{@arch_cflags}' LDFLAGS='#{@arch_ldflags}' \ diff --git a/packages/qemacs.rb b/packages/qemacs.rb index dc342a62e..374f1a983 100644 --- a/packages/qemacs.rb +++ b/packages/qemacs.rb @@ -20,12 +20,12 @@ class Qemacs < Package def self.build system "sed -i 's,css.h,libqhtml/css.h,' html2png.c" system "sed -i 's/$(prefix)/$(DESTDIR)$(prefix)/g' Makefile" - system './configure', \ - "--prefix=#{CREW_PREFIX}", \ - '--disable-x11', \ - '--disable-xv', \ - '--disable-xrender', \ - '--disable-html', \ + system './configure', + "--prefix=#{CREW_PREFIX}", + '--disable-x11', + '--disable-xv', + '--disable-xrender', + '--disable-html', '--disable-png' system 'make' end diff --git a/packages/vifm.rb b/packages/vifm.rb index 116a26b69..fc2b1cf45 100644 --- a/packages/vifm.rb +++ b/packages/vifm.rb @@ -18,10 +18,10 @@ class Vifm < Package }) def self.build - system './configure', \ - "--prefix=#{CREW_PREFIX}", \ - '--without-gtk', \ - '--without-X11', \ + system './configure', + "--prefix=#{CREW_PREFIX}", + '--without-gtk', + '--without-X11', # "--with-curses=#{CREW_PREFIX}/include/ncursesw" \ # "--with-curses-name=ncursesw" \ "CPPFLAGS=-I#{CREW_PREFIX}/include/ncursesw" diff --git a/tools/getrealdeps.rb b/tools/getrealdeps.rb index da7662c69..432f6f4dd 100755 --- a/tools/getrealdeps.rb +++ b/tools/getrealdeps.rb @@ -99,9 +99,7 @@ def main(pkg) end.flatten.compact.uniq # Figure out which Chromebrew packages provide the relevant deps. - pkgdeps = pkgdepsfiles.map do |file| - whatprovidesfxn(file, pkg) - end.sort.reject { |i| i.include?(pkg) }.map { |i| i.split("\n") }.flatten.uniq + pkgdeps = pkgdepsfiles.map { |file| whatprovidesfxn(file, pkg) }.sort.reject { |i| i.include?(pkg) }.map { |i| i.split("\n") }.flatten.uniq # Massage the glibc entries in the dependency list. pkgdeps = pkgdeps.map { |i| i.gsub(/glibc_build.*/, 'glibc') }.uniq