From f2adbf66137b0906f831b141db159c1722d09f62 Mon Sep 17 00:00:00 2001 From: apainintheneck Date: Sat, 13 May 2023 12:35:50 -0700 Subject: [PATCH] service: change custom name DSL After some discussion, we decided to change the DSL to get rid of the `plist_name` and `service_name` methods which aren't meaningful for most users. The new DSL looks like this: ```rb service do name macos: "name", linux: "name" end ``` I also updated some specs here to reflect these changes. There was some talk about maybe deprecating `plist_name` and `service_name` but I think that's outside of the scope of this PR so I'm leaving them as is for now. One benefit of this is that everything here is backwards compatible. --- Library/Homebrew/formulary.rb | 19 +++++-- Library/Homebrew/rubocops/service.rb | 12 ++-- Library/Homebrew/service.rb | 57 +++++++------------ Library/Homebrew/test/caveats_spec.rb | 3 +- Library/Homebrew/test/formula_spec.rb | 5 +- Library/Homebrew/test/formulary_spec.rb | 3 + .../Homebrew/test/rubocops/service_spec.rb | 10 ++-- Library/Homebrew/test/service_spec.rb | 8 +++ docs/Formula-Cookbook.md | 14 ++--- 9 files changed, 63 insertions(+), 68 deletions(-) diff --git a/Library/Homebrew/formulary.rb b/Library/Homebrew/formulary.rb index f077b6a41a..71df50fb29 100644 --- a/Library/Homebrew/formulary.rb +++ b/Library/Homebrew/formulary.rb @@ -258,15 +258,22 @@ module Formulary if (service_hash = json_formula["service"]) service_hash = Homebrew::Service.deserialize(service_hash) - run_params = service_hash.delete(:run) service do T.bind(self, Homebrew::Service) - case run_params - when Hash - run(**run_params) - when Array, String - run run_params + + if (run_params = service_hash.delete(:run)) + case run_params + when Hash + run(**run_params) + when Array, String + run run_params + end end + + if (name_params = service_hash.delete(:name)) + name(**name_params) + end + service_hash.each do |key, arg| public_send(key, arg) end diff --git a/Library/Homebrew/rubocops/service.rb b/Library/Homebrew/rubocops/service.rb index c439f8a276..29065b26aa 100644 --- a/Library/Homebrew/rubocops/service.rb +++ b/Library/Homebrew/rubocops/service.rb @@ -21,11 +21,8 @@ module RuboCop share: :opt_share, }.freeze - # These can be defined without using the `run` command. - RENAME_METHOD_CALLS = [:plist_name, :service_name].freeze - # At least one of these methods must be defined in a service block. - REQUIRED_METHOD_CALLS = [:run, *RENAME_METHOD_CALLS].freeze + REQUIRED_METHOD_CALLS = [:run, :name].freeze def audit_formula(_node, _class_node, _parent_class_node, body_node) service_node = find_block(body_node, :service) @@ -38,13 +35,12 @@ module RuboCop # so we don't show both of them at the same time. if (method_calls.keys & REQUIRED_METHOD_CALLS).empty? offending_node(service_node) - problem "Service blocks require `run`, `plist_name` or `service_name` to be defined." + problem "Service blocks require `run` or `name` to be defined." elsif !method_calls.key?(:run) - other_method_calls = method_calls.keys - RENAME_METHOD_CALLS + other_method_calls = method_calls.keys - [:name] if other_method_calls.any? offending_node(service_node) - problem "`run` must be defined to use methods other than " \ - "`service_name` and `plist_name` like #{other_method_calls}." + problem "`run` must be defined to use methods other than `name` like #{other_method_calls}." end end diff --git a/Library/Homebrew/service.rb b/Library/Homebrew/service.rb index 81b2c2c0e0..c53caacb2b 100644 --- a/Library/Homebrew/service.rb +++ b/Library/Homebrew/service.rb @@ -41,16 +41,9 @@ module Homebrew "homebrew.mxcl.#{@formula.name}" end - sig { params(name: T.nilable(String)).returns(String) } - def plist_name(name = nil) - case T.unsafe(name) - when nil - @plist_name ||= default_plist_name - when String - @plist_name = name - else - raise TypeError, "Service#plist_name expects a String" - end + sig { returns(String) } + def plist_name + @plist_name ||= default_plist_name end sig { returns(String) } @@ -58,16 +51,17 @@ module Homebrew "homebrew.#{@formula.name}" end - sig { params(name: T.nilable(String)).returns(String) } - def service_name(name = nil) - case T.unsafe(name) - when nil - @service_name ||= default_service_name - when String - @service_name = name - else - raise TypeError, "Service#service_name expects a String" - end + sig { returns(String) } + def service_name + @service_name ||= default_service_name + end + + sig { params(macos: T.nilable(String), linux: T.nilable(String)).void } + def name(macos: nil, linux: nil) + raise TypeError, "Service#name expects at least one String" if [macos, linux].none?(String) + + @plist_name = macos if macos + @service_name = linux if linux end sig { @@ -539,14 +533,13 @@ module Homebrew # Prepare the service hash for inclusion in the formula API JSON. sig { returns(Hash) } def serialize - custom_plist_name = plist_name if plist_name != default_plist_name - custom_service_name = service_name if service_name != default_service_name + name_params = { + macos: (plist_name if plist_name != default_plist_name), + linux: (service_name if service_name != default_service_name), + }.compact unless command? - return { - plist_name: custom_plist_name, - service_name: custom_service_name, - }.compact + return name_params.blank? ? {} : { name: name_params } end cron_string = if @cron.present? @@ -558,8 +551,7 @@ module Homebrew sockets_string = "#{@sockets[:type]}://#{@sockets[:host]}:#{@sockets[:port]}" if @sockets.present? { - plist_name: custom_plist_name, - service_name: custom_service_name, + name: name_params.presence, run: @run_params, run_type: @run_type, interval: @interval, @@ -584,15 +576,10 @@ module Homebrew sig { params(api_hash: Hash).returns(Hash) } def self.deserialize(api_hash) hash = {} - - %w[plist_name service_name].each do |key| - next if (value = api_hash[key]).nil? - - hash[key.to_sym] = value - end + hash[:name] = api_hash["name"].transform_keys(&:to_sym) if api_hash.key?("name") # The service hash might not have a "run" command if it only documents - # an existing service file with "plist_name" or "service_name". + # an existing service file with the "name" command. return hash unless api_hash.key?("run") hash[:run] = diff --git a/Library/Homebrew/test/caveats_spec.rb b/Library/Homebrew/test/caveats_spec.rb index e1ee5a3e8c..4a0681b3e3 100644 --- a/Library/Homebrew/test/caveats_spec.rb +++ b/Library/Homebrew/test/caveats_spec.rb @@ -211,8 +211,7 @@ describe Caveats do f = formula do url "foo-1.0" service do - plist_name "custom.mxcl.foo" - service_name "custom.foo" + name macos: "custom.mxcl.foo", linux: "custom.foo" end end expect(Utils::Service).to receive(:installed?).with(f).once.and_return(true) diff --git a/Library/Homebrew/test/formula_spec.rb b/Library/Homebrew/test/formula_spec.rb index 817f73ca8f..d67addd563 100644 --- a/Library/Homebrew/test/formula_spec.rb +++ b/Library/Homebrew/test/formula_spec.rb @@ -736,14 +736,13 @@ describe Formula do f = formula do url "https://brew.sh/test-1.0.tbz" service do - plist_name "custom.macos.beanstalkd" - service_name "custom.linux.beanstalkd" + name macos: "custom.macos.beanstalkd", linux: "custom.linux.beanstalkd" end end expect(f.plist_name).to eq("custom.macos.beanstalkd") expect(f.service_name).to eq("custom.linux.beanstalkd") - expect(f.service.serialize.keys).to contain_exactly(:plist_name, :service_name) + expect(f.service.serialize.keys).to contain_exactly(:name) end specify "service helpers return data" do diff --git a/Library/Homebrew/test/formulary_spec.rb b/Library/Homebrew/test/formulary_spec.rb index 16292bea7a..35865147b6 100644 --- a/Library/Homebrew/test/formulary_spec.rb +++ b/Library/Homebrew/test/formulary_spec.rb @@ -272,6 +272,7 @@ describe Formulary do "link_overwrite" => ["bin/abc"], "caveats" => "example caveat string\n/$HOME\n$HOMEBREW_PREFIX", "service" => { + "name" => { macos: "custom.launchd.name", linux: "custom.systemd.name" }, "run" => ["$HOMEBREW_PREFIX/opt/formula_name/bin/beanstalkd", "test"], "run_type" => "immediate", "working_dir" => "/$HOME", @@ -362,6 +363,8 @@ describe Formulary do expect(formula.service.command).to eq(["#{HOMEBREW_PREFIX}/opt/formula_name/bin/beanstalkd", "test"]) expect(formula.service.run_type).to eq(:immediate) expect(formula.service.working_dir).to eq(Dir.home) + expect(formula.plist_name).to eq("custom.launchd.name") + expect(formula.service_name).to eq("custom.systemd.name") expect do formula.install diff --git a/Library/Homebrew/test/rubocops/service_spec.rb b/Library/Homebrew/test/rubocops/service_spec.rb index 2a723f4c2d..e8d2d49d5c 100644 --- a/Library/Homebrew/test/rubocops/service_spec.rb +++ b/Library/Homebrew/test/rubocops/service_spec.rb @@ -11,7 +11,7 @@ describe RuboCop::Cop::FormulaAudit::Service do url "https://brew.sh/foo-1.0.tgz" service do - ^^^^^^^^^^ FormulaAudit/Service: Service blocks require `run`, `plist_name` or `service_name` to be defined. + ^^^^^^^^^^ FormulaAudit/Service: Service blocks require `run` or `name` to be defined. run_type :cron working_dir "/tmp/example" end @@ -25,8 +25,7 @@ describe RuboCop::Cop::FormulaAudit::Service do url "https://brew.sh/foo-1.0.tgz" service do - plist_name "custom.mcxl.foo" - service_name "custom.foo" + name macos: "custom.mcxl.foo", linux: "custom.foo" end end RUBY @@ -38,9 +37,8 @@ describe RuboCop::Cop::FormulaAudit::Service do url "https://brew.sh/foo-1.0.tgz" service do - ^^^^^^^^^^ FormulaAudit/Service: `run` must be defined to use methods other than `service_name` and `plist_name` like [:working_dir]. - plist_name "custom.mcxl.foo" - service_name "custom.foo" + ^^^^^^^^^^ FormulaAudit/Service: `run` must be defined to use methods other than `name` like [:working_dir]. + name macos: "custom.mcxl.foo", linux: "custom.foo" working_dir "/tmp/example" end end diff --git a/Library/Homebrew/test/service_spec.rb b/Library/Homebrew/test/service_spec.rb index 9878839181..51e5ad76a6 100644 --- a/Library/Homebrew/test/service_spec.rb +++ b/Library/Homebrew/test/service_spec.rb @@ -949,6 +949,10 @@ describe Homebrew::Service do describe ".deserialize" do let(:serialized_hash) do { + "name" => { + "linux" => "custom.systemd.name", + "macos" => "custom.launchd.name", + }, "environment_variables" => { "PATH" => "$HOMEBREW_PREFIX/bin:$HOMEBREW_PREFIX/sbin:/usr/bin:/bin:/usr/sbin:/sbin", }, @@ -961,6 +965,10 @@ describe Homebrew::Service do let(:deserialized_hash) do { + name: { + linux: "custom.systemd.name", + macos: "custom.launchd.name", + }, environment_variables: { PATH: "#{HOMEBREW_PREFIX}/bin:#{HOMEBREW_PREFIX}/sbin:/usr/bin:/bin:/usr/sbin:/sbin", }, diff --git a/docs/Formula-Cookbook.md b/docs/Formula-Cookbook.md index 9cc0ec23a3..edab78ca5a 100644 --- a/docs/Formula-Cookbook.md +++ b/docs/Formula-Cookbook.md @@ -875,14 +875,13 @@ There are two ways to add `launchd` plists and `systemd` services to a formula, ```ruby service do - # Corresponds to the launchd script at "#{plist_name}.plist". - plist_name "custom.plist.name" - - # Corresponds to the systemd script at "#{service_name}.service". - service_name "custom.service.name" + name macos: "custom.launchd.name", + linux: "custom.systemd.name" end ``` + To find the file we append `.plist` to the `launchd` service name and `.service` to the `systemd` service name internally. + 2. If the formula does not provide a service file you can generate one using the following stanza: ```ruby @@ -893,7 +892,7 @@ There are two ways to add `launchd` plists and `systemd` services to a formula, #### Service block methods -This table lists the options you can set within a `service` block. One of the `run`, `plist_name` or `service_name` fields is required inside the service block. The `run` field indicates what command to run and is required before using fields other than `plist_name` and `service_name`. +This table lists the options you can set within a `service` block. The `run` or `name` field must be defined inside the service block. The `run` field indicates what command to run and is required before using fields other than `name`. | method | default | macOS | Linux | description | | ----------------------- | ------------ | :---: | :---: | ----------- | @@ -914,8 +913,7 @@ This table lists the options you can set within a `service` block. One of the `r | `process_type` | - | yes | no-op | type of process to manage: `:background`, `:standard`, `:interactive` or `:adaptive` | `macos_legacy_timers` | - | yes | no-op | timers created by `launchd` jobs are coalesced unless this is set | `sockets` | - | yes | no-op | socket that is created as an accesspoint to the service -| `plist_name` | `"homebrew.mxcl.#{formula.name}"` | yes | no | name of the `launchd` job -| `service_name` | `"homebrew.#{formula.name}"` | no | yes | name of the `systemd` job +| `name` | - | yes | yes | a hash with the `launchd` service name on macOS and/or the `systemd` service name on Linux For services that are kept alive after starting you can use the default `run_type`: