Fix regression when installing native extensions on universal rubies

[hanyang-tony]

Since #6945 the extension dir changed to Gem::BasicSpecification's implementation, we didn't hook that in rubygems_ext.rb. So for universal rubies, we ended up using the universal platform name when installing, but arch replaced platform name when checking. This lead to native extensions can never be correctly installed on universal rubies.

Hook Gem::BasicSpecifications so the behavior is consistent on installing and checking.

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

For universal rubies, the change from #6945 to def gem_build_complete_path is incorrect. Previously, extension_dir is invoked on the Bundler::StubSpecification, which is a child class of Specification. After the change to stub.gem_build_complete_path, @Stub is actually an instance of Gem::StubSpecification, thus gem_build_complete_path is from Gem::BasicSpecification.

The problem arise when in bundler, we replace the 'universal' part of Gem::Platform with the arch. But we still use the universal name for Gem::Specification, this is defined in 'rubygems_ext.rb'. Now the stub is using extension_dir defined in Gem::BasicSpecification, so the 'universal' part ended up being swapped by arch.

So, we install native extensions to 'extensions/universal-darwin-22/', but finding them in 'extensions/x86_64-darwin-22/'. Leading to native extension gems can never be installed on universal ruby.

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

Hook Gem::BasicSpecifications so the behavior is consistent on installing and checking.

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

[segiddins]

Looks good, mind adding a spec for the scenario that this addresses?

[hanyang-tony]

I realized we can avoid code duplication by changing class Specification to class BasicSpecification, since the former is a subclass of the latter. Also added a spec to test if StubSpecification's extensions_dir equals Specifications.

[hanyang-tony]

Looks good, mind adding a spec for the scenario that this addresses?

Done :)

[deivid-rodriguez]

@hanyang-tony I tried reverting the fix here, and the spec still passes. Can you have a look?

[hanyang-tony]

@hanyang-tony I tried reverting the fix here, and the spec still passes. Can you have a look?

Sorry, I was testing the wrong method. I've fixed it now. Due to the nature of how the Gem::StubSpecification::StubLine is loaded, I have to include a separate gemspec file instead of inline it in spec test. Please check it out.

[deivid-rodriguez]

Maybe I'm crazy but the new spec is still passing without this fix?

[hanyang-tony]

Maybe I'm crazy but the new spec is still passing without this fix?

This is a fix for 'universal ruby', Gem::Platform.local.cpu should be 'universal'. Other versions of ruby don't have this issue.

[deivid-rodriguez]

Ideally we'd find a way to test this in our CI, to avoid regressions, even if by mocking the platform somehow. Let's ship the bug fix though for now.

[hanyang-tony]

Ideally we'd find a way to test this in our CI, to avoid regressions, even if by mocking the platform somehow. Let's ship the bug fix though for now.

It's possible to hook Platform.local into universal ruby in this spec. Or add a workflow that hook it first then run the specs.

[hanyang-tony]

I've fixed the linting error, can we re-run the actions?

[hanyang-tony]

Can we re-run the failed test? Seems like it was not caused by the changes in this PR.

[deivid-rodriguez]

Ok, let's ship this as is.

It's possible to hook Platform.local into universal ruby in this spec. Or add a workflow that hook it first then run the specs.

Would happily accept such a test to cover this 👍.

[Bo98]

It's possible to hook Platform.local into universal ruby in this spec

You could take inspiration from 11229b1 which uses simulate_platform and simulate_ruby_platform.

[hanyang-tony]

It's possible to hook Platform.local into universal ruby in this spec

You could take inspiration from 11229b1 which uses simulate_platform and simulate_ruby_platform.

I tried to use simulate_ruby_platform, but it doesn't work in this case. Because the RUBY_PLATFORM isn't used directly by this spec, it's the hook in rubygems_ext.rb that make use of it. The hook only run once, before the spec is run, so change the platform in spec doesn't do anything.

[Bo98]

Yeah it's designed to work when running a Bundle process (install_gemfile) rather than direct method calls. So if the problem can be translated to a real world steps to reproduce then we could test that. Was the problem revealed from doing a bundle exec after an install? The existing tests only tested the install.

[hanyang-tony]

The regression caused bundler to search gem extension in a different directory than the one which it was installed. The existing tests could work if the tested gem has native extension.