redis-rb / redis-client

Simple low level client for Redis 6+
MIT License
125 stars 60 forks source link

Workaround the Xcode 14 compilation issue #59

Closed casperisfine closed 1 year ago

casperisfine commented 1 year ago

Fix: https://github.com/redis-rb/redis-client/issues/58 Ref: https://bugs.ruby-lang.org/issues/19005

Rubies compiled with Xcode 14 are missing some flags in LDFLAGS which cause the hiredis gem to fail to compile.

We work around this by adding the flags back if we detect this bug.

stanhu commented 1 year ago

This workaround will work for now, though there's a reason why the Ruby interpreter handles the detection of the linker flags: iOS and other platforms may not support -Wl,-undefined,dynamic_lookup. We could tighten the check that RUBY_PLATFORM contains darwin to avoid that.

I think a better solution would be to export _ruby_abi_version only if the platform is on Ruby 3.2+. Maybe this would be better?

diff --git a/hiredis-client/ext/redis_client/hiredis/export.clang b/hiredis-client/ext/redis_client/hiredis/export.clang
index 10065f7..883acc2 100644
--- a/hiredis-client/ext/redis_client/hiredis/export.clang
+++ b/hiredis-client/ext/redis_client/hiredis/export.clang
@@ -1,2 +1 @@
 _Init_hiredis_connection
-_ruby_abi_version
diff --git a/hiredis-client/ext/redis_client/hiredis/extconf.rb b/hiredis-client/ext/redis_client/hiredis/extconf.rb
index 39204c3..91a909e 100644
--- a/hiredis-client/ext/redis_client/hiredis/extconf.rb
+++ b/hiredis-client/ext/redis_client/hiredis/extconf.rb
@@ -56,6 +56,7 @@ if RUBY_ENGINE == "ruby" && !RUBY_PLATFORM.match?(/mswin/)

   if `cc --version`.match?(/ clang /i) || RbConfig::CONFIG['CC'].match?(/clang/i)
     $LDFLAGS << ' -Wl,-exported_symbols_list,"' << File.join(__dir__, 'export.clang') << '"'
+    $LDFLAGS << " -Wl,-exported_symbol,_ruby_abi_version" if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('3.2.0')
   elsif RbConfig::CONFIG['CC'].match?(/gcc/i)
     $LDFLAGS << ' -Wl,--version-script="' << File.join(__dir__, 'export.gcc') << '"'
   end
casperisfine commented 1 year ago

export _ruby_abi_version only if the platform is on Ruby 3.2+

I'd be OK with that.

casperisfine commented 1 year ago

I pushed a new commit, let me know what you think. Also if you are able to repro on your machine I appreciate if you could check if it works for you.

stanhu commented 1 year ago

Commit looks good! Using asdf-ruby, I validated:

Branch Ruby version bundle exec rake test pass?
master 3.1.2 :x:
handle-clang-14-bug 3.1.2 :white_check_mark:
handle-clang-14-bug 3.2.0-dev :white_check_mark: (see below)

I did get this test failure in 3.2.0-dev, which seems unrelated:

<internal:gc>:251: warning: double_heap is deprecated, please use expand_heap instead
Running test suite with driver: RedisClient::HiredisConnection
/Users/stanhu/github/redis-client/test/support/raise_warnings.rb:7:in `warn': <internal:gc>:251: warning: double_heap is deprecated, please use expand_heap instead (RuntimeError)
    from <internal:gc>:251:in `verify_compaction_references'
    from /Users/stanhu/github/redis-client/test/hiredis/test_helper.rb:23:in `<top (required)>'
    from /Users/stanhu/github/redis-client/test/redis_client/command_builder_test.rb:3:in `require'
    from /Users/stanhu/github/redis-client/test/redis_client/command_builder_test.rb:3:in `<top (required)>'
    from /Users/stanhu/.asdf/installs/ruby/3.2.0-dev/lib/ruby/gems/3.2.0+3/gems/rake-13.0.6/lib/rake/rake_test_loader.rb:21:in `require'
    from /Users/stanhu/.asdf/installs/ruby/3.2.0-dev/lib/ruby/gems/3.2.0+3/gems/rake-13.0.6/lib/rake/rake_test_loader.rb:21:in `block in <main>'
    from /Users/stanhu/.asdf/installs/ruby/3.2.0-dev/lib/ruby/gems/3.2.0+3/gems/rake-13.0.6/lib/rake/rake_test_loader.rb:6:in `select'
    from /Users/stanhu/.asdf/installs/ruby/3.2.0-dev/lib/ruby/gems/3.2.0+3/gems/rake-13.0.6/lib/rake/rake_test_loader.rb:6:in `<main>'
rake aborted!

I should note that asdf-ruby installs OpenSSL in ~/.asdf/installs/ruby/<ruby_version>/openssl and configures Ruby to use that path with --with-openssl-dir by default. For 3.2.0-dev, I needed to use OPENSSL_PREFIX=~/.asdf/installs/ruby/3.2.0-dev/openssl. Perhaps we can improve the Makefile so it automatically searches the Ruby-configured directory, but that's a separate issue.