Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Add two specs for choosing platform specific gems #6247

Closed
wants to merge 2 commits into from

Conversation

larskanis
Copy link
Contributor

This is a follow-up to #5337. It adds two specs for choosing platform specific gems. The first spec succeeds, but the second spec fails currently.

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

Bundler doesn't properly resolve platform gems with ruby version constraints.

rake-compiler tags all fat binary gems with appropriate required_ruby_version constraints. The intention behind this tag is to give bundler a chance to select the most suitable gem for the given platform and ruby version. Unfortunately it doesn't work.

This leads to issues for all Windows users whenever a new Ruby version comes out. The intended effect (to make the install of gems easy) turns to the opposite, because the platform specific gem effectively blocks the installation of the "ruby" platform gem.

These issues can be seen at many popular gems with C extensions: ffi #598, nokogiri #1706, pg

What was your diagnosis of the problem?

The first test case doesn't offer a x64-mingw32 gem in the latest version. Bundler therefore selects the latest "ruby" platform gem so that the installation succeeds. This is OK and expected.

The second test is very similar, but offers a x64-mingw32 gem in addition. This additional gem however is limited to ruby versions not matching the running ruby version. Bundler should exclude this gem therefore and should select the "ruby" platform gem as in the first test. However it doesn't resolve to the "ruby" platform gem, but to an older x64-mingw32 gem, which wasn't tagged by a required_ruby_version constraint to that point in time (and will not work with the latest ruby version in practice). That is the current situation with ffi and nokogiri.

The current workaround is to set force_ruby_platform to true, but this has several negative consequences since it's a global and not a per-gem setting. Also it has to be enabled manuelly by each (Windows) user. Insofar it's not a general solution.

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

Why did you choose this fix out of the possible options?

I didn't dig deeper into bundlers internals, but my hope is that someone with more knowledge of bundler can solve this issue that way or at least give me a pointer where I could start to fix this.

The first spec succeeds, but the second spec fails currently.
@ghost
Copy link

ghost commented Jan 6, 2018

Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler 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 Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI 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 #bundler channel on Slack.

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

@indirect
Copy link
Member

indirect commented Jan 7, 2018

According to the scenario you just described, it sounds like Bundler is correctly choosing a gem that declares itself to work in the current situation.

If a gem itself is broken, Bundler resolving to that gem is not a bug in Bundler, it is a bug in that gem. Can you explain more about why you think this is a bug in Bundler?

@larskanis
Copy link
Contributor Author

larskanis commented Jan 7, 2018

See the following table. "X" marks which of the given 4 gems are available on the index. "=>" marks, which gem is chosen in which situation.

1) Ruby-2.3 or 2.5

2) Ruby-2.5

3) Ruby-2.5

4) Ruby-2.3

5) Ruby-2.3 or 2.5

foo-1.0.0

X

X

X

X

X

foo-1.0.0-x64-mingw32

X

X <= current

 

X

 

foo-1.1.0

X <= OK

X <= expected

X <= expected

X

X <= OK

foo-1.1.0-x64-mingw32

only Ruby < 2.5

 

X

X

X <= OK

 
   

=> resolver fails currently

  

Case 1) corresponds to the first and 2) to the second spec in the attached PR. Cases 1), 4) and 5) succeed and the result is as expected.

In case 2) bunder finds a valid solution, but the solution is bad for two reasons: 1. it installs a (possibly very) old gem, which didn't receive any security or feature updates. And 2. it is an inconsistent solution, because the result of the resolver is different to 1) although the list of suitable gems is exactly the same as in 1).

To avoid situation 2) it would be an option to yank all the old x64-mingw32 gems, which are not tagged with require_ruby_version, but this leads directly to case 3): bundler completely fails to resolve.

In case 3) bundler doesn't find a solution due to the availability of a x64-mingw32 gem in the latest version 1.1.0. This is although the list of suitable gems is exactly the same as in case 5). It should therefore pick "foo-1.1.0" instead of giving up.

For some reasons bundler doesn't always resolve like in case 2), but also like in case 3). I guess this is due to some optimization while downloading the gem metadata. Therefore the linked issues describe case 3) although the situation is actually case 2): ffi #598, nokogiri #1706

@indirect
Copy link
Member

indirect commented Jan 8, 2018

@larskanis thank you for that extremely detailed explanation!

It sounds like you are describing two completely different (although possibly related) problems:

Problem 1: When specific-platform gems are disallowed by required_ruby_version, Bundler fails the resolution when you would instead expect it to fall back to a pure-ruby version of that gem.

Problem 2: Rather than choose the valid option of a newer pure-ruby gem, Bundler chooses the equally valid option of an older platform-specific gem.

