redis-rb / redis-client

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

hiredis-client: use -fvisibility=hidden instead of -exported_symbols_list #82

Closed casperisfine closed 1 year ago

casperisfine commented 1 year ago

Rather than to use an exported symbols list, we compile with -fvisibility=hidden and mark the init function with RUBY_FUNC_EXPORTED.

This was suggested by @nobu.

Ref: https://github.com/redis-rb/redis-client/issues/58#issuecomment-1397802949

casperisfine commented 1 year ago

Yeah, but CI is broken for some reason, not sure if related or not.

casperisfine commented 1 year ago

Somehow the hiredis test suite pass on my machine, but on CI it's consistently broken for 3.1 and 3.2. 😫

eregon commented 1 year ago

Is it also broken on master? Maybe trigger a run from master's commit to see if it's due to these changes or not.

casperisfine commented 1 year ago

Is it also broken on master?

No it passes fine: https://github.com/redis-rb/redis-client/actions/runs/3968428783/jobs/6801585027

the pure-ruby part of the test suite also pass fine, only the hiredis (binding) part is failing with a weird EAGAIN issue.

I probably won't have time to debug this much before a week or two though :/

casperisfine commented 1 year ago

interesting, if I try to repro locally inside the ruby:3.2 docker image, I get a ENOTSOCK error on connect, that's even wierder.

@eregon given that you are on Linux if I understand correctly, I wonder if you get the same issue?

casperisfine commented 1 year ago

So the issue seem to be that on Linux the visibility=hidden doesn't apply to the symbols coming from the libhiredis.a, I tried a few solution I found on the web for no luck so far.

eregon commented 1 year ago

Even though libhiredis.a is compiled with visibility=hidden?

casperisfine commented 1 year ago

Well, somehow on 3.2 it is not passed:

cc -std=c99 -pedantic -c -g -fPIC  -DHIREDIS_TEST_SSL -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb alloc.c
cc -std=c99 -pedantic -c -g -fPIC  -DHIREDIS_TEST_SSL -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb net.c
cc -std=c99 -pedantic -c -g -fPIC  -DHIREDIS_TEST_SSL -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb hiredis.c
cc -std=c99 -pedantic -c -g -fPIC  -DHIREDIS_TEST_SSL -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb sds.c
cc -std=c99 -pedantic -c -g -fPIC  -DHIREDIS_TEST_SSL -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb async.c
cc -std=c99 -pedantic -c -g -fPIC  -DHIREDIS_TEST_SSL -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb read.c
cc -std=c99 -pedantic -c -g -fPIC  -DHIREDIS_TEST_SSL -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb sockcompat.c
ar rcs libhiredis.a alloc.o net.o hiredis.o sds.o async.o read.o sockcompat.o
cc -std=c99 -pedantic -c -g -fPIC  -DHIREDIS_TEST_SSL -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb ssl.c

But on 3.0 it is:

cc -std=c99 -pedantic -c -g -fPIC  "-I/opt/hostedtoolcache/Ruby/3.0.5/x64/openssl/include" "-fvisibility=hidden" -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb alloc.c
cc -std=c99 -pedantic -c -g -fPIC  "-I/opt/hostedtoolcache/Ruby/3.0.5/x64/openssl/include" "-fvisibility=hidden" -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb net.c
cc -std=c99 -pedantic -c -g -fPIC  "-I/opt/hostedtoolcache/Ruby/3.0.5/x64/openssl/include" "-fvisibility=hidden" -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb hiredis.c
cc -std=c99 -pedantic -c -g -fPIC  "-I/opt/hostedtoolcache/Ruby/3.0.5/x64/openssl/include" "-fvisibility=hidden" -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb sds.c
cc -std=c99 -pedantic -c -g -fPIC  "-I/opt/hostedtoolcache/Ruby/3.0.5/x64/openssl/include" "-fvisibility=hidden" -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb async.c
cc -std=c99 -pedantic -c -g -fPIC  "-I/opt/hostedtoolcache/Ruby/3.0.5/x64/openssl/include" "-fvisibility=hidden" -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb read.c
cc -std=c99 -pedantic -c -g -fPIC  "-I/opt/hostedtoolcache/Ruby/3.0.5/x64/openssl/include" "-fvisibility=hidden" -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb sockcompat.c
ar rcs libhiredis.a alloc.o net.o hiredis.o sds.o async.o read.o sockcompat.o
cc -std=c99 -pedantic -c -g -fPIC  "-I/opt/hostedtoolcache/Ruby/3.0.5/x64/openssl/include" "-fvisibility=hidden" -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb ssl.c

So that's likely the source of the issue yeah.

casperisfine commented 1 year ago

Ok, I finally figured it out.

@flavorjones is working on a cleanup of the extconf.rb though, so I'll wait for his PR first.

eregon commented 1 year ago

@flavorjones is working on a cleanup of the extconf.rb though, so I'll wait for his PR first.

Sounds like heaven to me :) (I've come across many extconf.rb, and most could afford a good cleanup, it tends to be the least readable Ruby scripts of all)

casperisfine commented 1 year ago

Ok, all green. Thank you all! It's much cleaner now.