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

[rust gem] Major improvements for gem scaffolding #7608

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

ianks
Copy link
Collaborator

@ianks ianks commented Apr 25, 2024

  1. Currently, running bundle gem --ext=rust results in a project that doesn't support cargo test by default. This causes unnecessary toil for developers who expect cargo test to work out of the box. This PR fixes that by using rb-sys-env to configure the proper linker arguments for libruby.
  2. Support building precompiled gems out of the box, since it's a well-supported and recommended pattern to ease distribution burden. Users often stumble to set this up, so let's do it for them

@hsbt hsbt force-pushed the cargo-test-work-by-default branch from f483e2c to b090e96 Compare May 9, 2024 08:19
@simi
Copy link
Member

simi commented May 31, 2024

@ianks this looks amazing. I did some local testing and I have faced following problem.

$ ~/code/work/oss/rubygems/bundler/exe/bundle gem --ext=rust ryba
...
$ cd ryba
$ cargo test
...
   Compiling rb-sys-build v0.9.97
   Compiling rb-sys v0.9.97
    Finished test [unoptimized + debuginfo] target(s) in 27.65s
     Running unittests src/lib.rs (target/debug/deps/ryba-715184ccf66f2799)

running 1 test
/tmp/ryba/target/debug/deps/ryba-715184ccf66f2799: symbol lookup error: /home/retro/.rubies/ruby-3.3.1/lib/ruby/3.3.0/x86_64-linux/enc/encdb.so: undefined symbol: rb_encdb_declare
error: test failed, to rerun pass `--lib`

Caused by:
  process didn't exit successfully: `/tmp/ryba/target/debug/deps/ryba-715184ccf66f2799` (exit status: 127)
$ cargo --version
cargo 1.70.0 (ec8a8a0ca 2023-04-25)
$ rustc --version
rustc 1.70.0 (90c541806 2023-05-31)

Is that anything related to my setup? 🤔 Similar error seems to fail CI (https://github.com/rubygems/rubygems/actions/runs/9014478174/job/24767239926?pr=7608).

@ianks
Copy link
Collaborator Author

ianks commented May 31, 2024

hmm that's strange @simi, are you on linux?

# Fall back to loading the non-versioned extension if version-specific loading fails.
begin
RUBY_VERSION =~ /(\d+\.\d+)/
require "#{Regexp.last_match(1)}/<%= File.basename(config[:namespaced_path]) %>/<%= config[:underscored_name] %>"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something users frequently stumble on when trying to precompile gems, so let's just use the recommended pattern by rake-compiler as a default: https://github.com/rake-compiler/rake-compiler?tab=readme-ov-file#cross-compilation---the-future-is-now

@@ -41,7 +37,7 @@ Gem::Specification.new do |spec|
spec.extensions = ["ext/<%= config[:underscored_name] %>/extconf.rb"]
<%- end -%>
<%- if config[:ext] == 'rust' -%>
spec.extensions = ["ext/<%= config[:underscored_name] %>/Cargo.toml"]
spec.add_dependency "rb_sys", "~> 0.9"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CargoBuilder was a good first start, but it's much less flexible and not the best default IMO. rb-sys + create_rust_makefile is much more robust these days, so let's use it

@@ -12,4 +12,11 @@ publish = false
crate-type = ["cdylib"]

[dependencies]
magnus = { version = "0.6.2" }
magnus = { version = "0.6.3" }
rb-sys = { version = "0.9", features = ["stable-api-compiled-fallback"] }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this, we support ruby-head out of the box. This allows users to run development/unstable rubies in production without friction

@ianks ianks force-pushed the cargo-test-work-by-default branch from 8c4d73d to 37a8c8b Compare May 31, 2024 03:24
@ianks ianks force-pushed the cargo-test-work-by-default branch from 37a8c8b to ac358f4 Compare May 31, 2024 03:24
@ianks ianks changed the title [rust gem] Make cargo test work by default [rust gem] Major improvements for gem scaffolding May 31, 2024
@ianks ianks force-pushed the cargo-test-work-by-default branch from d6d7443 to bea50ba Compare May 31, 2024 03:30
@simi
Copy link
Member

simi commented May 31, 2024

hmm that's strange @simi, are you on linux?

Yes!

$ uname -a
Linux retro 6.8.10-300.fc40.x86_64 #1 SMP PREEMPT_DYNAMIC Fri May 17 21:20:54 UTC 2024 x86_64 GNU/Linux

@ianks
Copy link
Collaborator Author

ianks commented May 31, 2024

Confirmed that cross-compilation works out of the box: https://github.com/ianks/testingbun/actions/runs/9312563096

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.

None yet

2 participants