If I am understanding correctly, problem 1 sounds like a bug that we need to investigate and fix. Once that bug is fixed, if problem 2 is still present it will likely need to be fixed by changes to users' Gemfiles. Getting a different valid resolution than the one you want means you need to inform Bundler about what it is that you want, so that Bundler can deliver it.

Thank you again for your detailed report and explanation!

@segiddins
Copy link
Member

segiddins commented Jan 13, 2018

I think (2) is reasonable -- imho, a closer version should be preferred to a closer platform match.

Thinking about this further, I can't really say preferring one is more reasonable than the other, and given changing the behavior doesn't look easy, I'm 1/2 tempted to say that bit is actually OK?

Added on another commit to test (1) directly, though

@larskanis
Copy link
Contributor Author

@segiddins Thank you for looking at this issue! Your spec shows another interesting failure.

However I imploringly hope this issue can be fixed! RubyInstaller-2.5.0 was downloaded almost 50000 times within the last two weeks, although I explicitly discourage user to use 2.5. Now almost every user stumbles upon this issue. And I get daily "same here" reports about these failures.

The only way to work around it, is to use force_ruby_platform, or go without bundler and just use rubygems with --platform ruby option. Although force_ruby_platform is more usable since RubyInstaller-2.4, thanks to the MSYS2 foundation, it's still far from "just works".

@segiddins
Copy link
Member

I imploringly hope this issue can be fixed

I spent a fair bit of time looking into it, and I don't see any super-simple fix. I can't promise when I'll be able to get back around to this particular issue, but contributions towards fixing it would be greatly appreciated!

@btir
Copy link

btir commented Dec 10, 2019

Any updates here? The problem remains in place after 2 years!
@larskanis @segiddins @indirect

kou added a commit to kou/bundler that referenced this pull request Dec 29, 2019
It resolves rubygems#6247. This changes includes the patches that add (RSpec)
specs for this situation in rubygems#6247.

If there is a platform specific gem but it can't be resolved available
version, Bundler reports an error.

For example,

    @Index = build_index do
      gem "bar", "1.0.0"
      gem "foo", "1.0.0"
      gem "foo", "1.0.0", "x64-mingw32" do
        dep "bar", "< 1"
      end
    end
    dep "foo"
    platforms "x64-mingw32"

raises an error because foo-1.0.0-x64-mingw32 requires bar<1 but there
isn't bar<1.

With this change, foo-1.0.0 (no x64-mingw32) is used as
fallback. Because foo-1.0.0 doesn't depend on bar<1.
kou added a commit to kou/bundler that referenced this pull request Dec 29, 2019
It resolves rubygems#6247. This changes includes the patches that add (RSpec)
specs for this situation in rubygems#6247.

If there is a platform specific gem but it can't be resolved available
version, Bundler reports an error.

For example,

    @Index = build_index do
      gem "bar", "1.0.0"
      gem "foo", "1.0.0"
      gem "foo", "1.0.0", "x64-mingw32" do
        dep "bar", "< 1"
      end
    end
    dep "foo"
    platforms "x64-mingw32"

raises an error because foo-1.0.0-x64-mingw32 requires bar<1 but there
isn't bar<1.

With this change, foo-1.0.0 (no x64-mingw32) is used as
fallback. Because foo-1.0.0 doesn't depend on bar<1.
kou added a commit to kou/bundler that referenced this pull request Dec 29, 2019
It resolves rubygems#6247. This changes includes the patches that add (RSpec)
specs for this situation in rubygems#6247.

If there is a platform specific gem but it can't be resolved available
version, Bundler reports an error.

For example,

    @Index = build_index do
      gem "bar", "1.0.0"
      gem "foo", "1.0.0"
      gem "foo", "1.0.0", "x64-mingw32" do
        dep "bar", "< 1"
      end
    end
    dep "foo"
    platforms "x64-mingw32"

raises an error because foo-1.0.0-x64-mingw32 requires bar<1 but there
isn't bar<1.

With this change, foo-1.0.0 (no x64-mingw32) is used as
fallback. Because foo-1.0.0 doesn't depend on bar<1.
@kou
Copy link
Contributor

kou commented Dec 29, 2019

I've implemented these added specs in #7522.
#7522 includes specs added in this pull request.

@deivid-rodriguez
Copy link
Member

Closing in favor of #7522 then. Thanks @kou and everybody helping out here!

kou added a commit to kou/bundler that referenced this pull request Jan 3, 2020
It resolves rubygems#6247. This changes includes the patches that add (RSpec)
specs for this situation in rubygems#6247.

If there is a platform specific gem but it can't be resolved available
version, Bundler reports an error.

