-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Utils: shortened brew paths #11537
Utils: shortened brew paths #11537
Conversation
Review period will end on 2021-06-16 at 01:53:16 UTC. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea but implementation needs some tweaks!
Moved some things around. Now the new methods live in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! 🎉
Review period ended. |
This reverts commits 318175c, e7ab760 and 3b35af6 (PR Homebrew#11537).
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?Overview
This PR adds:
utils/path
Utils.path_is_parent_of?
determines if onePathname
/String
is a parent of anotherPathname
/String
Utils.shortened_brew_path
expresses aPathname
/String
as a shell expansion where possible, e.g./usr/local/Cellar/cmake
as$(brew --cellar)/cmake
. Where possible, it can also express aPathname
as a formula's opt prefix, e.g./usr/local/opt/openjdk/libexec/openjdk.jdk/Contents/Home
becomes$(brew --prefix openjdk)/libexec/openjdk.jdk/Contents/Home
language/java
Language::Java.short_java_home_shell
,Language::Java.short_java_home_env
,Language::Java.overridable_short_java_home_env
which are shortened versions of the existing respective methodsThis PR also modifies
write_env_script
,write_exec_script
, andwrite_jar_script
to useshortened_brew_path
.Rationale
Now that we are bottling everything, there are many cases where we can't use
all:
bottles because an env script, exec script, jar script etc. has a platform-dependentHOMEBREW_PREFIX
in it (e.g./usr/local
vs/opt/homebrew
).The result is that we have to carry extra copies of some bottles where the bulk of the payload is the same. This is especially the case in Java based formulae, where the resulting bottle can be tens or hundreds of megabytes in size.
Commands like
$(brew --prefix)
,$(brew --cellar)
, etc. don't invoke the full Homebrew installation (they short-circuit to shell script instead), so using these where possible in scripts makes the resulting bottle portable while adding minimal performance cost.A resulting env script might look something like this:
I'm especially open to input on the naming of any of the new methods introduced in this PR. Wasn't really sure about a most suitable name so just went with my gut for now.
Note: I'm not happy with the tests for the additional Java methods right now. In the test env, they evaluate to
$(brew --prefix)/opt/openjdk/libexec/...
when in practice (from testing with formulae) it actually produces a more desired$(brew --prefix openjdk)/libexec/...
. Both are technically correct (whenopenjdk
is installed), but just wanted to point out this slight discrepancy. I think this is due to how theopenjdk
formula is being mocked in that test, but after trying to manually set up folders/symlink them where appropriate in the test, I still couldn't get it working exactly as desired.