Closed casperisfine closed 2 years ago
The upstream fix for that issue was done in https://github.com/ruby/fiddle/commit/7fede8a3096f3aeb582806e3c8f65b8d415d3cbf
You can build it with -Wno-error=implicit-function-declaration
flags.
@hsbt yes I'm aware, the question is more wether there is desire to have this option applied by default to older rubies by ruby-build
when compiling with clang or wether it's something for the wiki.
There was a recent discussion about -Wno-error=implicit-function-declaration
in https://github.com/rbenv/ruby-build/issues/2001.
My feeling is it's generally unsafe to do that by default.
https://github.com/ruby/fiddle/commit/7fede8a3096f3aeb582806e3c8f65b8d415d3cbf mentions pkg-config
, if pkg-config
is installed does it avoid the problem? If so maybe one way to solve this is to add pkg-config
in https://github.com/rbenv/ruby-build/wiki ?
Another alternative would be to pass RUBY_CFLAGS=-DUSE_FFI_CLOSURE_ALLOC
automatically as mentioned in https://github.com/ffi/ffi/issues/869#issuecomment-752123090, but probably only for recent enough libffi, and I'm not sure how to find that out.
In general if one wants to build old Rubies, I think there is a much higher chance for that to work on Linux or in Docker. Apple/macOS seems rather careless about compatibility and typically old software on latest macOS doesn't work.
if pkg-config is installed does it avoid the problem?
I don't think so because it's installed on my system:
$ which pkg-config
/opt/homebrew/bin/pkg-config
Is there a way to disable Fiddle? That bug seems pretty messy, an AFAIK Fiddle is very rarely used. Also handling libffi correctly on M1 is unlikely to work well with an old Fiddle.
We can skip it with --with-out-ext=+,fiddle
option.
In general ruby-build does not patch Ruby/deps and does not try to fix things the OS updates messed up, the only exception being openssl because that's hard to do manually.
I think what we need to progress here is: understand if the -Wno-error=implicit-function-declaration
is fine or dangerous for this particular case:
make test-all
on it, or run Fiddle's tests in another way?-Wno-error=implicit-function-declaration
is never ideal because it ignores all such cases and not just this one, and some may be benign but in general it depends, e.g. if the function uses varargs it'd pass wrong values to the function.
That's what makes RUBY_CFLAGS=-DUSE_FFI_CLOSURE_ALLOC
more appealing to me, but the trade-off how to find out when passing that is appropriate?
We could apply https://github.com/ruby/fiddle/commit/7fede8a3096f3aeb582806e3c8f65b8d415d3cbf automatically but that'd be patching Ruby which in general we don't want to be in the business of maintaining such things or trying to support too far in old Ruby/new OS or vice-versa. Still may be the easiest and most reliable solution though. @casperisfine Could you try if installing with that patch works?
Disabling fiddle would be easy to implement, but would cause issues if people actually use Fiddle.
- Could you (@casperisfine) e.g. build 2.5.9 from source and run
make test-all
on it, or run Fiddle's tests in another way?
Ok, so it was quite tricky for many reasons. What I ended up doing:
ruby/fiddle
v1.0.0
, since AFAICT it's the version included with 2.5.9
.test/fiddle/helper.rb
with the one from latest fiddle.rake test
$ rake test
Run options:
# Running tests:
[ 64/142] Fiddle::TestFunc#test_syscall_with_tainted_string#<Thread:0x0000000132105c88@/Users/byroot/src/github.com/ruby/fiddle/test/fiddle/test_func.rb:17 run> terminated with exception (report_on_exception is true):
Traceback (most recent call last):
1: from /Users/byroot/src/github.com/ruby/fiddle/test/fiddle/test_func.rb:19:in `block (2 levels) in test_syscall_with_tainted_string'
/Users/byroot/src/github.com/ruby/fiddle/test/fiddle/test_func.rb:19:in `call': tainted parameter not allowed (SecurityError)
[ 84/142] Fiddle::TestHandle#test_safe_function_lookup#<Thread:0x0000000132126bb8@/Users/byroot/src/github.com/ruby/fiddle/test/fiddle/test_handle.rb:20 run> terminated with exception (report_on_exception is true):
Traceback (most recent call last):
1: from /Users/byroot/src/github.com/ruby/fiddle/test/fiddle/test_handle.rb:23:in `block in test_safe_function_lookup'
/Users/byroot/src/github.com/ruby/fiddle/test/fiddle/test_handle.rb:23:in `[]': Insecure operation - [] (SecurityError)
[ 85/142] Fiddle::TestHandle#test_safe_handle_open#<Thread:0x000000013212ee80@/Users/byroot/src/github.com/ruby/fiddle/test/fiddle/test_handle.rb:12 run> terminated with exception (report_on_exception is true):
Traceback (most recent call last):
2: from /Users/byroot/src/github.com/ruby/fiddle/test/fiddle/test_handle.rb:14:in `block in test_safe_handle_open'
1: from /Users/byroot/src/github.com/ruby/fiddle/test/fiddle/test_handle.rb:14:in `new'
/Users/byroot/src/github.com/ruby/fiddle/test/fiddle/test_handle.rb:14:in `initialize': Insecure operation - initialize (SecurityError)
Finished tests in 0.766470s, 185.2649 tests/s, 404.4516 assertions/s.
142 tests, 310 assertions, 0 failures, 0 errors, 3 skips
Mmh, so besides these $SAFE
-related errors it seems to work fine.
- Checking out
ruby/fiddle
v1.0.0
, since AFAICT it's the version included with2.5.9
.
I think it's the typical issue of CRuby not increasing the version number while changing the Fiddle in-tree, so that might not be the same version, and explain those $SAFE
-related errors.
@mislav & @hsbt: Any preference between the possibilities in https://github.com/rbenv/ruby-build/issues/2022#issuecomment-1208387615?
I feel the patch is the safest/most reliable, but a bit hesitant as I don't think ruby-build should maintain a collection of patches for trying to build old Rubies on new OS.
There is also of course:
-Werror=implicit-function-declaration
on macOS, which is an issue likely to happen with a broad range of software compiled from source on macOS (since that werror is enabled default (apparently for -Wall
) on recent macOS but not on Linux, IMHO weird/inconsistent for clang to do that, would be better if same default on Linux).Yeah, all rubies and even extensions from that era have problems with various new errors introduced by clang
in the last few years.
I totally understand that trying to fix this issue is a slippery slope for ruby-build, and might not be justified for EOL rubies, but I wonder if trying to disable most of the new default flags clang introduced in the last years for older rubies when compiling on MacOS could make sense as a fix that wouldn't require much maintenance.
@byroot Does Shopify still use Ruby 2.5? I'm negative to add the additional workaround to EOL Rubies. I'm afraid other people ask, "We still use Ruby 1.8/1.9(also other versions), Please add workaround". There is no reason to reject it if we add workaround for Ruby 2.5.
I want to add like RUBY_CFLAGS=-DUSE_FFI_CLOSURE_ALLOC
workaround to only wiki.
@hsbt, no our entire infra has been moved to 3.1, this is just me maintaining gems that are still compatible with Ruby 2.5.
I totally understand your stance on workarounds, I'll update the wiki when I'm back at work Monday.
Steps to reproduce the behavior
Expected vs. actual behavior
I expect ruby to compile.
However it fails to compile the
ffi
extensionLogs
2.5 being EOL, it's very unlikely this will ever be fixed upstream. So the question is wether
ruby-build
should flip that compilation flag so that 2.5 (and older probably?) can still be installed.cc @eregon