Skip to content

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

@ianks ianks requested review from deivid-rodriguez, hsbt and simi April 25, 2024 03:00
@ianks ianks force-pushed the cargo-test-work-by-default branch from 5537262 to f483e2c Compare April 25, 2024 03:00
@hsbt hsbt force-pushed the cargo-test-work-by-default branch from f483e2c to b090e96 Compare May 9, 2024 08:19
@simi
Copy link
Contributor

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

<%- 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

[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
Contributor

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

@deivid-rodriguez
Copy link
Contributor

deivid-rodriguez commented Oct 31, 2024

@ianks Can you rebase this now that #7610 is merged?

@deivid-rodriguez
Copy link
Contributor

@ianks Are you still interested in moving this PR forward?

@ianks
Copy link
Collaborator Author

ianks commented Jan 30, 2025

bandwidth is running low atm, but i reached out in slack to some folks to see if anyone can take it over for me. let's see if anyone volunteers in the next couple of days?

@gjtorikian
Copy link
Contributor

I cherry picked these commits into #8455.

@deivid-rodriguez
Copy link
Contributor

Closing in favor of #8455.

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.

4 participants