Skip to content
This repository has been archived by the owner. It is now read-only.

Fix more leaks to default copy of bundler #7274

Merged
9 commits merged into from Aug 18, 2019

Conversation

deivid-rodriguez
Copy link
Member

@deivid-rodriguez deivid-rodriguez commented Aug 3, 2019

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

The problem was that in some places, it was still possible to end up requiring files in a different copy of bundler (the default copy). I noticed this when I removed a rubygems monkeypatch from the test suite that was preventing the default copy of bundler from being activated when requiring files.

This thing:

https://github.com/bundler/bundler/blob/e1c518363641208429f397170354054b3d28effd/spec/support/hax.rb#L15-L20

What was your diagnosis of the problem?

My diagnosis was that I should use relative requires wherever they were missing.

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

My fix is to remove the rubygems hack, migrate the rest of the internal requires to be relative, and also introduce some hacks on our specs to make sure we never load the incorrect copy of bundler.

I think this PR should fix the issues in rubygems/rubygems#2863.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Aug 3, 2019

Unfortunately this patch seems to introduce some flakyness on a single realworld spec

https://github.com/bundler/bundler/blob/e1c518363641208429f397170354054b3d28effd/spec/realworld/double_check_spec.rb#L1-L40

And I'm not yet sure why 😬.

@deivid-rodriguez deivid-rodriguez force-pushed the fix_more_leaks_to_default_copy_of_bundler branch from 9f72bc2 to 9e135d4 Compare Aug 3, 2019
@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Aug 3, 2019

Actually, nevermind, the flakyness is already present in master, so it's unrelated to this PR.

I'll investigate how the flaky got started separately.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Aug 3, 2019

I still want to review some things in this PR, so please don't merge it just yet :)

spec/install/gemfile/groups_spec.rb Outdated Show resolved Hide resolved
spec/support/path.rb Outdated Show resolved Hide resolved
@deivid-rodriguez deivid-rodriguez force-pushed the fix_more_leaks_to_default_copy_of_bundler branch 2 times, most recently from 86ca451 to 547a116 Compare Aug 6, 2019
@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Aug 6, 2019

@hsbt I addressed your feedback and now hopefully all paths are independent from the specific folder structure where the tests are run, thanks for pointing that out 👍.

I still want to improve a few other things in this PR, so I'll let you know when it's ready.

@deivid-rodriguez deivid-rodriguez force-pushed the fix_more_leaks_to_default_copy_of_bundler branch 3 times, most recently from dbd8141 to 300dcf7 Compare Aug 8, 2019
@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Aug 8, 2019

I'm now happy with this PR, @hsbt. You can have another look :)

@deivid-rodriguez deivid-rodriguez requested a review from hsbt Aug 8, 2019
@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Aug 12, 2019

@hsbt If you're ok, I'd like to merge this.

@hsbt
Copy link
Member

hsbt commented Aug 13, 2019

@deivid-rodriguez I'm going to try to merge this pull-request into ruby repo soon.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Aug 13, 2019

Thank you @hsbt, let me know how it goes! Hopefully it allows removing some of those :ruby_core tags :)

ghost pushed a commit that referenced this issue Aug 16, 2019
7301: Track changes from ruby core master r=hsbt a=hsbt

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

I'm going to merge #7274. But the ruby-core source has some of the changes for bundler source.

### What was your diagnosis of the problem?


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

ruby core team fixed them:

* Removed circular require warning at `shared_helper.rb`
* Support test at GitHub Actions, It helps that bundler will migrate Actions from Azure Pipelines too.
* Fixed broken examples at ruby core repository


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



Co-authored-by: ohbarye <over.rye@gmail.com>
Co-authored-by: Hiroshi SHIBATA <hsbt@ruby-lang.org>
Co-authored-by: Yusuke Endoh <mame@ruby-lang.org>
@hsbt
Copy link
Member

hsbt commented Aug 16, 2019

We need to rebase this after merging #7303

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Aug 16, 2019

We need to rebase this after merging #7303

Yes, I'll take care of that 👍.

@deivid-rodriguez deivid-rodriguez force-pushed the fix_more_leaks_to_default_copy_of_bundler branch 4 times, most recently from f2f81de to 97ddcb4 Compare Aug 17, 2019
If we use system bundler, when booting the "outermost" bundler process,
bundler will save the path to the system bundler in BUNDLE_BIN_PATH, and
use it again when booting the "innermost" bundler process (`bundle exec
echo foo`).

That means that second process will use the system bundler path again.
However, we have `-rsupport/hax` in RUBYOPT, so that file will load from
the local copy of bundler, and that file will load `bundler/version`
from the project (not from system), because -Ilib is in the LOAD_PATH.

That will end up causing redefinition errors because the same constant
will be loaded from two different locations.

In general, this is expected behavior, normally you will wrap the
process with `Bundler.with_original_env` to reset the environment.
However, the easiest fix here is to not use system bundler, because it's
not really necessary and thus doesn't help the readability of the spec.
Instead, make sure we always load the local copy of bundler during
specs, and never end up using the default copy.
The version we're vendoring actually relaxed this restriction back to
2.3.0+, so we can always use the vendored version.
@deivid-rodriguez deivid-rodriguez force-pushed the fix_more_leaks_to_default_copy_of_bundler branch from 7eca509 to 8ef571e Compare Aug 17, 2019
@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Aug 17, 2019

@hsbt This one is ready, can I merge it?

hsbt
hsbt approved these changes Aug 17, 2019
Copy link
Member

@hsbt hsbt left a comment

I confirmed to pass these changes in ruby repo.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Aug 18, 2019

@bundlerbot r=hsbt

ghost pushed a commit that referenced this issue Aug 18, 2019
7274: Fix more leaks to default copy of bundler r=hsbt a=deivid-rodriguez

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

The problem was that in some places, it was still possible to end up requiring files in a different copy of bundler (the default copy). I noticed this when I removed a rubygems monkeypatch from the test suite that was preventing the default copy of bundler from being activated when requiring files.

This thing:

https://github.com/bundler/bundler/blob/e1c518363641208429f397170354054b3d28effd/spec/support/hax.rb#L15-L20

### What was your diagnosis of the problem?

My diagnosis was that I should use relative requires wherever they were missing.

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

My fix is to remove the rubygems hack, migrate the rest of the internal requires to be relative, and also introduce some hacks on our specs to make sure we never load the incorrect copy of bundler.

I think this PR should fix the issues in rubygems/rubygems#2863.


Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
@ghost
Copy link

ghost commented Aug 18, 2019

Build succeeded

@ghost ghost merged commit 8ef571e into master Aug 18, 2019
3 checks passed
@ghost ghost deleted the fix_more_leaks_to_default_copy_of_bundler branch Aug 18, 2019
ghost pushed a commit to rubygems/rubygems that referenced this issue Mar 11, 2020
7301: Track changes from ruby core master r=hsbt a=hsbt

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

I'm going to merge rubygems/bundler#7274. But the ruby-core source has some of the changes for bundler source.

### What was your diagnosis of the problem?


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

ruby core team fixed them:

* Removed circular require warning at `shared_helper.rb`
* Support test at GitHub Actions, It helps that bundler will migrate Actions from Azure Pipelines too.
* Fixed broken examples at ruby core repository


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



Co-authored-by: ohbarye <over.rye@gmail.com>
Co-authored-by: Hiroshi SHIBATA <hsbt@ruby-lang.org>
Co-authored-by: Yusuke Endoh <mame@ruby-lang.org>
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

2 participants