Skip to content

bundle/brew_services: avoid shelling out to brew#21833

Draft
carlocab wants to merge 1 commit intomainfrom
brew_services-no-shell-out
Draft

bundle/brew_services: avoid shelling out to brew#21833
carlocab wants to merge 1 commit intomainfrom
brew_services-no-shell-out

Conversation

@carlocab
Copy link
Copy Markdown
Member

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests (excluding integration tests) for your changes? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) with your changes locally?

  • AI was used to generate or assist with generating this PR. Please specify below how you used AI to help you, and what steps you have taken to manually verify the changes.

brew services and brew bundle now live in this repo, so we can try
to avoid shelling out to brew when we don't need to.

There are still multiple other places this is happening, but I'll leave
those for future work (which I'll try to get to soon).

`brew services` and `brew bundle` now live in this repo, so we can try
to avoid shelling out to `brew` when we don't need to.

There are still multiple other places this is happening, but I'll leave
those for future work (which I'll try to get to soon).
Copilot AI review requested due to automatic review settings March 25, 2026 15:07
@carlocab carlocab self-assigned this Mar 25, 2026
Copy link
Copy Markdown
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, smaller diff than I expected!

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates brew bundle’s services integration to avoid spawning a separate brew services list --json process by querying services directly via the in-repo services implementation.

Changes:

  • Replace Utils.safe_popen_read(HOMEBREW_BREW_FILE, "services", "list", "--json") + JSON parsing with Homebrew::Services::Formulae.services_list.
  • Update bundle-related specs to stub Homebrew::Services::Formulae.services_list instead of stubbing Utils.safe_popen_read.
  • Add explicit require "services/formulae" in the affected specs (and where needed for stubbing).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
Library/Homebrew/bundle/brew_services.rb Switch started-services detection from shelling out to using Homebrew::Services::Formulae.services_list.
Library/Homebrew/test/bundle/brew_spec.rb Update dump spec to stub services list via Homebrew::Services::Formulae.
Library/Homebrew/test/bundle/brew_services_spec.rb Update started-services specs to stub Homebrew::Services::Formulae.services_list with symbol-keyed hashes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +68 to +70
require "services/formulae"
Homebrew::Services::Formulae.services_list.filter_map do |hash|
hash.fetch(:name) if states_to_skip.exclude?(hash.fetch(:status))
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Linux/systemd, brew services sets DBUS_SESSION_BUS_ADDRESS and XDG_RUNTIME_DIR from HOMEBREW_DBUS_SESSION_BUS_ADDRESS/HOMEBREW_XDG_RUNTIME_DIR before querying service state (see cmd/services.rb). By calling Homebrew::Services::Formulae.services_list directly here, systemctl --user calls may run without those env vars and incorrectly report services as not loaded/running. Consider mirroring the env setup (ideally scoped via with_env to avoid permanently mutating ENV) before invoking services_list, and add a spec to cover the systemd env behavior.

Suggested change
require "services/formulae"
Homebrew::Services::Formulae.services_list.filter_map do |hash|
hash.fetch(:name) if states_to_skip.exclude?(hash.fetch(:status))
old_dbus = ENV["DBUS_SESSION_BUS_ADDRESS"]
old_xdg = ENV["XDG_RUNTIME_DIR"]
new_dbus = ENV["HOMEBREW_DBUS_SESSION_BUS_ADDRESS"]
new_xdg = ENV["HOMEBREW_XDG_RUNTIME_DIR"]
ENV["DBUS_SESSION_BUS_ADDRESS"] = new_dbus if new_dbus
ENV["XDG_RUNTIME_DIR"] = new_xdg if new_xdg
begin
require "services/formulae"
Homebrew::Services::Formulae.services_list.filter_map do |hash|
hash.fetch(:name) if states_to_skip.exclude?(hash.fetch(:status))
end
ensure
if old_dbus.nil?
ENV.delete("DBUS_SESSION_BUS_ADDRESS")
else
ENV["DBUS_SESSION_BUS_ADDRESS"] = old_dbus
end
if old_xdg.nil?
ENV.delete("XDG_RUNTIME_DIR")
else
ENV["XDG_RUNTIME_DIR"] = old_xdg
end

Copilot uses AI. Check for mistakes.
@carlocab
Copy link
Copy Markdown
Member Author

There's some permissions handling I need to think about carefully here. Marking as draft for now.

@carlocab carlocab marked this pull request as draft March 25, 2026 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants