-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Prefer require_relative
for internal requires
#7100
Conversation
Oh, I also introduced a separate change, but that I think it goes in the same direction. It's e9a39a9. In few words, if we're loading a file inside the bundler installation, we should directly load that instead of going through rubygems... stuff. |
This comment has been minimized.
This comment has been minimized.
2fa60e0
to
5be17d9
Compare
5be17d9
to
82489e0
Compare
Due to the way rubygems monkey-patched require interacts with default gems, and given that bundler is a default gem, and that bundler manipulates the LOAD_PATH in very intricated ways, we can reduce the risk of "leaking" to a different copy of `bundler` by using `require_relative` for internal requires.
Looks good to me! Only thing I’d ask for would be adding a quality spec that asserts there are no |
So that the spec can be run in isolation.
82489e0
to
a48a24b
Compare
a48a24b
to
0dff2a9
Compare
require_relative seems to got some problem on some ruby version rails/rails#30627 Most libraries does not need to consider this problem, I think. but bundler is widely used. So, it may be in trouble. |
Thanks for pointing that out @walf443! That's too bad, I wish we could do this without risk... 😞. @amatsuda Is |
The ruby fix was backported and released with 2.4.4, and it was not backported to 2.3, but 2.3 is already EOL'd. So... I'm on the fence. |
Ideally, I should justify that doing this actually fixes a default gem activation issue, so this change would be a bug fix. I'll see what I can do! |
IIRC we said that we would stop supporting 2.3 at the same time as ruby-core, so I think I'm okay with officially dropping support for 2.3 from master, if you want to do that soon-ish? |
Ok, then I say let's get this in? I can open a separate PR for the support drop if/when we decide to do it. |
Let's do it. 👍🏻 |
Going in! @bundlerbot r+ |
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>
Build succeeded |
7137: Gemspec require relative r=deivid-rodriguez a=deivid-rodriguez ### What was the end-user problem that led to this PR? This is a follow up to #7100. Gemspec is another place where `require_relative` sounds like a win. ### What is your fix for the problem, implemented in this PR? My fix changes bundler's gemspec to use `require_relative`, and extracts the relevant change from #4397 to also change the generated gemspec from `bundle gem`. Co-authored-by: Miklos Fazekas <mfazekas@szemafor.com> Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
7193: More `require_relative` r=deivid-rodriguez a=deivid-rodriguez This is a follow up to #7062 and #7100, migrating the last missing internal requires to use `require_relative`. I thought I had migrated everything already, but I had not. As a note, I'm migrating thor's code here. I will open a PR to upstream's thor to incorporate this changes, but thor is still stuck with 1.8 support, so we have to wait a bit. Given the very low activity thor has, it's fine to make the changes here first, and wait until we can incorporate them upstream. ### What was the end-user problem that led to this PR? The problem was that sometimes we can end up requiring the default version of bundler, instead of the current copy that's being run, and that's dangerous and can cause version mismatch issues. ### What is your fix for the problem, implemented in this PR? My fix is to always use `require_relative` for internal requires. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
7248: Fix nested bundle exec's when bundler is a default gem r=deivid-rodriguez a=MSP-Greg ### What was the end-user problem that led to this PR? The problem was that when bundler is a default gem, nested `bundle exec` commands generate a LoadError. ``` /home/travis/.rvm/rubies/ruby-head/bin/bundle:30:in `load': cannot load such file -- /home/travis/.rvm/rubies/ruby-head/lib/bin/bundle (LoadError) ``` ### What was your diagnosis of the problem? Not accounting for Bundler being installed as a default gem. When it's a default, the lib and exe folders do not share the same root folder. This was the result of a change in e742c3d (#7100). ### Repo Example Using Ruby master/trunk/ruby-head (as of ruby/ruby@0c6c937), from a folder where `bundle exec` can be ran: ``` bundle exec "bundle exec 'ruby -v'" ``` ### What is your fix for the problem, implemented in this PR? Small adjustment to logic for finding the correct exe/bundle file. ### Why did you choose this fix out of the possible options? I chose this fix because it's similar to previous code. Fixes #7244. Co-authored-by: MSP-Greg <msp-greg@users.noreply.github.com>
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 torequire
, 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.