Skip to content

Raise if ruby platform is forced and there are no ruby variants#5495

Merged
deivid-rodriguez merged 6 commits intomasterfrom
raise-if-ruby-platform-forced-and-no-ruby-variants
Jun 28, 2022
Merged

Raise if ruby platform is forced and there are no ruby variants#5495
deivid-rodriguez merged 6 commits intomasterfrom
raise-if-ruby-platform-forced-and-no-ruby-variants

Conversation

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

@deivid-rodriguez deivid-rodriguez commented Apr 25, 2022

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

We have been reported a case where Bundler generates a lockfile when using force_ruby_platform that then makes Bundler crash when run on a different platform.

The case was reported by @stex here: #5422 (comment).

When using force_ruby_platform on a Gemfile like this:

source "https://rubygems.org"

gem "sorbet", "0.5.9889"

the following Gemfile.lockfile is generated


GEM
  remote: https://rubygems.org/
  specs:
    sorbet (0.5.9889)
      sorbet-static (= 0.5.9889)
    sorbet-static (0.5.9889-universal-darwin-14)
    sorbet-static (0.5.9889-universal-darwin-15)
    sorbet-static (0.5.9889-universal-darwin-16)
    sorbet-static (0.5.9889-universal-darwin-17)
    sorbet-static (0.5.9889-universal-darwin-18)
    sorbet-static (0.5.9889-universal-darwin-19)
    sorbet-static (0.5.9889-universal-darwin-20)
    sorbet-static (0.5.9889-universal-darwin-21)
    sorbet-static (0.5.9889-x86_64-linux)

PLATFORMS
  ruby

DEPENDENCIES
  sorbet (= 0.5.9889)

BUNDLED WITH
   2.3.12

And using that lockfile under a M1 docker container (which has platform "aarch64-linux") leads to a crash because no compatible gem can be found for that arch.

Closes #5422.

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

We should give a better error message when dealing with this Gemfile.lock file anyways, but in my opinion, the above lockfile should not be generated at all because the user is explicitly saying "I don't want platform specific gems" by specifying force_ruby_platform, and sorbet-static can't satisfy that.

I prepared a patch to implement the above and was really happy because it seemed to work well and it led to removing this line:

https://github.com/rubygems/rubygems/blob/2faada63ae65d8559bcba9e083f3c35ba6c18fdc/bundler/lib/bundler/match_platform.rb#L18

Which was the specific line giving @lloeki issues when trying to add non gnu libc support at #4488, so this could also unblock that.

However... It does not work on truffleruby because... truffleruby needs its own special behaviour that it's hard to support. In particular, truffleruby declares its own platform as ruby (like force_ruby_platform does) because it's ABI incompatible with CRuby, so it doesn't want platform specific gems installed. However, it also keeps a hardcoded list of gem names where platform specific gems apparently work, and it's supposed to actually install platform specific gems in those cases.

I will investigate how to keep truffleruby working fine and still be able to introduce this, but I'm opening a WIP PR for now because I'm not sure when I'll get to it.

Make sure the following tasks are checked

@lloeki
Copy link
Copy Markdown
Contributor

lloeki commented Apr 26, 2022

However... It does not work on truffleruby because... truffleruby needs its own special behaviour that it's hard to support. In particular, truffleruby declares its own platform as ruby (like force_ruby_platform does) because it's ABI incompatible with CRuby, so it doesn't want platform specific gems installed. However, it also keeps a hardcoded list of gem names where platform specific gems apparently work, and it's supposed to actually install platform specific gems in those cases.

Oh my. Makes me think of Alpine-packaged Ruby in some ways.

@deivid-rodriguez deivid-rodriguez force-pushed the raise-if-ruby-platform-forced-and-no-ruby-variants branch 3 times, most recently from 4d506f7 to 6fa1d4e Compare June 26, 2022 03:48
@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

Alright, I managed to figure this out, so I'll set this as ready.

In the end, it was not a truffleruby specific issue 😳. It was just that a spec only run on truffleruby was the one that caught the problem, but the issue also happened when running the same test case on other rubies.

I did have a lot of trouble with truffleruby because my fixes was just not being picked up there, until I found that it monkeypatches Bundler in a very confusing way.

@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review June 26, 2022 09:32
@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

This might fully close #5422, it does fix the only reproducible case reported there. I'll set this PR to close that ticket and if still reproducible after upgrading, people can reopen it.

@deivid-rodriguez deivid-rodriguez force-pushed the raise-if-ruby-platform-forced-and-no-ruby-variants branch from 6fa1d4e to 12d9cca Compare June 27, 2022 10:59
The test repo 4 only includes gems defined by the block, which is enough
for this kind of spec.
Since they don't need to install any gems.
Platforms should be handled (and mostly were handled) at a higher level
in the resolution process.
It's more correct, since an array of specs is return, but also
workarounds an issue in truffleruby, which will forcefully patch all
Bundler versions including this method in order to cherry-pick a bug
fix rather than patching only the versions where the bug is present.
That means that truffleruby will ignore any other bug fixes /
improvements this method receives in the future. An easy escape to this
is to rename the method.
The `sorbet-static` gem is an example of these gems. We may have a
lockfile locked only to RUBY, but with only platform specific variants
of sorbet-static included.

This commit makes sure we no longer generate these lockfiles and print
an error instead.
@deivid-rodriguez deivid-rodriguez force-pushed the raise-if-ruby-platform-forced-and-no-ruby-variants branch from 12d9cca to e17d7e9 Compare June 27, 2022 17:46
@deivid-rodriguez deivid-rodriguez merged commit f1c346d into master Jun 28, 2022
@deivid-rodriguez deivid-rodriguez deleted the raise-if-ruby-platform-forced-and-no-ruby-variants branch June 28, 2022 18:26
deivid-rodriguez added a commit that referenced this pull request Jun 29, 2022
…and-no-ruby-variants

Raise if ruby platform is forced and there are no ruby variants

(cherry picked from commit f1c346d)
@eregon
Copy link
Copy Markdown
Member

eregon commented Jul 6, 2022

I did have a lot of trouble with truffleruby because my fixes was just not being picked up there, until I found that it monkeypatches Bundler in a very confusing way.

I am sorry about that. The reason for that way is to patch older Bundler versions (which need that fix), we should only do something if Bundler is older than when that fix was added.

@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

Yeah, I came to the same conclusion in a1843d6 commit's message.

I wanted to provide a fix but you know, too many things, and renaming the method on our side made sense anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NoMethodError: undefined method `full_name' for nil:NilClass bug that should have been fixed

3 participants