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/ruby.sh: search PATH for Ruby on Linux. update.sh: keep HOMEBREW_RUBY_PATH set. #7545
utils/ruby.sh: search PATH for Ruby on Linux. update.sh: keep HOMEBREW_RUBY_PATH set. #7545
Conversation
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.
see that it was added on purpose (bcca2a7) but I think the logic behind unsetting it has to be more complex.
I agree with this but the logic should be made more complex rather than just removed.
Since bcca2a7 was authored by you, do you remember under what conditions |
I think I got it: |
I implemented the following logic:
This will keep Is that close to what you were thinking? |
The issue was/is:
This should never be the case. |
Library/Homebrew/cmd/update.sh
Outdated
if [[ -n $HOMEBREW_FORCE_VENDOR_RUBY && | ||
-x $HOMEBREW_LIBRARY/Homebrew/vendor/portable-ruby/current/bin/ruby ]] | ||
then | ||
export HOMEBREW_RUBY_PATH="$HOMEBREW_LIBRARY/Homebrew/vendor/portable-ruby/current/bin/ruby" |
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.
This is already done by utils/ruby.sh
when brew update-report
is called.
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.
This should never be the case.
utils/ruby.sh
should be finding the relevant Ruby from the PATH
echo $PATH
right next to HOMEBREW_RUBY_PATH="$(type -P ruby)"
in utils/ruby.sh
-- shows that PATH is /usr/bin:/bin:/usr/sbin:/sbin
so it finds system Ruby only.
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.
ruby.sh
is being run again so I don't see a reason to duplicate the logic here. It could do something with HOMEBREW_PATH
if needed. @sjackman also had thoughts about this.
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.
ruby.sh
is being run again
Exactly. And in case of update
command, HOMEBREW_RUBY_PATH
is unset in two places: here (unconditionally for everyone) and then for non-developers in ruby.sh. Because of the unset here, brew update
is the only command that fails on old systems where Ruby has to be bootstrapped and Homebrew pointed to it with HOMEBREW_RUBY_PATH
...
Anyway, I'll move logic to ruby.sh
@rmNULL is working on a patch where on Linux before failing because it cannot find |
I've done exactly that, see the diff.
|
@rmNULL Could you please test this PR? |
gladly. |
Library/Homebrew/utils/ruby.sh
Outdated
old_ruby_path="$HOMEBREW_RUBY_PATH" | ||
old_ruby_usable=$(test-ruby "$HOMEBREW_RUBY_PATH") | ||
fi | ||
|
||
if [[ -z "$HOMEBREW_DEVELOPER" ]] | ||
then | ||
unset HOMEBREW_RUBY_PATH |
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.
How about just unsetting this unconditionally? It would allow simplifying the logic above and below
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.
Yes, It should be OK to do it now.
Library/Homebrew/utils/ruby.sh
Outdated
if [[ $old_ruby_usable != true ]] | ||
then | ||
odie <<-EOS | ||
Failed to find usable Ruby $required_ruby_version! |
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.
Would be nice to move this new logic below so the messaging can be shared rather than repeated.
Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>
Hi @maxim-belkin , can u please verify this? I'm on x86_64 |
when no usable Ruby is found.
CI tests are overrated Thanks, @rmNULL! |
https://github.com/Homebrew/brew/pull/7545/checks?check_run_id=679406315#step:12:44 |
Library/Homebrew/utils/ruby.sh
Outdated
@@ -45,15 +56,29 @@ EOS | |||
then | |||
HOMEBREW_RUBY_PATH="/System/Library/Frameworks/Ruby.framework/Versions/Current/usr/bin/ruby" | |||
else | |||
HOMEBREW_RUBY_PATH="$(type -P ruby)" | |||
HOMEBREW_RUBY_PATH=$(type -P ruby) | |||
if [[ $(test-ruby $HOMEBREW_RUBY_PATH) != "true" ]] |
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.
@maxim-belkin L#60,
the test-ruby fn considers $1.
i suspect this will be problematic.
can i know the trick that handles spaced ruby path here?
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.
right -- here quotes are necessary. Will fix! Thanks again!
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.
Fixed in 083f56f.
I was considering using "$@"
but then decided to proceed with $1
. Thanks for catching it.
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.
Refactoring here looking good. One more tweak and, given @sjackman is happy, I'm 👍
Library/Homebrew/utils/ruby.sh
Outdated
HOMEBREW_RUBY_PATH=$(PATH="$HOMEBREW_PATH" type -P ruby) | ||
if [[ $(test-ruby "$HOMEBREW_RUBY_PATH") != "true" ]] | ||
then | ||
if [[ $old_ruby_usable != true ]] |
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.
Would be good to avoid quite so much nesting here and instead use an intermediate variable(s) and multiple if
s at the same level?
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.
Converted nested if
s to a single if/elif/else conditional block in b4267d8.
Had to add a line to test-ruby
that checks that tested file exists and is executable.
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.
Thanks, Maxim!
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.
Sorry! One last question about old_ruby_path
(would love it removed it possible?) and then this is 👍. Great work here @maxim-belkin
Library/Homebrew/utils/ruby.sh
Outdated
then | ||
unset HOMEBREW_RUBY_PATH | ||
fi | ||
[[ -n $HOMEBREW_RUBY_PATH ]] && old_ruby_path=$HOMEBREW_RUBY_PATH |
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'm probably missing something but: why keep this around at all if we're checking PATH
and HOMEBREW_PATH
anyway?
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.
This part handles the cases when user sets HOMEBREW_RUBY_PATH
directly. Ability to set HOMEBREW_RUBY_PATH
directly instead of modifying PATH
is congruent with how Homebrew allows to set HOMEBREW_CURL_PATH
and HOMEBREW_GIT_PATH
.
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.
Homebrew allows to set
HOMEBREW_CURL_PATH
andHOMEBREW_GIT_PATH
.
We don't allow that (intentionally at least) and don't document them as supported. HOMEBREW_FORCE_BREWED_CURL
and HOMEBREW_FORCE_BREWED_GIT
are the entry points for that. What's the use-case for not wanting to use the ruby
in your HOMEBREW_PATH
or PATH
or the vendored Ruby.
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 don't see this applicable in Mac(in my limited view) but in linux
i have a ruby 2.6 installed just to make homebrew work, the ruby version provided by the package manager is 2.7.
I wish to have ruby 2.7 earlier in PATH
. As i don't want which to use ruby installed just to maintain homebrew.
Now this is not a strict requirement, i can chuck away the homebrew ruby somewhere behind in PATH.
but this implementation (type -P ruby
) finds the first executable ruby, and it will fail and ask me to install ruby 2.6(or try to download) even though a ruby 2.6 is present.
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 see. I guess ideally we'd actually search through the HOMEBREW_PATH
and PATH
(which -a
?) to find a ruby
that passes our test and only give up if we don't find any?
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 don't agree. brew update has code specifically to install curl and git if needed before doing anything else. If that doesn't work: let's fix it rather than relying on hacking undocumented functionality to make it work.
If the system curl
is too old, and the user has a newer version of curl
say in their Home directory, HOMEBREW_CURL_PATH
is currently how you point Homebrew to that newer version of curl
. Homebrew can't download the bottles or source code of curl
without a recent version of curl
, so HOMEBREW_FORCE_BREWED_CURL
is not helpful in this case.
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.
HOMEBREW_CURL_PATH
are is the intentionally developer-exposed environment variable, and HOMEBREW_CURL
is the not developer-exposed internal enivronment variable. HOMEBREW_CURL_PATH
is active only when HOMEBREW_DEVELOPER
is active. See
Lines 96 to 106 in b2fc8ad
if [[ -n "$HOMEBREW_FORCE_BREWED_CURL" && | |
-x "$HOMEBREW_PREFIX/opt/curl/bin/curl" ]] && | |
"$HOMEBREW_PREFIX/opt/curl/bin/curl" --version >/dev/null | |
then | |
HOMEBREW_CURL="$HOMEBREW_PREFIX/opt/curl/bin/curl" | |
elif [[ -n "$HOMEBREW_DEVELOPER" && -x "$HOMEBREW_CURL_PATH" ]] | |
then | |
HOMEBREW_CURL="$HOMEBREW_CURL_PATH" | |
else | |
HOMEBREW_CURL="curl" | |
fi |
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 have to say that searching through the clean and user PATHs is likely to impact performance. That
old_ruby_path
was playing a role of cache, I guess.
I ran few tests,
master branch for reference
ruby 2.6 is first in which -a ruby
ruby 2.6 is last in which -a ruby
ruby 2.6 set using HOMEBREW_RUBY_PATH(tested on previous commit)
once the 2.6 executable is found, may be we cache it somewhere. that should give a little speed bump in the next runs.
Searching through PATH for every brew
call don't seem such a nice idea imo.
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.
so
HOMEBREW_FORCE_BREWED_CURL
is not helpful in this case.
I thought curl
had an HTTP mirror but I forgot it no longer does as I did not know this was required for Linux: Homebrew/homebrew-core@2f5aa14. There was also an audit
to ensure that curl
always had an HTTP mirror.
I'd suggest that if we want to be more accessible to older Linux versions we go down that route rather than requiring the user to manually find and download a suitable curl
, set HOMEBREW_DEVELOPER
and use an undocumented and unsupported environment variable.
Alternatively or additionally, we could take the same approach as here with curl
and git
where we look through all of those in the PATH
.
once the 2.6 executable is found, may be we cache it somewhere.
This seems like a nice idea. Would be cool to have in this PR but I don't consider it blocking; this is already a fairly niche case (which we could warn users on?).
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.
once the 2.6 executable is found, may be we cache it somewhere.
This seems like a nice idea. Would be cool to have in this PR but I don't consider it blocking; this is already a fairly niche case (which we could warn users on?).
I can add it in this PR.
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.
One suggested tweak but I'm happy with this as-is. Nice work, @maxim-belkin
Library/Homebrew/utils/ruby.sh
Outdated
break | ||
fi | ||
done | ||
IFS=$' \t\n' |
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.
Is it worth setting this back to whatever it was before e.g. OLD_IFS
or something?
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.
$' \t\n'
is the default value for IFS
:
IFS The Internal Field Separator that is used for word splitting after expansion and to split lines into words with the read builtin command. The default value is ``<space><tab><new-line>''.
I'm restoring it back to its normal value after setting it to $'\n'
which causes word splitting to happen on new lines only.
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.
Yup but now to understand the code I have to know that this is the default value whereas if you used OLD_IFS
I'd know why it was being set back to this 😁
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.
to understand the code I have to know that this is the default value
I can add a comment explaining/reminding that $' \t\n'
is the default value.
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.
Works for me 👍
Not for this PR, but a related comment. We have environment variables that are exposed to the user (or developer) and environment variables that are not settable by the user, completely ignored by Homebrew, and used for internal communication between Homebrew components. For example, # an environment variable settable by the user starts with HOMEBREW_
export HOMEBREW_NO_AUTO_UPDATE=1
# an environment variable used for internal communication of Homebrew not settable by the user starts with homebrew_
export homebrew_curl=/usr/local/opt/curl/bin/curl
# an unexported shell variable has no prefix, and specifically does not start with homebrew_
local foo=bar |
+1 for the idea in general but I have to admit that I like that Homebrew uses clear and easy to see |
@sjackman @maxim-belkin yeh, I'd also like to see an internal variable distinction. I like |
Exit from the 'setup-ruby' function when user issued `vendor-install` command. We do so instead of wrapping everything in ```sh if [[ "$HOMEBREW_COMMAND" != "vendor-install" ]] ``` `git diff` when whitespaces are ignored: $ git diff -w diff --git a/Library/Homebrew/utils/ruby.sh b/Library/Homebrew/utils/ruby.sh index 7974e909c..4be204309 100644 --- a/Library/Homebrew/utils/ruby.sh +++ b/Library/Homebrew/utils/ruby.sh @@ -27,8 +27,11 @@ If there's no Homebrew Portable Ruby available for your processor: unset HOMEBREW_RUBY_PATH - if [[ "$HOMEBREW_COMMAND" != "vendor-install" ]] + if [[ "$HOMEBREW_COMMAND" == "vendor-install" ]] then + return 0 + fi + if [[ -x "$vendor_ruby_path" ]] then HOMEBREW_RUBY_PATH="$vendor_ruby_path" @@ -85,7 +88,6 @@ If there's no Homebrew Portable Ruby available for your processor: HOMEBREW_RUBY_PATH="$vendor_ruby_path" fi fi - fi export HOMEBREW_RUBY_PATH }
1. Repurpose 'vendor_ruby_current_version' variable: now this is not a pointer to a file but actual version number 2. Introduce 'vendor_ruby_latest_version' variable: it holds the value of the latest version of portable Ruby
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.
Looks good! Shout when you consider this ready to merge @maxim-belkin.
This is still in progress. Question: if "vendor Ruby" is not up-to-date (that is, current version != latest version) but is usable (e.g., installed version is 2.6.3, latest is 2.6.6) -- shall Homebrew upgrade it? If so, shall Homebrew fail upon failing to upgrade it? |
Short answer: yes and yes |
I wonder if the second "yes" is reasonable / user-friendly. Instead of failing, we could search for a usable Ruby in PATH or /System folder (on a Mac). |
I suppose, but I wouldn't want to overly complicate the code to handle this (hopefully uncommon) edge case. |
FYI if interested: the logic to remove an unnecessary vendored-ruby is in
Agreed. |
The difference is
Thank you for the pointer! @MikeMcQuaid, changes that I'm working on are probably out of scope of this PR and I think it makes sense to merge this one now and discuss the other changes in a separate issue/PR. Apologies for delaying this. |
Thanks again @maxim-belkin! |
Yes, thank you, Maxim! |
Thank you for the reviews and patience, @MikeMcQuaid, @sjackman, and @rmNULL! |
Thanks for the effort :) |
brew style
with your changes locally?brew tests
with your changes locally?Unsetting
HOMEBREW_RUBY_PATH
on platforms where system Ruby is too old breaks Homebrew. I see that it was added on purpose (bcca2a7) but I think the logic behind unsetting it has to be more complex.