-
Notifications
You must be signed in to change notification settings - Fork 22
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
Fix broken compilation with Ruby interpreter compiled with XCode 14 #114
Conversation
13dd567
to
18b57e0
Compare
Another option would be to use % gcc -dynamic -bundle -o encoder.bundle encoder.o -L. -L/private/tmp/ruby/lib -L/private/tmp/ruby/lib/ruby/gems/2.7.0/gems/libyajl2-2.1.0/lib/libyajl2/vendored-libyajl2/lib -L. -fstack-protector-strong -Wl,-multiply_defined,suppress -lruby.2.7 -m64 -lruby.2.7
Undefined symbols for architecture arm64:
"_yajl_gen_alloc", referenced from:
_mEncoder_do_yajl_encode in encoder.o
"_yajl_gen_array_close", referenced from:
_gen_array_close in encoder.o
"_yajl_gen_array_open", referenced from:
_gen_array_open in encoder.o
"_yajl_gen_bool", referenced from:
_gen_true in encoder.o
_gen_false in encoder.o
"_yajl_gen_config", referenced from:
_mEncoder_do_yajl_encode in encoder.o
"_yajl_gen_free", referenced from:
_mEncoder_do_yajl_encode in encoder.o
"_yajl_gen_get_buf", referenced from:
_mEncoder_do_yajl_encode in encoder.o
"_yajl_gen_map_close", referenced from:
_gen_map_close in encoder.o
"_yajl_gen_map_open", referenced from:
_gen_map_open in encoder.o
"_yajl_gen_null", referenced from:
_gen_null in encoder.o
"_yajl_gen_number", referenced from:
_gen_number in encoder.o
"_yajl_gen_string", referenced from:
_gen_cstring in encoder.o
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation) % gcc -dynamic -bundle -o encoder.bundle encoder.o -L. -L/private/tmp/ruby/lib -L/private/tmp/ruby/lib/ruby/gems/2.7.0/gems/libyajl2-2.1.0/lib/libyajl2/vendored-libyajl2/lib -L. -fstack-protector-strong -Wl,-multiply_defined,suppress -lruby.2.7 -m64 -lruby.2.7 -bundle_loader /tmp/ruby/lib/ruby/gems/2.7.0/gems/libyajl2-1.2.0/lib/libyajl2/vendored-libyajl2/lib/libyajl.bundle
% |
@stanhu Yes, adding |
ext/ffi_yajl/ext/encoder/extconf.rb
Outdated
# a result Ruby interpreters compiled under XCode 14 no longer specify | ||
# this flag by default in DLDFLAGS. Let's specify the list of dynamic symbols | ||
# here to avoid compilation failures. | ||
if macos? |
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.
We can probably just look at whether LDFLAGS
has -Wl,-undefined dynamic_lookup
, or just do this all the time.
72d64b8
to
bf55e49
Compare
can this be merged, please, and released to RubyGems? 🥺 it works for me on MacOS using Apple M1 with ARM64 architecture 🥳 |
XCode 14 warns if the `-undefined dynamic_lookup` linker option is used. As a result, the `configure` script for the Ruby interpreter disables these flags, causing building the native gem to fail with undefined symbols: ``` compiling encoder.c encoder.c:307:14: warning: unused function 'rb_cBignum_ffi_yajl' [-Wunused-function] static VALUE rb_cBignum_ffi_yajl(VALUE self, VALUE rb_yajl_gen, VALUE state) { ^ 1 warning generated. linking shared-object ffi_yajl/ext/encoder.bundle Undefined symbols for architecture arm64: "_yajl_gen_alloc", referenced from: _mEncoder_do_yajl_encode in encoder.o "_yajl_gen_array_close", referenced from: _gen_array_close in encoder.o "_yajl_gen_array_open", referenced from: _gen_array_open in encoder.o "_yajl_gen_bool", referenced from: _gen_true in encoder.o _gen_false in encoder.o "_yajl_gen_config", referenced from: _mEncoder_do_yajl_encode in encoder.o "_yajl_gen_free", referenced from: _mEncoder_do_yajl_encode in encoder.o "_yajl_gen_get_buf", referenced from: _mEncoder_do_yajl_encode in encoder.o "_yajl_gen_map_close", referenced from: _gen_map_close in encoder.o "_yajl_gen_map_open", referenced from: _gen_map_open in encoder.o "_yajl_gen_null", referenced from: _gen_null in encoder.o "_yajl_gen_number", referenced from: _gen_number in encoder.o "_yajl_gen_string", referenced from: _gen_cstring in encoder.o ld: symbol(s) not found for architecture arm64 clang: error: linker command failed with exit code 1 (use -v to see invocation) make: *** [encoder.bundle] Error 1 ``` To fix this, we can explicitly list the symbols that will be dynamically looked up and use that in the `-U` linker option to specify that it is okay for that symbol to have no definition. Relates to ruby/ruby#6193 Signed-off-by: Stan Hu <stanhu@gmail.com>
bf55e49
to
966eee2
Compare
Kudos, SonarCloud Quality Gate passed!
|
Note that Ruby 2.7.7 and 3.0.5 have shipped a fix for https://bugs.ruby-lang.org/issues/19005, so this pull request isn't needed for those versions. |
Thank you for sharing.
I tried installing on Ruby 3.1.3 and got an error. However, I was able to install successfully with the following settings:
Hasn't the fix for this issue been backported to Ruby 3.1.3?
|
Yes, I've confirmed that Ruby 3.1.3 appears to have the fix (ruby/ruby@bf92aac), but for some reason I'm not seeing the irb(main):001:0> RUBY_VERSION
=> "3.1.3"
irb(main):002:0> RbConfig::CONFIG['ADDITIONAL_DLDFLAGS']
=> "" Whereas 3.0.5 has this: irb(main):001:0> RUBY_VERSION
=> "3.0.5"
irb(main):004:0> RbConfig::CONFIG['ADDITIONAL_DLDFLAGS']
=> "-Wl,-undefined,dynamic_lookup" |
Actually, I'm seeing the problem show up in Ruby 3.2.0 as well. |
Confirmed there is a problem with Ruby 3.1.3 and up: https://bugs.ruby-lang.org/issues/19005#note-25 UPDATE: This bug has been reported in https://bugs.ruby-lang.org/issues/19082. |
@tpowell-progress Is there anything I can do to get this merged? Our team is hitting this failure now that we've updated to Ruby 3.1.4. |
Sorry, the supporting projects fly below my radar most of the time. However, I just ran into this problem as well on ARM64 as well. I need to see what the release process is for this, but adding it to the to do list. |
Ok, I'm merging but also bumping to 2.5.0. Have an ARM64 build for chef that will need to include and also need to bump chef-powershell so as to not cause a version conflict for the next Chef 18 release. |
FYI, this is breaking in the Windows pipelines publicly accessible example, so I won't be able to merge this in until I get to the bottom of that issue. Difficulty: It works on my VMs (but fails on multiple CI setups) |
@tpowell-progress To be clear, the Windows error seems unrelated to this pull request? I'm not quite sure where
|
@stanhu Yes, I've even compared the .gem files without finding anything (such as vendored artifacts created by an intermediate step) that would contribute to the issue. I'm going to create a clean branch off |
I've had to yank 2.5 due to a few pins being on |
@tpowell-progress Unfortunately we had already updated our project to use v2.5.0, so the yank caused our builds to fail. |
I should note on Windows Server 2022, this gem installed and built fine on Ruby 3.1.3:
Unfortunately, yanking the gem also makes it harder to replicate the issue. |
Working on a 3.0.0 build due to it being a "breaking" change in certain cases. |
@stanhu Pushed a 3.0.0 version of the gem so that all the |
Unfortunately the only reason we depend on I've reproduced the problem by installing Chef and running Chef's version of
I think C:\opscode\chef\embedded\lib\ruby\gems\3.1.0\gems\libyajl2-2.1.0\lib\libyajl2\vendored-libyajl2\lib>dir
Volume in drive C has no label.
Volume Serial Number is 3E45-2818
Directory of C:\opscode\chef\embedded\lib\ruby\gems\3.1.0\gems\libyajl2-2.1.0\lib\libyajl2\vendored-libyajl2\lib
06/02/2023 03:14 PM <DIR> .
06/02/2023 03:08 PM <DIR> ..
06/02/2023 03:08 PM 1,345 libyajl.def
06/02/2023 03:08 PM 147,441 libyajl.so
2 File(s) 148,786 bytes
2 Dir(s) 21,475,872,768 bytes free For reference, my local Ruby install contains this in C:\>dir C:\Ruby31-x64\lib\ruby\gems\3.1.0\gems\libyajl2-2.1.0\lib\libyajl2\vendored-libyajl2\lib
Volume in drive C has no label.
Volume Serial Number is 3E45-2818
Directory of C:\Ruby31-x64\lib\ruby\gems\3.1.0\gems\libyajl2-2.1.0\lib\libyajl2\vendored-libyajl2\lib
06/12/2023 07:21 PM <DIR> .
06/12/2023 07:21 PM <DIR> ..
06/12/2023 07:21 PM 1,345 libyajl.def
06/12/2023 07:21 PM 81,408 libyajl.so
06/12/2023 07:21 PM 34,398 libyajldll.a
3 File(s) 117,151 bytes
2 Dir(s) 21,474,656,256 bytes free Since It seems to me there are two ways to solve this:
|
@tpowell-progress Given that CI appears to be trying to update from an existing Chef Omnibus install, it does seem to me that shipping I did a quick scan of the build rules in https://github.com/chef/chef/tree/main/omnibus. I didn't see where
I don't suppose you know why that file might have been dropped? I'm launching a local build to see if I can replicate this. |
Ok, I think https://github.com/chef/omnibus-software/blob/e43f50af2e03dc073174f8de819d08432b9b22b7/config/software/ruby-cleanup.rb#L40-L53 is removing all Perhaps there should be an exception for Windows? UPDATE: I see this is needed to clean up some cruft, like |
By default, omnibus-software will clean up all *.a files in the Ruby gem path (https://github.com/chef/omnibus-software/blob/e43f50af2e03dc073174f8de819d08432b9b22b7/config/software/ruby-cleanup.rb#L40-L53). By convention, Windows implib files should have a .lib extension (https://learn.microsoft.com/en-us/cpp/build/reference/implib-name-import-library?view=msvc-170), so this change makes it possible to preserve the required library. Relates to chef/ffi-yajl#114
Ok, it seems that the Windows implib should have a C:\opscode\chef\embedded\lib\ruby\gems\3.1.0\gems\libyajl2-2.1.0\lib\libyajl2\vendored-libyajl2\lib>copy c:\opscode\libyajldll.a libyajldll.lib
1 file(s) copied.
C:\opscode\chef\embedded\lib\ruby\gems\3.1.0\gems\libyajl2-2.1.0\lib\libyajl2\vendored-libyajl2\lib>c:\opscode\chef\embedded\bin\gem install c:\opscode\ffi-yajl-2.5.0.gem
Temporarily enhancing PATH for MSYS/MINGW...
Building native extensions. This could take a while...
Successfully installed ffi-yajl-2.5.0
Parsing documentation for ffi-yajl-2.5.0
Installing ri documentation for ffi-yajl-2.5.0
Done installing documentation for ffi-yajl after 1 seconds
1 gem installed chef/libyajl2-gem#28 would fix the extension. |
By default, omnibus-software will clean up all *.a files in the Ruby gem path (https://github.com/chef/omnibus-software/blob/e43f50af2e03dc073174f8de819d08432b9b22b7/config/software/ruby-cleanup.rb#L40-L53). By convention, Windows implib files should have a .lib extension (https://learn.microsoft.com/en-us/cpp/build/reference/implib-name-import-library?view=msvc-170), so this change makes it possible to preserve the required library. Relates to chef/ffi-yajl#114 Signed-off-by: Stan Hu <stanhu@gmail.com>
By default, omnibus-software will clean up all *.a files in the Ruby gem path (https://github.com/chef/omnibus-software/blob/e43f50af2e03dc073174f8de819d08432b9b22b7/config/software/ruby-cleanup.rb#L40-L53). By convention, Windows implib files should have a .lib extension (https://learn.microsoft.com/en-us/cpp/build/reference/implib-name-import-library?view=msvc-170), so this change makes it possible to preserve the required library. Relates to chef/ffi-yajl#114 Signed-off-by: Stan Hu <stanhu@gmail.com>
I've added an internal issue (CHEF-4109) for this as will break internal development as well for ARM Macs. |
XCode 14 warns if the
-undefined dynamic_lookup
linker option is used. As a result, theconfigure
script for the Ruby interpreter disables these flags, causing building the native gem to fail with undefined symbols:To fix this, we can explicitly list the symbols that will be dynamically looked up and use that in the
-U
linker option to specify that it is okay for that symbol to have no definition.Relates to ruby/ruby#6193
Description
[Please describe what this change achieves]
Issues Resolved
[List any existing issues this PR resolves, or any Discourse or
StackOverflow discussions that are relevant]
Check List