Fix rubygems 2.2 compatibility

[voxik]

This is follow up of #3234

[voxik]

I'd love to add some test case for this, but unfortunately, I can't see too many $LOAD_PATH tests in current test suite. Any help is appreciated.

[TimMoore]

Most of the existing tests are integration tests for real-world types of scenarios. What's the problem that this change fixes?

[voxik]

I don't think that Bundler does good job cleaning load path. And there are several concerns:

1. Bundler supports RubyGems 2.2 way of adding gem paths to $LOAD_PATH [1]. For example on Fedora, we store the binary extensions into the completely separate tree, e.g. the Ruby code goes to /usr/share folders, while the binary extensions goes to /usr/lib. This should allow us to have better support for various interpreters without too much duplication. However, Bundler does not clean such gems from $LOAD_PATH correctly. As you can see from example at Fix RubyGems 2.2+ compatibility. #3234 (comment) it leaves behind load paths, where binary extension stays.
2. Bundler used to delete everything from the load path. The only qualification was that it has to be at the Bundler.rubygems.gem_path.any. So if I add some load path manually using RUBYLIB, Bundler does not care and removes it.

This patch is changing this. It asks RubyGems "what are the load paths of the stuff you loaded" and removes just that paths. Since these paths were cleaned from load path, it cleans also the Gem.loaded_specs and later just adds Bundler back on load path.

[1] https://github.com/bundler/bundler/blob/master/lib/bundler/rubygems_ext.rb#L37

[TimMoore]

@voxik could you rebase this onto the upstream master? There was a recent fix for the build failures on Ruby 1.8.7.

[voxik]

@TimMoore I did that, but it does not help :/

[TimMoore]

You're right... that fix doesn't work deterministically.

Still, though, some jobs failed due to a spec failure that appears to be related to this change.

[voxik]

Are you referring to

 1) Bundler.setup preactivated gems raises an exception if a pre activated gem conflicts with the bundle
     Failure/Error: expect(out).to eq("You have already activated thin 1.1, but your Gemfile requires thin 1.0. Prepending `bundle exec` to your command may solve this.")

       expected: "You have already activated thin 1.1, but your Gemfile requires thin 1.0. Prepending `bundle exec` to your command may solve this."
            got: "FAIL"

       (compared using ==)
     # ./spec/runtime/setup_spec.rb:550

or something else? That would be weird if it is related to my change. But let me investigate ...

[voxik]

Hi, I updated the patch and now all tests are passing. I'd appreciate if you could merge.

[indirect]

This is a good fix, thanks. Unfortunately, Bundler can't use any Gem methods directly. Everything needs to go through the Bundler::RubygemsIntegration class (which you can access as Bundler.rubygems). The Rubygems API is too volatile to access directly. Can you update this pull request to only use Bundler.rubygems methods? Then I will merge it. Thanks!

[voxik]

@indirect Sure! I modified the code.

However, I am unsure if that is enough. May be I should move the whole Bundler.rubygems.loaded_specs.map loop into the rubygems_integration.rb, to avoid usage of Gem::Specification as well and keep the loaded_specs untouched?

[indirect]

@voxik I would prefer that, yes. If you could make that change it would be great. How can we test this to make sure it does not break in the future? We can create a gem with a binary extension for the test.

[voxik]

@indirect Ok, I'll try to update the patch in that direction.

[voxik]

I pushed updated patch, if you could review. And I have to take look on some test case ....

[voxik]

Ok, so I put also test case in place. There appears to be single test failure [1] related to this test case on travis, but I have no clue how it may happens. I tend to believe, that the error is exhibited just by coincidence.

I the PR is acceptable in its current form, I'd prefer to squash the commits prior merge.

[1] https://travis-ci.org/bundler/bundler/jobs/43453419

[TimMoore]

@voxik I've rerun https://travis-ci.org/bundler/bundler/jobs/43453419 a few times and the failure happens consistently.

This backtrace is in the logs:

/home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/ext/builder.rb:12: uninitialized constant Gem::UserInteraction (NameError)
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/core_ext/kernel_require.rb:55:in `gem_original_require'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/core_ext/kernel_require.rb:55:in `require'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/ext.rb:13
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/core_ext/kernel_require.rb:55:in `gem_original_require'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/core_ext/kernel_require.rb:55:in `require'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/specification.rb:1431:in `build_extensions'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/stub_specification.rb:60:in `build_extensions'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/basic_specification.rb:56:in `contains_requirable_file?'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/specification.rb:925:in `find_inactive_by_path'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/core_ext/kernel_require.rb:55:in `find'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/specification.rb:924:in `each'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/specification.rb:924:in `find'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/specification.rb:924:in `find_inactive_by_path'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems.rb:185:in `try_activate'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/core_ext/kernel_require.rb:132:in `require'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/user_interaction.rb:8
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/core_ext/kernel_require.rb:55:in `gem_original_require'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/core_ext/kernel_require.rb:55:in `require'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/config_file.rb:7
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/core_ext/kernel_require.rb:55:in `gem_original_require'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/core_ext/kernel_require.rb:55:in `require'
    from /home/travis/build/bundler/bundler/lib/bundler/rubygems_integration.rb:2
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/core_ext/kernel_require.rb:55:in `gem_original_require'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/core_ext/kernel_require.rb:55:in `require'
    from /home/travis/build/bundler/bundler/lib/bundler.rb:6
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/core_ext/kernel_require.rb:55:in `gem_original_require'
    from /home/travis/build/bundler/bundler/tmp/rubygems/lib/rubygems/core_ext/kernel_require.rb:55:in `require'
    from -e:6

Can you take a look?

[strzibny]

I believe the fail is connected with rubygems/rubygems#784, so I would say it's safe to merge.

[TimMoore]

Sorry, but we're not going to merge something that breaks the build.

[voxik]

@TimMoore I added several comments to rubygems/rubygems#784 which is indeed the reason for the failures, not this patch (although this patch may lead to issues such as the one with rdiscount), but this is again issue of RubyGems or rdiscount and orthogonal to this patch.

[indirect]

@voxik I'm happy to merge this as soon as Rubygems fixes rubygems/rubygems#784, but we can only allow these changes to happen in versions of Rubygems that have fixed that bug.

This patch needs to either: 1) work around the rubygems issue by not activating unless the rubygems issue is already fixed or 2) fix the rubygems issue via monkeypatching for the exact versions of 2.2 that are broken. Is either one of those feasible for you?

[skull-squadron]

Submitted rubygems/rubygems#1137 to unblock rubygems/rubygems#784. Credit to @voxik

[indirect]

ping @steakknife @voxik can either of you rebase this and check the rubygems version for a compatible one before applying the fix?

[skull-squadron]

@indirect @voxik Can you guys confirm that a patch is even needed for MRI 2.2 functionality? Latest RubyGems@master reverted conflicting native/Ruby loading behavior. The cleaning of $LOAD_PATH is still important for a reasonable amount of non-legacy-breaking environment isolation, however it might be less net work to not conflate the two.