Skip to content

Enforce explicit formula install-step bases (28/29) #23404

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
MikeMcQuaid wants to merge 1 commit into
base: main
Choose a base branch
from
+183 −34
Draft

Enforce explicit formula install-step bases (28/29) #23404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 95 additions & 4 deletions Library/Homebrew/rubocops/install_steps.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@ class InstallSteps < FormulaCop
# cannot coexist after the incremental conversion bridge is removed.
# CONFLICT_MSG = "`post_install` and `post_install_steps` cannot both be used."
REDUNDANT_SERVICE_PATH_DIRS_MSG = "`%<block>s` only creates directories created by `brew services`."
EXPLICIT_BASE_MSG = "Formula install-step paths must specify their base explicitly."
EXPLICIT_BASE_STEP_METHODS = [:if_path_exists, :unless_path_exists, :mkdir, :mkdir_p, :touch, :remove,
:inreplace, :write, :write_file, :init_data_dir, :set_permissions].freeze
RUN_PATH_KEYWORDS = [:stdin_path, :stdout_path, :chdir].freeze
ABSOLUTE_PATH_TEMPLATE_TOKENS = %w[
HOMEBREW_PREFIX HOMEBREW_CELLAR prefix opt_prefix bin sbin lib libexec share pkgshare var etc pkgetc rack
staged_path appdir caskroom_path temp bash_completion zsh_completion fish_completion pwsh_completion
].freeze
CERTIFICATE_REMOVE_SOURCE = 'rm(pkgetc/"cert.pem") if (pkgetc/"cert.pem").exist?'
CERTIFICATE_INSTALL_SYMLINK_SOURCE =
'pkgetc.install_symlink Formula["ca-certificates"].pkgetc/"cert.pem"'
Expand Down Expand Up @@ -89,7 +97,11 @@ def audit_formula(formula_nodes)
# problem CONFLICT_MSG
# end

