Automatically detect and link to Homebrew's libyaml #1929
Conversation
bin/ruby-build
Outdated
| local libdir="$(brew --prefix libyaml 2>/dev/null || true)" | ||
| if [ -d "$libdir" ]; then | ||
| local libdir="$(homebrew_yaml_dir)" | ||
| if [ ! -z "$libdir" ]; then |
bin/ruby-build
Outdated
| @@ -1022,20 +1023,28 @@ locate_llvm() { | |||
| } | |||
|
|
|||
| needs_yaml() { | |||
| [[ "$RUBY_CONFIGURE_OPTS" != *--with-libyaml-dir=* ]] && | |||
| ! use_homebrew_yaml | |||
| [ -z "$(homebrew_yaml_dir)" ] | |||
There was a problem hiding this comment.
For backwards compatibility, please restore to the old implementation of needs_yaml. There was no need to change it, and the new implementation doesn't print the line ruby-build: using libyaml from homebrew anymore.
bin/ruby-build
Outdated
| homebrew_yaml_dir() { | ||
| if [[ "$RUBY_CONFIGURE_OPTS" != *--with-libyaml-dir=* ]]; then | ||
| local libdir="$(brew --prefix libyaml 2>/dev/null || true)" | ||
| if [[ -d "$libdir" ]]; then |
There was a problem hiding this comment.
Style: double brackets are not needed here, please use single ones
bin/ruby-build
Outdated
| echo "ruby-build: using libyaml from homebrew" | ||
| package_option ruby configure --with-libyaml-dir="$libdir" | ||
| else | ||
| return 1 | ||
| fi | ||
| } | ||
|
|
||
| homebrew_yaml_dir() { | ||
| if [[ "$RUBY_CONFIGURE_OPTS" != *--with-libyaml-dir=* ]]; then |
There was a problem hiding this comment.
It could be surprising that a function called homebrew_yaml_dir performs this check first. I would rather that the caller of homebrew_yaml_dir performs the check instead, even if that's going to lead to some repetition. Explicitness is better than burying some magic behavior deep down a call chain
|
thanks for the review, @mislav! I ended up reverting most of my changes and instead dropped the fixture usage in favor of an inline definition to avoid future false negatives in this test |
|
anything else I need to do to move this forward? |
|
Sorry for my inactivity. I was about to push some changes to PR a few weeks ago, but I got blocked on the inflexible stubbing support that we right now have in tests. So I'm going to try to improve that too. Nothing that you need to do for this PR anymore; thank you! |
|
I think I have issues this PR could fix. Trying to install on Ventura is bailing on not finding libyaml. |
|
@KevDog In the meantime, you can explicitly set |
|
Friendly bump 🙏🏼 Another use case is that package managers that depend on |
|
On a related note: https://twitter.com/eregontp/status/1631281524866678788 I guess this is only an issue on darwin-aarch64 and not darwin-amd64 because on the latter /usr/local/lib is part of the default library path. Maybe Homebrew should consider using /usr/local/lib or another place part of the default library path again on darwin-aarch64 and deal with "Rosetta vs not" in another way. That would be an issue for Homebrew of course. But yeah for now I think we very much need this in |
Co-authored-by: Mislav Marohnić <git@mislav.net>
mislav
changed the title
20230306: - Add mruby-3.2.0 by @hasumikin in rbenv/ruby-build#2155 - Automatically detect and link to Homebrew's libyaml by @dreyks in rbenv/ruby-build#1929 20230309: - Add JRuby 9.4.2.0 by @headius in rbenv/ruby-build#2160
20230306: - Add mruby-3.2.0 by @hasumikin in rbenv/ruby-build#2155 - Automatically detect and link to Homebrew's libyaml by @dreyks in rbenv/ruby-build#1929 20230309: - Add JRuby 9.4.2.0 by @headius in rbenv/ruby-build#2160
|
Thanks! |
closes #1928
turns out there already was a test for this behaviour but it was falsely passing due to
needs_yaml(that was run only from tests) was callinguse_homebrew_yaml(which was otherwise not called in production)Bash is not my native language and I've had very limited experience with BATS framework so there might be things I've done wrong :)