For example,

    @Index = build_index do
      gem "bar", "1.0.0"
      gem "foo", "1.0.0"
      gem "foo", "1.0.0", "x64-mingw32" do
        dep "bar", "< 1"
      end
    end
    dep "foo"
    platforms "x64-mingw32"

raises an error because foo-1.0.0-x64-mingw32 requires bar<1 but there
isn't bar<1.

With this change, foo-1.0.0 (no x64-mingw32) is used as
fallback. Because foo-1.0.0 doesn't depend on bar<1.
kou added a commit to kou/bundler that referenced this pull request Jan 5, 2020
It resolves rubygems#6247. This changes includes the patches that add (RSpec)
specs for this situation in rubygems#6247.

If there is a platform specific gem but it can't be resolved available
version, Bundler reports an error.

For example,

    @Index = build_index do
      gem "bar", "1.0.0"
      gem "foo", "1.0.0"
      gem "foo", "1.0.0", "x64-mingw32" do
        dep "bar", "< 1"
      end
    end
    dep "foo"
    platforms "x64-mingw32"

raises an error because foo-1.0.0-x64-mingw32 requires bar<1 but there
isn't bar<1.

With this change, foo-1.0.0 (no x64-mingw32) is used as
fallback. Because foo-1.0.0 doesn't depend on bar<1.
kou added a commit to kou/bundler that referenced this pull request Jan 7, 2020
It resolves rubygems#6247. This changes includes the patches that add (RSpec)
specs for this situation in rubygems#6247.

If there is a platform specific gem but it can't be resolved available
version, Bundler reports an error.

For example,

    @Index = build_index do
      gem "bar", "1.0.0"
      gem "foo", "1.0.0"
      gem "foo", "1.0.0", "x64-mingw32" do
        dep "bar", "< 1"
      end
    end
    dep "foo"
    platforms "x64-mingw32"

raises an error because foo-1.0.0-x64-mingw32 requires bar<1 but there
isn't bar<1.

With this change, foo-1.0.0 (no x64-mingw32) is used as
fallback. Because foo-1.0.0 doesn't depend on bar<1.
kou added a commit to kou/bundler that referenced this pull request Jan 14, 2020
It resolves rubygems#6247. This changes includes the patches that add (RSpec)
specs for this situation in rubygems#6247.

If there is a platform specific gem but it can't be resolved available
version, Bundler reports an error.

For example,

    @Index = build_index do
      gem "bar", "1.0.0"
      gem "foo", "1.0.0"
      gem "foo", "1.0.0", "x64-mingw32" do
        dep "bar", "< 1"
      end
    end
    dep "foo"
    platforms "x64-mingw32"

raises an error because foo-1.0.0-x64-mingw32 requires bar<1 but there
isn't bar<1.

With this change, foo-1.0.0 (no x64-mingw32) is used as
fallback. Because foo-1.0.0 doesn't depend on bar<1.
ghost pushed a commit that referenced this pull request Jan 16, 2020
7522: Improve platform specific gem resolution r=deivid-rodriguez a=kou

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

Platform specific gems aren't resolved when platform specific gems are conflicted.

For example, in the following situation, foo-1.0.0-x64-mingw32 that requires bar<1 is conflicted because there is no bar<1. Without this change, Bundler raises a conflict error. But users can use foo-1.0.0 (no x64-mingw32) in this situation. With this change, Bundler resolves to foo-1.0.0 (no x64-mingw32).

```ruby
@Index = build_index do
  gem "bar", "1.0.0"
  gem "foo", "1.0.0"
  gem "foo", "1.0.0", "x64-mingw32" do
    dep "bar", "< 1"
  end
end
dep "foo"
platforms "x64-mingw32"
````

See also #6247. This change includes the specs that were added in #6247.

### What was your diagnosis of the problem?

Not platform specific gem (foo-1.0.0 in the above case) isn't tried to be resolved when platform specific gem (foo-1.0.0-x64-mingw32 in the above case) is available.

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

In this PR, not platform specific gem (foo-1.0.0 in the above case) is also tried to be resolved even when platform specific gem (foo-1.0.0-x64-mingw32 in the above case) is available. Priority is "platform specific gem" -> "not platform specific gem". So platform specific gem is usable, platform specific gem is used. Not platform specific gem is used as fallback.

`search_for` represents this. Here is the `search_for` specification:
https://github.com/bundler/bundler/blob/master/lib/bundler/vendor/molinillo/lib/molinillo/modules/specification_provider.rb#L11-L13

> Search for the specifications that match the given dependency. The specifications in the returned array will be considered in reverse order, so the latest version ought to be last.

## Why did you choose this fix out of the possible options?

I choose this fix because this is based on Molinillo's specification.

Co-authored-by: Lars Kanis <[email protected]>
Co-authored-by: Samuel Giddins <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants