Skip to content

completion: update SHELL for ngrok#22327

Open
branchv wants to merge 1 commit into
Homebrew:mainfrom
branchv:completions-shell
Open

completion: update SHELL for ngrok#22327
branchv wants to merge 1 commit into
Homebrew:mainfrom
branchv:completions-shell

Conversation

@branchv
Copy link
Copy Markdown
Member

@branchv branchv commented May 17, 2026


  • 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? Performance claims (e.g. "this is faster") must include Hyperfine benchmarks.
  • 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. Non-maintainers may only have one AI-assisted/generated PR open at a time.

ngrok cask appears to ignore $SHELL unless it looks like a path

$ SHELL=zsh ngrok completion | head -1
# bash completion for ngrok                                -*- shell-script -*-
$ SHELL=/zsh ngrok completion | head -1
#compdef ngrok

I would send ngrok a PR instead, but it's closed source and is simple enough to support on our side

Copilot AI review requested due to automatic review settings May 17, 2026 19:51
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 adjusts Homebrew’s generated shell completion logic to set SHELL as an absolute path (e.g., /bin/zsh) when invoking completion generators, to accommodate tools (like ngrok) that treat SHELL differently depending on whether it looks like a path.

Changes:

  • Set SHELL to /bin/<shell> when generating completions for both formulae and casks.
  • Update cask generated-completion specs to reflect the new SHELL value format.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
Library/Homebrew/formula.rb Changes the SHELL env passed to completion generators to an absolute /bin/<shell> path.
Library/Homebrew/cask/artifact/generated_completion.rb Applies the same SHELL env change for cask-generated completions.
Library/Homebrew/test/cask/artifact/generated_completion_spec.rb Updates expectations/mocking to match the new SHELL env format.

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

Comment on lines 2459 to 2461
shells.each do |shell|
popen_read_env = { "SHELL" => shell.to_s }
popen_read_env = { "SHELL" => "/bin/#{shell}" }
script_path = completion_script_path_map[shell]
Comment on lines 2459 to 2461
shells.each do |shell|
popen_read_env = { "SHELL" => shell.to_s }
popen_read_env = { "SHELL" => "/bin/#{shell}" }
script_path = completion_script_path_map[shell]
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.

This doesn't seem like the right fix. /bin/bash and /bin/zsh exist on macOS but may not on Linux and /bin/fish doesn't.


expect(bash_dir/"foo").to be_a_file
expect((bash_dir/"foo").read).to eq("bash completion output")
expect((bash_dir/"foo").read).to eq("/bin/bash completion output")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doesn't seem right?

expect((zsh_dir/"_foo").read).to eq("/bin/zsh completion output")
expect(fish_dir/"foo.fish").to be_a_file
expect((fish_dir/"foo.fish").read).to eq("fish completion output")
expect((fish_dir/"foo.fish").read).to eq("/bin/fish completion output")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This path doesn't exist

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Answered below, but we could set them to paths in homebrew's prefix if you think that makes more sense

@branchv
Copy link
Copy Markdown
Member Author

branchv commented May 17, 2026

/bin/bash and /bin/zsh exist on macOS but may not on Linux and /bin/fish doesn't.

Right, but this path doesn't need to exist here as it's only purpose is for the completion generator to infer what shell to generate for. Just as setting SHELL=fish when fish is not guaranteed to be installed. Does that make sense?

@MikeMcQuaid
Copy link
Copy Markdown
Member

@branchv I don't think we should make a global completion change like this just for an ngrok cask. It seems likely to break more than it will fix, sorry.

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