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.

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

Add brew shellenv - see #4780 #4849

Merged
merged 1 commit into from Sep 7, 2018

Conversation

Projects
None yet
3 participants
@jamescostian
Copy link
Contributor

jamescostian commented Sep 6, 2018

  • 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 for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Adds brew shellenv based on conversation in #4780

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

Looks good to me, thanks @jamescostian and sorry for all the back and forth! Is this sufficient for your use-case @sjackman? If so, mind

@@ -0,0 +1,7 @@
describe "brew shellenv", :integration_test do
it "doesn't fail" do

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 7, 2018

Member

Could you assert_match on e.g. the HOMEBREW_PREFIX/bin being present?

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Sep 7, 2018

Looks good to me. Thanks, James! The one minor test change suggested by make would be good, to check that stdout contains #{HOMEBREW_PREFIX}/bin.

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Sep 7, 2018

After this is merge into Homebrew/brew and Linuxbrew/brew, we can update the installation instructions.

@jamescostian

This comment has been minimized.

Copy link
Contributor

jamescostian commented Sep 7, 2018

Just added that to the test 👍

@sjackman sjackman self-assigned this Sep 7, 2018

@sjackman sjackman merged commit 922843f into Homebrew:master Sep 7, 2018

3 checks passed

codecov/patch 100% of diff hit (target 71.34%)
Details
codecov/project 71.36% (+0.02%) compared to cb2cf65
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Sep 7, 2018

Merged. Thanks for your contribution to Homebrew, James! 🏆

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Sep 7, 2018

There's two ways that brew shellenv could be added to ~/.profile.

~/.linuxbrew/bin/brew shellenv >>~/.profile

or

echo 'eval $(~/.linuxbrew/bin/brew shellenv)' >>~/.profile

I have a slight preference for the latter using eval. Your opinion?

@jamescostian jamescostian deleted the jamescostian:shellenv branch Sep 7, 2018

@jamescostian

This comment has been minimized.

Copy link
Contributor

jamescostian commented Sep 7, 2018

Running time bin/brew shellenv on my laptop (processor is i7-4870HQ, running at 2.8GHz, mostly idling) reports a real time of 0.40s, which IMO makes it way too slow for me to be willing to eval it more than once

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Sep 7, 2018

Agreed. It's 0.9s in my Docker instance.

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Sep 7, 2018

So the installation instructions should be…

eval $(~/.linuxbrew/bin/brew shellenv)
brew shellenv >>~/.profile
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 9, 2018

@sjackman My recommendation would be to advise people put eval in their profile and those who care about the overhead can hardcode it if necessary.

Another possible extension for this script would be exporting e.g. HOMEBREW_PREFIX and HOMEBREW_CELLAR so scripts (or the profile itself) can avoid calling brew --prefix to further improve performance.

Nice work @jamescostian!

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Sep 9, 2018

My recommendation would be to advise people put eval in their profile and those who care about the overhead can hardcode it if necessary.

Is that so that we can change the output of brew shellenv, and have all users up-to-date without needing to modify their shell configuration?

I suspect that implementing this script in shell rather than Ruby would execute quite a bit faster. If it's going in ~/.profile maybe it's not such a big deal, since it'd only be executed when opening a new terminal (or ssh session). I'd be more concerned if it's going in ~/.bashrc where it'd be executed for every subshell.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 10, 2018

Is that so that we can change the output of brew shellenv, and have all users up-to-date without needing to modify their shell configuration?

Yep 👍

I suspect that implementing this script in shell rather than Ruby would execute quite a bit faster

It would indeed. Given it only relies on HOMEBREW_PREFIX being defined and even any extensions I see would only need things already defined in the Bash layer it should be possible to rewrite it in Bash, name it cmd/shellenv.sh and it'll avoid needing to ever execute the Ruby layer.

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Sep 10, 2018

@jamescostian Would you be interested in submitting a PR to rewrite shellenv in shell so that it takes less time to execute?

@jamescostian

This comment has been minimized.

Copy link
Contributor

jamescostian commented Sep 10, 2018

Sure, the bash rewrite is pretty simple:

