Skip to content
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

Add gem feature to reduce Windows linking bugs #24

Closed
wants to merge 1 commit into from

Conversation

ianks
Copy link
Contributor

@ianks ianks commented Nov 3, 2022

~It turns out the linking behavior of gems is more nuanced than we previously accounted for in magnus. On MacOS and Linux, you do not want to link in libruby, but on Windows you do. The behavior is a tricky and is not well suited for imperative interface (i.e. features = ["link-ruby"]. In the future, there may even be more unknown edge cases that we need to account for, so knowing that the user whats to build a gem makes it possible for rb-sys to do what the user really wants.

To make cross-platform linking reliable, this PR brings in the gem feature from rb-sys so things Just Work™️ for gems. This should reduce a class of pesky, head-scatching linker issues for windows compilation.~

In oxidize-rb/rb-sys#94, I changed the strategy for how rb-sys detects if it should build for usage in a gem. It relies on the create_rust_makefile setting --cfg=rb_sys_gem in the RUSTFLAGS. This strategy makes it so magnus no longer needs to worry about anything when a Gem is being built, which should solve some integration headaches.

As such, we no longer need to conditionally do things like setting the ruby_abi_version, since that is now now handled automatically.

@gjtorikian
Copy link
Contributor

Thanks for this. Just for my education—why is this set as a feature and not the default? I didn't see any adverse effects in Linux and macOS builds with it toggled on, so I'm wondering what (if any) harm there is in just always linking.

@ianks
Copy link
Contributor Author

ianks commented Nov 4, 2022

PSA: The windows failures are unrelated, tracking them here: MSP-Greg/ruby-loco#8

@ianks
Copy link
Contributor Author

ianks commented Nov 4, 2022

Thanks for this. Just for my education—why is this set as a feature and not the default? I didn't see any adverse effects in Linux and macOS builds with it toggled on, so I'm wondering what (if any) harm there is in just always linking.

Good question! Actually, when writing a gem you do not want to link Ruby on unix systems. Symbols are dynamically looked up from the Ruby executable. This is how mkmf works. When --enable-shared is setup, libruby is a dylib and if you link against the correct dylib, things should be fine. If for some reason you link against the wrong libruby (perhaps system ruby), you are in for a world of hurt. Also, static linked libruby will just immediately segfault.

Another use case is embed, in which case your executable is a Rust program that loads libruby, and initializes the Ruby interpreter. In this case, you always need to link Ruby.

@ianks
Copy link
Contributor Author

ianks commented Nov 6, 2022

OK, so I had an idea to only infer the gem feature if --cfg=rb_sys_gem is set, which would allow things to work properly without having to specify anything. Going to give that a shot... oxidize-rb/rb-sys#94

@ianks ianks changed the title Add gem feature to reduce Windows linking bugs Remove gem awareness from magnus, rely on rb-sys Nov 7, 2022
Cargo.toml Show resolved Hide resolved
@ianks ianks changed the title Remove gem awareness from magnus, rely on rb-sys Rely on rb-sys for gem compilation awareness Nov 7, 2022
@ianks ianks changed the title Rely on rb-sys for gem compilation awareness Add gem feature to reduce Windows linking bugs Nov 7, 2022
@ianks
Copy link
Contributor Author

ianks commented Nov 7, 2022

closed in favor of #25

@ianks ianks closed this Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants