Homebrew / brew

🍺 The missing package manager for macOS (or Linux)
https://brew.sh
BSD 2-Clause "Simplified" License
41.36k stars 9.73k forks source link

Allow shellenv to accept a shell name parameter #15358

Closed halostatue closed 1 year ago

halostatue commented 1 year ago

Verification

Provide a detailed description of the proposed feature

brew shellenv does not currently accept a shell name parameter to override its default output, based on the output /bin/ps. The ps approach seems best for most initialization and interactive use cases, but for some advanced use cases, it would be desirable to be able to explicitly choose which shell information is displayed, like:

$ brew shellenv zsh
export HOMEBREW_PREFIX="…"
…
$ brew shellenv fish
set -gx HOMEBREW_PREFIX="…"

What is the motivation for the feature?

An interesting caching technique was discussed in twpayne/chezmoi that could see someone writing a template that calls {{ output "brew" "shellenv" | trim }} in a template to capture that information. If they (like me) are running under fish, but keep some bash and/or zsh scripts with this caching, then the output of brew shellenv will be fish-specific and not zsh-specific.

How will the feature be relevant to at least 90% of Homebrew users?

It will not be. It will be invisible to most users since it should have no impact for the 98% use case.

What alternatives to the feature have been considered?

There are no alternatives because of the specificity of the call to /bin/ps.

I am willing to open a PR for this since the audience is narrow. The changes would be:

  1. Library/Homebrew/cmd/shellenv.sh:20 needs to read to read $1 or use the output of ps if not provided ("${1:-$(/bin/ps -p "${PPID}" -c -o comm=)}")
  2. Library/Homebrew/cmd/shellenv.sh:1–7 would need to be updated to show the additional optional parameter and document its use.
  3. Library/Homebrew/brew.sh:84 may need to pass "$1" or "$@". (Local testing suggests that "$1" is being inherited in some way.)
diff --git Library/Homebrew/cmd/shellenv.sh Library/Homebrew/cmd/shellenv.sh
index a19a2223d520..6ffac2a599a5 100644
--- Library/Homebrew/cmd/shellenv.sh
+++ Library/Homebrew/cmd/shellenv.sh
@@ -1,4 +1,4 @@
-#:  * `shellenv`
+#:  * `shellenv [bash|csh|fish|pwsh|sh|tcsh|zsh]`
 #:
 #:  Print export statements. When run in a shell, this installation of Homebrew will be added to your `PATH`, `MANPATH`, and `INFOPATH`.
 #:
@@ -6,6 +6,8 @@
 #:  To help guarantee idempotence, this command produces no output when Homebrew's `bin` and `sbin` directories are first and second
 #:  respectively in your `PATH`. Consider adding evaluation of this command's output to your dotfiles (e.g. `~/.profile`,
 #:  `~/.bash_profile`, or `~/.zprofile`) with: `eval "$(brew shellenv)"`
+#:
+#:  The shell can be specified explicitly with a supported shell name parameter. Unknown shells will output POSIX exports.

 # HOMEBREW_CELLAR and HOMEBREW_PREFIX are set by extend/ENV/super.rb
 # HOMEBREW_REPOSITORY is set by bin/brew
@@ -18,7 +20,7 @@ homebrew-shellenv() {
     return
   fi

-  case "$(/bin/ps -p "${PPID}" -c -o comm=)" in
+  case "${1:-$(/bin/ps -p "${PPID}" -c -o comm=)}" in
     fish | -fish)
       echo "set -gx HOMEBREW_PREFIX \"${HOMEBREW_PREFIX}\";"
       echo "set -gx HOMEBREW_CELLAR \"${HOMEBREW_CELLAR}\";"

This produces the expected result (I’m running under fish):