echo "export PATH=\"$HOMEBREW_PREFIX/bin:$HOMEBREW_PREFIX/sbin:\$PATH\""
echo "export MANPATH=\"$HOMEBREW_PREFIX/share/man:\$MANPATH\""
echo "export INFOPATH=\"$HOMEBREW_PREFIX/share/info:\$INFOPATH\""
echo "export HOMEBREW_PREFIX=\"$HOMEBREW_PREFIX\""

But there are two issues:

  1. I'm getting an error at the end of the output: "Library/Homebrew/brew.sh: line 405: homebrew-shellenv: command not found" - any tips for that?
  2. Even if I comment out the line that makes that error, the execution time is .10s

That execution time seems to be impacted most from this line and this line. They both talk to git and they both look important enough to keep.

One way to decrease the execution time to .01s would be to treat shellenv completely differently: see the next comment for a better way

--- a/bin/brew
+++ b/bin/brew
@@ -65,7 +65,7 @@ do
 done

 # test-bot does environment filtering itself
-if [[ -z "$HOMEBREW_NO_ENV_FILTERING" && "$1" != "test-bot" ]]
+if [[ -z "$HOMEBREW_NO_ENV_FILTERING" && "$1" != "test-bot" && "$1" != "shellenv" ]]
 then
   PATH="/usr/bin:/bin:/usr/sbin:/sbin"

@@ -82,6 +82,11 @@ then
   done

   exec /usr/bin/env -i "${FILTERED_ENV[@]}" /bin/bash "$HOMEBREW_LIBRARY/Homebrew/brew.sh" "$@"
+elif [[ "$1" == "shellenv" ]]
+then
+  echo "export PATH=\"$HOMEBREW_PREFIX/bin:$HOMEBREW_PREFIX/sbin:\$PATH\""
+  echo "export MANPATH=\"$HOMEBREW_PREFIX/share/man:\$MANPATH\""
+  echo "export INFOPATH=\"$HOMEBREW_PREFIX/share/info:\$INFOPATH\""
+  echo "export HOMEBREW_PREFIX=\"$HOMEBREW_PREFIX\""
 else
   source "$HOMEBREW_LIBRARY/Homebrew/brew.sh"
 fi
@jamescostian

This comment has been minimized.

Copy link
Contributor

jamescostian commented Sep 10, 2018

Update: here's a much less ugly approach (or at least, it's already been used elsewhere in the code):

--- a/Library/Homebrew/brew.sh
+++ b/Library/Homebrew/brew.sh
@@ -17,6 +17,10 @@ case "$*" in
   --prefix)            echo "$HOMEBREW_PREFIX"; exit 0 ;;
   --cellar)            echo "$HOMEBREW_CELLAR"; exit 0 ;;
   --repository|--repo) echo "$HOMEBREW_REPOSITORY"; exit 0 ;;
+  shellenv)            echo "export PATH=\"$HOMEBREW_PREFIX/bin:$HOMEBREW_PREFIX/sbin:\$PATH\"";
+                       echo "export MANPATH=\"$HOMEBREW_PREFIX/share/man:\$MANPATH\"";
+                       echo "export INFOPATH=\"$HOMEBREW_PREFIX/share/info:\$INFOPATH\"";
+                       echo "export HOMEBREW_PREFIX=\"$HOMEBREW_PREFIX\"";
+                       exit 0 ;;
 esac

 # A depth of 1 means this command was directly invoked by a user.

This approach also brings the time down to .01s

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Sep 10, 2018

I have a mild preference for the following style over using backslashes to escape.

echo "export PATH='$HOMEBREW_PREFIX/bin:$HOMEBREW_PREFIX/sbin'":'"$PATH"'

I'll defer to Mike on your question regarding speed.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 11, 2018

I'm getting an error at the end of the output: "Library/Homebrew/brew.sh: line 405: homebrew-shellenv: command not found" - any tips for that?

You need to put your code inside a function homebrew-shellenv() { .. }.

On my machine there's still a 3.5x speedup by moving this from Ruby to shell (0.72s to 0.21s). I'm not convinced it's worth making the brew.sh code less maintainable to put this directly in brew.sh (imagine if we want to extend brew shellenv in future or even just keep having the --help work).

@lock lock bot added the outdated label Oct 11, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Oct 11, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.