Include directory in CargoBuilder install path

[matsadler]

What was the end-user or developer problem that led to this PR?

This is an attempt to address issue #6204 "bundle gem --ext=rust generates gem that fails to load when installed".

The CargoBuilder is installing Rust extensions in lib/gemname.dlext, but the Rust extension template is set up expecting the extension to be installed in lib/gemname/gemname.dlext.

I don't think a concrete decision was made in that ticket, but my reading was that lib/gemname/gemname.dlext is the desired destination, and the bug is in CargoBuilder, not the Rust extension template.

Furthermore, it seems the consensus was that while this would be a breaking change to CargoBuilder, it has not ever been announced as stable, so it's fine to make the change.

What is your fix for the problem, implemented in this PR?

Currently CargoBuilder installs extensions to lib/gemname.dlext, these changes update that to lib/dirname/cratename.dlext.

dirname is sourced from the extension path, dropping the first and last parts, for example "ext/foo/bar/Cargo.toml" becoming "foo/bar".

cratename is sourced by calling out to the cargo metadata command.

Using dirname as the first part should better match the directory structure and is analogous to how C extensions are built. Users have control over this as they can nest directories however they want.

Using cratename instead of gemname makes CargoBuilder more reliable, as it better detects the location of the extension once cargo has compiled it to move to the final install location. This also provides a benefit to users as they have more control over the crate name, and can choose to give it a different name to the gem if they wish. And it allows for multiple Rust extensions in a single gem, which might not be a common use case, but it matches what can be done with C extensions.
There is also benefit to users of Magnus (as used by the Rust extension template) as the autogenerated Init function generated by Magnus will match the crate name, not the gem name, potentially saving confusing debugging.

As I was making these changes I wanted to be able to only call #cargo_crate_name once, to avoid multiple calls out to cargo. To do this I refactored #build, inlining a number of helper methods only used a single time. Hopefully this makes the flow of #build easier to understand.

I also updated the gem spec validation to allow Cargo.lock to be anywhere in the gem, as requiring it to be in the root seemed to be a frustrating limitation.

I updated the CargoBuilder tests, and reused the custom_name test extending it to cover the new behaviour. As I was updating the tests I found two files in the fixtures that seemed to be unused, so removed them.

Make sure the following tasks are checked

• Describe the problem / feature
• Write tests for features and bug fixes
• Write code to solve the problem
• Make sure you follow the current code style and write meaningful commit messages without tags

[welcome]

Thanks for opening a pull request and helping make RubyGems and Bundler better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use GitHub Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of GitHub Actions in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems or #bundler channel on Slack.

For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide

[simi]

Thanks for the detailed info. If I understand it well, lib/gemname/gemname.dlext is the same path as for C extensions. I think that's the way to go for now.

[simi]

Windows CI seems unhappy. Any ideas how to fix that? Anything I can help with (I have access to Windows machine if needed)?

[matsadler]

So part of this change is calling out to cargo metadata to get the crate name. It's possible there's more than one crate, so that returns the data like:

{
  "packages": [
    {"name": "foo", "manifest_path": "path/to/foo/Cargo.toml", ...},
    {"name": "bar", "manifest_path": "path/to/bar/Cargo.toml", ...},
  ]
}

We know the path to the crate we're trying to build, as it's passed as the first argument to build, so my code matches them up and gets the name that way.

The error on Windows is happening right around this bit of code, and my guess is that Ruby and cargo aren't agreeing on the exact format of the paths on Windows, so they aren't matching.

I've added a better error message in this case, which should hopefully confirm that's the issue. If it is it's probably just a case of figuring out how to normalise the paths.

[simi]

@matsadler feel free to check CI now

[simi]

TL;DR

  looking for: D:/a/rubygems/rubygems/tmp/test_rubygems_20230127-4224-1i30256/ext/Cargo.toml
...
  found:
  rust_ruby_example at D:\a\rubygems\rubygems\tmp\test_rubygems_20230127-4224-1i30256\ext\Cargo.toml

full error

Error: test_build_cdylib(TestGemExtCargoBuilder):
  Gem::InstallError: failed to determine cargo package name
  
  looking for: D:/a/rubygems/rubygems/tmp/test_rubygems_20230127-4224-1i30256/ext/Cargo.toml
  
  found:
  rust_ruby_example at D:\a\rubygems\rubygems\tmp\test_rubygems_[202](https://github.com/rubygems/rubygems/actions/runs/4021593599/jobs/6918549856#step:8:203)30127-4224-1i30256\ext\Cargo.toml
D:/a/rubygems/rubygems/lib/rubygems/ext/cargo_builder.rb:229:in `cargo_crate_name'
D:/a/rubygems/rubygems/lib/rubygems/ext/cargo_builder.rb:24:in `build'
D:/a/rubygems/rubygems/test/rubygems/test_gem_ext_cargo_builder.rb:35:in `block in test_build_cdylib'
     32:     Dir.chdir @ext do
     33:       ENV.update(@rust_envs)
     34:       builder = Gem::Ext::CargoBuilder.new
  => 35:       builder.build "Cargo.toml", @dest_path, output
     36:     end
     37: 
     38:     output = output.join "\n"
D:/a/rubygems/rubygems/test/rubygems/test_gem_ext_cargo_builder.rb:32:in `chdir'
D:/a/rubygems/rubygems/test/rubygems/test_gem_ext_cargo_builder.rb:32:in `test_build_cdylib'

[simi]

So it seems (thanks to failing specs message now) this produces wrong path on Windows.

D:/a/rubygems/rubygems/tmp/test_rubygems_20230127-4224-1i30256/ext/Cargo.toml

Since actually proper one is present int cargo metadata output.

D:\a\rubygems\rubygems\tmp\test_rubygems_20230127-4224-1i30256\ext\Cargo.toml

This seems to be known Ruby limitation. I'll check on Windows machine if there's any way to produce proper Windows path in here. By looking at the other code, this could work (not tested).

cargo_toml = cargo_toml.tr(File::ALT_SEPARATOR, File::SEPARATOR) if File::ALT_SEPARATOR

[matsadler]

I think that tr(ALT_SEPARATOR ... trick should work. I've pushed a changed using that approach.

[simi]

@matsadler seems ok. Is this ready to merge for now?

[matsadler]

Yep, if there’s no more comments then I’m happy for this to be merged.