$ brew shellenv pwsh
[System.Environment]::SetEnvironmentVariable('HOMEBREW_PREFIX','/opt/homebrew',[System.EnvironmentVariableTarget]::Process)
[System.Environment]::SetEnvironmentVariable('HOMEBREW_CELLAR','/opt/homebrew/Cellar',[System.EnvironmentVariableTarget]::Process)
[System.Environment]::SetEnvironmentVariable('HOMEBREW_REPOSITORY','/opt/homebrew',[System.EnvironmentVariableTarget]::Process)
[System.Environment]::SetEnvironmentVariable('PATH',$('/opt/homebrew/bin:/opt/homebrew/sbin:'+$ENV:PATH),[System.EnvironmentVariableTarget]::Process)
[System.Environment]::SetEnvironmentVariable('MANPATH',$('/opt/homebrew/share/man'+$(if(${ENV:MANPATH}){':'+${ENV:MANPATH}})+':'),[System.EnvironmentVariableTarget]::Process)
[System.Environment]::SetEnvironmentVariable('INFOPATH',$('/opt/homebrew/share/info'+$(if(${ENV:INFOPATH}){':'+${ENV:INFOPATH}})),[System.EnvironmentVariableTarget]::Process)
$ brew shellenv
set -gx HOMEBREW_PREFIX "/opt/homebrew";
set -gx HOMEBREW_CELLAR "/opt/homebrew/Cellar";
set -gx HOMEBREW_REPOSITORY "/opt/homebrew";
set -q PATH; or set PATH ''; set -gx PATH "/opt/homebrew/bin" "/opt/homebrew/sbin" $PATH;
set -q MANPATH; or set MANPATH ''; set -gx MANPATH "/opt/homebrew/share/man" $MANPATH;
set -q INFOPATH; or set INFOPATH ''; set -gx INFOPATH "/opt/homebrew/share/info" $INFOPATH;
MikeMcQuaid commented 1 year ago

@halostatue This seems reasonable, thanks for opening. Can you open a PR?

ChengLuffy commented 1 year ago

This is a disruptive change but no warning message is provided.

My macOS was previously configured with fishshell and Homebrew, and in ~/.config/fish/config.fish there is this line of code eval "$(/opt/homebrew/bin/brew shellenv), I don't know if this was added by Homebrew or fishshell automatically generates it.

Since this pull-request is merged, after I run brew update, every time I start the shell, I get an error and Homebrew doesn't work properly.

Now, I changed it to eval "$(/opt/homebrew/bin/brew shellenv fish) and it worked again.

Hope this helps others.

halostatue commented 1 year ago

@ChengLuffy Thanks for the report. This is a bug that I missed in my latest testing and I’ve submitted a fix PR in #15383.

MikeMcQuaid commented 1 year ago

This is a disruptive change but no warning message is provided.

@ChengLuffy There was a bug from a first-time contributor. Please try to be more understanding in future 🙇🏻

@halostatue thanks for the quick turnaround here ❤️

ChengLuffy commented 1 year ago

@

This is a disruptive change but no warning message is provided.

@ChengLuffy There was a bug from a first-time contributor. Please try to be more understanding in future 🙇🏻

@halostatue thanks for the quick turnaround here ❤️

Because English is not my native language, my language may be a bit stiff. I'm sorry for my rudeness.

@halostatue Thank you for your timely response and code contribution.

MikeMcQuaid commented 1 year ago

@ChengLuffy No problem at all, thanks for apologising! ❤️

halostatue commented 1 year ago

I’m rather embarrassed that I missed this.

@ChengLuffy I was not offended by your message, as this should not have been a disruptive change and it’s my error for not testing the base case after the most recent changes. Nothing that a quick set -x wouldn’t have shown if I’d done my testing right. (I use fish as my default shell, so this is completely on me.)

I think that I had missed it because my shfmt configuration differs from the Homebrew shfmt configuration, so I have to disable my automatic formatting or break the formatting…and it looked like a mechanical change.

@MikeMcQuaid I’m curious whether there are thoughts on writing tests for these shell scripts so that this sort of error can be avoided in the future.

MikeMcQuaid commented 1 year ago

@MikeMcQuaid I’m curious whether there are thoughts on writing tests for these shell scripts so that this sort of error can be avoided in the future.

Yes, we have some integration tests that could be extended to our Bash scripts but currently are not. Provided they aren't excessively slow: that would be a great contribution if you're game to try? Happy to help if you can get a PR half-working!

halostatue commented 1 year ago

It’ll be a little while before I can get to this, but I’ll "save" this issue notification for revisiting at a later date.