audit_step_block(post_install_steps_block)
redundant_post_install_steps = post_install_steps_block.present? &&
redundant_service_path_dirs_block?(post_install_steps_block,
service_path_dirs,
:post_install_steps)
audit_step_block(post_install_steps_block) unless redundant_post_install_steps
add_redundant_service_path_dirs_offense(post_install_steps_block, service_path_dirs, :post_install_steps)
redundant_post_install = post_install_method.present? &&
redundant_service_path_dirs_block?(post_install_method, service_path_dirs,
Expand All @@ -105,10 +117,89 @@ def audit_formula(formula_nodes)

sig { params(block_node: T.nilable(RuboCop::AST::BlockNode)).void }
def audit_step_block(block_node)
return unless (offense_node = install_step_block_offense_node(block_node))
return if block_node.nil?

offending_node(offense_node)
problem STEP_BLOCK_MSG
if (offense_node = install_step_block_offense_node(block_node))
offending_node(offense_node)
problem STEP_BLOCK_MSG
return
end

add_implicit_var_path_offenses(block_node)
end

sig { params(block_node: RuboCop::AST::BlockNode).void }
def add_implicit_var_path_offenses(block_node)
block_node.each_descendant(:send) do |node|
send_node = T.cast(node, RuboCop::AST::SendNode)
next if send_node.receiver

if EXPLICIT_BASE_STEP_METHODS.include?(send_node.method_name)
add_implicit_var_base_offense(send_node)
elsif send_node.method_name == :run
add_implicit_var_run_path_offenses(send_node)
end
end
end

sig { params(send_node: RuboCop::AST::SendNode).void }
def add_implicit_var_base_offense(send_node)
path_node = send_node.arguments.first
return if path_node.nil? || explicit_formula_step_path?(path_node)

options = send_node.arguments.last
options = nil unless options&.hash_type?
return if options && T.cast(options, RuboCop::AST::HashNode).pairs.any? do |pair|
pair.key.sym_type? && pair.key.value == :base
end

add_offense(send_node, message: EXPLICIT_BASE_MSG) do |corrector|
if options
corrector.insert_after(T.cast(options, RuboCop::AST::HashNode).pairs.last.source_range, ", base: :var")
else
argument = send_node.arguments.last
next if argument.nil?

range = if argument.loc.respond_to?(:heredoc_end) && argument.loc.heredoc_end
argument.loc.expression
else
argument.source_range
end
corrector.insert_after(range, ", base: :var")
end
end
end

sig { params(send_node: RuboCop::AST::SendNode).void }
def add_implicit_var_run_path_offenses(send_node)
options = send_node.arguments.last
return unless options&.hash_type?

T.cast(options, RuboCop::AST::HashNode).pairs.each do |pair|
next if !pair.key.sym_type? || !RUN_PATH_KEYWORDS.include?(pair.key.value)
next if explicit_formula_step_path?(pair.value)

add_offense(pair.value, message: EXPLICIT_BASE_MSG) do |corrector|
next unless pair.value.str_type?

path = T.cast(pair.value, RuboCop::AST::StrNode).str_content
corrector.replace(pair.value, "{{var}}/#{path}".dump)
end
end
end

sig { params(node: RuboCop::AST::Node).returns(T::Boolean) }
def explicit_formula_step_path?(node)
if node.array_type?
paths = node.child_nodes
return paths.present? && paths.all? { |path| explicit_formula_step_path?(path) }
end
return false unless node.str_type?

path = T.cast(node, RuboCop::AST::StrNode).str_content
return true if path.start_with?("/", "~")

ABSOLUTE_PATH_TEMPLATE_TOKENS.any? { |token| path.start_with?("{{#{token}}}") }
end

sig {
Expand Down
118 changes: 88 additions & 30 deletions Library/Homebrew/test/rubocops/install_steps_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ class Foo < Formula
url "https://brew.sh/foo-1.0.tgz"

post_install_steps do
touch "foo/state"
touch "foo/state", base: :var
end

def post_install; end
Expand All @@ -28,7 +28,64 @@ class Foo < Formula
def post_install; end

post_install_steps do
touch "foo/state"
touch "foo/state", base: :var
end
end
RUBY
end

it "autocorrects implicit formula var paths" do
expect_offense(<<~RUBY)
class Foo < Formula
url "https://brew.sh/foo-1.0.tgz"

post_install_steps do
mkdir_p "log/foo"
^^^^^^^^^^^^^^^^^ FormulaAudit/InstallSteps: Formula install-step paths must specify their base explicitly.
write "foo/state", "ready"
^^^^^^^^^^^^^^^^^^^^^^^^^^ FormulaAudit/InstallSteps: Formula install-step paths must specify their base explicitly.
init_data_dir "foo", using: :postgresql_initdb
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FormulaAudit/InstallSteps: Formula install-step paths must specify their base explicitly.
if_path_exists "foo/state" do
^^^^^^^^^^^^^^^^^^^^^^^^^^ FormulaAudit/InstallSteps: Formula install-step paths must specify their base explicitly.
touch "foo/checked"
^^^^^^^^^^^^^^^^^^^ FormulaAudit/InstallSteps: Formula install-step paths must specify their base explicitly.
end
run "foo", base: :bin, stdin_path: "foo/input"
^^^^^^^^^^^ FormulaAudit/InstallSteps: Formula install-step paths must specify their base explicitly.
end
end
RUBY

expect_correction(<<~RUBY)
class Foo < Formula
url "https://brew.sh/foo-1.0.tgz"

post_install_steps do
mkdir_p "log/foo", base: :var
write "foo/state", "ready", base: :var
init_data_dir "foo", using: :postgresql_initdb, base: :var
if_path_exists "foo/state", base: :var do
touch "foo/checked", base: :var
end
run "foo", base: :bin, stdin_path: "{{var}}/foo/input"
end
end
RUBY
end

it "accepts formula paths with explicit bases or absolute tokens" do
expect_no_offenses(<<~RUBY)
class Foo < Formula
url "https://brew.sh/foo-1.0.tgz"

post_install_steps do
mkdir_p "log/foo", base: :var
touch "{{var}}/foo/state"
if_path_exists "/etc/foo.conf" do
write "foo.conf", "ready", base: :etc
end
run "foo", base: :bin, chdir: "{{libexec}}/foo"
end
end
RUBY
Expand All @@ -53,20 +110,21 @@ class Foo < Formula
url "https://brew.sh/foo-1.0.tgz"

post_install_steps do
mkdir_p "foo"
touch "foo/state"
touch "foo/#{formula_name}"
mkdir_p "foo", base: :var
touch "foo/state", base: :var
touch "foo/#{formula_name}", base: :var
move "source", "target"
move_contents "source", "target"
inreplace "foo.conf", %r{{{HOMEBREW_CELLAR}}/foo/[^/]+}, "{{opt_prefix}}", audit_result: false
inreplace "foo.conf", %r{{{HOMEBREW_CELLAR}}/foo/[^/]+}, "{{opt_prefix}}", base: :var,
audit_result: false
symlink "source", "target", source_base: :relative, overwrite: true, remove_on_uninstall: true
write_file "foo.conf", "key = value\n", base: :etc
write_file "foo/adjacent", "first" "second"
set_permissions "foo", "0755"
write_file "foo/adjacent", "first" "second", base: :var
set_permissions "foo", "0755", base: :var
run "foo", args: ["--repair"]
terminate_process "foo", attempts: 3
change_dylib_id "lib/libfoo.dylib", "{{opt_prefix}}/lib/libfoo.1.dylib", resolve_source: true
if_path_exists "foo" do
if_path_exists "foo", base: :var do
warn "foo exists"
end
configure_gcc_runtime
Expand All @@ -76,10 +134,10 @@ class Foo < Formula
configure_php
bootstrap_cpython
bootstrap_pypy abi_version: "3.10"
write_file "foo/banner", <<~TEXT
write_file "foo/banner", <<~TEXT, base: :var
literal banner
TEXT
init_data_dir formula_name, using: :postgresql
init_data_dir formula_name, using: :postgresql, base: :var
symlink_tree "source", "#{formula_name}"
symlink_children "source", suffix: "-#{version.major}"
compile_gsettings_schemas
Expand All @@ -90,8 +148,8 @@ class Foo < Formula
update_mime_database
update_desktop_database
on_macos do
if_path_exists "foo" do
touch "foo/scoped-state"
if_path_exists "foo", base: :var do
touch "foo/scoped-state", base: :var
end
end
on_linux do
Expand Down Expand Up @@ -125,7 +183,7 @@ class Foo < Formula
url "https://brew.sh/foo-1.0.tgz"

post_install_steps do
write_file "foo.conf", "prefix = #{prefix}"
write_file "foo.conf", "prefix = #{prefix}", base: :var
^^^^^^^^^ FormulaAudit/InstallSteps: Steps blocks may only contain install step DSL calls. Prefer canonical calls: `mkdir_p`, `touch`, `move`, `move_contents`, `copy`, `remove`, `inreplace`, `symlink`, `symlink_tree`, `symlink_children`, `write_file`, `init_data_dir`, `compile_gsettings_schemas`, `update_gdk_pixbuf_loaders_cache`, `update_gtk_icon_cache`, `update_mime_database`, `update_desktop_database`, `set_permissions`, `run`, `terminate_process`, `warn`, `change_dylib_id`, `configure_gcc_runtime`, `install_gzipped_executable`, `configure_glibc_runtime`, `configure_clang_system`, `configure_php`, `bootstrap_cpython`, `bootstrap_pypy`, `if_path_exists`, `unless_path_exists`, `on_macos`, `on_linux`.
end
end
Expand All @@ -152,8 +210,8 @@ class Foo < Formula
url "https://brew.sh/foo-1.0.tgz"

post_install_steps do
mkdir_p "log/foo"
touch "foo/state"
mkdir_p "log/foo", base: :var
touch "foo/state", base: :var
move "move-source", "move-target"
symlink "move-target", "linked-target", source_base: :relative, overwrite: true
end
Expand Down Expand Up @@ -182,7 +240,7 @@ class Foo < Formula

post_install_steps do
write_file "foo/foo.conf", "key = value\n", base: :etc
write_file "foo/banner", <<~TEXT
write_file "foo/banner", <<~TEXT, base: :var
literal banner
TEXT
end
Expand All @@ -207,7 +265,7 @@ class Foo < Formula
url "https://brew.sh/foo-1.0.tgz"

post_install_steps do
write_file "foo.conf", "key = value"
write_file "foo.conf", "key = value", base: :var
end
end
RUBY
Expand Down Expand Up @@ -250,7 +308,7 @@ class Foo < Formula
url "https://brew.sh/foo-1.0.tgz"

post_install_steps do
touch "postgresql/state"
touch "postgresql/state", base: :var
end

def post_install
Expand Down Expand Up @@ -297,13 +355,13 @@ class Foo < Formula
url "https://brew.sh/foo-1.0.tgz"

post_install_steps do
touch "postgresql/state"
mkdir_p "log"
touch "postgresql/state", base: :var
mkdir_p "log", base: :var
symlink_tree "include/postgresql", "include/{{formula_name}}"
symlink_tree "lib/postgresql", "lib/{{formula_name}}"
symlink_tree "share/postgresql", "share/{{formula_name}}"
symlink_children "bin", suffix: "-{{version.major}}"
init_data_dir formula_name, using: :postgresql
init_data_dir formula_name, using: :postgresql, base: :var
end

def post_install
Expand Down Expand Up @@ -350,7 +408,7 @@ class Mysql < Formula
url "https://brew.sh/foo-1.0.tgz"

post_install_steps do
init_data_dir "mysql", using: :mysql
init_data_dir "mysql", using: :mysql, base: :var
end

def post_install
Expand Down Expand Up @@ -390,7 +448,7 @@ class Foo < Formula
url "https://brew.sh/foo-1.0.tgz"

post_install_steps do
init_data_dir "mysql", using: :mariadb
init_data_dir "mysql", using: :mariadb, base: :var
end
end
RUBY
Expand Down Expand Up @@ -460,7 +518,7 @@ class Foo < Formula
post_install_steps do
symlink_tree "include/postgresql", "include/{{formula_name}}"
symlink_children "bin", suffix: "-{{version.major}}"
init_data_dir formula_name, using: :postgresql
init_data_dir formula_name, using: :postgresql, base: :var
symlink "cert.pem", "cert.pem",
source_formula: "ca-certificates",
source_base: :formula_pkgetc,
Expand Down Expand Up @@ -594,8 +652,8 @@ class Foo < Formula

post_install_steps do
^^^^^^^^^^^^^^^^^^^^^ FormulaAudit/InstallSteps: `post_install_steps` only creates directories created by `brew services`.
mkdir_p "run/foo"
mkdir_p "log/foo"
mkdir_p "run/foo", base: :var
mkdir_p "log/foo", base: :var
end

service do
Expand Down Expand Up @@ -627,8 +685,8 @@ class Foo < Formula
url "https://brew.sh/foo-1.0.tgz"

post_install_steps do
mkdir_p "run/foo"
mkdir_p "state/foo"
mkdir_p "run/foo", base: :var
mkdir_p "state/foo", base: :var
end

service do
Expand All @@ -645,7 +703,7 @@ class Foo < Formula
url "https://brew.sh/foo-1.0.tgz"

post_install_steps do
mkdir_p "run"
mkdir_p "run", base: :var
end

service do
Expand Down
Loading