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

Use realpath in clean_load_path #6502

Merged
merged 8 commits into from Jul 10, 2018
Merged

Conversation

ojab
Copy link
Contributor

@ojab ojab commented Apr 23, 2018

see #6465, basically we're not rejecting bundler_lib because we have symlink there and real path in $LOAD_PATH

$ irb
2.5.1 :001 > require 'bundler/setup'
# what happens next will shock you
From: /real_path/.rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/shared_helpers.rb @ line 339 Bundler::SharedHelpers#clean_load_path:

    330: def clean_load_path
    331:   # handle 1.9 where system gems are always on the load path
    332:   return unless defined?(::Gem)
    333:
    334:   bundler_lib = bundler_ruby_lib
    335:
    336:   loaded_gem_paths = Bundler.rubygems.loaded_gem_paths
    337:
    338:   binding.pry
 => 339:   $LOAD_PATH.reject! do |p|
    340:     path = File.expand_path(p)
    341:     path = File.realpath(path) if File.exist?(path)
    342:     next if path.start_with?(bundler_lib)
    343:     loaded_gem_paths.delete(p)
    344:   end
    345:   $LOAD_PATH.uniq!
    346: end

[1] pry(#<Bundler::Runtime>)> bundler_lib
=> "/real_path/.rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib"
[2] pry(#<Bundler::Runtime>)> loaded_gem_paths
=> ["/symlinked_path/.rvm/gems/ruby-2.5.1@global/gems/did_you_mean-1.2.0/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/byebug-10.0.2/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/extensions/x86_64-linux/2.5.0/byebug-10.0.2",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/coderay-1.1.2/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/method_source-0.9.0/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/pry-0.11.3/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/pry-byebug-3.6.0/lib",
 "/real_path/.rvm/rubies/ruby-2.5.1/lib/ruby/gems/2.5.0/gems/fileutils-1.0.2/lib",
 "/real_path/.rvm/rubies/ruby-2.5.1/lib/ruby/gems/2.5.0/gems/psych-3.0.2/lib",
 "/real_path/.rvm/rubies/ruby-2.5.1/lib/ruby/gems/2.5.0/extensions/x86_64-linux/2.5.0/psych-3.0.2"]

@ghost
Copy link

ghost commented Apr 23, 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

@colby-swandale
Copy link
Member

This will need a test

@alexdean
Copy link

@ojab i took a pass at writing some tests for this, but i'm not sure i'm on the right track. (not clear on what should be in $LOAD_PATH when this method finishes.)

@ojab ojab changed the base branch from 1-16-stable to master April 27, 2018 13:41
@ojab
Copy link
Contributor Author

ojab commented Apr 27, 2018

Sorry for delay.

  • rebased to master
  • added spec that fails on ruby-2.5.1 + bundler master and passes on ruby-2.5.1 and bundler from my branch

So the question is: what exactly we should test? Right now I'm setting up symlinked RUBY_HOME w/ bundler and requiring 'bundler/setup' (and it looks quite ugly tbh, suggestions are welcome).

  1. Do we need to test that clean_load_path actually removes bundler_lib from $LOAD_PATH?
  2. What should we do with Bundler.rubygems.loaded_gem_paths? In my test setup $LOAD_PATH contains both symlinked and real path for some gems, while Bundler.rubygems.loaded_gem_paths contains real paths only for system gems (I don't know if "system" is proper term here), such as fiddle/fileutils/json/etc, all other have symlinked paths. So should we convert it all to the real paths during clean_load_path or we're already ok here? Everything works as is AFAICS, but I don't know if we have can of worms here.
  3. Should I add some missing directories to $LOAD_PATH that I'm testing? If so — is it ok to add it to the same it block or better add another one (I don't know preferred style in bundler)?
  4. Right now travis testing 2.5.0, should I bump it to 2.5.1 (and the same for 2.3/2.4 rubies)? If so — do we need to retain 2.5.0, considering behaviour change and unknown reason for that change? (we know when this failure is happening, but don't know why and when it actually started happening)

@ojab
Copy link
Contributor Author

ojab commented Apr 27, 2018

  1. Do we need to check that p.start_with?(bundler_lib) or this case is impossible?

Copy link
Member

@segiddins segiddins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable, with the config changes reverted

@@ -336,7 +336,9 @@ def clean_load_path
loaded_gem_paths = Bundler.rubygems.loaded_gem_paths

$LOAD_PATH.reject! do |p|
next if File.expand_path(p).start_with?(bundler_lib)
path = File.expand_path(p)
path = File.realpath(path) if File.exist?(path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to also check if File.respond_to?(:realpath)


ENV["GEM_PATH"] = symlinked_gem_home.to_path
ruby <<-R
$LOAD_PATH.delete_at(0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update this to delete a particular path, with a comment why it's being deleted

ENV["GEM_PATH"] = symlinked_gem_home.to_path
ruby <<-R
$LOAD_PATH.delete_at(0)
puts (require 'bundler/setup')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imho we should add something like raise "missing bundler lib path in #{$LOAD_PATH}" unless $LOAD_PATH.find ...

next if File.expand_path(p).start_with?(bundler_lib)
path = File.expand_path(p)
path = File.realpath(path) if File.exist?(path)
next if path.start_with?(bundler_lib)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next if path.start_with?(bundler_lib) || p.start_with?(bundler_lib) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|| File.expand_path(p).start_with?(bundler_lib)?

@ojab ojab changed the title [RFC] Use realpath in clean_load_path Use realpath in clean_load_path May 1, 2018
@ojab ojab force-pushed the 1-16-stable branch 18 times, most recently from f14c04e to c47f8cd Compare May 2, 2018 14:20
@eregon
Copy link
Contributor

eregon commented Jul 9, 2018

@indirect Could you share your opinion on which way to go seems best?
This issue affects every RVM user for JRuby & TruffleRuby, as well as anyone having Bundler installing in a Gem directory referenced as a symlink in GEM_PATH.

@indirect
Copy link
Member

@eregon after really digging in to why this test was failing on Travis with RVM, it turns out that RVM was "installing" bundler by simply dropping a copy of Bundler into site_ruby. No gemspec, no gem installation, just putting "bundler.rb" into a default LOAD_PATH. It had literally never occurred to me that anyone might do that, and so our tests were totally unprepared for that situation. 😬

My additional commits to this PR add some defensive deleting, removing Bundler if the bare files have been copied into site_ruby. The tests now pass, and this fix is now ready to go for the next release of Bundler.

Thanks everyone for all of your help!

@indirect
Copy link
Member

@bundlerbot r+

@bundlerbot
Copy link
Collaborator

📌 Commit 865ec52 has been approved by indirect

@bundlerbot
Copy link
Collaborator

⌛ Testing commit 865ec52 with merge 9ca3afe...

bundlerbot added a commit that referenced this pull request Jul 10, 2018
Use realpath in clean_load_path

see #6465, basically we're not rejecting `bundler_lib` because we have symlink there and real path in `$LOAD_PATH`
```
$ irb
2.5.1 :001 > require 'bundler/setup'
# what happens next will shock you
From: /real_path/.rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/shared_helpers.rb @ line 339 Bundler::SharedHelpers#clean_load_path:

    330: def clean_load_path
    331:   # handle 1.9 where system gems are always on the load path
    332:   return unless defined?(::Gem)
    333:
    334:   bundler_lib = bundler_ruby_lib
    335:
    336:   loaded_gem_paths = Bundler.rubygems.loaded_gem_paths
    337:
    338:   binding.pry
 => 339:   $LOAD_PATH.reject! do |p|
    340:     path = File.expand_path(p)
    341:     path = File.realpath(path) if File.exist?(path)
    342:     next if path.start_with?(bundler_lib)
    343:     loaded_gem_paths.delete(p)
    344:   end
    345:   $LOAD_PATH.uniq!
    346: end

[1] pry(#<Bundler::Runtime>)> bundler_lib
=> "/real_path/.rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib"
[2] pry(#<Bundler::Runtime>)> loaded_gem_paths
=> ["/symlinked_path/.rvm/gems/ruby-2.5.1@global/gems/did_you_mean-1.2.0/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/byebug-10.0.2/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/extensions/x86_64-linux/2.5.0/byebug-10.0.2",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/coderay-1.1.2/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/method_source-0.9.0/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/pry-0.11.3/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/pry-byebug-3.6.0/lib",
 "/real_path/.rvm/rubies/ruby-2.5.1/lib/ruby/gems/2.5.0/gems/fileutils-1.0.2/lib",
 "/real_path/.rvm/rubies/ruby-2.5.1/lib/ruby/gems/2.5.0/gems/psych-3.0.2/lib",
 "/real_path/.rvm/rubies/ruby-2.5.1/lib/ruby/gems/2.5.0/extensions/x86_64-linux/2.5.0/psych-3.0.2"]
```
@bundlerbot
Copy link
Collaborator

☀️ Test successful - status-travis
Approved by: indirect
Pushing 9ca3afe to master...

@bundlerbot bundlerbot merged commit 865ec52 into rubygems:master Jul 10, 2018
@indirect indirect removed their request for review July 10, 2018 07:03
colby-swandale pushed a commit that referenced this pull request Jul 10, 2018
Use realpath in clean_load_path

see #6465, basically we're not rejecting `bundler_lib` because we have symlink there and real path in `$LOAD_PATH`
```
$ irb
2.5.1 :001 > require 'bundler/setup'
# what happens next will shock you
From: /real_path/.rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/shared_helpers.rb @ line 339 Bundler::SharedHelpers#clean_load_path:

    330: def clean_load_path
    331:   # handle 1.9 where system gems are always on the load path
    332:   return unless defined?(::Gem)
    333:
    334:   bundler_lib = bundler_ruby_lib
    335:
    336:   loaded_gem_paths = Bundler.rubygems.loaded_gem_paths
    337:
    338:   binding.pry
 => 339:   $LOAD_PATH.reject! do |p|
    340:     path = File.expand_path(p)
    341:     path = File.realpath(path) if File.exist?(path)
    342:     next if path.start_with?(bundler_lib)
    343:     loaded_gem_paths.delete(p)
    344:   end
    345:   $LOAD_PATH.uniq!
    346: end

[1] pry(#<Bundler::Runtime>)> bundler_lib
=> "/real_path/.rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib"
[2] pry(#<Bundler::Runtime>)> loaded_gem_paths
=> ["/symlinked_path/.rvm/gems/ruby-2.5.1@global/gems/did_you_mean-1.2.0/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/byebug-10.0.2/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/extensions/x86_64-linux/2.5.0/byebug-10.0.2",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/coderay-1.1.2/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/method_source-0.9.0/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/pry-0.11.3/lib",
 "/symlinked_path/.rvm/gems/ruby-2.5.1/gems/pry-byebug-3.6.0/lib",
 "/real_path/.rvm/rubies/ruby-2.5.1/lib/ruby/gems/2.5.0/gems/fileutils-1.0.2/lib",
 "/real_path/.rvm/rubies/ruby-2.5.1/lib/ruby/gems/2.5.0/gems/psych-3.0.2/lib",
 "/real_path/.rvm/rubies/ruby-2.5.1/lib/ruby/gems/2.5.0/extensions/x86_64-linux/2.5.0/psych-3.0.2"]
```

(cherry picked from commit 9ca3afe)
@eregon
Copy link
Contributor

eregon commented Jul 10, 2018

@indirect Thank you. I agree removing that copy on site_ruby should make everything saner for testing.

I think this copy of bundler in site_ruby is created by running gem update --system with MRI 2.5.1.
At least I can confirm gem update --system adds lib/ruby/site_ruby/2.5.0/bundler when I run it locally.
TravisCI's MRI 2.5.1 (https://rubies.travis-ci.org/ubuntu/14.04/x86_64/ruby-2.5.1.tar.bz2) already contains the site_ruby bundler.
I'd guess this might be due to travis-ci/travis-rubies@e62622d or RVM updating RubyGems on rvm install ruby-2.5.1.

In TravisCI's MRI 2.5.1, there is a gemspec in the default gems dir (same for my local copy of MRI 2.5.1): lib/ruby/gems/2.5.0/specifications/default/bundler-1.16.2.gemspec

return expanded unless File.respond_to?(:realpath)

while File.exist?(expanded) && File.realpath(expanded) != expanded
expanded = File.realpath(expanded)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK File.realpath() resolves all symlinks it can, so a single call should be enough, unlike readlink(1).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ojab for making a PR to fix this :)

bundlerbot added a commit to rubygems/rubygems that referenced this pull request Jul 16, 2018
…ndale

Expand symlinks in gem path

# Description:

The environment variable `GEM_PATH` can contain symlinks.
This is notably the case on RVM, with a typical `GEM_PATH` being
`GEM_PATH=/home/eregon/.rvm/gems/ruby-2.5.1:/home/eregon/.rvm/gems/ruby-2.5.1@global` (the `...@global` is a symlink to `/home/eregon/.rvm/rubies/ruby-2.5.1/lib/ruby/gems/2.5.0`).

Currently, RubyGems just takes the paths from `GEM_HOME` and `GEM_PATH` and store them in `Gem.dir` and `Gem.path`, without resolving symlinks.
Similarly those paths might end up in `$LOAD_PATH` and still contain symlinks.

This makes all code relying on `__FILE__`, `caller`, `$LOADED_FEATURES` or `$LOAD_PATH`, etc more brittle for paths comparisons, by having to expand paths with e.g. `File.realpath` before any path comparison involving those paths.
As an example, Bundler has a recent bug due to this, where it fails to detect its own `bundler-1.16.2/lib` directory in `$LOAD_PATH` due to the presence of symlinks in GEM_PATH: rubygems/bundler#6502 rubygems/bundler#6465

While this specific case of course should be fixed in Bundler, it's not hard to imagine many other places might have similar assumptions.
So I think it's just more robust for all gems to have RubyGems expands symlinks from the environment `GEM_HOME` and `GEM_PATH`.

Ruby implementations since Ruby 2.4.4 [resolve symlinks from `$LOAD_PATH`](oracle/truffleruby@9a76df4#diff-7346c44420f7a2c1d276e7f67dcee8f8R369).
Therefore, having symlinks in `$LOAD_PATH` means an extra cost during `require` to expand each `$LOAD_PATH` with symlinks.
So it's more efficient if there are already realpaths in `$LOAD_PATH`, and less surprising for everybody (e.g., paths in `$LOADED_FEATURES` are actual concatenation of one of the `$LOAD_PATH` + `/` + required feature).
______________

# Tasks:

- [x] Describe the problem / feature
- [x] Write tests
- [x] Write code to solve the problem
- [x] Get code review from coworkers / friends

I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).
ghost pushed a commit that referenced this pull request Sep 19, 2018
6694: Fix local spec failure r=indirect a=deivid-rodriguez

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

The problem was that bundler specs won't pass on my system. One of the two failures I was getting is the spec added by #6502.

### What was your diagnosis of the problem?

My diagnosis was that my system has a similar setup as TravisCI (Linux with a ruby version manager on top), but I don't set the `TRAVIS` environment variable to run the tests. This means some trickery used in the specs to get them green on TravisCI is not applied to my local run, so specs failed.

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

My fix is to make this spec green independently from whether it's run on TravisCI or not. My changes also make this spec not delete any files from the rubygems installation where the specs are running. In #6502 (comment) @indirect said that the files being deleted here are put in there by `RVM`. However, I don't think that's accurate, it's `gem update --system` putting these files in there. So if these files should not be there, we should not put them there in the first place instead of deleting them during bundler's specs.

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

I chose this fix because the alternative would be to set the `TRAVIS` environment variable before running the specs locally, but that would mean that `bundler`'s specs would delete files from my global `rubygems` installation, which seems... no good.

Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
ghost pushed a commit that referenced this pull request Sep 23, 2018
6694: Fix local spec failure r=deivid-rodriguez a=deivid-rodriguez

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

The problem was that bundler specs won't pass on my system. One of the two failures I was getting is the spec added by #6502.

### What was your diagnosis of the problem?

My diagnosis was that my system has a similar setup as TravisCI (Linux with a ruby version manager on top), but I don't set the `TRAVIS` environment variable to run the tests. This means some trickery used in the specs to get them green on TravisCI is not applied to my local run, so specs failed.

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

My fix is to make this spec green independently from whether it's run on TravisCI or not. My changes also make this spec not delete any files from the rubygems installation where the specs are running. In #6502 (comment) @indirect said that the files being deleted here are put in there by `RVM`. However, I don't think that's accurate, it's `gem update --system` putting these files in there. So if these files should not be there, we should not put them there in the first place instead of deleting them during bundler's specs.

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

I chose this fix because the alternative would be to set the `TRAVIS` environment variable before running the specs locally, but that would mean that `bundler`'s specs would delete files from my global `rubygems` installation, which seems... no good.

Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
hsbt pushed a commit to rubygems/rubygems that referenced this pull request Oct 9, 2018
…ndale

Expand symlinks in gem path

# Description:

The environment variable `GEM_PATH` can contain symlinks.
This is notably the case on RVM, with a typical `GEM_PATH` being
`GEM_PATH=/home/eregon/.rvm/gems/ruby-2.5.1:/home/eregon/.rvm/gems/ruby-2.5.1@global` (the `...@global` is a symlink to `/home/eregon/.rvm/rubies/ruby-2.5.1/lib/ruby/gems/2.5.0`).

Currently, RubyGems just takes the paths from `GEM_HOME` and `GEM_PATH` and store them in `Gem.dir` and `Gem.path`, without resolving symlinks.
Similarly those paths might end up in `$LOAD_PATH` and still contain symlinks.

This makes all code relying on `__FILE__`, `caller`, `$LOADED_FEATURES` or `$LOAD_PATH`, etc more brittle for paths comparisons, by having to expand paths with e.g. `File.realpath` before any path comparison involving those paths.
As an example, Bundler has a recent bug due to this, where it fails to detect its own `bundler-1.16.2/lib` directory in `$LOAD_PATH` due to the presence of symlinks in GEM_PATH: rubygems/bundler#6502 rubygems/bundler#6465

While this specific case of course should be fixed in Bundler, it's not hard to imagine many other places might have similar assumptions.
So I think it's just more robust for all gems to have RubyGems expands symlinks from the environment `GEM_HOME` and `GEM_PATH`.

Ruby implementations since Ruby 2.4.4 [resolve symlinks from `$LOAD_PATH`](oracle/truffleruby@9a76df4#diff-7346c44420f7a2c1d276e7f67dcee8f8R369).
Therefore, having symlinks in `$LOAD_PATH` means an extra cost during `require` to expand each `$LOAD_PATH` with symlinks.
So it's more efficient if there are already realpaths in `$LOAD_PATH`, and less surprising for everybody (e.g., paths in `$LOADED_FEATURES` are actual concatenation of one of the `$LOAD_PATH` + `/` + required feature).
______________

# Tasks:

- [x] Describe the problem / feature
- [x] Write tests
- [x] Write code to solve the problem
- [x] Get code review from coworkers / friends

I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).
ghost pushed a commit that referenced this pull request Apr 23, 2019
7100: Prefer `require_relative` for internal requires r=deivid-rodriguez a=deivid-rodriguez

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

The problem was that `bundler` seems to, in some very rare cases, leak to the copy of itself installed as a default gem. I have been able to reproduce this only for stuff that we have already fixed. For example: #6502. However, I have the gut feeling that this can still happen under some conditions, because sometimes we still get reports from people using bundler 2, and getting the error "You must user Bundler 2 or greater with this Gemfile". 

### What was your diagnosis of the problem?

My diagnosis was that somehow, due to the complicated LOAD_PATH manipulation bundler does, we may endup requiring bundler files in another copy of bundler.

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

My fix is not really a fix, although it _might_ prevent the potential issue from happening. As @colby-swandale would say, we should fix the real culprit instead. However, I think using `require_relative` is a better practice anyways, because it makes it clear that you are requiring "internal" files and not files from some dependencies. And it should also be faster because it does not search the LOAD_PATH. And it skips the rubygems monkeypatches to `require`, which seems also good.

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

I chose this fix because I think it's a good practice.

Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants