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

Prefer require_relative for internal requires #7100

Merged
7 commits merged into from Apr 23, 2019
Merged

Prefer require_relative for internal requires #7100

7 commits merged into from Apr 23, 2019

Conversation

deivid-rodriguez
Copy link
Member

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.

@deivid-rodriguez
Copy link
Member Author

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.

@Cruzinho10

This comment has been minimized.

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.
@segiddins
Copy link
Member

Looks good to me! Only thing I’d ask for would be adding a quality spec that asserts there are no bundler/* regular requires, so we can stay consistent

So that the spec can be run in isolation.
@walf443
Copy link
Contributor

walf443 commented Apr 22, 2019

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.

@deivid-rodriguez
Copy link
Member Author

Thanks for pointing that out @walf443! That's too bad, I wish we could do this without risk... 😞.

@amatsuda Is bundler potentially vulnerable to run into this problem? We do have some specs for GEM_HOME being a symlinked location, but I don't think we have specs for just the bundler gem being installed at a symlinked location, which is the potential situation where this problem would happen, I think?

@deivid-rodriguez
Copy link
Member Author

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.

@deivid-rodriguez
Copy link
Member Author

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!

@indirect
Copy link
Member

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?

@deivid-rodriguez
Copy link
Member Author

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.

@indirect
Copy link
Member

Let's do it. 👍🏻

@deivid-rodriguez
Copy link
Member Author

Going in!

@bundlerbot r+

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>
@ghost
Copy link

ghost commented Apr 23, 2019

Build succeeded

@ghost ghost merged commit 0dff2a9 into master Apr 23, 2019
@ghost ghost deleted the require_relative branch April 23, 2019 09:18
ghost pushed a commit that referenced this pull request Apr 26, 2019
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>
ghost pushed a commit that referenced this pull request Jun 10, 2019
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>
ghost pushed a commit that referenced this pull request Jul 19, 2019
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>
This pull request was closed.
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

6 participants